Support for generic ASTs

[ilyavolodin]

Rendered RFC

Summary

Enable support for non-estree compliant ASTs and open possibilities for creating parsers and rules for new languages.

Related Issues

eslint/eslint#13305

[platinumazure]

Just a random thought: What if ESLint instead had a translation layer where the AST could have type properties added using the value of the nodeIdentifier property name? We already do a pass through the AST to add parent, so I don't know if it would even impact performance significantly.

Main drawback is that type might be banned as a property name from the AST, unless there was another parse result property indicating what property type should be moved to... which doesn't sound great...

---

Another thought, though it's a horrible idea in terms of the massive breakage...

What if we had a wrapper AST node, which would always have type, parent, and then another property to get the raw node object and its other info? So something like node.raw.test to get at the test property specific to an IfStatement node, but node.type would still be "IfStatement".

Doesn't solve things like code path analysis, though.

[kaicataldo]

Thanks for writing this up! I agree that this is the direction the community should head (I've played around with this a bit myself!). That being said, I actually don't think this should be shoehorned into ESLint. ESLint is really stable at this point, and if we're going to really go all in on supporting other languages, I personally think it's best to approach the problem with a fresh start and without all the baggage an older, stable project accrues over time.

I've thought about this quite a bit and would love to chat!

[ljharb]

I don't understand. This is "ES" lint. Why would linting things that aren't ES be a first-class feature?

This sounds like a different project to me.

[ilyavolodin]

I don't understand. This is "ES" lint. Why would linting things that aren't ES be a first-class feature?

Non estree ASTs will not be first-class features. ESLint will still come bundled with espree, you will have to setup other languages with their own parsers and their own set of rules that would be coming from plugins. Nothing will work out of the box for things other then JS. Core rules will also only support JS based ASTs and nothing else. But this will give you an option of linting things other then JS as well as your JS code.

Thanks for writing this up! I agree that this is the direction the community should head (I've played around with this a bit myself!). That being said, I actually don't think this should be shoehorned into ESLint. ESLint is really stable at this point, and if we're going to really go all in on supporting other languages, I personally think it's best to approach the problem with a fresh start and without all the baggage an older, stable project accrues over time.

I would generally agree, that starting from a scratch would be a cleaner and better way to handle this. However, ESLint has 7 years worth of development and bug fixes, something that new project wouldn't have. There is also, not that much refactoring required to enable this feature (as shown in the linked branch), so, yes, ideally, you would start from the scratch, but it would be a massive undertaking and the advantage that you would get by doing that is completely negated by the effort.

Just a random thought: What if ESLint instead had a translation layer where the AST could have type properties added using the value of the nodeIdentifier property name? We already do a pass through the AST to add parent, so I don't know if it would even impact performance significantly.

Sorry, but I don't understand why we would do that? I already refactored ESLint's code to accept parser driven nodeIdentifier, it wasn't that much work. I don't quite understand what would translation layer buy us and it has high potential of collision. For example, GraphQL AST is identified by kind property, however, there are certain node types that also have type property as well. Again, the refactoring is already done on the branch linked from the RFC. It needs a little more clean up and fixing of one broken unit test, but other then that, it's mostly just refactoring existing code, not much new code is added at all.

[ilyavolodin]

Just to be clear. This RFC is not proposing anything that cannot be done now. I already have a custom parser that is capable of parsing GraphQL and tweak resulting AST to conform to hard-coded assumptions that ESLint is making. I can create a set of rules that would allow me to lint those GraphQL file. This RFC is proposing to just remove hard-coded assumptions that are based on estree AST to make it slightly easier and give custom parser slightly more control. No extra effort is required from the team to maintain anything related to other ASTs going forward, and no additional support. ESLint custom parsers and processors already allow you to do mostly everything that's needed, examples being https://github.com/typescript-eslint/typescript-eslint and https://github.com/azeemba/eslint-plugin-json.

This is mostly just minor refactoring that would allow to support arbitrary ASTs.

One extra advantage that this would allow, is integration with prettier. Currently eslint-plugin-prettier can do formatting through ESLint directly, but prettier supports formats other then JS, and if you want to use it for things like GraphQL in addition to JS, you not only have to setup eslint-plugin-prettier but also prettier as a stand-alone utility for formatting things outside of JS file. With this addition, prettier could handle all file formatting directly through eslint-plugin-prettier (not a huge selling point for me, personally, since I don't like prettier and prefer not to use it).

[platinumazure]

I already refactored ESLint's code to accept parser driven nodeIdentifier, it wasn't that much work.

Wouldn't we need to change all rules to use node[nodeIdentifier] and make that available via RuleContext? Or are you thinking it's okay for JS rules to continue to use node.type?

[ilyavolodin]

Wouldn't we need to change all rules to use node[nodeIdentifier] and make that available via RuleContext? Or are you thinking it's okay for JS rules to continue to use node.type?

RFC specifically says that current rules will not work for anything other than JS code. It's not reasonable to expect rules to work across multiple languages. I also think that non-JS rules should not be added to the core at all, they should be supplied as plugins.

[nzakas]

A slightly different take: we could easily support other AST formats that represent JavaScript with this proposal, such as Shift or the TypeScript AST. So, I don’t think this should purely be looked at as ESLint working with other languages.

If people chose to use it to support other languages, it would be a “void your warranty” type of situation where we wouldn’t support it.

I’m on the fence overall. Need more time to think it through.

[ilyavolodin]

If people chose to use it to support other languages, it would be a “void your warranty” type of situation where we wouldn’t support it.

Absolutely, I specifically tried to word RFC that way. I don't want to have users to get any ideas at all that ESLint team will support anything for a specific non-estree AST.

Another bonus point is that this has potential of eliminating the need for babel-eslint and change the way typescript-eslint parser is written. Both of them right now try to translate their respective ASTs to match ESLint's assumptions. With this change, they wouldn't have to. For example, babel can just be used as a parser for ESLint directly, instead of having babel-eslint (although, I'm fully aware that will cause issues with rules working for different ASTs).

[kaicataldo]

A slightly different take: we could easily support other AST formats that represent JavaScript with this proposal, such as Shift or the TypeScript AST. So, I don’t think this should purely be looked at as ESLint working with other languages.

Thinking of this proposal in this light is really compelling to me. Given how many folks rely on ESLint to lint code transformed by Babel & TypeScript, I’m all for improving this integration story.

[kaicataldo]

To clarify my position: I generally support this initiative, but I would ultimately really like to see complete support for arbitrary ASTs (i.e. scope and code path analysis also supported). This feels like a step in that direction, but feels like an incomplete solution to me, and my concern with that is that we have done a lot of bolting on of smaller, iterative changes in the past that ended up amounting to complex and confusing behavior. I'd love to be approaching feature changes in a more holistic way going forward, so that we can do our best to ensure that the developer experience doesn't suffer.

To this end, I think it's worth defining what we believe the scope of the project should be. Is the end goal here to allow folks to experiment with other parsers? If so, adding this feature with a warning that we won't support it in core seems like a totally fine approach, and I would support this.

If the end goal is wanting to head towards being a generic linter, I think it would be prudent to take a step back and think about what we would want that to look like before we start adding more features.

In the short term, I'm fully supportive of improving the 3rd party parser integration story (though I may be biased since I also help maintain babel-eslint 😄)! Supporting code path analysis for arbitrary ASTs would be a huge win for babel-eslint, in particular.

[ljharb]

If there's a desire to be a generic linter, than an AST-based autofix format seems like it'd be needed first.

[ilyavolodin]

@kaicataldo in general I'm not opposed to the idea of looking at the bigger picture. However in this case, I actually wasn't even sure if I should create an RFC for the changes I'm proposing, since they are just refactoring to remove hard-coded values. This actually cleans up ESLint's code and removes some assumptions. I would be perfectly fine discussing general approach for support of arbitrary ASTs, but in that case, could we separate it out? Reason why I don't want to mix those two discussions together is I know how long something like a full re-thinking of the position of ESLint might take, and I would like to get an approval to go ahead with refactoring sooner rather then later.

[kaicataldo]

@ilyavolodin Yes, definitely. Approaching this as a refactor rather than a fully fleshed out feature makes a lot of sense to me and clears up my concerns.

[JamesHenry]

This parser I created for compiling Angular flavoured HTML and linting it with ESLint might be a useful reference for how this manifests today in a non-JS/TS (and therefore very different AST) context:

https://github.com/angular-eslint/angular-eslint/blob/master/packages/template-parser/src/index.ts

It uses the Angular compiler (which is a wrapper around the TypeScript compiler) which produces a specific AST for the HTML templates, and then provides a parserService method which allows ESLint rules to write selectors on that custom AST.

Example usage within a rule: https://github.com/angular-eslint/angular-eslint/blob/master/packages/eslint-plugin-template/src/rules/banana-in-a-box.ts#L34

(Thank you (as always) to @mysticatea for the hard work on the Vue parser and plugin which provided the inspiration for how to achieve this based on ESLint's current architecture)

[btmills]

Like you, I'm intrigued by the idea of using ESLint on all the JS-adjacent code in my codebases, so I'm supportive of this experimental/warranty-voiding direction for now while we figure out the use cases.

Like @nzakas mentioned, I'd also thought that this could make the Babel and TypeScript implementations much easier. They're already providing alternative implementations for core rules that don't work with their translated ASTs, so I'd suspect that being able to work on the original AST could remove lots of lost-in-translation edge cases.

[btmills]

Like @kaicataldo, I could see us adding more thorough support for arbitrary ASTs in the future including full support for scope and code path analysis. If we do that, we'd need to provide a way for parsers to supply their own code path analysis. In this hypothetical future, I imagine we'd standardize the existing code path analysis interface or something very similar to it as a public API and allow parsers to pass in an alternative implementation matching that interface. That could make this boolean property redundant or lead to invalid combinations of the two values (i.e. supportsCodePathAnalysis: false, codePathAnalysis: {Implementation} makes no sense).

Like you, I don't want to try to standardize pluggable code path analysis in this RFC, so my only goal is to avoid something that we'll soon want to make redundant. Instead of this being a boolean property, what if it's called codePathAnalyzer, and for now its only possible value is null? If the property is absent, we keep the existing behavior and use the built-in code path analyzer, but if it's present and null, we disable code path analysis. This allows us a built-in hook for whenever we standardize a public API for pluggable code path analyzers.

We could also move this property to be at the same level as scopeManager.

[ilyavolodin]

I would be fine with this change, but it can be pretty confusing to somebody from outside looking at the API. If you pass in null it all-of-a-sudden disables a feature. Seems a bit magical. What I would prefer instead is to have a variable codePathAnalyzer which would right now be of type boolean (defaulting to true). In the future we can make it accept function or boolean. Boolean true would mean enable codePathAnalyzer, false - disable, function means use this for codePathAnalyzer.

[btmills]

You're right, that would be pretty confusing. And from the implementation side, handling codePathAnalyzer of type boolean | function isn't difficult at all.

[btmills]

Custom parsers can already return a custom scopeManager property. When I was playing around with another non-ESTree AST, I threw together a do-nothing ScopeManager implementation. Similar to the above idea, what if we add scopeManager: null as a valid value that serves the same purpose as supportScope: false would here?

[ilyavolodin]

I added this property because ESLint's documentation is incorrect. Right now it states that you can return null from parser for scopeManager, however, that's not true. ESLint will accept null/undefined scopeManager, but will blow up, because it will try to navigate it. Because of that, I had create scopeManager like this:

scopeManager: {
    variables: [], scopes: [{ set: new Map(), variables: [], through: [] }], globalScope: { set: new Map(), variables: [] }, getDeclaredVariables: () => {}
}

And then there's a bunch of code in the core that tries to analyze and attach those scopes to nodes. But I think if we can accept null as scopeManager and disable core attachment code, that would be perfectly fine from my side.

[btmills]

Since scopeManager and codePathAnalyzer could often be used together, I think we should make them consistent. That would argue for either 1) making scopeManager accept boolean | ScopeManager or 2) using the existing scopeManager documentation as precedent and fixing it to allow null and having codePathAnalyzer accept null. I agree with your argument for case (1) in the codePathAnalyzer discussion above, and if scopeManager: null doesn't work anyway, changing it to boolean | ScopeManager wouldn't break anything that isn't already broken.

[nzakas]

I'd prefer scopeManager and codePathAnalyzer to be either an object or null, and just forego booleans altogether. I like the idea of saying "here's the thing you should use", and if it's null, we know not to use anything.

[ilyavolodin]

@nzakas What if you do want to use existing one (i.e. eslint's built-in codePathAnalyzer)? How do you specify that with null | object? I would imagine that both babel and TypeScript parser would want to continue using built-in codePathAnalyzer for some time, until the have a need to create their own implementation. Also, if we want to match scopeManager and codePathAnalyzer, do both of them need to be return at the top level? We can't move scopeManager into parserSupportOptions suggested in this RFC, because that would be a breaking change.

[nzakas]

I'd envision that simply not specifying scopeManager or codePathAnalyzer would mean "use the default".

[ilyavolodin]

So it's a tri-state? object|null|undefined? Ok, I can work with that, although to be honest, to me, boolean|object seems a lot more descriptive.

[btmills]

I see how object | null | undefined is the most literal way to type the option, but I also see how object | boolean is friendlier. So I'll throw out a bad idea for the purposes of discussion:

If I were doing this in a language that had a variant type I might do something like this:

type CodePathAnalyzerConfig =
    | Off     /* Or None, Disabled */
    | Default /* Or On, BuiltIn */
    | Custom(CodePathAnalyzer);

Since we can't quite do that with JS, what about this?

type ParseForESLintReturnValue = {
    // ...
    codePathAnalyzer?:  // Optional
        | "off"     // Or "none", "disabled"
        | "default" // Or "on", "builtin"
        | CodePathAnalyzer,
}

If the property is missing/undefined, that's the same as "default".

[ilyavolodin]

I like this a bit better then null|undefined|function, it's a bit clearer in it's intentions. Although I still think boolean|function is easier to understand. But difference is minor.

[nzakas]

Works for me

[btmills]

• Explicitly decide against making non-ESTree ASTs possible.
• Alternative to nodeIdentifier: require translations from non-ESTree ASTs to add a type.
• Alternative to supportScope: parsers could implement and pass a do-nothing ScopeManager implementation.

[ilyavolodin]

Those all describe status-quo, so I assumed this would be talking more about different implementation solutions.

[btmills]

Maybe eventually, but I feel safe waiting to see if there's demand for such a thing before implementing it, and I don't see any reason we couldn't do that in a semver-minor change.

[btmills]

Yes, and I think that's actually preferable. It's my impression that we all sort of stumbled onto the realization around the same time that ESLint is closer to supporting non-ESTree ASTs than we might have thought before investigating. If we keep this as experimental, void-your-warranty type stuff, then we get to see where people want to take it rather than building the "perfect" design in an ivory tower before we know what the requirements will be.

[btmills]

This question sounds similar to the v1 processors API where processors specified what languages they supported. I prefer the v2 API where users are responsible for configuring whatever processor on whatever file, which would argue that we don't need to do this.

[ilyavolodin]

Oh, sorry, wasn't clear on this question, I didn't mean it as parser would drive inclusion of files (or not), I meant more from a metadata perspective. For example, HTML formatter might want to break down percentage of errors from a lint run based on the language, or something like that.

[btmills]

Ah! In that case I could see it as being speculatively useful, and it'd be easier to do from the very beginning rather than adding retroactively. I'm curious to see what other use cases people can come up with for this.

[btmills]

Is it worth mentioning the "All Nodes" requirements for range and loc properties and explicitly noting that this RFC would keep those as-is?

[ilyavolodin]

Sure. Do you have an alternative for keeping those as-is? I just struggled with this, since GraphQL parser returns locations in character offset from the start of the file, vs. ESLint requiring columns and lines. Translating between those was pretty painful, so if there's an alternative you can think of, I would be all for that!:-)

[btmills]

As much as it would open up the ability to use existing third-party parsers, I don't think it's practical to remove this requirement. Rules frequently reference node.loc and node.range, so there's no way we'll easily get away from having line and column information available somehow. We could allow passing getLoc(node: ASTNode) => SourceLocation and getRange(node: ASTNode) => [number, number], but I don't think that's a great idea either. It does mean a parser wouldn't have to do a pre-traversal adding translated loc/range to the AST, but developers would still have to implement that translation algorithm, it just moves to on-demand rather than up-front. My gut says that the overhead of a function call relative to property access would kill performance unless it turns out that we only need location info for a minority of nodes making on-demand less expensive than up-front. About the only thing I like about that idea is that we could do it while maintaining backwards compatibility.

[ilyavolodin]

Just an FYI, I did release a very first version of parser capable of supporting GraphQL through ESLint (https://github.com/ilyavolodin/graphql-eslint) and you can see all the hoops I had to jump through in order please ESLint's assumptions about ASTs (https://github.com/ilyavolodin/graphql-eslint/blob/master/packages/parser/src/index.js). Most of them (but not all) will be fixed with this refactoring effort.

[ilyavolodin]

@btmills @nzakas @kaicataldo I updated this RFC to incorporate suggested changes. Please re-review when you get a chance.

[nzakas]

This looks good to me now. Even though people will think it strange to support non-JS languages in ESLint, this seems like a small enough change that allows experimentation for a bunch of different use cases. It kind of reminds me of when we added the ability to specify parser, not realizing that eventually we'd have things like the TypeScript parser and Vue parser.

[nzakas]

Thought: If this is a function, maybe it should have an action name? I'm still partial to getNodeType(), but I'm not opposed to something else. Also, I suppose this could be allowed to be a function or a string for simplicity?

Not critical, mostly posted here for others' thoughts.

[ilyavolodin]

Sure, that sounds like a good enhancement to me, I'll make the change, since most parsers probably wouldn't need support for variable node name. In terms of name, I think getNodeType is a bit misleading, for me, for example, it would mean given the node, get me it's type, not the get me the property that identifies all nodes. We could change it to getNodeIdentifier(), but that just feels overly verbose to me.

[nzakas]

Ah, I see the confusion. To me, we need getNodeType(node) to return the node type string (like "Program") rather than just returning the property name. As in the TypeScript example I mentioned, there's just no way to get the node type unless you calculate it, which means we can't just return a property to go look it up.

[ilyavolodin]

So what you are saying is that getNodeType function would take an ASTNode and return back a string representing the type of that node, because TypeScript ASTNodes do not contain identifying property? Am I understanding this correctly? My biggest concern here would be performance implication of this change, since for every single node we wold have to call into the parser. But I guess that could be optimized later. I'm also thinking that if we make getNodeType be either a function or a string, the name will be somewhat confusing when it's of type string. But it's a minor concern.

[nzakas]

Yes, that's correct. If we want to still allow a string to be specified, then we can probably keep the name as nodeIdentifier (or maybe nodeType).

[ilyavolodin]

I've updated RFC with the latest set of feedback. Could you review once again, and if everything looks good, maybe this can be moved into Final commenting state?

[Shinigami92]

We could link this to https://github.com/eslint/espree with [espree](https://github.com/eslint/espree)

[Shinigami92]

Skipping some parts of the comments above
I really love prettier's plugin eco-system. That said, prettier is a much more generic name as eslint 🤔
I would suggest a strategy of building a global linter were ecma/js/ts is only a plugin for the linter
I would be happy to help and initiate such a new project 🙋‍♂️

[nzakas]

Moved into final commenting stage.

@Shinigami92 That's a significantly larger project than what this RFC is intending to do. I think this RFC gets us moving in that direction and maybe there will be other incremental changes we can make to better support something like you describe without starting over.

[btmills]

LGTM. Nice work @ilyavolodin!

[ilyavolodin]

Any chance that this can be merged now? I think this would require approvals from @mysticatea and @mdjermanovic for full TSC consent.

[nzakas]

Given that we have four out of five TSC members approving, I'm merging this.