fix #3544: hack around node cjs export name bugs

CHANGELOG.md

@@ -2,6 +2,35 @@
 
 ## Unreleased
 
+* Work around a bug in node's CommonJS export name detector ([#3544](https://github.com/evanw/esbuild/issues/3544))
+
+    The export names of a CommonJS module are dynamically-determined at run time because CommonJS exports are properties on a mutable object. But the export names of an ES module are statically-determined at module instantiation time by using `import` and `export` syntax and cannot be changed at run time.
+
+    When you import a CommonJS module into an ES module in node, node scans over the source code to attempt to detect the set of export names that the CommonJS module will end up using. That statically-determined set of names is used as the set of names that the ES module is allowed to import at module instantiation time. However, this scan appears to have bugs (or at least, can cause false positives) because it doesn't appear to do any scope analysis. Node will incorrectly consider the module to export something even if the assignment is done to a local variable instead of to the module-level `exports` object. For example:
+
+    ```js
+    // confuseNode.js
+    exports.confuseNode = function(exports) {
+      // If this local is called "exports", node incorrectly
+      // thinks this file has an export called "notAnExport".
+      exports.notAnExport = function() {
+      };
+    };
+    ```
+
+    You can see that node incorrectly thinks the file `confuseNode.js` has an export called `notAnExport` when that file is loaded in an ES module context:
+
+    ```console
+    $ node -e 'import("./confuseNode.js").then(console.log)'
+    [Module: null prototype] {
+      confuseNode: [Function (anonymous)],
+      default: { confuseNode: [Function (anonymous)] },
+      notAnExport: undefined
+    }
+    ```
+
+    To avoid this, esbuild will now rename local variables that use the names `exports` and `module` when generating CommonJS output for the `node` platform.
+
 * Fix the return value of esbuild's `super()` shim ([#3538](https://github.com/evanw/esbuild/issues/3538))
 
     Some people write `constructor` methods that use the return value of `super()` instead of using `this`. This isn't too common because [TypeScript doesn't let you do that](https://github.com/microsoft/TypeScript/issues/37847) but it can come up when writing JavaScript. Previously esbuild's class lowering transform incorrectly transformed the return value of `super()` into `undefined`. With this release, the return value of `super()` will now be `this` instead:

internal/bundler_tests/bundler_default_test.go

@@ -1673,6 +1673,31 @@ func TestExportWildcardFSNodeCommonJS(t *testing.T) {
 	})
 }
 
+// https://github.com/evanw/esbuild/issues/3544
+func TestNodeAnnotationFalsePositiveIssue3544(t *testing.T) {
+	default_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.mjs": `
+				export function confuseNode(exports) {
+					// If this local is called "exports", node incorrectly
+					// thinks this file has an export called "notAnExport".
+					// We must make sure that it doesn't have that name
+					// when targeting Node with CommonJS.
+					exports.notAnExport = function() {
+					};
+				}
+			`,
+		},
+		entryPaths: []string{"/entry.mjs"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			OutputFormat:  config.FormatCommonJS,
+			AbsOutputFile: "/out.js",
+			Platform:      config.PlatformNode,
+		},
+	})
+}
+
 func TestMinifiedBundleES6(t *testing.T) {
 	default_suite.expectBundled(t, bundled{
 		files: map[string]string{

internal/bundler_tests/snapshots/snapshots_default.txt

@@ -5120,6 +5120,24 @@ module.exports = foo;
 ---------- /out/no-warnings-here.js ----------
 console.log(module, exports);
 
+================================================================================
+TestNodeAnnotationFalsePositiveIssue3544
+---------- /out.js ----------
+// entry.mjs
+var entry_exports = {};
+__export(entry_exports, {
+  confuseNode: () => confuseNode
+});
+module.exports = __toCommonJS(entry_exports);
+function confuseNode(exports2) {
+  exports2.notAnExport = function() {
+  };
+}
+// Annotate the CommonJS export names for ESM import in node:
+0 && (module.exports = {
+  confuseNode
+});
+
 ================================================================================
 TestNodeModules
 ---------- /Users/user/project/out.js ----------

internal/linker/linker.go

@@ -5270,6 +5270,19 @@ func (c *linkerContext) renameSymbolsInChunk(chunk *chunkInfo, filesInOrder []ui
 	}
 	reservedNames := renamer.ComputeReservedNames(moduleScopes, c.graph.Symbols)
 
+	// Node contains code that scans CommonJS modules in an attempt to statically
+	// detect the  set of export names that a module will use. However, it doesn't
+	// do any scope analysis so it can be fooled by local variables with the same
+	// name as the CommonJS module-scope variables "exports" and "module". Avoid
+	// using these names in this case even if there is not a risk of a name
+	// collision because there is still a risk of node incorrectly detecting
+	// something in a nested scope as an top-level export. Here's a case where
+	// this happened: https://github.com/evanw/esbuild/issues/3544
+	if c.options.OutputFormat == config.FormatCommonJS && c.options.Platform == config.PlatformNode {
+		reservedNames["exports"] = 1
+		reservedNames["module"] = 1
+	}
+
 	// These are used to implement bundling, and need to be free for use
 	if c.options.Mode != config.ModePassThrough {
 		reservedNames["require"] = 1

scripts/end-to-end-tests.js

@@ -2109,6 +2109,26 @@ tests.push(
     'foo.js': `export default 123`,
     'node.js': `import out from './out.js'; if (out !== 123) throw 'fail'`,
   }),
+  test(['--bundle', 'foo.js', '--outfile=out.js', '--format=cjs', '--platform=node'], {
+    'foo.js': `
+      export function confuseNode(exports) {
+        // If this local is called "exports", node incorrectly
+        // thinks this file has an export called "notAnExport".
+        // We must make sure that it doesn't have that name
+        // when targeting Node with CommonJS. See also:
+        // https://github.com/evanw/esbuild/issues/3544
+        exports.notAnExport = function() {
+        };
+      }
+    `,
+    'node.js': `
+      exports.async = async () => {
+        const foo = await import('./out.js')
+        if (typeof foo.confuseNode !== 'function') throw 'fail: confuseNode'
+        if ('notAnExport' in foo) throw 'fail: notAnExport'
+      }
+    `,
+  }, { async: true }),
 
   // External package
   test(['--bundle', 'foo.js', '--outfile=out.js', '--format=cjs', '--external:fs'], {