@types/eslint allow non-ESTree AST for parsers in flat configs

[Logicer16]

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change.
• Follow the advice from the readme.
• Avoid common mistakes.
• Run pnpm test <package to test>.

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: https://eslint.org/docs/latest/use/configure/migration-guide#custom-parsers
• If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the package.json.

---

(Original Changes for reference)

Loosens the types for the parser in a flat config to allow for ones which output a custom AST which does not extend ESTree's AST.

With the new flat config files, it allows for the config, and consequently the parser, to the typechecked. In its current state a compile-time error will occur on most uses of a custom parser.

[typescript-bot]

@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 PR in the Playground.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Only a DT maintainer can approve changes without 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.

---

Diagnostic Information: What the bot saw about this PR

{
  "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"
}

[typescript-bot]

🔔 @pmdartus @j-f1 @saadq @JasonHK @bradzacher @JounQin — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

[bradzacher]

I'm not quite sure I understand here - what's the motivation behind this?
Your PR description contains very little context.
Could you please provide more context and examples of real-world usecases that would be improved or enabled by this?

There's a whole lot of complexity being introduced here and it would be good to have a clear picture of why we'd add this complexity so we can determine if we're accepting the appropriate trade-offs.

[typescript-bot]

@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!

[Logicer16]

@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.

[typescript-bot]

@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?

[bradzacher]

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 (ref typescript-eslint/typescript-eslint#7935).

[Logicer16]

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

Test Config

import {Linter} from "eslint";
import htmlParser from "@html-eslint/parser";
import mdxParser from "eslint-mdx";
import * as graphQLParser from "@graphql-eslint/eslint-plugin";
import yamlParser from "yaml-eslint-parser";
import tomlParser from "toml-eslint-parser";
import jsonParser from "jsonc-eslint-parser";
import astroParser from "astro-eslint-parser";
import vueParser from "vue-eslint-parser";
import svelteParser from "svelte-eslint-parser";
import typescriptParser from "@typescript-eslint/parser"

export const myConfig: 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 is https://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 of ESTree.Comment have been updated to a new AST.Comment which aims to match isCommentToken and the docs. To partially* fix issues with vue-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 with jsonc-eslint-parser.
• Scope.Scope["type"] now allows any string as is permitted by the docs allowing "customized scope analysis for experimental/enhancement syntaxes". To fix issues with astro-eslint-parser

*With vue-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 by the interface described in the docs, these custom comment types will be filtered out by eslint's isCommentToken at runtime. I've opted to follow the behaviour in the source and only allow specific type strings.

[bradzacher]

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.

[Logicer16]

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 their type property is checked, they are generic ones. The only difference between ESTree.Node and ESTree.BaseNode is that the type property is restricted to just the type strings which ESTree provides whereas BaseNode allows any string. If any API method wants to use the additional properties of a specific node type, it must first check the type 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.

[bradzacher]

Sorry - you may have misunderstood me.

I mean cases like Variable

DefinitelyTyped/types/eslint/index.d.ts

Lines 67 to 73 in 4153f49

	interface Variable {
	name: string;
	scope: Scope;
	identifiers: ESTree.Identifier[];
	references: Reference[];
	defs: Definition[];
	}

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

Or DefinitionType which specifically returns some estree nodes:

DefinitelyTyped/types/eslint/index.d.ts

Line 93 in 4153f49

type DefinitionType =

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 like eslint-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 the NodeListener 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's ArrayExpression 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.

DefinitelyTyped/types/eslint/index.d.ts

Lines 649 to 669 in 4153f49

	type Node = AST.Node & NodeParentExtension;
	interface RuleListener extends NodeListener {
	onCodePathStart?(codePath: CodePath, node: Node): void;
	onCodePathEnd?(codePath: CodePath, node: Node): void;
	onCodePathSegmentStart?(segment: CodePathSegment, node: Node): void;
	onCodePathSegmentEnd?(segment: CodePathSegment, node: Node): void;
	onCodePathSegmentLoop?(fromSegment: CodePathSegment, toSegment: CodePathSegment, node: Node): void;
	[key: string]:
	| ((codePath: CodePath, node: Node) => void)
	| ((segment: CodePathSegment, node: Node) => void)
	| ((fromSegment: CodePathSegment, toSegment: CodePathSegment, node: Node) => void)
	| ((node: Node) => void)
	| NodeListener[keyof NodeListener]
	| undefined;
	}

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 including parent isn't technically correct - eg the Program node always has parent: undefined.

Similarly the methods on SourceCode are typed by default to return T 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.

[bradzacher]

RC for the above comment.

[typescript-bot]

@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]

@JoshuaKGoldberg

Actually I was surprised that @bradzacher and you partly agree to use any 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 of not using any 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

[typescript-bot]

@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.

[bradzacher]

FWIW we just shipped flat config support in typescript-eslint.

To make things compatible with the @types/eslint types as well as other ASTs I went with the loose types for flat configs:

• https://github.com/typescript-eslint/typescript-eslint/blob/4bc6944f880570273d8486d07bbac63186c8dfe0/packages/utils/src/ts-eslint/Parser.ts#L19-L52
• https://github.com/typescript-eslint/typescript-eslint/blob/4bc6944f880570273d8486d07bbac63186c8dfe0/packages/utils/src/ts-eslint/Processor.ts#L49-L86
• https://github.com/typescript-eslint/typescript-eslint/blob/4bc6944f880570273d8486d07bbac63186c8dfe0/packages/utils/src/ts-eslint/Rule.ts#L645-L678

And I also omitted the configs from a plugin definition to ensure there's no clash with the legacy configs.
https://github.com/typescript-eslint/typescript-eslint/blob/4bc6944f880570273d8486d07bbac63186c8dfe0/packages/utils/src/ts-eslint/Config.ts#L168-L176

So I'll reiterate that I'm strongly in favour of this approach - so much so that I "put my money where my mouth is" and shipped it.

---

@JounQin if you want to block this in lieu of your dream of a fully generic library definition I'll just say that I disagree with you and think you're making the wrong trade-off. But I obviously can't do anything to change your mind in that so I wish you all haste in designing and implementing that so that we can unbreak users ASAP.

This is my final stamp.

@JounQin
If you are putting your foot down and blocking this for the generic system - please comment so that @Logicer16 can know to close the PR.
Otherwise please stamp this so that it can land.

[JounQin]

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

I was against with any, for unknown 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.

[typescript-bot]

@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?

[typescript-bot]

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.)

[typescript-bot]

@Logicer16: Everything looks good here. I am ready to merge this PR (at e1705c2) 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.)

[Logicer16]

Ready to merge