Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(eslint-plugin): [no-unnecessary-condition] false positives with branded types #7466

Merged
merged 2 commits into from
Aug 19, 2023
Merged

fix(eslint-plugin): [no-unnecessary-condition] false positives with branded types #7466

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59ed605
fix(eslint-plugin): [no-unnecessary-condition] false positives with b…
MBuchalik Aug 13, 2023
aef1214
Improve test coverage and handle branded types in 'isPossiblyTruthy'
MBuchalik Aug 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Comment on lines +31 to +33
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 types string and {}. string in 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 as never, i.e. here we shouldn't even see that this was ever defined as an intersection in the first place

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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/ts-api-utils#258

// PossiblyFalsy flag includes literal values, so exclude ones that
// are definitely truthy
.filter(t => !isTruthyLiteral(t))
Expand Down
5 changes: 5 additions & 0 deletions packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ const result2 = foo() == null;
necessaryConditionTest('null | object'),
necessaryConditionTest('undefined | true'),
necessaryConditionTest('void | true'),
// "branded" type
necessaryConditionTest('string & {}'),
necessaryConditionTest('string & { __brand: string }'),
necessaryConditionTest('number & {}'),
necessaryConditionTest('boolean & {}'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Contributor Author

@MBuchalik MBuchalik Aug 19, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


necessaryConditionTest('any'), // any
necessaryConditionTest('unknown'), // unknown
Expand Down