fix(lint): handle merged namespaces in noUnusedVariables#9678
fix(lint): handle merged namespaces in noUnusedVariables#9678raashish1601 wants to merge 1 commit intobiomejs:mainfrom
Conversation
|
WalkthroughThe PR enhances the Possibly related PRs
Suggested labels
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: 2
🤖 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 542-552: The loops that call is_merge_candidate_used (iterating
scope.bindings()) must not return on the first Some(..) because later bindings
may mark the name as used/exported; instead, scan all candidates: if any call
yields Some(true) return Some(true) immediately; otherwise record if any
returned Some(false) and continue; after the loop return Some(false) if a false
was seen, else return None. Apply the same change to the analogous loop in
is_value_merged_with_used_namespace (and to the other affected loop at the
568-577 region) so both functions aggregate results across all merge candidates
rather than depending on the bindings() order.
- Around line 497-505: The helper is missing export-default binding variants
causing false positives; update the is_namespace_merge_value_declaration
function to include the JsFunctionExportDefaultDeclaration,
JsClassExportDefaultDeclaration, and TsDeclareFunctionExportDefaultDeclaration
match arms so those default-export bindings are treated as value declarations,
and add a snapshot test under tests/specs/correctness/no_unused_variables/ that
exercises namespace merge behavior with default exports to prevent regressions.
🪄 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: 5c3ea453-5991-4b6c-b77f-d962d97a58dd
⛔ Files ignored due to path filters (6)
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/validNamespaceDeclarationMergingClass.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMergingEnum.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMergingFunction.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMergingPropertyAccess.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (7)
crates/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/validNamespaceDeclarationMergingClass.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMergingEnum.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMergingFunction.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validNamespaceDeclarationMergingPropertyAccess.ts
| fn is_namespace_merge_value_declaration(decl: &AnyJsBindingDeclaration) -> bool { | ||
| matches!( | ||
| decl, | ||
| AnyJsBindingDeclaration::JsVariableDeclarator(_) | ||
| | AnyJsBindingDeclaration::JsFunctionDeclaration(_) | ||
| | AnyJsBindingDeclaration::JsClassDeclaration(_) | ||
| | AnyJsBindingDeclaration::TsEnumDeclaration(_) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
import re
path = Path("crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs")
text = path.read_text()
helper = re.search(
r"fn is_namespace_merge_value_declaration\(decl: &AnyJsBindingDeclaration\) -> bool \{\n(?P<body>.*?)\n\}",
text,
re.S,
)
body = helper.group("body")
for variant in [
"JsFunctionDeclaration",
"JsFunctionExportDefaultDeclaration",
"JsClassDeclaration",
"JsClassExportDefaultDeclaration",
"TsDeclareFunctionExportDefaultDeclaration",
]:
print(f"{variant}: {'handled in helper' if variant in body else 'MISSING from helper'}")
print("\nDistinct declaration kinds referenced elsewhere in the file:")
for variant in [
"JsFunctionExportDefaultDeclaration",
"JsClassExportDefaultDeclaration",
"TsDeclareFunctionExportDefaultDeclaration",
]:
print(f"{variant}: {'present' if variant in text else 'absent'}")
PYRepository: biomejs/biome
Length of output: 505
Add missing export default variants to namespace merge detection and include a test case.
The is_namespace_merge_value_declaration helper skips three binding kinds: JsFunctionExportDefaultDeclaration, JsClassExportDefaultDeclaration, and TsDeclareFunctionExportDefaultDeclaration. These are referenced elsewhere in the file but not handled here, causing named default exports to fall through to the false-positive path. Add the three variants to the match and include a snapshot test in tests/specs/correctness/no_unused_variables/ to cover default export merging scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs` around
lines 497 - 505, The helper is missing export-default binding variants causing
false positives; update the is_namespace_merge_value_declaration function to
include the JsFunctionExportDefaultDeclaration, JsClassExportDefaultDeclaration,
and TsDeclareFunctionExportDefaultDeclaration match arms so those default-export
bindings are treated as value declarations, and add a snapshot test under
tests/specs/correctness/no_unused_variables/ that exercises namespace merge
behavior with default exports to prevent regressions.
| for scope_binding in scope.bindings() { | ||
| let other = scope_binding.tree(); | ||
| if let Some(is_used) = is_merge_candidate_used( | ||
| model, | ||
| binding, | ||
| &other, | ||
| &name, | ||
| is_namespace_merge_value_declaration, | ||
| ) { | ||
| return Some(is_used); | ||
| } |
There was a problem hiding this comment.
Do not stop at the first merge candidate.
Both loops return the first Some(..) from is_merge_candidate_used, so a later used/exported counterpart is never seen. That makes the result depend on scope.bindings() order rather than the whole merged set.
♻️ Safer scan
for scope_binding in scope.bindings() {
let other = scope_binding.tree();
- if let Some(is_used) = is_merge_candidate_used(
+ if is_merge_candidate_used(
model,
binding,
&other,
&name,
is_namespace_merge_value_declaration,
- ) {
- return Some(is_used);
+ ) == Some(true)
+ {
+ return Some(true);
}
}
Some(false)Apply the same tweak in is_value_merged_with_used_namespace.
Also applies to: 568-577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs` around
lines 542 - 552, The loops that call is_merge_candidate_used (iterating
scope.bindings()) must not return on the first Some(..) because later bindings
may mark the name as used/exported; instead, scan all candidates: if any call
yields Some(true) return Some(true) immediately; otherwise record if any
returned Some(false) and continue; after the loop return Some(false) if a false
was seen, else return None. Apply the same change to the analogous loop in
is_value_merged_with_used_namespace (and to the other affected loop at the
568-577 region) so both functions aggregate results across all merge candidates
rather than depending on the bindings() order.
Merging this PR will not alter performance
Comparing Footnotes
|
Summary
noUnusedVariablesfor TypeScript declaration merging between namespaces and same-name valuesregistry.itemContext
lint/correctness/noUnusedVariablestriggering on namespaces used to define component props #7664mainTesting