feat(eslint-plugin): [no-unnecessary-condition] add checkTypePredicates #10009

kirkwaiblinger · 2024-09-18T02:41:26Z

PR Checklist

Addresses an existing open issue:
- fixes Enhancement: [no-unnecessary-condition] Check truthiness assertion functions #9076,
- fixes Enhancement: [no-unnecessary-condition] report on type guards usages where the type is already equal #10007
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Basically completely rewrite the existing method findAssertedArgument in SBE to use checker.getResolvedSignature() and checker.getTypePredicateOfSignature(), and move it into shared location
Put new behavior behind option "checkTypePredicates" - see this discord conversation for the thought process on naming
Add docs
Enable internally and suppress violations

typescript-eslint · 2024-09-18T02:41:28Z

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 · 2024-09-18T02:41:40Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`42caf70`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/66f204213e781c00082a7779
😎 Deploy Preview	https://deploy-preview-10009--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 98 (🔴 down 1 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

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

nx-cloud · 2024-09-18T02:42:18Z

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

codecov · 2024-09-18T03:19:08Z

Codecov Report

Attention: Patch coverage is 93.44262% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.72%. Comparing base (1c183ab) to head (42caf70).
Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/eslint-plugin/src/util/assertionFunctionUtils.ts	91.11%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10009      +/-   ##
==========================================
+ Coverage   88.70%   88.72%   +0.01%     
==========================================
  Files         426      427       +1     
  Lines       14835    14896      +61     
  Branches     4312     4329      +17     
==========================================
+ Hits        13159    13216      +57     
- Misses       1533     1538       +5     
+ Partials      143      142       -1

Flag	Coverage Δ
unittest	`88.72% <93.44%> (+0.01%)`	⬆️

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

Files with missing lines	Coverage Δ
...slint-plugin/src/rules/no-unnecessary-condition.ts	`99.54% <100.00%> (+0.02%)`	⬆️
...s/eslint-plugin/src/rules/prefer-optional-chain.ts	`100.00% <ø> (ø)`
...plugin/src/rules/prefer-string-starts-ends-with.ts	`98.47% <100.00%> (-0.01%)`	⬇️
...int-plugin/src/rules/strict-boolean-expressions.ts	`100.00% <100.00%> (ø)`
packages/type-utils/src/getTypeName.ts	`100.00% <100.00%> (ø)`
...s/eslint-plugin/src/util/assertionFunctionUtils.ts	`91.11% <91.11%> (ø)`

... and 10 files with indirect coverage changes

JoshuaKGoldberg

Seems very reasonable to me! The comments were quite helpful, thanks for that. 🙌

Requesting changes on added test coverage.

packages/typescript-estree/tests/lib/semanticInfo.test.ts

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

kirkwaiblinger · 2024-09-21T00:24:01Z

packages/eslint-plugin/src/util/assertionFunctionUtils.ts

FYI - I realized while working on this that we can use checker.getTypePredicateOfSignature(signature), which.... does all the heavy lifting for us 🤦‍♂️

So this is now much simpler. 🥳

See 4ac6765 for just the diff of the algorithm change

kirkwaiblinger · 2024-09-21T00:27:26Z

packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts

