Could not traverse ClassPrivateMethod when estree plugin enabled

[coderaiser]

Bug Report

Current Behavior
When I'm using estree and classPrivateMethods and try to traverse ClassPrivateMethod I get crash:

coderaiser@cloudcmd:~/issues$ node traverse.js
/home/coderaiser/issues/node_modules/@babel/traverse/lib/scope/index.js:667
      for (const param of params) {
                          ^

TypeError: params is not iterable
    at Scope.crawl (/home/coderaiser/@iocmd/issues/babel-traverse/node_modules/@babel/traverse/lib/scope/index.js:667:27)
    at Scope.init (/home/coderaiser/@iocmd/issues/babel-traverse/node_modules/@babel/traverse/lib/scope/index.js:634:32)
    at NodePath.setScope (/home/coderaiser/@iocmd/issues/babel-traverse/node_modules/@babel/traverse/lib/path/context.js:126:30)
    at NodePath.setContext (/home/coderaiser/@iocmd/issues/babel-traverse/node_modules/@babel/traverse/lib/path/context.js:141:8)
    at NodePath.pushContext (/home/coderaiser/@iocmd/issues/babel-traverse/node_modules/@babel/traverse/lib/path/context.js:207:8)
    at TraversalContext.visitQueue (/home/coderaiser/@iocmd/issues/babel-traverse/node_modules/@babel/traverse/lib/context.js:106:14)
    at TraversalContext.visitMultiple (/home/coderaiser/@iocmd/issues/babel-traverse/node_modules/@babel/traverse/lib/context.js:85:17)
    at TraversalContext.visit (/home/coderaiser/@iocmd/issues/babel-traverse/node_modules/@babel/traverse/lib/context.js:144:19)
    at Function.traverse.node (/home/coderaiser/@iocmd/issues/babel-traverse/node_modules/@babel/traverse/lib/index.js:94:17)
    at NodePath.visit (/home/coderaiser/@iocmd/issues/babel-traverse/node_modules/@babel/traverse/lib/path/context.js:95:18)

Input Code

• REPL or Repo link if applicable:

const babel = require('@babel/parser');
const traverse = require('@babel/traverse').default;

const source = `
    class User {
        #set() {
        }

        #get() {
        }

        get() {
            return this.#get();
        }
    }
`;

const babelAST = babel.parse(source, {
    plugins: [
        'estree',
        'classPrivateMethods',
    ],
});

traverse(babelAST, {
    enter(path) {
        console.log('+');
    }
});

Expected behavior/code
I expected no crash, regular traverse. Some time ago everything worked good, but after update everything is broke.

Environment
@babel/parser: v7.3.2
@babel/traverse: v7.2.3

Possible Solution

params is empty in ClassPrivateMethod so path.get('params') returns single path, not an array. MethodDefinition also has no params but it works good and not processed by this code.

[babel-bot]

Hey @coderaiser! 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.

[nicolo-ribaudo]

@babel/traverse doesn't aim to be compatible with estree, so we will never guarantee that it works.

Anyway, this inconsistency between public and private methods should be fixed. Demo: https://astexplorer.net/#/gist/3264a96e825cb7114c07221e47c08471/8b4e4ed7563e05cd0fbbdd4f5e7f15b6936f167b

Public methods are made ESTree-compatible at https://github.com/babel/babel/blob/master/packages/babel-parser/src/plugins/estree.js#L200. We should do the same thing, but first estree/estree#180 should be merged.

[coderaiser]

Fixed with https://github.com/coderaiser/estree-to-babel/releases/tag/v1.8.0.

[nicolo-ribaudo]

We will need to update @babel/parser's estree plugin when the estree PR is merged.

[satazor]

@nicolo-ribaudo @coderaiser I’m a bit confused as it was said that the issue was fixed but I’m still getting it. Is it fixed in master but wasn’t yet released?

[nicolo-ribaudo]

No, It has been worked around in a separate repo. It still need to be fixed in Babel, but we are blocked by estree/estree#180

[coderaiser]

@satazor you can use estree-to-babel to have ability to use @babel/traverse when in estree mode of babel.

[satazor]

@coderaiser I'm getting this error in the context of https://github.com/babel/babel-eslint. Is there any way to use estree-to-babel in this scenario?

[coderaiser]

@satazor babel-eslint uses bablylon-to-espree, it is the opposite of estree-to-babel, so I don't think that it will help.

[JounQin]

Just a question, when I manually change `opts.parserOpts.plugins = ['@babel/proposal-class-properties', '@babel/proposal-private-methods']` before `ast = parse(code, opts)`, the `parse` function will not throw error any more. Is it as expected?

Sorry, I just found out estree plugin is the key point.

[kaicataldo]

Trying to resolve this for #9506, and it sounds like we need to decide what our policy is re experimental features and ESTree. The ESTree spec generally doesn't add language features until they hit stage 4 (see estree/estree#204 as an example). Waiting for the ESTree spec makes the @babel/eslint-* packages redundant, because ESLint core adds in support for language features when they hit stage 4 and are defined in the ESTree spec.

How do we want to proceed? This is actually a big part of why ESLint core doesn't support experimental features - it would mean we would have to change any differences in ASTs if they're different when they're finalized.

Seems like the ideal way forward would be for ESTree to develop working/living specs for experimental features, and then we could update our semver policy for the @babel/eslint-* and @babel/parser packages to reserve the right to update the ESTree plugin and any other necessary changes to handle the changed AST in a semver-minor release. I don't know if this is really possible though.

[nicolo-ribaudo]

I propose this policy:

1. Only adapt things in the estree @babel/parser plugin once they are part of the ESTree specification
2. Experimental features use the normal Babel AST, even when estree is enabled
3. If needed (for example, for 11.0.0-beta.0 - Private Instance Methods - Parsing error: params is not iterable babel-eslint#749), the experimental Babel AST can be adapted by @babel/eslint-parser so that it works with ESLint's existing features
4. When they became stage 4, we can "fix" the estree plugin and make it compliant. Every bugfix is technically breaking, and while this could break anyone relying on the old behavior it's just because the old behavior wasn't doing what they asked @babel/parser to do (i.e. follow the ESTree spec)

I think that we should never have an AST node which is neither Babel's AST nor ESTree: we don't need a new AST format.

[kaicataldo]

Wanted to check in with others who might be more knowledgeable about this. This error is thrown during the initial parsing, and I'm not seeing a way to easily access the @babel-traverse.Scope class to subclass it. Would it make sense to try to expose this somehow? Or do we want to try to monkeypatch it in @babel/eslint-parser for now instead of modifying the public API of @babel/core? Please let me know if I'm missing something!

[nicolo-ribaudo]

Does @babel/eslint-parser use @babel/traverse? Doesn't it only use @babel/core's .parse method?

[kaicataldo]

I've actually been trying to track this down, because I was confused by this too. It looks like it does do some scope analysis at parse time. Here's a stack trace I logged by manually throwing an error when the Scope class is instantiated:

at new Scope (/Users/kai/Code/babel/packages/babel-traverse/lib/scope/index.js:174:17)
at NodePath.getScope (/Users/kai/Code/babel/packages/babel-traverse/lib/path/index.js:122:29)
at NodePath.setScope (/Users/kai/Code/babel/packages/babel-traverse/lib/path/context.js:130:21)
at NodePath.setContext (/Users/kai/Code/babel/packages/babel-traverse/lib/path/context.js:147:8)
at new File (/Users/kai/Code/babel/packages/babel-core/lib/transformation/file/file.js:106:8)
at normalizeFile (/Users/kai/Code/babel/packages/babel-core/lib/transformation/normalize-file.js:151:10)
at parseSync (/Users/kai/Code/babel/packages/babel-core/lib/parse.js:56:37)
at parse (/Users/kai/Code/babel/eslint/babel-eslint-parser/src/parse.js:10:11)
at _default (/Users/kai/Code/babel/eslint/babel-eslint-parser/src/parse-with-scope.js:6:15)
at Object.parseForESLint (/Users/kai/Code/babel/eslint/babel-eslint-parser/src/index.js:25:10

For reference, this is where the error is thrown.

[nicolo-ribaudo]

Ok, calling babel.parse definitely shouldn't do any scope analysis. It should only call parser.parse + all the config loading logic.

[kaicataldo]

Yeah, that makes sense to me. I'll keep digging and see if we need to update babel.parse().

[danez]

I have the same problem in react-docgen which only tries to parse a file with a private method.

Is this fixed with #10914?

[kaicataldo]

The error here should be fixed, yes!