feat(eslint-plugin): [no-unnecessary-type-assertion] improve reporting of literal types (POC) #10631
Conversation
|
Thanks for the PR, @ronami! 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. |
|
View your CI Pipeline Execution ↗ for commit 8d17d09.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10631 +/- ##
==========================================
+ Coverage 86.94% 86.97% +0.02%
==========================================
Files 446 446
Lines 15508 15534 +26
Branches 4518 4529 +11
==========================================
+ Hits 13484 13510 +26
Misses 1669 1669
Partials 355 355
Flags with carried forward coverage won't be shown. Click here to find out more.
|
PR Checklist
Overview
The
no-unnecessary-type-assertionrule has custom logic for handling literal types:typescript-eslint/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts
Lines 227 to 229 in 328b7df
To my understanding, the current logic is as follows: If the cast type is a literal type, it would only be reported if it's the initializer of a
constdeclaration. If it isn't a literal type, it would be reported as long as the assertion isn'tas const(along with the asserted type and expression having the same type).Zooming into the logic of handling literals, I think the rule tries to predict when a literal would be widened. To my understanding (quoting Widening and Narrowing in Typescript), a literal would be widened in the following cases:
To my understanding, the rule somewhat handles (2) as it only reports literals that are assigned to a
constvariable declaration but ignores (1). (2) is still somewhat ignored, as "fresh" literals are relevant here too.In my opinion, this logic may be improved so the rule can catch additional cases that it currently misses (and not fail on some false positives), mostly around "fresh literals".
I'm basing this on the TypeScript Handbook, Widening and Narrowing in Typescript and the literal widening PR, along with testing this on the playground. See this comment #1056 (comment), as I think they're somewhat related.
This isn't something I have a thorough understanding of, but it seems like the case with the following simplified example:
See this playground link for bugs in the current implementation which I think may be improved.
The goal of sending this draft POC PR is to make it easier to discuss or test this on the deploy preview playground. This PR roughly implements what's described above (along with fixing #10257 and #10257 (comment)).
This is a POC proposal to a topic I'm far from being an expert in, so I'd be happy for opinions on this.
Note: Should some of these assumptions be checked/verified by the TypeScript devs? There might be edge cases that I missed that require APIs that aren't currently public.
Please let me know if this should be an issue instead; I wasn't sure if this should be an issue, a draft PR, or an issue accompanied by a draft PR.
Without taking "fresh literals" into consideration, I think differentiating between these two would be difficult: 1, 2.