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

Introduce scope tracking in the parser #9493

Merged
merged 27 commits into from Feb 25, 2019

Conversation

@danez
Copy link
Member

danez commented Feb 11, 2019

Q                       A
Fixed Issues? Fixes #6722 (Fixes maybe #6283) Fixes #9316 Fixes #4331
Patch: Bug Fix? y
Major: Breaking Change? n
Minor: New Feature? n
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT (acorn MIT)

This introduces scope tracking in the parser. The scope tracking replaces some of the Flags we currently have (inGenerator, inFunction, ...).

Spec changes:

  • We now report an syntax error if super is used even though there is no superClass
  • Duplicate bindings will throw according to spec

The idea here is to enter and exit scopes, during parsing so we always can check the current scope. A scope consists of flags (which adds information we need in several places like SCOPE_FUNCTION, SCOPE_ASYNC, ...) and 3 collections of declared bindings in this scope (var, lexical(let, const, classes) and functions).

TODO:

  • Add tests for duplicate variable bindings.
  • Migrate some traverse tests (duplicate declarations) to not use the parser

Big thanks to acorn, as most of this here is ported from acorn. :) Same goes for copyright on most of it.

@nicolo-ribaudo nicolo-ribaudo self-requested a review Feb 11, 2019
@existentialism existentialism self-requested a review Feb 11, 2019
@danez danez added the PR: WIP label Feb 11, 2019
@danez danez added this to In progress in Parser: Spec Compliance, Refactoring and Performance via automation Feb 12, 2019
@danez danez force-pushed the danez:scope-tracking branch from 791ac8d to 786b6b1 Feb 13, 2019
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Feb 13, 2019

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

@danez danez force-pushed the danez:scope-tracking branch from a36341b to 620249c Feb 15, 2019
@danez

This comment has been minimized.

Copy link
Member Author

danez commented Feb 15, 2019

We have some tests that test the duplicate declaration checks of babel-traverse, but as the parser now throws first we are not getting to the point of testing duplicate declarations in traverse.

I see 3 options:

  1. Leave the tests as is and don't care that we are not testing traverse.
  2. Add an option to the parser to disable the duplicate check.
  3. Move the tests to the parser (as thats what they are testing now) but also recreate new tests for traverse which do not use the parser but node builders instead to create the AST.

1 and 2 are not really good options for me. So I guess I will go for 3 which is the most work but the best I feel? Any other ideas on this?

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Feb 15, 2019

👍 for 3. We could ask help to the community to rewrite the @babel/traverse tests.

@danez danez force-pushed the danez:scope-tracking branch from 498341f to 464ec62 Feb 15, 2019
@danez

This comment has been minimized.

Copy link
Member Author

danez commented Feb 15, 2019

👍 for 3. We could ask help to the community to rewrite the @babel/traverse tests.

Turns out it was only 4 tests. Probably took longer to write the message above, than it will take to fix them :D

@nicolo-ribaudo nicolo-ribaudo dismissed their stale review Feb 17, 2019

Whoops, wrong PR. I meant to approve #9522

Parser: Spec Compliance, Refactoring and Performance automation moved this from Reviewed to In progress Feb 17, 2019
@nicolo-ribaudo nicolo-ribaudo self-requested a review Feb 17, 2019
@danez danez force-pushed the danez:scope-tracking branch from 005e13a to bbc61f0 Feb 20, 2019
@danez danez removed the PR: WIP label Feb 20, 2019
},
},
]);

This comment has been minimized.

Copy link
@danez

danez Feb 20, 2019

Author Member

Not sure if i should leave that or not. If this check would be in traverse I would leave it, but in the transform, the only valid usecase now would be someone creating in a plugin invalid code and then running the transform.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Feb 20, 2019

Member

I don't think that we need it: currently all the other early errors (except for the duplicated bindings errors) are only in @babel/parser.

Copy link
Member

nicolo-ribaudo left a comment

I think that we should also add binding types for Flow and TS types, and check if they are duplicated.

In flow types and vars share the same namespace (type A = string; var A is an error), while in TypeScript they are in different namespaces. type A = string; type A = number; is an error in both languages.

packages/babel-parser/src/util/scopeflags.js Outdated Show resolved Hide resolved
packages/babel-parser/src/parser/lval.js Outdated Show resolved Hide resolved

