@Logicer16 Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

• eslint —on npm,on unpkg
  • owner-approval:@JounQin,@bradzacher

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PRin the Playground.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Only a DT maintainer can approve changeswithout tests

All of the items on the list are green.To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

{"type":"info","now":"-","pr_number":68232,"author":"Logicer16","headCommitOid":"e1705c2b53455f39aadcb291540fb2b05ea5e0e4","mergeBaseOid":"eb3890d3f5d74638cc97e862a273cf94b5bfca71","lastPushDate":"2024-01-17T11:24:14.000Z","lastActivityDate":"2024-02-27T20:04:27.000Z","mergeOfferDate":"2024-02-27T16:31:54.000Z","mergeRequestDate":"2024-02-27T20:04:27.000Z","mergeRequestUser":"Logicer16","hasMergeConflict":false,"isFirstContribution":true,"tooManyFiles":false,"hugeChange":false,"popularityLevel":"Critical","pkgInfo": [    {"name":"eslint","kind":"edit","files": [        {"path":"types/eslint/index.d.ts","kind":"definition"        }      ],"owners": ["pmdartus","j-f1","saadq","JasonHK","bradzacher","JounQin"      ],"addedOwners": [],"deletedOwners": [],"popularityLevel":"Critical"    }  ],"reviews": [    {"type":"approved","reviewer":"jakebailey","date":"2024-02-27T16:31:13.000Z","isMaintainer":true    },    {"type":"approved","reviewer":"JounQin","date":"2024-02-27T05:25:43.000Z","isMaintainer":false    },    {"type":"approved","reviewer":"bradzacher","date":"2024-02-26T20:40:31.000Z","isMaintainer":false    },    {"type":"stale","reviewer":"JoshuaKGoldberg","date":"2024-01-21T16:16:25.000Z","abbrOid":"363e1f5"    }  ],"mainBotCommentID":1895612488,"ciResult":"pass"}

🔔@pmdartus@j-f1@saadq@JasonHK@bradzacher@JounQin — pleasereview this PR in the next few days. Be sure to explicitly selectApprove orRequest Changes in the GitHub UI so I know what's going on.

@Logicer16 One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

@bradzacher Sorry, should've explained myself before I submitted the pr. 😅

I've updated the description to include an explanation. Let me know if anything is unclear.

@bradzacher Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

Sorry, to clarify - I understand what you've done here. Instead I'm seeking to understand the why - what are the clear and real usecase examples that are currently broken and that this change fixes.

In its current state a compile-time error will occur on almost every use of a custom parser.

Most eslint parsers don't provide TS types I thought - do you have an example?

This is especially important with the new flat config files as it allows for the config, and consequently the parser, to the typechecked

Do you have an example config that's not currently working? If your example is@typescript-eslint - I would counter with the project not supporting flat configs yet (reftypescript-eslint/typescript-eslint#7935).

Most eslint parsers don't provide TS types I thought

Of the 10 popular parsers I've sampled, all but@html-eslint/parser provide types.eslint-mdx and@graphql-eslint/eslint-plugin use the@types/eslint types for their exports. The remaining 7 all defined custom AST which causes this issue. In particular these are:

• yaml-eslint-parser
• toml-eslint-parser
• jsonc-eslint-parser
• astro-eslint-parser
• vue-eslint-parser
• svelte-eslint-parser
• @typescript-eslint/parser

import{Linter}from"eslint";importhtmlParserfrom"@html-eslint/parser";importmdxParserfrom"eslint-mdx";import*asgraphQLParserfrom"@graphql-eslint/eslint-plugin";importyamlParserfrom"yaml-eslint-parser";importtomlParserfrom"toml-eslint-parser";importjsonParserfrom"jsonc-eslint-parser";importastroParserfrom"astro-eslint-parser";importvueParserfrom"vue-eslint-parser";importsvelteParserfrom"svelte-eslint-parser";importtypescriptParserfrom"@typescript-eslint/parser"exportconstmyConfig:Linter.FlatConfig[]=[{languageOptions:{parser:htmlParser}},{languageOptions:{parser:svelteParser}},{languageOptions:{parser:mdxParser}},{languageOptions:{parser:graphQLParser}},{languageOptions:{parser:yamlParser}},{languageOptions:{parser:jsonParser}},{languageOptions:{parser:tomlParser}},{languageOptions:{parser:astroParser}},{languageOptions:{parser:vueParser}},{languageOptions:{parser:typescriptParser}}];

