fix(lint): handle namespace declaration merging in noUnusedVariables#9320
Conversation
🦋 Changeset detectedLatest commit: 55b1008 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 detection for TypeScript declaration merging between a namespace and a value declaration (const, function, or class) that share the same name. Introduces private helpers to locate a binding's containing statement list, discover merged declarations, and determine whether a merged counterpart is exported or otherwise used; the noUnusedVariables unused check short‑circuits for such merged pairs. Adds a changeset entry and several tests covering valid and invalid namespace/value merge scenarios. 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)
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
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidNamespaceUnused.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging2.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging3.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
.changeset/fix-namespace-declaration-merging-unused.mdcrates/biome_js_analyze/src/lint/correctness/no_unused_variables.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidNamespaceUnused.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging2.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging3.ts
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
Outdated
Show resolved
Hide resolved
2f0117e to
f2f404f
Compare
|
@ematipico @dyc3 It's a pleasure contributing in this project! |
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/correctness/no_unused_variables.rs`:
- Around line 532-549: The merge-pair logic currently allows any non-namespace
binding to match a namespace (is_namespace && !other_is_namespace) which can
hide unused-namespace diagnostics; define other_is_value analogous to is_value
(i.e., check other_decl for the same explicit value declaration kinds used to
set is_value) and replace the is_merge_pair condition with (is_namespace &&
other_is_value) || (is_value && other_is_namespace), leaving the subsequent
checks (other_list_range, other.name_token, and the export/unused check)
unchanged so only explicit value declarations are considered namespace-merge
candidates.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidNamespaceUnused.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging2.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging3.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
.changeset/fix-namespace-declaration-merging-unused.mdcrates/biome_js_analyze/src/lint/correctness/no_unused_variables.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidNamespaceUnused.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging2.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging3.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging.ts
- crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging3.ts
- crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidNamespaceUnused.ts
- crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMerging2.ts
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidNamespaceUnused.ts
Show resolved
Hide resolved
Merging this PR will not alter performance
Comparing Footnotes
|
|
If we can, lets try to address the perf issue. |
Scope-gate merge candidates: only match bindings whose declarations share the same containing statement list (JsModuleItemList or JsStatementList) to prevent a same-name binding in a nested scope from suppressing a real unused diagnostic. Added regression fixture for shadowed same-name bindings.
…edVariables Replace `model.all_bindings().any(...)` with `model.scope(&statement_list).bindings().any(...)` in `is_declaration_merged_with_used`. This reduces iteration from O(total_bindings) to O(scope_bindings) per check, eliminating the O(N²) behavior on large files. Additional improvements: - Reorder checks: name comparison (cheap) before declaration type (expensive .declaration() call) - Constrain merge-pair candidates: replace `!other_is_namespace` with explicit `other_is_value` to prevent imports/parameters from suppressing namespace diagnostics
Add `/* should generate diagnostics */` to ensure the test framework asserts that diagnostics are actually produced. Update snapshot to reflect the new line numbers.
5160ac7 to
3976c5b
Compare
…used Move `is_declaration_merged_with_used` to run only after the reference check determines a binding appears unused. Most bindings have external references and now skip the merge check entirely, avoiding the cost of `declaration()`, `containing_statement_list()`, `scope()`, and `scope.bindings()` iteration for the common case.
ematipico
left a comment
There was a problem hiding this comment.
There's some opportunity to make the code a bit more readable, can you please address that ?
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
Outdated
Show resolved
Hide resolved
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/correctness/no_unused_variables.rs`:
- Around line 493-500: The function is_namespace_merge_value_declaration
currently checks JsVariableDeclarator, JsFunctionDeclaration, and
JsClassDeclaration but misses TypeScript enums; update
is_namespace_merge_value_declaration to also treat TsEnumDeclaration as a value
declaration so enum+namespace merges (e.g., export enum Foo {} with namespace
Foo {}) are recognized as valid merges and not flagged as unused—add
AnyJsBindingDeclaration::TsEnumDeclaration(_) to the matches arm for
is_namespace_merge_value_declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6873cdaf-a0b7-4ad7-a4a9-344cedab2fc3
📒 Files selected for processing (1)
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
Summary
Fixes #7664.
TypeScript allows declaration merging between namespaces and value declarations (
const,function,class) of the same name. Before this fix,noUnusedVariablesflagged both sides of a merged declaration as unused even when one was exported or referenced, because Biome's semantic model treats each binding independently.This PR adds
is_declaration_merged_with_used()tois_unused(). When a namespace binding appears unused, it checks whether a non-namespace binding with the same name exists and is exported or has external references. The reverse is also handled: when a value binding appears unused, it checks whether a namespace with the same name is exported. This avoids infinite recursion by only checkingis_exported(notis_unused) in the value→namespace direction.Covered patterns
All three patterns are now correctly identified as used through declaration merging.
AI Disclosure
Test Plan
validNamespaceDeclarationMerging.ts— const + namespace + export default (no diagnostics)validNamespaceDeclarationMerging2.ts— export function + namespace (no diagnostics)validNamespaceDeclarationMerging3.ts— function + namespace + export default (no diagnostics)invalidNamespaceUnused.ts— standalone namespace and both-unused const+namespace (diagnostics emitted correctly)noUnusedVariablestests passcargo clippy -p biome_js_analyze -- -D warningspasses cleanrustfmt --edition 2024 --checkpasses cleanDocumentation
Rule documentation updated inline (valid example added to
declare_lint_rule!doc comment).