fix #2537: sort map keys to fix non-determinism

CHANGELOG.md

@@ -41,6 +41,58 @@
 
     These seem to be used by Yarn to avoid the `.` and `..` path segments in the middle of relative paths. The removal of these character sequences seems relatively harmless in this case since esbuild shouldn't ever generate such path segments. This change should add support to esbuild for Yarn's [`pnpIgnorePatterns`](https://yarnpkg.com/configuration/yarnrc/#pnpIgnorePatterns) feature.
 
+* Fix non-determinism issue with legacy block-level function declarations and strict mode ([#2537](https://github.com/evanw/esbuild/issues/2537))
+
+    When function declaration statements are nested inside a block in strict mode, they are supposed to only be available within that block's scope. But in "sloppy mode" (which is what non-strict mode is commonly called), they are supposed to be available within the whole function's scope:
+
+    ```js
+    // This returns 1 due to strict mode
+    function test1() {
+      'use strict'
+      function fn() { return 1 }
+      if (true) { function fn() { return 2 } }
+      return fn()
+    }
+
+    // This returns 2 due to sloppy mode
+    function test2() {
+      function fn() { return 1 }
+      if (true) { function fn() { return 2 } }
+      return fn()
+    }
+    ```
+
+    To implement this, esbuild compiles these two functions differently to reflect their different semantics:
+
+    ```js
+    function test1() {
+      "use strict";
+      function fn() {
+        return 1;
+      }
+      if (true) {
+        let fn2 = function() {
+          return 2;
+        };
+      }
+      return fn();
+    }
+    function test2() {
+      function fn() {
+        return 1;
+      }
+      if (true) {
+        let fn2 = function() {
+          return 2;
+        };
+        var fn = fn2;
+      }
+      return fn();
+    }
+    ```
+
+    However, the compilation had a subtle bug where the automatically-generated function-level symbols for multible hoisted block-level function declarations in the same block a sloppy-mode context were generated in a random order if the output was in strict mode, which could be the case if TypeScript's `alwaysStrict` setting was set to true. This lead to non-determinism in the output as the minifier would randomly exchange the generated names for these symbols on different runs. This bug has been fixed by sorting the keys of the unordered map before iterating over them.
+
 ## 0.15.7
 
 * Add `--watch=forever` to allow esbuild to never terminate ([#1511](https://github.com/evanw/esbuild/issues/1511), [#1885](https://github.com/evanw/esbuild/issues/1885))

internal/bundler/bundler.go

@@ -1228,8 +1228,12 @@ func (s *scanner) maybeParseFile(
 	if resolveResult.UnusedImportFlagsTS != 0 {
 		optionsClone.UnusedImportFlagsTS = resolveResult.UnusedImportFlagsTS
 	}
-	optionsClone.TSTarget = resolveResult.TSTarget
-	optionsClone.TSAlwaysStrict = resolveResult.TSAlwaysStrict
+	if resolveResult.TSTarget != nil {
+		optionsClone.TSTarget = resolveResult.TSTarget
+	}
+	if resolveResult.TSAlwaysStrict != nil {
+		optionsClone.TSAlwaysStrict = resolveResult.TSAlwaysStrict
+	}
 
 	// Set the module type preference using node's module type rules
 	if strings.HasSuffix(path.Text, ".mjs") {

internal/bundler/bundler_default_test.go

@@ -6796,3 +6796,39 @@ yes4.js: NOTE: This file is considered to be an ECMAScript module because of the
 `,
 	})
 }
+
+// See: https://github.com/evanw/esbuild/issues/2537
+func TestNonDeterminismIssue2537(t *testing.T) {
+	loader_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.ts": `
+				export function aap(noot: boolean, wim: number) {
+					let mies = "teun"
+					if (noot) {
+						function vuur(v: number) {
+							return v * 2
+						}
+						function schaap(s: number) {
+							return s / 2
+						}
+						mies = vuur(wim) + schaap(wim)
+					}
+					return mies
+				}
+			`,
+			"/tsconfig.json": `
+				{
+					"compilerOptions": {
+						"alwaysStrict": true
+					}
+				}
+			`,
+		},
+		entryPaths: []string{"/entry.ts"},
+		options: config.Options{
+			Mode:              config.ModeBundle,
+			AbsOutputFile:     "/out.js",
+			MinifyIdentifiers: true,
+		},
+	})
+}

internal/bundler/snapshots/snapshots_loader.txt

@@ -957,6 +957,27 @@ module.exports = foo;
 ---------- /out/no-warnings-here.js ----------
 console.log(module, exports);
 
+================================================================================
+TestNonDeterminismIssue2537
+---------- /out.js ----------
+// entry.ts
+function b(o, e) {
+  let r = "teun";
+  if (o) {
+    let u = function(n) {
+      return n * 2;
+    }, t = function(n) {
+      return n / 2;
+    };
+    var i = u, a = t;
+    r = u(e) + t(e);
+  }
+  return r;
+}
+export {
+  b as aap
+};
+
 ================================================================================
 TestRequireCustomExtensionBase64
 ---------- /out.js ----------

internal/js_parser/js_parser.go

@@ -1305,10 +1305,30 @@ func (p *parser) declareSymbol(kind js_ast.SymbolKind, loc logger.Loc, name stri
 
 }
 
+// This type is just so we can use Go's native sort function
+type scopeMemberArray []js_ast.ScopeMember
+
+func (a scopeMemberArray) Len() int          { return len(a) }
+func (a scopeMemberArray) Swap(i int, j int) { a[i], a[j] = a[j], a[i] }
+
+func (a scopeMemberArray) Less(i int, j int) bool {
+	ai := a[i].Ref
+	bj := a[j].Ref
+	return ai.InnerIndex < bj.InnerIndex || (ai.InnerIndex == bj.InnerIndex && ai.SourceIndex < bj.SourceIndex)
+}
+
 func (p *parser) hoistSymbols(scope *js_ast.Scope) {
 	if !scope.Kind.StopsHoisting() {
-	nextMember:
+		// We create new symbols in the loop below, so the iteration order of the
+		// loop must be deterministic to avoid generating different minified names
+		sortedMembers := make(scopeMemberArray, 0, len(scope.Members))
 		for _, member := range scope.Members {
+			sortedMembers = append(sortedMembers, member)
+		}
+		sort.Sort(sortedMembers)
+
+	nextMember:
+		for _, member := range sortedMembers {
 			symbol := &p.symbols[member.Ref.InnerIndex]
 
 			// Handle non-hoisted collisions between catch bindings and the catch body.

internal/resolver/resolver.go

@@ -681,12 +681,7 @@ func (r resolverQuery) finalizeResolve(result *ResolveResult) {
 							dirInfo.enclosingTSConfigJSON.PreserveValueImports,
 						)
 						result.TSTarget = dirInfo.enclosingTSConfigJSON.TSTarget
-						if tsAlwaysStrict := dirInfo.enclosingTSConfigJSON.TSAlwaysStrict; tsAlwaysStrict != nil {
-							result.TSAlwaysStrict = tsAlwaysStrict
-						} else {
-							// If "alwaysStrict" is absent, it defaults to "strict" instead
-							result.TSAlwaysStrict = dirInfo.enclosingTSConfigJSON.TSStrict
-						}
+						result.TSAlwaysStrict = dirInfo.enclosingTSConfigJSON.TSAlwaysStrictOrStrict()
 
 						if r.debugLogs != nil {
 							r.debugLogs.addNote(fmt.Sprintf("This import is under the effect of %q",

internal/resolver/tsconfig_json.go

@@ -48,6 +48,15 @@ type TSConfigJSON struct {
 	PreserveValueImports           bool
 }
 
+func (config *TSConfigJSON) TSAlwaysStrictOrStrict() *config.TSAlwaysStrict {
+	if config.TSAlwaysStrict != nil {
+		return config.TSAlwaysStrict
+	}
+
+	// If "alwaysStrict" is absent, it defaults to "strict" instead
+	return config.TSStrict
+}
+
 type TSConfigPath struct {
 	Text string
 	Loc  logger.Loc
@@ -242,7 +251,7 @@ func ParseTSConfigJSON(
 		if valueJSON, keyLoc, ok := getProperty(compilerOptionsJSON, "alwaysStrict"); ok {
 			if value, ok := getBool(valueJSON); ok {
 				valueRange := js_lexer.RangeOfIdentifier(source, valueJSON.Loc)
-				result.TSStrict = &config.TSAlwaysStrict{
+				result.TSAlwaysStrict = &config.TSAlwaysStrict{
 					Name:   "alwaysStrict",
 					Value:  value,
 					Source: source,

pkg/api/api_impl.go

@@ -1398,7 +1398,7 @@ func transformImpl(input string, transformOpts TransformOptions) TransformResult
 				result.PreserveValueImports,
 			)
 			tsTarget = result.TSTarget
-			tsAlwaysStrict = result.TSAlwaysStrict
+			tsAlwaysStrict = result.TSAlwaysStrictOrStrict()
 		}
 	}
 
@@ -1879,6 +1879,7 @@ type metafileEntry struct {
 	size       int
 }
 
+// This type is just so we can use Go's native sort function
 type metafileArray []metafileEntry
 
 func (a metafileArray) Len() int          { return len(a) }

scripts/js-api-tests.js

@@ -4127,6 +4127,18 @@ let transformTests = {
     assert.strictEqual(code2, `class Foo {\n  foo;\n}\n`)
   },
 
+  async tsconfigRawAlwaysStrict({ esbuild }) {
+    const input = `console.log(123)`
+
+    assert.strictEqual((await esbuild.transform(input, { loader: 'ts', tsconfigRaw: { compilerOptions: { strict: false } } })).code, `console.log(123);\n`)
+    assert.strictEqual((await esbuild.transform(input, { loader: 'ts', tsconfigRaw: { compilerOptions: { alwaysStrict: false } } })).code, `console.log(123);\n`)
+    assert.strictEqual((await esbuild.transform(input, { loader: 'ts', tsconfigRaw: { compilerOptions: { alwaysStrict: false, strict: true } } })).code, `console.log(123);\n`)
+
+    assert.strictEqual((await esbuild.transform(input, { loader: 'ts', tsconfigRaw: { compilerOptions: { strict: true } } })).code, `"use strict";\nconsole.log(123);\n`)
+    assert.strictEqual((await esbuild.transform(input, { loader: 'ts', tsconfigRaw: { compilerOptions: { alwaysStrict: true } } })).code, `"use strict";\nconsole.log(123);\n`)
+    assert.strictEqual((await esbuild.transform(input, { loader: 'ts', tsconfigRaw: { compilerOptions: { alwaysStrict: true, strict: false } } })).code, `"use strict";\nconsole.log(123);\n`)
+  },
+
   async tsImplicitUseDefineForClassFields({ esbuild }) {
     var { code } = await esbuild.transform(`class Foo { foo }`, {
       loader: 'ts',