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

Bug: ESLint parser and plugin have wrong types for programatic use in ESLint flat config #7494

Closed
4 tasks done
jaydenseric opened this issue Aug 17, 2023 · 7 comments
Closed
4 tasks done
Labels
bug Something isn't working working as intended Issues that are closed as they are working as intended

Comments

@jaydenseric
Copy link

jaydenseric commented Aug 17, 2023

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Relevant Package

  • parser
  • eslint-plugin

Playground Link

No response

Repro Code

ESLint has a new “flat” config system:

Unfortunately, when attempting to import and use the TypeScript ESLint plugin and parser within the ESLint config of type import("eslint").Linter.FlatConfig they result in TypeScript errors.

In eslint.config.js:

// @ts-check

import eslintPluginTypescript from "@typescript-eslint/eslint-plugin";
import eslintParserTypescript from "@typescript-eslint/parser";

/**
 * ESLint config.
 * @satisfies {Array<import("eslint").Linter.FlatConfig>}
 */
const eslintConfig = [
  {
    languageOptions: {
      parser: eslintParserTypescript,
    },
    plugins: {
      "@typescript-eslint": eslintPluginTypescript,
    },
  },
];

export default eslintConfig;

ESLint Config

No response

tsconfig

No response

Expected Result

The TypeScript ESLint plugin and parser should be usable within ESLint “flat” config of type import("eslint").Linter.FlatConfig without causing TypeScript errors.

Actual Result

The languageOptions.parser has this TypeScript error:

Type 'typeof import("[redacted]/node_modules/@typescript-eslint/parser/dist/index")' is not assignable to type 'ParserModule | undefined'.
  Type 'typeof import("[redacted]/node_modules/@typescript-eslint/parser/dist/index")' is not assignable to type 'ObjectMetaProperties & { parseForESLint(text: string, options?: any): ESLintParseResult; }'.
    Type 'typeof import("[redacted]/node_modules/@typescript-eslint/parser/dist/index")' is not assignable to type '{ parseForESLint(text: string, options?: any): ESLintParseResult; }'.
      The types of 'parseForESLint(...).ast.comments' are incompatible between these types.
        Type 'Comment[] | undefined' is not assignable to type 'Comment[]'.
          Type 'undefined' is not assignable to type 'Comment[]'.

The plugins["@typescript-eslint"] has this TypeScript error:

Type '{ configs: Record<string, Config>; rules: TypeScriptESLintRules; }' is not assignable to type 'Plugin'.
  Types of property 'rules' are incompatible.
    Type 'TypeScriptESLintRules' is not assignable to type 'Record<string, RuleModule | ((context: RuleContext) => RuleListener)>'.
      'string' index signatures are incompatible.
        Type 'RuleModule<string, unknown[], RuleListener>' is not assignable to type 'RuleModule | ((context: RuleContext) => RuleListener)'.
          Type 'RuleModule<string, unknown[], RuleListener>' is not assignable to type 'RuleModule'.
            Types of property 'create' are incompatible.
              Type '(context: Readonly<RuleContext<string, unknown[]>>) => RuleListener' is not assignable to type '(context: RuleContext) => RuleListener'.
                Types of parameters 'context' and 'context' are incompatible.
                  Type 'RuleContext' is not assignable to type 'Readonly<RuleContext<string, unknown[]>>'.
                    The types of 'parserOptions.ecmaVersion' are incompatible between these types.
                      Type '3 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 | 2015 | 2016 | 2017 | 2018 | 2019 | 2020 | 2021 | 2022 | 2023 | 2024 | "latest" | undefined' is not assignable to type '"latest" | EcmaVersion | undefined'.
                        Type '14' is not assignable to type '"latest" | EcmaVersion | undefined'.
Screenshot 2023-08-17 at 2 32 12 pm

Additional Info

The TypeScript ESLint project should be using the same ESLint types from @types/eslint like the rest of the ecosystem is using, instead of internally maintaining separate types. Where there are problems with the @types/eslint types they should be fixed in contributions so the entire ecosystem benefits.

This attitude is upsetting:

/*
We intentionally do not include @types/eslint.
This is to ensure that nobody accidentally uses those incorrect types
instead of the ones declared within this package
*/

