fix(eslint-plugin): [no-unnecessary-type-assertion] conflict with TS for variables used before assignment #9209
Conversation
|
Thanks for the PR, @liuxingbaoyu! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
no-unnecessary-type-assertion conflict with TS for variables used before assignmentno-unnecessary-type-assertion conflict with TS for variables used before assignment
Edit: d'oh, no, I'm wrong - this PR is ready for review as-is after all! |
There was a problem hiding this comment.
Thanks for starting on this! I'm marking this as a draft because the root accepted issue is #4513, which adds on the idea of lets. Feel free to un-draft and re-request review once it's ready to address those too. Cheers! ✨
(I haven't looked at the source yet - in case it gets refactored to handle lets)
Edit: sorry, I was confused for a moment - ignore the drafting.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts
Show resolved
Hide resolved
packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts
Show resolved
Hide resolved
packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts
Show resolved
Hide resolved
|
👋 Hey @liuxingbaoyu ! Just checking in, is this still something you have time for? No worries if not - you got us 98% of the way there, so we can take care of the last few chores and merge it in for you if you like. But if you have a few minutes to take care of the last few chores.... then it saves us a few minutes ❤️ |
|
@kirkwaiblinger Sorry for forgetting about it. Thanks! I will finish it soon! |
ea318d9 to
7891100
Compare
| const declaratorNode: TSESTree.VariableDeclaration = | ||
| services.tsNodeToESTreeNodeMap.get(declaration); |
There was a problem hiding this comment.
I noticed it when rebasing, I'm not sure if this will be affected.🤔
#9660
There was a problem hiding this comment.
@auvred Could you double check this?
There was a problem hiding this comment.
@liuxingbaoyu Thanks for calling this out. Good attention to detail!
There was a problem hiding this comment.
Sorry for such a long reply! I missed the notification :(
I think we can't just rely on services.tsNodeToESTreeNodeMap.get when dealing with declarations, as these declarations may be contained in other files that tsNodeToESTreeNodeMap doesn't know about.
Imagine the following:
// var-declaration.ts
var varDeclarationFromFixture = 1;
// file.ts
varDeclarationFromFixture!;The rule throws runtime error on this code.
FAIL tests/rules/no-unnecessary-type-assertion.test.ts
● Test suite failed to run
TypeError: Converting circular structure to JSON
--> starting at object with constructor 'Object'
| property 'expression' -> object with constructor 'Object'
--- property 'parent' closes the circle
at stringify (<anonymous>)
at messageParent (../../node_modules/jest-worker/build/workers/messageParent.js:29:19)
Test Suites: 1 failed, 1 total
Tests: 0 total
Snapshots: 0 total
Time: 5.074 s
Ran all test suites matching /no-unnecessary-type-assertion/i.
Since it requires more than one file to reproduce, I can't reproduce it in online playground, so here's the patch:
diff --git a/packages/eslint-plugin/tests/fixtures/tsconfig.json b/packages/eslint-plugin/tests/fixtures/tsconfig.json
index 6ae5e6473..c16815aaf 100644
--- a/packages/eslint-plugin/tests/fixtures/tsconfig.json
+++ b/packages/eslint-plugin/tests/fixtures/tsconfig.json
@@ -12,6 +12,7 @@
"file.ts",
"consistent-type-exports.ts",
"mixed-enums-decl.ts",
- "react.tsx"
+ "react.tsx",
+ "var-declaration.ts"
]
}
diff --git a/packages/eslint-plugin/tests/fixtures/var-declaration.ts b/packages/eslint-plugin/tests/fixtures/var-declaration.ts
new file mode 100644
index 000000000..5253648a8
--- /dev/null
+++ b/packages/eslint-plugin/tests/fixtures/var-declaration.ts
@@ -0,0 +1 @@
+var varDeclarationFromFixture = 1;
diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts
index afc0b3c3b..b636eec84 100644
--- a/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts
+++ b/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts
@@ -1088,6 +1088,20 @@ const bar = foo.a;
},
{
code: `
+varDeclarationFromFixture!;
+ `,
+ output: `
+varDeclarationFromFixture;
+ `,
+ errors: [
+ {
+ messageId: 'unnecessaryAssertion',
+ line: 3,
+ },
+ ],
+ },
+ {
+ code: `
var x = 1;
x!;
`,You can run git apply to apply it locally.
There was a problem hiding this comment.
Thanks for the test! It seems we can skip it in this case, since the declaration in the different file never appears used before assignment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9209 +/- ##
==========================================
- Coverage 88.07% 87.94% -0.14%
==========================================
Files 402 403 +1
Lines 13691 13806 +115
Branches 3982 4027 +45
==========================================
+ Hits 12058 12141 +83
- Misses 1324 1358 +34
+ Partials 309 307 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Requesting changes on #9209 (comment). All these declarations are very tricky 🙂
no-unnecessary-type-assertion conflict with TS for variables used before assignment
PR Checklist
Overview
This PR makes the rule ignore var declared in child scopes.