Proposal: Allow parsers to supply their own Visitor Keys to estraverse/eslint-scope

[soda0289]

There are several node type's that currently do not get traversed when using parsers such as Babel or Typescript . These include Type Parameters, and Decorators. I believe we should define an API in eslint to modify the Visitor Keys for estraverse and eslint-scope. This would then allow parser's to pass in visitor keys using the parser services and eliminate the need for monkey patching or supporting experimental ecma features.

This was suggested by @kaicataldo to overcome the problems that traversing decorators would have caused
#8365 (comment)

[kaicataldo]

Also mentioned in the comment, but this would be beneficial both for the typescript parser and for babel-eslint.

[platinumazure]

I think this might need TSC approval anyway, but fwiw I'm 👍 to this.

[kaicataldo]

@eslint/eslint-team Would love to hear what the rest of the team thinks about and whether or not we should include decorators in #8365. I'm happy to champion this and implement if we decide this is something we want and come up with an implementation we can agree on.

[not-an-aardvark]

I'm 👍 for allowing parsers to supply custom keys, but I don't think we should add explicit support for decorators. (Parsers could indicate that decorators should be traversed in VisitorKeys.)

[kaicataldo]

Great! Pending TSC approval tomorrow, we just need to come up with a concrete proposal of how this should work!

[not-an-aardvark]

Can I suggest that we come up with a concrete proposal before the TSC approves this?

In issues like #7416, we've run into problems where the TSC approved the general idea of doing something, but then we ran into design issues while implementing. I agree that this is generally a good idea, but I might not necessarily agree with any given proposal to address it. It seems like in general, the details of core proposals are important enough that the TSC should review them in addition to the vague core proposal.

[kaicataldo]

Yes! Sorry that wasn't clear - I wasn't suggesting we wait.

[hzoo]

Yeah, right now we do https://github.com/babel/babel-eslint/blob/f59d2009652e14acf1231a1268a8324cf046514e/index.js#L59-L61

And then we override methods like visitClass to visit decorators for the no-unused-vars/no-undef rule https://github.com/babel/babel-eslint/blob/f59d2009652e14acf1231a1268a8324cf046514e/index.js#L198-L199

Could just be an export: exports.VisitorKeys = {}

[soda0289]

We could make it part of eslint api and create get/set or modify functions for VisitorKeys.

Not sure if it would be a good idea to expose the eslint api to the parsers. It would allow for the parser to know which rules are loaded and possibly warn users of incompatibles or even supply extra details or hooks to certain rules. It might get overwhelming if start passing too much stuff through parser services but VisitorKeys seems like a good fit.

[not-an-aardvark]

I don't think exposing it as a mutable part of the eslint API is a good idea, because any given parser's modification will be exposed to any other module running eslint in the same process. (Admittedly that's also what happens now if we're monkeypatching estraverse directly, but I think it would be better to not do that.)

Using parser services could be the way to go.

[mysticatea]

I'm thinking good if we expose parser.analyzeScope(ast, parserOptions)-like API instead of VisitorKeys.
It will allow us to override the handlers of the referencer without monky-patches.

[soda0289]

@mysticatea How would the parser be able to override the handlers of the referencer without monkey-patches?

We still need a way for rules to traverse nodes, such as decorators and type parameters. I still thinking passing a VisitorKeys would be useful and extending the scoper is another issue.

[kaicataldo]

@mysticatea I'm curious how this would allow us to avoid monkey patching too - would each parser be required to implement its own scope analysis then?

[kaicataldo]

Thoughts on modifying eslint-scope's API to allow for handlers to be overridden/passed in? The parser could then provide the node handler functions which could then be passed through to eslint-scope.

