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

Missing referencePaths after crawl #10896

Open
regiontog opened this issue Dec 19, 2019 · 10 comments · May be fixed by #11011
Open

Missing referencePaths after crawl #10896

regiontog opened this issue Dec 19, 2019 · 10 comments · May be fixed by #11011

Comments

@regiontog
Copy link

@regiontog regiontog commented Dec 19, 2019

Bug Report

  • I would like to work on a fix!

Current Behavior
scope.crawl() does not add references to bindings correctly in some cases. In particular I get the bug when referencing a class inside a class method.

Input Code
regiontog/babel@master...bug/missing-references-after-crawl

const path = getPath("class a { build() { return new a(); } }");
path.scope.crawl();
const referencePaths = path.scope.bindings.a.referencePaths;
expect(referencePaths).toHaveLength(1);

Expected behavior/code
I expect the binding a to have 1 reference inside the scope of the build class method after a crawl. The binding has 1 reference before the crawl call is made.

Environment

System:
    OS: Linux 5.4 Arch Linux
  Binaries:
    Node: 13.3.0 - /usr/bin/node
    Yarn: 1.21.0 - /usr/bin/yarn
    npm: 6.12.1 - /usr/bin/npm
  Monorepos:
    Lerna: 3.19.0
npmPackages:
    @babel/cli: ^7.7.0 => 7.7.5
    @babel/core: ^7.7.2 => 7.7.5
    @babel/eslint-plugin-development: ^1.0.1 => 1.0.1
    @babel/plugin-proposal-class-properties: ^7.7.0 => 7.7.4
    @babel/plugin-proposal-export-namespace-from: ^7.5.2 => 7.7.4
    @babel/plugin-proposal-nullish-coalescing-operator: ^7.4.4 => 7.7.4
    @babel/plugin-proposal-numeric-separator: ^7.2.0 => 7.7.4
    @babel/plugin-proposal-object-rest-spread: ^7.7.4 => 7.7.4
    @babel/plugin-proposal-optional-chaining: ^7.6.0 => 7.7.5
    @babel/plugin-transform-flow-strip-types: ^7.7.4 => 7.7.4
    @babel/plugin-transform-for-of: ^7.7.4 => 7.7.4
    @babel/plugin-transform-modules-commonjs: ^7.7.0 => 7.7.5
    @babel/plugin-transform-runtime: ^7.6.2 => 7.7.6
    @babel/preset-env: ^7.7.1 => 7.7.6
    @babel/preset-flow: ^7.0.0 => 7.7.4
    @babel/register: ^7.7.0 => 7.7.4
    @babel/runtime: ^7.7.2 => 7.7.6
    babel-eslint: ^11.0.0-beta.2 => 11.0.0-beta.2
    babel-jest: ^24.9.0 => 24.9.0
    babel-plugin-transform-charcodes: ^0.2.0 => 0.2.0
    eslint: ^6.0.1 => 6.7.2
    eslint-config-babel: ^9.0.0 => 9.0.0
    gulp-babel: ^8.0.0 => 8.0.0
    jest: ^24.9.0 => 24.9.0
    lerna: ^3.19.0 => 3.19.0
    rollup-plugin-babel: ^4.0.0 => 4.3.3
  • How you are using Babel: API

Possible Solution

It seems suspect to me to add the reference to a binding in the references own scope here

Additional context/Screenshots
Output of test linked above:

scope › binding paths › reference paths after crawl

    expect(received).toHaveLength(expected)

    Expected length: 1
    Received length: 0
    Received array:  []

      276 |       path.scope.crawl();
      277 |       const referencePaths = path.scope.bindings.a.referencePaths;
    > 278 |       expect(referencePaths).toHaveLength(1);
          |                              ^
      279 |     });
      280 |   });
      281 |

      at Object.<anonymous> (packages/babel-traverse/test/scope.js:278:30)
@babel-bot

This comment has been minimized.

Copy link
Collaborator

@babel-bot babel-bot commented Dec 19, 2019

Hey @regiontog! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

@JLHwung

This comment has been minimized.

Copy link
Contributor

@JLHwung JLHwung commented Jan 9, 2020

Hi @regiontog, thanks for your report!

This issue comes from how babel registers bindings for the id of ClassDeclaration

ClassDeclaration(path) {
const id = path.node.id;
if (!id) return;
const name = id.name;
path.scope.bindings[name] = path.scope.getBinding(name);
},

As you can see the binding variable is referencing the returned result from path.scope.getBinding - That means if the binding is not defined in this scope, it will walk up until it finds one.

Now let's take a look at your example