I have run into frustrations before because Microsoft have not been participating in contributing to the main ESLint types the community is using, and as a consequence they are of poor quality.

Versions

package version
@typescript-eslint/eslint-plugin 6.4.0
@typescript-eslint/parser 6.4.0
TypeScript 5.1.6
ESLint 8.47.0
node 20.5.0
@jaydenseric jaydenseric added bug Something isn't working triage Waiting for maintainers to take a look labels Aug 17, 2023
@bradzacher
Copy link
Member

bradzacher commented Aug 17, 2023

We don't officially support flat configs yet - they aren't truly production ready and there are various issues for us to add support for them that the ESLint team has yet to resolve and document.

Once the system is more stable and the ESLint team has solved the backwards-compatibility problem, we'll look to supporting things.

Rest assured that we are talking and working with the ESLint team as best we can to progress things.


It is expected that the types are incorrect. @types/eslint uses @types/estree. The latter is fundamentally incompatible with our tooling because we have our own AST types for our "typescript-estree" representation.
There's nothing that we can do about this - there is just no way for us to augment @types/estree with our AST extensions - this is a limitation of TS.

This is also why we explicitly provide our own types for eslint - because we can't redirect @types/eslint away from @types/estree.

This isn't an "us vs them" or a "we don't want to worry about DefinitelyTyped" attitude - this is just a reality of working within the limitations of TS - our hands are just tied.


Once we add support for flat config types (#7273) then you can use our types instead and things should work then. Though note that the types aren't us supporting flat configs as I mentioned above.

@bradzacher bradzacher closed this as not planned Won't fix, can't repro, duplicate, stale Aug 17, 2023
@bradzacher bradzacher added working as intended Issues that are closed as they are working as intended and removed triage Waiting for maintainers to take a look labels Aug 17, 2023
@jaydenseric
Copy link
Author

jaydenseric commented Aug 17, 2023

@types/eslint should be redone in such a way using generics and things so that the ESTree AST isn't assumed; ideally they could derive the AST types from the configured parser by manually providing the AST types so they are correct for given situations.

Is the TypeScript ESLint AST a superset of the ESTree one? Then maybe the @types/eslint types referring to the AST could allow a record of additional unknown properties on the AST types.

Or maybe the types for ESLint configs and plugins should not try to make assumptions about the AST produced by parsers at all.

The ESLint config types need to account for being able to have different parsers and ASTs accordingly for different files within a single config. Does the flat config types in #7273 account for that, or does it assume every file, plugin, and rule is using the AST from the TypeScript parser?

I still disagree that it's ok to pretend that @types/eslint doesn't exist and for projects to use types instead provided from just one parser of many they could be using in their ESLint config. Does every ESLint parser that produces a non ESTree AST need to re-invent all the ESLint types? Ideally the ecosystem uses and maintains one base set of types.

It's hard to accept the premise that no types in @types/eslint could be used at all by TypeScript ESLint internals or exported types. There are 1209 lines of types, and TypeScript ESLint is using none of it at all. Absolutely a lot of that could be exported and reused, or be improved to make that re-use possible.

If @types/eslint types for things like RuleTester have incorrect hard-coded assumptions about AST, then those should be fixed. At the very least types for things that are universal to ESLint regardless of the parsers used should be exported in a clean way from @types/eslint so they could be referenced in TypeScript ESLint types.

For example, look at the LintResult interface from @types/eslint:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/79827c35dddb3992189c59273099c795a16492b0/types/eslint/index.d.ts#L1116-L1128

    interface LintResult {
        filePath: string;
        messages: Linter.LintMessage[];
        suppressedMessages: Linter.SuppressedLintMessage[];
        errorCount: number;
        fatalErrorCount: number;
        warningCount: number;
        fixableErrorCount: number;
        fixableWarningCount: number;
        output?: string | undefined;
        source?: string | undefined;
        usedDeprecatedRules: DeprecatedRuleUse[];
    }

I can't see TypeScript AST specific things in there (even if there was, the AST types could be passed in by adding some generic args). Compare the exact same interface in TypeScript ESLint:

/**
* The LintResult value is the information of the linting result of each file.
*/
export interface LintResult {
/**
* The number of errors. This includes fixable errors.
*/
errorCount: number;
/**
* The number of fatal errors.
* @since 7.32.0
*/
fatalErrorCount?: number;
/**
* The absolute path to the file of this result. This is the string "<text>" if the file path is unknown (when you
* didn't pass the options.filePath option to the eslint.lintText() method).
*/
filePath: string;
/**
* The number of errors that can be fixed automatically by the fix constructor option.
*/
fixableErrorCount: number;
/**
* The number of warnings that can be fixed automatically by the fix constructor option.
*/
fixableWarningCount: number;
/**
* The array of LintMessage objects.
*/
messages: ESLint.LintMessage[];
/**
* The source code of the file that was linted, with as many fixes applied as possible.
*/
output?: string;
/**
* The original source code text. This property is undefined if any messages didn't exist or the output
* property exists.
*/
source?: string;
/**
* The array of SuppressedLintMessage objects.
*
* @since 8.8.0
*/
suppressedMessages?: SuppressedLintMessage[];
/**
* The information about the deprecated rules that were used to check this file.
*/
usedDeprecatedRules: DeprecatedRuleInfo[];
/**
* The number of warnings. This includes fixable warnings.
*/
warningCount: number;
}

It's got nice JSDoc descriptions of every property for good intellisense. None of that quality has been contributed to the wider ESLint ecosystem.

@jaydenseric
Copy link
Author

Here is an example where Microsoft is not contributing type fixes for ESLint to the wider ecosystem in @types/eslint.

I had to fix @types/eslint with a PR to add "off" to the available options for ESLint config globals entries:

DefinitelyTyped/DefinitelyTyped#62941

The ESLint types maintained internally in the TypeScript ESLint project had that value for the last 2 years, and didn't contribute the better types for the wider ecosystem:

export type GlobalVariableOptionBase = 'off' | 'readonly' | 'writable';

@bradzacher
Copy link
Member

Note that this project is not affiliated with (or even sponsored by) Microsoft.
We are an independent project run by volunteers.


The reason that we cannot just augment @types/estree is because
a) you cannot change an existing property's type through declaration merge augmentation
b) you cannot declaration merge type aliases - which are used extensively throughout AST types to name the node unions.

