fix(eslint-plugin): [prefer-nullish-coalescing] doesn't report on ternary but on equivalent || #10517

OlivierZal · 2024-12-18T19:46:03Z

PR Checklist

Addresses an existing open issue: fixes Bug: [prefer-nullish-coalescing] Doesn't report on ternary, but does on equivalent || #10470
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

typescript-eslint · 2024-12-18T19:46:07Z

Thanks for the PR, @OlivierZal!

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-12-18T19:46:25Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`b771c7f`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6790394a28e6e700084afda9
😎 Deploy Preview	https://deploy-preview-10517--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 88 (🔴 down 10 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (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-12-18T19:46:56Z

View your CI Pipeline Execution ↗ for commit b771c7f.

Command	Status	Duration	Result
`nx run-many --target=build --exclude website --...`	✅ Succeeded	5s	View ↗
`nx run-many --target=clean`	✅ Succeeded	11s	View ↗

☁️ Nx Cloud last updated this comment at 2025-01-22 05:56:38 UTC

codecov · 2024-12-28T22:00:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.18%. Comparing base (a6e3fcd) to head (b771c7f).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10517      +/-   ##
==========================================
+ Coverage   87.17%   87.18%   +0.01%     
==========================================
  Files         448      450       +2     
  Lines       15598    15617      +19     
  Branches     4555     4562       +7     
==========================================
+ Hits        13597    13616      +19     
  Misses       1645     1645              
  Partials      356      356

Flag	Coverage Δ
unittest	`87.18% <100.00%> (+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.16% <ø> (-0.08%)`	⬇️
...lint-plugin/src/rules/prefer-nullish-coalescing.ts	`95.97% <100.00%> (+0.37%)`	⬆️
...es/eslint-plugin/src/util/getValueOfLiteralType.ts	`100.00% <100.00%> (ø)`
...slint-plugin/src/util/truthinessAndNullishUtils.ts	`100.00% <100.00%> (ø)`

OlivierZal · 2024-12-29T01:14:46Z

@JoshuaKGoldberg, @kirkwaiblinger, this check seems to be stuck.

However, tests passed so I guess the PR is ready for a first review.

JoshuaKGoldberg

A good start - nice job working with such tricky existing logic!

The tests for valid cases and primitives are very nice and thorough. Requesting changes on more thorough coverage of invalid please. Thanks!

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts

packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts

OlivierZal · 2024-12-31T13:18:22Z

Hi @JoshuaKGoldberg, thanks for the review, I've made at least 2 of the requested changes:

handling nit;
adding original issue tests;
adding more tests here and here.

Regarding the fix/suggestion logic tests following the isFixable modification, the only change is the introduction of an "implicitEquality" case (I made it a constant in 20db420 for more clarity):

the first occurrence is actually the negation of this case in order to keep the pre-existing logic, so it is already covered by the existing tests;
the next condition (handling == / != operators) excludes this "implicitEquality" case (so it is also covered by the existing tests);
the next one (handling any / unknown types) leads to always valid expressions (no fix/suggestion), covered by the tests I've added here;
then the "implicitEquality" case occurs: the fix/suggestion logic for invalid expressions is covered here (x ?? y), tests for valid expressions (no fix/suggestion) have been added here;
the following excludes the "implicitEquality" case since a return already happened here in such a case (so they are also covered by the existing tests);
the fix/suggestion logic related to this addition is tested by the "invalid" tests I've added – !x ? y : x for the "consequent" case and x ? x : y for the "alternate" case (handles the new !operator condition) –, which all fall here (x ?? y).

Do you see any other cases I’ve missed that I could add? Or do you have something else in mind?

JoshuaKGoldberg

I think this is plenty thorough enough for me 😄 but I've also kind of burned out on reviewing prefer-nullish-coalescing. We should get review from another @typescript-eslint/triage-team member.

OlivierZal · 2025-01-03T19:35:28Z

@JoshuaKGoldberg, @kirkwaiblinger, waiting for this PR having a second review, I keep updating the branch each time a new PR is merged. Is it the right thing to do, or can I leave it as is until the review?

OlivierZal · 2025-01-20T21:37:52Z

Ready for review, but I remain open to feedback on #10517 (comment)

kirkwaiblinger

I think the current algorithm and factorization looks good! There's one minor complaint, where I think you've added a workaround which can be avoided - I made a little suggestion PR in order to illustrate this, see OlivierZal#2. Fortunately the test cases you've written validated this well when I was playing around 👍

I think if we make those few changes we'll be close to done iterating! 🙂

suggestion for prefer-nullish-coalescing PR

OlivierZal · 2025-01-21T20:49:20Z

@kirkwaiblinger, I’m delighted I wasn’t too far off the mark 😅. Ultimately, I realized that the inconsistency mentioned in the corresponding issue went beyond the cited example, and since I’m still quite new to the project, I wasn’t entirely sure if taking a few liberties was appropriate.

Thank you so much for your time, help, and guidance.

The last commit only fixes a lint issue in this commit; all the other checks passed successfully.

kirkwaiblinger

Few little nits but this is otherwise basically an approval! Great work on this!! It's delightful work on a real tricky part of the codebase 👍 Thanks for working on this!

