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

Update pathCache in NodePath#_replaceWith #12391

Merged
merged 5 commits into from Nov 24, 2020
Merged

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Nov 24, 2020

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 😄)

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: traverse i: regression labels Nov 24, 2020
@babel-bot
Copy link
Collaborator

babel-bot commented Nov 24, 2020

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 24, 2020

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

@@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

it("preserves arrow function body's context", function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@@ -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", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
expect(generate(ast).code).toMatchInlineSnapshot(`
"() => {
return a.b.c;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As expected v7.12.7 returns

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

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

@JLHwung JLHwung merged commit eea3065 into babel:main Nov 24, 2020
@JLHwung JLHwung deleted the issue12386 branch November 24, 2020 20:07
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Dec 2, 2020
* 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>
@github-actions 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 github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: regression outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: traverse PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript non-null assertion operator not stripped off with deep optional chaining
4 participants