Is the TypeScript ESLint AST a superset of the ESTree one? Then maybe the @types/eslint types referring to the AST could allow a record of additional unknown properties on the AST types.

This is also why you can't easily define @types/eslint generically unless you define unconstrained generics and thread those unconstrained generics through everywhere - which makes the types quite complicated to maintain.
It's doable but it's a lot of effort given that the types are fully maintained by volunteers.


Does the flat config types in #7273 account for that, or does it assume every file, plugin, and rule is using the AST from the TypeScript parser?

Right now we haven't really thought too deeply about the usecase of a truly strictly typed config. Most users aren't looking for strictly typed configs given ESLint config files are required to be .js and they are strictly runtime-validated by ESLint.


It's entirely possible we could reuse some parts of @types/eslint. There are parts of it that are AST-agnostic. However again we come back to the fact that @types/eslint depends on @types/estree.
This is a BIG problem for DevX because TS doesn't discriminate between transient and explicit dependencies when it comes to auto-imports. This means if we have a setup like @typescript-eslint/utils -> @types/eslint -> @types/estree then when a user is defining a rule they might accidentally auto-import an AST type from @types/estree - which would be the wrong type and would then give them weird errors!

DefinitelyTyped isn't designed in a way that allows you to break up a package - each @types package has to correspond to a real package. This means you can't split up @types/eslint into two packages (one with the AST-agnostic types and one without), and you can't remove the dependency on @types/estree - so it doesn't make sense for us to use @types/eslint.


So why didn't we contribute our types upstream to @types/eslint after we wrote such nice, well-annotated types? Well because we're unpaid volunteers that maintain this project in our free time. Preparing such a PR to DT takes a lot of time and energy.

Any community member has been free to contribute our types to the DT repo - but nobody has been so motivated to volunteer their time either. Everything is maintained by volunteers and as such people put there time where they see the most value, and nobody has seen enough value.

If you see such value - you're welcome and encouraged to contribute our improvements! Be the change you want to see!

@bradzacher
Copy link
Member

