Update pathCache in `NodePath#_replaceWith` #12391

JLHwung · 2020-11-24T04:44:41Z

Q	A
Fixed Issues?	Fixes #12386
Patch: Bug Fix?	Yes
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

This PR fixes a regression introduced in #12302, where we replaced the pathCache to Node-indexed Map. Before that PR, the pathCache is searched by comparing NodePath.node === targetNode, where targetNode is defined by container[key]. After we replaced it with Node-indexed map, whenever we assign new value to this.container[this.key], i.e. in replaceWithMultiple, we should also update the pathCache otherwise if we query the replaced node in pathCache, we lost the access of previously cached NodePath. Why? Because when we update this.node and this.container[this.key], we only update the values of the map-backed pathCache, which is analogous to the exact array-backed pathCache before #12302. Now that we replaced pathCache to a Map, we should also update the keys so that the replaced node points to the hosting NodePath.

Note that the second commit of #12302 3a365bf is exactly for such purpose, to remove this.node from the cache since we assign null to this.container[this.key]. Since we assign replacement to this.container[this.key] in NodePath#_replaceWith we should apply similar routine here. So we set a new cache for replacement and removed the old node cache entries.

As a summary, the side-effect introduced in #12302 is: whenever we update the targetNode (this.container[this.key]) of a created NodePath, we should also update the pathCache. I searched this.container[this.key] = usage in the codebase, it seems that there are only two of them so this fix seems to be complete.

Todo:

add tests to @babel/traverse. (I will be appreciated if you come up with such test and push to this branch, so we can merge 😄)

babel-bot · 2020-11-24T04:47:00Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/33175/

codesandbox-ci · 2020-11-24T04:51:01Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 307b003:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

JLHwung · 2020-11-24T05:04:53Z

packages/babel-traverse/src/path/replacement.js

