TypeStrong / atom-typescript

The only TypeScript package you will ever need
https://atom.io/packages/atom-typescript
MIT License
1.13k stars 205 forks source link

Plugin ignores @ts-ignore directives #1546

Closed kaiyoma closed 3 years ago

kaiyoma commented 4 years ago

If a line has // @ts-ignore on it, the plugin is not ignoring TypeScript errors on the following line. If I run the type checker from the command line, the line is correctly ignored.

lierdakil commented 4 years ago

You're probably getting reports from somewhere else. Either that, or it's an upstream issue. Otherwise, more info needed, because the plugin doesn't even handle @ts-ignore comments, it's entirely handled by the tsserver itself.

kaiyoma commented 4 years ago

I'm not sure what you mean by "getting reports from somewhere else" or "it's an upstream issue". atom-typescript reports the same type errors that I get when I run the command line tsc tool, except that the command line tool is correctly ignoring // @ts-ignore lines, but the Atom plugin is not.

What more info do you need? Happy to provide.

kaiyoma commented 4 years ago

Here's a really simple repro:

mkdir test
cd test
yarn init -y
yarn add typescript save-svg-as-png
touch index.ts

Then add this to index.ts:

// @ts-ignore
import saveSvgAsPng from 'save-svg-as-png';

saveSvgAsPng();

atom-typescript will flag the import with the error "Could not find a declaration file for module", but this shouldn't be happening because there's a // @ts-ignore on the previous line.

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-6.html#suppress-errors-in-ts-files-using--ts-ignore-comments

lierdakil commented 4 years ago

Can't reproduce, pretty much as expected.

Do you have other TypeScript plug-ins installed? F.ex. ide-typescript? That's what I meant by "from somewhere else"

kaiyoma commented 4 years ago

No, I don't have any other TypeScript plugins installed. If I disable atom-typescript, then the error goes away. If I re-enable the plugin, the error comes back, so I don't quite understand how this issue could be coming from anywhere else.

I can repro this problem on multiple laptops, running different OSes, with different repos.

Here's the repro (just as described above): https://imgur.com/a/YBSFQwu

lierdakil commented 4 years ago

It doesn't change the fact that I couldn't repro though...

But okay, I figured out what's going on thanks to your gif (great thinking!). So apparently you're using typescript in non-strict mode, which makes missing module declarations a "suggestion diagnostic" instead of an error. // @ts-ignore doesn't affect those, and tsc doesn't show them, so the behaviour is more or less as expected.

You have two options here: either use TypeScript in strict mode (specifically, set "noImplicitAny": true in tsconfig.json), then // @ts-ignore will work. Or you might ignore missing declaration suggestions entirely, in that case, add 7016 to ignoredSuggestionDiagnostics Atom-TypeScript option (added in newly released v13.6.0), as in the picture: image

The reason I've added the option is you probably don't want to globally turn off the error 7016, but it's the same diagnostic code, so ignoredDiagnosticCodes option wouldn't be the best choice in this context.

P.S. Sorry I didn't realize what's going on sooner, but there were some red herrings and my instincts of running tsc --init on new projects did add another layer of confusion.

kaiyoma commented 4 years ago

No, this isn't quite correct I'm afraid. For my work repos, we are using TypeScript in strict mode. We have this defined in tsconfig.json:

    "strict": true,

I've upgraded to the latest version of the plugin and I'm still seeing the issue. The command-line tool doesn't report any problems, but atom-typescript still is.

lierdakil commented 4 years ago

As I said, the "blue ones" are suggestion diagnostics, those aren't affected by // @ts-ignore and aren't reported by tsc, so it's not something particularly surprising. If you find that distracting, you can disable those by editing the Atom-TypeScript settings, as outlined in my comment above (on case-by-case basis). If I'm misunderstanding the issue, please attach a screenshot showing the issue so that we're on the same page at least.

kaiyoma commented 4 years ago

As I said, the "blue ones" are suggestion diagnostics, those aren't affected by // @ts-ignore and aren't reported by tsc,

This may technically be true, but the missing declaration file is also an error. In fact, atom-typescript is showing this message in both red and blue:

image

If I run the type checker on the command line, the error is reported:

$ yarn tsc --noEmit
.../index.tsx:5:17 - error TS7016: Could not find a declaration file for module 'save-svg-as
-png'. '.../node_modules/save-svg-as-png/lib/saveSvgAsPng.js' implicitly has an 'any' type.
  Try `npm install @types/save-svg-as-png` if it exists or add a new declaration (.d.ts) file containing `declare module 'save-svg-as-png';`

5 import SVG from 'save-svg-as-png';
                  ~~~~~~~~~~~~~~~~~

Found 1 error.

If I add the ts-ignore back in:

// eslint-disable-next-line @typescript-eslint/ban-ts-ignore
// @ts-ignore: Module not converted to TS yet
import SVG from 'save-svg-as-png';

