feat: ESLint Language Plugins #99
Conversation
nzakas
added
Initial Commenting
|
Thank you, @nzakas, it’s an honor to be called out and this is a very interesting possibility to think about! Having looked over the proposal on my phone during lunch, here are some initial thoughts about how it might apply to markdownlint:
|
|
I'm the maintainer of eslint-plugin-vue, and a member and user of Stylelint. If the integration works well, that would be great! It might also be possible to unify the editor integration features maintained by each linter. However I have a question. Any idea how vue's language options would be configured if Stylelint was integrated via ESLint core? |
This could also allow us to use |
There was a problem hiding this comment.
would this make life easier for TypeScript ESLint?
No, I don't think it would make much of a difference for us.
At a glance it looks like it would actually make things harder for us as we would need to duplicate ESLint's code around SourceFile creation.
As I mentioned in the rewrite discussion - we purposely want our parser to produce an ESTree-compatible AST and we want TypeScript to be treated like JavaScript as much as possible by ESLint so that we can leverage the rule ecosystem (including the core ESLint rules) without anyone needing to make any changes anywhere.
|
|
||
| * [GraphQL-ESLint](https://github.com/B2o5T/graphql-eslint) | ||
| * [eslint-plugin-json](https://www.npmjs.com/package/eslint-plugin-json) | ||
| * [typescript-eslint](https://typescript-eslint.io) |
There was a problem hiding this comment.
I personally don't think that @typescript-eslint hacks anything substantial.
On pure JS code our parser will produce the exact same AST as espree (well it would have some additional, empty properties, but otherwise the same AST).
In parsing TS we still produce an AST that is compatible with the ESTree spec, with some new nodes and properties to support TS-specific language features.
For the most part the fact that the user is using TS instead of JS should be invisible to plugin authors, barring some edge-cases.
I would call some of the extension rules we have as hacky - though most of that hacking is because we want to avoid fully forking a rule to save us having to take on the maintenance burden of merging in upstream changes.
There was a problem hiding this comment.
If memory serves, you also needed to create your own scope manager?
FWIW, parseForESLint() will still work, though I think it itself is pretty hacky and would love to move away from it in the future.
There was a problem hiding this comment.
If memory serves, you also needed to create your own scope manager?
We did need to, yes, but that's because certain decisions were made early on in the AST definition that caused conflicts with ESLint's rules.
For example - types use Identifier nodes for their names. Seems like a good idea in theory, but in practice this is an issue because the scope manager is built fairly naively and just sees most Identifiers as possible variable references - causing false positives for no-unused-vars, no-undef, etc.
We tried augmenting the lint rule with an extra rule (like eslint-plugin-react/jsx-uses-vars does) to mark identifiers as used to hide them from no-unused-vars, but that was hacky and inaccurate.
A new scope manager was the best option to ensure the ecosystem would work as expected.
FWIW the scope manager is essentially just a fork of eslint-scope - adding additional code paths so that the scope analysis can understand type declarations and such (I ended up rewriting most of it from the ground up to modernise the code, but at its core it is a fork as I used eslint-scope as a reference).
There was a problem hiding this comment.
Fun fact - I also did a similar line of work for Flow. hermes-eslint is a package not unlike typescript-eslint - it enables full support for Flow code in ESLint rules (the hermes parser can parse Flow code and it includes visitor keys, as well as a scope manager that understands flow types)
There was a problem hiding this comment.
Ah, super interesting, I wasn't aware of hermes-eslint.
| * [eslint-plugin-json](https://www.npmjs.com/package/eslint-plugin-json) | ||
| * [typescript-eslint](https://typescript-eslint.io) | ||
|
|
||
| Instead of forcing others to rewrite what is core linting functionality, or asking people to produce an ESTree format AST for some language other than JavaScript, we can make everyone's lives easier by providing formal language support such that languages can be plugged in to ESLint easily. |
There was a problem hiding this comment.
for TS/Flow - IMO producing an ESTree AST is paramount as it allows them to leverage the existing rule ecosystem.
for other things (css, markdown, json, graphql, etc) definitely agree that producing some ESTree-like AST is bad and difficult.
There was a problem hiding this comment.
I've been working on JSON(C|5)?, YAML, TOML, Astro, and Svelte, (and Vue) parsers, and I agree they're difficult.
https://github.com/ota-meshi/eslint-plugin-jsonc
https://github.com/ota-meshi/eslint-plugin-yml
https://github.com/ota-meshi/eslint-plugin-toml
https://github.com/ota-meshi/eslint-plugin-astro
https://github.com/ota-meshi/eslint-plugin-svelte
https://github.com/vuejs/eslint-plugin-vue
But I think it was easier than thinking about integrating these editors and whatnot.
(※ Vue parser is not something I started with. Also, the svelte plugin is not official.)
There was a problem hiding this comment.
I've also written a JSON/JSON-C parser, and I did it with the specific goal of making it ESTree-like for easier linting:
https://github.com/humanwhocodes/momoa
| data: object; | ||
| loc: LocationRange; | ||
| suggest: Array<Suggestion>; | ||
| fix(fixer: Fixer): Iterable<FixOp>; |
There was a problem hiding this comment.
AST-based autofixing (and suggestions) is a popular idea that's mentioned in the rewrite discussion (eslint/eslint#16557). It's also how various other linters do autofixing (including ember-template-lint, a handlebars template linter that I help maintain. As an aside, we've been interested to use ESLint under the hood for ember-template-lint for a long time, so excited to see this RFC).
When adding support for new languages, it seems like AST-based autofixing would be a critical feature to have ready, for a number of reasons:
- Support the migration of other linters that are already using AST-based autofixing without having to rewrite those autofixers to be based on string manipulation.
- Avoid getting off to a bad start by requiring all the new language plugins to use string-manipulation-based autofixers that could soon be obsolete.
- We could even decide to ONLY allow AST-based autofixing for language plugins. If that's the goal, the best time to enforce it is before any string-manipulation-based autofixers have been written. If that isn't the goal, then perhaps just make AST-based autofixing the default/recommendation for language plugins.
There was a problem hiding this comment.
The issue with ast vs string autofixing is that you need a printer to convert the AST back to code and that printer has to make decisions about where to put braces or parentheses that aren't always encoded in the AST.
For example - const x = (1); the AST doesn't encode the location of the =, the existence of the parentheses, or the existence of the semicolon (I.e const x= 1 produces the exact same AST). Some of the info can be inferred from ranges sometimes - but a lot cannot.
So unless ESLint wants to start making stylistic decisions about code it prints after fixes - it needs a solution for that (like using a CST). Which to me is outside the scope of this particular RFC.
There was a problem hiding this comment.
A CST still might be better done before this RFC, though, for all the reasons indicated.
There was a problem hiding this comment.
There is nothing about this RFC that requires a CST, nor is there anything about this RFC that precludes AST autofixing. However, I don't think it's prudent to bundle AST mutation with this RFC or make it a dependency. What a language decides to return from parse() is up to it - AST, CST, whatever. Each language can decide whether or not it wants to allow AST mutation, and it may not be possible to mutate the ASTs of different languages in the same way.
Fundamentally, AST mutation just requires:
- The ability to mutate the AST. For ESLint's architecture, this would likely need to be applied after linting vs. during traversing like in Babel. This is already the way autofixing works, so that wouldn't be an issue.
- The ability to generate the source code given a set of AST/CST nodes.
Both of these can be added on to a language definition later.
|
@DavidAnson thanks for taking a look! Indeed, in order for markdown linting to work in ESLint, we'd need to get line and column information. I haven't looked too deeply at markdown-it (though I have tinkered a bit). Would switching to Remark be an option? It appears they already output an AST with location information. |
|
@ota-meshi Thanks for reviewing! I would need to look closer at vue-eslint-parser. Does it create an entire AST containing all HTML, CSS, and JS as one structure? Is there an example output anywhere I can look at? Am I correct in that you also maintain astro-eslint-parser? Would you have the same concerns with that? |
|
I don't think remark existed when I started markdownlint for Node. If it's a good choice here, there is already a remark-lint package that could be a more natural fit. I haven't looked much into swapping the parser because it would be very disruptive. (Though I should consider that when I look at getting better line/column info out of markdown-it.) |
|
@DavidAnson oh that's right, I've seen remark-lint before. IMHO, it's a lot more difficult to use due to the rule format, so it seems like another alternative would be welcome. I haven't looked closely, but what would prevent you from using the Remark AST and translating it into your current AST format? (TypeScript-ESLint does this, translating the TypeScript AST into an ESTree-compatible format.) |
Time? Intelligence? Maybe nothing? :) One thing I'd want from switching is a more useful tree, so I'd maybe only translate to keep extensions working. |
|
@DavidAnson I doubt intelligence is the gating factor but I do understand time. :) I'd be interested in exploring this with you if you're open to it. I feel fairly confident that there would be a way to use remark for parsing and then create an adapter to make existing markdownlint rules work. |
|
I appreciate the offer! I want to get the current round of releases out and then I should be in a better place to explore parser stuff. I assume this none of this is blocking for your proposal? |
|
Hi @nzakas, thanks for sharing the RFC. This idea sounds good to end-users. But as a Stylelint maintainer, I also have a similar concern to @ota-meshi's one.
The current implementation of Stylelint is mainly based on the PostCSS AST. If compatibility would break, the Stylelint community might not reap the idea’s benefits. |
vue-eslint-parser parses result of If you want to check the AST of postcss-html, you can check it at https://ota-meshi.github.io/postcss-html/. Inside
I maintain astro-eslint-parser (still experimental). |
|
@DavidAnson that's correct, we can move forward either way. Just offering to help out after the fact. :) @ybiquitous my idea is to still parse into PostCSS AST format, but that would be done inside of ESLint, so (in theory) rules running against PostCSS nodes would still work. @ota-meshi Hmm, I'm still not getting a good picture of how this is currently working. You are using |
I use vue-eslint-parser to parse
I haven't embedded it yet. But I think we may need to embed the PostCSS AST inside the vue-eslint-parser parsed (ESTree-like) AST if it's optimal for the Language Plugins. |
|
@nzakas It's confusing and a shame that I wasn't called out on the typescript-eslint note, and if I had been I could have also noted the other massive (5M downloads per month) ecosystem project which is relevant here: https://github.com/angular-eslint/angular-eslint
@yeonjuan thank you for starting the html-eslint project - there will be a good amount of overlap between template rules in angular-eslint and html-eslint, maybe we could figure out a way to collaborate? |
|
@ota-meshi Ah, I see! That makes sense. So the way this could work would be to create a processor that splits out the JS and CSS parts so they can be parsed separately. Then, you would be able to configure rules just for the JS and CSS parts. @JamesHenry I didn't intentionally omit you -- I just haven't seen you in other discussions so you're weren't top-of-mind. That is why the RFC process requires at least 28 days, to make sure we are open for feedback from folks we don't think of right from the start. So it looks like you are parsing into one AST that contains everything, correct? Would this proposal make life easier for you in any way? |
Hmm.. I don't think returning partial text in the processor is a good option for me if it is the same as the current system. |
|
@ota-meshi part of the role of processors is to normalize that information by mapping the whole into parts and vice versa. Another possibility that this proposal opens up: You can parse an entire file into a single AST, where parts are ESTree and parts are PostCSS. Then, you can enable both JS and CSS rules on the file, and assuming there are no naming collisions between nodes in different sections of the AST, the rules should just work. |
I like the idea of including JS and CSS (and HTML) information in one tree, but I'm a little concerned that it would work well with the check rules for each language plugin. |
|
@ota-meshi your current system can continue to work, you won't need to switch over. So the worst case is that you keep doing things the way that you are. I think ultimately, though, treating a Vue file or an Astro file as its own type of AST made up of HTML, CSS, and JS would allow the most flexibility. |
| * After reading a file, ESLint passes the file information to `parse()` to create a raw parse result. | ||
| * The raw parse result is then passed into `createSourceCode()` to create the `SourceCode` object that ESLint will use to interact with the code. | ||
|
|
||
| #### The `validateOptions()` Method |
There was a problem hiding this comment.
Perhaps we could also allow customizing merging behavior. In fact, instead of the validateOptions() method, this could be a languageOptionsSchema property. We could then use the provided schema in place of the current languageOptions schema.
There was a problem hiding this comment.
For convenience, we could move deepMerge (I assume this merge strategy was intended for language options) into ObjectSchema. Then, specifying a schema that just validates the object as a whole wouldn't be much more complicated than specifying a validateOptions() method:
// An ESLintLanguage implementation
{
// ...
languageOptionsSchema: {
merge: "deepAssign",
validate(options) {
// ....
}
}
}There was a problem hiding this comment.
Huh, that is an interesting idea. That would definitely give the same level of control to language implementers while also providing a bit more structure than a function that just throws an error. I like it!
Co-authored-by: James Henry <james@henry.sc>
|
@nzakas: I just stumbled upon this RFC and I'm very excited to hear about this development. May I give this a try with our custom ESLint plugin @sap/eslint-plugin-cds and report back? In our case, we currently hack into the rule creation such that writing our |
|
@mnkiefer of course! We don't have a prototype to play with yet, but all feedback is welcome. |
|
|
||
| ## Open Questions | ||
|
|
||
| ### Should we eliminate `:exit` from selectors? |
There was a problem hiding this comment.
I think this question was more important for earlier versions of this RFC where language implementations were expected to handle selectors. Now that parsing and processing of selectors stays in the ESLint core, this doesn't seem that closely related to the goals of this RFC, so we could evaluate this change separately?
@nzakas: Thank you! I've read the RFC and can report that this approach could make our current plugin for the CDS language much simpler, depending on whether the following scenario could be handled by ESLint: As already mentioned, our plugin does not rely on an AST but the CDS language model (CSN) which is computed from multiple files. Also, most of our CDS lint rules can't be considered for each file in isolation but only in the context of the entire model. Hence, for the first parsed file belonging to a certain model: We compute the model, run all of the lint rules on it and cache the results. If the next file is part of that model, the lint results for that file can simply be collected from cache and be reported. I'm wondering whether it would be possible for ESLint's LintResultCache to receive results for files it has not yet seen? |
|
@mnkiefer This seems like a much larger discussion that is prudent to do here. Can you open a discussion on the eslint/eslint repo so we can continue there?
No. The way you're describing this working isn't the way ESLint works internally, so I'm not sure it will be a good fit without significant changes. |
@nzakas: Thanks for getting back to me on this. I've opened a discussion here. 👍🏼 |
* main: chore: Add Mastodon status post to workflow (eslint#110) feat: ESLint Language Plugins (eslint#99) feat: support for testing invalid rule schemas and runtime exceptions (eslint#103) feat!: check for parsing errors in suggestion fixes (eslint#101)
Summary
This RFC specifies a plugin format that would allow ESLint plugins to fully define their own languages, effectively expanding ESLint from a JavaScript-focused linter into a more general-purpose linter.
I would love some feedback from:
The goal here is to take the boring parts of a linter (file finding, configuration, etc.) and separate that out from the JS-specific parts so no one needs to rebuild the boring parts over and over again.
Related Issues