const path = getPath("class a { build() { return new a(); } }");
path.scope.crawl();

Before crawling, the binding a can be accessed in the following two nodes:

Program.bindings.a
ClassDeclaration.bindings.a ==> Program.bindings.a

Keep in mind that scope.crawl() will reset Program.bindings to {}, but the old Program.bindings.a would not be GCed because it is still referenced in ClassDeclaration.bindings.a. Therefore after crawling the binding a can still be accessed in the following two nodes.

Program.bindings.a
ClassDeclaration.bindings.a ==> ClassDeclaration.bindings.a before crawl
                              (getBinding returns existent old binding info)
==> Program.bindings.a before crawl

But ClassDeclaration.bindings.a refers to the binding generated before scope.crawl(), so the referencePaths is screwed up because it is assigned to the binding before crawl.

We can walk up from path.scope.parent instead of path.scope, so that we avoid accessing obsolete binding information.

@existentialism

This comment has been minimized.

Copy link
Member

@existentialism existentialism commented Jan 10, 2020

@JLHwung sidenote, fantastic explanation :)

@mischnic

This comment has been minimized.

Copy link

@mischnic mischnic commented Jan 11, 2020

I'll close my old issue #9327 as a duplicate.

@JLHwung

This comment has been minimized.

Copy link
Contributor

@JLHwung JLHwung commented Jan 14, 2020

friendly ping @regiontog, let me know if you are working on this. Should you have questions on developing a fix, please ask in the #development slack channel.

@regiontog

This comment has been minimized.

Copy link
Author

@regiontog regiontog commented Jan 14, 2020

Thanks, I've been busy this week, but I was planning on taking a look tomorrow since I have the day off. I don't mind if someone else wants to take a look either.

regiontog added a commit to regiontog/babel that referenced this issue Jan 14, 2020
@regiontog

This comment has been minimized.

Copy link
Author

@regiontog regiontog commented Jan 14, 2020

@JLHwung You were right, the fix is exactly as you suggested. The only thing that is not entirely clear to me is when the parent scope gets the a binding. I assume previously in the traverse if it is top-to-bottom. But I don't see a Program visitor.

Nevertheless my test passes now, I'll check if the bug in parcel is resolved by the same change. I can submit a PR with the fix and the test if you guys are ok with the change.

Edit: https://github.com/regiontog/parcel2-bug builds fine with this fix applied to babel.

@JLHwung

This comment has been minimized.

Copy link
Contributor

@JLHwung JLHwung commented Jan 14, 2020

The parent scope will get the a binding in the Declaration visitor

Declaration(path) {
// delegate block scope handling to the `BlockScoped` method
if (path.isBlockScoped()) return;
// this will be hit again once we traverse into it after this iteration
if (path.isExportDeclaration() && path.get("declaration").isDeclaration()) {
return;
}
// we've ran into a declaration!
const parent =
path.scope.getFunctionParent() || path.scope.getProgramParent();
parent.registerDeclaration(path);
},
, since ClassDeclaration is also Declaration. Its parent scope will be registered with newly created binding info.

Looking into this code I think it maybe more clear to merge ClassDeclaration visitor with Declaration visitor, so we define an extra binding in the class scope for the class id so that the class method can reference it.

@regiontog

This comment has been minimized.

Copy link
Author

@regiontog regiontog commented Jan 14, 2020

Looking into this code I think it maybe more clear to merge ClassDeclaration visitor with Declaration visitor, so we define an extra binding in the class scope for the class id so that the class method can reference it.

My only problem with the current approach that it presumably depends on the Declaration visitor being called before the ClassDeclaration visitor. I don't know if that is by design since Declaration is less specific or not. But if that's not a contract traverse must uphold then depending on it might not be a good idea.

@JLHwung

This comment has been minimized.

Copy link
Contributor

@JLHwung JLHwung commented Jan 15, 2020

My only problem with the current approach that it presumably depends on the Declaration visitor being called before the ClassDeclaration visitor.

The Declaration visitor is called before ClassDeclaration because babel-traverse explode the visitors according to the Object.keys:

for (const nodeType of Object.keys(visitor)) {

I admit that this is too subtle and essentially the object keys are unordered. So here we are on the same page. I propose to merge the ClassDeclaration visitor with Declaration visitor:

Declaration(path) {
  // register binding on parent scope
  // ...
  // register class identifier binding in class scope
  if (t.isClassDeclaration(path)) {
    // ...
  }
}

and then we don't have to worry about the execution order of Declaration and ClassDeclaration.

@regiontog regiontog linked a pull request that will close this issue Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.