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

Feature: getParserServices should throw informative error when used with non-typescript-eslint parser #6814

Closed
4 tasks done
RebeccaStevens opened this issue Apr 1, 2023 · 10 comments · Fixed by #8146
Closed
4 tasks done
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request

Comments

@RebeccaStevens
Copy link
Contributor

RebeccaStevens commented Apr 1, 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

utils

Playground Link

No response

Repro Code

const parserServices = getParserServices(context, true);

ESLint Config

{
  "parser": "@babel/eslint-parser",
  "parserOptions": {
    "ecmaVersion": "latest",
    "requireConfigFile": false,
    "babelOptions": {
      "babelrc": false,
      "configFile": false
    }
  }
}

tsconfig

{
  "compilerOptions": {
    "lib": ["ESNext"],
    "module": "commonjs",
    "moduleResolution": "node",
    "skipLibCheck": true,
    "target": "ESNext"
  },
  "files": ["file.ts"]
}

Expected Result

typeof parserServices should be ParserServicesWithoutTypeInformation

Actual Result

An error is thrown:

You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.

Additional Info

This check is testing for null or undefined but my context.parserServices is an empty object and thus the error is thrown.

https://github.com/typescript-eslint/typescript-eslint/blob/v6/packages/utils/src/eslint-utils/getParserServices.ts#L35-L39

Edit: Looks like parserServices isn't actually set directly on the context object but is instead being inherited through its prototype chain.

Versions

package version
@typescript-eslint/eslint-plugin 6.0.0-alpha.106
@typescript-eslint/parser 6.0.0-alpha.106
@typescript-eslint/scope-manager 6.0.0-alpha.106
@typescript-eslint/typescript-estree 6.0.0-alpha.106
@typescript-eslint/type-utils 6.0.0-alpha.106
@typescript-eslint/utils 6.0.0-alpha.106
TypeScript 5.0.3
ESLint 8.36.0
node 18.15.0
@RebeccaStevens RebeccaStevens added bug Something isn't working triage Waiting for maintainers to take a look labels Apr 1, 2023
@bradzacher
Copy link
Member

bradzacher commented Apr 1, 2023

I cannot reproduce this. I tried installing v6 of the parser and the plugin, then running naming-convention (which uses getParserServices(context, true)) and the rule runs fine without crashing.

const compilerOptions =
util.getParserServices(context, true).program?.getCompilerOptions() ?? {};

For interest's sake I did a console.log(context.parserServices) and it dumped out:

{
  program: null,
  esTreeNodeToTSNodeMap: WeakMap { <items unknown> },
  tsNodeToESTreeNodeMap: WeakMap { <items unknown> } 
}

Which is what I'd expect for our v6 changes.

In terms of the getParserServices logic:

  • As above - context.parserServices != null && context.parserServices.esTreeNodeToTSNodeMap != null && context.parserServices.tsNodeToESTreeNodeMap != null - so the first if is skipped entirely
  • For the second if - context.parserServices.program == null, however allowWithoutFullTypeInformation === true, so the second if is skipped as well.

So it returns without error.

but my context.parserServices is an empty object and thus the error is thrown

If it's an empty object then it's likely it was not provided by our parser - our parser has returned a complete object with properties for context.parserServices for a very, very long time (since v3.0.0 #716).

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Apr 1, 2023
@RebeccaStevens
Copy link
Contributor Author

RebeccaStevens commented Apr 1, 2023

Ah, I should have made it more clear. This issue occurs with Babel parser (I use Babel parser when testing my plugin in environments without TypeScript).

Should I add a check to my plugin to test what parser is bring used before calling this function? Or maybe this function could perform that check?

@bradzacher
Copy link
Member

bradzacher commented Apr 1, 2023

The general problem is that if a user uses babel as their parser then lint rules can subtly break in many ways - the babel AST has many differences compared to our AST.
Here are a bunch of examples where the AST differs.

People can happily use babel as their parser for JS code - but not for TS code.

We can consider handing this case in case the user wants to use a 3rd party rule on pure JS code parsed by another parser (though TBH they should still just use our parser everywhere). The issue is that we can't provide the maps at all in this case.

@RebeccaStevens
Copy link
Contributor Author

Maybe getParserServices(context, true) should just return null if the parser isn't @typescript-eslint/parser

@bradzacher
Copy link
Member

Perhaps a 3rd boolean param to explicitly opt-in to other parsers?
I don't like the idea of just making things silently pass when it could lead to breakages. I'd rather it if plugin authors had to explicitly opt-in as a way of saying "I solemnly swear I understand what I am doing and the problems I'm inviting.

@erictheswift
Copy link

Not sure if related, transient issue in WebStorm with same stacktrace: #6575 (comment)

@JoshuaKGoldberg JoshuaKGoldberg added triage Waiting for maintainers to take a look and removed awaiting response Issues waiting for a reply from the OP or another party labels Aug 5, 2023
@JoshuaKGoldberg
Copy link
Member

Agreed that silently passing when things should break would be confusing. I'd like to see it switched to throwing an error by default honestly. If you're trying to get type info in a rule and it's a babel AST, you probably wouldn't want the rule to run at all.

Maybe we can switch the existing second boolean param to a type union of boolean and an options object? Thoughts?

@JoshuaKGoldberg JoshuaKGoldberg added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Aug 17, 2023
@bradzacher
Copy link
Member

The usecase for "I want to support TS code as well as JS code and I want to support other parsers for JS and TS parser for TS and if and only if there are types then I want to consume the types" is pretty niche.

Most of the time it is instead "I want to support TS parser and consume types" or "I want to support any parser".

This is definitely a very, very rare case - which is why this is the first report of it.

We already advise against progressive enhancements with type information due to the weirdness it introduces by potentially hiding configuration problems - eg "oops I borked my config and this file wasn't type-aware parsed" should blow up - rather than having a rule silently turn features off and false-negativing.

I guess I would need to better understand the usecase for such flexibility before we consider this.

@JoshuaKGoldberg
Copy link
Member

Coming back to this: I do think we should throw an error if someone is using typed linting with a parser other than Babel. That's not a use case that's likely to work.

The real issue, then, is that the error should probably be more informative than the current:

You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.

Accepting PRs to add an error message that explicitly says... something around the lines of the parser needing to be ours. I haven't put much thought into wording 🙂

@JoshuaKGoldberg JoshuaKGoldberg added enhancement New feature or request and removed bug Something isn't working awaiting response Issues waiting for a reply from the OP or another party labels Oct 20, 2023
@JoshuaKGoldberg JoshuaKGoldberg changed the title Bug: v6 getParserServices(context, true) throwing error Feature: getParserServices should throw informative error when used with non-typescript-eslint parser Oct 20, 2023
@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 20, 2023
@JoshuaKGoldberg
Copy link
Member

We might want to have #8150 supercede this. Rules can't make much of a decision on this themselves. context has parserOptions and parserPath, for example, but third-party plugins such as Astro and Vue use their own parsers that wrap @typescript-eslint/parser. #8146 (comment)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 15, 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 enhancement New feature or request
Projects
None yet
4 participants