feat(lint): add ignoreMixedLogicalExpressions option to useNullishCoa…#9974
feat(lint): add ignoreMixedLogicalExpressions option to useNullishCoa…#9974pkallos wants to merge 1 commit intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 9a9b316 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 |
| } | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
aiming to implement function this equivalent in eslint to detect mixed logical expressions
WalkthroughAdds an 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.
Actionable comments posted: 4
🤖 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/add-ignore-mixed-logical-expressions.md:
- Line 5: Update the changeset entry to the repo’s standard changeset format:
mention the added option ignoreMixedLogicalExpressions on the
useNullishCoalescing rule, state that it suppresses diagnostics for mixed || and
&& expression trees and also covers the ||= assignment form, and include a link
to issue `#9232` and a link to the useNullishCoalescing rule documentation (use
the official issue URL and the rule docs URL) so the change references both the
bug report and the rule page.
In `@crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs`:
- Around line 147-164: Update the option documentation for
ignoreMixedLogicalExpressions to reflect its actual behavior: state that it
suppresses transformations across a mixed logical tree (not just `||` mixed with
`&&`), including `||=` handled by run_logical_or_assignment() and parenthesized
subexpressions that the helper walks through; adjust the descriptive sentence
and examples to mention "same mixed logical tree" and note the ||= case so the
docs match the implementation in run_logical_or_assignment and the
parenthesis-aware logic.
- Around line 515-586: is_in_mixed_logical_tree currently only expands
JsLogicalExpression nodes during the BFS, so nested JsAssignmentExpression nodes
(like LogicalOrAssign `||=`) are not traversed and chains such as a ||= (b ||= c
&& d) are missed; modify is_in_mixed_logical_tree to treat
JsAssignmentExpression (specifically LogicalOrAssign) similar to
JsLogicalExpression inside the loop: when casting current to
JsAssignmentExpression (or detecting LogicalOrAssign operator) enqueue its
parent and its right-hand side (and handle parenthesized expressions same as for
JsParenthesizedExpression), and ensure seed_children still queues initial
children; also add a regression spec that tests nested `||=` chains (e.g., a ||=
(b ||= c && d)) to verify the fix.
In `@crates/biome_rule_options/src/use_nullish_coalescing.rs`:
- Around line 23-30: Add the serde skip-serializing attribute to the optional
field so unset values are omitted from serialized output: annotate the field
ignore_mixed_logical_expressions in use_nullish_coalescing.rs with
#[serde(skip_serializing_if = "Option::is_none")] (ensure serde::Serialize is
available for the struct) so None does not serialize as null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4233adc5-faf3-47d2-a27f-33cc034ff90c
⛔ Files ignored due to path filters (4)
crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDefault.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDisabled.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 (7)
.changeset/add-ignore-mixed-logical-expressions.mdcrates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDefault.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDefault.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDisabled.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDisabled.tscrates/biome_rule_options/src/use_nullish_coalescing.rs
Merging this PR will not alter performance
Comparing Footnotes
|
1b9fc0a to
1a7af7f
Compare
|
@ematipico @dyc3 this is ready for a look when you have a moment! Also, just so I know, what would be the typical path to promote a rule out of nursery? Should I aim to do that after finalizing all the option adds and getting feature-parity with the eslint rule? Is there another step in the process or just PR to promote it out of nursery? |
|
Generally, after a rule is completed, we wait until the rule has been out for an entire minor to promote it out of nursery. So if a rule is released first in 2.4.8, it could get promoted in 2.6.0 at the earliest. It's something we do at our discretion depending on open bugs and such. |
ematipico
left a comment
There was a problem hiding this comment.
The logic of the new code is difficult to grasp and review. I left some suggestions
| /// Default: `false` | ||
| pub ignore_ternary_tests: Option<bool>, | ||
|
|
||
| /// Whether to ignore `||` and `||=` expressions that are part of a mixed |
There was a problem hiding this comment.
| /// Whether to ignore `||` and `||=` expressions that are part of a mixed | |
| /// Whether to ignore `||` and `||=` binary operations that are part of a mixed |
| /// logical expression tree with `&&`, including through parenthesized | ||
| /// subexpressions. | ||
| /// | ||
| /// When `true`, the rule will not report `||` or `||=` expressions where | ||
| /// the operator is mixed with `&&` in the same logical tree. |
There was a problem hiding this comment.
The two paragraphs repeat the same thing. I'm sure we can rephrase it.
| /// BFS through the connected tree of `||`/`||=` nodes to find any `&&` | ||
| /// operator, matching ESLint's `isMixedLogicalExpression` behavior. | ||
| /// Works for both `JsLogicalExpression` (`||`) and `JsAssignmentExpression` (`||=`). | ||
| fn is_in_mixed_logical_tree(node: &JsSyntaxNode) -> bool { |
There was a problem hiding this comment.
Don't create functions that accept generic nodes, because you can't know what's the expected node the function is supposed to receive
| /// operator, matching ESLint's `isMixedLogicalExpression` behavior. | ||
| /// Works for both `JsLogicalExpression` (`||`) and `JsAssignmentExpression` (`||=`). | ||
| fn is_in_mixed_logical_tree(node: &JsSyntaxNode) -> bool { | ||
| use std::collections::HashSet; |
| /// BFS through the connected tree of `||`/`||=` nodes to find any `&&` | ||
| /// operator, matching ESLint's `isMixedLogicalExpression` behavior. | ||
| /// Works for both `JsLogicalExpression` (`||`) and `JsAssignmentExpression` (`||=`). |
There was a problem hiding this comment.
Reword this docs. We don't care what eslint does, we care about the business logic of this function.
| if let Some(logical) = JsLogicalExpression::cast_ref(node) { | ||
| if let Ok(left) = logical.left() { | ||
| queue.push(left.into_syntax()); | ||
| } | ||
| if let Ok(right) = logical.right() { | ||
| queue.push(right.into_syntax()); | ||
| } | ||
| } else if let Some(assignment) = JsAssignmentExpression::cast_ref(node) | ||
| && let Ok(right) = assignment.right() | ||
| { | ||
| queue.push(right.into_syntax()); |
There was a problem hiding this comment.
I suggest creating a new node, union of these two nodes. Having typed nodes is more valuable, safer and better to use . You can use declare_node_union.
Make sure you use the correct naming convention
|
|
||
| let mut i = 0; | ||
| while i < queue.len() { | ||
| let current = queue[i].clone(); | ||
| i += 1; |
There was a problem hiding this comment.
Why don't you use a simple loop?
| /// BFS through the connected tree of `||`/`||=` nodes to find any `&&` | ||
| /// operator, matching ESLint's `isMixedLogicalExpression` behavior. | ||
| /// Works for both `JsLogicalExpression` (`||`) and `JsAssignmentExpression` (`||=`). | ||
| fn is_in_mixed_logical_tree(node: &JsSyntaxNode) -> bool { |
There was a problem hiding this comment.
I suggest returning Option<bool> so you can use the try operator, and stop using let-else all over
| } | ||
|
|
||
| // Walk through parenthesized expressions in both directions | ||
| if let Some(paren) = JsParenthesizedExpression::cast_ref(¤t) { |
There was a problem hiding this comment.
You can use omit_parethesis from AnyJsExpression to retrieve the real expression
| seed_children(node, &mut queue); | ||
|
|
||
| let mut i = 0; | ||
| while i < queue.len() { | ||
| let current = queue[i].clone(); | ||
| i += 1; | ||
|
|
||
| if !seen.insert(current.text_trimmed_range()) { | ||
| continue; | ||
| } | ||
|
|
||
| // Walk through parenthesized expressions in both directions | ||
| if let Some(paren) = JsParenthesizedExpression::cast_ref(¤t) { | ||
| if let Ok(expr) = paren.expression() { | ||
| queue.push(expr.into_syntax()); | ||
| } | ||
| if let Some(parent) = current.parent() { | ||
| queue.push(parent); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if let Some(logical) = JsLogicalExpression::cast_ref(¤t) | ||
| && let Ok(op) = logical.operator() | ||
| { | ||
| if op == JsLogicalOperator::LogicalAnd { | ||
| return true; | ||
| } | ||
| if op == JsLogicalOperator::LogicalOr { | ||
| if let Some(parent) = current.parent() { | ||
| queue.push(parent); | ||
| } | ||
| if let Ok(left) = logical.left() { | ||
| queue.push(left.into_syntax()); | ||
| } | ||
| if let Ok(right) = logical.right() { | ||
| queue.push(right.into_syntax()); | ||
| } | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This function is really difficult to understand.
We walk the parents and create a queue of certain nodes, then we iterate the queue again and we add other nodes to the queue given a certain logic.
I suggest the following changes to help readability:
- clearly state the business logic of the function
- use typed nodes from the beginning
- comment the parts of the code that are hard to read
|
@ematipico really appreciate you taking the time to review this and provide feedback, will make sure to get it all addressed, and try my best to bake in the themes into subsequent PRs. |
b3238c0 to
9a9b316
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDefault.ts (2)
24-31: Comment example doesn't match the code below.The comment describes
(a || b) || (c && d)but the actual expression on line 31 isx || y || (z && w). Either tweak the comment to usex/y/z/wor adjust the example to make them agree — future readers will thank you.🤖 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/ignoreMixedLogicalExpressionsDefault.ts` around lines 24 - 31, The comment above the example is inconsistent: it describes `(a || b) || (c && d)` but the actual code uses `x || y || (z && w)` (assigned to r5); update the comment so it uses the same identifiers as the code (e.g., change the comment to reference `x || y || (z && w)` or rename the expression to `(a || b) || (c && d)`), ensuring the comment and the expression around r5 and the variables x, y, z, w match.
1-2: Filename is a touch misleading.The option's default value is
false(per PR summary and ESLint parity), but this file tests the behaviour whenignoreMixedLogicalExpressionsis set totrue. Pairing…Default.tswith…Disabled.tsmakes the intent ambiguous — something likeignoreMixedLogicalExpressionsEnabled.tswould mirror the disabled fixture more symmetrically. Not a blocker; rename only if you agree.🤖 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/ignoreMixedLogicalExpressionsDefault.ts` around lines 1 - 2, The test file name is misleading: it asserts behavior for ignoreMixedLogicalExpressions=true but is named ignoreMixedLogicalExpressionsDefault.ts; rename this fixture to ignoreMixedLogicalExpressionsEnabled.ts (to mirror the existing ignoreMixedLogicalExpressionsDisabled.ts) and update any test manifests or imports that reference ignoreMixedLogicalExpressionsDefault.ts so the test runner picks up the renamed file.crates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rs (2)
373-378: Minor:is_some_and(|mixed| mixed)reads awkwardly.Since
is_in_mixed_logical_treereturnsOption<bool>but callers never use?here, consider either returning a plainboolor simplifying the call site. Same pattern on lines 406-411.♻️ Optional simplification
- if options.ignore_mixed_logical_expressions() - && is_in_mixed_logical_tree(AnyJsLogicalOrLikeExpression::from(logical.clone())) - .is_some_and(|mixed| mixed) - { - return None; - } + if options.ignore_mixed_logical_expressions() + && is_in_mixed_logical_tree(AnyJsLogicalOrLikeExpression::from(logical.clone())) + == Some(true) + { + return None; + }Or drop the
Optionentirely and returnbool, since theNonecase (non-||/||=node) is already guaranteed by the two call sites.🤖 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 373 - 378, The call site uses is_in_mixed_logical_tree(...).is_some_and(|mixed| mixed) which reads awkwardly; change these to a clearer boolean check such as is_in_mixed_logical_tree(AnyJsLogicalOrLikeExpression::from(logical.clone())).unwrap_or(false) (or == Some(true)) where options.ignore_mixed_logical_expressions() is checked, and do the same simplification for the other occurrence around the block that spans lines 406-411; alternatively, change is_in_mixed_logical_tree to return bool and update both call sites (use the function name is_in_mixed_logical_tree and the type AnyJsLogicalOrLikeExpression to locate and update implementations/usages).
507-585: Traversal logic is tight — nested||=chains are now handled.
climb_to_logical_or_rootbounds the upward walk at the first non-||/||=ancestor, andoperand_reaches_logical_andrecurses through bothJsLogicalExpressionand||=JsAssignmentExpressionnodes (viaomit_parentheses). I traced a few shapes mentally —(a || b) && c,x || y || (z && w),a ||= (b ||= c && d),a ||= b || (c && d)— and each settles on the correct root and correctly detects (or doesn't detect) the&&. Previous review feedback on typed nodes, bounded ancestor walks andomit_parentheseshas all landed.One tiny readability nit: the doc on
is_in_mixed_logical_treeadvertises?-chaining for theNonecase, but both callers use.is_some_and(...)(flagged separately). If you flip this helper to returnbool, you can remove thethen_some(())?guard too — the two call sites already satisfy the precondition by construction.🤖 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 507 - 585, Change is_in_mixed_logical_tree to return bool instead of Option<bool>: remove the leading node.is_logical_or_form().then_some(())? check, update the signature fn is_in_mixed_logical_tree(node: AnyJsLogicalOrLikeExpression) -> bool, and simply compute and return the boolean (Some(...) -> bool). Update callers that currently use .is_some_and(...) on is_in_mixed_logical_tree to treat it as a plain bool (drop the Option handling). Keep existing helper functions (climb_to_logical_or_root, has_logical_and_directly_above, tree_contains_logical_and, operand_reaches_logical_and, parent_logical_or) unchanged.
🤖 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 373-378: The call site uses
is_in_mixed_logical_tree(...).is_some_and(|mixed| mixed) which reads awkwardly;
change these to a clearer boolean check such as
is_in_mixed_logical_tree(AnyJsLogicalOrLikeExpression::from(logical.clone())).unwrap_or(false)
(or == Some(true)) where options.ignore_mixed_logical_expressions() is checked,
and do the same simplification for the other occurrence around the block that
spans lines 406-411; alternatively, change is_in_mixed_logical_tree to return
bool and update both call sites (use the function name is_in_mixed_logical_tree
and the type AnyJsLogicalOrLikeExpression to locate and update
implementations/usages).
- Around line 507-585: Change is_in_mixed_logical_tree to return bool instead of
Option<bool>: remove the leading node.is_logical_or_form().then_some(())? check,
update the signature fn is_in_mixed_logical_tree(node:
AnyJsLogicalOrLikeExpression) -> bool, and simply compute and return the boolean
(Some(...) -> bool). Update callers that currently use .is_some_and(...) on
is_in_mixed_logical_tree to treat it as a plain bool (drop the Option handling).
Keep existing helper functions (climb_to_logical_or_root,
has_logical_and_directly_above, tree_contains_logical_and,
operand_reaches_logical_and, parent_logical_or) unchanged.
In
`@crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDefault.ts`:
- Around line 24-31: The comment above the example is inconsistent: it describes
`(a || b) || (c && d)` but the actual code uses `x || y || (z && w)` (assigned
to r5); update the comment so it uses the same identifiers as the code (e.g.,
change the comment to reference `x || y || (z && w)` or rename the expression to
`(a || b) || (c && d)`), ensuring the comment and the expression around r5 and
the variables x, y, z, w match.
- Around line 1-2: The test file name is misleading: it asserts behavior for
ignoreMixedLogicalExpressions=true but is named
ignoreMixedLogicalExpressionsDefault.ts; rename this fixture to
ignoreMixedLogicalExpressionsEnabled.ts (to mirror the existing
ignoreMixedLogicalExpressionsDisabled.ts) and update any test manifests or
imports that reference ignoreMixedLogicalExpressionsDefault.ts so the test
runner picks up the renamed file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f83c7cc-c244-4a65-8212-a81e69077441
⛔ Files ignored due to path filters (11)
crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/assignmentCommentTrivia.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/assignmentInvalid.ts.snapis excluded by!**/*.snapand included by**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/ignoreMixedLogicalExpressionsDefault.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDisabled.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/ternaryCommentTrivia.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ternaryInvalid.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 (7)
.changeset/add-ignore-mixed-logical-expressions.mdcrates/biome_js_analyze/src/lint/nursery/use_nullish_coalescing.rscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDefault.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDefault.tscrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDisabled.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDisabled.tscrates/biome_rule_options/src/use_nullish_coalescing.rs
✅ Files skipped from review due to trivial changes (3)
- crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDisabled.options.json
- .changeset/add-ignore-mixed-logical-expressions.md
- crates/biome_js_analyze/tests/specs/nursery/useNullishCoalescing/ignoreMixedLogicalExpressionsDefault.options.json
|
@ematipico pushed a revision, responses inline.
reworked the whole mixed-logical detection path , it's now one typed node union, one small climb helper, one small descent helper overall shape is now in
reworded as specified!
collapsed to a single line, also added in
doc now reworded, shortened.
no helper takes a bare
climb stops at the first non-
replaced with more sane loop!
split into four typed helpers: the non-obvious rule (
done as also:
|
This PR was developed with AI assistance.
Summary
Partial fix for #9232.
Adds the
ignoreMixedLogicalExpressionsoption touseNullishCoalescing. This is the first in a series of PRs to reach feature parity with ESLint'sprefer-nullish-coalescing(split from #9841 per reviewer feedback to keep PRs scoped).When
true, suppresses diagnostics on||and||=expressions that are mixed with&&in the same expression tree. Defaults tofalseto match ESLint.ignoreMixedLogicalExpressionsboolfalse||when mixed with&&in the same expressionImplementation uses BFS to walk the connected tree of
||/||=nodes (through parenthesized expressions) looking for any&&operator.Test Plan
Two test files, one per option value:
ignoreMixedLogicalExpressionsDefault.ts— optiontrue, verifies mixed expressions are suppressed and non-mixed expressions still diagnoseignoreMixedLogicalExpressionsDisabled.ts— optionfalse, verifies all expressions diagnose normallyCovers: basic mixing (
(a || b) && c), chained||with&&sibling (x || y || (z && w)), three-way chains without parens,||=with direct&&, and negative cases (no mixing, should still diagnose).Docs
Option documented in rustdoc with
json,options+ts,use_optionsexamples. No website PR needed — rule is still in nursery.