If your example ishttps://github.com/typescript-eslint - I would counter with the project not supporting flat configs yet

While yes, they don't yet support flat configs, flat configs have an identical parser api to the legacy config format. As long as the parser already implemented the correct interfaces, no changes need to be made for them to support the new format. This is evident in the pr you referenced. Regardless, 6 other parsers still experience this issue.

During my testing, I've also fixed some related issues which also prevent other ASTs:

• Uses ofESTree.Comment have been updated to a newAST.Comment which aims to matchisCommentToken and thedocs. To partially* fix issues withvue-eslint-parser
• ESTree.Program["sourceType"] may be omitted from AST.Program for cases where it is not applicable to a language.The docs do not list this as a required property. To fix issues withjsonc-eslint-parser.
• Scope.Scope["type"] now allows any string as is permitted bythe docs allowing "customized scope analysis for experimental/enhancement syntaxes". To fix issues withastro-eslint-parser

*Withvue-eslint-parser specifically, they will continue to throw an error due to their types as they provide their own custom comment types. While this is permitted bythe interface described in the docs, these custom comment types will be filtered out by eslint'sisCommentToken at runtime. I've opted to follow the behaviour in the source and only allow specific type strings.

My big question would be - why validate the parser shape at all? By allowing non-estree-typed parsers you're introducing an explicit tear into the types - some of the APIs across the eskijt types accept estree nodes, some accept more generic ones. Others are expliclty typed to return estree nodes too - which is where the bigger tear is.

Instead of making some of the surface area generic - why not just make the config itself more generic - eg enforce the rough shape of a parser for validation reasons but leave the specifics untyped (eg like{ parseForESLint: unknown, ... }).

That way the API surface area of each parser and plugin can remain typed in whatever way they want or need and the configs can enforce you pass something parser-like and we limit the surface area of the change to just the place that needs it.

some of the APIs accept estree nodes, some accept more generic ones. Others are expliclty typed to return estree nodes too

The unique thing about ESTree nodes is that up until the point theirtype property is checked, theyare generic ones. The only difference betweenESTree.Node andESTree.BaseNode is that thetype property is restricted to just the type strings which ESTree provides whereasBaseNode allows any string. If any API method wants to use the additional properties of a specific node type, it must first check thetype property, which consequently excludes all other node types, including custom ones. If a function does this filtering to its output or requires it for its input, that should be reflected in its type, as is already the case in many places in@types/eslint.

Also, the nodes being passed to and returned from the API are the same nodes returned by the parser. If a custom parser is being used and that parser provides non-ESTree nodes, those will be the ones passed to and returned from those functions. This change reflects that aspect of the API.

why not just make the config itself more generic

This is certainly a possibility that I hadn't considered. While this would work, you'd sacrifice the guarantee that the parser is the correct type. This may allow passing faulty parsers to eslint which would otherwise be stopped due to type checking. In use cases such as sharable configs, this may allow issues to go undetected when they would otherwise be detected at compile time.

Sorry - you may have misunderstood me.

I mean cases likeVariable

There's no requirement that it returns an estreeIdentifier in the.identifiers property - for example @typescript-eslint/parser has its own identifier with additional information

OrDefinitionType which specifically returns some estree nodes:

To be honest the entire scope manager bit of the ESLint is wrong too in this case - because there's no requirement that a parser returns anything like this - a parser need only return something vaguely looking likeeslint-scope - but otherwise it can do as it pleases. For example see@typescript-eslint/scope-manager which is returned when using@typescript-eslint/parser.

Then there's also things like theNodeListener type which explicitly declares a strict binding of selector keys to function arguments - however this is again wrong - for example there's no reason that a parser'sArrayExpression remotely resembles estree's representation. That only needs to hold true if you want the default rules and default scope manager to work.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/4153f4919588dfe3c821cc83d86cf07e924c94f1/types/eslint/index.d.ts#L489-644

But at the same time the code path selectors are now completely generically typed - to the point of being pretty useless.

They now will be typed to return a node of the form{type: string, parent: Node} - which means you have to manually cast things to a type with some information. Additionally always includingparent isn't technically correct - eg theProgram node always hasparent: undefined.

Similarly the methods onSourceCode are typed by default to returnT extends AST.Node. Meaning a lot of usecases now will not get any useful types out of these functions and will have to manually annotate the tyoes

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/4153f4919588dfe3c821cc83d86cf07e924c94f1/types/eslint/index.d.ts#L224-471

