fix(noUnusedVariables): ignore interfaces/namespaces in script files #8506
Conversation
🦋 Changeset detectedLatest commit: 90c0133 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 |
WalkthroughAdds an early-exit to the noUnusedVariables lint rule so top-level TypeScript interface and namespace declarations in script files (files with no top-level imports/exports and package type not Module) are not reported as unused. Introduces a private helper to detect top-level TsInterfaceDeclaration/TsModuleDeclaration within the top-level module item list, gates the exemption by package.json module type, updates imports for required AST node types, and adds tests (validScriptGlobals.ts and invalidScriptGlobals.ts). Adjusts quick_test layout and test setup to exercise the change. Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
🧠 Learnings (32)📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-11-24T18:06:12.048ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-21T21:15:03.796ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-11-24T18:05:42.356ZApplied to files:
📚 Learning: 2025-12-22T09:26:56.943ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-11-24T18:05:42.356ZApplied to files:
📚 Learning: 2025-11-24T18:05:42.356ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
🔇 Additional comments (7)
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/tests/specs/correctness/noUnusedVariables/validScriptGlobals.ts (1)
1-8: Consider adding a test case with mixed declarations.The test validates that interfaces and namespaces aren't flagged, but doesn't verify that regular unused variables in the same script file are still correctly reported. Adding a case like:
const unused = 1; // should still be flagged interface Foo {} // should not be flaggedwould ensure the fix is properly scoped.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validScriptGlobals.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/wet-memes-shop.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs(3 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validScriptGlobals.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Write changesets that are concise (1-3 sentences), user-focused, use past tense for actions taken and present tense for Biome behavior, include code examples for rules, and end sentences with periods
Files:
.changeset/wet-memes-shop.md
crates/**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update inline rustdoc documentation for rules, assists, and their options when adding new features or changing existing features in Rust crates
Files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
🧠 Learnings (23)
📓 Common learnings
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noUnused` prefix for rules that report unused entities (e.g., `noUnusedVariables`)
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Check if a variable is global before banning it to avoid false positives when the variable is redeclared in local scope; use the semantic model to verify global scope
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noUndeclared` prefix for rules that report undefined entities (e.g., `noUndeclaredVariables`)
📚 Learning: 2025-12-12T10:11:05.564Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-12T10:11:05.564Z
Learning: Create a changeset for user-facing bug fixes and features using `just new-changeset`, selecting appropriate packages and change type (`major`, `minor`, or `patch`)
Applied to files:
.changeset/wet-memes-shop.md
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Commit rule changes with message format: `feat(biome_<crate>): <ruleName>` to follow Biome's conventional commit style
Applied to files:
.changeset/wet-memes-shop.md
📚 Learning: 2025-08-05T14:43:29.581Z
Learnt from: dyc3
Repo: biomejs/biome PR: 7081
File: packages/@biomejs/biome/configuration_schema.json:7765-7781
Timestamp: 2025-08-05T14:43:29.581Z
Learning: The file `packages/biomejs/biome/configuration_schema.json` is auto-generated and should not be manually edited or reviewed for schema issues; any changes should be made at the code generation source.
Applied to files:
.changeset/wet-memes-shop.md
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noUnused` prefix for rules that report unused entities (e.g., `noUnusedVariables`)
Applied to files:
.changeset/wet-memes-shop.mdcrates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noUndeclared` prefix for rules that report undefined entities (e.g., `noUndeclaredVariables`)
Applied to files:
.changeset/wet-memes-shop.mdcrates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Check if a variable is global before banning it to avoid false positives when the variable is redeclared in local scope; use the semantic model to verify global scope
Applied to files:
.changeset/wet-memes-shop.mdcrates/biome_js_analyze/src/lint/correctness/no_unused_variables.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validScriptGlobals.ts
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noUseless` prefix for rules that report unnecessary code that could be removed or simplified (e.g., `noUselessConstructor`)
Applied to files:
.changeset/wet-memes-shop.mdcrates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noRestricted` prefix for rules that report user-banned entities (e.g., `noRestrictedGlobals`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noRedundant` prefix for rules that report redundant code (e.g., `noRedundantUseStrict`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noUnknown` prefix for rules that report mistyped entities in CSS (e.g., `noUnknownUnit`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Framework-specific rules should be named using the `use` or `no` prefix followed by the framework name (e.g., `noVueReservedProps`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noEmpty` prefix for rules that report empty code (e.g., `noEmptyBlockStatements`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `noInvalid` prefix for rules that report runtime errors from mistyping (e.g., `noInvalidConstructorSuper`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Use `ConditionalParsedSyntax` for syntax that is only valid in specific contexts (e.g., strict mode, file types, language versions) and call `or_invalid_to_bogus()` to convert to a bogus node if not supported
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/local_inference.rs : Implement local inference in dedicated modules to derive type definitions from expressions without context of surrounding scopes
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/js_module_info/collector.rs : Implement module-level (thin) inference to resolve `TypeReference::Qualifier` variants by looking up declarations in module scopes and handling import statements
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use `Semantic<T>` query type instead of `Ast<T>` when a rule needs to access the semantic model for binding references and scope information
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:27.810Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:27.810Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Import the `FormatNode` trait and implement it for your Node when creating formatters in biome_js_formatter
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:27.810Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:27.810Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : When formatting AST nodes, use mandatory tokens from the AST instead of hardcoding token strings (e.g., use `node.l_paren_token().format()` instead of `token("(")`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Use `TypeReference` variants (`Qualifier`, `Resolved`, `Import`, `Unknown`) to represent different phases of type resolution
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/scoped_resolver.rs : Implement full inference to resolve `TypeReference::Import` variants across the entire module graph
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `declare_node_union!` macro to query multiple node types at once by joining them into an enum with `Any*Like` naming convention
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
🔇 Additional comments (3)
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs (2)
309-312: Short-circuit placement looks correct.The exemption is properly integrated early in the run flow, returning
Noneto indicate no violation. This placement is appropriate, assuming theis_script_declarationlogic is corrected.However, this depends on fixing the critical bug in
is_script_declaration. Once corrected, verify with a test that includes both unused variables and interfaces/namespaces in the same script file.
11-14: Imports look correct.All necessary types are imported to support the script detection logic.
.changeset/wet-memes-shop.md (1)
1-5: Well-written changeset.Follows all guidelines: concise, user-focused, uses present tense, includes a code example, references the issue, and ends with a full stop. As per coding guidelines for changesets.
| /// Returns `true` if the file is considered a script | ||
| /// and is an interface or namespace | ||
| fn is_script_declaration(binding: &AnyJsIdentifierBinding) -> bool { | ||
| binding | ||
| .syntax() | ||
| .ancestors() | ||
| .find_map(JsModuleItemList::cast) | ||
| .is_some_and(|module_list| { | ||
| // Only check top-level declarations | ||
| let is_top_level = module_list.parent::<JsModule>().is_some(); | ||
|
|
||
| if !is_top_level { | ||
| return false; | ||
| } | ||
|
|
||
| // Presence of imports/exports means its a module | ||
| let has_import_or_export = (&module_list).into_iter().any(|item| { | ||
| let kind = item.syntax().kind(); | ||
| JsImport::can_cast(kind) || JsExport::can_cast(kind) | ||
| }); | ||
|
|
||
| if has_import_or_export { | ||
| return false; | ||
| } | ||
|
|
||
| // We can't determine if an interface/namespace augments a global just from syntax | ||
| // so all of them are treated the same | ||
| module_list.into_iter().any(|item| { | ||
| let kind = item.syntax().kind(); | ||
| TsInterfaceDeclaration::can_cast(kind) || TsModuleDeclaration::can_cast(kind) | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Critical logic error: function exempts all declarations, not just interfaces/namespaces.
The function checks whether the file contains ANY interface or namespace (lines 440-443), but doesn't verify that the current binding itself is from an interface or namespace declaration. This means ALL top-level declarations in a script file that happens to contain at least one interface/namespace would be incorrectly exempted.
For example:
const unused = 1; // Would incorrectly pass is_script_declaration
interface Foo {} // Should passThe unused variable would be exempted because the file contains an interface, even though unused itself isn't an interface or namespace.
🔎 Suggested fix
fn is_script_declaration(binding: &AnyJsIdentifierBinding) -> bool {
+ // First verify this binding is from an interface or namespace
+ let Some(decl) = binding.declaration() else {
+ return false;
+ };
+ let is_interface_or_namespace = matches!(
+ decl,
+ AnyJsBindingDeclaration::TsInterfaceDeclaration(_)
+ | AnyJsBindingDeclaration::TsModuleDeclaration(_)
+ );
+ if !is_interface_or_namespace {
+ return false;
+ }
+
+ // Then check if it's in a script file (no imports/exports)
binding
.syntax()
.ancestors()
.find_map(JsModuleItemList::cast)
.is_some_and(|module_list| {
- // Only check top-level declarations
let is_top_level = module_list.parent::<JsModule>().is_some();
-
if !is_top_level {
return false;
}
-
- // Presence of imports/exports means its a module
- let has_import_or_export = (&module_list).into_iter().any(|item| {
+
+ // If there are imports/exports, it's a module, not a script
+ !module_list.into_iter().any(|item| {
let kind = item.syntax().kind();
JsImport::can_cast(kind) || JsExport::can_cast(kind)
- });
-
- if has_import_or_export {
- return false;
- }
-
- // We can't determine if an interface/namespace augments a global just from syntax
- // so all of them are treated the same
- module_list.into_iter().any(|item| {
- let kind = item.syntax().kind();
- TsInterfaceDeclaration::can_cast(kind) || TsModuleDeclaration::can_cast(kind)
})
})
}🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs around
lines 413-445, the function currently treats any top-level file that contains
some interface or namespace as exempting all declarations; change the logic to
determine whether the specific binding itself is declared by an interface or
namespace. Concretely, instead of scanning the module list for any
interface/module, locate the binding's declaration ancestor (e.g., find the
nearest TsInterfaceDeclaration or TsModuleDeclaration that is an ancestor of the
binding), ensure that declaration is top-level in a script (no imports/exports
in the module), and only return true when the binding is actually part of that
interface/namespace declaration; keep the existing top-level and import/export
checks but apply them to the declaration containing the binding rather than to
the whole file.
There was a problem hiding this comment.
Add a test case for what the bot suggested.
|
I modified the function like coderabbit suggested so now it contains material generated by ai. Just an FYI Also would you like me to add a test case like coderabbit suggested in its first nitpick? IMO I think it's a nice touch |
CodSpeed Performance ReportMerging #8506 will not alter performanceComparing Summary
Footnotes
|
dyc3
left a comment
There was a problem hiding this comment.
Thanks for the PR! Just a couple notes, also note the CI
|
|
||
| /// Returns `true` if the file is considered a script | ||
| /// and is an interface or namespace | ||
| fn is_script_declaration(binding: &AnyJsIdentifierBinding) -> bool { |
There was a problem hiding this comment.
I'm pretty sure we should be determining this from JsFileSource
|
|
||
| namespace M { | ||
| export const n = 1; | ||
| } No newline at end of file |
There was a problem hiding this comment.
I would leave a brief comment to explain why this is a valid case, because this issue is a bit convoluted.
| /// Returns `true` if the file is considered a script | ||
| /// and is an interface or namespace | ||
| fn is_script_declaration(binding: &AnyJsIdentifierBinding) -> bool { | ||
| binding | ||
| .syntax() | ||
| .ancestors() | ||
| .find_map(JsModuleItemList::cast) | ||
| .is_some_and(|module_list| { | ||
| // Only check top-level declarations | ||
| let is_top_level = module_list.parent::<JsModule>().is_some(); | ||
|
|
||
| if !is_top_level { | ||
| return false; | ||
| } | ||
|
|
||
| // Presence of imports/exports means its a module | ||
| let has_import_or_export = (&module_list).into_iter().any(|item| { | ||
| let kind = item.syntax().kind(); | ||
| JsImport::can_cast(kind) || JsExport::can_cast(kind) | ||
| }); | ||
|
|
||
| if has_import_or_export { | ||
| return false; | ||
| } | ||
|
|
||
| // We can't determine if an interface/namespace augments a global just from syntax | ||
| // so all of them are treated the same | ||
| module_list.into_iter().any(|item| { | ||
| let kind = item.syntax().kind(); | ||
| TsInterfaceDeclaration::can_cast(kind) || TsModuleDeclaration::can_cast(kind) | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add a test case for what the bot suggested.
|
I checked the behavior of |
|
Yeah you're right, it looks like its only determined by if the file extension is In the meantime, @coderabbitai can you file an issue about possibly auto detecting a |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 44 @dyc3, I've filed the issue about auto-detecting 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validScriptGlobals.ts (1)
1-4: Consider adding an explanatory comment for the interface case as well.The namespace at line 6 has a helpful comment explaining why it's valid. The
Array<T>interface merges with the globalArraytype (declaration merging), which is the core issue from #8331. A brief note here would help future readers understand why this shouldn't trigger a diagnostic.
🧹 Nitpick comments (2)
crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidScriptGlobals.ts (1)
1-5: Add a comment explaining the expected diagnostic behaviour.This test file validates that
const cis flagged as unused whileinterface Iandnamespace Nare exempt (per the new script-file logic). Without a comment, readers might wonder why interface and namespace are in an "invalid" test file when they won't produce diagnostics.Consider adding something like:
+/* Only `const c` should be flagged; interface and namespace are exempt in script files. */ const c = 1;crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs (1)
414-454: Implementation is correct; consider documenting the script-file exemption in the rule's main docs.The logic properly checks that the binding itself is from an interface or namespace before applying the script-file exemption—addressing the earlier critical issue about exempting all declarations.
Per coding guidelines, you may want to add a brief note in the
declare_lint_rule!docs (around lines 65-95 in the "Valid" section) explaining that top-level interfaces and namespaces in script files are not reported, since they may be used for global augmentation.🔎 Example documentation addition
/// ### Valid /// +/// Top-level interfaces and namespaces in script files (files without +/// imports or exports) are not reported, as they may augment global types: +/// +/// ```ts +/// interface Array<T> { +/// customMethod: () => void; +/// } +/// ``` +/// /// ```js /// function foo(b) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidScriptGlobals.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validScriptGlobals.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/wet-memes-shop.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs(3 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidScriptGlobals.ts(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validScriptGlobals.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/wet-memes-shop.md
🧰 Additional context used
📓 Path-based instructions (1)
crates/**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update inline rustdoc documentation for rules, assists, and their options when adding new features or changing existing features in Rust crates
Files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
🧠 Learnings (14)
📚 Learning: 2025-12-19T12:53:30.399Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.399Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Check if a variable is global using the semantic model to avoid false positives
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validScriptGlobals.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidScriptGlobals.ts
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/js_module_info/collector.rs : Implement module-level (thin) inference to resolve `TypeReference::Qualifier` variants by looking up declarations in module scopes and handling import statements
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-12-19T12:53:30.399Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.399Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Set `language` to `jsx`, `ts`, or `tsx` for rules that only apply to specific JavaScript dialects
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Distinguish between `TypeData::Unknown` and `TypeData::UnknownKeyword` to measure inference effectiveness versus explicit user-provided unknown types
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/scoped_resolver.rs : Implement full inference to resolve `TypeReference::Import` variants across the entire module graph
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-12-19T12:53:30.399Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.399Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Lint rules should check syntax according to language specification and emit error diagnostics
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-12-19T12:53:30.399Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.399Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Lint rules should perform static analysis of source code to detect invalid or error-prone patterns and emit diagnostics with proposed fixes
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-12-19T12:53:30.399Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.399Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Specify `fix_kind: FixKind::Unsafe` in `declare_lint_rule!` for unsafe code actions
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Use `ConditionalParsedSyntax` for syntax that is only valid in specific contexts (e.g., strict mode, file types, language versions) and call `or_invalid_to_bogus()` to convert to a bogus node if not supported
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/local_inference.rs : Implement local inference in dedicated modules to derive type definitions from expressions without context of surrounding scopes
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:27.810Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:27.810Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Import the `FormatNode` trait and implement it for your Node when creating formatters in biome_js_formatter
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-12-19T12:53:30.399Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.399Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Use `RuleSource::Eslint(...).same()` when implementing a rule that matches the behavior of an ESLint rule
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:27.810Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:27.810Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : When formatting AST nodes, use mandatory tokens from the AST instead of hardcoding token strings (e.g., use `node.l_paren_token().format()` instead of `token("(")`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Use `TypeReference` variants (`Qualifier`, `Resolved`, `Import`, `Unknown`) to represent different phases of type resolution
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs (1)
crates/biome_analyze/src/lib.rs (1)
is_top_level(776-778)
🔇 Additional comments (2)
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs (2)
10-14: LGTM!The added imports are all utilised by the new
is_script_declarationhelper.
309-313: LGTM!Clean integration. The early exit for script declarations is placed sensibly after the declaration file check and before the underscore prefix logic.
|
There's already infrastructure for that. If we pull the manifest, we can check |
|
Thanks that makes sense. I'm still pretty new to the codebase. Could you point me to where this is exposed to the lint rules if you’d like me to update this PR to use that. |
|
There's a |
|
I kept the imports/exports check in |
Summary
Fixes interfaces and namespaces being incorrectly flagged by the
noUnusedVariablesrule in script files (files without top-level imports or exports).Before (incorrect):
After (correct):
The rule now treats script files differently and does not report unused top-level interface or namespace declarations.
Fixes #8331
Test Plan
Added a new test case (
validScriptGlobals.ts) covering unused interfaces and namespaces in script files.