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.

Sign up for GitHub

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

Jump to bottom

Bug/missing references after crawl #11011

Merged
merged 7 commits into from Jan 20, 2020
Merged

Bug/missing references after crawl #11011

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
804969c
Bug replication test
regiontog Dec 19, 2019
48a7790
Simplify test
regiontog Dec 19, 2019
37af725
Merge branch 'master' into bug/missing-references-after-crawl
regiontog Jan 14, 2020
5c0b0ac
Fixes #10896
regiontog Jan 14, 2020
63a12f2
Merge path.crawl `ClassDeclaration` and `Declaration` visitors
regiontog Jan 15, 2020
92b4192
Move registartion of class id in crawl to BlockScoped
regiontog Jan 18, 2020
725e959
Add some assertions to crawl test
regiontog Jan 18, 2020
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
16 changes: 8 additions & 8 deletions packages/babel-traverse/src/scope/index.js
View file
Open in desktop
Expand Up @@ -72,6 +72,14 @@ const collectorVisitor = {
const parent =
path.scope.getFunctionParent() || path.scope.getProgramParent();
parent.registerDeclaration(path);

// Register class identifier in class' scope if this is a class declaration.
if (path.isClassDeclaration() && path.node.id) {
const id = path.node.id;
const name = id.name;

path.scope.bindings[name] = parent.getBinding(name);
}
},

ReferencedIdentifier(path, state) {
Expand Down Expand Up @@ -131,14 +139,6 @@ const collectorVisitor = {
scope.getBlockParent().registerDeclaration(path);
},

ClassDeclaration(path) {
const id = path.node.id;
if (!id) return;

const name = id.name;
path.scope.bindings[name] = path.scope.getBinding(name);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the following change all that's necessary?

-    path.scope.bindings[name] = path.scope.getBinding(name);
+    path.scope.bindings[name] = path.scope.parent.getBinding(name);

Copy link
Member

Choose a reason for hiding this comment

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

I agree. parent is the nearest function scope, but classes are block-scoped:

let A;
function fn() {
  {
    class A {}
  }
  A; // This references the let declaration, not the path.
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This change

-    path.scope.bindings[name] = path.scope.getBinding(name);
+    path.scope.bindings[name] = path.scope.parent.getBinding(name);

implicitly depends on the fact that the Declaration visitor runs before ClassDeclaration visitor, so that path.scope.parent.getBinding always points to the new binding which may be initialized in Declaration visitor or BlockScoped visitor.

See #10896 (comment)

Besides that, I agree we should do path.scope.parent instead of parent.

Copy link
Contributor Author

@regiontog regiontog Jan 16, 2020 •
edited

Choose a reason for hiding this comment

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

implicitly depends on the fact that the Declaration visitor runs before ClassDeclaration visitor, so that path.scope.parent.getBinding always points to the new binding which may be initialized in Declaration visitor or BlockScoped visitor.

If the class binding may be(always is?) initialized in BlockScoped won't this stop the identifier from being visible inside the class scope if the visitors are merged?

babel/packages/babel-traverse/src/scope/index.js

Lines 62 to 64 in 63a12f2

Declaration(path) {
// delegate block scope handling to the `BlockScoped` method
if (path.isBlockScoped()) return;

Copy link
Contributor

Choose a reason for hiding this comment

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

@regiontog Ah you are right! The ClassDeclaration binding is registered in BlockScoped and BlockScoped runs before ClassDeclaration. I am curious why the test can even get passed.

},

Block(path) {
const paths = path.get("body");
for (const bodyPath of (paths: Array)) {
Expand Down
7 changes: 7 additions & 0 deletions packages/babel-traverse/test/scope.js
View file
Open in desktop
Expand Up @@ -289,6 +289,13 @@ describe("scope", () => {
column: 32,
});
});

it("reference paths after crawl", function() {
const path = getPath("class a { build() { return new a(); } }");
path.scope.crawl();
const referencePaths = path.scope.bindings.a.referencePaths;
expect(referencePaths).toHaveLength(1);
});
});

describe("duplicate bindings", () => {
Expand Down