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

@ts-expect-error and @ts-ignore overly broad in supressing errors #47551

Closed
MLoughry opened this issue Jan 21, 2022 · 4 comments
Closed

@ts-expect-error and @ts-ignore overly broad in supressing errors #47551

MLoughry opened this issue Jan 21, 2022 · 4 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@MLoughry
Copy link

Bug Report

We migrated our codebase with >1.5M lines of TS code to use strict mode everywhere, resulting in >30k new errors caught by strict mode. To ease the migration, we annotated each error with // @ts-expect-error and additional comments pointing to a wiki about the migration. However, we're finding the disable comment to be overly-broad, suppressing new errors that are unrelated to the line in question or strict mode. In particular, adding a new property to a type doesn't flag instances where we have a @ts-expect-error suppressing a strictNullChecks error.

🔎 Search Terms

ignore, expect, error, @ts-ignore, @ts-expect-error

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about @ts-ignore and @ts-expect-error

⏯ Playground Link

https://www.typescriptlang.org/play?ts=4.5.4#code/JYOwLgpgTgZghgYwgAgGIHt3IN4ChnJwBcyIArgLYBG0A3PslSQM5hSgDm9AvrrgPT9kAUQAeABwgJIAE2TQo6KMhlkUYLAAMyIGRBigIMzcnGLJUMAE9kmuJoA0AoVTJhkzMuLMRmzX-JQisqq6lgUwH6cpubQ1rZUmrgI6CCsjHBQJBhYALw4DILIAAJgzAC0EBJSYJVBSgzEyDp6BiBGuLzOyADC6BTiwAA20MgpQTVDNgpKzMipyGAAFig0Q+gA7shDhsgAFJopae5UcABe2ZjI+diaAJTdoYtYyygRUSAcMegW8ZqJyVS6VOFzQVxujRIAEZOkA

Playground link with relevant code

💻 Code

interface Foo {
  a: number;
  b: string;
}

// Expected error due to `undefined` property `a`,
// but suppresses error due to missing property `b`
const bar: Foo = {
  // @ts-expect-error
  a: undefined
}

// Compiler correctly errors on the below line (`const baz: Foo = {`)
// due to the missing property `b`
const baz: Foo = {
  a: 1
}

🙁 Actual behavior

Compiler suppresses error about missing property.

🙂 Expected behavior

Compiler raises error about missing property that is not on the line covered by @ts-expect-error

@RyanCavanaugh
Copy link
Member

Duplicate #38409, #19139

@RyanCavanaugh RyanCavanaugh added Duplicate An existing issue was already created and removed Duplicate An existing issue was already created labels Jan 21, 2022
@RyanCavanaugh
Copy link
Member

On second viewing, this isn't a duplicate.

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Jan 22, 2022
@RyanCavanaugh
Copy link
Member

In this very specific case, we could turn off the rule that says we shouldn't report missing property errors when a type error between a declared and provided property don't match. But there's not really much we can do in the general case -- once your program has a type error in it, it's up to our heuristics to decide exactly what the most useful reporting of that type error is. It might be in one place, it might be in four places, it might be in ten. The feature design of ts-ignore is intentionally extremely simple and it's not tractable to turn it into a more complex system.

@dbalatero
Copy link

@MLoughry I figured out how to use the TypeScript AST to detect problem spots where ts-expect-error might be hiding other errors:

https://gist.github.com/dbalatero/82bb17596fe4e8b7d1c5d56d8f6018fb

If you want to try it on your codebase, you can try dropping it in and wrapping it in a little script like I show in the README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

3 participants