typescript-eslint / typescript-eslint

:sparkles: Monorepo for all the tooling which enables ESLint to support TypeScript
https://typescript-eslint.io
Other
15.2k stars 2.72k forks source link

Bug: [no-unsafe-call] Unclear message "Unsafe call of an `error` type typed value" (especially in JSDoc) #9591

Open karlhorky opened 2 months ago

karlhorky commented 2 months ago

The language and UX around no-unsafe-call (and similar rules) messages indicate "an error type typed value", which seems unclear and maybe also misleading.

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

Playground Link

https://typescript-eslint.io/play/#ts=5.5.2&fileType=.js&code=MYewdgzgLgBApgGxgXhgegFQZgASgTwAc4YBvAUQTgFs4woBfGDNGAClIYEoAoHxAHQALAE5wAZgICWYYAgCuAEzgQ2AcgCGAI2BreQA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1tieQEMAZolpk%2B8eOiiJo0DtEjgwAXxBKgA&tsconfig=&tokens=false

Repro Code

const el = /** @type {Element} */ ({})

el.href.includes('abc')

Screenshot of typescript-eslint playground, showing IntelliSense hover with the message "Unsafe call of an `error` type typed value"

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/<rule-name>": ["error", ...<options>],
  },
};

tsconfig

{
  "compilerOptions": {
  }
}

Expected Result

I expected the error message:

Unsafe call of a method on an `any` typed value

Differences:

Actual Result

I observed the error message:

Unsafe call of an `error` type typed value

Confusing parts:

Using this in TypeScript or with checkJs: true leads to a more understandable experience:

IntelliSense hover over "href" showing the error message "Property 'href' does not exist on type 'Element'."

Hovering over href in el.href shows that TypeScript is reporting an error:

Property 'href' does not exist on type 'Element'.

Additional Info

This also affects other rules, since they also create messages the same way.

The type for this seems to be coming from the ts-api-utils function isIntrinsicErrorType

cc @JoshuaKGoldberg

github-actions[bot] commented 2 months ago

Uh oh! @karlhorky, the image you shared is missing helpful alt text. Check your issue body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

πŸ€– Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

bradzacher commented 2 months ago

These messages were recently added to help people differentiate between a real any and an any due to a type that TS can't resolve (eg due to bad config or something).

Often times (not always) an "error" any comes from an out-of-sync workspace (eg you're using project references and changed another project and ts-eslint hasn't picked it up). Which is why we tried to differentiate them - so people don't get as confused about why vscode says the type is there but eslint says it's any.

More than welcome a PR to improve the message to make that clearer.

karlhorky commented 2 months ago

That motivation sounds pretty good - differentiating between these causes for any sounds like a helpful thing for users πŸ‘

So, understood - an implementation of improving error messages to address this issue should avoid moving back to the original, undifferentiated any message.

JoshuaKGoldberg commented 2 months ago

Ha, yeah, I'd meant to file an issue like this once most the PRs moving to "an error typed value" were in. This is great. Thanks!

For reference:

kiliancs commented 2 months ago

In our monorepo, the message for the error type comes up a lot, and developers are often puzzled as to why.

Of course there's probably always a legit cause, but the cause is often in a far away file that TypeScript or eslint are caching up to. Restarting the eslint server often resolves the issue, while other times we need to perform an intermediate build step. We have a number of intermediate build steps that definitely contribute to this situation (and we are working to address this).

Still, regardless of the specifics of our circumstance, I understand that when the error type occurs, there is a TypeScript error somewhere. Is that right? If so, the additional errors raised by no-unsafe-<rule> may actually become a distraction from the actual TypeScript error, and the code that is being flagged is often (although not necessarily) just fine.

Would typescript-eslint consider adding an option to disable reporting when the issue is an error type? If the response is favorable, I am open to contributing this change.

Happy to create a separate issue for this, but the discussion here seems highly relevant, my point being the confusion may be avoidable, besides message updates.

karlhorky commented 2 months ago

Would typescript-eslint consider adding an option to disable reporting when the issue is an error type? If the response is favorable, I am open to contributing this change.

Hm, I would personally not want the problem to be disabled in this case - I want it to be reported. Even though the message is confusing, it does indicate a problem that needs to be fixed, which I don't think should be swallowed.

Still, regardless of the specifics of our circumstance, I understand that when the error type occurs, there is a TypeScript error somewhere. Is that right? If so, the additional errors raised by no-unsafe-<rule> may actually become a distraction from the actual TypeScript error, and the code that is being flagged is often (although not necessarily) just fine.

However, your sentences here had me wondering - @JoshuaKGoldberg would it be possible to report the original TypeScript error (or at least the file path + line number + character number)? I would imagine implementation of this is either pretty easy to implement or very, very hard πŸ€”

kiliancs commented 2 months ago

Thanks for the very quick response @karlhorky !

Even though the message is confusing, it does indicate a problem that needs to be fixed, which I don't think should be swallowed.

At the risk of repeating myself, I wanted to clarify that I am very much on board with not swallowing errors in general. Here, however, there will already be an error, and I would prefer not to propagate it.

Note I won't be able to build my project with that error, and that the error may even be in the same file or in a file that I have open; I might even be introducing the error as I'm editing the file. TypeScript has taken this policy of trying to report the error at its source, and not propagate it (citation/examples needed), and I think that policy would also be beneficial here.

karlhorky commented 2 months ago

there will already be an error

Hm, I wonder if sometimes this is not true. πŸ€” In my experience in the last days since we turned this on, it's appeared as if the TypeScript error can be somewhere unobservable, obscured by various settings.

Generally, I get where you're going, that if there is an error somewhere else, then it can be confusing to propagate / re-report it elsewhere.

One more point: I wonder if also having that context at the usage location (where typescript-eslint reports) can be valuable context for developers at all...

Josh-Cena commented 2 months ago

This rule's behavior has always been like this (reporting on error-typed values), so I'm not sure why a change in the message motivates a change to its behavior. Nevertheless @kiliancs I would welcome another proposal for your desired change and we can discuss.

JoshuaKGoldberg commented 2 months ago

report the original TypeScript error ... it's appeared as if the TypeScript error can be somewhere unobservable, obscured by various settings.

The root cause is very often https://github.com/microsoft/vscode-eslint/issues/1774 or something like it. In this issue's terminology, "error" refers to the inability for TypeScript to create a type for a node. That's not the same as a type error. We don't get nice source locations for these, just an "error type".

KrappRamiro commented 2 months ago

In my case, restarting my EsLint server in VsCode fixed the issue

Image shows the Command Pallete of Vscode, highlighting the Restart Eslint server command

github-actions[bot] commented 2 months ago

Uh oh! @KrappRamiro, the image you shared is missing helpful alt text. Check https://github.com/typescript-eslint/typescript-eslint/issues/9591#issuecomment-2259514239.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

πŸ€– Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.