fix(eslint-plugin): [no-unnecessary-condition] false positives with branded types #7466
Conversation
|
Thanks for the PR, @MBuchalik! 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. |
| // Intersections like `string & {}` can also be possibly falsy, | ||
| // requiring us to look into the intersection. | ||
| .flatMap(type => tsutils.intersectionTypeParts(type)) |
There was a problem hiding this comment.
This works because:
- if it is not an intersection,
intersectionTypeParts(type)just returns the original type, making the function behave like before - if it is something like
string & {},intersectionTypeParts(type)returns an array containing the typesstringand{}.stringin this example is possibly falsy, making our function correctly report that the entire thing is possibly falsy - if we have something like
string & number, the type checker directly reports this type asnever, i.e. here we shouldn't even see that this was ever defined as an intersection in the first place
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7466 +/- ##
==========================================
- Coverage 85.25% 85.24% -0.01%
==========================================
Files 386 386
Lines 13358 13361 +3
Branches 3943 3944 +1
==========================================
+ Hits 11388 11390 +2
- Misses 1593 1594 +1
Partials 377 377
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I don't know why the type checker is failing. Looks like it is not related to this PR. Type checking succeeds locally on my machine. |
It looks like node crashed https://cloud.nx.app/runs/4ett8bkkpx I doubt it is related to this PR. |
| necessaryConditionTest('string & {}'), | ||
| necessaryConditionTest('string & { __brand: string }'), | ||
| necessaryConditionTest('number & {}'), | ||
| necessaryConditionTest('boolean & {}'), |
There was a problem hiding this comment.
[Testing] Normally we'll want both invalid and valid cases added. This just adds valid. Could you add some more coverage?
Additionally, some maybe missing areas:
bigint- ...I don't think symbol brands are a thing, so maybe it's fine to skip them?
{} & { __brand: string }: should this still cause a report?{ a: number } & { b: string }: should this still cause a report?string & string
There was a problem hiding this comment.
Thanks for the feedback! 😃
I have added more tests, especially for more complex scenarios that looked like potential edge cases to me.
While adding tests for invalid cases, I noticed that isPossiblyTruthy also needed to be fixed: If we have a scenario like "" & { __brand: string }, the rule would not report any error. Thus, we need to look at the intersection members and make sure that all of them are not falsy.
(Interesting side note: The rule as currently found on main would report alwaysTruthy.)
Two points regarding your comments:
I don't think symbol brands are a thing, so maybe it's fine to skip them?
I think so too 👍
{} & { __brand: string }and{ a: number } & { b: string }
I believe that both should behave just like if you wrote { __brand: string }, since the first is equal to { __brand: string }, and the second is equal to { a: number, b: string }.
Thus, { a: number } & { b: string } & string should not cause a report. But { a: number } & { b: string } & "foo" should cause an "alwaysTruthy" report.
Btw: I tried to find a scenario where we need to recursively unwrap unions/intersections. Something like foo & (bar | (baz & frub)). But I simply couldn't come up with one where the type checker actually reports it in this particular shape. Or, in other words: I couldn't find a scenario where the current implementation would fail. Thus, I decided not to add any recursive logic for now.
| @@ -28,6 +28,9 @@ const isTruthyLiteral = (type: ts.Type): boolean => | |||
| const isPossiblyFalsy = (type: ts.Type): boolean => | |||
| tsutils | |||
| .unionTypeParts(type) | |||
| // Intersections like `string & {}` can also be possibly falsy, | |||
| // requiring us to look into the intersection. | |||
| .flatMap(type => tsutils.intersectionTypeParts(type)) | |||
There was a problem hiding this comment.
[External] Heh, I wonder if ts-api-utils should export a function like typeParts that essentially calls intersectionTypeParts(unionTypeParts(type))...? Not a blocker for this PR, just ruminating.
JoshuaKGoldberg
added
the
awaiting response
github-actions
bot
removed
the
awaiting response
PR Checklist
Overview
This PR fixes false positives in rule
no-unnecessary-conditioncaused by branded types, as described in #7293.Note: #7293 only talks about branded strings, but I noticed that the problem occurred for any type.
💩