fix(eslint-plugin): [no-unnecessary-type-assertion] enhance type assertion handling for IIFE #11826
Conversation
…rtion handling for IIFE and add related tests
|
Thanks for the PR, @Tamashoo! 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 2f02909
☁️ 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 #11826 +/- ##
=======================================
Coverage 90.53% 90.54%
=======================================
Files 523 523
Lines 53096 53124 +28
Branches 8838 8844 +6
=======================================
+ Hits 48073 48101 +28
Misses 5010 5010
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
This is looking good, thanks for the PR! I poked at the rule in a bunch of different ways and couldn't figure out amy way to break the new implementation. Nice work 🙂.
Just requesting changes on more thorough testing. This area of TypeScript (inferred types, overloads/signatures) tends to cause bugs in lint rules a lot.
| { | ||
| code: ` | ||
| const UNDEFINED = (() => {})() as undefined; | ||
| `, | ||
| }, |
There was a problem hiding this comment.
[Testing] There's no need to store these values in variables - other than to test that storing in a variable isn't broken. Let's split this into two, simpler tests:
const value = (() => {})() as undefined;(() => {})() as undefined;...and trim out the unnecessary variables from the rest:
const f = () => {};
f() as undefined;| const signatures = functionType.getCallSignatures(); | ||
|
|
||
| if (signatures.length > 0) { | ||
| const returnType = checker.getReturnTypeOfSignature(signatures[0]); |
There was a problem hiding this comment.
[Testing] This code hardcodes the return type to what's in signatures[0], not the specific resolved signature in case of an overload:
interface Overloaded {
(): undefined; // or void?
(value: string): void; // or undefined?
}
(((value) => { }) as Overloaded)("") as undefined;I couldn't find a way to break this rule with overloads (phew!) - but these cases should be added as tests to prevent regressions. Overload signatures are a mess to deal with and tend to break over time.
| // treat it as void (TypeScript infers () => {} as () => undefined, but it should be void) | ||
| if ( | ||
| callee.returnType == null && | ||
| isTypeFlagSet(returnType, ts.TypeFlags.Undefined) |
There was a problem hiding this comment.
[Testing] Another common soruce of bugs is knowing when to check for a type flag being set vs. something specifically and only being a type. Let's add in at least one test for undefined | void just to be safe:
interface Unioned {
(): undefined | void;
}
((() => { }) as Unioned)() as undefined;
PR Checklist
voidreturn type toundefined#11529Overview
Added special handling for type checking of IIFEs in the no-unnecessary-type-assertion rule.
Introduced an isIIFE type guard and a getUncastType helper function to correctly resolve IIFE return types, converting implicit undefined to void for functions without explicit return type annotations.
This improves the accuracy of unnecessary assertion detection for IIFE expressions.