New: Allow parsers to supply Visitor Keys/Fallback (fixes #8392)

[soda0289]

What is the purpose of this pull request? (put an "X" next to item)
[ X ] Add something to the core

Still need to write tests just need feedback on implementation. This would allow for typescript-eslint-parser to fix issues with the no-undef rule and allow rules to traverse decorators and type parameters.

What changes did you make? (Give an overview)
Modified the Traverser class to accept an optional VisitorKey object and VisitorFallback function. These are then passed into eslint-scope and estravese.

Updated the parseForESLint function to have two extra options:

• visitorKeys
• visitorFallback

These values are passed into the Traverser constructor.

Is there anything you'd like reviewers to focus on?
Currently the visitorKeys passed into eslint, with the parseForESLint(), modifies the VisitorKeys by replacing a key. This is the default behavior of both estraverse and eslint-scope. I think this should be fine as it allows parsers to both add and delete visitor properties but maybe there is better way.

Both VisitorKeys and VisitorFallback are passed into Traverser but it could be its own class that wraps them both.

[eslintbot]

LGTM

[mention-bot]

@soda0289, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @mysticatea and @roadhump to be potential reviewers.

[kaicataldo]

Thanks for working on this! I'm fine with this implementation.

I'd like to re-evaluate the current monkeypatching of type annotations In light of the changes being made in this PR. Since we don't support parsing of types by default, I wonder if it might actually make more sense to revert the monkeypatching I added and have parsers that support types supply VisitorKeys instead. That way the responsibility for types isn't split between the monkeypatched estraverse.VisitorKeys and the parser-supplied VisitorKeys.

We could still update core rules to account for types should they exist (as we do with JSX nodes), since there is an official ESTree extension for them.

@eslint/eslint-team Thoughts? If we decide we want to revert the monkeypatching I think we should do so soon, since we're stilll doing alpha releases of v4.

[soda0289]

@kaicataldo I also believe we should remove type annotations from being traversed.

Type annotations are not defined as a node by ESTree only where they should be and each parser may do things differently. So it makes sense for a parser to define its own visitor keys for type annotations. A parser may also have type annotations in places that ESLint would never traverse anyway.

I think it might conflict with #8465 but since it only runs once when the traverser is initialized it should be.

[kaicataldo]

Made a PR reverting my previous commits so that we can discuss: #8584

[mysticatea]

My position is:

I thought the enhancement of only VisitorKeys is not enough because custom parsers should be able to provide new syntax which has new scopes.

#8392 (comment)

[soda0289]

@mysticatea You are correct that this change is not enough to allow new node types to be scoped but it still reduces number incorrect errors rules likes no-undef throw and allows for rules to be written for new node types. Plus it gives us more time to come up with a better long term solution to eslint-scope instead of forcing one into this change.

Currently you can not write rules for decorators, type annotations, or type parameters because they are children of known nodes. This would be fixed by this change.

The visitor fallback that we currently use will traverse every single property of unknown nodes this causes eslint-scope to mark code like the following as failing:

interface Foo {
  bar: string;
}

It complains that Foo is undefined, but in this case it is a declaration. This change would allow this to be fixed since parsers could supply their own visitor fallback that does not traverse these unsupported nodes. It is not perfect but it is better then doing nothing.

Even when we update eslint-scope there are still parts of code-path-analysis that would need to be updated to handle unknown nodes as well. I think trying to update everything at once would be to difficult and should be done separately. Improvements can be made by this change alone.

Finally I think this change is breaking since rule authors would now need to write rules that could traverse any child of a known node instead of the predefined VisitorKeys. Although I think we have made changes to VisitorKeys before which were not considered breaking changes. I don't think updating eslint-scope or code-path-analysis, to accept new node types, would be a breaking change for eslint, since rules would behave the same way, and would allow us to update these at a later time.

[mysticatea]

Thank you for the details.

Hmm, but I'm not sure who use visitor keys for eslint-scope because babel-eslint is overriding some methods for types and decorators (and I guess typescript-parser needs the similar patch). Parser possibly needs to override the methods of eslint-scope to address new syntax, so I think the solution is that it delegates the scope analysis to the parsers. I think it's not difficult.

The code-path-analysis part, yes, I agree that it's difficult.

[soda0289]

I do not like the idea of monkey patching eslint-scope to support unknown nodes like babel-eslint does. This patch would still require a monkey patch in babel-eslint which is unfortunate but does solve some issues in typescript-eslint-parser without the need to monkey patch anything.

I could support having the parsers expose their own scope analysis tool but they would be exact forks of eslint-scope with minor tweaks, since the api exposed to rules and current scope behavior of es6 nodes would need to be the same . I think we should be able to come up with a solution that allows parsers to modify or add their own node handlers.

I can but some more thought into a solution for eslint-scope since I think it would be nice to have multiple node handlers instead of just one like the current code base allows.

[mysticatea]

I do not like the idea of monkey patching eslint-scope to support unknown nodes like babel-eslint does.

Sure.

This patch would still require a monkey patch in babel-eslint which is unfortunate but does solve some issues in typescript-eslint-parser without the need to monkey patch anything.

Is it true?
For example, parserOptions.ecmaVersion and parserOptions.sourceType change eslint-scope behaviors about ES2015 syntax. typescript-eslint-parser does not have those parser options but supports ES2015 syntax, so I guess typescript-eslint-parser still needs monkey patching. This is about this part of babel-eslint.

I could support having the parsers expose their own scope analysis tool but they would be exact forks of eslint-scope with minor tweaks, since the api exposed to rules and current scope behavior of es6 nodes would need to be the same . I think we should be able to come up with a solution that allows parsers to modify or add their own node handlers.

I think those are separated matters.

1. ESLint delegates the scope analysis to parsers to support new syntax.
   • Parsers can customize analysis options, includes ecmaVersion, sourceType and visitorKeys.
   • Parsers can use forks of eslint-scope to override methods for right now.
2. eslint-scope exposes APIs to support new syntax without monkey patching.
   • Parsers can abandon the forks of eslint-scope by the API.

I don't think good if parsers return the all options of the scope analyzer because the options will increase for each found problems.

[soda0289]

I didn't realize a monkey patch was required to set options such as ecmaVersion or sourceType. This does further complicate the issues of having one version of eslint-scope handle the needs of every parser.

I think having parser create their own fork would work well, and makes sense given the current situation. I have patches for eslint-scope to support decorators and type annotations ready to go so this would be very simple to do.

I still think its possible to create one version of eslint-scope that could support plugins for new node types or ecma versions, similar to acorn, but would require a lot of work. And we can always abandon forks and go with this route if we can perfect it.

[kaicataldo]

Should we discuss this at the next TSC meeting? Sounds like we have some concrete ideas and have to decide on a path forward.

[btmills]

We discussed this in the 2017-06-01 TSC meeting and had two questions:

• Does this proposal add visitorKeys to the return value of parseForESLint, allowing us to add support for scope analysis in a similar fashion in the future?
• Will it have a negligible (or positive) impact on performance?

If the answer to both of those questions is yes, then we can accept this!

[soda0289]

@btmills

Does this proposal add visitorKeys to the return value of parseForESLint, allowing us to add support for scope analysis in a similar fashion in the future?

Yes, this PR adds visitorKeys and visitorFallback function to the results of parseForESLint(). It would allow for scope analysis to be returned as well, if we choose that solution. I still think we can come up a solution where only plugins need to be added to eslint-scope and they might not necessarily have to be included by the parseForESLint() function, but nothing is preventing it at this time.

Will it have a negligible (or positive) impact on performance?

I have not tested performance so its difficult for me to say what result would occur. I can see a positive performance gain coming from adding visitorKeys. Currently we visit every single property of an unknown node with visitorFallback which will not be necessary if we provide a list of all known nodes and replace visitorFallback function. All this PR does is provide a list of keys to eslint-scope and estraverse which I don't think will have a negative effect since the amount of keys will be relatively small and only applied when using a non default parser.

[mysticatea]

Will it have a negligible (or positive) impact on performance?

In fact, we had been using estraverse's keys option before. When we stopped using the option, it improved performance about 5% (#5543). So it's just a concern.

[soda0289]

@mysticatea I can run some performance tests but I'm guessing it will be related to the number of keys I pass in. We could submit a PR to estraverse to allow the VisitorKeys to be replaced, with some new option, instead of merged with the defaults. I think we should avoid overriding the exported VisitorKeys since it is used by other applications.

[kaicataldo]

Friendly ping

[mysticatea]

I realized some problems, so I made another PR: #8755.

• visitorKeys option cannot handle decorators properly because eslint-scope does not use visitorKeys to traverse classes; it's hardcoded in visitClass method. So I felt visitorKeys option is not helpful for scope analyzing.
• Traverser is used in other places; SourceCode#getNodeByRangeIndex() and no-unmodified-loop-condition rule. Those also have to use visitorKeys.
• SourceCode object can be reused. In that case, ESLint skips parsing phase, so visitorKeys is lost.

[soda0289]

@mysticatea Thanks for looking into this! I'll take a look at your PR.

[not-an-aardvark]

Should this be closed in favor of #8755?

[kaicataldo]

Yes, I think so. Thanks @soda0289 for getting this started!