fix(eslint-plugin): fix crash and false positives in no-useless-default-assignment
#11845
Conversation
|
Thanks for the PR, @ulrichstark! 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 project configuration. |
|
View your CI Pipeline Execution ↗ for commit 84419df
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11845 +/- ##
=======================================
Coverage 90.58% 90.58%
=======================================
Files 524 524
Lines 53324 53409 +85
Branches 8892 8917 +25
=======================================
+ Hits 48301 48383 +82
- Misses 5010 5013 +3
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
no-useless-default-assignment if no signaturesno-useless-default-assignment
|
I pulled this fork, built, and |
kirkwaiblinger
left a comment
There was a problem hiding this comment.
I think this generally looks good, at least for the fixes for #11846 and #11849...
I'll note, though, that it seems like 3 independent, non-overlapping fixes. Usually we'd prefer to have one PR per issue for things that aren't highly coupled, since it simplifies our work as reviewers and also makes it easier to revert if there's a big problem with one of the fixes (not likely in this case). In any case, one of the issues is not accepting PRs yet, so we can't proceed with this PR in its current form quite yet.
Would you be willing to break this apart into the independent fixes? 🙏 That would expedite being able to get the fixes for #11846 and #11849 completed.
packages/eslint-plugin/tests/rules/no-useless-default-assignment.test.ts
Show resolved
Hide resolved
|
Thanks @kirkwaiblinger for the review! I removed the changes for #11847 and brought back the noUncheckedIndexedAccess tests. This PR now only closes #11846 and #11849. |
| `, | ||
| ` | ||
| declare const tuple: [string]; | ||
| const [a, b = 'default'] = tuple; |
There was a problem hiding this comment.
(praise) TIL that TS allows out-of-bounds tuple destructuring if a default value is present.
Comically it means that
const [a, outOfBounds = undefined] = ["foo"] as const;is kind of a false positive if you really squint, but I think let's completely ignore that edge case as niche and unimportant.
PR Checklist
Overview
The rule now treats types as if
isNoUncheckedIndexedAccessis enabled to not report false positives and suggest unsafe fixes. This required me to move some test cases from the "invalid" section to the "valid" section. Also added reproduction test cases for every issue this PR fixes.Fixes #11846 (3 false positives)
Fixes #11849 (1 crash)