feat(lint): add ternary expression detection to useNullishCoalescing#9248
feat(lint): add ternary expression detection to useNullishCoalescing#9248pkallos wants to merge 1 commit intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: fc346e5 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 |
| "level": "error", | ||
| "options": { | ||
| "ignoreTernaryTests": true | ||
| } |
There was a problem hiding this comment.
as with the prior PR for this, the inclusion of the option here is meant to move towards feature parity with the corresponding eslint rule, logically this one is relatively straightforward because it should just "turn off" the ternary check behavior that's added in this changeset.
if any issue lmk and I can remove!
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ternary (conditional) expression support to the useNullishCoalescing lint: the rule now queries both logical-or and conditional expressions, recognises loose/strict/compound nullish checks, and computes diagnostics and safe fixes for converting patterns like 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs (1)
225-232: Minor concern: potential early return on missing trivia.Lines 228 and 231 use
?onfirst_leading_trivia()andlast_trailing_trivia(). If the original ternary has no leading/trailing trivia, this returnsNoneand the action silently fails. This is likely rare but could suppress fixes unexpectedly.Consider whether an empty trivia fallback would be more robust, or if this is intentional defensive behaviour.
💡 Optional: handle missing trivia gracefully
- let new_expr = AnyJsExpression::from(new_expr) - .prepend_trivia_pieces( - ternary.syntax().first_leading_trivia()?.pieces(), - )? - .append_trivia_pieces( - ternary.syntax().last_trailing_trivia()?.pieces(), - )?; + let mut new_expr = AnyJsExpression::from(new_expr); + if let Some(leading) = ternary.syntax().first_leading_trivia() { + new_expr = new_expr.prepend_trivia_pieces(leading.pieces())?; + } + if let Some(trailing) = ternary.syntax().last_trailing_trivia() { + new_expr = new_expr.append_trivia_pieces(trailing.pieces())?; + }🤖 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 225 - 232, The current transform uses `?` on `ternary.syntax().first_leading_trivia()` and `last_trailing_trivia()` in the `AnyJsExpression::from(new_expr).prepend_trivia_pieces(...)?` / `append_trivia_pieces(...)?` chain which causes an early return when trivia is absent; change those calls to provide an empty-fallback instead of propagating `None` (e.g., replace `first_leading_trivia()?` with `first_leading_trivia().map(|t| t.pieces()).unwrap_or_default()` and similarly for `last_trailing_trivia()`) so `prepend_trivia_pieces`/`append_trivia_pieces` receive an empty iterator when there is no trivia and the fix is applied reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Around line 225-232: The current transform uses `?` on
`ternary.syntax().first_leading_trivia()` and `last_trailing_trivia()` in the
`AnyJsExpression::from(new_expr).prepend_trivia_pieces(...)?` /
`append_trivia_pieces(...)?` chain which causes an early return when trivia is
absent; change those calls to provide an empty-fallback instead of propagating
`None` (e.g., replace `first_leading_trivia()?` with
`first_leading_trivia().map(|t| t.pieces()).unwrap_or_default()` and similarly
for `last_trailing_trivia()`) so `prepend_trivia_pieces`/`append_trivia_pieces`
receive an empty iterator when there is no trivia and the fix is applied
reliably.
ℹ️ 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/ignoreTernaryTestsEnabled.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryCommentTrivia.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryValid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (8)
.changeset/use-nullish-coalescing-ternary.mdcrates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryCommentTrivia.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryValid.tscrates/biome_rule_options/src/use_nullish_coalescing.rs
|
@ematipico @dyc3 hopefully this is a more maintainer-friendly implementation vs #9240 🫡 |
Merging this PR will not alter performance
Comparing Footnotes
|
| JsSyntaxKind::JS_CALL_EXPRESSION | JsSyntaxKind::JS_NEW_EXPRESSION | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
I tried looking for a reusable construct here but wasn’t able to locate one, the gist of this check is looking for statements that are side effect producing.
if you have any thoughts about side effectfulness detection or other cases than just function call or object construction please let me know
e64550a to
7daecf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs (1)
100-112: MoveUseNullishCoalescingStatebelow theimpl Ruleblock for consistency.Small layout nit, but it keeps nursery rule files aligned with the house style and easier to scan.
Based on learnings: In
crates/biome_analyze/**/*.rsrule files, all helper functions, structs, and enums must be placed below theimpl Ruleblock; only node unions are exempt.🤖 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 100 - 112, The enum UseNullishCoalescingState is declared above the impl Rule block; move its declaration so it appears below the impl Rule block to match project layout conventions. Locate the UseNullishCoalescingState enum and cut/paste it to a position after the impl Rule { ... } implementation for the rule (the impl for Rule in this file), ensuring any uses inside the impl still compile and adjusting use/import ordering if necessary.
🤖 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 409-417: The current fix logic must be tightened: change
extract_nullish_comparison_operand to return which literal was tested (null or
undefined), extend the NullishCheckKind enum so StrictSingle carries that
literal and update the Compound variant validation to ensure the two sides test
different literals; then in the validation branch that uses
ctx.type_of_expression and type_has_null_and_undefined, for StrictSingle(lit)
only verify that the opposite literal is impossible in the expression type (not
both), and for Compound require one side is null and the other is undefined;
update the matching code paths that construct/consume NullishCheckKind
(including the branches around where StrictSingle and Compound are checked) and
add unit tests (e.g., x !== null ? x : y with type string | undefined) to cover
these cases.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Around line 100-112: The enum UseNullishCoalescingState is declared above the
impl Rule block; move its declaration so it appears below the impl Rule block to
match project layout conventions. Locate the UseNullishCoalescingState enum and
cut/paste it to a position after the impl Rule { ... } implementation for the
rule (the impl for Rule in this file), ensuring any uses inside the impl still
compile and adjusting use/import ordering if necessary.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
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 (1)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs
7daecf1 to
0c22263
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs (1)
17-94: Consider adding a compound-check example to the documentation.The rule docs cover simple strict/loose patterns but don't show compound checks like
x !== null && x !== undefined ? x : y. A brief example would help users understand the full scope of detected patterns.📚 Suggested documentation addition
Add after line 59 in the Invalid examples:
/// const value = x == null ? 'default' : x; // should use ?? /// ``` +/// +/// ```ts,expect_diagnostic +/// declare const x: string | null | undefined; +/// const value = x !== null && x !== undefined ? x : 'default'; // should use ?? +/// ``` ///🤖 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 17 - 94, The documentation for the UseNullishCoalescing rule is missing an example of a compound null/undefined check; update the doc comment inside the declare_lint_rule for UseNullishCoalescing by adding an Invalid example that shows a compound ternary pattern such as checking both null and undefined (e.g., "x !== null && x !== undefined ? x : 'default'") so users can see that patterns like x !== null && x !== undefined ? x : y are flagged and should be rewritten to use ??.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Around line 17-94: The documentation for the UseNullishCoalescing rule is
missing an example of a compound null/undefined check; update the doc comment
inside the declare_lint_rule for UseNullishCoalescing by adding an Invalid
example that shows a compound ternary pattern such as checking both null and
undefined (e.g., "x !== null && x !== undefined ? x : 'default'") so users can
see that patterns like x !== null && x !== undefined ? x : y are flagged and
should be rewritten to use ??.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryCommentTrivia.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryValid.ts.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 (8)
.changeset/use-nullish-coalescing-ternary.mdcrates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryCommentTrivia.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryValid.tscrates/biome_rule_options/src/use_nullish_coalescing.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/biome_rule_options/src/use_nullish_coalescing.rs
- crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.options.json
- crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryValid.ts
- crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryCommentTrivia.ts
ematipico
left a comment
There was a problem hiding this comment.
Tests that use the new option are missing. We must add them, and the documentation must be updated to reflect the new option
There was a problem hiding this comment.
I think the changeset can be a little more succinct, the documentation will take care of explaining the rest.
There was a problem hiding this comment.
I'm a bit unsure about when a changeset is required/useful.
From the contributing docs:
If the PR you're about to open is a bugfix/feature visible to users of the Biome toolchain or of the published Biome crates, you are encouraged to provide a changeset.
In this comment for #9257 there was some feedback that maybe I shouldn't include a changeset because the rule isn't new or released yet, so I removed the changeset from there.
This PR is making a similar type of change (adding an enhancement to an existing the nursery rule), should it be accompanied with a changeset or should I remove it from the PR?
Maybe they both need changesets, or neither need changesets?
Happy to do it either way! Just curious because I have another PR coming up for implementing #9232 and I'll want to apply the same practice.
Please let me know and I'll adjust.
| /// offered when type analysis confirms the left operand can only be truthy or | ||
| /// nullish (not other falsy values like `0` or `''`). | ||
| /// | ||
| /// For ternary expressions, this rule detects patterns like `x !== null ? x : y` |
There was a problem hiding this comment.
The documentation should now document the option you added.
Please refer to the contribution guide to understand the format
There was a problem hiding this comment.
OK I believe I've managed to adjust the documentation to describe the behavior and have documented the option in a new ## Options
I also added that same documentation for ignoreConditionalTests , which was an option I had introduced alongside the initial implementation in #8952 without following the pattern, so hopefully this cleans that up in the right way as well!
0c22263 to
fbb8738
Compare
|
@ematipico thanks for spending time on this! I've squashed/pushed some fixes to address the feedback. Shortened the changeset statement. Added ## Options documentation for both ignoreConditionalTests and ignoreTernaryTests in the rule's doc comments, following the json,options + ts,use_options format from the contribution guide. Also shortened the changeset.
I may have mustunderstood but I think spec tests for the option already existed (ignoreTernaryTestsEnabled.ts + .options.json), but the doc-comment test blocks were missing; those are now added too. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs (1)
455-465:⚠️ Potential issue | 🔴 CriticalUnsafe auto-fix is still possible for strict-single and malformed compound checks
On Line 461,
StrictSingleis treated as fixable unless the type has both null and undefined. That still permits behaviour changes (e.g. null-only check withstring | undefined).
Also, Lines 538-545 allow compound checks that can test the same literal twice (null/nullorundefined/undefined) and still be fixable.Please carry the tested literal kind through
extract_nullish_comparison_operand, then:
- for strict-single: ensure the other nullish variant is impossible,
- for compound: require one side checks
nulland the other checksundefined.Suggested direction (minimal diff sketch)
-enum NullishCheckKind { Loose, StrictSingle, Compound } +enum NullishCheckKind { Loose, StrictSingle(NullishLiteralKind), Compound } -fn extract_nullish_comparison_operand(...) -> Option<AnyJsExpression> +fn extract_nullish_comparison_operand(...) -> Option<(AnyJsExpression, NullishLiteralKind)> -NullishCheckKind::StrictSingle => !type_has_null_and_undefined(&ty) +NullishCheckKind::StrictSingle(kind) => type_excludes_opposite_nullish(&ty, kind) -if let (Some(checked_left), Some(checked_right)) = (...) +if let (Some((checked_left, left_kind)), Some((checked_right, right_kind))) = (...) + && left_kind != right_kindAlso applies to: 510-545, 551-582
🤖 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 455 - 465, The current fixability logic incorrectly allows unsafe auto-fixes for StrictSingle and malformed Compound checks; update extract_nullish_comparison_operand to return which literal kind (Null or Undefined) each side tests, then use that in the can_fix computation: for NullishCheckKind::StrictSingle, confirm the extracted literal kind rules out the other nullish variant (i.e., the expression's type cannot include the opposite literal) before marking fixable; for NullishCheckKind::Compound, require that the two extracted literal kinds are complementary (one Null and one Undefined) and reject cases where both sides test the same literal; apply this change wherever the StrictSingle/Compound fixability is evaluated (functions handling NullishCheckKind and uses of extract_nullish_comparison_operand).
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs (1)
147-159: MoveUseNullishCoalescingStatebelow theimpl RuleblockSmall layout tidy-up: this enum is a helper type for the rule and is better placed below
impl Ruleto match repository conventions (node unions are the explicit exception).Based on learnings: In
crates/biome_analyze/**/*.rsrule files, all helper functions, structs, and enums must be placed below theimpl Ruleblock; only node unions may stay above for readability.🤖 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 147 - 159, Move the helper enum UseNullishCoalescingState so it appears below the impl Rule block (i.e., after the Rule implementation) to follow the repository convention that helper functions/structs/enums live below impl Rule; update any local references inside the file (e.g., places that pattern-match on UseNullishCoalescingState) to the same visibility/scope after moving it so compilation and name resolution remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Around line 455-465: The current fixability logic incorrectly allows unsafe
auto-fixes for StrictSingle and malformed Compound checks; update
extract_nullish_comparison_operand to return which literal kind (Null or
Undefined) each side tests, then use that in the can_fix computation: for
NullishCheckKind::StrictSingle, confirm the extracted literal kind rules out the
other nullish variant (i.e., the expression's type cannot include the opposite
literal) before marking fixable; for NullishCheckKind::Compound, require that
the two extracted literal kinds are complementary (one Null and one Undefined)
and reject cases where both sides test the same literal; apply this change
wherever the StrictSingle/Compound fixability is evaluated (functions handling
NullishCheckKind and uses of extract_nullish_comparison_operand).
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Around line 147-159: Move the helper enum UseNullishCoalescingState so it
appears below the impl Rule block (i.e., after the Rule implementation) to
follow the repository convention that helper functions/structs/enums live below
impl Rule; update any local references inside the file (e.g., places that
pattern-match on UseNullishCoalescingState) to the same visibility/scope after
moving it so compilation and name resolution remain unchanged.
ℹ️ 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/ignoreTernaryTestsEnabled.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryCommentTrivia.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryValid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (8)
.changeset/use-nullish-coalescing-ternary.mdcrates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryCommentTrivia.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryValid.tscrates/biome_rule_options/src/use_nullish_coalescing.rs
✅ Files skipped from review due to trivial changes (1)
- crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.options.json
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryCommentTrivia.ts
- crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.ts
- .changeset/use-nullish-coalescing-ternary.md
- crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreTernaryTestsEnabled.ts
- crates/biome_rule_options/src/use_nullish_coalescing.rs
- crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryValid.ts
900a784 to
430c515
Compare
Ah that's on me. The GitHub UI cut off the name of the file, and I couldn't see the suffix. Sorry about that |
All good! Odds are on your side that you'd be right haha |
430c515 to
fc346e5
Compare
AI Disclosure: This PR was developed with AI assistance (Claude).
Summary
Closes #9229
Extends
useNullishCoalescingto detect ternary expressions that perform explicit nullish checks and suggest rewriting them with??.Detected patterns include strict equality (
x !== null ? x : y), loose equality (x != null ? x : y), compound checks (x !== null && x !== undefined ? x : y), and member access (obj.prop !== null ? obj.prop : 'default').Fix safety:
x !== null): fix only offered when type analysis confirms the variable can't be bothnullandundefinednew: no fix offered, since??changes the evaluation countA new
ignoreTernaryTestsoption (default:false) allows disabling ternary detection.Comments attached to the
?and:tokens are transferred to the corresponding branch in the??output.Continues the incremental approach from #8952. Addresses #8043.
Test Plan
ternaryInvalid.ts: strict/loose/compound patterns, member access, precedence edge cases, side-effect safety, various nullish type combinationsternaryValid.ts: non-nullish comparisons, mixed operands, non-matching branchesternaryCommentTrivia.ts: comment preservation through the fixignoreTernaryTestsEnabled.ts: option suppresses all ternary diagnosticscargo test -p biome_js_analyze -- nursery::use_nullish_coalescing(10 tests)Docs
Rule doc comments updated with ternary examples. Option documented inline.