fix(useDestructuring): exclude plain assignments and non-iterable index types#9457
fix(useDestructuring): exclude plain assignments and non-iterable index types#9457Vadaski wants to merge 4 commits intobiomejs:mainfrom
Conversation
…ex types
- Skip rule for assignment expressions (not variable declarations), since
`foo = obj.foo` cannot be safely refactored to `({ foo } = obj)` for
pre-declared variables.
- Add type-based guard for numeric computed member access: only suggest
array destructuring when the object type is array-like or iterable.
Index-signature objects (e.g. `{[key: string]: string}`) are now
correctly excluded.
Fixes: biomejs#8480
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 54d0d9c 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
WalkthroughThe useDestructuring lint now runs on 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)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs (1)
274-326: Consider handling union and intersection types.The type matching is comprehensive for primitives, but
TsUnionTypeandTsIntersectionTypearen't explicitly handled. For example,string | number[]could contain an iterable variant. The current fallback toNoneis conservative, but you might want to refine this in future.🤖 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_destructuring.rs` around lines 274 - 326, The matcher currently omits AnyTsType::TsUnionType and AnyTsType::TsIntersectionType; update array_destructuring_support_for_type to handle these by iterating their member types (for union: node.types(), for intersection: node.types()), recursively calling array_destructuring_support_for_type (incrementing depth and passing model) and then combining results: return Some(true) if any member yields Some(true), return Some(false) if no true and at least one Some(false), otherwise return None; reuse the existing recursion pattern and depth checks used for TsParenthesizedType/TsTypeOperatorType and calls to array_destructuring_support_for_declaration.
🤖 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/tests/specs/nursery/useDestructuring/valid.ts`:
- Around line 27-35: The test has a type mismatch: variable "thing" is declared
as SomeObj but is assigned someObj.thing (a number). Fix by changing the
declaration of "thing" from type SomeObj to number (i.e., let thing: number;) so
the assignment thing = someObj.thing; is type-correct; update the declaration
near the SomeObj type and the let thing binding in this test.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs`:
- Around line 274-326: The matcher currently omits AnyTsType::TsUnionType and
AnyTsType::TsIntersectionType; update array_destructuring_support_for_type to
handle these by iterating their member types (for union: node.types(), for
intersection: node.types()), recursively calling
array_destructuring_support_for_type (incrementing depth and passing model) and
then combining results: return Some(true) if any member yields Some(true),
return Some(false) if no true and at least one Some(false), otherwise return
None; reuse the existing recursion pattern and depth checks used for
TsParenthesizedType/TsTypeOperatorType and calls to
array_destructuring_support_for_declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 516728d2-d38a-4e4f-b8e2-a1d900488f6a
⛔ Files ignored due to path filters (3)
crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/valid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/valid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (5)
.changeset/fix-use-destructuring-illegal-cases.mdcrates/biome_js_analyze/src/lint/nursery/use_destructuring.rscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalid.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/valid.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/valid.ts
💤 Files with no reviewable changes (1)
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalid.js
'thing' should be declared as 'number' (the type of someObj.thing), not 'SomeObj' (the whole object type). Caught by CodeRabbit review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
This PR introduced some sort of recursion + depth short circuit. This hasn't been tested nor documented (in the code and for our users).
I don' this the depth short circuit is needed. We hardly use it in our lint rules.
| !matches!( | ||
| array_destructuring_support_for_expression(object, model, 0), | ||
| Some(false) | ||
| ) |
There was a problem hiding this comment.
This function serves little to no value. Just use it with the try operator array_destructuring_support_for_expression()? and you're good
| ) | ||
| } | ||
|
|
||
| fn array_destructuring_support_for_expression( |
There was a problem hiding this comment.
Please provide some docstrings that explain the business logic of the function.
| let declaration = model.binding(&reference)?.tree().declaration()?; | ||
| array_destructuring_support_for_declaration(&declaration, model, depth + 1) |
There was a problem hiding this comment.
This is plain recursion, which we try to avoid. You can achieve the same result with a simple loop
| model: &SemanticModel, | ||
| depth: usize, | ||
| ) -> Option<bool> { | ||
| if depth >= 8 { |
There was a problem hiding this comment.
Case in point. What's height and why? Hence the docs
| model: &SemanticModel, | ||
| depth: usize, | ||
| ) -> Option<bool> { | ||
| if depth >= 8 { |
There was a problem hiding this comment.
Magic number that we don't want
| } | ||
|
|
||
| let declaration = model.binding(reference)?.tree().declaration()?; | ||
| return array_destructuring_support_for_declaration(&declaration, model, depth + 1); |
Remove depth parameter and depth>=8 guard from recursive helpers in use_destructuring rule as requested in PR 9457 maintainer feedback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Vadaski
left a comment
Vadaski
left a comment
There was a problem hiding this comment.
Thanks for the feedback, @ematipico! You're right — the depth short-circuit was unnecessary overhead. I've removed all depth parameters and guard checks from the four recursive helper functions (array_destructuring_support_for_expression, array_destructuring_support_for_declaration, array_destructuring_support_for_type_annotation, array_destructuring_support_for_type). All existing tests still pass.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs (1)
229-231:⚠️ Potential issue | 🔴 CriticalCyclic type aliases can blow the stack here.
A chain like
type A = B; type B = A;will ping-pong betweenTsTypeAliasDeclarationandTsReferenceTypeforever. In analyser code, that is a nasty little stack-overflow trap on invalid-but-parseable input. Please add cycle detection or make the walk iterative.Also applies to: 277-296
🤖 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_destructuring.rs` around lines 229 - 231, The recursion from AnyJsBindingDeclaration::TsTypeAliasDeclaration into array_destructuring_support_for_type can infinite-loop on cyclic type aliases (e.g., A -> B -> A); add cycle detection by threading a mutable visited set (e.g., HashSet<Id> or HashSet<String>) through array_destructuring_support_for_type and any helpers that descend into TsReferenceType and other type nodes, checking and early-returning when an alias id/name is already present, and ensure all call sites (including the other spots noted around the 277-296 range) pass this visited set; alternatively convert the recursive walk to an explicit stack-based loop that tracks seen aliases and stops when a cycle is detected.
🤖 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_destructuring.rs`:
- Around line 180-184: The current helper treats None (unknown) as supported;
adjust array_destructuring_support_for_expression to return Some(true) for
proven-iterable, Some(false) for proven-not-iterable, and None for unknown, and
change supports_array_destructuring to only return true when
array_destructuring_support_for_expression(...) is Some(true) (i.e., do not
treat None as safe). Additionally, allow a permissive true only when the
expression's static type is explicitly the `any` type by querying the
SemanticModel (use the existing SemanticModel type-check helper you have) so
unknown expressions like foo.bar[0] or getFoo()[0] don't get suggestions unless
explicitly typed as any. Ensure the same logic is applied to the checks
implemented in the code block handled in lines 187-199.
---
Duplicate comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs`:
- Around line 229-231: The recursion from
AnyJsBindingDeclaration::TsTypeAliasDeclaration into
array_destructuring_support_for_type can infinite-loop on cyclic type aliases
(e.g., A -> B -> A); add cycle detection by threading a mutable visited set
(e.g., HashSet<Id> or HashSet<String>) through
array_destructuring_support_for_type and any helpers that descend into
TsReferenceType and other type nodes, checking and early-returning when an alias
id/name is already present, and ensure all call sites (including the other spots
noted around the 277-296 range) pass this visited set; alternatively convert the
recursive walk to an explicit stack-based loop that tracks seen aliases and
stops when a cycle is detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5cbf1e4-5d87-454b-ace8-fa15bbe75a37
📒 Files selected for processing (1)
crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs
| fn supports_array_destructuring(object: &AnyJsExpression, model: &SemanticModel) -> bool { | ||
| !matches!( | ||
| array_destructuring_support_for_expression(object, model), | ||
| Some(false) | ||
| ) |
There was a problem hiding this comment.
None currently means “unknown”, but it gets treated as “safe”.
Lines 180-184 accept anything except Some(false), while Lines 187-199 only prove iterability for array literals and identifiers. That means cases like foo.bar[0] or getFoo()[0] still receive an array-destructuring suggestion even though their iterability was never verified. Please split “unknown” from “supported”, or only keep the permissive path for an explicit any case.
Also applies to: 187-199
🤖 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_destructuring.rs` around lines
180 - 184, The current helper treats None (unknown) as supported; adjust
array_destructuring_support_for_expression to return Some(true) for
proven-iterable, Some(false) for proven-not-iterable, and None for unknown, and
change supports_array_destructuring to only return true when
array_destructuring_support_for_expression(...) is Some(true) (i.e., do not
treat None as safe). Additionally, allow a permissive true only when the
expression's static type is explicitly the `any` type by querying the
SemanticModel (use the existing SemanticModel type-check helper you have) so
unknown expressions like foo.bar[0] or getFoo()[0] don't get suggestions unless
explicitly typed as any. Ensure the same logic is applied to the checks
implemented in the code block handled in lines 187-199.
…ion on cyclic type aliases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Vadaski
left a comment
Vadaski
left a comment
There was a problem hiding this comment.
Thanks @coderabbitai — you're right, removing the depth guard exposed the stack-overflow risk. Fixed by threading a HashSet<TextRange> (visited set) through all recursive helpers. Cycle detection is inserted at both recursion entry points: TsTypeAliasDeclaration and TsReferenceType. A regression test for type CyclicA = CyclicB; type CyclicB = CyclicA; is also added. Codex review: no findings.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs (1)
181-203: 🛠️ Refactor suggestion | 🟠 MajorDocstrings still missing for helper functions.
Per prior feedback, please add docstrings explaining the business logic — particularly the tri-state return semantics (
Some(true)= iterable,Some(false)= not iterable,None= unknown).🤖 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_destructuring.rs` around lines 181 - 203, Add docstrings to the helper functions to document their business logic and the tri-state return semantics: update supports_array_destructuring and array_destructuring_support_for_expression (and any related helper like array_destructuring_support_for_declaration) with short comments that explain what each function does, what inputs it expects, and that they return Some(true) when the expression is iterable (supports array destructuring), Some(false) when it is known to be not iterable, and None when the iterability is unknown; keep the descriptions brief and mention any notable behavior such as omitting parentheses and following identifier bindings via SemanticModel.
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs (2)
244-245: Catch-all returnsNonefor 31 unhandledAnyJsBindingDeclarationvariants.This is acceptable as a starting point, but worth noting that declarations like
JsCatchDeclaration, function declarations with return types, and import specifiers will fall through to permissive handling.🤖 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_destructuring.rs` around lines 244 - 245, The catch-all arm ("_ => None") in the match over AnyJsBindingDeclaration silently returns None for many unhandled variants; replace the blanket fallback with explicit matches (e.g., handle JsCatchDeclaration, function declarations with return types, import specifiers, etc.) or at minimum enumerate the remaining AnyJsBindingDeclaration variants in explicit arms that return None with a comment/TODO so the behavior is deliberate and discoverable; update the match in the code that matches on AnyJsBindingDeclaration (the block ending with "_ => None") to either implement proper handling for those specific variants or add explicit arms with explanatory comments.
317-318: Union and intersection types fall through toNone.For
TsUnionType, you could check whether all variants are iterable (returningSome(true)) or any is definitively non-iterable (Some(false)). Not critical, but would improve precision.🤖 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_destructuring.rs` around lines 317 - 318, The match currently falls through to None for union/intersection types; update the type-checking function (the match that returns Option<bool> for iterability — the arm ending with `_ => None`) to handle TsUnionType and TsIntersectionType by iterating their variants: map each variant to the same Option<bool> check, then for both unions and intersections return Some(false) if any variant is Some(false), return Some(true) if all variants are Some(true), otherwise return None; reference TsUnionType and TsIntersectionType in the new match arms and reuse the existing per-variant check logic.
🤖 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_destructuring.rs`:
- Around line 181-203: Add docstrings to the helper functions to document their
business logic and the tri-state return semantics: update
supports_array_destructuring and array_destructuring_support_for_expression (and
any related helper like array_destructuring_support_for_declaration) with short
comments that explain what each function does, what inputs it expects, and that
they return Some(true) when the expression is iterable (supports array
destructuring), Some(false) when it is known to be not iterable, and None when
the iterability is unknown; keep the descriptions brief and mention any notable
behavior such as omitting parentheses and following identifier bindings via
SemanticModel.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs`:
- Around line 244-245: The catch-all arm ("_ => None") in the match over
AnyJsBindingDeclaration silently returns None for many unhandled variants;
replace the blanket fallback with explicit matches (e.g., handle
JsCatchDeclaration, function declarations with return types, import specifiers,
etc.) or at minimum enumerate the remaining AnyJsBindingDeclaration variants in
explicit arms that return None with a comment/TODO so the behavior is deliberate
and discoverable; update the match in the code that matches on
AnyJsBindingDeclaration (the block ending with "_ => None") to either implement
proper handling for those specific variants or add explicit arms with
explanatory comments.
- Around line 317-318: The match currently falls through to None for
union/intersection types; update the type-checking function (the match that
returns Option<bool> for iterability — the arm ending with `_ => None`) to
handle TsUnionType and TsIntersectionType by iterating their variants: map each
variant to the same Option<bool> check, then for both unions and intersections
return Some(false) if any variant is Some(false), return Some(true) if all
variants are Some(true), otherwise return None; reference TsUnionType and
TsIntersectionType in the new match arms and reuse the existing per-variant
check logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 066fe9c6-6f2e-4b2d-924c-aafd68de587c
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/useDestructuring/valid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/fix-use-destructuring-cyclic-aliases.mdcrates/biome_js_analyze/src/lint/nursery/use_destructuring.rscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/valid.ts
|
Closing, i'm not satisfied by the bot |
Vadaski
left a comment
Vadaski
left a comment
There was a problem hiding this comment.
Thanks @coderabbitai — added /// docstrings to all five helper functions (supports_array_destructuring, array_destructuring_support_for_expression, array_destructuring_support_for_declaration, array_destructuring_support_for_type_annotation, array_destructuring_support_for_type) documenting each function's purpose and the tri-state return semantics (Some(true) = iterable, Some(false) = non-iterable, None = unknown). Codex review: no findings.
|
Thanks for the transparency, @ematipico. Totally understand — I've been using Biome in my own projects and wanted to give back, but I can see how the back-and-forth with automated tools made this noisy to review. I'll be more mindful of that in future contributions. Appreciate the time you spent looking at it. |
2 participants
Summary
Fixes #8480.
The
useDestructuringrule was incorrectly suggesting illegal destructuring in two cases:Case 1: Plain assignment expressions
thing = other.thingcannot be converted toconst { thing } = other—thingis already declared. The destructuring form({ thing } = other)changes binding semantics and may not be a valid refactor. Fixed by restricting the rule query toJsVariableDeclaratoronly (skippingJsAssignmentExpression).Case 2: Numeric index on index-signature types
Array destructuring was suggested even for index-signature objects that have no iterable protocol. Fixed by resolving the declared type of the object expression and returning
falsefor non-iterable types likeTsObjectType.any-typed expressions are treated conservatively (suggestion still shown).Test Plan
valid.jsandvalid.tstest cases for both regression scenarios.use_destructuringsnapshot tests pass:cargo test -p biome_js_analyze use_destructuring.invalid.js.snapto reflect removed false-positive cases.🤖 Generated with Claude Code