fix(useOptionalChain): fix negated expressions#9567
Merged
Conversation
🦋 Changeset detectedLatest commit: 967f340 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
Contributor
WalkthroughThis PR extends the Possibly related PRs
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)
Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/complexity/use_optional_chain.rs (1)
995-1102: Consider extracting shared logic to reduce duplication.
optional_chain_expression_nodes_from_negated_oris ~100 lines nearly identical tooptional_chain_expression_nodes. The only differences are the traversal operator (||vs&&), the expression extraction (strip_negationvsnormalized_optional_chain_like), and the finalnegatedflag.A helper that accepts these as parameters would halve the code and simplify future maintenance. That said, it's not blocking for this bug fix.
♻️ Sketch of potential refactor
// Pseudocode outline enum ChainMode { LogicalAnd, NegatedOr, } fn extract_chain_nodes( &mut self, logical: &JsLogicalExpression, model: &SemanticModel, mode: ChainMode, ) -> Option<LogicalAndChainNodes> { let (expected_op, extract_head, negated) = match mode { ChainMode::LogicalAnd => (JsLogicalOperator::LogicalAnd, normalize_fn, false), ChainMode::NegatedOr => (JsLogicalOperator::LogicalOr, strip_negation_fn, true), }; // ... shared traversal and comparison logic ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/complexity/use_optional_chain.rs` around lines 995 - 1102, optional_chain_expression_nodes_from_negated_or duplicates almost all logic from optional_chain_expression_nodes; extract the shared traversal/comparison code into a single helper (e.g., extract_chain_nodes) that takes parameters for the expected JsLogicalOperator, a head-extraction function (use strip_negation for negated || and normalized_optional_chain_like for &&), and a negated flag; move the loop that walks next_chain_head, the cmp_chain comparison, the parts_pop/tail logic, and final assembly into that helper and have optional_chain_expression_nodes_from_negated_or and optional_chain_expression_nodes call it, keeping references to symbols like JsLogicalOperator::LogicalOr/LogicalAnd, strip_negation, normalized_optional_chain_like, Self::from_expression, self.cmp_chain, and LogicalAndChainNodes 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/complexity/use_optional_chain.rs`:
- Around line 995-1102: optional_chain_expression_nodes_from_negated_or
duplicates almost all logic from optional_chain_expression_nodes; extract the
shared traversal/comparison code into a single helper (e.g.,
extract_chain_nodes) that takes parameters for the expected JsLogicalOperator, a
head-extraction function (use strip_negation for negated || and
normalized_optional_chain_like for &&), and a negated flag; move the loop that
walks next_chain_head, the cmp_chain comparison, the parts_pop/tail logic, and
final assembly into that helper and have
optional_chain_expression_nodes_from_negated_or and
optional_chain_expression_nodes call it, keeping references to symbols like
JsLogicalOperator::LogicalOr/LogicalAnd, strip_negation,
normalized_optional_chain_like, Self::from_expression, self.cmp_chain, and
LogicalAndChainNodes unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e03c48de-e8fb-4cb3-870d-0cb795d4b552
⛔ Files ignored due to path filters (2)
crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/invalidNegatedOrChain.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/validNegatedOrChain.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/feat-use-optional-chain-negated-or.mdcrates/biome_js_analyze/src/lint/complexity/use_optional_chain.rscrates/biome_js_analyze/tests/specs/complexity/useOptionalChain/invalidNegatedOrChain.jscrates/biome_js_analyze/tests/specs/complexity/useOptionalChain/validNegatedOrChain.js
ematipico
force-pushed
the
fix/negated-optional-chain
branch
from
2c7f647 to
967f340
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/complexity/use_optional_chain.rs (1)
998-1102: Consider sharing the&&and negated-||chain walkers.
optional_chain_expression_nodes_from_negated_or()is now almost a line-for-line fork ofoptional_chain_expression_nodes(). Pulling the common traversal into one helper would stop the tail-splitting and existing-optional-token bookkeeping from drifting the next time this rule picks up another corner-case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/complexity/use_optional_chain.rs` around lines 998 - 1102, The two functions optional_chain_expression_nodes_from_negated_or and optional_chain_expression_nodes duplicate the same traversal/collection logic; extract the shared loop into a single helper (e.g., walk_logical_chain) that takes parameters for: how to obtain the initial next_chain_head (a closure using logical.left()/right() and whether to strip_negation), a negated boolean, and callbacks to record prefix/parts; reuse existing helpers/symbols (strip_negation, normalized_optional_chain_like, Self::from_expression, cmp_chain, self.buf, LogicalAndChainNodes) inside the consolidated loop and keep only the small differences (initial head selection and negated flag) in the callers so all tail-splitting and optional-token bookkeeping is performed by the shared helper.
🤖 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/complexity/use_optional_chain.rs`:
- Around line 712-727: In is_inside_another_negated_chain, the code assumes the
stripped node sits directly under JsUnaryExpression, so change the parent
lookups to skip over any JsParenthesizedExpression wrappers: when locating the
unary around self.head (the unary variable from
self.head.parent::<JsUnaryExpression>()?) walk up through
JsParenthesizedExpression nodes until you find a JsUnaryExpression (or None),
and do the same when inspecting grand_parent.right() before calling
strip_negation/ Self::from_expression; update the parent traversal logic in
is_inside_another_negated_chain to transparently bypass
JsParenthesizedExpression nodes so nested negated chains like !(foo.bar) are
recognized correctly.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/complexity/use_optional_chain.rs`:
- Around line 998-1102: The two functions
optional_chain_expression_nodes_from_negated_or and
optional_chain_expression_nodes duplicate the same traversal/collection logic;
extract the shared loop into a single helper (e.g., walk_logical_chain) that
takes parameters for: how to obtain the initial next_chain_head (a closure using
logical.left()/right() and whether to strip_negation), a negated boolean, and
callbacks to record prefix/parts; reuse existing helpers/symbols
(strip_negation, normalized_optional_chain_like, Self::from_expression,
cmp_chain, self.buf, LogicalAndChainNodes) inside the consolidated loop and keep
only the small differences (initial head selection and negated flag) in the
callers so all tail-splitting and optional-token bookkeeping is performed by the
shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: edd8f2e9-b2ea-48cf-a72c-5ecaceecaf7a
⛔ Files ignored due to path filters (2)
crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/invalidNegatedOrChain.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/validNegatedOrChain.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/feat-use-optional-chain-negated-or.mdcrates/biome_js_analyze/src/lint/complexity/use_optional_chain.rscrates/biome_js_analyze/tests/specs/complexity/useOptionalChain/invalidNegatedOrChain.jscrates/biome_js_analyze/tests/specs/complexity/useOptionalChain/validNegatedOrChain.js
✅ Files skipped from review due to trivial changes (3)
- .changeset/feat-use-optional-chain-negated-or.md
- crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/validNegatedOrChain.js
- crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/invalidNegatedOrChain.js
Comment on lines
+712
to
+727
| fn is_inside_another_negated_chain(&self) -> Option<bool> { | ||
| let unary = self.head.parent::<JsUnaryExpression>()?; | ||
| let parent = unary.parent::<JsLogicalExpression>()?; | ||
| let grand_parent = parent.parent::<JsLogicalExpression>()?; | ||
| if !matches!(grand_parent.operator().ok()?, JsLogicalOperator::LogicalOr) { | ||
| return Some(false); | ||
| } | ||
| if grand_parent | ||
| .left() | ||
| .ok() | ||
| .as_ref() | ||
| .and_then(|e| e.as_js_logical_expression()) | ||
| == Some(&parent) | ||
| { | ||
| let stripped = strip_negation(&grand_parent.right().ok()?)?; | ||
| let gp_right_chain = Self::from_expression(stripped).ok()?; |
Contributor
There was a problem hiding this comment.
Skip transparent parentheses when suppressing nested negated chains.
strip_negation() already accepts !(foo.bar), but this helper assumes the stripped node sits directly under JsUnaryExpression. With !foo || !(foo.bar) || !(foo.bar.baz), the inner || chain then misses the nesting check and reports alongside the outer one. Please walk past JsParenthesizedExpression before looking for the unary parent.
🩹 Possible fix
- let unary = self.head.parent::<JsUnaryExpression>()?;
+ let unary = iter::successors(self.head.parent::<AnyJsExpression>(), |expression| {
+ if matches!(expression, AnyJsExpression::JsParenthesizedExpression(_)) {
+ expression.parent::<AnyJsExpression>()
+ } else {
+ None
+ }
+ })
+ .last()?
+ .as_js_unary_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/complexity/use_optional_chain.rs` around
lines 712 - 727, In is_inside_another_negated_chain, the code assumes the
stripped node sits directly under JsUnaryExpression, so change the parent
lookups to skip over any JsParenthesizedExpression wrappers: when locating the
unary around self.head (the unary variable from
self.head.parent::<JsUnaryExpression>()?) walk up through
JsParenthesizedExpression nodes until you find a JsUnaryExpression (or None),
and do the same when inspecting grand_parent.right() before calling
strip_negation/ Self::from_expression; update the parent traversal logic in
is_inside_another_negated_chain to transparently bypass
JsParenthesizedExpression nodes so nested negated chains like !(foo.bar) are
recognized correctly.
dyc3
approved these changes
Mar 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #7211
Code implemented with the help of a coding agent, but then modified to fit our coding style.
Test Plan
New tests added, mostly taken from typescript-eslint
Docs
Added a new test case