estraverse looks a little bit different. Unfortunately, the keys option does not do a deep merge (https://github.com/estools/estraverse/blob/master/estraverse.js#L135-L142), which is why we modify estraverse.VisitorKeys directly. We could potentially see if a new option that modifies existing VisitorKeys is feasible or try to use the fallback option with a method, but I'm not sure the latter gives us enough granular control. Initially, I still think the parser could provide a list of VisitorKeys to ESLint that would then be looped over and pushed to estraverse.VisitorKeys, if need be, and then we could make that better internally later.

So, to summarize: seems like parsers should be able to supply an object with two options:

• A list of node handler functions that can be passed to eslint-scope for parser-specific syntax
• A list of traversal instructions that are then used to modify `estraverse.VisitorKeys

I'm new to this part of the project, so please bear with me!

Edit: Ah, @soda0289 beat me to it: eslint/eslint-scope#30

[mysticatea]

My idea is:

If a custom parser provides parser.analyzeScope function, ESLint use it instead of embedded scope analysis. The function will return the object which is compatible to eslint-scope's scope objects. Of course, custom parsers can use eslint-scope package and additional VisitorKeys option in the function. Also, custom parsers can use another ways (e.g. fork eslint-scope and patch it).

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

[btmills]

The TSC discussed this in the 2017-04-13 meeting and decided to postpone official approval until a more detailed proposal is available.

[ilyavolodin]

I agree with @mysticatea position. Just VisitorKeys seems insufficient to cover the need of new syntax. I'm not sure that I like the idea of substitution of eslint-scope completely by parser. It would be better to modify eslint-scope to accept some sort of hooks that would allow certain actions for certain node types.

[soda0289]

The eslint-scope issue is another problem. I opened an issue for a possible fix here: eslint/eslint-scope#30. This proposal was not meant to fix all the problems of new syntax. It was just to allow eslint to traverse new node types.

Right now you cannot write rules for Decorators or Type Parameters because they are children of known nodes. When we define VisitorKeys we don't set typeParameter or decorators as a possible child node and do not traverse them. This means rules cannot be written for these nodes and instead we have to have create rules with node listeners for all parents of decorators and type parameters.

[soda0289]

I think if we exposed a getVisitorKeys() api function that could tell parsers what eslint currently traverses. And allowed parsers to pass in VisitorKeys, either via parser services or a module export, we could overcome the problem of eslint not traversing some node types.

[kaicataldo]

@soda0289 Feel free! Been pretty busy and would love to see this implemented :D

[kaicataldo]

It looks like we have a WIP PR with a proposal for this. Would be great to evaluate again.

Related to this, I think we should revert the monkeypatching for type annotations we added a few weeks ago: #8584

[btmills]

We discussed this in the 2017-06-01 TSC meeting but did not come to a resolution. Is there a specific proposal that we can evaluate in light of #8582?

[soda0289]

This proposal is to just allow parser's to pass in VisitorKeys and I think VisitorFallback function makes sense too.
I'm not really sure what else I can say about this. Is there some concern or question the TSC has? I did answer the questions in the PR.

[ilyavolodin]

@soda0289 I think we are just a bit lost as to exactly what is being proposed here, and how does that conflict/work with what @mysticatea is proposing. Just a summary of what the proposed changes are, and how will they help with preventing monkey-patching in estraverse/escope in the future, would really help us figure out what exactly are we approving:-)

[soda0289]

This proposal would allow parsers to pass in VisitorKeys and VisitorFallback function to escope and estraverse. This would be accomplished by adding two properties to the returned object from parseForEslint.

This would prevent the need to monkey patch the exported VisitorKeys from estravese, which is used by both eslint-scope and estraverse and possible other applications. This would allow eslint plugins to write rules for nodes that are children of known nodes as well as only traverse nodes that are known to be handled correctly.

This proposal is not intended to remove the monkey patch of eslint-scope. It is just the first step towards improving support for parsers in eslint. If we want to remove the monkey patch to eslint-scope we have to either provide hooks for node listeners, provided plugins for new node types, or as @mysticatea suggested have parers provide their own scope anaylsis tool.

In summary this proposal is just to allow eslint and its tools to traverse new node types and for rules to be written for decorators, type annotations, and type parameters. It will not fix the problems of eslint-scope handling unknown nodes.

[kaicataldo]

@eslint/eslint-tsc Would be great if we could decide on what we want to do here. This will make things much easier both for the typscript parser and babel-eslint. Ideally, we can live in a world where our parsers don't need to monkeypatch at all (we're seeing the pain points right now as babel-eslint has to now change its own monkeypatching behavior).