fix(lint/noUnnecessaryConditions): extend detection coverage#10108
Open
IxxyDev wants to merge 6 commits into
Open
fix(lint/noUnnecessaryConditions): extend detection coverage#10108IxxyDev wants to merge 6 commits into
IxxyDev wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: c490e5c 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
|
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:
WalkthroughThe lint rule noUnnecessaryConditions was enhanced to use type information. It now reports redundant optional chaining ( 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/biome_js_type_info/tests/conditionals.rs (1)
103-112: Nit: add symmetric!is_nullish()assertion for parity.The other two tests assert both
!is_truthy()and!is_nullish(); the string case only asserts non-truthy. Adding the symmetric check would make the regression guard uniform with minimal cost.🔧 Suggested addition
assert!( !conditional.is_truthy(), "optional primitive parameter should not be classified as truthy, got {conditional:?}" ); + assert!( + !conditional.is_nullish(), + "optional primitive parameter should not be classified as nullish, got {conditional:?}" + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_type_info/tests/conditionals.rs` around lines 103 - 112, The test optional_string_param_is_not_truthy only asserts !conditional.is_truthy(); add the symmetric nullish check by asserting !conditional.is_nullish() as well so the string-parameter regression guard matches the other tests; locate the test function optional_string_param_is_not_truthy and add an assertion using the conditional_type_of_first_param result: assert!(!conditional.is_nullish(), "optional primitive parameter should not be classified as nullish, got {conditional:?}");crates/biome_js_analyze/src/lint/nursery/no_unnecessary_conditions.rs (2)
749-781: Avoid theStringallocation — storeTokenTextinstead.
CaseLiteral::String(String)viainner_string_text(&token).text().to_string()forces a heap allocation for every case clause. Sinceinner_string_textalready returns aTokenText, you can carry it through and call.text()at comparison time.♻️ Suggested refactor
-enum CaseLiteral { - String(String), +enum CaseLiteral { + String(TokenText), Number(f64), Boolean(bool), Null, } @@ AnyJsLiteralExpression::JsStringLiteralExpression(s) => { let token = s.value_token().ok()?; - Some(CaseLiteral::String(inner_string_text(&token).text().to_string())) + Some(CaseLiteral::String(inner_string_text(&token))) } @@ CaseLiteral::String(s) => { if ty.is_string_or_string_literal() { if matches!(raw, TypeData::String) { return true; } - return ty.is_string_literal(s.as_str()); + return ty.is_string_literal(s.text()); } false }As per coding guidelines: "Avoid string allocations in rule implementations by using string references (&str) or TokenText instead of calling to_string()".
🤖 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/no_unnecessary_conditions.rs` around lines 749 - 781, The CaseLiteral::String variant allocates a new String; update the CaseLiteral enum and extract_case_literal to store TokenText (or &TokenText) instead of String and return that from inner_string_text(&token) without calling .to_string(), then adjust all places that match on CaseLiteral::String to compare using .text() (or otherwise access TokenText) rather than relying on owned String; specifically modify the CaseLiteral enum, the extract_case_literal function (replace inner_string_text(&token).text().to_string() with returning the TokenText) and any downstream match/compare logic to use TokenText.text() for comparisons.
789-850:type_could_equal_literal— conservative null handling is fine, keep the TODO.Treating
Undefined/VoidKeywordas possibly matchingcase null:is the right call until===disjointness is modelled. The inline TODO at line 845 is an adequate placeholder; consider linking it to a tracking issue so it doesn't get orphaned.🤖 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/no_unnecessary_conditions.rs` around lines 789 - 850, The conservative handling of Null in single_type_could_equal_literal (called by type_could_equal_literal) is correct; do not change the matching logic, but update the inline TODO inside the CaseLiteral::Null arm to reference a tracking issue/bug ID so it doesn't get orphaned—locate the TODO in single_type_could_equal_literal (the Comment above the matches!(raw, TypeData::Null | TypeData::Undefined | TypeData::VoidKeyword) line) and replace the generic TODO with a short note linking the issue number or URL.
🤖 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/no_unnecessary_conditions.rs`:
- Around line 769-772: The JsNumberLiteralExpression branch in
extract_case_literal currently uses token.text_trimmed().parse::<f64>().ok()
which silently skips non-decimal JS numeric forms (hex/bin/oct, underscores,
bigint suffix), causing those case literals to be ignored; update the
AnyJsLiteralExpression::JsNumberLiteralExpression handling to either (a) parse
JS numeric literal forms correctly (handle 0x/0b/0o prefixes, numeric separators
`_`, and strip a trailing `n` for bigint then convert/ceil to f64 or return a
distinct CaseLiteral variant) or (b) at minimum add a TODO and a clear comment
noting unsupported formats and conservatively return None, referencing the
AnyJsLiteralExpression::JsNumberLiteralExpression branch and CaseLiteral::Number
to locate the code to change.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/no_unnecessary_conditions.rs`:
- Around line 749-781: The CaseLiteral::String variant allocates a new String;
update the CaseLiteral enum and extract_case_literal to store TokenText (or
&TokenText) instead of String and return that from inner_string_text(&token)
without calling .to_string(), then adjust all places that match on
CaseLiteral::String to compare using .text() (or otherwise access TokenText)
rather than relying on owned String; specifically modify the CaseLiteral enum,
the extract_case_literal function (replace
inner_string_text(&token).text().to_string() with returning the TokenText) and
any downstream match/compare logic to use TokenText.text() for comparisons.
- Around line 789-850: The conservative handling of Null in
single_type_could_equal_literal (called by type_could_equal_literal) is correct;
do not change the matching logic, but update the inline TODO inside the
CaseLiteral::Null arm to reference a tracking issue/bug ID so it doesn't get
orphaned—locate the TODO in single_type_could_equal_literal (the Comment above
the matches!(raw, TypeData::Null | TypeData::Undefined | TypeData::VoidKeyword)
line) and replace the generic TODO with a short note linking the issue number or
URL.
In `@crates/biome_js_type_info/tests/conditionals.rs`:
- Around line 103-112: The test optional_string_param_is_not_truthy only asserts
!conditional.is_truthy(); add the symmetric nullish check by asserting
!conditional.is_nullish() as well so the string-parameter regression guard
matches the other tests; locate the test function
optional_string_param_is_not_truthy and add an assertion using the
conditional_type_of_first_param result: assert!(!conditional.is_nullish(),
"optional primitive parameter should not be classified as nullish, got
{conditional:?}");
🪄 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: f000c5fb-cc1f-4e1c-860a-80c0d58d81fc
⛔ Files ignored due to path filters (4)
crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
.changeset/fix-no-unnecessary-conditions-6611.mdcrates/biome_js_analyze/src/lint/nursery/no_unnecessary_conditions.rscrates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.tscrates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.jscrates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.tscrates/biome_js_type_info/tests/conditionals.rs
IxxyDev
force-pushed
the
fix/no-unnecessary-conditions-6611
branch
from
6de452f to
4fb9528
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.changeset/fix-no-unnecessary-conditions-6611.md (1)
5-12: Please condense this changeset body to 1–3 sentences.Great content, but this reads like mini release notes; the project guideline asks for concise changesets. Keep the issue/rule links and one compact example, and let the PR carry the full detail.
✂️ Suggested concise rewrite
-Fixed [`#6611`](https://github.com/biomejs/biome/issues/6611): the lint rule [`noUnnecessaryConditions`](https://biomejs.dev/linter/rules/no-unnecessary-conditions/) now correctly handles several cases that were previously missed. - -- Optional chaining on a non-nullish type is now flagged based on type information. Example: `function bar(arg: string) { return arg?.length }` now reports an unnecessary `?.`. -- Nullish coalescing on a non-nullish type is now flagged: `function bar(arg: string) { return arg ?? "default" }` now reports `??` as unnecessary. -- Logical expressions on member and call access use type information: `interface Foo { items: string[] }; function f(foo: Foo) { return foo.items || [] }` now reports `|| []` as unnecessary. -- Comparisons between a non-nullish type and `null`/`undefined` are flagged: `function f(x: string) { return x === null }` now reports. -- The `!expr` form of a condition now detects always-falsy: `const a = []; if (!a) {}` is now reported. -- `switch` statements are no longer wrongly flagged on the discriminant for literal unions like `'a' | 'b' | 'c'`. Instead, matching the behavior of `@typescript-eslint/no-unnecessary-condition`, individual `case` clauses that are statically unreachable — such as `case 'd':` inside `switch (value: 'a' | 'b' | 'c')` — are now flagged. +Fixed [`#6611`](https://github.com/biomejs/biome/issues/6611): the lint rule [`noUnnecessaryConditions`](https://biomejs.dev/linter/rules/no-unnecessary-conditions/) now uses type information to report missed unnecessary conditions, including redundant `?.`, `??`, `||`, and null/undefined comparisons. +Biome now also reports always-false negated conditions (for example, `if (![]) {}`) and unreachable `case` clauses for literal-union `switch` statements.As per coding guidelines: ".changeset/**: Write changeset descriptions concisely and clearly (1-3 sentences)...".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/fix-no-unnecessary-conditions-6611.md around lines 5 - 12, Condense the changeset body to 1–3 sentences: keep the issue link (`#6611`) and the linter rule link (noUnnecessaryConditions), summarize the key fix in one compact sentence (e.g., "noUnnecessaryConditions now uses type information to catch unnecessary optional chaining, nullish coalescing, logical expressions, null comparisons, always-falsy negations, and unreachable switch cases"), and include one short example such as `function bar(arg: string) { return arg?.length }` to illustrate the change; remove the multiline bullet list and detailed examples so the file reads as a concise summary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.changeset/fix-no-unnecessary-conditions-6611.md:
- Around line 5-12: Condense the changeset body to 1–3 sentences: keep the issue
link (`#6611`) and the linter rule link (noUnnecessaryConditions), summarize the
key fix in one compact sentence (e.g., "noUnnecessaryConditions now uses type
information to catch unnecessary optional chaining, nullish coalescing, logical
expressions, null comparisons, always-falsy negations, and unreachable switch
cases"), and include one short example such as `function bar(arg: string) {
return arg?.length }` to illustrate the change; remove the multiline bullet list
and detailed examples so the file reads as a concise summary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 619ef3b4-9b8f-42e2-a606-f948f92f5848
📒 Files selected for processing (1)
.changeset/fix-no-unnecessary-conditions-6611.md
Closes biomejs#6611. - Flag always-falsy conditions via logical-not (`!expr`). - Detect unnecessary optional chaining and nullish coalescing on non-nullish types, including on member and call expressions. - Detect null/undefined comparisons on non-nullish types. - Replace the prior incorrect discriminant-wide flag on `switch` with per-`case` unreachability analysis (matching `@typescript-eslint/no-unnecessary-condition`). - Handle hex, binary, octal, and numeric-separator literals in `case` clauses via `as_number`, rather than raw `parse::<f64>`.
IxxyDev
force-pushed
the
fix/no-unnecessary-conditions-6611
branch
from
4fb9528 to
5074ca0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/no_unnecessary_conditions.rs (1)
445-450: Deduplicate with the newis_undefined_identifierhelper.You just added
is_undefined_identifierfor exactly this check — might as well use it here too so the"undefined"string literal only lives in one place.♻️ Proposed refactor
AnyJsExpression::JsIdentifierExpression(id_expr) => { - if let Ok(name) = id_expr.name() - && name.value_token().ok()?.text_trimmed() == "undefined" - { + if is_undefined_identifier(expr) { return Some(IssueKind::AlwaysFalsyCondition(expr.range())); }🤖 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/no_unnecessary_conditions.rs` around lines 445 - 450, The identifier check inside the AnyJsExpression::JsIdentifierExpression arm duplicates the `"undefined"` literal; replace the manual name/value_token check with the new helper is_undefined_identifier to deduplicate the logic. Specifically, in the match arm for AnyJsExpression::JsIdentifierExpression(id_expr) call is_undefined_identifier(id_expr) and, if true, return Some(IssueKind::AlwaysFalsyCondition(expr.range())); leave the surrounding match and return behavior unchanged so semantics remain identical.
🤖 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/no_unnecessary_conditions.rs`:
- Around line 378-383: The helper is_undefined_identifier matches the identifier
purely by text which can misidentify a shadowed binding (e.g., const undefined =
1) as the real undefined; add a TODO above is_undefined_identifier describing
this limitation and why it’s acceptable (mirrors `@typescript-eslint` behavior and
strict mode forbids rebinding), so future maintainers know the known blind spot
and won’t chase it as a bug.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/no_unnecessary_conditions.rs`:
- Around line 445-450: The identifier check inside the
AnyJsExpression::JsIdentifierExpression arm duplicates the `"undefined"`
literal; replace the manual name/value_token check with the new helper
is_undefined_identifier to deduplicate the logic. Specifically, in the match arm
for AnyJsExpression::JsIdentifierExpression(id_expr) call
is_undefined_identifier(id_expr) and, if true, return
Some(IssueKind::AlwaysFalsyCondition(expr.range())); leave the surrounding match
and return behavior unchanged so semantics remain identical.
🪄 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: 50db4a5a-3bfe-42e7-a3c3-30db01298d5e
⛔ Files ignored due to path filters (4)
crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
.changeset/fix-no-unnecessary-conditions-6611.mdcrates/biome_js_analyze/src/lint/nursery/no_unnecessary_conditions.rscrates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.tscrates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.jscrates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.tscrates/biome_js_type_info/tests/conditionals.rs
✅ Files skipped from review due to trivial changes (2)
- crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.js
- crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_js_type_info/tests/conditionals.rs
- crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.ts
Merging this PR will not alter performance
Comparing 
Footnotes
|
dyc3
reviewed
Apr 24, 2026
| 130 │ break; | ||
|
|
||
| i The type being checked can never be falsy, making this condition redundant. | ||
| i The discriminant's type can never equal this value. |
Contributor
There was a problem hiding this comment.
nit: "discriminant" is a bit jargony.
ematipico
requested changes
Apr 27, 2026
Comment on lines
+5
to
+12
| Fixed [#6611](https://github.com/biomejs/biome/issues/6611): the lint rule [`noUnnecessaryConditions`](https://biomejs.dev/linter/rules/no-unnecessary-conditions/) now correctly handles several cases that were previously missed. | ||
|
|
||
| - Optional chaining on a non-nullish type is now flagged based on type information. Example: `function bar(arg: string) { return arg?.length }` now reports an unnecessary `?.`. | ||
| - Nullish coalescing on a non-nullish type is now flagged: `function bar(arg: string) { return arg ?? "default" }` now reports `??` as unnecessary. | ||
| - Logical expressions on member and call access use type information: `interface Foo { items: string[] }; function f(foo: Foo) { return foo.items || [] }` now reports `|| []` as unnecessary. | ||
| - Comparisons between a non-nullish type and `null`/`undefined` are flagged: `function f(x: string) { return x === null }` now reports. | ||
| - The `!expr` form of a condition now detects always-falsy: `const a = []; if (!a) {}` is now reported. | ||
| - `switch` statements are no longer wrongly flagged on the discriminant for literal unions like `'a' | 'b' | 'c'`. Instead, matching the behavior of `@typescript-eslint/no-unnecessary-condition`, individual `case` clauses that are statically unreachable — such as `case 'd':` inside `switch (value: 'a' | 'b' | 'c')` — are now flagged. |
Member
There was a problem hiding this comment.
Changesets aren't places for technical details nor documentation. This is written in our guidelines. Reduce it to a few lines, and instead pour more time into expanding the documentation
Comment on lines
+49
to
+71
| /// ```ts | ||
| /// interface Foo { items: string[] } | ||
| /// function f(x: Foo) { | ||
| /// return x.items || []; // x.items is string[], always truthy | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// ```ts | ||
| /// const a = []; | ||
| /// if (!a) {} // always false — array literals are always truthy | ||
| /// ``` | ||
| /// | ||
| /// ```ts | ||
| /// function f(x: string) { | ||
| /// return x === null; // always false — x is never null | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// ```ts | ||
| /// function f(v: 'a' | 'b') { | ||
| /// switch (v) { case 'c': return 1; } // 'c' is unreachable | ||
| /// } | ||
| /// ``` |
Member
There was a problem hiding this comment.
There's more value in showing incorrect cases than showing valid cases. Still, in this PR there are zero new incorrect cases.
We should:
- reduce the valid blocks (they can all exist in a single block)
- add more invalid cases
| /// switch (v) { | ||
| /// case 'a': break; | ||
| /// case 'b': break; | ||
| /// case 'c': break; // all cases reachable — no flag |
Member
There was a problem hiding this comment.
What's a flag in lint rule? Please reword the comment
Comment on lines
+353
to
+358
| .note(markup! { | ||
| "The discriminant's type can never equal this value." | ||
| }) | ||
| .note(markup! { | ||
| "Remove the case, or change the discriminant." | ||
| }), |
Member
There was a problem hiding this comment.
Please use beginner friendly terminology if possible. This is also covered in the contribution guide
Comment on lines
+379
to
+383
| matches!(expr, AnyJsExpression::JsIdentifierExpression(id) | ||
| if id.name().ok() | ||
| .and_then(|n| n.value_token().ok()) | ||
| .is_some_and(|t| t.text_trimmed() == "undefined")) | ||
| } |
Member
There was a problem hiding this comment.
This can be written in a cleaner way
JsIdentifierExpression::cast_ref(...).is_some_and(...)Plus the bot is right
Comment on lines
+767
to
+768
| Some(CaseLiteral::String(inner_string_text(&token).text().to_string())) | ||
| } |
Member
There was a problem hiding this comment.
Don't allocate string. Use either Text or TokenText
Comment on lines
+782
to
+787
| /// Returns whether the given `ty` could possibly equal the given literal value. | ||
| /// | ||
| /// This is a narrow, conservative predicate — when in doubt (unknown types, | ||
| /// unresolved references, type variables, complex types), it returns `true`. | ||
| /// Used to determine case-clause reachability: if this returns `false`, the | ||
| /// `case` is statically guaranteed to never match. |
Member
There was a problem hiding this comment.
Please reword this comment. There are grammar errors, and it's hard to read. Remove those em dashes please
| } | ||
|
|
||
| fn single_type_could_equal_literal(ty: &Type, literal: &CaseLiteral) -> bool { | ||
| use biome_js_type_info::TypeData; |
| single_type_could_equal_literal(ty, literal) | ||
| } | ||
|
|
||
| fn single_type_could_equal_literal(ty: &Type, literal: &CaseLiteral) -> bool { |
Member
There was a problem hiding this comment.
Please document this function. It accepts the same types as the previous function, and the names are very similar
Comment on lines
+851
to
+854
| /// Checks whether a `case <literal>:` clause is reachable given the type of | ||
| /// the enclosing `switch`'s discriminant. Mirrors the behavior of | ||
| /// `@typescript-eslint/no-unnecessary-condition`, which flags each case that | ||
| /// can never equal the discriminant. |
Member
There was a problem hiding this comment.
We don't care whether a function mirrors eslint, that's granted if we use the same() metadata.
Instead, focus on explaining the business logic
- Trim the changeset to a concise summary per project guidelines. - Rewrite the rustdoc with grouped invalid examples and a short valid section. - Reword diagnostic messages to avoid jargon (drop "discriminant", "Mirrors @typescript-eslint", em-dashes) and clarify advice. - Switch is_undefined_identifier to JsIdentifierExpression::cast_ref and document the syntactic-only matching. - Avoid allocating a String in CaseLiteral::String by storing TokenText. - Document type_could_equal_literal / single_type_could_equal_literal and the per-case reachability check; remove eslint-mirroring wording. - Update test fixtures and snapshots accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/no_unnecessary_conditions.rs`:
- Around line 719-750: The type-aware null/undefined checks should only run for
equality operators — add a guard so this block executes only when the binary
operator is one of ==, ===, !=, !==; return None otherwise. Additionally, for
strict equality (=== and !==) use stricter narrowing instead of
conditional.is_nullish() (e.g., conditional.is_null() or
conditional.is_undefined() as appropriate) while keeping the existing
conditional.is_non_nullish() logic for the non-strict cases; update the logic
around typed_side, ty, and conditional (and the branches using
conditional.is_non_nullish() / conditional.is_nullish()) accordingly so you
don't apply these checks to relational operators like < or >.
🪄 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: 281aa05a-5b6c-40a6-b725-b83506d9d9c5
⛔ Files ignored due to path filters (3)
crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
.changeset/fix-no-unnecessary-conditions-6611.mdcrates/biome_js_analyze/src/lint/nursery/no_unnecessary_conditions.rscrates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.jscrates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.tscrates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.jscrates/biome_js_type_info/tests/conditionals.rs
✅ Files skipped from review due to trivial changes (2)
- crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/invalid.js
- crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/valid.js
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_js_type_info/tests/conditionals.rs
- Gate type-aware null/undefined comparison to equality operators only, so `<`/`>` against null/undefined never trigger the rule. - Restrict the dual `is_nullish` branch to loose equality, since strict equality distinguishes null from undefined and `is_nullish` conflates them. - Reword comments to drop em-dashes and the "flag" terminology. - Merge valid rustdoc examples into a single code block. - Trim the changeset to one sentence per the contributing guide.
Author
|
Tried to address your comments. |
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
3 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 #6611.
Expands
noUnnecessaryConditionsdetection:?.) and nullish coalescing (??) on non-nullish types.null/undefinedcomparisons with non-nullish types.!expr).caseclauses inswitchon literal unions (replacing the prior incorrect discriminant-wide flag).Test plan
cargo test -p biome_js_analyze(2377 spec + 10 doctests)cargo test -p biome_js_type_info --test conditionalscargo clippy -p biome_js_analyze --all-targets -- -D warningscargo fmt --all -- --check