perf(lint): avoid string allocations in some lint rules#10088
Conversation
|
8665ee5 to
4f1f22e
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
4f1f22e to
95bdc33
Compare
WalkthroughThis PR refactors lint rules across multiple modules to reduce string allocations in internal state. Changes consistently replace owned strings ( 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)
Comment |
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 (3)
crates/biome_js_analyze/src/lint/suspicious/no_duplicate_jsx_props.rs (1)
56-130:⚠️ Potential issue | 🟠 MajorPlease add snapshot coverage for this rule rewrite.
This changes core duplicate-grouping behaviour (including namespaced keying), but there are no snapshot updates in the PR metadata. Please add/refresh snapshots under
tests/specs/suspicious/noDuplicateJsxProps/(including namespaced and spread-adjacent cases) to lock behaviour.As per coding guidelines: “All code changes MUST include appropriate tests: lint rules require snapshot tests in
tests/specs/{group}/{rule}/.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/suspicious/no_duplicate_jsx_props.rs` around lines 56 - 130, The PR changed duplicate-grouping behavior in run and the attribute keying in attribute_name_key (including namespace handling and interactions with adjacent spreads) but no snapshot tests were added/updated; add or refresh snapshot tests under tests/specs/suspicious/noDuplicateJsxProps/ that exercise plain duplicate props, namespaced props, and cases with spread props adjacent to duplicates to lock the new behavior, using the rule's existing test harness so the snapshots capture the diagnostic output produced by run and diagnostic for these variants.crates/biome_js_analyze/src/lint/correctness/no_undeclared_variables.rs (1)
65-141:⚠️ Potential issue | 🟠 MajorAdd snapshot coverage for this rule refactor.
This rule’s diagnostic pipeline changed, but the PR metadata says no snapshot updates were included. Please add/refresh snapshots for
noUndeclaredVariablesto guard against behavioural drift.As per coding guidelines: "All code changes MUST include appropriate tests: lint rules require snapshot tests in
tests/specs/{group}/{rule}/."🤖 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_undeclared_variables.rs` around lines 65 - 141, The PR refactored the NoUndeclaredVariables rule (impl NoUndeclaredVariables, run and diagnostic) but did not update snapshot tests; add or refresh snapshot tests for the noUndeclaredVariables rule under tests/specs/correctness/noUndeclaredVariables (or the existing group folder) to cover the changed diagnostic messages and token locations produced by NoUndeclaredVariables::run and NoUndeclaredVariables::diagnostic; run the test harness to regenerate snapshots and commit the updated snapshot files so CI can validate there is no unintended behavioral drift.crates/biome_js_analyze/src/lint/complexity/no_useless_undefined_initialization.rs (1)
116-126:⚠️ Potential issue | 🟡 MinorFilter non-identifier declarators before pushing signals
At Line 126,
runpushes declarators even when the binding is not an identifier; those are then dropped indiagnostic(Lines 135-141). That creates non-actionable signals and a bit of extra churn in a perf-focused PR.Suggested tweak
- let is_exported = decl - .id() - .ok() - .and_then(|id| id.as_any_js_binding()?.as_js_identifier_binding().cloned()) - .is_some_and(|binding| binding.is_exported(model)); - - if is_exported { + let Some(binding) = decl + .id() + .ok() + .and_then(|id| id.as_any_js_binding()?.as_js_identifier_binding().cloned()) + else { + continue; + }; + + if binding.is_exported(model) { continue; } signals.push(decl);Also applies to: 135-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/complexity/no_useless_undefined_initialization.rs` around lines 116 - 126, The code currently pushes declarators (decl) into signals inside run even when the binding isn't an identifier, causing diagnostic to later drop them; update the run logic to only push declarators whose binding is an identifier by checking decl.id().ok().and_then(|id| id.as_any_js_binding()?.as_js_identifier_binding()).is_some() (reusing the same predicate used for is_exported) before signals.push(decl); similarly ensure diagnostic (or any other consumer) no longer needs to filter non-identifier entries so signals only contains identifier bindings.
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/suspicious/no_import_cycles.rs (1)
208-210: Defensive handling noted.The
continuehere is defensive — based on the code flow, allResolvedPathobjects in state should have valid paths (validated before being added infind_cycle). If this ever triggers unexpectedly, the diagnostic would silently skip a path in the cycle chain, which might confuse users. Consider whether a debug assertion or logging would help catch this edge case during development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/suspicious/no_import_cycles.rs` around lines 208 - 210, The current loop silently continues when path.as_path() returns None on a ResolvedPath, which can hide unexpected invalid state; instead of silently skipping, replace the bare continue with a defensive check that surfaces the issue during development—use a debug assertion or logging inside the loop (e.g., debug_assert!(path.as_path().is_some(), "ResolvedPath in find_cycle must have a valid path") or process_logger.warn/error with context) referencing the ResolvedPath value and the find_cycle flow so developers can detect and investigate if this branch is ever hit; keep behavior in release builds unchanged (i.e., still skip if None) but ensure developers get notified in debug/testing.crates/biome_js_analyze/src/lint/suspicious/no_duplicate_jsx_props.rs (1)
67-94: Nested duplicate scan makes this path O(n²).Nice allocation work, but this double loop plus
processed_names.containsre-scans attributes repeatedly. A single-pass grouping byAttributeNameKeykeeps the same semantics with linear behaviour.Possible linear-pass refactor
- let mut duplicated_attributes = Vec::new(); - let mut processed_names = Vec::new(); - - for (index, attr) in attributes.iter().enumerate() { - let Some(name) = attribute_name_key(attr) else { - continue; - }; - - if processed_names.contains(&name) { - continue; - } - - let mut duplicates = vec![attr.clone()]; - - for other in attributes.iter().skip(index + 1) { - let Some(other_name) = attribute_name_key(other) else { - continue; - }; - - if name == other_name { - duplicates.push(other.clone()); - } - } - - if duplicates.len() > 1 { - processed_names.push(name); - duplicated_attributes.push(duplicates); - } - } - - duplicated_attributes + use rustc_hash::FxHashMap; + + let mut by_name: FxHashMap<AttributeNameKey, Vec<JsxAttribute>> = FxHashMap::default(); + for attr in attributes { + let Some(name) = attribute_name_key(&attr) else { + continue; + }; + by_name.entry(name).or_default().push(attr); + } + + by_name + .into_values() + .filter(|group| group.len() > 1) + .collect()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/suspicious/no_duplicate_jsx_props.rs` around lines 67 - 94, The current nested loop in the block that builds duplicated_attributes (using processed_names and attribute_name_key over attributes) is O(n²); change it to a single-pass grouping by AttributeNameKey: iterate once over attributes, call attribute_name_key(attr) and push attr.clone() into a HashMap<AttributeNameKey, Vec<Attribute>> (preserving original order if needed), then collect map values with length > 1 into duplicated_attributes; remove processed_names and the inner .contains/.skip logic and keep all references to attribute_name_key, duplicated_attributes, attributes, and duplicates consistent.
🤖 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/complexity/no_useless_undefined_initialization.rs`:
- Around line 116-126: The code currently pushes declarators (decl) into signals
inside run even when the binding isn't an identifier, causing diagnostic to
later drop them; update the run logic to only push declarators whose binding is
an identifier by checking decl.id().ok().and_then(|id|
id.as_any_js_binding()?.as_js_identifier_binding()).is_some() (reusing the same
predicate used for is_exported) before signals.push(decl); similarly ensure
diagnostic (or any other consumer) no longer needs to filter non-identifier
entries so signals only contains identifier bindings.
In `@crates/biome_js_analyze/src/lint/correctness/no_undeclared_variables.rs`:
- Around line 65-141: The PR refactored the NoUndeclaredVariables rule (impl
NoUndeclaredVariables, run and diagnostic) but did not update snapshot tests;
add or refresh snapshot tests for the noUndeclaredVariables rule under
tests/specs/correctness/noUndeclaredVariables (or the existing group folder) to
cover the changed diagnostic messages and token locations produced by
NoUndeclaredVariables::run and NoUndeclaredVariables::diagnostic; run the test
harness to regenerate snapshots and commit the updated snapshot files so CI can
validate there is no unintended behavioral drift.
In `@crates/biome_js_analyze/src/lint/suspicious/no_duplicate_jsx_props.rs`:
- Around line 56-130: The PR changed duplicate-grouping behavior in run and the
attribute keying in attribute_name_key (including namespace handling and
interactions with adjacent spreads) but no snapshot tests were added/updated;
add or refresh snapshot tests under tests/specs/suspicious/noDuplicateJsxProps/
that exercise plain duplicate props, namespaced props, and cases with spread
props adjacent to duplicates to lock the new behavior, using the rule's existing
test harness so the snapshots capture the diagnostic output produced by run and
diagnostic for these variants.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/suspicious/no_duplicate_jsx_props.rs`:
- Around line 67-94: The current nested loop in the block that builds
duplicated_attributes (using processed_names and attribute_name_key over
attributes) is O(n²); change it to a single-pass grouping by AttributeNameKey:
iterate once over attributes, call attribute_name_key(attr) and push
attr.clone() into a HashMap<AttributeNameKey, Vec<Attribute>> (preserving
original order if needed), then collect map values with length > 1 into
duplicated_attributes; remove processed_names and the inner .contains/.skip
logic and keep all references to attribute_name_key, duplicated_attributes,
attributes, and duplicates consistent.
In `@crates/biome_js_analyze/src/lint/suspicious/no_import_cycles.rs`:
- Around line 208-210: The current loop silently continues when path.as_path()
returns None on a ResolvedPath, which can hide unexpected invalid state; instead
of silently skipping, replace the bare continue with a defensive check that
surfaces the issue during development—use a debug assertion or logging inside
the loop (e.g., debug_assert!(path.as_path().is_some(), "ResolvedPath in
find_cycle must have a valid path") or process_logger.warn/error with context)
referencing the ResolvedPath value and the find_cycle flow so developers can
detect and investigate if this branch is ever hit; keep behavior in release
builds unchanged (i.e., still skip if None) but ensure developers get notified
in debug/testing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27f5153b-e79c-4c25-a455-689880d2cd6a
📒 Files selected for processing (10)
crates/biome_css_analyze/src/lint/suspicious/no_duplicate_custom_properties.rscrates/biome_js_analyze/src/lint/complexity/no_useless_undefined_initialization.rscrates/biome_js_analyze/src/lint/correctness/no_global_dirname_filename.rscrates/biome_js_analyze/src/lint/correctness/no_undeclared_variables.rscrates/biome_js_analyze/src/lint/nursery/no_duplicated_spread_props.rscrates/biome_js_analyze/src/lint/style/no_restricted_globals.rscrates/biome_js_analyze/src/lint/suspicious/no_alert.rscrates/biome_js_analyze/src/lint/suspicious/no_duplicate_jsx_props.rscrates/biome_js_analyze/src/lint/suspicious/no_import_cycles.rscrates/biome_js_analyze/src/syntax/correctness/no_duplicate_private_class_members.rs
Summary
A bunch of our older rules allocate strings more than they should. This is a quick attempt to resolve that for some of them. If there's a perf improvement, I'll add a changeset. There's also some cosmetic refactors to slightly realign some of these rules with our current patterns.
Generated initially by gpt 5.4, but i tweaked some things by hand.
Test Plan
no snapshot updates.
Docs