// The functions in this module keep track of declared variables in the
// current scope in order to detect duplicate variable names.
export default class ScopeParser extends UtilParser {

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Feb 21, 2019

Member

Since ScopeParser is pretty much its own thing, maybe we could use it like this.scope.* instead of adding it to the prototype chain, like we do for State?

This comment has been minimized.

Copy link
@danez

danez Feb 21, 2019

Author Member

I changed it to be a separate class, which works and might be nice to have like this if we would want to reuse this parts in traverse for example.

But when looking into flow I noticed another problem: extending the scope tracking is not as easy possible with the external class. So for typescript we probably would want to add another binding group for types. Flow has some custom rules too when it comes to declare class|var|function. I'm not sure if we actually want to support extending the scope tracking but it might be nice.

packages/babel-parser/src/parser/statement.js Outdated Show resolved Hide resolved
packages/babel-parser/src/parser/statement.js Outdated Show resolved Hide resolved
packages/babel-parser/src/parser/expression.js Outdated Show resolved Hide resolved
@danez danez force-pushed the danez:scope-tracking branch from 6996482 to 680468b Feb 21, 2019
danez added 4 commits Feb 12, 2019
danez added 2 commits Feb 22, 2019
…cate
@danez danez force-pushed the danez:scope-tracking branch from 932b7ea to ae97eda Feb 22, 2019
@danez

This comment has been minimized.

Copy link
Member Author

danez commented Feb 22, 2019

Okay so I added fixes for flow:

  • declare module {} has it's own scope now
  • type and opaque type are now registered as lexical declaration, so the are handled the same as let or const

I also added tests for TS to ensure types do not count as duplicate declarations of other lexical declarations.

Any further additions for flow and typescript need changes to the logic in how duplicate declarations are checked. But the problem is that there is no easy way to extend the scope handler, as any addition (for example a new scope group for typescript type declarations) will need adjustments to all other parts of the scope tracking logic, so that lexical and function declarations are checked correctly against for example flow interfaces.
The only way to achieve correct scope tracking for flow and TS is to check on initialization which language we are parsing and depending on that choose a corresponding ScopeHandler. This ScopeHandlers for JS, TS and flow ultimately will need to be copy and pasted and kept in sync for bug fixes as I do not see a way to easy overwrite a subset of the logic of the duplicate detection algorithm for above reasons.

For now this PR does not introduce any duplicate checks for flow an ts, besides the ones mentions above, and we can follow up in a separate PR.

Copy link
Member

nicolo-ribaudo left a comment

This PR looks good, I left some last minor comments.

  • Could you add a test that the first code throws while the second doesn't, in script scope? (I couldn't find it, but maybe it already exists? I searched for var in the diff)
    {
      function f() {}
      var f;
    }
    function f() {}
    var f;
packages/babel-parser/src/parser/statement.js Outdated Show resolved Hide resolved
packages/babel-parser/src/parser/util.js Outdated Show resolved Hide resolved
danez added 4 commits Feb 22, 2019
Parser: Spec Compliance, Refactoring and Performance automation moved this from In progress to Reviewed Feb 23, 2019
@danez danez added this to the v7.4.0 milestone Feb 24, 2019
@danez danez merged commit a739114 into babel:master Feb 25, 2019
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 86.98% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Parser: Spec Compliance, Refactoring and Performance automation moved this from Reviewed to Done Feb 25, 2019
@danez danez deleted the danez:scope-tracking branch Feb 25, 2019
alanshaw added a commit to pull-stream/stream-to-pull-stream that referenced this pull request Mar 20, 2019
This started causing problems when babel released this change yesterday babel/babel#9493

resolves #15
alanshaw added a commit to ipfs/js-ipfs that referenced this pull request Mar 20, 2019
Pending resolution of pull-stream/stream-to-pull-stream#16. The extra `destroy` function causes babel to throw after babel/babel#9493 was merged and released.

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
alanshaw added a commit to ipfs/js-ipfs that referenced this pull request Mar 20, 2019
Pending resolution of pull-stream/stream-to-pull-stream#16. The extra `destroy` function causes babel to throw after babel/babel#9493 was merged and released.

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@lock lock bot added the outdated label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.