metro-transform-plugins: Avoid generating duplicate references to the…

… same node Summary: Babel does not deal well with multiple references to the same node in a single AST. Specifically it can lead to `NodePath`s not being unique and therefore to incorrect scope analyses - see babel/babel#6375 for pointers to manifestations of this in Babel. These days, Babel's test suite ensures that Babel's first-party plugins do not emit such duplicates (babel/babel#7149) but there is no such guarantee for third-party plugins. This diff: 1. fixes the logic in `import-export-plugin` to avoid emitting repeated nodes. 2. adds tests to *all* the Babel plugins in `metro-transform-plugins` asserting that there are no repeated nodes in the resulting ASTs. See also the similar facebook/fbjs#440 Reviewed By: GijsWeterings Differential Revision: D28646299 fbshipit-source-id: 9cceed06cad1d21214f47ecaaaab2d12e6ba031c
facebook · Jun 3, 2021 · 4fea2bd · 4fea2bd
1 parent c955c50
commit 4fea2bd
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 20 deletions.
diff --git a/packages/metro-transform-plugins/package.json b/packages/metro-transform-plugins/package.json
@@ -23,6 +23,7 @@
     "@babel/code-frame": "^7.0.0",
     "@babel/plugin-syntax-nullish-coalescing-operator": "^7.0.0",
     "@babel/plugin-transform-flow-strip-types": "^7.0.0",
+    "@babel/types": "^7.0.0",
     "metro": "0.66.0"
   }
 }
diff --git a/packages/metro-transform-plugins/src/__mocks__/test-helpers.js b/packages/metro-transform-plugins/src/__mocks__/test-helpers.js
@@ -11,6 +11,8 @@
 'use strict';
 
 const generate = require('@babel/generator').default;
+const t = require('@babel/types');
+
 const {transformSync} = require('@babel/core');
 
 opaque type Code = string;
@@ -32,12 +34,29 @@ function makeTransformOptions(plugins, options) {
   };
 }
 
+function validateOutputAst(ast) {
+  const seenNodes = new Set();
+  t.traverseFast(ast, function enter(node) {
+    if (seenNodes.has(node)) {
+      throw new Error(
+        'Found a duplicate ' +
+          node.type +
+          ' node in the output, which can cause' +
+          ' undefined behavior in Babel.',
+      );
+    }
+    seenNodes.add(node);
+  });
+}
+
 function transformToAst(
   plugins: $ReadOnlyArray<Plugin>,
   code: Code,
   options: Options = {},
 ) {
-  return transformSync(code, makeTransformOptions(plugins, options)).ast;
+  const ast = transformSync(code, makeTransformOptions(plugins, options)).ast;
+  validateOutputAst(ast);
+  return ast;
 }
 
 function transform(

diff --git a/packages/metro-transform-plugins/src/import-export-plugin.js b/packages/metro-transform-plugins/src/import-export-plugin.js
@@ -275,9 +275,9 @@ function importExportPlugin({types: t}: {types: Types, ...}): PluginObj<State> {
                 path.insertBefore(
                   withLocation(
                     importTemplate({
-                      IMPORT: state.importDefault,
+                      IMPORT: t.cloneNode(state.importDefault),
                       FILE: resolvePath(
-                        nullthrows(path.node.source),
+                        t.cloneNode(nullthrows(path.node.source)),
                         state.opts.resolve,
                       ),
                       LOCAL: temp,
@@ -296,7 +296,7 @@ function importExportPlugin({types: t}: {types: Types, ...}): PluginObj<State> {
                   withLocation(
                     importNamedTemplate({
                       FILE: resolvePath(
-                        nullthrows(path.node.source),
+                        t.cloneNode(nullthrows(path.node.source)),
                         state.opts.resolve,
                       ),
                       LOCAL: temp,
@@ -312,7 +312,7 @@ function importExportPlugin({types: t}: {types: Types, ...}): PluginObj<State> {
                   withLocation(
                     importNamedTemplate({
                       FILE: resolvePath(
-                        nullthrows(path.node.source),
+                        t.cloneNode(nullthrows(path.node.source)),
                         state.opts.resolve,
                       ),
                       LOCAL: temp,
@@ -358,7 +358,7 @@ function importExportPlugin({types: t}: {types: Types, ...}): PluginObj<State> {
           state.imports.push({
             node: withLocation(
               importSideEffectTemplate({
-                FILE: resolvePath(file, state.opts.resolve),
+                FILE: resolvePath(t.cloneNode(file), state.opts.resolve),
               }),
               loc,
             ),
@@ -380,7 +380,7 @@ function importExportPlugin({types: t}: {types: Types, ...}): PluginObj<State> {
               id: sharedModuleImport,
               init: withLocation(
                 t.callExpression(t.identifier('require'), [
-                  resolvePath(file, state.opts.resolve),
+                  resolvePath(t.cloneNode(file), state.opts.resolve),
                 ]),
                 loc,
               ),
@@ -397,9 +397,9 @@ function importExportPlugin({types: t}: {types: Types, ...}): PluginObj<State> {
                 state.imports.push({
                   node: withLocation(
                     importTemplate({
-                      IMPORT: state.importAll,
-                      FILE: resolvePath(file, state.opts.resolve),
-                      LOCAL: local,
+                      IMPORT: t.cloneNode(state.importAll),
+                      FILE: resolvePath(t.cloneNode(file), state.opts.resolve),
+                      LOCAL: t.cloneNode(local),
                     }),
                     loc,
                   ),
@@ -410,9 +410,9 @@ function importExportPlugin({types: t}: {types: Types, ...}): PluginObj<State> {
                 state.imports.push({
                   node: withLocation(
                     importTemplate({
-                      IMPORT: state.importDefault,
-                      FILE: resolvePath(file, state.opts.resolve),
-                      LOCAL: local,
+                      IMPORT: t.cloneNode(state.importDefault),
+                      FILE: resolvePath(t.cloneNode(file), state.opts.resolve),
+                      LOCAL: t.cloneNode(local),
                     }),
                     loc,
                   ),
@@ -424,9 +424,12 @@ function importExportPlugin({types: t}: {types: Types, ...}): PluginObj<State> {
                   state.imports.push({
                     node: withLocation(
                       importTemplate({
-                        IMPORT: state.importDefault,
-                        FILE: resolvePath(file, state.opts.resolve),
-                        LOCAL: local,
+                        IMPORT: t.cloneNode(state.importDefault),
+                        FILE: resolvePath(
+                          t.cloneNode(file),
+                          state.opts.resolve,
+                        ),
+                        LOCAL: t.cloneNode(local),
                       }),
                       loc,
                     ),
@@ -435,17 +438,23 @@ function importExportPlugin({types: t}: {types: Types, ...}): PluginObj<State> {
                   path.scope.push({
                     id: local,
                     init: withLocation(
-                      t.memberExpression(sharedModuleImport, imported),
+                      t.memberExpression(
+                        t.cloneNode(sharedModuleImport),
+                        t.cloneNode(imported),
+                      ),
                       loc,
                     ),
                   });
                 } else {
                   state.imports.push({
                     node: withLocation(
                       importNamedTemplate({
-                        FILE: resolvePath(file, state.opts.resolve),
-                        LOCAL: local,
-                        REMOTE: imported,
+                        FILE: resolvePath(
+                          t.cloneNode(file),
+                          state.opts.resolve,
+                        ),
+                        LOCAL: t.cloneNode(local),
+                        REMOTE: t.cloneNode(imported),
                       }),
                       loc,
                     ),