Prevent @babel/traverse path cache side-effects in `generateFunctio…

…nMap` (#906)

Summary:
Pull Request resolved: #906

## Motivation
For performance reasons, we'd like to have the option to re-use an AST (without cloning) after deriving source map information from it. However, due to babel/babel#6437, traversing the AST within `generateFunctionMap` causes the `babel/traverse` path cache to be populated with entries Babel transform plugins can't use - in particular because `hub` is assumed to be set.

## Change
This diffs saves and clears the traverse cache before traversal, and then restores it afterwards. This isn't pretty but it's the cleanest approach I could find - we include tests to verify that this is (and continues to be) necessary.

## Other approaches
I tried some other things before resorting to manipulating `babel/traverse` internals:
 - Actually providing a `hub` so that it's set on the cache means adding a dependency on `babel/core`, and requires a similar level of hackery / assumptions of Babel internals.
 - Clearing the bad cache with `traverse.clearNode(ast)` or `traverse.cache.clearPath()` doesn't work because other tools (eg, Jest) rely on `properties` held in the path cache.

Changelog: [Internal]

Reviewed By: jacdebug, motiz88

Differential Revision: D42068914

fbshipit-source-id: e928b7ee1ffc5462f0ead7bcf4077b818a4cd95a

flow-typed/babel-traverse.js

@@ -1845,6 +1845,8 @@ declare module '@babel/traverse' {
   declare export var visitors: Visitors;
 
   declare export type Cache = {
+    path: $ReadOnlyWeakMap<BabelNode, mixed>,
+    scope: $ReadOnlyWeakMap<BabelNode, mixed>,
     clear(): void,
     clearPath(): void,
     clearScope(): void,

packages/metro-source-map/package.json

@@ -23,6 +23,7 @@
   },
   "license": "MIT",
   "devDependencies": {
+    "@babel/core": "^7.20.0",
     "@babel/parser": "^7.20.0",
     "uglify-es": "^3.1.9"
   }

packages/metro-source-map/src/__tests__/generateFunctionMap-test.js

@@ -17,7 +17,9 @@ const {
   generateFunctionMap,
   generateFunctionMappingsArray,
 } = require('../generateFunctionMap');
+const {transformFromAstSync} = require('@babel/core');
 const {parse} = require('@babel/parser');
+const traverse = require('@babel/traverse').default;
 const {
   SourceMetadataMapConsumer,
 } = require('metro-symbolicate/src/Symbolication');
@@ -1602,4 +1604,86 @@ function parent2() {
       `);
     });
   });
+
+  describe('@babel/traverse path cache workaround (babel#6437)', () => {
+    /* These tests exist due to the need to work around a Babel issue:
+       https://github.com/babel/babel/issues/6437
+       In short, using `@babel/traverse` outside of a transform context
+       pollutes the cache in such a way as to break subsequent transformation
+       of the same AST.
+
+       This commonly manifests as: "Cannot read properties of undefined
+       (reading 'addHelper')", and is due to a missing `hub` property normally
+       provided by `@babel/core` but not populated when using `traverse` alone.
+
+       We need to work around this by not mutating the cache on traversal.
+
+       Note though that we must also must be careful to preserve any existing
+       cache, because others (Fast Refresh, Jest) rely on cached properties set
+       on paths. */
+
+    // A minimal(?) Babel transformation that requires a `hub`, modelled on
+    // `@babel/plugin-transform-modules-commonjs` and the `wrapInterop` call in
+    // `@babel/helper-module-transforms`
+    const transformRequiringHub = (ast: BabelNodeFile) =>
+      transformFromAstSync(ast, '', {
+        plugins: [
+          () => ({
+            visitor: {
+              Program: {
+                enter: path => {
+                  expect(path.hub).toBeDefined();
+                },
+              },
+            },
+          }),
+        ],
+        babelrc: false,
+        cloneInputAst: false,
+      });
+
+    let ast;
+
+    beforeEach(() => {
+      ast = getAst('arbitrary(code)');
+      traverse.cache.clearPath();
+    });
+
+    it('requires a workaround for traverse cache pollution', () => {
+      /* If this test fails, it likely means either:
+         1. There are multiple copies of `@babel/traverse` in node_modules, and
+            the one used by `@babel/core` is not the one used by this test.
+            This masks the issue, and probably means you should deduplicate
+            yarn.lock.
+         2. https://github.com/babel/babel/issues/6437 has been fixed upstream,
+            In that case, we should be able to remove cache-related hacks
+            around `traverse` from generateFunctionMap, and these tests. */
+
+      // Perform a trivial traversal.
+      traverse(ast, {});
+
+      // Expect that the path cache is polluted with entries lacking `hub`.
+      expect(() => transformRequiringHub(ast)).toThrow();
+    });
+
+    it('successfully works around traverse cache pollution', () => {
+      generateFunctionMap(ast);
+
+      // Check that the `hub` property is present on paths when transforming.
+      transformRequiringHub(ast);
+    });
+
+    it('does not reset the path cache', () => {
+      const dummyCache: Map<mixed, mixed> = new Map();
+      // $FlowIgnore[prop-missing] - Writing to readonly map for test purposes.
+      traverse.cache.path.set(ast, dummyCache);
+
+      generateFunctionMap(ast);
+
+      // Check that we're not working around the issue by clearing the cache -
+      // that causes problems elsewhere.
+      expect(traverse.cache.path.get(ast)).toBe(dummyCache);
+      expect(dummyCache.size).toBe(0);
+    });
+  });
 });

packages/metro-source-map/src/generateFunctionMap.js

@@ -165,11 +165,21 @@ function forEachMapping(
     },
   };
 
+  // Traversing populates/pollutes the path cache (`traverse.cache.path`) with
+  // values missing the `hub` property needed by Babel transformation, so we
+  // save, clear, and restore the cache around our traversal.
+  // See: https://github.com/facebook/metro/pull/854#issuecomment-1336499395
+  const previousCache = traverse.cache.path;
+  traverse.cache.clearPath();
   traverse(ast, {
+    // Our visitor doesn't care about scope
+    noScope: true,
+
     Function: visitor,
     Program: visitor,
     Class: visitor,
   });
+  traverse.cache.path = previousCache;
 }
 
 const ANONYMOUS_NAME = '<anonymous>';