fix lexically-declared names in strict mode

Author: evanw

CHANGELOG.md

@@ -6,9 +6,9 @@
 
     The character tables that determine which characters form valid JavaScript identifiers have been updated from Unicode version 14.0.0 to the newly-released Unicode version 15.0.0. I'm not putting an example in the release notes because all of the new characters will likely just show up as little squares since fonts haven't been updated yet. But you can read https://www.unicode.org/versions/Unicode15.0.0/#Summary for more information about the changes.
 
-* Disallow duplicate lexically-declared names in nested blocks
+* Disallow duplicate lexically-declared names in nested blocks and in strict mode
 
-    It's supposed to be a syntax error for a nested block to declare two symbols with the same name unless all duplicate entries are either `function` declarations or all `var` declarations. However, esbuild was overly permissive and allowed this when duplicate entries were either `function` declarations or `var` declarations (even if they were mixed). This check has now been made more restrictive to match the JavaScript specification:
+    In strict mode or in a nested block, it's supposed to be a syntax error to declare two symbols with the same name unless all duplicate entries are either `function` declarations or all `var` declarations. However, esbuild was overly permissive and allowed this when duplicate entries were either `function` declarations or `var` declarations (even if they were mixed). This check has now been made more restrictive to match the JavaScript specification:
 
     ```js
     // JavaScript allows this

internal/js_ast/js_ast.go

@@ -1548,6 +1548,7 @@ type Scope struct {
 	Parent    *Scope
 	Children  []*Scope
 	Members   map[string]ScopeMember
+	Replaced  []ScopeMember
 	Generated []Ref
 
 	// The location of the "use strict" directive for ExplicitStrictMode

internal/js_parser/js_parser.go

@@ -1231,6 +1231,7 @@ func (p *parser) declareSymbol(kind js_ast.SymbolKind, loc logger.Loc, name stri
 
 		case mergeReplaceWithNew:
 			symbol.Link = ref
+			p.currentScope.Replaced = append(p.currentScope.Replaced, existing)
 
 			// If these are both functions, remove the overwritten declaration
 			if p.options.minifySyntax && kind.IsFunction() && symbol.Kind.IsFunction() {
@@ -1268,6 +1269,28 @@ func (a scopeMemberArray) Less(i int, j int) bool {
 }
 
 func (p *parser) hoistSymbols(scope *js_ast.Scope) {
+	if scope.StrictMode != js_ast.SloppyMode {
+		for _, replaced := range scope.Replaced {
+			symbol := &p.symbols[replaced.Ref.InnerIndex]
+			if symbol.Kind.IsFunction() {
+				if member, ok := scope.Members[symbol.OriginalName]; ok && p.symbols[member.Ref.InnerIndex].Kind.IsFunction() {
+					where, notes := p.whyStrictMode(scope)
+					notes[0].Text = fmt.Sprintf("Duplicate lexically-declared names are not allowed %s. %s", where, notes[0].Text)
+
+					p.log.AddErrorWithNotes(&p.tracker,
+						js_lexer.RangeOfIdentifier(p.source, member.Loc),
+						fmt.Sprintf("The symbol %q has already been declared", symbol.OriginalName),
+
+						append([]logger.MsgData{p.tracker.MsgData(
+							js_lexer.RangeOfIdentifier(p.source, replaced.Loc),
+							fmt.Sprintf("The symbol %q was originally declared here:", symbol.OriginalName),
+						)}, notes...),
+					)
+				}
+			}
+		}
+	}
+
 	if !scope.Kind.StopsHoisting() {
 		// We create new symbols in the loop below, so the iteration order of the
 		// loop must be deterministic to avoid generating different minified names

internal/js_parser/js_parser_lower.go

@@ -170,6 +170,7 @@ const (
 	legacyOctalEscape
 	ifElseFunctionStmt
 	labelFunctionStmt
+	duplicateLexicallyDeclaredNames
 )
 
 func (p *parser) markStrictModeFeature(feature strictModeFeature, r logger.Range, detail string) {
@@ -205,47 +206,54 @@ func (p *parser) markStrictModeFeature(feature strictModeFeature, r logger.Range
 	case labelFunctionStmt:
 		text = "Function declarations inside labels"
 
+	case duplicateLexicallyDeclaredNames:
+		text = "Duplicate lexically-declared names"
+
 	default:
 		text = "This feature"
 	}
 
 	if p.isStrictMode() {
-		var notes []logger.MsgData
-		where := "in strict mode"
+		where, notes := p.whyStrictMode(p.currentScope)
+		p.log.AddErrorWithNotes(&p.tracker, r,
+			fmt.Sprintf("%s cannot be used %s", text, where), notes)
+	} else if !canBeTransformed && p.isStrictModeOutputFormat() {
+		p.log.AddError(&p.tracker, r,
+			fmt.Sprintf("%s cannot be used with the \"esm\" output format due to strict mode", text))
+	}
+}
 
-		switch p.currentScope.StrictMode {
-		case js_ast.ImplicitStrictModeClass:
-			notes = []logger.MsgData{p.tracker.MsgData(p.enclosingClassKeyword,
-				"All code inside a class is implicitly in strict mode")}
+func (p *parser) whyStrictMode(scope *js_ast.Scope) (where string, notes []logger.MsgData) {
+	where = "in strict mode"
 
-		case js_ast.ImplicitStrictModeTSAlwaysStrict:
-			tsAlwaysStrict := p.options.tsAlwaysStrict
-			t := logger.MakeLineColumnTracker(&tsAlwaysStrict.Source)
-			notes = []logger.MsgData{t.MsgData(tsAlwaysStrict.Range, fmt.Sprintf(
-				"TypeScript's %q setting was enabled here:", tsAlwaysStrict.Name))}
+	switch scope.StrictMode {
+	case js_ast.ImplicitStrictModeClass:
+		notes = []logger.MsgData{p.tracker.MsgData(p.enclosingClassKeyword,
+			"All code inside a class is implicitly in strict mode")}
 
-		case js_ast.ImplicitStrictModeJSXAutomaticRuntime:
-			notes = []logger.MsgData{p.tracker.MsgData(logger.Range{Loc: p.firstJSXElementLoc, Len: 1},
-				"This file is implicitly in strict mode due to the JSX element here:"),
-				{Text: "When React's \"automatic\" JSX transform is enabled, using a JSX element automatically inserts " +
-					"an \"import\" statement at the top of the file for the corresponding the JSX helper function. " +
-					"This means the file is considered an ECMAScript module, and all ECMAScript modules use strict mode."}}
+	case js_ast.ImplicitStrictModeTSAlwaysStrict:
+		tsAlwaysStrict := p.options.tsAlwaysStrict
+		t := logger.MakeLineColumnTracker(&tsAlwaysStrict.Source)
+		notes = []logger.MsgData{t.MsgData(tsAlwaysStrict.Range, fmt.Sprintf(
+			"TypeScript's %q setting was enabled here:", tsAlwaysStrict.Name))}
 
-		case js_ast.ExplicitStrictMode:
-			notes = []logger.MsgData{p.tracker.MsgData(p.source.RangeOfString(p.currentScope.UseStrictLoc),
-				"Strict mode is triggered by the \"use strict\" directive here:")}
+	case js_ast.ImplicitStrictModeJSXAutomaticRuntime:
+		notes = []logger.MsgData{p.tracker.MsgData(logger.Range{Loc: p.firstJSXElementLoc, Len: 1},
+			"This file is implicitly in strict mode due to the JSX element here:"),
+			{Text: "When React's \"automatic\" JSX transform is enabled, using a JSX element automatically inserts " +
+				"an \"import\" statement at the top of the file for the corresponding the JSX helper function. " +
+				"This means the file is considered an ECMAScript module, and all ECMAScript modules use strict mode."}}
 
-		case js_ast.ImplicitStrictModeESM:
-			_, notes = p.whyESModule()
-			where = "in an ECMAScript module"
-		}
+	case js_ast.ExplicitStrictMode:
+		notes = []logger.MsgData{p.tracker.MsgData(p.source.RangeOfString(scope.UseStrictLoc),
+			"Strict mode is triggered by the \"use strict\" directive here:")}
 
-		p.log.AddErrorWithNotes(&p.tracker, r,
-			fmt.Sprintf("%s cannot be used %s", text, where), notes)
-	} else if !canBeTransformed && p.isStrictModeOutputFormat() {
-		p.log.AddError(&p.tracker, r,
-			fmt.Sprintf("%s cannot be used with the \"esm\" output format due to strict mode", text))
+	case js_ast.ImplicitStrictModeESM:
+		_, notes = p.whyESModule()
+		where = "in an ECMAScript module"
 	}
+
+	return
 }
 
 func (p *parser) markAsyncFn(asyncRange logger.Range, isGenerator bool) (didGenerateError bool) {

internal/js_parser/js_parser_test.go

@@ -533,6 +533,40 @@ func TestStrictMode(t *testing.T) {
 	expectParseError(t, "with (y) z; await 0", tlaKeyword)
 	expectParseError(t, "for await (x of y); with (y) z", tlaKeyword)
 	expectParseError(t, "with (y) z; for await (x of y);", tlaKeyword)
+
+	cases := []string{
+		"function f() {} function f() {}",
+		"function f() {} function *f() {}",
+		"function *f() {} function f() {}",
+		"function f() {} async function f() {}",
+		"async function f() {} function f() {}",
+		"function f() {} async function *f() {}",
+		"async function *f() {} function f() {}",
+		"{ function f() {} function f() {} }",
+		"switch (0) { case 1: function f() {} default: function f() {} }",
+	}
+
+	fAlreadyDeclaredError :=
+		"<stdin>: ERROR: The symbol \"f\" has already been declared\n" +
+			"<stdin>: NOTE: The symbol \"f\" was originally declared here:\n" +
+			"<stdin>: NOTE: Duplicate lexically-declared names are not allowed"
+
+	for _, c := range cases {
+		expectParseError(t, c, "")
+
+		expectParseError(t, "'use strict'; "+c, fAlreadyDeclaredError+" in strict mode. "+
+			"Strict mode is triggered by the \"use strict\" directive here:\n")
+
+		expectParseError(t, "function foo() { 'use strict'; "+c+" }", fAlreadyDeclaredError+" in strict mode. "+
+			"Strict mode is triggered by the \"use strict\" directive here:\n")
+
+		expectParseError(t, c+" export {}", fAlreadyDeclaredError+" in an ECMAScript module. "+
+			"This file is considered to be an ECMAScript module because of the \"export\" keyword here:\n")
+	}
+
+	expectParseError(t, "var x; var x", "")
+	expectParseError(t, "'use strict'; var x; var x", "")
+	expectParseError(t, "var x; var x; export {}", "")
 }
 
 func TestExponentiation(t *testing.T) {
@@ -1445,7 +1479,6 @@ func TestFunction(t *testing.T) {
 	expectPrintedMangle(t, "var f; function f() { x() } function f() { y() }", "var f;\nfunction f() {\n  y();\n}\n")
 	expectPrintedMangle(t, "function f() { x() } function f() { y() } var f", "function f() {\n  y();\n}\nvar f;\n")
 	expectPrintedMangle(t, "function f() { x() } var f; function f() { y() }", "function f() {\n  x();\n}\nvar f;\nfunction f() {\n  y();\n}\n")
-	expectPrintedMangle(t, "export function f() { x() } function f() { y() }", "export function f() {\n  x();\n}\nfunction f() {\n  y();\n}\n")
 
 	redeclaredError := "<stdin>: ERROR: The symbol \"f\" has already been declared\n" +
 		"<stdin>: NOTE: The symbol \"f\" was originally declared here:\n"