It's worth noting that you're coming into this from the fresh perspective of a user who has run into troubles with types for a brand new, not-ready-for-production feature.

However this is the first time in the ecosystem that people have actively needed to import and pass a parser, a plugin OR a shared config somewhere.

Previously (for the entire lifetime of ESLint) it was taboo to explicitly import/require a parser or plugin directly and instead all the importing was done by ESLint itself via the paths or identifiers you pass.

As such our approach suited the ecosystem well! From the perspective of a consumer of the ESLint types:

  • when adding types to a "legacy config" you could use either our types or @types/eslint and have the same experience as everything is stringly typed.
  • when writing scripts that coordinated ESLint you could choose to use either our types or @types/eslint and have exactly the same experience.
  • when writing ESLint rules:
    • For pure JS you could use either types and have a similar experience as our AST is a superset it works for either - but it would be clunky due to the additional types in the unions.
    • For TS you would only use our types so you would have the correct AST.

This is the first time ever that parsers, plugins and configs have had to even consider the types that they export. It's a big turning point in the ecosystem.

As I mentioned - we haven't moved to land support for flat configs yet on purpose because they are not production ready, nor are they officially released. There is still a lot the ESLint team need to do to get them to a good state for project like ours to add support.

This is a space that's actively being explored and worked on as our limited volunteer time dictates.

@jaydenseric
Copy link
Author

jaydenseric commented Aug 17, 2023

It's worth noting that you're coming into this from the fresh perspective of a user who has run into troubles with types for a brand new, not-ready-for-production feature.

However this is the first time in the ecosystem that people have actively needed to import and pass a parser, a plugin OR a shared config somewhere.

No. I've needed and used ESLint related types for various projects for years now, and noticed how bad they were. I also noticed that Microsoft had been working with higher quality typings for ESLint internally within TypeScript ESLint and felt dismay; here is an example of a private group chat discussion on this topic:

Screenshot 2023-08-17 at 9 25 15 pm Screenshot 2023-08-17 at 9 27 53 pm Screenshot 2023-08-17 at 9 25 35 pm

The ecosystem is more than just inexperienced consumers trying to use ESLint in a project. It also includes package authors publishing plugins, etc.

ESLint flat config is not the only reason the ecosystem needs quality types in @types/eslint.

This is the first time ever that parsers, plugins and configs have had to even consider the types that they export. It's a big turning point in the ecosystem.

Anything that's published JavaScript should have good types. It's disappointing that key maintainers only start to care about types once millions of consumers start rioting and raising issues. The story for a package author can be a frustrating nightmare as soon as you try to publish a plugin or something normies don't do often. As an ecosystem we really need to start prioritising quality for package creators more than for consumers.

Quite a few ESLint plugin authors probably started out attempting to nicely type their plugins, got frustrated with the poor ESLint types available, and just published vanilla. Here we are down the road and it's now hard to cleanup the types for whole ecosystem of plugins, and the people who did use the old types will probably have to migrate to whatever the final higher quality types end up being. And the problem eventually ripples up to the millions of users when they start defining config in type safe JavaScript. Prevention is the cure.

@bradzacher
Copy link
Member

bradzacher commented Aug 17, 2023

I also noticed that Microsoft had been working with higher quality typings for ESLint internally within TypeScript ESLint and felt dismay

Again - Microsoft is in no way affiliated with this project.
This project has no Microsoft employees working on it, nor does it belong to Microsoft, nor is it even sponsored by Microsoft.

This is a purely community maintained project.
We maintainers are volunteers that do it for the love of the craft.


We have docs on how to build rules using our tooling.
https://typescript-eslint.io/developers/custom-rules

These docs explicitly warn against using the @types/ packages.
There are several hundred plugins published to NPM using our tooling.

We have published our own types and tooling for writing rules in typescript for typescript as far back as 4 years ago (#425).


I'm sorry that you're having a bad time with the @types/ packages - but they are not maintained or owned by us.

At this point your tone and language are very hostile and as a volunteer maintainer I'm no longer going to engage in this, so I'm going to end this discussion here.
Have a nice day.

@typescript-eslint typescript-eslint locked as too heated and limited conversation to collaborators Aug 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working working as intended Issues that are closed as they are working as intended
Projects
None yet
Development

No branches or pull requests

2 participants