Introduce scope tracking in the parser by danez · Pull Request #9493 · babel/babel

danez · 2019-02-11T11:08:33Z

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.

babel-bot · 2019-02-13T09:11:28Z

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

danez · 2019-02-15T09:36:41Z

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:

Leave the tests as is and don't care that we are not testing traverse.
Add an option to the parser to disable the duplicate check.
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 · 2019-02-15T09:52:23Z

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

danez · 2019-02-15T10:49:11Z

👍 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

Whoops, wrong PR. I meant to approve #9522

danez · 2019-02-20T20:07:57Z

packages/babel-plugin-transform-classes/src/transformClass.js

-    },
-  },
-]);
-


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.

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.

nicolo-ribaudo

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

packages/babel-parser/src/parser/lval.js

nicolo-ribaudo · 2019-02-20T23:43:54Z

packages/babel-parser/src/parser/scope.js

+
+// 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 {


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?

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

packages/babel-parser/src/parser/expression.js

…s lexical declarations

…cate

danez · 2019-02-22T09:20:49Z

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.

nicolo-ribaudo

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

packages/babel-parser/src/parser/util.js

This started causing problems when babel released this change yesterday babel/babel#9493 resolves #15

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>

danez added PR: Spec Compliance 👓 A type of pull request used for our changelog categories pkg: parser labels Feb 11, 2019

nicolo-ribaudo self-requested a review February 11, 2019 11:14

existentialism self-requested a review February 11, 2019 15:40

nicolo-ribaudo mentioned this pull request Feb 11, 2019

Disallow super inside base classes #9316

Closed

danez added the PR: WIP label Feb 11, 2019

danez mentioned this pull request Feb 16, 2019

Make tests spec compliant by avoiding duplicate declarations in input files #9522

Merged

nicolo-ribaudo previously approved these changes Feb 17, 2019

View reviewed changes

nicolo-ribaudo self-requested a review February 17, 2019 10:32

danez removed the PR: WIP label Feb 20, 2019

danez commented Feb 20, 2019

View reviewed changes

nicolo-ribaudo reviewed Feb 21, 2019

View reviewed changes

danez added 13 commits February 22, 2019 01:15

Introduce scope tracking

53a4734

Fix tests

34e8d60

Add new tests

ce309e1

Remove constructor-super check from transform as it is now in parser

a881a6a

Correctly handle class properties and class scope

b3a9724

Fix duplicate name check

3d8731e

Convert scope identifier storage to array

730d181

Enter a new scope in typescript module blocks

a4266df

Add test for duplicate declaration

9a331a8

Rename error for duplicate exports

a9df489

Treat class declarations as lexical declaration

b98794f

Update whitelist

7db208e

Add tests

8ff404c

danez added 6 commits February 22, 2019 01:15

One more test

937d9c2

Make scope a separate class and fix review comments

4321f11

Do not allow new.target in top scope arrow function

b2b235e

Correctly enter new scope for declare module and treat type aliases a…

3fb7672

…s lexical declarations

Tests for typescript scope tracking to not mark type aliases as dupli…

4278c5a

…cate

Fix flow scope tracking

ae97eda

danez added the PR: Needs Review A pull request awaiting more approvals label Feb 22, 2019

nicolo-ribaudo reviewed Feb 22, 2019

View reviewed changes

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 February 22, 2019 15:59

Remove ident from test names as redundant

c5d62da

Add test case for var and function

0d00d37

Improve error messages

5c92055

Improve literal regex

9aa3d12

nicolo-ribaudo approved these changes Feb 23, 2019

View reviewed changes

existentialism approved these changes Feb 24, 2019

View reviewed changes

danez added this to the v7.4.0 milestone Feb 24, 2019

danez added PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release and removed PR: Needs Review A pull request awaiting more approvals labels Feb 24, 2019

danez merged commit a739114 into babel:master Feb 25, 2019

danez deleted the scope-tracking branch February 25, 2019 19:04

alanshaw mentioned this pull request Mar 20, 2019

docs: add DHT config documentation ipfs/js-ipfs#1951

Merged

alanshaw pushed a commit to pull-stream/stream-to-pull-stream that referenced this pull request Mar 20, 2019

fix: remove unused destroy

404365b

This started causing problems when babel released this change yesterday babel/babel#9493 resolves #15

alanshaw mentioned this pull request Mar 20, 2019

fix: remove unused destroy pull-stream/stream-to-pull-stream#16

Merged

alanshaw mentioned this pull request Mar 20, 2019

chore: update stream-to-pull-stream dependency ipfs/js-ipfs#1952

Merged

tanhauhau mentioned this pull request Jun 23, 2019

declare name for flow interface #10100

Closed

JLHwung mentioned this pull request Jul 15, 2019

Flow: interface identifier should be declared in the scope #10220

Merged

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019

Uh oh!

Conversation

danez commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danez commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo commented Feb 15, 2019

Uh oh!

danez commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danez Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicolo-ribaudo Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

danez Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danez commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danez commented Feb 11, 2019 •

edited

Loading

babel-bot commented Feb 13, 2019 •

edited

Loading

danez commented Feb 15, 2019 •

edited

Loading

danez commented Feb 15, 2019 •

edited

Loading

danez commented Feb 22, 2019 •

edited

Loading