typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star15.8k

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

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Merged

JoshuaKGoldberg merged 2 commits intotypescript-eslint:mainfromMBuchalik:7293-branded-types-false-positives

Aug 19, 2023

Merged

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

JoshuaKGoldberg merged 2 commits intotypescript-eslint:mainfromMBuchalik:7293-branded-types-false-positives

Aug 19, 2023

Conversation

Copy link

Contributor

MBuchalik commentedAug 13, 2023•
edited
Loading

PR Checklist

Addresses an existing open issue:fixesBug: [no-unnecessary-condition] False positives for branded strings #7293
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

This PR fixes false positives in ruleno-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.

💩

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

59ed605

…randed types (typescript-eslint#7293)

Copy link

Contributor

typescript-eslintbot commentedAug 13, 2023

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.

Copy link

netlifybot commentedAug 13, 2023•
edited
Loading

✅ Deploy Preview fortypescript-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 yourNetlify site configuration.

Copy link

nx-cloudbot commentedAug 13, 2023•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitaef1214. 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 💌 fromNxCloud.

MBuchalik commented

Aug 13, 2023

View reviewed changes

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

Comment on lines +31 to +33

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

Copy link

ContributorAuthor

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 likestring & {},intersectionTypeParts(type) returns an array containing the typesstring and{}.string in this example is possibly falsy, making our function correctly report that the entire thing is possibly falsy
if we have something likestring & 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

Copy link

codecovbot commentedAug 13, 2023•
edited
Loading

Codecov Report

Merging#7466 (aef1214) intomain (479f9f6) willdecrease coverage by0.01%.
Report is 15 commits behind head on main.
The diff coverage is100.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%)`	⬆️

... and3 files with indirect coverage changes

Copy link

ContributorAuthor

MBuchalik commentedAug 13, 2023

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.

MBuchalik marked this pull request as ready for review

August 13, 2023 14:22

Copy link

Contributor

rubiesonthesky commentedAug 14, 2023

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 crashedhttps://cloud.nx.app/runs/4ett8bkkpx I doubt it is related to this PR.

JoshuaKGoldberg requested changes

Aug 15, 2023

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Cool, looks like a great start - thanks! 🙌

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

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

		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 bothinvalid andvalid cases added. This just addsvalid. 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

ContributorAuthor

MBuchalikAug 19, 2023•
edited
Loading

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 forinvalid cases, I noticed thatisPossiblyTruthy 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 onmain would reportalwaysTruthy.)

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 likefoo & (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.

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

Copy link

Member

JoshuaKGoldbergAug 15, 2023

Choose a reason for hiding this comment

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

[External] Heh, I wonder ifts-api-utils should export a function liketypeParts that essentially callsintersectionTypeParts(unionTypeParts(type))...? Not a blocker for this PR, just ruminating.

JoshuaKGoldberg/ts-api-utils#258