From b9a893aab68a6f6980b738a34d7345b5dbb7757d Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Wed, 2 Mar 2016 12:20:11 -0800 Subject: [PATCH 01/10] Move NodePath cache out of the AST As mentioned on the task https://phabricator.babeljs.io/T7179 having this cache on the AST leads to all sorts of portability and reuse bugs. This moves the cache into a clearable WeakMap which will fix the following: 1. Moving the AST between different babel versions or tools will not lead into sharing potentially outdated cached information 2. `.clear()` can be called on the cache by a plugin to clear potentially outdated information. This is helpful when implementing two seperate pipelines that should not share information. I think the next step (which is harder, I tried) is to isolate cache and make it live on a transform or pipeline level state (like the `hub`). The reason it is hard is because the `babel-traverse` main API -- although requires the state object to be passed -- not many callers do. To fix this we should release a patch version that warns about this and fix all the internal callers. Next couple of releases we can start throwing when no state is passed (or we can create our own state). --- packages/babel-traverse/src/index.js | 1 + packages/babel-traverse/src/path/cache.js | 25 +++++++++++++++++++ packages/babel-traverse/src/path/constants.js | 1 - packages/babel-traverse/src/path/index.js | 8 ++++-- .../babel-traverse/src/path/modification.js | 4 +-- 5 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 packages/babel-traverse/src/path/cache.js delete mode 100644 packages/babel-traverse/src/path/constants.js diff --git a/packages/babel-traverse/src/index.js b/packages/babel-traverse/src/index.js index 01a48112c4..099e6f0bf7 100644 --- a/packages/babel-traverse/src/index.js +++ b/packages/babel-traverse/src/index.js @@ -9,6 +9,7 @@ import * as t from "babel-types"; export { default as NodePath } from "./path"; export { default as Scope } from "./scope"; export { default as Hub } from "./hub"; +export { default as cache } from "./path/cache"; export { visitors }; export default function traverse( diff --git a/packages/babel-traverse/src/path/cache.js b/packages/babel-traverse/src/path/cache.js new file mode 100644 index 0000000000..a58ecc5f08 --- /dev/null +++ b/packages/babel-traverse/src/path/cache.js @@ -0,0 +1,25 @@ +let wm = new WeakMap(); + +// To implement clear we need to export a facade. +export default { + clear() { + wm = new WeakMap(); + }, + + delete(k) { + return wm.delete(k) + }, + + get(k) { + return wm.get(k) + }, + + has(k) { + return wm.has(k) + }, + + set(k, v) { + wm.set(k, v); + return wm; + }, +}; diff --git a/packages/babel-traverse/src/path/constants.js b/packages/babel-traverse/src/path/constants.js deleted file mode 100644 index 11431edfe4..0000000000 --- a/packages/babel-traverse/src/path/constants.js +++ /dev/null @@ -1 +0,0 @@ -export const PATH_CACHE_KEY = "_paths"; //Symbol(); diff --git a/packages/babel-traverse/src/path/index.js b/packages/babel-traverse/src/path/index.js index 4fa06c9b30..fadb928ab6 100644 --- a/packages/babel-traverse/src/path/index.js +++ b/packages/babel-traverse/src/path/index.js @@ -4,12 +4,12 @@ import type Hub from "../hub"; import type TraversalContext from "../context"; import * as virtualTypes from "./lib/virtual-types"; import buildDebug from "debug"; -import { PATH_CACHE_KEY } from "./constants"; import invariant from "invariant"; import traverse from "../index"; import assign from "lodash/object/assign"; import Scope from "../scope"; import * as t from "babel-types"; +import cache from "./cache"; let debug = buildDebug("babel"); @@ -69,7 +69,11 @@ export default class NodePath { let targetNode = container[key]; - let paths = parent[PATH_CACHE_KEY] = parent[PATH_CACHE_KEY] || []; + let paths = cache.get(parent) || []; + if (!cache.has(parent)) { + cache.set(parent, paths); + } + let path; for (let i = 0; i < paths.length; i++) { diff --git a/packages/babel-traverse/src/path/modification.js b/packages/babel-traverse/src/path/modification.js index b2827613a3..c32745bb9c 100644 --- a/packages/babel-traverse/src/path/modification.js +++ b/packages/babel-traverse/src/path/modification.js @@ -1,7 +1,7 @@ /* eslint max-len: 0 */ // This file contains methods that modify the path/node in some ways. -import { PATH_CACHE_KEY } from "./constants"; +import cache from "./cache"; import PathHoister from "./lib/hoister"; import NodePath from "./index"; import * as t from "babel-types"; @@ -136,7 +136,7 @@ export function insertAfter(nodes) { export function updateSiblingKeys(fromIndex, incrementBy) { if (!this.parent) return; - let paths = this.parent[PATH_CACHE_KEY]; + let paths = cache.get(this.parent); for (let i = 0; i < paths.length; i++) { let path = paths[i]; if (path.key >= fromIndex) { From 29ef158204a030cff0a3d1054d47411dfaec8a58 Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Wed, 2 Mar 2016 12:45:33 -0800 Subject: [PATCH 02/10] Semis --- packages/babel-traverse/src/path/cache.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/babel-traverse/src/path/cache.js b/packages/babel-traverse/src/path/cache.js index a58ecc5f08..39dda7a758 100644 --- a/packages/babel-traverse/src/path/cache.js +++ b/packages/babel-traverse/src/path/cache.js @@ -7,15 +7,15 @@ export default { }, delete(k) { - return wm.delete(k) + return wm.delete(k); }, get(k) { - return wm.get(k) + return wm.get(k); }, has(k) { - return wm.has(k) + return wm.has(k); }, set(k, v) { From 5367d5d15161ef7d0124170984c6561303a36d74 Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Wed, 2 Mar 2016 17:38:26 -0800 Subject: [PATCH 03/10] Make sure we update the cache in all the right places --- packages/babel-traverse/src/index.js | 5 ++++- packages/babel-types/src/index.js | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/babel-traverse/src/index.js b/packages/babel-traverse/src/index.js index 099e6f0bf7..00f2a423e7 100644 --- a/packages/babel-traverse/src/index.js +++ b/packages/babel-traverse/src/index.js @@ -39,7 +39,8 @@ traverse.explode = visitors.explode; traverse.NodePath = require("./path"); traverse.Scope = require("./scope"); -traverse.Hub = require("./hub"); +traverse.Hub = require("./hub") +traverse.cache = require("./path/cache"); traverse.cheap = function (node, enter) { if (!node) return; @@ -88,6 +89,8 @@ traverse.clearNode = function (node) { if (key[0] === "_" && node[key] != null) node[key] = undefined; } + traverse.cache.delete(node); + let syms: Array = Object.getOwnPropertySymbols(node); for (let sym of syms) { node[sym] = null; diff --git a/packages/babel-types/src/index.js b/packages/babel-types/src/index.js index 03cac174eb..43aa56968a 100644 --- a/packages/babel-types/src/index.js +++ b/packages/babel-types/src/index.js @@ -373,6 +373,12 @@ function _inheritComments(key, child, parent) { } } + +// Can't use import because of cyclic dependency between babel-traverse +// and this module (babel-types). This require needs to appear after +// we export the TYPES constant. +const traverse = require("babel-traverse").default; + /** * Inherit all contextual properties from `parent` node to `child` node. */ @@ -399,6 +405,9 @@ export function inherits(child, parent) { t.inheritsComments(child, parent); + if (traverse.cache.has(parent)) { + traverse.cache.set(child, traverse.cache.get(parent)); + } return child; } From 3c148148bc70ef59079fcb9daf03e6568211609b Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Wed, 2 Mar 2016 17:39:04 -0800 Subject: [PATCH 04/10] Semicolon --- packages/babel-traverse/src/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/babel-traverse/src/index.js b/packages/babel-traverse/src/index.js index 00f2a423e7..5ec1036000 100644 --- a/packages/babel-traverse/src/index.js +++ b/packages/babel-traverse/src/index.js @@ -39,7 +39,7 @@ traverse.explode = visitors.explode; traverse.NodePath = require("./path"); traverse.Scope = require("./scope"); -traverse.Hub = require("./hub") +traverse.Hub = require("./hub"); traverse.cache = require("./path/cache"); traverse.cheap = function (node, enter) { From bf91a6837559adc1d045f139273a03295849ed8b Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Wed, 2 Mar 2016 18:28:21 -0800 Subject: [PATCH 05/10] Move scope cache to the cache module --- packages/babel-traverse/src/index.js | 2 +- packages/babel-traverse/src/path/cache.js | 30 ++++------------ packages/babel-traverse/src/path/index.js | 8 ++--- .../babel-traverse/src/path/modification.js | 4 +-- packages/babel-traverse/src/scope/index.js | 35 ++++++------------- packages/babel-types/src/index.js | 5 +-- 6 files changed, 26 insertions(+), 58 deletions(-) diff --git a/packages/babel-traverse/src/index.js b/packages/babel-traverse/src/index.js index 5ec1036000..7eb65732b8 100644 --- a/packages/babel-traverse/src/index.js +++ b/packages/babel-traverse/src/index.js @@ -89,7 +89,7 @@ traverse.clearNode = function (node) { if (key[0] === "_" && node[key] != null) node[key] = undefined; } - traverse.cache.delete(node); + traverse.cache.path.delete(node); let syms: Array = Object.getOwnPropertySymbols(node); for (let sym of syms) { diff --git a/packages/babel-traverse/src/path/cache.js b/packages/babel-traverse/src/path/cache.js index 39dda7a758..08a771a5fc 100644 --- a/packages/babel-traverse/src/path/cache.js +++ b/packages/babel-traverse/src/path/cache.js @@ -1,25 +1,7 @@ -let wm = new WeakMap(); +export let path = new WeakMap(); +export let scope = new WeakMap(); -// To implement clear we need to export a facade. -export default { - clear() { - wm = new WeakMap(); - }, - - delete(k) { - return wm.delete(k); - }, - - get(k) { - return wm.get(k); - }, - - has(k) { - return wm.has(k); - }, - - set(k, v) { - wm.set(k, v); - return wm; - }, -}; +export function clear() { + path = new WeakMap(); + scope = new WeakMap(); +} diff --git a/packages/babel-traverse/src/path/index.js b/packages/babel-traverse/src/path/index.js index fadb928ab6..7547e95581 100644 --- a/packages/babel-traverse/src/path/index.js +++ b/packages/babel-traverse/src/path/index.js @@ -9,7 +9,7 @@ import traverse from "../index"; import assign from "lodash/object/assign"; import Scope from "../scope"; import * as t from "babel-types"; -import cache from "./cache"; +import { path as pathCache } from "./cache"; let debug = buildDebug("babel"); @@ -69,9 +69,9 @@ export default class NodePath { let targetNode = container[key]; - let paths = cache.get(parent) || []; - if (!cache.has(parent)) { - cache.set(parent, paths); + let paths = pathCache.get(parent) || []; + if (!pathCache.has(parent)) { + pathCache.set(parent, paths); } let path; diff --git a/packages/babel-traverse/src/path/modification.js b/packages/babel-traverse/src/path/modification.js index c32745bb9c..9155a97dc6 100644 --- a/packages/babel-traverse/src/path/modification.js +++ b/packages/babel-traverse/src/path/modification.js @@ -1,7 +1,7 @@ /* eslint max-len: 0 */ // This file contains methods that modify the path/node in some ways. -import cache from "./cache"; +import { path as pathCache } from "./cache"; import PathHoister from "./lib/hoister"; import NodePath from "./index"; import * as t from "babel-types"; @@ -136,7 +136,7 @@ export function insertAfter(nodes) { export function updateSiblingKeys(fromIndex, incrementBy) { if (!this.parent) return; - let paths = cache.get(this.parent); + let paths = pathCache.get(this.parent); for (let i = 0; i < paths.length; i++) { let path = paths[i]; if (path.key >= fromIndex) { diff --git a/packages/babel-traverse/src/scope/index.js b/packages/babel-traverse/src/scope/index.js index 05c492f2d4..c4fe62188b 100644 --- a/packages/babel-traverse/src/scope/index.js +++ b/packages/babel-traverse/src/scope/index.js @@ -10,38 +10,17 @@ import * as messages from "babel-messages"; import Binding from "./binding"; import globals from "globals"; import * as t from "babel-types"; - -// - -const CACHE_SINGLE_KEY = Symbol(); -const CACHE_MULTIPLE_KEY = Symbol(); +import { scope as scopeCache } from "../path/cache"; /** * To avoid creating a new Scope instance for each traversal, we maintain a cache on the * node itself containing all scopes it has been associated with. - * - * We also optimise for the case of there being only a single scope associated with a node. */ function getCache(node, parentScope, self) { - let singleCache = node[CACHE_SINGLE_KEY]; - - if (singleCache) { - // we've only ever associated one scope with this node so let's check it - if (matchesParent(singleCache, parentScope)) { - return singleCache; - } - } else if (!node[CACHE_MULTIPLE_KEY]) { - // no scope has ever been associated with this node - node[CACHE_SINGLE_KEY] = self; - return; - } - - // looks like we have either a single scope association that was never matched or - // multiple assocations, let's find the right one! - return getCacheMultiple(node, parentScope, self, singleCache); -} + let scopes: Array = scopeCache.get(node) || []; +<<<<<<< HEAD function matchesParent(scope, parentScope) { if (scope.parent === parentScope) { return true; @@ -58,11 +37,17 @@ function getCacheMultiple(node, parentScope, self, singleCache) { } // loop through and check each scope to see if it matches our parent +======= +>>>>>>> Move scope cache to the cache module for (let scope of scopes) { - if (matchesParent(scope, parentScope)) return scope; + if (scope.parent === parentScope) return scope; } scopes.push(self); + + if (!scopeCache.has(node)) { + scopeCache.set(node, scopes); + } } // diff --git a/packages/babel-types/src/index.js b/packages/babel-types/src/index.js index 43aa56968a..6c559d87aa 100644 --- a/packages/babel-types/src/index.js +++ b/packages/babel-types/src/index.js @@ -405,8 +405,9 @@ export function inherits(child, parent) { t.inheritsComments(child, parent); - if (traverse.cache.has(parent)) { - traverse.cache.set(child, traverse.cache.get(parent)); + const pathCache = traverse.cache.path; + if (pathCache.has(parent)) { + pathCache.set(child, pathCache.get(parent)); } return child; } From b53755422c16175bb39a85be97ac55356d469b5b Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Wed, 2 Mar 2016 18:37:29 -0800 Subject: [PATCH 06/10] Move things around --- packages/babel-traverse/src/{path => }/cache.js | 0 packages/babel-traverse/src/index.js | 3 +-- packages/babel-traverse/src/path/index.js | 2 +- packages/babel-traverse/src/path/modification.js | 2 +- 4 files changed, 3 insertions(+), 4 deletions(-) rename packages/babel-traverse/src/{path => }/cache.js (100%) diff --git a/packages/babel-traverse/src/path/cache.js b/packages/babel-traverse/src/cache.js similarity index 100% rename from packages/babel-traverse/src/path/cache.js rename to packages/babel-traverse/src/cache.js diff --git a/packages/babel-traverse/src/index.js b/packages/babel-traverse/src/index.js index 7eb65732b8..e641314145 100644 --- a/packages/babel-traverse/src/index.js +++ b/packages/babel-traverse/src/index.js @@ -9,7 +9,6 @@ import * as t from "babel-types"; export { default as NodePath } from "./path"; export { default as Scope } from "./scope"; export { default as Hub } from "./hub"; -export { default as cache } from "./path/cache"; export { visitors }; export default function traverse( @@ -40,7 +39,7 @@ traverse.explode = visitors.explode; traverse.NodePath = require("./path"); traverse.Scope = require("./scope"); traverse.Hub = require("./hub"); -traverse.cache = require("./path/cache"); +traverse.cache = require("./cache"); traverse.cheap = function (node, enter) { if (!node) return; diff --git a/packages/babel-traverse/src/path/index.js b/packages/babel-traverse/src/path/index.js index 7547e95581..63f971a5b3 100644 --- a/packages/babel-traverse/src/path/index.js +++ b/packages/babel-traverse/src/path/index.js @@ -9,7 +9,7 @@ import traverse from "../index"; import assign from "lodash/object/assign"; import Scope from "../scope"; import * as t from "babel-types"; -import { path as pathCache } from "./cache"; +import { path as pathCache } from "../cache"; let debug = buildDebug("babel"); diff --git a/packages/babel-traverse/src/path/modification.js b/packages/babel-traverse/src/path/modification.js index 9155a97dc6..6465207ebf 100644 --- a/packages/babel-traverse/src/path/modification.js +++ b/packages/babel-traverse/src/path/modification.js @@ -1,7 +1,7 @@ /* eslint max-len: 0 */ // This file contains methods that modify the path/node in some ways. -import { path as pathCache } from "./cache"; +import { path as pathCache } from "../cache"; import PathHoister from "./lib/hoister"; import NodePath from "./index"; import * as t from "babel-types"; From 403d6153fdd7bfdbf0f983b5789188d243b5baac Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Wed, 2 Mar 2016 18:50:18 -0800 Subject: [PATCH 07/10] correct cache path --- packages/babel-traverse/src/scope/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/babel-traverse/src/scope/index.js b/packages/babel-traverse/src/scope/index.js index c4fe62188b..e4a92a9285 100644 --- a/packages/babel-traverse/src/scope/index.js +++ b/packages/babel-traverse/src/scope/index.js @@ -10,7 +10,7 @@ import * as messages from "babel-messages"; import Binding from "./binding"; import globals from "globals"; import * as t from "babel-types"; -import { scope as scopeCache } from "../path/cache"; +import { scope as scopeCache } from "../cache"; /** * To avoid creating a new Scope instance for each traversal, we maintain a cache on the From fc19ac2af5d93208afd45bfa818c094bf441135b Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Wed, 2 Mar 2016 19:13:38 -0800 Subject: [PATCH 08/10] Remove merge artificats --- packages/babel-traverse/src/scope/index.js | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/packages/babel-traverse/src/scope/index.js b/packages/babel-traverse/src/scope/index.js index e4a92a9285..084100c815 100644 --- a/packages/babel-traverse/src/scope/index.js +++ b/packages/babel-traverse/src/scope/index.js @@ -20,25 +20,6 @@ import { scope as scopeCache } from "../cache"; function getCache(node, parentScope, self) { let scopes: Array = scopeCache.get(node) || []; -<<<<<<< HEAD -function matchesParent(scope, parentScope) { - if (scope.parent === parentScope) { - return true; - } -} - -function getCacheMultiple(node, parentScope, self, singleCache) { - let scopes: Array = node[CACHE_MULTIPLE_KEY] = node[CACHE_MULTIPLE_KEY] || []; - - if (singleCache) { - // we have a scope assocation miss so push it onto our scopes - scopes.push(singleCache); - node[CACHE_SINGLE_KEY] = null; - } - - // loop through and check each scope to see if it matches our parent -======= ->>>>>>> Move scope cache to the cache module for (let scope of scopes) { if (scope.parent === parentScope) return scope; } From d5e78384ef0c078db46d3cb1f38de075003d3856 Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Mon, 7 Mar 2016 12:50:29 -0800 Subject: [PATCH 09/10] Only export methods and not the entire cache --- packages/babel-traverse/src/index.js | 14 ++++++++++++-- packages/babel-types/src/index.js | 5 +---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/packages/babel-traverse/src/index.js b/packages/babel-traverse/src/index.js index e641314145..8821cb510f 100644 --- a/packages/babel-traverse/src/index.js +++ b/packages/babel-traverse/src/index.js @@ -5,6 +5,7 @@ import * as visitors from "./visitors"; import * as messages from "babel-messages"; import includes from "lodash/collection/includes"; import * as t from "babel-types"; +import * as cache from "./cache"; export { default as NodePath } from "./path"; export { default as Scope } from "./scope"; @@ -39,7 +40,6 @@ traverse.explode = visitors.explode; traverse.NodePath = require("./path"); traverse.Scope = require("./scope"); traverse.Hub = require("./hub"); -traverse.cache = require("./cache"); traverse.cheap = function (node, enter) { if (!node) return; @@ -88,7 +88,7 @@ traverse.clearNode = function (node) { if (key[0] === "_" && node[key] != null) node[key] = undefined; } - traverse.cache.path.delete(node); + cache.path.delete(node); let syms: Array = Object.getOwnPropertySymbols(node); for (let sym of syms) { @@ -127,3 +127,13 @@ traverse.hasType = function (tree: Object, scope: Object, type: Object, blacklis return state.has; }; + +traverse.clearCache = function() { + cache.clear(); +}; + +traverse.copyCache = function(source, destination) { + if (cache.path.has(source)) { + cache.path.set(destination, cache.path.get(source)); + } +}; diff --git a/packages/babel-types/src/index.js b/packages/babel-types/src/index.js index 6c559d87aa..7211804bdd 100644 --- a/packages/babel-types/src/index.js +++ b/packages/babel-types/src/index.js @@ -404,11 +404,8 @@ export function inherits(child, parent) { } t.inheritsComments(child, parent); + traverse.copyCache(parent, child); - const pathCache = traverse.cache.path; - if (pathCache.has(parent)) { - pathCache.set(child, pathCache.get(parent)); - } return child; } From ec18fa0059287583ad5d029a2f42205751093de7 Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Mon, 7 Mar 2016 12:50:57 -0800 Subject: [PATCH 10/10] Add clearCache test --- packages/babel-traverse/test/traverse.js | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/babel-traverse/test/traverse.js b/packages/babel-traverse/test/traverse.js index b2cc301171..1e4f59b1f8 100644 --- a/packages/babel-traverse/test/traverse.js +++ b/packages/babel-traverse/test/traverse.js @@ -123,4 +123,26 @@ suite("traverse", function () { assert.ok(!traverse.hasType(ast, null, "ArrowFunctionExpression")); }); + + test("clearCache", function () { + var paths = []; + traverse(ast, { + enter: function (path) { + paths.push(path); + } + }); + + traverse.clearCache(); + + var paths2 = []; + traverse(ast, { + enter: function (path) { + paths2.push(path); + } + }); + + paths2.forEach(function (p, i) { + assert.notStrictEqual(p, paths[i]); + }); + }); });