The red error goes away, but the blue one hangs around:

image

And the command-line no longer reports errors:

$ yarn tsc --noEmit
Done in 37.62s.

I guess I'm finding it confusing as to why the plugin is behaving differently than the tool, since I would expect them to behave the same. If tsc doesn't report "suggestion diagnostics", why does the plugin? Can these blue messages be turned off completely so that the plugin and the tool match up?

lierdakil commented 4 years ago

Okay, a bit of background. There are actually two distinct tools TypeScript compiler provides:

  1. CLI compiler interface, tsc
  2. TypeScript Language Service Server daemon, tsserver

Most tools offering interactive information have to use the latter (i.e. tsserver), because using tsc directly would be too slow, and it would be missing a lot of features.

tsserver offers several "channels" for diagnostic messages, one of those being "suggestion" channel. tsc doesn't even get close to the code path associated with this channel, hence it's handled somewhat differently from other channels (which are reported by tsc).

AFAIK, originally, the "suggestion" channel was intended to be used for actionable code spans, for instance suggestion diagnostics 80001 "File is a CommonJS module; it may be converted to an ES6 module." and 80002 "This constructor function may be converted to a class declaration." have actions attached to them that would do the suggested refactoring automatically (implemented as intentions/code actions in Atom-TypeScript). However, at some point during development, it started reporting some "error" class diagnostics on that channel, too, particularly when those are explicitly disabled on the "error" channel. I don't really want to ignore all suggestion diagnostics, since that would be throwing away the baby with the bathwater -- there are some useful suggestion diagnostics after all. But I see the need to suppress some of those as irrelevant.

I don't really have any bright ideas on how to differentiate between useful and irrelevant diagnostics on the suggestion channel in a forward-compatible way. If you have any ideas, feel free to share. For now, I've added a way to selectively disable particular diagnostics on the "suggestion" channel. Relevant to the issue at hand, you can suppress the "missing declaration" diagnostics on the "suggestion" channel with the following setting: Ignore suggestion diagnostics option set to 7016 as I outlined above.

P.S. Some people might actually find these error reports on the suggestion channel useful, in "this doesn't make tsc spew errors, but it's an issue that needs to be fixed at some point" kind of way. I appreciate that this is personal preference.

P.P.S. Something I realized only after writing the message, on my system, tsserver doesn't actually report both the error and the suggestion diagnostics for "missing declaration", it's either "error" in the "strict" mode, or "suggestion" in "non-strict" mode. Perhaps you've stumbled upon some kind of bug in tsserver here? I can't really debug this since I can't reproduce on my end. If this is an OpenSource project, perhaps you could point me to the sources, so I could try to reproduce on the code base you're working with? If not, then perhaps this is caused by some particular combination of settings in tsconfig.json, perhaps you could at least share that? Thanks in advance. In the meantime, I'll go search for existing bug reports on the TypeScirpt repo just in case this is a known issue.

P.P.P.S. Also, what version of TypeScript does your project use? If it's not the latest, this might've been fixed upstream. Yes, I know, upgrading can be painful, I'm not suggesting that. Just trying to pinpoint the issue.

kaiyoma commented 4 years ago

Ahhhh, okay, this makes a lot more sense now. Thank you for the detailed explanation. Future readers of this thread will appreciate the explanation too, I'm sure.

An option to disable all messages from the suggestion channel could be useful, but I won't make a request for that at this time. For now, I can add 7016 to the exclusion list and that's fine for me.

This isn't an open source repo, so I can't share any code. Here's a sanitized version of our tsconfig:

{
  "compilerOptions": {
    "allowSyntheticDefaultImports": true,
    "alwaysStrict": true,
    "baseUrl": ".",
    "declaration": true,
    "declarationDir": "./typings",
    "esModuleInterop": true,
    "importHelpers": true,
    "jsx": "preserve",
    "module": "commonjs",
    "noFallthroughCasesInSwitch": true,
    "noImplicitReturns": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "outDir": "lib",
    "paths": {
      (tsconfig versions of all our webpack aliases)
    },
    "resolveJsonModule": true,
    "sourceMap": true,
    "strict": true,
    "target": "esnext",
    "typeRoots": ["./node_modules/@types", "types"]
  }
}

We're on the latest version of TypeScript (3.8.3).

lierdakil commented 4 years ago

Hi, a quick update, I've added an option to filter out most non-suggestion diagnostics reported on the suggestion channel. This can be enabled with atom-typescript.ignoreNonSuggestionSuggestionDiagnostics ("Ignore suggestion diagnostics that are not actionable suggestions" in UI) option.

github-actions[bot] commented 3 years ago

This issue has been marked as stale because it did not have any activity for the last 90 days or more. Remove the stale label or comment or this will be closed in 14 days