@@ -50,7 +50,7 @@ export function replaceWithMultiple(nodes: Array<Object>) {
  nodes = this._verifyNodeList(nodes);
  t.inheritLeadingComments(nodes[0], this.node);
  t.inheritTrailingComments(nodes[nodes.length - 1], this.node);
-  pathCache.get(this.parent).delete(this.node);
+  pathCache.get(this.parent)?.delete(this.node);


This fix is inspired from ancestry commit that in some user cases the pathCache is nullish in NodePath#_replaceWith.

nicolo-ribaudo · 2020-11-24T13:44:45Z

packages/babel-traverse/test/conversion.js

@@ -51,6 +51,14 @@ describe("conversion", function () {
      expect(generateCode(rootPath)).toBe("() => {\n  return false;\n};");
    });

+    it("preserves arrow function body's context", function () {


Do you need another test, or is this enough? (Since there is the to-do item in the PR description)

I can work on it if needed.

This test is copied from the replaceWith test above, see also #12391 (comment). The second commit fixes another regression introduced in #12302 that is not yet reported.

I can work on it if needed.

Appreciated. Feel free to push 😄. I am still trying to figure out how we should test the pathCache behaviour in @babel/traverse.

nicolo-ribaudo

I'm cannot find a test case that is failing before this PR but working after 😅 In the worst case, we already have the optional chaining one which should prevent regressions.

However, I found another test case that fails. It's not related to this PR, but part of the bigger `path.context` management problem:

const out = babel.transformSync("[b]", {
  configFile: false,
  plugins: [
    () => ({
      visitor: {
        ExpressionStatement(path) {
          path.traverse({ Identifier(p) {}, noScope: true });

          const arr = path.get("expression");
          const x = arr.unshiftContainer("elements", [
            { type: "Identifier", name: "x" },
          ])[0];
          console.log("Has scope?", !!x.scope);
        },
      },
    }),
  ],
});

if you comment out the path.traverse call, this logs true. With the path.traverse, it logs false.

packages/babel-traverse/src/path/replacement.js

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

nicolo-ribaudo

This fix looks good. I tried creating a test again but it's super hard, and imho it shouldn't block this PR (since it already has "indirect" tests anyway).

JLHwung · 2020-11-24T18:56:16Z

packages/babel-traverse/test/replacement.js

@@ -112,6 +112,33 @@ describe("path/replacement", function () {
      });
      expect(visitCounter).toBe(1);
    });
+
+    // https://github.com/babel/babel/issues/12386
+    it("updates pathCache with the replaced node", () => {


I come up with a slightly simplified test from the regression example: 8d8782a#diff-f6b6fd6b98ff2127cf882c020e42e695555b277d7073252187299fb7c3f875a2

This test does not depend on the behaviour of proposal-optional-chaining and transform-typescript.

JLHwung · 2020-11-24T18:58:04Z

packages/babel-traverse/test/replacement.js

+      });
+      expect(generate(ast).code).toMatchInlineSnapshot(`
+        "() => {
+          return a.b.c;


As expected v7.12.7 returns

() => { return (a.b).c; };

here, which means the test captures the behaviour difference from the path cache.

* add tests on regression 12386 * fix: update cache on _replaceWith * fix: pathCache in replaceWithMultiple could be nullish * Update packages/babel-traverse/src/path/replacement.js Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> * test: add replaceWith test to traverse Co-authored-by: Brian Ng <bng412@gmail.com> Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

existentialism and others added 2 commits November 23, 2020 23:11

add tests on regression 12386

8d8782a

fix: update cache on _replaceWith

3949b40

JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: traverse i: regression labels Nov 24, 2020

fix: pathCache in replaceWithMultiple could be nullish

04d106c

JLHwung commented Nov 24, 2020

View reviewed changes

nicolo-ribaudo reviewed Nov 24, 2020

View reviewed changes

This was referenced Nov 24, 2020

Requeue parent path after processing optionals #12389

Closed

Bumping @babel/preset-env from 7.12.1 to 7.12.7 breaks tests (jest) #12393

Closed

nicolo-ribaudo reviewed Nov 24, 2020

View reviewed changes

packages/babel-traverse/src/path/replacement.js Outdated Show resolved Hide resolved

Update packages/babel-traverse/src/path/replacement.js

9b91f0f

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

nicolo-ribaudo approved these changes Nov 24, 2020

View reviewed changes

existentialism approved these changes Nov 24, 2020

View reviewed changes

JLHwung commented Nov 24, 2020

View reviewed changes

JLHwung force-pushed the issue12386 branch from fc63612 to 7cb15a5 Compare November 24, 2020 19:03

test: add replaceWith test to traverse

307b003

JLHwung force-pushed the issue12386 branch from 7cb15a5 to 307b003 Compare November 24, 2020 19:05

JLHwung merged commit eea3065 into babel:main Nov 24, 2020

JLHwung deleted the issue12386 branch November 24, 2020 20:07

pmdartus mentioned this pull request Nov 30, 2020

chore: Upgrade dev dependencies salesforce/lwc#2109

Merged

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 24, 2021

github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update pathCache in `NodePath#_replaceWith` #12391

Update pathCache in `NodePath#_replaceWith` #12391

JLHwung commented Nov 24, 2020 •

edited by gitpod-io bot

Loading

babel-bot commented Nov 24, 2020 •

edited

Loading

codesandbox-ci bot commented Nov 24, 2020 •

edited

Loading

JLHwung Nov 24, 2020

nicolo-ribaudo Nov 24, 2020 •

edited

Loading

JLHwung Nov 24, 2020

JLHwung Nov 24, 2020

nicolo-ribaudo left a comment

nicolo-ribaudo left a comment

JLHwung Nov 24, 2020

JLHwung Nov 24, 2020

Update pathCache in NodePath#_replaceWith #12391

Update pathCache in NodePath#_replaceWith #12391

Conversation

JLHwung commented Nov 24, 2020 • edited by gitpod-io bot Loading

babel-bot commented Nov 24, 2020 • edited Loading

codesandbox-ci bot commented Nov 24, 2020 • edited Loading

JLHwung Nov 24, 2020

Choose a reason for hiding this comment

nicolo-ribaudo Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

JLHwung Nov 24, 2020

Choose a reason for hiding this comment

JLHwung Nov 24, 2020

Choose a reason for hiding this comment

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

JLHwung Nov 24, 2020

Choose a reason for hiding this comment

JLHwung Nov 24, 2020

Choose a reason for hiding this comment

Update pathCache in `NodePath#_replaceWith` #12391

Update pathCache in `NodePath#_replaceWith` #12391

JLHwung commented Nov 24, 2020 •

edited by gitpod-io bot

Loading

babel-bot commented Nov 24, 2020 •

edited

Loading

codesandbox-ci bot commented Nov 24, 2020 •

edited

Loading

nicolo-ribaudo Nov 24, 2020 •

edited

Loading