@@ -516,29 +516,6 @@ declare const nullableString: string | null;
 assert(3 as any, nullableString);
      `,
    },
-    // Intentional TS error - A rest parameter must be last in a parameter list.


These tests were for coverage reasons that no longer make sense with the new algorithm.
None of them crashes. 2 of them now do report, but they match how TS thinks about the situation, so it's not really a false report 🤷‍♂️

kirkwaiblinger · 2024-09-21T16:27:23Z

Per discord conversation, we'll want to bring #10007 into this in order to put it behind the same option. Marking as draft until then

kirkwaiblinger · 2024-09-23T05:22:20Z

This has changed a lot since its initial version. I would recommend reviewing it more or less from scratch

JoshuaKGoldberg

LGTM, very nice refactor!

Just one nit suggestion on test-utils.ts, otherwise 🚀 from me.

JoshuaKGoldberg · 2024-09-23T16:33:32Z

packages/typescript-estree/tests/test-utils/test-utils.ts

@@ -139,6 +140,7 @@ export function omitDeep(
            value.push(visit(el, node));
          }
          node[prop] = value;
+          // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- I don't know if it's safe to change this.


Seems odd to me to leave an "I don't know if ..." in code. I tried removing them locally and some semanticInfo.test.ts snapshots broke. I got around it by:

Changing the type of oNode and the return types of function visit & function omitDeep to UnknownObject | undefined

Removing this if (isObjectLike(child)) { from the else

WDYT?

Hmm, well I'm scared of that since the one usage of omitDeep is asserted to be not null

typescript-eslint/packages/typescript-estree/tests/test-utils/test-utils.ts

Lines 75 to 82 in d6c67fe

/**

* Returns a raw copy of the typescript AST

* @param ast the AST object

* @returns copy of the AST object

*/

export function deeplyCopy<T extends NonNullable<unknown>>(ast: T): T {

return omitDeep(ast) as T;

}

But I'm realizing now that this is "just" test code so I feel a little less scared of messing around with it

An easier fix for now seems like just turning isObjectLike into an ordinary boolean-returning function, since its type predicate isn't actually needed in the two places it's used. 🤷 Plus, no runtime changes, yay!

🤝 perfect!

[no-unnecessary-condition]: add checkTruthinessAssertions

c3e212e

kirkwaiblinger changed the title ~~[no-unnecessary-condition]: add checkTruthinessAssertions~~ feat(eslint-plugin): [no-unnecessary-condition]: add checkTruthinessAssertions Sep 18, 2024

kirkwaiblinger changed the title ~~feat(eslint-plugin): [no-unnecessary-condition]: add checkTruthinessAssertions~~ feat(eslint-plugin): [no-unnecessary-condition] add checkTruthinessAssertions Sep 18, 2024

test -u

337e9b5

kirkwaiblinger marked this pull request as ready for review September 18, 2024 04:23

JoshuaKGoldberg requested changes Sep 18, 2024

View reviewed changes

packages/typescript-estree/tests/lib/semanticInfo.test.ts Outdated Show resolved Hide resolved

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

change internal reports

d72bbd3

kirkwaiblinger mentioned this pull request Sep 18, 2024

Enhancement: [no-unnecessary-condition] report on type guards usages where the type is already equal #10007

Closed

4 tasks

kirkwaiblinger added the enhancement New feature or request label Sep 18, 2024

added some coverage

f86e85e

kirkwaiblinger requested a review from JoshuaKGoldberg September 18, 2024 21:16

hugely simplify

4ac6765

kirkwaiblinger commented Sep 21, 2024

View reviewed changes

kirkwaiblinger added 2 commits September 20, 2024 19:22

uge

150f221

yooj

d6c67fe

kirkwaiblinger added the DO NOT MERGE PRs which should not be merged yet label Sep 21, 2024

kirkwaiblinger marked this pull request as draft September 21, 2024 16:27

kirkwaiblinger added 3 commits September 22, 2024 21:25

changes

e9aa652

some changes

e242683

test changes

b8ce05c

kirkwaiblinger changed the title ~~feat(eslint-plugin): [no-unnecessary-condition] add checkTruthinessAssertions~~ feat(eslint-plugin): [no-unnecessary-condition] add checkTypePredicates Sep 23, 2024

kirkwaiblinger added 3 commits September 22, 2024 22:43

finishing touches

cd2b461

snapshots

12df5fc

fixup

8dbb540

Merge branch 'main' into no-unnecessary-condition-assertion-functions

3dbf4b7

kirkwaiblinger removed the DO NOT MERGE PRs which should not be merged yet label Sep 23, 2024

kirkwaiblinger marked this pull request as ready for review September 23, 2024 05:22

JoshuaKGoldberg previously approved these changes Sep 23, 2024

View reviewed changes

JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Sep 23, 2024

remove type predicate

42caf70

kirkwaiblinger dismissed JoshuaKGoldberg’s stale review via 42caf70 September 24, 2024 00:13

JoshuaKGoldberg merged commit a916ff2 into typescript-eslint:main Sep 29, 2024
62 checks passed

kirkwaiblinger deleted the no-unnecessary-condition-assertion-functions branch September 29, 2024 22:53

kirkwaiblinger mentioned this pull request Sep 30, 2024

Repo: Convert nullThrows to an assertion function #10077

Closed

github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2024

	/**
	* Returns a raw copy of the typescript AST
	* @param ast the AST object
	* @returns copy of the AST object
	*/
	export function deeplyCopy<T extends NonNullable<unknown>>(ast: T): T {
	return omitDeep(ast) as T;
	}

Uh oh!

feat(eslint-plugin): [no-unnecessary-condition] add checkTypePredicates #10009

feat(eslint-plugin): [no-unnecessary-condition] add checkTypePredicates #10009

Uh oh!

Conversation

kirkwaiblinger commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Overview

Uh oh!

typescript-eslint bot commented Sep 18, 2024

Uh oh!

netlify bot commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for typescript-eslint ready!

Uh oh!

nx-cloud bot commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

codecov bot commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kirkwaiblinger Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger commented Sep 21, 2024

Uh oh!

kirkwaiblinger commented Sep 23, 2024

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kirkwaiblinger commented Sep 18, 2024 •

edited

Loading

netlify bot commented Sep 18, 2024 •

edited

Loading

nx-cloud bot commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading

kirkwaiblinger Sep 21, 2024 •

edited

Loading