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

MBuchalik · 2023-08-13T13:43:02Z

PR Checklist

Addresses an existing open issue: fixes Bug: [no-unnecessary-condition] False positives for branded strings #7293
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This PR fixes false positives in rule no-unnecessary-condition caused by branded types, as described in #7293.

Note: #7293 only talks about branded strings, but I noticed that the problem occurred for any type.

Please include your favorite emoji at the bottom of your pull request to hint to us that you did in fact read this file.

💩

…randed types (typescript-eslint#7293)

typescript-eslint · 2023-08-13T13:43:05Z

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.

netlify · 2023-08-13T13:43:06Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`aef1214`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/64e08a80dd6ff80008a5771a
😎 Deploy Preview	https://deploy-preview-7466--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2023-08-13T13:48:01Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit aef1214. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 37 targets

Sent with 💌 from NxCloud.

MBuchalik · 2023-08-13T13:51:06Z

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

+    // Intersections like `string & {}` can also be possibly falsy,
+    // requiring us to look into the intersection.
+    .flatMap(type => tsutils.intersectionTypeParts(type))


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

codecov · 2023-08-13T13:58:49Z

Codecov Report

Merging #7466 (aef1214) into main (479f9f6) will decrease coverage by 0.01%.
Report is 15 commits behind head on main.
The diff coverage is 100.00%.

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

Flag	Coverage Δ
unittest	`85.24% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...slint-plugin/src/rules/no-unnecessary-condition.ts	`98.38% <100.00%> (+0.02%)`	⬆️

... and 3 files with indirect coverage changes

MBuchalik · 2023-08-13T14:22:04Z

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.

rubiesonthesky · 2023-08-14T14:41:46Z

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.

JoshuaKGoldberg

Cool, looks like a great start - thanks! 🙌

Requesting changes on more test coverage. Which I'm guessing might necessitate changes to the rule's logic.

JoshuaKGoldberg · 2023-08-15T19:32:43Z

packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts

+    necessaryConditionTest('string & {}'),
+    necessaryConditionTest('string & { __brand: string }'),
+    necessaryConditionTest('number & {}'),
+    necessaryConditionTest('boolean & {}'),


[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

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.

JoshuaKGoldberg · 2023-08-15T19:33:24Z

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

@@ -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))


[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

JoshuaKGoldberg

Swell, thanks! ✨

fix(eslint-plugin): [no-unnecessary-condition] false positives with b…

59ed605

…randed types (typescript-eslint#7293)

MBuchalik commented Aug 13, 2023

View reviewed changes

MBuchalik marked this pull request as ready for review August 13, 2023 14:22

JoshuaKGoldberg requested changes Aug 15, 2023

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Aug 15, 2023

Improve test coverage and handle branded types in 'isPossiblyTruthy'

aef1214

MBuchalik requested a review from JoshuaKGoldberg August 19, 2023 09:57

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Aug 19, 2023

JoshuaKGoldberg approved these changes Aug 19, 2023

View reviewed changes

JoshuaKGoldberg merged commit b52658f into typescript-eslint:main Aug 19, 2023

MBuchalik deleted the 7293-branded-types-false-positives branch August 19, 2023 15:03

github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MBuchalik commented Aug 13, 2023 •

edited

Loading

typescript-eslint bot commented Aug 13, 2023

netlify bot commented Aug 13, 2023 •

edited

Loading

nx-cloud bot commented Aug 13, 2023 •

edited

Loading

MBuchalik Aug 13, 2023

codecov bot commented Aug 13, 2023 •

edited

Loading

MBuchalik commented Aug 13, 2023

rubiesonthesky commented Aug 14, 2023

JoshuaKGoldberg left a comment

JoshuaKGoldberg Aug 15, 2023

MBuchalik Aug 19, 2023 •

edited

Loading

JoshuaKGoldberg Aug 15, 2023

JoshuaKGoldberg left a comment

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

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

Conversation

MBuchalik commented Aug 13, 2023 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Aug 13, 2023

netlify bot commented Aug 13, 2023 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Aug 13, 2023 • edited Loading

☁️ Nx Cloud Report

MBuchalik Aug 13, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 13, 2023 • edited Loading

Codecov Report

MBuchalik commented Aug 13, 2023

rubiesonthesky commented Aug 14, 2023

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Aug 15, 2023

Choose a reason for hiding this comment

MBuchalik Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

JoshuaKGoldberg Aug 15, 2023

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

MBuchalik commented Aug 13, 2023 •

edited

Loading

netlify bot commented Aug 13, 2023 •

edited

Loading

nx-cloud bot commented Aug 13, 2023 •

edited

Loading

codecov bot commented Aug 13, 2023 •

edited

Loading

MBuchalik Aug 19, 2023 •

edited

Loading