feat(lint): add useNullishCoalescing nursery rule#8952
Conversation
🦋 Changeset detectedLatest commit: f39f564 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a new nursery lint rule Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dyc3
left a comment
There was a problem hiding this comment.
Mostly just some housekeeping things, I don't see any obvious deal breakers with the actual impl.
crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/invalid.ts
Show resolved
Hide resolved
Merging this PR will not alter performance
Comparing Footnotes
|
Will do! @ematipico and @dyc3 thank you very much for the prompt and thorough feedback, appreciate your time on this! |
bf492f8 to
098f864
Compare
|
Alright, I have made the following changes to address feedback:
Hopefully not an issue but I also then amended/squashed the commits, and then rebased the branch onto There's one outstanding issue which is about the implementation of the Thank you team! |
b2d3bf1 to
7716eb5
Compare
|
Hi @ematipico and @dyc3, please let me know if there's anything else I should do here! |
dyc3
left a comment
There was a problem hiding this comment.
Looks good. We've since merged next back into main, so we can put this back to merge into the main branch.
Adds the useNullishCoalescing nursery rule, which suggests using ?? instead of || when the left operand may be nullish. - Uses Typed<JsLogicalExpression> for type-aware analysis - Only offers safe fix when type analysis confirms replacement is safe - Ignores conditional test positions by default (configurable) - Preserves comment trivia in fixes Addresses biomejs#8043
7716eb5 to
addcb34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs (1)
185-214: Minor: two separate.parent()calls could be unified.The parent node is fetched twice (lines 187–189 and 192–195). Not a real performance concern since it's O(1), but extracting the parent once into a local would make the intent slightly clearer.
♻️ Optional tidy-up
fn is_safe_syntax_context_for_replacement(logical: &JsLogicalExpression) -> bool { - let is_parenthesized = logical - .syntax() - .parent() - .is_some_and(|parent| JsParenthesizedExpression::can_cast(parent.kind())); + let parent = logical.syntax().parent(); + let is_parenthesized = parent + .as_ref() + .is_some_and(|p| JsParenthesizedExpression::can_cast(p.kind())); - if !is_parenthesized - && logical - .syntax() - .parent() - .is_some_and(|parent| JsLogicalExpression::can_cast(parent.kind())) - { + if !is_parenthesized + && parent + .as_ref() + .is_some_and(|p| JsLogicalExpression::can_cast(p.kind())) + { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs` around lines 185 - 214, In is_safe_syntax_context_for_replacement, avoid calling logical.syntax().parent() twice: capture the parent once into a local (e.g., let parent = logical.syntax().parent()) and reuse it for both the JsParenthesizedExpression::can_cast and JsLogicalExpression::can_cast checks; keep the rest of the logic (the left/right checks using is_unparenthesized_and_or_expression on logical.left()/right()) unchanged.crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/valid.ts (1)
1-53: Good coverage of the "no diagnostic" scenarios.Covers
??already in use, non-nullish types, conditional test positions, and non-nullish unions. One gap worth considering: a test case foranyandunknowntyped left operands would help document the rule's behaviour for those common types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/valid.ts` around lines 1 - 53, Add test cases exercising left operands typed as any and unknown to document rule behavior: declare variables like maybeAny: any and maybeUnknown: unknown and add expressions using both || and ?? (e.g., const aa = maybeAny || 'fallback'; const ab = maybeAny ?? 'default'; const ba = maybeUnknown || 'fallback'; const bb = maybeUnknown ?? 'default') so the spec covers how the analyzer treats any/unknown for both nullish and logical-or patterns; place them alongside existing cases (e.g., near maybeStr, x, y) and ensure names (maybeAny, maybeUnknown, aa, ab, ba, bb) match so they’re easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/chubby-trees-refuse.md:
- Around line 1-3: Update the changeset in .changeset/chubby-trees-refuse.md so
the top-level change type is "minor" instead of "patch" for the "@biomejs/biome"
entry; locate the front-matter block that lists "@biomejs/biome": patch and
change the value to "@biomejs/biome": minor to match the PR targeting the next
branch and the repository's change-type guidelines.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Around line 185-214: In is_safe_syntax_context_for_replacement, avoid calling
logical.syntax().parent() twice: capture the parent once into a local (e.g., let
parent = logical.syntax().parent()) and reuse it for both the
JsParenthesizedExpression::can_cast and JsLogicalExpression::can_cast checks;
keep the rest of the logic (the left/right checks using
is_unparenthesized_and_or_expression on logical.left()/right()) unchanged.
In `@crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/valid.ts`:
- Around line 1-53: Add test cases exercising left operands typed as any and
unknown to document rule behavior: declare variables like maybeAny: any and
maybeUnknown: unknown and add expressions using both || and ?? (e.g., const aa =
maybeAny || 'fallback'; const ab = maybeAny ?? 'default'; const ba =
maybeUnknown || 'fallback'; const bb = maybeUnknown ?? 'default') so the spec
covers how the analyzer treats any/unknown for both nullish and logical-or
patterns; place them alongside existing cases (e.g., near maybeStr, x, y) and
ensure names (maybeAny, maybeUnknown, aa, ab, ba, bb) match so they’re easy to
locate.
|
OK awesome, thanks! I rebased it back onto main and adjusted the PR base to main. |
|
Hope it's OK, I created a set of follow-on issues according to the plan I described in the original issue/PR, basically chunking out the work to bring this rule progressively into feature parity with the corresponding eslint rule. Can start tackling those in order, lmk if there's anything else! |
|
Hey @pkallos @dyc3 @ematipico — I've implemented issue #9229 (ternary nullish checks) as a follow-on to this PR. PR: #9240 It extends
Also adds Fix safety is handled carefully:
Would appreciate a review when you get a chance! |
AI Disclosure: This PR was developed with AI assistance (Claude).
Summary
Adds the
useNullishCoalescingnursery rule, which suggests using the nullish coalescing operator (??) instead of logical OR (||) when the left operand may be nullish.This is the first in a series of (proposed) incremental PRs to bring
prefer-nullish-coalescingfunctionality to Biome, as discussed this comment in #8089. The approach is to start with the core||->??detection, get it validated in nursery, then follow up with enhancements for||=->??=, ternary expressions, and if-statement patterns.The
??operator only checks fornullandundefined, while||checks for any falsy value including0,'', andfalse. This can prevent bugs where legitimate falsy values are incorrectly treated as missing.Implementation details:
Typed<JsLogicalExpression>query to only trigger when the left operand containsnullorundefinedin its type||in conditional test positions (if/while/for/do-while/ternary) where falsy-checking may be intentional. Configurable viaignoreConditionalTestsoption.Inspired by
@typescript-eslint/prefer-nullish-coalescing. Addresses #8043. Builds on the work in #8089.Test Plan
??)ignoreConditionalTests: falseoptioncargo test -p biome_js_analyze --test spec_tests specs::nursery::use_nullish_coalescingDocs
Documentation is included in the rule's doc comments with examples for invalid and valid cases. The rule options (
ignoreConditionalTests) are documented inline.