Fix dependency retargeting with ambiguous reexports (#9380)

Author: mischnic

packages/core/core/src/BundleGraph.js

@@ -295,32 +295,44 @@ export default class BundleGraph {
                   symbols.set(from, existing);
                 } else {
                   invariant(isReexportAll);
-                  let local = `${node.value.id}$rewrite$${asset}$${from}`;
-                  symbols.set(from, {
-                    isWeak: true,
-                    local,
-                    loc: reexportAllLoc,
-                  });
-                  // It might already exist with multiple export-alls causing ambiguous resolution
-                  if (
-                    node.value.sourceAssetId != null &&
-                    assetGraph.hasContentKey(node.value.sourceAssetId)
-                  ) {
-                    let sourceAssetId = nullthrows(
-                      assetGraphNodeIdToBundleGraphNodeId.get(
-                        assetGraph.getNodeIdByContentKey(
-                          nullthrows(node.value.sourceAssetId),
+                  if (as === from) {
+                    // Keep the export-all for non-renamed reexports, this still correctly models
+                    // ambiguous resolution with multiple export-alls.
+                    symbols.set('*', {
+                      isWeak: true,
+                      local: '*',
+                      loc: reexportAllLoc,
+                    });
+                  } else {
+                    let local = `${node.value.id}$rewrite$${asset}$${from}`;
+                    symbols.set(from, {
+                      isWeak: true,
+                      local,
+                      loc: reexportAllLoc,
+                    });
+                    if (node.value.sourceAssetId != null) {
+                      let sourceAssetId = nullthrows(
+                        assetGraphNodeIdToBundleGraphNodeId.get(
+                          assetGraph.getNodeIdByContentKey(
+                            node.value.sourceAssetId,
+                          ),
                         ),
-                      ),
-                    );
-                    let sourceAsset = nullthrows(graph.getNode(sourceAssetId));
-                    invariant(sourceAsset.type === 'asset');
-                    let sourceAssetSymbols = sourceAsset.value.symbols;
-                    if (sourceAssetSymbols && !sourceAssetSymbols.has(as)) {
-                      sourceAssetSymbols.set(as, {
-                        loc: reexportAllLoc,
-                        local: local,
-                      });
+                      );
+                      let sourceAsset = nullthrows(
+                        graph.getNode(sourceAssetId),
+                      );
+                      invariant(sourceAsset.type === 'asset');
+                      let sourceAssetSymbols = sourceAsset.value.symbols;
+                      if (sourceAssetSymbols) {
+                        // The `as == from` case above should handle multiple export-alls causing
+                        // ambiguous resolution. So the current symbol is unambiguous and shouldn't
+                        // already exist on the importer.
+                        invariant(!sourceAssetSymbols.has(as));
+                        sourceAssetSymbols.set(as, {
+                          loc: reexportAllLoc,
+                          local: local,
+                        });
+                      }
                     }
                   }
                 }
@@ -342,7 +354,7 @@ export default class BundleGraph {
                   },
                   usedSymbolsUp,
                   // This is only a temporary helper needed during symbol propagation and is never
-                  // read afterwards.
+                  // read afterwards (and also not exposed through the public API).
                   usedSymbolsDown: new Set(),
                 }),
               };

packages/core/integration-tests/test/integration/scope-hoisting/es6/re-export-all-ambiguous/entry.js

@@ -0,0 +1,3 @@
+if (Date.now() > 0) {
+	output = require("./index.js").default;
+}

packages/core/integration-tests/test/integration/scope-hoisting/es6/re-export-all-ambiguous/index.js

@@ -0,0 +1,3 @@
+import { b,c } from "./lib";
+
+export default b + " " + c;

packages/core/integration-tests/test/integration/scope-hoisting/es6/re-export-all-ambiguous/lib-a.js

@@ -0,0 +1,2 @@
+module.exports = {};
+// export const a = 89;

packages/core/integration-tests/test/integration/scope-hoisting/es6/re-export-all-ambiguous/lib-b.js

@@ -0,0 +1 @@
+export const b = 123;

packages/core/integration-tests/test/integration/scope-hoisting/es6/re-export-all-ambiguous/lib-c.js

@@ -0,0 +1 @@
+export { c2 as c } from "./lib-c2.js";

packages/core/integration-tests/test/integration/scope-hoisting/es6/re-export-all-ambiguous/lib-c2.js

@@ -0,0 +1 @@
+export const c2 = 999;

packages/core/integration-tests/test/integration/scope-hoisting/es6/re-export-all-ambiguous/lib.js

@@ -0,0 +1,3 @@
+export * from "./lib-a";
+export * from "./lib-b";
+export * from "./lib-c";

packages/core/integration-tests/test/integration/scope-hoisting/es6/re-export-all-ambiguous/package.json

@@ -0,0 +1,10 @@
+{
+	"sideEffects": [
+		"entry.js",
+		"index.js",
+		"lib-a.js",
+		"lib-b.js",
+		"lib-c2.js",
+		"lib.js"
+	]
+}

packages/core/integration-tests/test/scope-hoisting.js

@@ -148,7 +148,7 @@ describe('scope hoisting', function () {
       assert.equal(output, 2);
     });
 
-    it('supports import * as from a library that has export *', async function () {
+    it('supports dependency rewriting for import * as from a library that has export *', async function () {
       let b = await bundle(
         path.join(
           __dirname,
@@ -403,6 +403,21 @@ describe('scope hoisting', function () {
       assert.match(contents, /output="foo bar"/);
     });
 
+    it('supports re-exporting all with ambiguous CJS and non-renaming and renaming dependency retargeting', async function () {
+      let b = await bundle(
+        path.join(
+          __dirname,
+          '/integration/scope-hoisting/es6/re-export-all-ambiguous/entry.js',
+        ),
+        {
+          mode: 'production',
+        },
+      );
+
+      let output = await run(b);
+      assert.strictEqual(output, '123 999');
+    });
+
     it('supports re-exporting all exports from an external module', async function () {
       let b = await bundle(
         path.join(