Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(eslint-plugin): [no-unnecessary-type-assertion] false positive on non-null assertion after an implicitly-any variable gets initialised inside conditional block#11082
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -118,9 +118,11 @@ export default createRule<Options, MessageIds>({ | ||
| if ( | ||
| // is it `const x!: number` | ||
| declaration.initializer == null && | ||
| declaration.exclamationToken == null | ||
| ) { | ||
| if (declaration.type == null) { | ||
| return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. We're definitely in the right ballpark, but I notice that this introduces a regression with the following case: functionfoo(){letxif(Math.random()>0.5){x=3;}else{x=4;}// should be flagged as unnecessaryx!;} I wonder if it's possible to get that case correct still? (it may or may not be feasible to prevent some false positives or negatives) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. actually - I think you can ignore this regression... This is most likely a symptom of#10334 /microsoft/TypeScript#60514 ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @kirkwaiblinger The regression also occurs in this simplified case:
As you mentioned, it's not possible to fix that case for now. Should I close this PR until that issue is resolved? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Hmmm. I'm not sure which choice is the lesser of two evils here... fixing the bug at the cost of that rather undesirable-looking regression, or marking the bug as blocked. I'm gently leaning towards making this work as blocked unless we can find away to proceed while mitigating this regression 🤔 But then again if it was that important of a case, you'd think we might have a test case that caught it? Looking for thoughts from @typescript-eslint/triage-team too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yeah I don't really want to wade into these murky waters 🙃. +1 on marking as blocked. | ||
| } | ||
| // check if the defined variable type has changed since assignment | ||
| const declarationType = checker.getTypeFromTypeNode(declaration.type); | ||
| const type = getConstrainedTypeAtLocation(services, node); | ||