feat(lint): extend useNullishCoalescing to detect ternary nullish checks#9240
feat(lint): extend useNullishCoalescing to detect ternary nullish checks#9240hammadxcm wants to merge 3 commits intobiomejs:mainfrom
Conversation
Extend the useNullishCoalescing nursery rule to detect ternary expressions that perform explicit nullish checks and suggest rewriting them with the ?? operator. Detected patterns: - a !== null ? a : b -> a ?? b - a !== undefined ? a : b -> a ?? b - a != null ? a : b -> a ?? b - a === null ? b : a -> a ?? b (inverted) - a === null || a === undefined ? b : a -> a ?? b (compound) - a !== null && a !== undefined ? a : b -> a ?? b (compound) Also handles null/undefined on either side of the comparison, member expressions, computed members, and parenthesized subjects. Adds the ignoreTernaryTests option (default: false) to allow skipping ternary expressions entirely when set to true. Closes biomejs#9229
🦋 Changeset detectedLatest commit: b7c7c5f 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 |
github-actions
bot
added
A-Linter
WalkthroughExtends the nursery rule useNullishCoalescing to also detect ternary expressions that perform explicit nullish checks and suggest rewriting them with Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs (1)
454-462:undefinedshadowing is not checked.
is_undefined_identifierdoes a raw text comparison against"undefined". In non-strict-mode JS,undefinedcan be shadowed. This is a known limitation shared with ESLint's equivalent rule, so likely acceptable — but worth a brief note in the function's doc comment.🤖 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 454 - 462, Add a short doc comment above the is_undefined_identifier function explaining that it performs a raw text comparison on the identifier token and does not perform symbol resolution, so it will not detect cases where the global undefined has been shadowed (e.g., in non-strict-mode code or when a local variable named "undefined" exists); reference the function name is_undefined_identifier in the comment so future readers know this limitation applies to that helper..changeset/extend-nullish-coalescing-ternary.md (1)
18-18: Tiny grammar nit: "independently from" → "independently of" is more standard English.-Added a new `ignoreTernaryTests` option (default: `false`) to allow disabling this check independently from the existing logical OR detection. +Added a new `ignoreTernaryTests` option (default: `false`) to allow disabling this check independently of the existing logical OR detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/extend-nullish-coalescing-ternary.md at line 18, Tiny grammar nit: change "independently from" to "independently of" in the changeset text; update the sentence that describes the new option ignoreTernaryTests (default: false) so it reads "independently of the existing logical OR detection" instead of "independently from".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Around line 53-59: The rule currently treats strict single-null/undefined
ternaries as safe to auto-fix to `??`, but that's semantically unsafe; update
the Ternary handling in use_nullish_coalescing.rs so fixes are only emitted when
the ternary is actually loose or compound: either change the emitted fix kind
for Ternary from FixKind::Safe to FixKind::Unsafe, or add an
`is_loose_or_compound` flag on the Ternary variant and make the `action` return
None unless that flag is true (mirroring the `can_fix` guard used by the
LogicalOr path); adjust any related logic that sets the Ternary variant so it
detects `!=`/`==` or both null/undefined checks (`a !== null && a !==
undefined`) before allowing a safe fix.
- Around line 464-470: expressions_are_equivalent currently uses text comparison
and will treat call expressions like foo() as equivalent even though replacing a
ternary with the nullish-coalescing operator changes call count; add a check for
call expressions inside the subject expression (e.g. traverse the subject
AnyJsExpression and detect AnyJsCallExpression or call-like nodes) and use that
in extract_ternary_nullish_check (or in the action arm where the fix is
produced) to either skip emitting the automatic fix or mark the fix as unsafe
when a call expression is present; update the logic that produces the fix to
only apply the transformation when the subject contains no call expressions.
In
`@crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.js`:
- Around line 64-65: The test currently uses foo() twice in the ternary for v19
which changes semantics if foo() has side effects; update the test so foo() is
only invoked once (e.g. assign its result to a temporary/subject variable and
use that in the conditional and result) or move this case into a separate
category/excluded list for side-effect-unsafe subjects so it does not assert a
transformation that calls foo() only once; key symbols: v19 and foo().
---
Nitpick comments:
In @.changeset/extend-nullish-coalescing-ternary.md:
- Line 18: Tiny grammar nit: change "independently from" to "independently of"
in the changeset text; update the sentence that describes the new option
ignoreTernaryTests (default: false) so it reads "independently of the existing
logical OR detection" instead of "independently from".
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Around line 454-462: Add a short doc comment above the is_undefined_identifier
function explaining that it performs a raw text comparison on the identifier
token and does not perform symbol resolution, so it will not detect cases where
the global undefined has been shadowed (e.g., in non-strict-mode code or when a
local variable named "undefined" exists); reference the function name
is_undefined_identifier in the comment so future readers know this limitation
applies to that helper.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryValid.js.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (7)
.changeset/extend-nullish-coalescing-ternary.mdcrates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.jscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.jscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryValid.jscrates/biome_rule_options/src/use_nullish_coalescing.rs
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs
Outdated
Show resolved
Hide resolved
| // === In variable declaration === | ||
| const v19 = foo() !== undefined ? foo() : 'default'; |
There was a problem hiding this comment.
foo() subject — see earlier comment about side-effect safety.
This test case will produce a safe fix that changes foo() from being called twice to once. Depending on how the main issue is resolved, this case may need to move to a separate category or be excluded.
🤖 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/ternaryInvalid.js`
around lines 64 - 65, The test currently uses foo() twice in the ternary for v19
which changes semantics if foo() has side effects; update the test so foo() is
only invoked once (e.g. assign its result to a temporary/subject variable and
use that in the conditional and result) or move this case into a separate
category/excluded list for side-effect-unsafe subjects so it does not assert a
transformation that calls foo() only once; key symbols: v19 and foo().
Distinguish fix applicability for ternary nullish checks: - Loose equality (!=, ==) and compound strict checks: safe fix (covers both null and undefined, matching ?? semantics). - Strict single checks (===, !==): unsafe fix (only checks one of null/undefined, ?? checks both). - Call expression subjects: no fix offered (ternary evaluates subject twice, ?? evaluates once).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs (1)
90-90:⚠️ Potential issue | 🟠 Major
fix_kind: FixKind::Safeconflicts with theMaybeIncorrectfixes emitted for strict ternary checks.The rule advertises
FixKind::Safe, but the Ternary action arm explicitly emitsApplicability::MaybeIncorrectfor strict single checks (===/!==). The rule-level declaration should beFixKind::Unsafeto truthfully describe the broadest fix severity the rule can produce.🛠️ Proposed fix
- fix_kind: FixKind::Safe, + fix_kind: FixKind::Unsafe,🤖 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` at line 90, The rule currently sets fix_kind: FixKind::Safe but the Ternary action emits fixes with Applicability::MaybeIncorrect for strict ternary checks (===/!==), so update the rule-level declaration to use fix_kind: FixKind::Unsafe to reflect the broadest severity; locate the fix_kind assignment and replace FixKind::Safe with FixKind::Unsafe and keep the existing Ternary action that emits Applicability::MaybeIncorrect unchanged.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs (1)
200-200:extract_ternary_nullish_checkis called twice — cache results in state.
run_ternary(line 285) andaction(line 200) both callextract_ternary_nullish_check, traversing the same AST subtree twice. Thesubjectandfallbackexpressions could be stored directly inUseNullishCoalescingState::Ternarysoactiononly reads them without redoing the work.♻️ Sketch of the refactor
Ternary { range: TextRange, can_fix: bool, is_safe_fix: bool, + subject: AnyJsExpression, + fallback: AnyJsExpression, },Then in
run_ternary:+ let subject = info.subject.clone(); + let fallback = info.fallback.clone(); Some(UseNullishCoalescingState::Ternary { range: conditional.syntax().text_trimmed_range(), can_fix, is_safe_fix, + subject, + fallback, })And in
action, destructuresubject/fallbackfrom state instead of callingextract_ternary_nullish_checkagain.🤖 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` at line 200, The code calls extract_ternary_nullish_check twice (from run_ternary and action); change the state to cache its results by adding subject and fallback into UseNullishCoalescingState::Ternary, have run_ternary call extract_ternary_nullish_check once and populate those fields on the state variant, and update action to destructure/read subject and fallback from UseNullishCoalescingState::Ternary instead of calling extract_ternary_nullish_check again so the AST subtree is only traversed once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Around line 100-112: Move the helper enum UseNullishCoalescingState so it is
declared after the impl Rule for UseNullishCoalescing block and make it
non-public (remove the pub), since helper types must live below the impl and
State is an internal detail; locate the current UseNullishCoalescingState
declaration and cut it, then paste it immediately after the closing brace of
impl Rule for UseNullishCoalescing and drop the pub keyword (keep the same
variants and fields).
---
Outside diff comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Line 90: The rule currently sets fix_kind: FixKind::Safe but the Ternary
action emits fixes with Applicability::MaybeIncorrect for strict ternary checks
(===/!==), so update the rule-level declaration to use fix_kind: FixKind::Unsafe
to reflect the broadest severity; locate the fix_kind assignment and replace
FixKind::Safe with FixKind::Unsafe and keep the existing Ternary action that
emits Applicability::MaybeIncorrect unchanged.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Line 200: The code calls extract_ternary_nullish_check twice (from run_ternary
and action); change the state to cache its results by adding subject and
fallback into UseNullishCoalescingState::Ternary, have run_ternary call
extract_ternary_nullish_check once and populate those fields on the state
variant, and update action to destructure/read subject and fallback from
UseNullishCoalescingState::Ternary instead of calling
extract_ternary_nullish_check again so the AST subtree is only traversed once.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (1)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs
Outdated
Show resolved
Hide resolved
| /// Enforce using nullish coalescing operator (`??`) instead of logical or (`||`) | ||
| /// or ternary expressions that perform explicit nullish checks. |
There was a problem hiding this comment.
This will break docs. Please fix it as per contribution guidelines
There was a problem hiding this comment.
Fixed — all doc examples now use expect_diagnostic and file= for invalid cases, and file= for valid cases, following the contribution guidelines for RuleDomain::Types rules.
There was a problem hiding this comment.
No, that's not what a meant. The first line can't contain newlines, it will break the codegen
| match state { | ||
| UseNullishCoalescingState::LogicalOr { operator_range, .. } => Some( | ||
| RuleDiagnostic::new( | ||
| rule_category!(), | ||
| *operator_range, | ||
| markup! { | ||
| "Use "<Emphasis>"??"</Emphasis>" instead of "<Emphasis>"||"</Emphasis>"." | ||
| }, | ||
| ) | ||
| .note(markup! { | ||
| "The "<Emphasis>"||"</Emphasis>" operator checks for all falsy values (including 0, '', and false), while "<Emphasis>"??"</Emphasis>" only checks for null and undefined." | ||
| }), | ||
| ), | ||
| UseNullishCoalescingState::Ternary { range, .. } => Some( | ||
| RuleDiagnostic::new( | ||
| rule_category!(), | ||
| *range, | ||
| markup! { | ||
| "Prefer "<Emphasis>"??"</Emphasis>" instead of a ternary expression with an explicit nullish check." | ||
| }, | ||
| ) | ||
| .note(markup! { | ||
| "This ternary can be simplified to the nullish coalescing operator." | ||
| }), | ||
| ), |
There was a problem hiding this comment.
I don't have much context about how the previous implementation worked, but here's the issue now: the third pillar of the rule is shown twice if we can fix the code. We should offer a solution once
There was a problem hiding this comment.
Fixed — removed the redundant .note() from the Ternary diagnostic. The fix action already communicates the solution, so the diagnostic now only shows the main message.
| } | ||
|
|
||
| Some(TernaryNullishInfo { | ||
| subject: non_nullish_branch.clone(), | ||
| fallback: fallback_branch, | ||
| covers_both_nullish, | ||
| }) | ||
| } | ||
|
|
||
| struct SimpleNullishCheck { | ||
| /// The non-nullish side of the comparison. | ||
| subject: AnyJsExpression, | ||
| /// Whether this is an equality check (`===` / `==`). | ||
| is_equality: bool, |
There was a problem hiding this comment.
The documentation of the function doesn't mention how the check is determined. Here you check right and then left, but you don't explain why, which is critical to the understanding of the business logic
There was a problem hiding this comment.
Fixed — added a comprehensive doc comment explaining that the right operand is checked first because the common convention is expr === null (subject on the left, literal on the right), and we fall back to checking the left operand for the reversed form null === expr.
| let (is_equality, is_loose) = match operator { | ||
| JsBinaryOperator::StrictEquality => (true, false), | ||
| JsBinaryOperator::StrictInequality => (false, false), | ||
| JsBinaryOperator::Equality => (true, true), |
There was a problem hiding this comment.
"Compound" doesn't have any meaning unless it's contextualised. What's a "compound nullish check"? Please provide docs that simplify things
There was a problem hiding this comment.
Fixed — added a doc comment that explains a "compound nullish check" combines two strict comparisons against null and undefined with a logical operator. Two forms are recognized:
- Equality form:
a === null || a === undefined(logical OR) - Inequality form:
a !== null && a !== undefined(logical AND)
Both sides must test the same subject, use strict equality/inequality (not loose), and check different nullish values.
| return Some(NullishKind::Null); | ||
| } | ||
| if is_undefined_identifier(expr) { | ||
| return Some(NullishKind::Undefined); | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
This is plain wrong. An expression can fold on multiple lines because there are multiple tokens, and text_trimmed() only removes trivia at the edges, the ones in the middle are kept.
Remove this check, it's full of false positives
There was a problem hiding this comment.
Fixed — replaced the text_trimmed() node-level comparison with a proper token-by-token comparison using descendants_with_tokens(). The new implementation iterates all tokens in both expressions and compares each token's kind() and text_trimmed() individually, which correctly ignores trivia between tokens (e.g. a . b and a.b now compare as equivalent).
| // both null and undefined (compound or loose equality). | ||
| let (check_subject, is_equality, covers_both_nullish) = | ||
| if let AnyJsExpression::JsLogicalExpression(logical) = &test { | ||
| // Compound: `a === null || a === undefined` or `a !== null && a !== undefined` |
There was a problem hiding this comment.
Please document. Especially what defines "simple null check"
There was a problem hiding this comment.
Fixed — added a doc comment explaining that a "simple nullish check" is a single binary expression that compares an expression against null or undefined using ===, !==, ==, or !=. Examples: a !== null, undefined === x, null != value.
| } | ||
|
|
||
| struct TernaryNullishInfo { | ||
| /// The expression to use as the left side of `??` (e.g. `a` in `a ?? b`). |
There was a problem hiding this comment.
Fixed — added a doc comment to run_ternary clarifying that this detects ternary expressions (condition ? consequent : alternate) where the condition is a nullish check. Also renamed the local variable from test to condition inside extract_ternary_nullish_check for clarity.
…hecks - Fix doc examples to use expect_diagnostic and file= per contribution guidelines - Remove redundant diagnostic note from ternary variant - Replace text_trimmed() expression comparison with token-by-token comparison - Improve documentation for all ternary helper functions - Change fix_kind to Unsafe to match MaybeIncorrect applicability - Cache subject/fallback in state to avoid double AST traversal - Move UseNullishCoalescingState below impl Rule block - Fix changeset grammar
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs (1)
226-226: 🛠️ Refactor suggestion | 🟠 Major
UseNullishCoalescingStateshould not bepub.The placement below
impl Ruleis now correct, but thepubvisibility remains. State is an internal implementation detail and should be private.♻️ Proposed fix
-pub enum UseNullishCoalescingState { +enum UseNullishCoalescingState {Based on learnings: "all helper functions, structs, and enums must be placed below the
impl Ruleblock."🤖 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` at line 226, The enum UseNullishCoalescingState is declared public but is an internal helper and should be private; change its visibility by removing the pub modifier from UseNullishCoalescingState and ensure the enum remains placed below the impl Rule block as an internal helper so only the module can access it (locate the enum declaration named UseNullishCoalescingState and drop the pub keyword).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Around line 566-573: contains_call_expression currently only looks for
JsSyntaxKind::JS_CALL_EXPRESSION and therefore misses `new` expressions which
are also double-evaluated by the ternary; update contains_call_expression to
return true for both call and new expressions (e.g. check node.kind() ==
JsSyntaxKind::JS_CALL_EXPRESSION || node.kind() ==
JsSyntaxKind::JS_NEW_EXPRESSION) so that `new Foo()` is treated the same as
`foo()` when deciding can_fix.
- Line 89: The metadata declares fix_kind: FixKind::Unsafe but the Ternary
action returns Applicability::Always when is_safe_fix is true, so make them
consistent: either change the rule metadata to fix_kind: FixKind::Safe if every
ternary rewrite performed by the Ternary action is truly safe, or keep
FixKind::Unsafe and modify the Ternary action to return
Applicability::MaybeIncorrect (instead of Applicability::Always) when
is_safe_fix is true; update the return logic in the Ternary action so it uses
Applicability::MaybeIncorrect whenever the rule metadata remains
FixKind::Unsafe.
---
Duplicate comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Line 226: The enum UseNullishCoalescingState is declared public but is an
internal helper and should be private; change its visibility by removing the pub
modifier from UseNullishCoalescingState and ensure the enum remains placed below
the impl Rule block as an internal helper so only the module can access it
(locate the enum declaration named UseNullishCoalescingState and drop the pub
keyword).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/commentTrivia.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreConditionalTestsDisabled.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (2)
.changeset/extend-nullish-coalescing-ternary.mdcrates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/extend-nullish-coalescing-ternary.md
| recommended: false, | ||
| severity: Severity::Information, | ||
| fix_kind: FixKind::Safe, | ||
| fix_kind: FixKind::Unsafe, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs | head -100Repository: biomejs/biome
Length of output: 4548
🏁 Script executed:
cat -n crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs | sed -n '85,95p'Repository: biomejs/biome
Length of output: 457
🏁 Script executed:
cat -n crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs | sed -n '200,220p'Repository: biomejs/biome
Length of output: 1086
🏁 Script executed:
# Search for action function and Applicability returns in this rule
rg -A 5 "fn action" crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rsRepository: biomejs/biome
Length of output: 361
🏁 Script executed:
# Look for other rules that use FixKind::Unsafe with Applicability
rg -B 5 "FixKind::Unsafe" crates/biome_js_analyze/src/lint/ | head -50Repository: biomejs/biome
Length of output: 4585
🏁 Script executed:
# Search for rules that return Applicability::Always with FixKind::Unsafe
rg "Applicability::Always" crates/biome_js_analyze/src/lint/nursery/ | head -20Repository: biomejs/biome
Length of output: 165
🏁 Script executed:
# Look at the LogicalOr action path to see what applicability it returns
rg -A 20 "UseNullishCoalescingState::LogicalOr" crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rsRepository: biomejs/biome
Length of output: 2784
🏁 Script executed:
# Check the metadata applicability function - what does it return?
rg -B 5 -A 10 "fn applicability" crates/biome_analyze/src/Repository: biomejs/biome
Length of output: 2041
🏁 Script executed:
# Look at how other rules handle multiple applicability levels with a single fix_kind
rg -B 5 "Applicability::Always" crates/biome_js_analyze/src/lint/ | grep -A 10 "FixKind"Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Check if there's documentation about fix_kind vs Applicability relationship
find crates/biome_analyze -name "*.md" -exec grep -l "fix_kind\|Applicability" {} \;Repository: biomejs/biome
Length of output: 93
🏁 Script executed:
# Check the FixKind to Applicability conversion
rg -B 3 -A 10 "impl.*From.*FixKind\|impl.*Into.*FixKind" crates/biome_analyze/src/Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Check the biome_diagnostics crate for FixKind definition
find . -path "*/biome_diagnostics/*" -name "*.rs" -exec rg -l "FixKind" {} \;Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look for enum FixKind definition
rg -B 2 -A 10 "enum FixKind" crates/Repository: biomejs/biome
Length of output: 993
🏁 Script executed:
# Find the TryInto/From implementation for FixKind to Applicability
rg -B 5 -A 15 "impl.*TryInto.*Applicability\|impl.*From.*FixKind.*Applicability" crates/Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Search more broadly for FixKind conversion
rg -B 3 -A 8 "try_into\(\)" crates/biome_analyze/src/rule.rs | head -40Repository: biomejs/biome
Length of output: 488
🏁 Script executed:
# Check biome_diagnostics Applicability enum
rg -B 2 -A 15 "^pub enum Applicability" crates/Repository: biomejs/biome
Length of output: 1621
🏁 Script executed:
# Find the From/TryInto implementation for FixKind
rg "FixKind" crates/biome_diagnostics/src/suggestion.rsRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look for impl blocks in rule.rs that handle FixKind conversion
rg -B 5 -A 10 "impl.*FixKind" crates/biome_analyze/src/rule.rsRepository: biomejs/biome
Length of output: 928
🏁 Script executed:
# Search for From/Into implementations with FixKind
rg "From<FixKind>\|impl.*TryFrom" crates/Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Check if FixKind is re-exported from biome_diagnostics
rg "pub.*FixKind\|use.*FixKind" crates/biome_analyze/src/lib.rsRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Check the Ternary action path more carefully - what determines is_safe_fix?
rg -B 30 "let applicability = if \*is_safe_fix" crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rsRepository: biomejs/biome
Length of output: 1518
🏁 Script executed:
# Look for similar patterns where action() returns different Applicability than fix_kind in other rules
rg -B 5 "Applicability::(Always|MaybeIncorrect)" crates/biome_js_analyze/src/lint/ | grep -A 3 "FixKind::" | head -40Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Check contributing guidelines for how fix_kind and Applicability should relate
cat crates/biome_analyze/CONTRIBUTING.mdRepository: biomejs/biome
Length of output: 50371
Ternary action returns Applicability::Always despite fix_kind: FixKind::Unsafe metadata.
Line 89 declares fix_kind: FixKind::Unsafe, which maps to Applicability::MaybeIncorrect by default. However, the Ternary action (lines 208–212) returns Applicability::Always when is_safe_fix is true. This breaks the contract where metadata describes the rule's safety level. Either declare fix_kind: FixKind::Safe (if all ternary rewrites are safe), or keep Unsafe and return MaybeIncorrect consistently for all actions.
🤖 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` at line
89, The metadata declares fix_kind: FixKind::Unsafe but the Ternary action
returns Applicability::Always when is_safe_fix is true, so make them consistent:
either change the rule metadata to fix_kind: FixKind::Safe if every ternary
rewrite performed by the Ternary action is truly safe, or keep FixKind::Unsafe
and modify the Ternary action to return Applicability::MaybeIncorrect (instead
of Applicability::Always) when is_safe_fix is true; update the return logic in
the Ternary action so it uses Applicability::MaybeIncorrect whenever the rule
metadata remains FixKind::Unsafe.
| /// Check whether an expression contains a call expression anywhere in its subtree. | ||
| /// When the subject contains calls, the ternary evaluates them twice (test + branch) | ||
| /// while `??` would only evaluate once, changing side-effect semantics. | ||
| fn contains_call_expression(expr: &AnyJsExpression) -> bool { | ||
| expr.syntax() | ||
| .descendants() | ||
| .any(|node| node.kind() == JsSyntaxKind::JS_CALL_EXPRESSION) | ||
| } |
There was a problem hiding this comment.
contains_call_expression misses new expressions.
new Foo() has the same double-evaluation problem as foo(). If the subject is a new expression — e.g. new Foo() !== null ? new Foo() : b — the constructor is called twice in the ternary but only once after ?? rewriting, yet can_fix is set to true.
🐛 Proposed fix
fn contains_call_expression(expr: &AnyJsExpression) -> bool {
expr.syntax()
.descendants()
- .any(|node| node.kind() == JsSyntaxKind::JS_CALL_EXPRESSION)
+ .any(|node| {
+ matches!(
+ node.kind(),
+ JsSyntaxKind::JS_CALL_EXPRESSION | JsSyntaxKind::JS_NEW_EXPRESSION
+ )
+ })
}🤖 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 566 - 573, contains_call_expression currently only looks for
JsSyntaxKind::JS_CALL_EXPRESSION and therefore misses `new` expressions which
are also double-evaluated by the ternary; update contains_call_expression to
return true for both call and new expressions (e.g. check node.kind() ==
JsSyntaxKind::JS_CALL_EXPRESSION || node.kind() ==
JsSyntaxKind::JS_NEW_EXPRESSION) so that `new Foo()` is treated the same as
`foo()` when deciding can_fix.
|
Closing. This is a bot and it can't get things right. |
ah that’s frustrating, sorry about that. i’m working on an implementation of this also, made a lot of progress last night; i’ll try to learn/apply what i can from your initial feedback on this PR |
|
take your time @pkallos, no need to rush things. Thank you! |
3 participants
AI Disclosure: This PR was developed with AI assistance (Claude Code).
Summary
useNullishCoalescingnursery rule to detect ternary expressions that perform explicit nullish checks and suggest rewriting them with??.ignoreTernaryTestsoption (default:false) to allow disabling ternary detection independently from the existing logical OR detection.Detected patterns
Simple checks:
Compound checks:
Also handles member expressions, computed members, parenthesized subjects, and function calls.
Test plan
ignoreTernaryTests: truesuppresses all ternary diagnosticsjust f,just l,just gen-analyzerall passCloses #9229