What I'm saying is that making this loose like this makes the config usecase easier - for sure. But it trades off against making the typed rule usecase really really hard. I'd actually guess that this would end up needing to be a breaking change because it would physically break codebases that use the types for that usecase.

I don't think making everything loose for the sake of the config is the best approach. It's some shaky middle ground that provides the worst of all worlds.

If you're really looking to keep on this particular route then IMO we need to make the entire codebase generic with type parameters for the AST so that users can provide their own AST types everywhere whilst still defaulting to the ESTree types as a useful base. But that is a massive undertaking and would require much testing.

I think narrowing the changes to the flat config would be best to keep the impact small so we don't break any existing usecases whilst also supporting the new ones.

@Logicer16 One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

the entire scope manager bit of the ESLint is wrong too in this case

that is a massive undertaking and would require much testing

You've raised some good points. Making the required corrections would be a massive undertaking especially when it doesn't seem eslint documents the interface required of parsers doing "customized scope analysis", instead only describing their owneslint-scope. It wouldn't be worth making such drastic changes when you cannot be certain of eslint's expectations.

As for the nodes, while it may be possible for those using these types to use a simple type guard function which indexesESTree.NodeMap (or an custom AST's equivalent) to avoid manual casting, this solution may be overlooked, placing a significant burden on those using the types.

there's no reason that a parser's ArrayExpression remotely resembles estree's representation

That case is more likely an issue with the parser as it undermines the runtime type checking which thetype property intends to provide. Reusing a node type when it doesn't extend the existing type is the runtime equivalence of casting incompatible types which creates obvious issues.

Regardless, the strict parser typing may be something worth coming back to if/when these types are rewritten, but at this point I'll just simplify the config and have updated the pr to modify reflect that.

Hey @Logicer16,

😒 Your PR doesn't modify any tests, so it's hard to know what's being fixed, and your changes might regress in the future. Please consideradding tests to cover the change you're making. Including tests allows this PR to be merged by yourself and the owners of this module.

This can potentially save days of time for you!

@bradzacher Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

Good timing!JoshuaKGoldberg/eslint-plugin-package-json#125 ->ota-meshi/jsonc-eslint-parser#198 provides justification fo allowing non-ESTree AST nodes. In short: when nodes aren't a flavor of JavaScript, such as withjsonc-eslint-parser, they're not assignable to theESTree.Node type. But they do seem to work.eslint-plugin-package-json hasn't had issues around them yet.

ota-meshi/jsonc-eslint-parser#198 (comment) has the suggestion of allowingESTree.Node | { type: string, loc: SourceLocation } - as that seems to be the actual shape allowed by ESLint's APIs. Can I suggest this PR be modified to use that union type? cc @Logicer16 if you're up for it 🙌

Btw, +1 to the bot's request for tests corresponding to this change.

@JounQin,@JoshuaKGoldberg Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

⏳ Hi @Logicer16,

It's been a few days since this PR was approved by JounQin, bradzacher and we're waiting for a DT maintainer to give a review.

If you would like to short-circuit this wait, you can edit some of thetest files in the package which verify how the.d.ts files work. This would allow the PR to be merged by you or the DT module owners after a re-review.

@Logicer16 One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

@JoshuaKGoldberg

Actually I was surprised that@bradzacher and you partly agree to useany which is absolutely forbidden in@typescript-eslint codebase, and we even have a rule to forbid it specifically.

Haha, yeah, I'm a big proponent ofnot usingany whenever possible. And I don't love the idea of using it here - a place that much of the ecosystem uses. But since neither proper types nor declaration merging are feasible in typescript-eslint right now, we're kind of holding everyone up. 😬

Filed an issue on typescript-eslint here:typescript-eslint/typescript-eslint#8347

@Logicer16 I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Mar 4th (in a week) if the issues aren't addressed.

Oh, sorry I was busy on business work and forgot this PR.

I was against withany, forunknown if you're not against I'm fine with it with a little bit uncomfortable.

I'll try to migrate to declaration merging in another PR.

@JoshuaKGoldberg Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @Logicer16.

(Ping@pmdartus,@j-f1,@saadq,@JasonHK.)

@Logicer16: Everything looks good here. I am ready to merge this PR (ate1705c2) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@pmdartus,@j-f1,@saadq,@JasonHK,@bradzacher,@JounQin: you can do this too.)

Ready to merge