kirkwaiblinger · 2025-01-21T23:32:37Z

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts

+    (parent.consequent === node ||
+      parent.alternate === node ||
+      node.type === AST_NODE_TYPES.UnaryExpression)
  ) {


Oh, I'm realizing that these UnaryExpression recursions are unnecessary as well, I think, right?

Suggested change

(parent.consequent === node ||

parent.alternate === node ||

node.type === AST_NODE_TYPES.UnaryExpression)

) {

(parent.consequent === node || parent.alternate === node)

) {

indeed, it was the second part of my "workaround", that your suggestion fixed.

kirkwaiblinger · 2025-01-21T23:33:05Z

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts

+    (parent.consequent === node ||
+      parent.alternate === node ||
+      node.type === AST_NODE_TYPES.UnaryExpression)


Suggested change

(parent.consequent === node ||

parent.alternate === node ||

node.type === AST_NODE_TYPES.UnaryExpression)

(parent.consequent === node || parent.alternate === node)

packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.mdx

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts

Co-authored-by: Kirk Waiblinger <53019676+kirkwaiblinger@users.noreply.github.com>

OlivierZal · 2025-01-22T00:19:37Z

Fixed!

kirkwaiblinger

JoshuaKGoldberg · 2025-01-23T13:37:41Z

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

[Praise] Really nice refactors here. Love to see a -62 line count on a rule 😄

JoshuaKGoldberg

🚀 thanks for pushing through on this!

@OlivierZal

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.21.0 | 8.22.0 | | npm | @typescript-eslint/parser | 8.21.0 | 8.22.0 | ## [v8.22.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8220-2025-01-27) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-template-expression] handle template literal type ([#10612](typescript-eslint/typescript-eslint#10612)) - **eslint-plugin:** \[prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor ([#10552](typescript-eslint/typescript-eslint#10552)) - **eslint-plugin:** \[no-extraneous-class] handle accessor keyword ([#10678](typescript-eslint/typescript-eslint#10678)) - **eslint-plugin:** \[no-shadow] don't report unnecessarily on valid ways of using module augmentation ([#10616](typescript-eslint/typescript-eslint#10616)) - **eslint-plugin:** \[no-duplicate-type-constituents] handle nested types ([#10638](typescript-eslint/typescript-eslint#10638)) - **eslint-plugin:** \[prefer-nullish-coalescing] doesn't report on ternary but on equivalent || ([#10517](typescript-eslint/typescript-eslint#10517)) ##### ❤️ Thank You - mdm317 - Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@OlivierZal

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.21.0 | 8.22.0 | | npm | @typescript-eslint/parser | 8.21.0 | 8.22.0 | ## [v8.22.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8220-2025-01-27) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-template-expression] handle template literal type ([#10612](typescript-eslint/typescript-eslint#10612)) - **eslint-plugin:** \[prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor ([#10552](typescript-eslint/typescript-eslint#10552)) - **eslint-plugin:** \[no-extraneous-class] handle accessor keyword ([#10678](typescript-eslint/typescript-eslint#10678)) - **eslint-plugin:** \[no-shadow] don't report unnecessarily on valid ways of using module augmentation ([#10616](typescript-eslint/typescript-eslint#10616)) - **eslint-plugin:** \[no-duplicate-type-constituents] handle nested types ([#10638](typescript-eslint/typescript-eslint#10638)) - **eslint-plugin:** \[prefer-nullish-coalescing] doesn't report on ternary but on equivalent || ([#10517](typescript-eslint/typescript-eslint#10517)) ##### ❤️ Thank You - mdm317 - Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

OlivierZal changed the title ~~fix(eslint-plugin): [prefer-nullish-coalescing] Doesn't report on ternary, but does on equivalent ||~~ fix(eslint-plugin): [prefer-nullish-coalescing] doesn't report on ternary but on equivalent || Dec 18, 2024

OlivierZal force-pushed the prefer-nullish-coalescing-10470 branch 10 times, most recently from db11672 to 586aa99 Compare December 20, 2024 21:43

OlivierZal mentioned this pull request Dec 21, 2024

Bug: [prefer-nullish-coalescing] doesn't report on chain expressions in a ternary #10531

Closed

4 tasks

OlivierZal force-pushed the prefer-nullish-coalescing-10470 branch 2 times, most recently from 0360ad0 to 3f23cb4 Compare December 28, 2024 21:44

OlivierZal marked this pull request as ready for review December 28, 2024 22:03

OlivierZal force-pushed the prefer-nullish-coalescing-10470 branch from 3f23cb4 to ea899f8 Compare December 28, 2024 23:30

JoshuaKGoldberg requested changes Dec 30, 2024

View reviewed changes

packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts Outdated Show resolved Hide resolved

packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts Show resolved Hide resolved

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Dec 30, 2024

JoshuaKGoldberg mentioned this pull request Dec 30, 2024

Docs: [prefer-nullish-coalescing] streamline code examples #10566

Closed

2 tasks

OlivierZal force-pushed the prefer-nullish-coalescing-10470 branch from dfa9df1 to 74a4059 Compare December 30, 2024 22:07

OlivierZal requested a review from JoshuaKGoldberg December 31, 2024 13:10

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Dec 31, 2024

JoshuaKGoldberg previously approved these changes Dec 31, 2024

View reviewed changes

OlivierZal added 4 commits January 20, 2025 22:49

Merge branch 'main' into prefer-nullish-coalescing-10470

28eeac8

align options for as much use cases as possible

2cfb35c

add tests for ternary + bool coercion

993d6b1

fixes and add test ternary + conditional

4822bb4

kirkwaiblinger mentioned this pull request Jan 21, 2025

suggestion for prefer-nullish-coalescing PR OlivierZal/typescript-eslint#2

Merged

kirkwaiblinger requested changes Jan 21, 2025

View reviewed changes

kirkwaiblinger and others added 4 commits January 21, 2025 12:12

suggestion

2defdf4

Merge pull request #2 from kirkwaiblinger/pnc-suggestion

dce2427

suggestion for prefer-nullish-coalescing PR

Merge branch 'main' into prefer-nullish-coalescing-10470

5bd6b4e

fix lint

80f3a20

OlivierZal requested a review from kirkwaiblinger January 21, 2025 20:49

kirkwaiblinger requested changes Jan 21, 2025

View reviewed changes

kirkwaiblinger added 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge and removed 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labels Jan 21, 2025

OlivierZal and others added 2 commits January 22, 2025 01:03

Update packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts

469bd65

Co-authored-by: Kirk Waiblinger <53019676+kirkwaiblinger@users.noreply.github.com>

nits

b771c7f

OlivierZal requested a review from kirkwaiblinger January 22, 2025 00:18

kirkwaiblinger approved these changes Jan 22, 2025

View reviewed changes

kirkwaiblinger added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jan 22, 2025

JoshuaKGoldberg reviewed Jan 23, 2025

View reviewed changes

JoshuaKGoldberg approved these changes Jan 23, 2025

View reviewed changes

JoshuaKGoldberg merged commit 1e2305e into typescript-eslint:main Jan 23, 2025
65 checks passed

FlorinAsavoaie mentioned this pull request Jan 28, 2025

Bug: [prefer-nullish-coalescing] Should not report error #10724

Closed

4 tasks

OlivierZal deleted the prefer-nullish-coalescing-10470 branch January 28, 2025 14:40

github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [prefer-nullish-coalescing] doesn't report on ternary but on equivalent || #10517

fix(eslint-plugin): [prefer-nullish-coalescing] doesn't report on ternary but on equivalent || #10517

OlivierZal commented Dec 18, 2024

typescript-eslint bot commented Dec 18, 2024

netlify bot commented Dec 18, 2024 •

edited

Loading

nx-cloud bot commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 28, 2024 •

edited

Loading

OlivierZal commented Dec 29, 2024 •

edited

Loading

JoshuaKGoldberg left a comment

OlivierZal commented Dec 31, 2024 •

edited

Loading

JoshuaKGoldberg left a comment

OlivierZal commented Jan 3, 2025

OlivierZal commented Jan 20, 2025 •

edited

Loading

kirkwaiblinger left a comment

OlivierZal commented Jan 21, 2025

kirkwaiblinger left a comment

kirkwaiblinger Jan 21, 2025

OlivierZal Jan 22, 2025

kirkwaiblinger Jan 21, 2025

OlivierZal commented Jan 22, 2025

kirkwaiblinger left a comment

JoshuaKGoldberg Jan 23, 2025

JoshuaKGoldberg left a comment

fix(eslint-plugin): [prefer-nullish-coalescing] doesn't report on ternary but on equivalent || #10517

fix(eslint-plugin): [prefer-nullish-coalescing] doesn't report on ternary but on equivalent || #10517

Conversation

OlivierZal commented Dec 18, 2024

PR Checklist

Overview

typescript-eslint bot commented Dec 18, 2024

netlify bot commented Dec 18, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Dec 18, 2024 • edited Loading

codecov bot commented Dec 28, 2024 • edited Loading

Codecov Report

OlivierZal commented Dec 29, 2024 • edited Loading

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

OlivierZal commented Dec 31, 2024 • edited Loading

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

OlivierZal commented Jan 3, 2025

OlivierZal commented Jan 20, 2025 • edited Loading

kirkwaiblinger left a comment

Choose a reason for hiding this comment

OlivierZal commented Jan 21, 2025

kirkwaiblinger left a comment

Choose a reason for hiding this comment

kirkwaiblinger Jan 21, 2025

Choose a reason for hiding this comment

OlivierZal Jan 22, 2025

Choose a reason for hiding this comment

kirkwaiblinger Jan 21, 2025

Choose a reason for hiding this comment

OlivierZal commented Jan 22, 2025

kirkwaiblinger left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 23, 2025

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

netlify bot commented Dec 18, 2024 •

edited

Loading

nx-cloud bot commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 28, 2024 •

edited

Loading

OlivierZal commented Dec 29, 2024 •

edited

Loading

OlivierZal commented Dec 31, 2024 •

edited

Loading

OlivierZal commented Jan 20, 2025 •

edited

Loading