Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: Have RuleCreator rules throw an error if used with an incompatible parser #8150

Closed
4 tasks done
JoshuaKGoldberg opened this issue Dec 27, 2023 · 8 comments
Closed
4 tasks done
Labels
breaking change This change will require a new major version to be released enhancement New feature or request triage Waiting for maintainers to take a look

Comments

@JoshuaKGoldberg
Copy link
Member

Before You File a Proposal Please Confirm You Have Done The Following...

Relevant Package

utils

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

#6814 -> #8146 and the discussion in #8149 are making me think that we really, really don't want users to use a parser that isn't at the very least running @typescript-eslint/parser internally. This is for two reasons:

  • typescript-eslint rules need to be able to assume the TSESTree shape for the AST
  • Type-checked rules need our generated type information

#8146 tries enforcing the exact parser being used - but that won't work for all users because some languages require wrapping parsing with their own (https://github.com/typescript-eslint/typescript-eslint/pull/8146/files#r1437273011). So checking the exact parser isn't likely feasible.

Instead, what if we augment RuleCreator so that rules verify some basic assumptions about the parsed AST / context before create(context)?

  • It can check the root nodes to make sure there's the expected TSESTree Program AST node
    • I haven't verified that this is true for other parsers - but likely an equivalent will hold up
  • It can it can run the normal checks from getParserServices on the esTreeNodeToTSNodeMap and tsNodeToESTreeNodeMap node maps existing
  • If the rule has type checking, it can run the normal check from getParserServices on the program existing if !allowWithoutFullTypeInformation

Additional Info

No response

@bradzacher
Copy link
Member

It can check the root nodes to make sure there's the expected TSESTree Program AST node

This wouldn't work as this is currently an Invariant within eslint itself. ESLint makes no assumptions about the AST other than the root node being a Program (or at least it used to).

Instead, what if we augment RuleCreator so that rules verify some basic assumptions about the parsed AST / context before create(context)?

Not all those that use our utils build specifically for our parser. Some people just want to use a nice type-safe API that allows them to work on an AST across both JS and TS codebases.

So we can't hard-enforce it here without providing mechanisms for people to opt-out.

The exception ofc is type-aware rules which hard require our parser as we're the only ones that can provide types.

It can it can run the normal checks from getParserServices on the esTreeNodeToTSNodeMap and tsNodeToESTreeNodeMap node maps existing

This could work as we're the only parser that provide these props to the parser services. This also means that we could enforce that parsers like Vue that manually call us properly pass on the required bits.


The one danger here is that this line of enforcement would hard-break usecases where people run our rules on JS code under eg the esprima parser (the default one) and expect them to silently do nothing.

This can be a pretty common case - so if we do step into this domain we would want to do it really consciously and clearly communicate what's happening, why, and how to fix things.

Though there are rules of ours that will work regardless of the parser or base file language (js vs TS) - eg naming-convention, member-ordering - and will work and report fine on JS code.

The biggest danger for us is when someone uses a parser like babel that can understand TS but produces the wrong AST. All other cases should be fine?

@Josh-Cena
Copy link
Member

Josh-Cena commented Dec 28, 2023

I do have a question: Babel and TS-ESLint diverge on some ASTs. Does that mean every ESLint rule out there that handles TS syntax already has an implicit dependency on either parser, or has to build compatibility for both? This is more about 3rd party rules, not us.

@bradzacher
Copy link
Member

bradzacher commented Dec 28, 2023

Does that mean every ESLint rule out there that handles TS syntax already has an implicit dependency on either parser, or has to build compatibility for both?

Yes - if they want to support babel as the TS eslint parser.
It's rare that people do this and it's rarer still that 3rd parties support it.
Why for the latter? Because if you use our tooling to build a plugin - then you're using our types for our AST - not babel's. If you're building a plugin without types then you're likely going to rely on whatever parser's AST that you're looking at. But if you google "typescript eslint" we're the first hit (babel isn't even on the front page) so I doubt people are using babel for TS or building against it for any reason other than "accidentally".


In the beginning our stance was "lets keep the AST the same"
But over time we went our own way and we couldn't easily make babel keep up (breaking changes require major releases which are hard for babel to do on our schedule). They also often would implement new TS features before us and thus would just expose their babel AST as the estree AST representation - whereas we weren't looking to copy them and made different decisions with the AST.
So now our stance is "using babel with our rules is unsupported" - and because we're the biggest plugin for TS in the ESLint ecosystem - people follow our rule - though that doesn't stop them from accidentally misconfiguring things.

@Josh-Cena
Copy link
Member

What if people build rules that handle TS syntax but don't use ts-eslint tooling? That doesn't sound uncommon, right? Rules always have a dependency on the parser used, to different extents.

@JoshuaKGoldberg JoshuaKGoldberg added the breaking change This change will require a new major version to be released label Dec 28, 2023
@JoshuaKGoldberg
Copy link
Member Author

Compromise: how about calling the normal getParserServices before rule.create() inside RuleCreator? That way we get the same normal error message. And we can look at augmenting the error message in #6814 -> #8146 separately to mention using our parser in the un-typed case.

@bradzacher
Copy link
Member

What if people build rules that handle TS syntax but don't use ts-eslint tooling? That doesn't sound uncommon, right?

That's the same as the case I mentioned above - "If you're building a plugin without types then you're likely going to rely on whatever parser's AST that you're looking at."

We're the only ones that provide TS types for the TS-ESTree AST. Babel only provides types for the babel AST - which isn't the same as the ESTree AST.

There are going to be cases of people building rules for TS code that don't use our tooling - sure - some people are going to write untyped code. In that case as I said - they'll use whatever AST they find. In some cases that may be babel's - but mostly it'll be ours because, as I mentioned, we're the only ones who show up on Google for the space!

So yes, it's not going to be uncommon that people write untyped rules for TS code - but it is uncommon (non-existent even) that people will write rules of any kind to target the babel TS estree ast.

@bradzacher
Copy link
Member

how about calling the normal getParserServices before rule.create() inside RuleCreator?

This will block people from using espree with our rules which I mentioned in the latter part of my comment above

We don't want to make config harder by hard forcing everyone to turn off all of our rules on any file not parsed by our parser. On pure JS files it doesn't matter what parser is used - our non-type-aware rules will either silently do nothing (as there's nothing to report on) or will work as expected (eg naming-convention or member-ordering).

It's specifically just the TS syntax usecase where we care about what parser was used.

@JoshuaKGoldberg
Copy link
Member Author

Makes sense. I'll drop this line of inquiry - seems fruitless. Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change This change will require a new major version to be released enhancement New feature or request triage Waiting for maintainers to take a look
Projects
None yet
3 participants