Skip to content

[utils] Use template literal types to parse esquery selectors to automatically type complex selectors #4065

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

Open
bradzacher opened this issue Oct 28, 2021 · 9 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request package: utils Issues related to the @typescript-eslint/utils package

Comments

@bradzacher
Copy link
Member

Template literal types (https://www.typescriptlang.org/docs/handbook/2/template-literal-types.html) were released in TS 4.1. since their announcement people have done all sorts of crazy and wonderful things with them - from simple cases like auto prefixing keys to ridiculous things like SQL query parsers.

We should be able to leverage them as part of our ESLint types to automatically type the node argument of an ESLint rule selector.

We don't need to handle every crazy case (selectors can get pretty complicated!) but at least handling things like property existence checks, sibling checks, and the :exit special selector would be great.

For reference here are the docs on, and implementation of esquery selectors
https://github.com/estools/esquery

It's worth mentioning that if we find that template literal types are too slow or complex, an alternative might be to consider using codegen to generate a number of common cases statically. But I'd love to achieve this using template literal types if possible!

@bradzacher bradzacher added enhancement New feature or request package: utils Issues related to the @typescript-eslint/utils package accepting prs Go ahead, send a pull request that resolves this issue labels Oct 28, 2021
@JoshuaKGoldberg
Copy link
Member

YES. This would be such a good type system challenge 😄

This feels like something that would be broadly useful outside of typescript-eslint for writing vanilla ESLint rules in TypeScript... I don't (yet?) know how that would be doable, but something to keep in mind maybe.

consider using codegen to generate a number of common cases statically

Perhaps an ESLint rule that parses the type from the selector and enforces the right type on the node?

@bradzacher
Copy link
Member Author

Perhaps an ESLint rule that parses the type from the selector and enforces the right type on the node?

That was what I was something I was thinking we could do as well, yeah!

That we we can handle some simpler cases with template literal types and lean on probably correct explicit types enforced via a lint rule.

We could also leverage codegen to generate more selector strings. Eg we could take the type defs and generate extra defs for every node like:

'VariableDeclarator[init]'?: (
  node: VariableDeclarator & {
    init: NonNullable<VariableDeclarator['init']>,
  },
)

@auvred
Copy link
Member

auvred commented Jan 31, 2024

I created magic-esquery as a side pet project (I was curious about implementing some type-level parser 😄). It's a type-level ESQuery parser and matcher. So it basically allows to infer types of nodes that can be queries by some selector
https://github.com/auvred/magic-esquery

Perhaps this could be useful for typescript-eslint!

It properly infers types for all the selectors used in @typescript-eslint/eslint-plugin, @stylistic/eslint-plugin and eslint-plugin-jest rules!

See tests:
@typescript-eslint/eslint-plugin - tests
@stylistic/eslint-plugin - tests
eslint-plugin-jest - test

You can try it online if you want!


I hope this will be useful for ts-eslint. Perhaps we can use this project as a reference to implement the same functionality as built-in in ts-eslint's utils.. Or maybe we can use it as is. If you want, I can file a draft pr with the magic-esquery used in RuleCreator.

So if you have any thoughts about this, please let me know!

@bradzacher
Copy link
Member Author

@auvred do we know what the typecheck perf hit is for the types?
I'm certainly very interested in having this as it greatly improves DevX and removes the need for many manual and possibly incorrect annotations.

@auvred
Copy link
Member

auvred commented Feb 21, 2024

do we know what the typecheck perf hit is for the types?

I removed explicit type annotations for >90% of existing selectors in the packages/eslint-plugin/rules/*

Tested with hyperfine with 3 runs. On my machine the results are as follows:

command Baseline With magic-esquery
yarn typecheck 9.099 s ± 0.207 s 10.919 s ± 0.051 s
yarn lint 54.137 s ± 0.335 s 57.145 s ± 1.121 s

So it adds +2s to the typecheck and +3s to the lint

@bradzacher
Copy link
Member Author

bradzacher commented Feb 21, 2024

🤔 that's really not that bad overall for a marked improvement in devx.
roughly how many selectors was that 90%? I'm assuming it's a few hundred.

I think this is a good thing to be added as part of our standard offering!

Will it break anything if we were to just introduce it as a dependency now? People's existing annotations won't break it, right?

If it will break - will need to wait for a major. If not then we could just do a feature bump addition!

@JoshuaKGoldberg
Copy link
Member

cc @bmish from eslint-community/eslint-plugin-eslint-plugin#339, thinking on what a rule to ban overly complex ESQuery selectors should consider "complex". I think it'd make sense to have "whatever magic-esquery can support" be an option for what's considered reasonable.

Aside: magic-esquery is a very cute name (I find it very charming 😄) but not super descriptive. It'd be nice to have one that's more clear IMO. Especially since folks will inevitably want to dive into it to investigate type edge cases... typed-esquery? esquery-types? ts-esquery? Etc.?

@auvred
Copy link
Member

auvred commented Feb 21, 2024

roughly how many selectors was that 90%? I'm assuming it's a few hundred.

After a quick look through my changes: there is ~130 "old" selectors (just plain AST node types + AST node types with :exit at the end) and ~65 more complex selectors, from which I removed type annotations. 130 + 65 = ~195 types are currently dynamically inferred.

In the group of "old" selectors I only included those that use node argument, because otherwise typescript doesn't calculate types for them. You can check this with tsc --extendedDiagnostics -> Instantiations

So I didn't take into account the following listeners:

// ...
return {
  Identifier() { doSomeStuff() },
  MemberExpression: doSomeStuff
}

Will it break anything if we were to just introduce it as a dependency now

In most cases, no. But there are a few places that can break, I think... For example naming-convention:

return Object.fromEntries(
Object.entries(selectors).map(([selector, { validator, handler }]) => {
return [
selector,
(node: Parameters<typeof handler>[0]): void => {
handler(node, validator);
},
] as const;
}),
);

It returns object with type { [k: string]: (node: never) => void; }. Unfortunately, it's a bit difficult to properly infer type of argument based on the property name:

createRule({
  create() {
    return {
      Identifier(node) { /* ... */ }
//      ^---------^-- we need to somehow link these two and consider that `Identifier` may be any string literal or string!     
    }
  }
})

Currently it works only if one of the listeners is a string literal.

Here is TypeScript playground link with the reproduction of this issue. Any help will be appreciated 😄

The solution linked above looks very hacky, but I can't think of any other way to do this without changing the overall createRule signature :(

@auvred
Copy link
Member

auvred commented Feb 21, 2024

Aside: magic-esquery is a very cute name (I find it very charming 😄) but not super descriptive. It'd be nice to have one that's more clear IMO. Especially since folks will inevitably want to dive into it to investigate type edge cases... typed-esquery? esquery-types? ts-esquery? Etc.?

I called it that, inspired by magic-regexp 😅. But typed-esquery or ts-esquery or something similar sounds great too! Let's consider renaming the package if the current name doesn't fit well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request package: utils Issues related to the @typescript-eslint/utils package
Projects
None yet
Development

No branches or pull requests

3 participants