feat(lint): add ignore option to no-unused-variables#9397
feat(lint): add ignore option to no-unused-variables#9397dyc3 merged 13 commits intobiomejs:nextfrom
Conversation
🦋 Changeset detectedLatest commit: eeceada The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
A-Linter
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
Needs to go against the next branch
| /// A Regex pattern to ignore matching variables from this rule with. | ||
| #[serde(skip_serializing_if = "Option::<_>::is_none")] | ||
| pub ignore_pattern: Option<Box<str>>, |
There was a problem hiding this comment.
- We try to ignore regex if possible because of the performance impact it can have.
- Do we have a use case for why it needs to be a regex or pattern?
- This needs to be a
Option<Box<[...]>>so the user can supply multiple identifiers to ignore.
Realistically, I don't really see this being a regex to be super useful. The rule already ignores variables that begin with _, so if you want to have a bunch of unused variables all with different names, you can just add the prefix.
There was a problem hiding this comment.
I just noticed you described your use case in your PR description, but its not super clear to me.
There was a problem hiding this comment.
I am fine making this a list for sure! I was just drawing inspiration the ESLint API which uses a single pattern.
Regex would conveniently allow specifying whether something is to be treated as a prefix or suffix for instance. In my use-case I want to ignore all .*Service$ for instance. I feel like treating the patterns as a .contains() might be a bit too drastic or is that what you had in mind? Generally fine with me but without regex taking the example above (i.e. an ignorePattern of Service), that would hit a variable called somethingServiceInstance as well which might not be intended.
I am open for ideas here!
There was a problem hiding this comment.
We have a type called RestrictedRegex. We should use that one
There was a problem hiding this comment.
Think a rough setup like this:
// service-decorator.js
export const Service = () => {
return (targetClass) => {
console.log('Registering service:', targetClass.name);
return targetClass;
};
};// services/a.js
@Service()
class ExampleService {}// services/b.js
@Service()
class AnotherExampleService {}// index.js
async main() {
const matchedFiles = glob('services/*.js');
for (const file of matchedFiles) {
await import(file);
}
launchApp();
}
main();Since there can be a lot of services (or for example GraphQL-Resolvers or Chatbot-Commands, etc.) in a project, it can be tedious to register them all by hand, having massive barrel exports of dozens or even hundred classes, just to register them to some library.
I am using a setup like the above to automatically load such files marked with a decorator and doing the registering logic inside the decorator. However since those classes are never actually imported anywhere statically, they are marked as unused which causes a bunch of errors.
There was a problem hiding this comment.
That's much more clear, thank you. Yeah, our semantic model isn't quite smart enough to pick that up.
It feels like to me it would make more sense add configurable filters based on the syntax construct (for lack of a better word) that is being defined.
Roughly speaking, you would configure the rule something like this:
{
"noUnusedVariables": {
"options": {
"ignore": {
"*": ["foo"], // all syntax constructs
"class": ["Bar"], // only classes
"function": ["*"], // ignore all functions
"variable": [], // only variables, but empty so no-op
// etc...
}
}
}
}where * is a hard coded special value to mean "everything", and the lists of identifiers are just strings and not regexes. I'm particularly wary of adding RestrictedRegex to this rule because it's one of the rules that gets run the most often on every codebase.
And then, for your specific use case ignoring all classes in those files, you could use noExcessiveClassesPerFile to make sure you don't add extra classes to those files, which would otherwise become false negatives.
There was a problem hiding this comment.
Alright, sounds like a plan! Just to confirm: The strings are to be interpreted as equality matches, correct? Not prefix, not suffix and not everything containing those strings?
There was a problem hiding this comment.
I'm particularly wary of adding RestrictedRegex to this rule because it's one of the rules that gets run the most often on every codebase.
Sure, it makes sense, however, it's not like we're adding regex by default. If the user defines an incorrect regex, it's not Biome's fault.
Still, if normal strings are fine for the time being, that's also fine.
There was a problem hiding this comment.
I updated the implementation with the same symbol types found in the diagnostic function, i.e. class, function, interface, parameter, type alias, type parameter and variable as well as the special "*" key.
I think retrofitting this with regex should be relatively straightforward if it comes up in the future (with some migration needed for the wildcard, I suppose).
| ) | ||
| }); | ||
| let is_used_as_reference = embedded_references.is_used_as_value(binding_name); | ||
| let is_ignored = ctx.options().ignore_pattern().is_match(binding_name); |
There was a problem hiding this comment.
nit: I would prefer if this check occurs as soon as possible, at the top of the rule, and to make the rule return early.
| /// | ||
| /// Default: `null` (no variables are excluded) | ||
| /// | ||
| /// #### Example |
There was a problem hiding this comment.
nit:
| /// #### Example | |
| /// #### Examples |
github-actions
bot
added
A-CLI
mvarendorff
force-pushed
the
feat/no-unused-variables-ignore-pattern
branch
from
04b98c9 to
5452b27
Compare
github-actions
bot
removed
A-Project
mvarendorff
changed the title
mvarendorff
force-pushed
the
feat/no-unused-variables-ignore-pattern
branch
from
6584108 to
3873de5
Compare
|
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 an ignore mechanism to the noUnusedVariables rule and its options. Introduces a new public Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 531-539: The current selection of specific_ignores uses the
binding's immediate parent kind which misclassifies destructured/rest
parameters; update the logic that computes specific_ignores (the match on
binding.syntax().parent()?.kind()) to walk up the ancestor chain from
binding.syntax() until you find the declaring node (e.g., a parameter list,
function declaration, class declaration, TS interface/type declarations) and
then match that ancestor's kind to choose ignore_options.parameter, .function,
.class, .interface, .type_alias, .type_parameter, or .variable accordingly so
destructured and rest parameters are classified as parameters rather than
variables.
In `@crates/biome_rule_options/src/no_unused_variables.rs`:
- Around line 26-27: Update the rustdoc so it no longer references the removed
`ignore_pattern` or "returns a regex" behavior: change the doc comment for the
`ignore()` method (in the no_unused_variables module / `NoUnusedVariables` type)
to describe that `ignore()` now returns structured ignore groups (the new
structured type) and explain how those groups are interpreted, instead of saying
it "returns a regex" or "doesn't match anything"; keep the description concise
and mention the returned structure name so readers can find the type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 311da800-3c27-490c-8b5a-43b437361a1c
⛔ Files ignored due to path filters (5)
crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidIgnoreAllPattern.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidIgnoreClassPattern.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidIgnoreWildcardPattern.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validIgnoredPattern.js.snapis excluded by!**/*.snapand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (11)
.changeset/shaggy-grapes-act.mdcrates/biome_js_analyze/src/lint/correctness/no_unused_variables.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidIgnoreAllPattern.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidIgnoreAllPattern.options.jsoncrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidIgnoreClassPattern.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidIgnoreClassPattern.options.jsoncrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidIgnoreWildcardPattern.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidIgnoreWildcardPattern.options.jsoncrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validIgnoredPattern.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/validIgnoredPattern.options.jsoncrates/biome_rule_options/src/no_unused_variables.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_rule_options/src/no_unused_variables.rs (1)
28-30: Consider usingunwrap_or_default()for brevity.Since
NoUnusedVariablesOptionsIgnorederivesDefault, this can be simplified.✨ Suggested simplification
pub fn ignore(&self) -> NoUnusedVariablesOptionsIgnore { - self.ignore.clone().unwrap_or(NoUnusedVariablesOptionsIgnore::default()) + self.ignore.clone().unwrap_or_default() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_rule_options/src/no_unused_variables.rs` around lines 28 - 30, The method NoUnusedVariablesOptions::ignore currently calls self.ignore.clone().unwrap_or(NoUnusedVariablesOptionsIgnore::default()); simplify it by using unwrap_or_default() on the cloned Option to leverage the Default impl of NoUnusedVariablesOptionsIgnore (e.g., replace self.ignore.clone().unwrap_or(NoUnusedVariablesOptionsIgnore::default()) with self.ignore.clone().unwrap_or_default()) to make the code shorter and clearer.
🤖 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_rule_options/src/no_unused_variables.rs`:
- Around line 28-30: The method NoUnusedVariablesOptions::ignore currently calls
self.ignore.clone().unwrap_or(NoUnusedVariablesOptionsIgnore::default());
simplify it by using unwrap_or_default() on the cloned Option to leverage the
Default impl of NoUnusedVariablesOptionsIgnore (e.g., replace
self.ignore.clone().unwrap_or(NoUnusedVariablesOptionsIgnore::default()) with
self.ignore.clone().unwrap_or_default()) to make the code shorter and clearer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c4d3aca-061c-48af-83a6-7067473d6a33
📒 Files selected for processing (1)
crates/biome_rule_options/src/no_unused_variables.rs
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Nice job! The documentation may require some polishing
| /// | ||
| /// Default: `{}` (no variables are excluded) | ||
| /// | ||
| /// #### Examples |
There was a problem hiding this comment.
We already have this header, it's not needed
Alternatively, add multiple examples, and the header becomes the name of the example
There was a problem hiding this comment.
I was taking the structure from the existing option: https://biomejs.dev/linter/rules/no-unused-variables/#ignorerestsiblings. To have a reference for how it should look like instead with multiple examples - is this desirable: https://biomejs.dev/assist/actions/use-sorted-attributes/#sortorder ? Or should everything including options just go into the Examples section?
There was a problem hiding this comment.
Yeah some docs are old and don't follow the new guidelines.
If you plan having multiple explanations, you could follow the organize imports lead, otherwise we don't need the additional header IMHO
There was a problem hiding this comment.
Shuffled a few things around and wrote a couple sentences for the new option. I also dropped an additional comment on the ignoreRestSiblings example for good measure. The other review comments should be addressed as well now!
| } | ||
| } | ||
|
|
||
| if is_ignored(binding, ctx.options()).unwrap_or(false) { |
There was a problem hiding this comment.
| if is_ignored(binding, ctx.options()).unwrap_or(false) { | |
| if is_ignored(binding, ctx.options()).unwrap_or_default() { |
In rust it's idiomatic to use the default like this.
| } | ||
| } | ||
|
|
||
| /// Returns `true` if `binding` is considered as ignored. |
There was a problem hiding this comment.
| /// Returns `true` if `binding` is considered as ignored. | |
| /// Returns `true` if `binding` is considered as ignored by the user. |
mvarendorff
force-pushed
the
feat/no-unused-variables-ignore-pattern
branch
from
3862a07 to
f1bf13e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs (1)
534-542:⚠️ Potential issue | 🟠 Major
ignore.parameteris still misclassified for destructured/rest params.At Line 534, matching on
binding.syntax().parent()?.kind()uses the immediate parent, so bindings likefunction f({ ignored }) {}orfunction f(...ignored) {}can fall through tovariableinstead ofparameter. That makesignore.parameterunreliable for common parameter forms.Suggested fix
- let specific_ignores = match binding.syntax().parent()?.kind() { - JsSyntaxKind::JS_FORMAL_PARAMETER => ignore_options.parameter, - JsSyntaxKind::JS_FUNCTION_DECLARATION => ignore_options.function, - JsSyntaxKind::JS_CLASS_DECLARATION => ignore_options.class, - JsSyntaxKind::TS_INTERFACE_DECLARATION => ignore_options.interface, - JsSyntaxKind::TS_TYPE_ALIAS_DECLARATION => ignore_options.type_alias, - JsSyntaxKind::TS_TYPE_PARAMETER => ignore_options.type_parameter, - _ => ignore_options.variable, - }; + let declaration = binding.declaration()?; + let declaration = declaration + .parent_binding_pattern_declaration() + .unwrap_or(declaration); + + let specific_ignores = match declaration { + AnyJsBindingDeclaration::JsFormalParameter(_) + | AnyJsBindingDeclaration::JsRestParameter(_) + | AnyJsBindingDeclaration::TsIndexSignatureParameter(_) + | AnyJsBindingDeclaration::TsPropertyParameter(_) => ignore_options.parameter, + AnyJsBindingDeclaration::JsFunctionDeclaration(_) => ignore_options.function, + AnyJsBindingDeclaration::JsClassDeclaration(_) => ignore_options.class, + AnyJsBindingDeclaration::TsInterfaceDeclaration(_) => ignore_options.interface, + AnyJsBindingDeclaration::TsTypeAliasDeclaration(_) => ignore_options.type_alias, + AnyJsBindingDeclaration::TsTypeParameter(_) | AnyJsBindingDeclaration::TsInferType(_) => { + ignore_options.type_parameter + } + _ => ignore_options.variable, + };🤖 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 534 - 542, specific_ignores misclassifies destructured/rest parameters because the code uses binding.syntax().parent()?.kind() to detect parameter context; instead, walk ancestors from binding.syntax() to find an enclosing parameter-like node (e.g., JS_FORMAL_PARAMETER, REST_PARAMETER, and any pattern/parameter wrapper kinds your AST provides) and base the match on that ancestor’s kind so ignore_options.parameter is chosen for destructured and rest params; update the match that sets specific_ignores (currently using binding.syntax().parent()?.kind()) to inspect the nearest ancestor parameter node before falling back to variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs`:
- Around line 534-542: specific_ignores misclassifies destructured/rest
parameters because the code uses binding.syntax().parent()?.kind() to detect
parameter context; instead, walk ancestors from binding.syntax() to find an
enclosing parameter-like node (e.g., JS_FORMAL_PARAMETER, REST_PARAMETER, and
any pattern/parameter wrapper kinds your AST provides) and base the match on
that ancestor’s kind so ignore_options.parameter is chosen for destructured and
rest params; update the match that sets specific_ignores (currently using
binding.syntax().parent()?.kind()) to inspect the nearest ancestor parameter
node before falling back to variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9083317d-5aab-4b88-af28-bfff75f43f27
📒 Files selected for processing (1)
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs (1)
521-545: Implementation looks solid and aligns with thediagnosticfunction's pattern.The match on
binding.syntax().parent()?.kind()correctly mirrors thesymbol_typedetection indiagnostic, ensuring consistent behaviour between ignore matching and diagnostic messages.One optional readability nit: lines 527 and 542 are rather long. Breaking them up could improve scannability.
,
♻️ Optional: break long lines for readability
- let is_all_ignored = ignore_options.all.unwrap_or_default().iter().any(|ignore| &**ignore == NoUnusedVariablesOptionsIgnore::IGNORE_ALL || &**ignore == binding_name); + let is_all_ignored = ignore_options + .all + .unwrap_or_default() + .iter() + .any(|ignore| { + &**ignore == NoUnusedVariablesOptionsIgnore::IGNORE_ALL || &**ignore == binding_name + });- let is_specific_ignored = specific_ignores.unwrap_or_default().iter().any(|ignore| &**ignore == NoUnusedVariablesOptionsIgnore::IGNORE_ALL || &**ignore == binding_name); + let is_specific_ignored = specific_ignores + .unwrap_or_default() + .iter() + .any(|ignore| { + &**ignore == NoUnusedVariablesOptionsIgnore::IGNORE_ALL || &**ignore == binding_name + });🤖 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 521 - 545, The two long iterator lines in the is_ignored function (where is_all_ignored and is_specific_ignored are computed) should be split for readability: extract the comparison predicate into a small closure or local function (e.g., let matches_ignore = |ignore: &String| -> bool { &**ignore == NoUnusedVariablesOptionsIgnore::IGNORE_ALL || &**ignore == binding_name };) and then replace the long iter.any(...) calls with iter().any(matches_ignore) (apply this to both the is_all_ignored and specific_ignores checks, referencing binding_name, options.ignore(), NoUnusedVariablesOptionsIgnore::IGNORE_ALL, specific_ignores, and is_specific_ignored to locate the spots).
🤖 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/correctness/no_unused_variables.rs`:
- Around line 521-545: The two long iterator lines in the is_ignored function
(where is_all_ignored and is_specific_ignored are computed) should be split for
readability: extract the comparison predicate into a small closure or local
function (e.g., let matches_ignore = |ignore: &String| -> bool { &**ignore ==
NoUnusedVariablesOptionsIgnore::IGNORE_ALL || &**ignore == binding_name };) and
then replace the long iter.any(...) calls with iter().any(matches_ignore) (apply
this to both the is_all_ignored and specific_ignores checks, referencing
binding_name, options.ignore(), NoUnusedVariablesOptionsIgnore::IGNORE_ALL,
specific_ignores, and is_specific_ignored to locate the spots).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94475733-2c2c-467f-a447-5ebdf5d33a1b
⛔ Files ignored due to path filters (1)
packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (2)
crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rscrates/biome_rule_options/src/no_unused_variables.rs
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Left some suggestions for the docs and the changeset
| "@biomejs/biome": minor | ||
| --- | ||
|
|
||
| Added `ignore` option to the [noUnusedVariables](https://biomejs.dev/linter/rules/no-unused-variables/) rule. The option allows excluding identifiers by providing a list of ignored names. |
There was a problem hiding this comment.
@mvarendorff can you please provide a more descriptive changeset? We're adding a new feature, so would want to show it off a bit more. For example we should explain what it solves, and provide a real world example.
There was a problem hiding this comment.
I hope this is more appropriate? If not, I am open to suggestions - writing changelogs is not really my forte 😄
| /// | ||
| /// Default: `{}` (no variables are excluded) | ||
| /// | ||
| /// For example, we can exclude all unused identifiers named `ignored` regardless of their kind, |
There was a problem hiding this comment.
| /// For example, we can exclude all unused identifiers named `ignored` regardless of their kind, | |
| /// For example, you can exclude all unused identifiers named `ignored` regardless of their kind, |
You should either use the third person "it" (e.g. Biome excludes) or "you" (e.g. you can exclude).
| /// "ignore": { | ||
| /// "*": ["ignored"], | ||
| /// "class": ["IgnoredClass"], | ||
| /// "function": ["*"], |
There was a problem hiding this comment.
The * as a value was never explained in the docs until now. We explained as a key though. We should address that. Plus, the description to the example doesn't mention function being used and the effects it has
There was a problem hiding this comment.
The docs already contained a note "The special identifier "*" can be used to match all identifiers in the respective group.". I rephrased it to indicate both usages as key and as value more clearly now (hopefully).
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
@mvarendorff as you can see the CI is failing. Use I believe the problem is that the new code block you added, emits more than one diagnostic (by design, a code block must emit only one diagnostic). You can add the |
mvarendorff
force-pushed
the
feat/no-unused-variables-ignore-pattern
branch
from
e844899 to
eeceada
Compare
Yup, that's the one, thank you! I saw the CI failure last night but skipped over the line that explained it. Added the ignore and now everything looks fine locally 🤞 |
|
@dyc3 please give your review once you have the chance |
3 participants
No AI was used in the creation of this PR. If the changes are hideous, that's entirely my doing.
Summary
Adds a new option
ignoretonoUnusedVariablesto exclude identifiers from the rule.A discussion on this feature exists and a recent comment approved of the option.
A similar option is also available in ESLint.
My specific use-case for this is a DI-style setup where a directory is globbed and files dynamically loaded with
await import(...). A decorator on classes in those files causes them to be registered to a DI provider however without this option, all classes are marked unused. With the option available, it becomes trivial to exclude these classes by means of a naming scheme (not perfect but better than disabling the check outright).Test Plan
Snapshot tests have been added.
Docs
I hope they cover everything that's needed. Let me know if anything is missing!