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: Differentiate AST node types without parent (for estree) and with parent (for eslint) #8347

Closed
4 tasks done
JoshuaKGoldberg opened this issue Feb 3, 2024 · 6 comments · Fixed by #8617
Closed
4 tasks done
Assignees
Labels
accepting prs Go ahead, send a pull request that resolves this issue breaking change This change will require a new major version to be released enhancement New feature or request
Milestone

Comments

@JoshuaKGoldberg
Copy link
Member

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

Relevant Package

parser

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

Spinning out of DefinitelyTyped/DefinitelyTyped#68232: right now, the types for ASTs generated by @typescript-eslint/parser have "one size fits all" types. DefinitelyTyped/DefinitelyTyped#68232 (comment):

https://github.com/typescript-eslint has options (just like espree, et al) which let you change the shape of the AST. By default those options turn off things like the ranges.

However eslint does pass in the options to turn on the things it needs. So our AST then matches their shape.
https://github.com/eslint/eslint/blob/60b966b6861da11617ddc15487bd7a51c584c596/lib/linter/linter.js#L828-L841

We could define this with conditional types - but then it still wouldn't match the types you've declared.

One tricky thing is that for the sake of simplicity I don't think we want to make custom lint rule users have to see type parameters & optionality pop up all over the place in their AST. I think we'll want to have two ASTs that can be generated:

  • parse: Utilizes the fancy new conditional types to make the AST shapes more precise
  • parseForESLint: Avoids the fancy new conditional types and uses the same existing non-generic versions of AST nodes

I think we can get there with the following changes:

  • Give NodeOrTokenData an type parameter object for whether it has loc and/or range
  • For the AST types that feed into types/src/generated/ast-spec.ts, create a second version of the nodes with that same type parameter object - i.e. ParsedProgram<* extends *> extends NodeOrTokenData<*>
  • Make parse configurable with that new type parameter object
  • Have parse's return type use a new ParseResult interface that forwards that type parameter object to its AST nodes
  • Make no changes to ParseForESLintResult or other rule infrastructure: it returns the same nodes as before

I.e. we'd be making a new set of node types that users of parse now see, without changing parseForESLint or custom rules.

Additional Info

No response

@bradzacher
Copy link
Member

we'd be making a new set of node types that users of parse now see, without changing parseForESLint

Note that changing the types of one without the other wouldn't fix the related problem in the ESLint types. The signature for a parser includes both functions.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/eslint/index.d.ts#L1182-L1187

Make no changes to ParseForESLintResult or other rule infrastructure: it returns the same nodes as before

This is my preference. As I stated in the other thread I don't think that there's any value in making parsers passed to the ESLint config type adhere to a strict shape beyond the basic parseForESLint shape.


TBH Parser's default options should be updated to match the options eslint passes in and we should remove the options entirely. Things break if you don't pass those options in - eg our scope analyser assumes certain options are set.

@JoshuaKGoldberg
Copy link
Member Author

TBH Parser's default options should be updated to match the options eslint passes in and we should remove the options entirely. Things break if you don't pass those options in - eg our scope analyser assumes certain options are set.

Oh! Reducing complexity that way did not occur to me as something we can do. I love it. But, are there any ecosystem consumers that expect the properties be set? cc @fisker and ... I don't know who else to tag 🤔

@bradzacher
Copy link
Member

There's a difference between parser and typescript-estree.

The former was only ever intended for use with eslint and the latter is a generic thing to be used by whatever.

@JoshuaKGoldberg
Copy link
Member Author

So just to be clear: is your proposal that parser always have the node properties be set, and typescript-estree is still configurable? or ...?

@bradzacher
Copy link
Member

we change @typescript-eslint/parser to export:

  • parse(code: string, options: ParserOptions): TSESTree.Program
  • parseForESLint(code: string, options: ParserOptions): { ast: TSESTree.Program, ... }

And we remove comments, loc, range, tokens from the ParserOptions type for /parser

@typescript-eslint typescript-eslint deleted a comment from github-actions bot Feb 5, 2024
@JoshuaKGoldberg JoshuaKGoldberg added breaking change This change will require a new major version to be released accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for maintainers to take a look labels Feb 24, 2024
@JoshuaKGoldberg JoshuaKGoldberg added this to the 8.0.0 milestone Feb 24, 2024
@JoshuaKGoldberg
Copy link
Member Author

#8617 was merged into v8. ✅

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue breaking change This change will require a new major version to be released enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants