feat(useDestructuring): add options for assignment/declaration and improve diagnostic for bare object assignments#9786
Conversation
…rove diagnostic for bare object assignments
🦋 Changeset detectedLatest commit: 1562508 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 |
|
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 a Biome changeset and extends the 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 |
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)
86-96: Minor redundancy:is_assignmentfield discarded and recreated.
should_suggest_destructuringreturnsUseDestructuringState::Object { is_assignment: false }(lines 213, 231), but the caller immediately discards it and creates a new variant with the correctis_assignmentvalue. Consider havingshould_suggest_destructuringreturn a simpler enum without the field, or accept the assignment context as a parameter.♻️ Optional: Simplify by removing placeholder field
One approach is to have an internal enum for the helper:
enum DestructuringKind { Array, Object, } fn should_suggest_destructuring(left: &str, right: &AnyJsExpression) -> Option<DestructuringKind> { // ... return DestructuringKind::Object or DestructuringKind::Array }Then the caller maps it to
UseDestructuringStatewith the correctis_assignmentvalue.Also applies to: 126-136
🤖 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 86 - 96, The code currently returns UseDestructuringState::Object with a placeholder is_assignment value from should_suggest_destructuring which the caller immediately discards and recreates; change should_suggest_destructuring to return a simpler enum (e.g., DestructuringKind with variants Array and Object) or an Option<DestructuringKind>, and update the callers that now call should_suggest_destructuring (references: should_suggest_destructuring, UseDestructuringState) to map the returned DestructuringKind into UseDestructuringState::Array or UseDestructuringState::Object { is_assignment: true/false } using the actual assignment context at the call site so the redundant placeholder field is removed.
🤖 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_rule_options/src/use_destructuring.rs`:
- Around line 35-53: The Default impl for DestructuringConfig produces array:
true/object: true while ASSIGNMENT_DEFAULT is array: false/object: false,
creating an intentional but undocumented difference when deserialising `{}` vs
omitting the key; add a focused unit test that deserialises an explicit empty
object into DestructuringConfig and asserts it equals
DestructuringConfig::default() (array/object true) and another that omits the
assignment key and asserts it equals DestructuringConfig::ASSIGNMENT_DEFAULT
(array/object false), and add a short inline comment above the
Default/ASSIGNMENT_DEFAULT declarations explaining the opt-in semantics to make
the distinction explicit (referencing DestructuringConfig, Default,
DECLARATOR_DEFAULT, ASSIGNMENT_DEFAULT).
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs`:
- Around line 86-96: The code currently returns UseDestructuringState::Object
with a placeholder is_assignment value from should_suggest_destructuring which
the caller immediately discards and recreates; change
should_suggest_destructuring to return a simpler enum (e.g., DestructuringKind
with variants Array and Object) or an Option<DestructuringKind>, and update the
callers that now call should_suggest_destructuring (references:
should_suggest_destructuring, UseDestructuringState) to map the returned
DestructuringKind into UseDestructuringState::Array or
UseDestructuringState::Object { is_assignment: true/false } using the actual
assignment context at the call site so the redundant placeholder field is
removed.
🪄 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: 9540ca4a-0faa-4816-9314-b96c817d4b62
⛔ Files ignored due to path filters (5)
crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignment.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/valid.js.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/fix-use-destructuring-illegal-suggestions.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/invalidAssignment.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignment.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useDestructuring/valid.jscrates/biome_rule_options/src/use_destructuring.rs
💤 Files with no reviewable changes (1)
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalid.js
| impl Default for DestructuringConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| array: true, | ||
| object: true, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl DestructuringConfig { | ||
| const DECLARATOR_DEFAULT: Self = Self { | ||
| array: true, | ||
| object: true, | ||
| }; | ||
|
|
||
| const ASSIGNMENT_DEFAULT: Self = Self { | ||
| array: false, | ||
| object: false, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's documentation explaining this behaviour difference
rg -n -C3 "assignmentExpression" --type=mdRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
# First, let's look at the full use_destructuring.rs file to understand the complete context
cat crates/biome_rule_options/src/use_destructuring.rsRepository: biomejs/biome
Length of output: 1806
🏁 Script executed:
# Search for uses of DestructuringConfig in tests
rg -n "DestructuringConfig" --type=rs -A2 -B2Repository: biomejs/biome
Length of output: 83
🏁 Script executed:
# Look for test files related to use_destructuring rule
fd -e rs "use_destructuring" | head -20Repository: biomejs/biome
Length of output: 169
🏁 Script executed:
# Find and check test files for the use_destructuring rule
find crates/biome_js_analyze -name "*use_destructuring*" -type fRepository: biomejs/biome
Length of output: 118
🏁 Script executed:
# Search for test configurations or test data related to this rule
rg -n "assignmentExpression\|variableDeclarator" crates/biome_js_analyze --type=rs -A2 -B2 | head -50Repository: biomejs/biome
Length of output: 83
🏁 Script executed:
# Look for test files in the specs directory
find . -path "*tests/specs*" -name "*destructuring*" -type fRepository: biomejs/biome
Length of output: 2429
🏁 Script executed:
# Let's look at the actual rule implementation
cat crates/biome_js_analyze/src/lint/nursery/use_destructuring.rsRepository: biomejs/biome
Length of output: 8597
🏁 Script executed:
# Search for test configuration or usage of this rule in comments/docs
rg -i "use.destructuring" --type=json -B2 -A2 | head -100Repository: biomejs/biome
Length of output: 543
🏁 Script executed:
# Check if there's a test specs directory for this rule
find crates/biome_js_analyze/tests -name "*use_destructuring*" -o -name "*useDestructuring*"Repository: biomejs/biome
Length of output: 117
🏁 Script executed:
# List the test files in the useDestructuring test directory
ls -la crates/biome_js_analyze/tests/specs/nursery/useDestructuring/Repository: biomejs/biome
Length of output: 911
🏁 Script executed:
# Check if there's a test config file that shows expected behaviour
find crates/biome_js_analyze/tests/specs/nursery/useDestructuring -name "*.json" -o -name "jsfmt.json"Repository: biomejs/biome
Length of output: 148
🏁 Script executed:
# Look for any test configuration or README
cat crates/biome_js_analyze/tests/specs/nursery/useDestructuring/jsfmt.json 2>/dev/null || echo "File not found"Repository: biomejs/biome
Length of output: 71
🏁 Script executed:
# Check the test options file
cat crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignment.options.jsonRepository: biomejs/biome
Length of output: 423
🏁 Script executed:
# Check the invalid assignment test case
cat crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignment.jsRepository: biomejs/biome
Length of output: 194
Inconsistency between Default and ASSIGNMENT_DEFAULT is intentional but underdocumented.
The behaviour difference is real: { "assignmentExpression": {} } deserialises to Some(DestructuringConfig { array: true, object: true }) (via Default), whilst omitting the key entirely returns ASSIGNMENT_DEFAULT (both false). This appears to be a deliberate opt-in pattern—when a user explicitly provides an empty object, they get permissive defaults; when they omit it entirely, the rule defaults to conservative.
However, this design choice isn't tested (no test case for empty {}) or documented. Consider adding a test case demonstrating this behaviour, or clarifying the semantics in a comment if this distinction is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_rule_options/src/use_destructuring.rs` around lines 35 - 53, The
Default impl for DestructuringConfig produces array: true/object: true while
ASSIGNMENT_DEFAULT is array: false/object: false, creating an intentional but
undocumented difference when deserialising `{}` vs omitting the key; add a
focused unit test that deserialises an explicit empty object into
DestructuringConfig and asserts it equals DestructuringConfig::default()
(array/object true) and another that omits the assignment key and asserts it
equals DestructuringConfig::ASSIGNMENT_DEFAULT (array/object false), and add a
short inline comment above the Default/ASSIGNMENT_DEFAULT declarations
explaining the opt-in semantics to make the distinction explicit (referencing
DestructuringConfig, Default, DECLARATOR_DEFAULT, ASSIGNMENT_DEFAULT).
Merging this PR will not alter performance
Comparing Footnotes
|
…structuring
Both variableDeclarator and assignmentExpression now default to
{array: true, object: true}, matching ESLint's prefer-destructuring
defaults. Unified the two separate default constants into a single
const fn new() to eliminate duplication.
| pub array: bool, | ||
| pub object: bool, |
There was a problem hiding this comment.
These should be Option<bool>, it needs to be possible to specify one and not the other.
Address review feedback: changed DestructuringConfig fields from bool to Option<bool> with getter methods, matching the codebase pattern used by NoDoubleEqualsOptions and NoBlankTargetOptions. This preserves user intent during config round-tripping by only serializing explicitly set values.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs (1)
64-96:⚠️ Potential issue | 🟠 MajorFix default configuration for
assignment_expressionto disable checks by default.The implementation currently defaults both
array()andobject()totrueforassignment_expression, but the PR description claims assignments should be disabled by default (following ESLint conventions where declarations are enabled, assignments disabled).Update the
assignment_expression()accessor incrates/biome_rule_options/src/use_destructuring.rsto return a config with different defaults thanvariable_declarator():pub fn assignment_expression(&self) -> &DestructuringConfig { static DEFAULT: DestructuringConfig = DestructuringConfig { array: Some(false), // Disabled by default object: Some(false), // Disabled by default }; self.assignment_expression.as_ref().unwrap_or(&DEFAULT) }Also ensure tests cover the default behavior for both contexts.
🤖 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 64 - 96, The assignment_expression() accessor currently falls back to defaults enabling both array/object but should default both to false; update the assignment_expression(&self) -> &DestructuringConfig in crates/biome_rule_options/src/use_destructuring.rs so its static DEFAULT has array: Some(false) and object: Some(false) (unlike variable_declarator()), and ensure the accessor uses self.assignment_expression.as_ref().unwrap_or(&DEFAULT); then add tests exercising should_suggest_destructuring via UseDestructuringQuery::JsAssignmentExpression to assert assignment checks are disabled by default while variable_declarator() still uses its existing default.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs (1)
213-231: Consider a cleaner intermediate representation.The function returns
is_assignment: falsein both places, but the caller always overwrites this value based on context. This works, but it's a touch confusing sinceis_assignmenthas no meaningful value here.An alternative would be a separate enum without the flag (e.g.,
SuggestionKind::Object | SuggestionKind::Array) that gets wrapped by the caller. That said, this is a minor nitpick – the current approach is functional and the intent is clear from the 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_destructuring.rs` around lines 213 - 231, The code returns UseDestructuringState::Object { is_assignment: false } in multiple branches even though the caller immediately overwrites is_assignment; introduce a simpler intermediate enum (e.g., SuggestionKind with variants Object and Array) returned from the helper logic (the match arms including the AnyJsExpression::JsStaticMemberExpression branch and the earlier object branch), and then have the caller convert/wrap SuggestionKind into UseDestructuringState by attaching the correct is_assignment flag; update the helper function return type and all places referencing UseDestructuringState in that helper so the flag is set only at the call site.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs`:
- Around line 64-96: The assignment_expression() accessor currently falls back
to defaults enabling both array/object but should default both to false; update
the assignment_expression(&self) -> &DestructuringConfig in
crates/biome_rule_options/src/use_destructuring.rs so its static DEFAULT has
array: Some(false) and object: Some(false) (unlike variable_declarator()), and
ensure the accessor uses
self.assignment_expression.as_ref().unwrap_or(&DEFAULT); then add tests
exercising should_suggest_destructuring via
UseDestructuringQuery::JsAssignmentExpression to assert assignment checks are
disabled by default while variable_declarator() still uses its existing default.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs`:
- Around line 213-231: The code returns UseDestructuringState::Object {
is_assignment: false } in multiple branches even though the caller immediately
overwrites is_assignment; introduce a simpler intermediate enum (e.g.,
SuggestionKind with variants Object and Array) returned from the helper logic
(the match arms including the AnyJsExpression::JsStaticMemberExpression branch
and the earlier object branch), and then have the caller convert/wrap
SuggestionKind into UseDestructuringState by attaching the correct is_assignment
flag; update the helper function return type and all places referencing
UseDestructuringState in that helper so the flag is set only at the call site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ada463b-404c-45a6-8d4a-bb451dd2b355
📒 Files selected for processing (3)
.changeset/fix-use-destructuring-illegal-suggestions.mdcrates/biome_js_analyze/src/lint/nursery/use_destructuring.rscrates/biome_rule_options/src/use_destructuring.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/fix-use-destructuring-illegal-suggestions.md
- crates/biome_rule_options/src/use_destructuring.rs
|
Re CodeRabbit "major" warning: It was based on the old PR description. Both variableDeclarator and assignmentExpression now default to {array: true, object: true}, matching ESLint's prefer-destructuring defaults. The code and description are aligned. Re @dyc3 's review: Updated DestructuringConfig fields to Option with getter methods, following the NoDoubleEqualsOptions / NoBlankTargetOptions pattern. Only explicitly set values are serialized back, preserving user intent. |
dyc3
left a comment
There was a problem hiding this comment.
we are missing tests for the options
- Changed DestructuringConfig fields to Option<bool> with Copy derive and getter methods following the NoDoubleEqualsOptions pattern - Removed getter methods from UseDestructuringOptions; callers use field access with unwrap_or_default() directly - Reverted assignment cases from invalid.js - Added comprehensive option tests: default behavior, partial config, per-context control, mixed contexts, explicit defaults, empty options
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs (1)
213-213: Minor: Placeholderis_assignmentvalue is overridden by caller.
should_suggest_destructuringreturnsis_assignment: falseas a placeholder, which the caller inrun()immediately overwrites with the correct value. This works but could be slightly cleaner—consider returning a simpler enum variant without the field, or documenting why the placeholder exists.Not blocking; the current approach is readable and functional.
Also applies to: 231-231
🤖 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` at line 213, should_suggest_destructuring currently returns UseDestructuringState::Object { is_assignment: false } as a placeholder which is immediately overwritten in run(); remove the redundant placeholder by changing the enum variant to a fieldless UseDestructuringState::Object (or add a distinct variant like ObjectPending) and update should_suggest_destructuring to return that simpler variant, then update run() (and any other matches) to construct the final Object { is_assignment: ... } only where the actual assignment context is known (adjust pattern matches on UseDestructuringState::Object accordingly).crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidPartialOptions.js (1)
1-2: Comment slightly misleading.Line 1 says "should generate diagnostics" but only lines 7 and 13 (object cases) should trigger diagnostics—lines 4 and 10 (array cases) are expected to be silent. Consider adjusting to:
-/* should generate diagnostics */ +/* should generate diagnostics for object cases only */ /* array disabled, but object should still fire */🤖 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/useDestructuring/invalidPartialOptions.js` around lines 1 - 2, Update the top-of-file comment to accurately reflect which cases should produce diagnostics: change "should generate diagnostics" to something like "should generate diagnostics for object cases only; array cases are disabled" so the comment matches the behavior described by the existing notes ("array disabled, but object should still fire") in this useDestructuring invalidPartialOptions test.
🤖 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_destructuring.rs`:
- Line 213: should_suggest_destructuring currently returns
UseDestructuringState::Object { is_assignment: false } as a placeholder which is
immediately overwritten in run(); remove the redundant placeholder by changing
the enum variant to a fieldless UseDestructuringState::Object (or add a distinct
variant like ObjectPending) and update should_suggest_destructuring to return
that simpler variant, then update run() (and any other matches) to construct the
final Object { is_assignment: ... } only where the actual assignment context is
known (adjust pattern matches on UseDestructuringState::Object accordingly).
In
`@crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidPartialOptions.js`:
- Around line 1-2: Update the top-of-file comment to accurately reflect which
cases should produce diagnostics: change "should generate diagnostics" to
something like "should generate diagnostics for object cases only; array cases
are disabled" so the comment matches the behavior described by the existing
notes ("array disabled, but object should still fire") in this useDestructuring
invalidPartialOptions test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a4188f0-637e-4e16-970c-8af194ac5afa
⛔ Files ignored due to path filters (13)
crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidArrayOnly.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignment.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignmentOnly.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidDeclaratorOnly.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidEmptyOptions.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidExplicitDefaults.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidMixedContexts.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidPartialOptions.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/validArrayOnly.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/validDisabledAssignment.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useDestructuring/validObjectOnly.js.snapis excluded by!**/*.snapand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (24)
crates/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/invalidArrayOnly.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidArrayOnly.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignment.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignmentOnly.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignmentOnly.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidDeclaratorOnly.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidDeclaratorOnly.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidEmptyOptions.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidEmptyOptions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidExplicitDefaults.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidExplicitDefaults.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidMixedContexts.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidMixedContexts.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidPartialOptions.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidPartialOptions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useDestructuring/validArrayOnly.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/validArrayOnly.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useDestructuring/validDisabledAssignment.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/validDisabledAssignment.options.jsoncrates/biome_js_analyze/tests/specs/nursery/useDestructuring/validObjectOnly.jscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/validObjectOnly.options.jsoncrates/biome_rule_options/src/use_destructuring.rs
💤 Files with no reviewable changes (1)
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalid.js
✅ Files skipped from review due to trivial changes (19)
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidMixedContexts.options.json
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/validDisabledAssignment.options.json
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidEmptyOptions.options.json
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignment.js
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidArrayOnly.options.json
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/validObjectOnly.js
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignmentOnly.options.json
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/validArrayOnly.js
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidPartialOptions.options.json
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidAssignmentOnly.js
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/validObjectOnly.options.json
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidEmptyOptions.js
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/validDisabledAssignment.js
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidDeclaratorOnly.options.json
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidExplicitDefaults.options.json
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidMixedContexts.js
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidDeclaratorOnly.js
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/validArrayOnly.options.json
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidArrayOnly.js
…tion
Replaced placeholder UseDestructuringState::Object { is_assignment: false }
in should_suggest_destructuring with a simpler DestructuringKind enum.
Callers now map DestructuringKind to UseDestructuringState with the
correct is_assignment value at the call site.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs (1)
67-96:⚠️ Potential issue | 🟠 MajorRestrict assignment expression diagnostics to unused-result contexts.
foo = obj.fooevaluates toobj.foo, but({ foo } = obj)evaluates toobj. The current code flags any matchingJsAssignmentExpressionunconditionally, which will incorrectly suggest rewriting cases likereturn foo = obj.fooorcall(foo = obj.foo)where the refactor changes the return value. Only report when the assignment result is discarded.🤖 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 67 - 96, The rule currently treats any JsAssignmentExpression the same; change UseDestructuringQuery::JsAssignmentExpression handling to only consider assignments whose result is discarded (i.e., the assignment is not used as a value). After obtaining left/right and before calling should_suggest_destructuring, check the assignment expression's parent/context to ensure its value is ignored (for example it is the top-level ExpressionStatement or otherwise not used in a return/call/binary/etc.); only then run should_suggest_destructuring and return UseDestructuringState (Array/Object) — otherwise return None. Reference: UseDestructuringQuery::JsAssignmentExpression, UseDestructuringState, should_suggest_destructuring, and the assignment_expression option.
🤖 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 64-71: Update the rule's rustdoc to add a "## Options" section
documenting the new config surface for both variableDeclarator and
assignmentExpression (include h3 headers for each option), state their default
values (e.g., false/unwrap_or_default behavior), and provide a brief example for
each — especially an assignment example showing how assignmentExpression enables
array/object destructuring fixes; add the same documentation near the
rule/assist docs referenced around the existing comments (also update the doc
blocks covering the code paths handling
UseDestructuringQuery::JsAssignmentExpression and the variableDeclarator branch
so users can find these options).
---
Outside diff comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_destructuring.rs`:
- Around line 67-96: The rule currently treats any JsAssignmentExpression the
same; change UseDestructuringQuery::JsAssignmentExpression handling to only
consider assignments whose result is discarded (i.e., the assignment is not used
as a value). After obtaining left/right and before calling
should_suggest_destructuring, check the assignment expression's parent/context
to ensure its value is ignored (for example it is the top-level
ExpressionStatement or otherwise not used in a return/call/binary/etc.); only
then run should_suggest_destructuring and return UseDestructuringState
(Array/Object) — otherwise return None. Reference:
UseDestructuringQuery::JsAssignmentExpression, UseDestructuringState,
should_suggest_destructuring, and the assignment_expression option.
🪄 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: 1cf42c58-8564-4c1b-b286-e6f0e15d44c0
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidPartialOptions.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (2)
crates/biome_js_analyze/src/lint/nursery/use_destructuring.rscrates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidPartialOptions.js
✅ Files skipped from review due to trivial changes (1)
- crates/biome_js_analyze/tests/specs/nursery/useDestructuring/invalidPartialOptions.js
Only suggest destructuring for assignment expressions when the result
is discarded (parent is ExpressionStatement). Destructuring changes the
evaluation result: `foo = obj.foo` returns `obj.foo`, but
`({ foo } = obj)` returns `obj`. This diverges from ESLint's
prefer-destructuring which has the same defect, so the source
annotation changes from .same() to .inspired().
Also adds Options documentation to the rule's rustdoc.
|
@dyc3 I've updated based on your feedback and added more testing. I have a question about your suggested approach though: is there a benefit to using Here's the changes summary:
|
Align options documentation with codebase patterns (noDoubleEquals, useSelfClosingElements): backtick-wrapped headings, Default line, JSON examples, and code examples per option. Revert invalid.js and its snapshot to match main.
…destructuring-8480 # Conflicts: # packages/@biomejs/backend-jsonrpc/src/workspace.ts
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@biomejs/biome](https://biomejs.dev) ([source](https://github.com/biomejs/biome/tree/HEAD/packages/@biomejs/biome)) | imports | patch | [`2.4.15` -> `2.4.16`](https://renovatebot.com/diffs/npm/@biomejs%2fbiome/2.4.15/2.4.16) | --- ### Release Notes <details> <summary>biomejs/biome (@​biomejs/biome)</summary> ### [`v2.4.16`](https://github.com/biomejs/biome/blob/HEAD/packages/@​biomejs/biome/CHANGELOG.md#2416) [Compare Source](https://github.com/biomejs/biome/compare/@biomejs/biome@2.4.15...@biomejs/biome@2.4.16) ##### Patch Changes - [#​10329](biomejs/biome#10329) [`ef764d5`](biomejs/biome@ef764d5) Thanks [@​Conaclos](https://github.com/Conaclos)! - Fixed an issue where diagnostics showed an incorrect location in Astro files. - [#​10363](biomejs/biome#10363) [`50aa415`](biomejs/biome@50aa415) Thanks [@​dyc3](https://github.com/dyc3)! - Fixed HTML formatting for a case where comments could cause the formatter to split up a closing tag, which would cause the resulting HTML to be syntactically invalid. Input: ```html <span ><!-- 1 --><span>a</span ><!-- 2 --><span>b</span ><!-- 3 --></span> ``` Output: ```diff <span ><!-- 1 - --> <span>a</span<!-- 2 - --> ><span>b</span><!-- 3 + --><span>a</span><!-- 2 + --><span>b</span><!-- 3 --></span > ``` - [#​10465](biomejs/biome#10465) [`0c718da`](biomejs/biome@0c718da) Thanks [@​dfedoryshchev](https://github.com/dfedoryshchev)! - Fixed diagnostics emitted by the `noUntrustedLicenses` rule. - [#​10358](biomejs/biome#10358) [`05c2617`](biomejs/biome@05c2617) Thanks [@​dyc3](https://github.com/dyc3)! - Fixed [#​10356](biomejs/biome#10356): `biome rage --linter` now displays rules enabled through linter domains in the enabled rules list. - [#​10300](biomejs/biome#10300) [`950247c`](biomejs/biome@950247c) Thanks [@​dyc3](https://github.com/dyc3)! - Fixed [#​10265](biomejs/biome#10265): Svelte function bindings such as `bind:value={get, set}` are now parsed more precisely, so [`noCommaOperator`](https://biomejs.dev/linter/rules/no-comma-operator/) won't emit false positives for that syntax anymore. - [#​9786](biomejs/biome#9786) [`e71f584`](biomejs/biome@e71f584) Thanks [@​MeGaNeKoS](https://github.com/MeGaNeKoS)! - Fixed [#​8480](biomejs/biome#8480): [`useDestructuring`](https://biomejs.dev/linter/rules/use-destructuring/) now provides `variableDeclarator` and `assignmentExpression` options to control which contexts enforce destructuring, matching ESLint's `prefer-destructuring` configuration. Both default to `{array: true, object: true}`. The diagnostic for object destructuring in assignment expressions now instructs users to wrap the assignment in parentheses. - [#​10425](biomejs/biome#10425) [`1948b72`](biomejs/biome@1948b72) Thanks [@​sjh9714](https://github.com/sjh9714)! - Fixed [#​10244](biomejs/biome#10244): The `useOptionalChain` rule now detects negated guard inequality chains like `!foo || foo.bar !== "x"`. - [#​10442](biomejs/biome#10442) [`001f94f`](biomejs/biome@001f94f) Thanks [@​ematipico](https://github.com/ematipico)! - Fixed [#​10411](biomejs/biome#10411): [`noMisusedPromises`](https://biomejs.dev/linter/rules/no-misused-promises/) no longer causes a stack overflow when a nested function returns an object with shorthand properties that shadow destructured variables from an outer scope. - [#​10318](biomejs/biome#10318) [`9b1577f`](biomejs/biome@9b1577f) Thanks [@​dyc3](https://github.com/dyc3)! - Added support for `formatter.trailingCommas` in overrides. This option was previously available in the top-level formatter configuration but missing from formatter overrides. - [#​10319](biomejs/biome#10319) [`2e37709`](biomejs/biome@2e37709) Thanks [@​dyc3](https://github.com/dyc3)! - Fixed Vue and Svelte formatting for standalone interpolations in inline elements. Biome now preserves existing newlines in cases like: ```diff - <span> {{ value }} </span> + <span> + {{ value }} + </span> ``` - [#​10365](biomejs/biome#10365) [`0a58eb0`](biomejs/biome@0a58eb0) Thanks [@​Netail](https://github.com/Netail)! - Fixed [#​10361](biomejs/biome#10361): [`noUnusedFunctionParameters`](https://biomejs.dev/linter/rules/no-unused-function-parameters/) now mentions the parameter name in the diagnostic. - [#​10439](biomejs/biome#10439) [`df6b867`](biomejs/biome@df6b867) Thanks [@​denbezrukov](https://github.com/denbezrukov)! - Fixed CSS and SCSS formatting for comments around declaration colons so comments between property names, colons, and values stay at the same boundary as Prettier. ```diff .selector { - color: /* red, */ - blue; + color: /* red, */ blue; } ``` - [#​10344](biomejs/biome#10344) [`b30208c`](biomejs/biome@b30208c) Thanks [@​siketyan](https://github.com/siketyan)! - Fixed [`#10123`](biomejs/biome#10123): Corrected the [`noReactNativeDeepImports`](https://biomejs.dev/linter/rules/no-react-native-deep-imports/) source rule to point to the proper upstream rule, so users can migrate from the original rule correctly. - [#​10328](biomejs/biome#10328) [`b59133f`](biomejs/biome@b59133f) Thanks [@​dyc3](https://github.com/dyc3)! - Fixed [#​10309](biomejs/biome#10309): Biome no longer adds newlines to Astro frontmatter when linter or assist `--write` mode is enabled. </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xOTUuMiIsInVwZGF0ZWRJblZlciI6IjQzLjE5NS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Reviewed-on: https://git.oirnoir.dev/OIRNOIR/YouTube-Helper-Client/pulls/5
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@biomejs/biome](https://biomejs.dev) ([source](https://github.com/biomejs/biome/tree/HEAD/packages/@biomejs/biome)) | imports | patch | [`2.4.15` -> `2.4.16`](https://renovatebot.com/diffs/npm/@biomejs%2fbiome/2.4.15/2.4.16) | --- ### Release Notes <details> <summary>biomejs/biome (@​biomejs/biome)</summary> ### [`v2.4.16`](https://github.com/biomejs/biome/blob/HEAD/packages/@​biomejs/biome/CHANGELOG.md#2416) [Compare Source](https://github.com/biomejs/biome/compare/@biomejs/biome@2.4.15...@biomejs/biome@2.4.16) ##### Patch Changes - [#​10329](biomejs/biome#10329) [`ef764d5`](biomejs/biome@ef764d5) Thanks [@​Conaclos](https://github.com/Conaclos)! - Fixed an issue where diagnostics showed an incorrect location in Astro files. - [#​10363](biomejs/biome#10363) [`50aa415`](biomejs/biome@50aa415) Thanks [@​dyc3](https://github.com/dyc3)! - Fixed HTML formatting for a case where comments could cause the formatter to split up a closing tag, which would cause the resulting HTML to be syntactically invalid. Input: ```html <span ><!-- 1 --><span>a</span ><!-- 2 --><span>b</span ><!-- 3 --></span> ``` Output: ```diff <span ><!-- 1 - --> <span>a</span<!-- 2 - --> ><span>b</span><!-- 3 + --><span>a</span><!-- 2 + --><span>b</span><!-- 3 --></span > ``` - [#​10465](biomejs/biome#10465) [`0c718da`](biomejs/biome@0c718da) Thanks [@​dfedoryshchev](https://github.com/dfedoryshchev)! - Fixed diagnostics emitted by the `noUntrustedLicenses` rule. - [#​10358](biomejs/biome#10358) [`05c2617`](biomejs/biome@05c2617) Thanks [@​dyc3](https://github.com/dyc3)! - Fixed [#​10356](biomejs/biome#10356): `biome rage --linter` now displays rules enabled through linter domains in the enabled rules list. - [#​10300](biomejs/biome#10300) [`950247c`](biomejs/biome@950247c) Thanks [@​dyc3](https://github.com/dyc3)! - Fixed [#​10265](biomejs/biome#10265): Svelte function bindings such as `bind:value={get, set}` are now parsed more precisely, so [`noCommaOperator`](https://biomejs.dev/linter/rules/no-comma-operator/) won't emit false positives for that syntax anymore. - [#​9786](biomejs/biome#9786) [`e71f584`](biomejs/biome@e71f584) Thanks [@​MeGaNeKoS](https://github.com/MeGaNeKoS)! - Fixed [#​8480](biomejs/biome#8480): [`useDestructuring`](https://biomejs.dev/linter/rules/use-destructuring/) now provides `variableDeclarator` and `assignmentExpression` options to control which contexts enforce destructuring, matching ESLint's `prefer-destructuring` configuration. Both default to `{array: true, object: true}`. The diagnostic for object destructuring in assignment expressions now instructs users to wrap the assignment in parentheses. - [#​10425](biomejs/biome#10425) [`1948b72`](biomejs/biome@1948b72) Thanks [@​sjh9714](https://github.com/sjh9714)! - Fixed [#​10244](biomejs/biome#10244): The `useOptionalChain` rule now detects negated guard inequality chains like `!foo || foo.bar !== "x"`. - [#​10442](biomejs/biome#10442) [`001f94f`](biomejs/biome@001f94f) Thanks [@​ematipico](https://github.com/ematipico)! - Fixed [#​10411](biomejs/biome#10411): [`noMisusedPromises`](https://biomejs.dev/linter/rules/no-misused-promises/) no longer causes a stack overflow when a nested function returns an object with shorthand properties that shadow destructured variables from an outer scope. - [#​10318](biomejs/biome#10318) [`9b1577f`](biomejs/biome@9b1577f) Thanks [@​dyc3](https://github.com/dyc3)! - Added support for `formatter.trailingCommas` in overrides. This option was previously available in the top-level formatter configuration but missing from formatter overrides. - [#​10319](biomejs/biome#10319) [`2e37709`](biomejs/biome@2e37709) Thanks [@​dyc3](https://github.com/dyc3)! - Fixed Vue and Svelte formatting for standalone interpolations in inline elements. Biome now preserves existing newlines in cases like: ```diff - <span> {{ value }} </span> + <span> + {{ value }} + </span> ``` - [#​10365](biomejs/biome#10365) [`0a58eb0`](biomejs/biome@0a58eb0) Thanks [@​Netail](https://github.com/Netail)! - Fixed [#​10361](biomejs/biome#10361): [`noUnusedFunctionParameters`](https://biomejs.dev/linter/rules/no-unused-function-parameters/) now mentions the parameter name in the diagnostic. - [#​10439](biomejs/biome#10439) [`df6b867`](biomejs/biome@df6b867) Thanks [@​denbezrukov](https://github.com/denbezrukov)! - Fixed CSS and SCSS formatting for comments around declaration colons so comments between property names, colons, and values stay at the same boundary as Prettier. ```diff .selector { - color: /* red, */ - blue; + color: /* red, */ blue; } ``` - [#​10344](biomejs/biome#10344) [`b30208c`](biomejs/biome@b30208c) Thanks [@​siketyan](https://github.com/siketyan)! - Fixed [`#10123`](biomejs/biome#10123): Corrected the [`noReactNativeDeepImports`](https://biomejs.dev/linter/rules/no-react-native-deep-imports/) source rule to point to the proper upstream rule, so users can migrate from the original rule correctly. - [#​10328](biomejs/biome#10328) [`b59133f`](biomejs/biome@b59133f) Thanks [@​dyc3](https://github.com/dyc3)! - Fixed [#​10309](biomejs/biome#10309): Biome no longer adds newlines to Astro frontmatter when linter or assist `--write` mode is enabled. </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xOTUuMiIsInVwZGF0ZWRJblZlciI6IjQzLjE5NS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Reviewed-on: https://git.oirnoir.dev/OIRNOIR/YouTube-Helper-Server/pulls/13
Summary
Fixed #8480:
useDestructuringsuggests illegal destructuring operations.Problem
The rule flagged bare object assignment expressions like
foo = object.fooand suggested destructuring, but the naive conversion{ foo } = objectis a syntax error in JavaScript because{at the start of a statement is parsed as a block statement. The correct syntax requires wrapping in parentheses:({ foo } = object).Array destructuring assignments (
[foo] = arrayfromfoo = array[0]) do not have this problem because[cannot start a block statement.Approach
ESLint's
prefer-destructuringsolves this by providing separate configuration for declarations (var/let/const) and assignments. This PR aligns with that approach:variableDeclaratorandassignmentExpressionoptions, each with optionalarrayandobjectbooleans (Option<bool>).{array: true, object: true}, matching ESLint'sprefer-destructuringdefaults.DestructuringConfigfields useOption<bool>with getter methods following the codebase pattern (e.g.,NoDoubleEqualsOptions), preserving user intent during config round-tripping.({ prop } = object).Not addressed
Case 2 from the issue (numeric index on non-array types, e.g.
first[0]wherefirstis{ [x: string]: string }) requires type inference and is out of scope.Test plan
invalid.jsverify diagnostics fire by default for both declaration and assignment contextsvalid.jscovers opted-out cases (optional chaining, private names,using/await using, etc.)AI disclosure
Claude Code was used to understand the problem and expected behavior, research ESLint's
prefer-destructuringapproach/configuration, write unit tests, and run the PR requirement pipeline.