Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Use NodePath#hub as part of the paths cache key" #15754

Merged
merged 1 commit into from Jul 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/babel-core/src/transformation/index.ts
Expand Up @@ -117,7 +117,7 @@ function* transformFile(file: File, pluginPasses: PluginPasses): Handler<void> {
passes,
file.opts.wrapPluginVisitorMethod,
);
traverse(file.ast.program, visitor, file.scope, null, file.path, true);
traverse(file.ast, visitor, file.scope);

for (const [plugin, pass] of passPairs) {
const fn = plugin.post;
Expand Down
1 change: 0 additions & 1 deletion packages/babel-traverse/package.json
Expand Up @@ -28,7 +28,6 @@
"globals": "condition:BABEL_8_BREAKING ? ^13.5.0 : ^11.1.0"
},
"devDependencies": {
"@babel/core": "workspace:^",
"@babel/helper-plugin-test-runner": "workspace:^"
},
"engines": {
Expand Down
29 changes: 2 additions & 27 deletions packages/babel-traverse/src/cache.ts
@@ -1,13 +1,8 @@
import type { Node } from "@babel/types";
import type NodePath from "./path";
import type Scope from "./scope";
import type { HubInterface } from "./hub";

let pathsCache: WeakMap<
HubInterface | typeof nullHub,
WeakMap<Node, Map<Node, NodePath>>
> = new WeakMap();
export { pathsCache as path };
export let path: WeakMap<Node, Map<Node, NodePath>> = new WeakMap();
export let scope: WeakMap<Node, Scope> = new WeakMap();

export function clear() {
Expand All @@ -16,29 +11,9 @@ export function clear() {
}

export function clearPath() {
pathsCache = new WeakMap();
path = new WeakMap();
}

export function clearScope() {
scope = new WeakMap();
}

// NodePath#hub can be null, but it's not a valid weakmap key because it
// cannot be collected by GC. Use an object, knowing tht it will not be
// collected anyway. It's not a memory leak because pathsCache.get(nullHub)
// is itself a weakmap, so its entries can still be collected.
const nullHub = Object.freeze({} as const);

export function getCachedPaths(hub: HubInterface | null, parent: Node) {
return pathsCache.get(hub ?? nullHub)?.get(parent);
}

export function getOrCreateCachedPaths(hub: HubInterface | null, parent: Node) {
let parents = pathsCache.get(hub ?? nullHub);
if (!parents) pathsCache.set(hub ?? nullHub, (parents = new WeakMap()));

let paths = parents.get(parent);
if (!paths) parents.set(parent, (paths = new Map()));

return paths;
}
19 changes: 3 additions & 16 deletions packages/babel-traverse/src/index.ts
Expand Up @@ -36,7 +36,6 @@ function traverse<S>(
scope: Scope | undefined,
state: S,
parentPath?: NodePath,
visitSelf?: boolean,
): void;

function traverse(
Expand All @@ -45,7 +44,6 @@ function traverse(
scope?: Scope,
state?: any,
parentPath?: NodePath,
visitSelf?: boolean,
): void;

function traverse<Options extends TraverseOptions>(
Expand All @@ -55,7 +53,6 @@ function traverse<Options extends TraverseOptions>(
scope?: Scope,
state?: any,
parentPath?: NodePath,
visitSelf?: boolean,
) {
if (!parent) return;

Expand All @@ -69,25 +66,13 @@ function traverse<Options extends TraverseOptions>(
}
}

if (!parentPath && visitSelf) {
throw new Error("visitSelf can only be used when providing a NodePath.");
}

if (!VISITOR_KEYS[parent.type]) {
return;
}

visitors.explode(opts as Visitor);

traverseNode(
parent,
opts as ExplodedVisitor,
scope,
state,
parentPath,
/* skipKeys */ null,
visitSelf,
);
traverseNode(parent, opts as ExplodedVisitor, scope, state, parentPath);
}

export default traverse;
Expand Down Expand Up @@ -115,6 +100,8 @@ traverse.node = function (

traverse.clearNode = function (node: t.Node, opts?: RemovePropertiesOptions) {
removeProperties(node, opts);

cache.path.delete(node);
};

traverse.removeProperties = function (
Expand Down
8 changes: 6 additions & 2 deletions packages/babel-traverse/src/path/index.ts
Expand Up @@ -8,7 +8,7 @@ import type { Visitor } from "../types";
import Scope from "../scope";
import { validate } from "@babel/types";
import * as t from "@babel/types";
import { getOrCreateCachedPaths } from "../cache";
import { path as pathCache } from "../cache";
import generator from "@babel/generator";

// NodePath is split across many files.
Expand Down Expand Up @@ -92,7 +92,11 @@ class NodePath<T extends t.Node = t.Node> {
// @ts-expect-error key must present in container
container[key];

const paths = getOrCreateCachedPaths(hub, parent);
let paths = pathCache.get(parent);
if (!paths) {
paths = new Map();
pathCache.set(parent, paths);
}

let path = paths.get(targetNode);
if (!path) {
Expand Down
4 changes: 2 additions & 2 deletions packages/babel-traverse/src/path/modification.ts
@@ -1,6 +1,6 @@
// This file contains methods that modify the path/node in some ways.

import { getCachedPaths } from "../cache";
import { path as pathCache } from "../cache";
import PathHoister from "./lib/hoister";
import NodePath from "./index";
import {
Expand Down Expand Up @@ -279,7 +279,7 @@ export function updateSiblingKeys(
) {
if (!this.parent) return;

const paths = getCachedPaths(this.hub, this.parent) || ([] as never[]);
const paths = pathCache.get(this.parent);

for (const [, path] of paths) {
if (typeof path.key === "number" && path.key >= fromIndex) {
Expand Down
6 changes: 2 additions & 4 deletions packages/babel-traverse/src/path/removal.ts
@@ -1,7 +1,7 @@
// This file contains methods responsible for removing a node.

import { hooks } from "./lib/removal-hooks";
import { getCachedPaths } from "../cache";
import { path as pathCache } from "../cache";
import type NodePath from "./index";
import { REMOVED, SHOULD_SKIP } from "./index";

Expand Down Expand Up @@ -46,9 +46,7 @@ export function _remove(this: NodePath) {
export function _markRemoved(this: NodePath) {
// this.shouldSkip = true; this.removed = true;
this._traverseFlags |= SHOULD_SKIP | REMOVED;
if (this.parent) {
getCachedPaths(this.hub, this.parent).delete(this.node);
}
if (this.parent) pathCache.get(this.parent).delete(this.node);
this.node = null;
}

Expand Down
6 changes: 3 additions & 3 deletions packages/babel-traverse/src/path/replacement.ts
Expand Up @@ -3,7 +3,7 @@
import { codeFrameColumns } from "@babel/code-frame";
import traverse from "../index";
import NodePath from "./index";
import { getCachedPaths } from "../cache";
import { path as pathCache } from "../cache";
import { parse } from "@babel/parser";
import {
FUNCTION_TYPES,
Expand Down Expand Up @@ -47,7 +47,7 @@ export function replaceWithMultiple(
nodes = this._verifyNodeList(nodes);
inheritLeadingComments(nodes[0], this.node);
inheritTrailingComments(nodes[nodes.length - 1], this.node);
getCachedPaths(this.hub, this.parent)?.delete(this.node);
pathCache.get(this.parent)?.delete(this.node);
this.node =
// @ts-expect-error this.key must present in this.container
this.container[this.key] = null;
Expand Down Expand Up @@ -210,7 +210,7 @@ export function _replaceWith(this: NodePath, node: t.Node) {
}

this.debug(`Replace with ${node?.type}`);
getCachedPaths(this.hub, this.parent)?.set(node, this).delete(this.node);
pathCache.get(this.parent)?.set(node, this).delete(this.node);

this.node =
// @ts-expect-error this.key must present in this.container
Expand Down
8 changes: 1 addition & 7 deletions packages/babel-traverse/src/traverse-node.ts
Expand Up @@ -24,19 +24,13 @@ export function traverseNode<S = unknown>(
state?: any,
path?: NodePath,
skipKeys?: Record<string, boolean>,
visitSelf?: boolean,
): boolean {
const keys = VISITOR_KEYS[node.type];
if (!keys) return false;

const context = new TraversalContext(scope, opts, state, path);
if (visitSelf) {
if (skipKeys?.[path.parentKey]) return false;
return context.visitQueue([path]);
}

for (const key of keys) {
if (skipKeys?.[key]) continue;
if (skipKeys && skipKeys[key]) continue;
if (context.visit(node, key)) {
return true;
}
Expand Down
40 changes: 2 additions & 38 deletions packages/babel-traverse/test/hub.js
@@ -1,46 +1,10 @@
import { transformSync } from "@babel/core";
import assert from "assert";
import { Hub } from "../lib/index.js";

describe("hub", function () {
it("default buildError should return TypeError", function () {
const hub = new Hub();
const msg = "test_msg";
expect(hub.buildError(null, msg)).toEqual(new TypeError(msg));
});

it("should be preserved across nested traversals", function () {
let origHub;
let innerHub = {};
let exprHub;
function plugin({ types: t, traverse }) {
return {
visitor: {
Identifier(path) {
if (path.node.name !== "foo") return;
origHub = path.hub;

const mem = t.memberExpression(
t.identifier("property"),
t.identifier("access"),
);
traverse(mem, {
noScope: true,
Identifier(path) {
if (path.node.name === "property") innerHub = path.hub;
},
});
const [p2] = path.insertAfter(mem);

exprHub = p2.get("expression").hub;
},
},
};
}

transformSync("foo;", { configFile: false, plugins: [plugin] });

expect(origHub).toBeInstanceOf(Object);
expect(exprHub).toBe(origHub);
expect(innerHub).toBeUndefined();
assert.deepEqual(hub.buildError(null, msg), new TypeError(msg));
});
});
1 change: 0 additions & 1 deletion yarn.lock
Expand Up @@ -3956,7 +3956,6 @@ __metadata:
resolution: "@babel/traverse@workspace:packages/babel-traverse"
dependencies:
"@babel/code-frame": "workspace:^"
"@babel/core": "workspace:^"
"@babel/generator": "workspace:^"
"@babel/helper-environment-visitor": "workspace:^"
"@babel/helper-function-name": "workspace:^"
Expand Down