Skip to content

Commit

Permalink
improve traverse cache pollution bug workaround testing (#1330)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #1330

**Problem:**
`traverse(ast` was polluting `traverse.cache.path` with values missing the `hub` property needed by Babel transformation.

**Former solution:**
the solution implemented formally, was saving `traverse.cache.path`, clearing the cache, and then re-assigning `traverse.cache.path` to be the one before the traversal.

Unfortunately, with the latest version of `babel/traverse` the assignment `traverse.cache.path = previousCache` no longer works.

This is because `path` inside the esmodule `traverse.cache` is a `let export` ([see source code](https://github.com/babel/babel/blob/main/packages/babel-traverse/src/cache.ts#L6C1-L21C1)) which is resoved to:
```
let pathsCache = exports.path = new WeakMap();
function clearPath() {
  exports.path = pathsCache = new WeakMap();
}
function getCachedPaths(hub, parent) {
// ...
pathsCache.get( //...
// ...
}
```
this means that re-writing `exports.path` breaks the module since `exports.path` won't be identical to `pathsCache` anymore and because it is used inside the module, the module breaks.

**Proposed solution:**
A one time shallow copy ("`shallowCopiedAST`") of the `ast` for the traverse ensures that the original `ast` is not polluted.

The copy is then deleted when `forEachMapping` which calls `traverse` is ends and `shallowCopiedAST` goes out of scope, referenced only by `traverse.cache.path` which is a `WeakMap`.

Reviewed By: robhogan

Differential Revision: D61336390

fbshipit-source-id: 600f4032a10fa289a323beceedd182417914261b
  • Loading branch information
vzaidman authored and facebook-github-bot committed Aug 30, 2024
1 parent c6315b7 commit 5d63c99
Showing 1 changed file with 61 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
'use strict';

import type {Context} from '../generateFunctionMap';
import type {NodePath} from '@babel/traverse';
import type {MetroBabelFileMetadata} from 'metro-babel-transformer';

const {
Expand All @@ -22,6 +23,10 @@ const {
const {transformFromAstSync} = require('@babel/core');
const {parse} = require('@babel/parser');
const traverse = require('@babel/traverse').default;
const STANDARDIZED_TYPES: Array<BabelNodeStandardized> =
// $FlowIgnore[prop-missing]
// $FlowIgnore[incompatible-type]
require('@babel/types').STANDARDIZED_TYPES;
const {
SourceMetadataMapConsumer,
} = require('metro-symbolicate/src/Symbolication');
Expand Down Expand Up @@ -1809,30 +1814,79 @@ function parent2() {
// 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) =>
const expectTransformPathesToHaveHub = (ast: BabelNodeFile) => {
let enterCount = 0;

const enter = (path: NodePath<BabelNode>) => {
enterCount++;
expect(path.hub).toBeDefined();
};

transformFromAstSync(ast, '', {
plugins: [
() => ({
visitor: Object.fromEntries(
STANDARDIZED_TYPES.map(type => [type, {enter}]),
) /** equivalent to:
visitor: {
Program: {
enter: path => {
"FunctionDeclaration": {
enter: (path: NodePath<BabelNode>) => {
enterCount++;
expect(path.hub).toBeDefined();
}
},
"Program": {
enter: (path: NodePath<BabelNode>) => {
enterCount++;
expect(path.hub).toBeDefined();
},
},
},
// ... the rest of all the possible ast node types
//
} **/,
}),
],
babelrc: false,
cloneInputAst: false,
});
expect(enterCount).toBe(61);
};

let ast;

beforeEach(() => {
ast = getAst('arbitrary(code)');
ast = getAst(`
window.foo = function bar() {
return false || {
a: {
"b": {
c: ['d', 1, {e: 'f'}],
g: function h() {
return (function(aa) {
if (null) {
return true;
}
return [{b: aa ? 2 : {b: 'ee'}}];
})(123);
}
}
}
}
}
window.foo();
`);
traverse.cache.clearPath();
});

it('transform with no traverse has `hub` in every node', () => {
/* Ensures that our expectations of how transform works regardless
of the existence of a traverse cache pollution issue are correct.
Namely- that each node is expected to have a hub present.
If this fails, it means that "hub" is no longer expected to
exist on each node, and the pollution tests bellow has to be adjusted. */
expectTransformPathesToHaveHub(ast);
});

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
Expand All @@ -1847,14 +1901,14 @@ function parent2() {
traverse(ast, {});

// Expect that the path cache is polluted with entries lacking `hub`.
expect(() => transformRequiringHub(ast)).toThrow();
expect(() => expectTransformPathesToHaveHub(ast)).toThrow();
});

it('successfully works around traverse cache pollution', () => {
generateFunctionMap(ast);

// Check that the `hub` property is present on paths when transforming.
transformRequiringHub(ast);
expectTransformPathesToHaveHub(ast);
});

it('does not reset the path cache', () => {
Expand Down

0 comments on commit 5d63c99

Please sign in to comment.