perf(lint): reduce string allocations, part 2#10093
Conversation
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
Once you finished the refactor, would you mind updating the contribution guidelines, templates and skills? I noticed we've removed a lot of |
|
Sure, I'll do that in a new PR |
a9e98ec to
2bc4575
Compare
WalkthroughMultiple lint rules across CSS and JavaScript analysers are refactored to replace owned string allocations with 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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_js_analyze/src/lint/style/use_trim_start_end.rs (1)
124-210:⚠️ Potential issue | 🟠 MajorPlease add snapshot coverage for the new fix branches.
This change adds/adjusts fix behaviour for computed members with template literals and quote-style handling. I can’t see corresponding snapshot updates here, so regressions in fixer output are easy to miss.
Please add (or point to) rule snapshots covering at least:
foo['trimLeft']()→foo['trimStart']()foo["trimRight"]()→foo["trimEnd"]()foo[\trimLeft`]()→foo`trimStart``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/style/use_trim_start_end.rs` around lines 124 - 210, The new fix branches in action (in use_trim_start_end.rs) introduce template-literal and quoted-member handling (references: function action, state.member_name, TrimMethod, extract_token_from_expression) but lack snapshot tests; add snapshot specs under tests/specs/style/use_trim_start_end/ covering at minimum the three cases foo['trimLeft']()→foo['trimStart'](), foo["trimRight"]()→foo["trimEnd"](), and foo[`trimLeft`]()→foo[`trimStart`]() so the fixer output for computed member templates and both single- and double-quoted string members is validated; ensure filenames and snapshot contents follow existing rule test conventions and assert the transformed code matches the expected replacement.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/style/use_trim_start_end.rs (1)
163-210: Optional perf tidy-up: build template nodes only in the template branch.
elements/template_elementsare allocated eagerly even whenis_templateis false. Deferring that work keeps this hot path a bit leaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/style/use_trim_start_end.rs` around lines 163 - 210, The code eagerly allocates elements and template_elements before computing transformed_expression, which wastes work when is_template is false; move the creation of elements/AnyJsTemplateElement (and make::js_template_chunk_element/js_template_chunk calls) into the branch that builds the template expression (the is_template arm that constructs AnyJsExpression::JsTemplateExpression) and only build those nodes when is_template is true, leaving the non-template branches (string literal arms) unchanged; keep references to computed_member_expression_opt and the existing js_template_expression construction but instantiate elements/template_elements inside that branch.
🤖 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_css_analyze/src/lint/suspicious/no_duplicate_properties.rs`:
- Around line 64-65: The scan currently returns early when encountering a
malformed property because run uses prop.value().ok()?; change this so the
malformed declaration is skipped and scanning continues: in the loop inside run
(the code handling prop, prop_name, prop_range) replace the fallible
`prop.value().ok()?` with a non-fatal check (e.g., if let Some(prop_name) =
prop.value().ok() { ... } else { continue }) so only that declaration is ignored
and duplicate-property detection proceeds for the rest of the block; also add a
snapshot test where an invalid declaration precedes a duplicate valid property
to lock the desired behaviour.
In `@crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rs`:
- Line 200: The asterisk-detection uses the bitwise-not expression `!b` instead
of an inequality, so expressions like `line.as_bytes().iter().take_while(|&&b|
!b == b'*')` (and the similar check around the skip-line logic at the other
occurrence) are wrong; change both closures to use `b !=` (e.g., `|&&b| b !=
b'*'`) so the take_while/count and skip logic correctly detect `'*'`.
In `@crates/biome_js_analyze/src/lint/style/use_trim_start_end.rs`:
- Around line 118-120: The diagnostic note in the .note(...) call inside
use_trim_start_end.rs is missing the opening quote before the current method
name (it currently renders {member_name}" is an alias for "{suggested_name}".);
update the markup string so it includes the opening quote before {member_name}
(e.g., "\"{member_name}\" is an alias for \"{suggested_name}.\"") in the
.note(markup! { ... }) expression to correctly quote both names; locate the
.note invocation that references member_name and suggested_name and fix the
markup there.
In `@crates/biome_js_analyze/src/lint/suspicious/no_shadow_restricted_names.rs`:
- Around line 63-65: Decide and make the rule-scope explicit for
no_shadow_restricted_names: choose whether the rule should match only JS
language-restricted names or also include environment globals (browser/Node),
then update the check from is_js_language_global(name) to the chosen helper
(e.g., is_language_global(name) for language-only or a combined helper like
is_env_or_language_global(name)), adjust State creation in
no_shadow_restricted_names to use that explicit helper, and add snapshot tests
under tests/specs/suspicious/no-shadow-restricted-names showing expected matches
for representative identifiers (NaN, Set, Array, Object, JSON for language;
window, document for browser; process, Buffer, globalThis for Node) so the
chosen scope is documented and locked by tests.
---
Outside diff comments:
In `@crates/biome_js_analyze/src/lint/style/use_trim_start_end.rs`:
- Around line 124-210: The new fix branches in action (in use_trim_start_end.rs)
introduce template-literal and quoted-member handling (references: function
action, state.member_name, TrimMethod, extract_token_from_expression) but lack
snapshot tests; add snapshot specs under tests/specs/style/use_trim_start_end/
covering at minimum the three cases foo['trimLeft']()→foo['trimStart'](),
foo["trimRight"]()→foo["trimEnd"](), and foo[`trimLeft`]()→foo[`trimStart`]() so
the fixer output for computed member templates and both single- and
double-quoted string members is validated; ensure filenames and snapshot
contents follow existing rule test conventions and assert the transformed code
matches the expected replacement.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/style/use_trim_start_end.rs`:
- Around line 163-210: The code eagerly allocates elements and template_elements
before computing transformed_expression, which wastes work when is_template is
false; move the creation of elements/AnyJsTemplateElement (and
make::js_template_chunk_element/js_template_chunk calls) into the branch that
builds the template expression (the is_template arm that constructs
AnyJsExpression::JsTemplateExpression) and only build those nodes when
is_template is true, leaving the non-template branches (string literal arms)
unchanged; keep references to computed_member_expression_opt and the existing
js_template_expression construction but instantiate elements/template_elements
inside that branch.
🪄 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: 71c65f22-04f2-4072-8bcc-dbedc70dda80
📒 Files selected for processing (10)
crates/biome_css_analyze/src/lint/suspicious/no_duplicate_properties.rscrates/biome_js_analyze/src/lint/complexity/use_date_now.rscrates/biome_js_analyze/src/lint/complexity/use_numeric_literals.rscrates/biome_js_analyze/src/lint/correctness/no_undeclared_dependencies.rscrates/biome_js_analyze/src/lint/correctness/no_unresolved_imports.rscrates/biome_js_analyze/src/lint/correctness/no_void_elements_with_children.rscrates/biome_js_analyze/src/lint/correctness/no_vue_duplicate_keys.rscrates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rscrates/biome_js_analyze/src/lint/style/use_trim_start_end.rscrates/biome_js_analyze/src/lint/suspicious/no_shadow_restricted_names.rs
| let prop_name = prop.value().ok()?; | ||
| let prop_range = prop.range(); |
There was a problem hiding this comment.
Don’t abort the whole scan on one malformed property name.
prop.value().ok()? at Line 64 exits run early, so one malformed declaration can suppress duplicate-property diagnostics for the rest of the block. Skip that declaration instead and keep scanning.
Suggested fix
- let prop_name = prop.value().ok()?;
+ let Ok(prop_name) = prop.value() else {
+ continue;
+ };Also worth adding a snapshot where an invalid declaration appears before a duplicate valid property, to lock this behaviour in.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let prop_name = prop.value().ok()?; | |
| let prop_range = prop.range(); | |
| let Ok(prop_name) = prop.value() else { | |
| continue; | |
| }; | |
| let prop_range = prop.range(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_css_analyze/src/lint/suspicious/no_duplicate_properties.rs`
around lines 64 - 65, The scan currently returns early when encountering a
malformed property because run uses prop.value().ok()?; change this so the
malformed declaration is skipped and scanning continues: in the loop inside run
(the code handling prop, prop_name, prop_range) replace the fallible
`prop.value().ok()?` with a non-fatal check (e.g., if let Some(prop_name) =
prop.value().ok() { ... } else { continue }) so only that declaration is ignored
and duplicate-property detection proceeds for the rest of the block; also add a
snapshot test where an invalid declaration precedes a duplicate valid property
to lock the desired behaviour.
| .take_while(|&&b| !char_is_asterisk(b)) | ||
| .count() | ||
| + 1 | ||
| line.as_bytes().iter().take_while(|&&b| !b == b'*').count() + 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify accidental bitwise-NOT equality checks against '*' in Rust sources.
# Expected: no matches after applying the fix.
rg -nP --type=rust "!b\s*==\s*b'\*'" -C2Repository: biomejs/biome
Length of output: 1148
🏁 Script executed:
#!/bin/bash
# Check if the file has an action function and fix_kind declaration
rg -A 20 "declare_lint_rule!" crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rs | head -40Repository: biomejs/biome
Length of output: 730
🏁 Script executed:
#!/bin/bash
# Check if there's an action function implementation in the file
rg -c "fn action" crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rsRepository: biomejs/biome
Length of output: 58
🏁 Script executed:
#!/bin/bash
# Verify there are no other bitwise NOT operator mistakes near the problematic lines
rg -nP "!b\s*(==|!=)" crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rsRepository: biomejs/biome
Length of output: 192
🏁 Script executed:
#!/bin/bash
# Get the full declare_lint_rule block to check for fix_kind
sed -n '60,80p' crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rsRepository: biomejs/biome
Length of output: 635
🏁 Script executed:
#!/bin/bash
# Check the action function to understand the fix behavior
rg -A 5 "impl Rule for" crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rs | head -20Repository: biomejs/biome
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Verify action function exists in the Rule impl block
rg -c "fn action\(&self" crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rsRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Get context around the action function to verify it's properly implemented
rg -B 2 -A 10 "fn action\(&self" crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rsRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check all functions in the Rule impl block
rg -A 100 "impl Rule for UseSingleJsDocAsterisk" crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rs | grep "fn " | head -10Repository: biomejs/biome
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Get the full structure of the Rule impl to see what functions are present
wc -l crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rsRepository: biomejs/biome
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Let's examine the action function to understand the intent
sed -n '240,298p' crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rsRepository: biomejs/biome
Length of output: 1744
🏁 Script executed:
#!/bin/bash
# Also check the get_first_asterisk_index function context more broadly
sed -n '190,230p' crates/biome_js_analyze/src/lint/correctness/use_single_js_doc_asterisk.rsRepository: biomejs/biome
Length of output: 1404
Fix the bitwise-NOT operator — these should be inequality checks.
!b flips bits on u8 and almost never equals b'*'. Both instances need b != instead. This breaks asterisk detection in both the index calculation and the skip-line logic.
Suggested fix
fn get_first_asterisk_index(line: &str) -> usize {
- line.as_bytes().iter().take_while(|&&b| !b == b'*').count() + 1
+ line.as_bytes().iter().take_while(|&&b| b != b'*').count() + 1
} // If first symbol is not an asterisk, we can skip the line
- if bytes.first().is_some_and(|&b| !b == b'*') {
+ if bytes.first().is_some_and(|&b| b != b'*') {
return None;
}Also applies to: 217
🤖 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/use_single_js_doc_asterisk.rs`
at line 200, The asterisk-detection uses the bitwise-not expression `!b` instead
of an inequality, so expressions like `line.as_bytes().iter().take_while(|&&b|
!b == b'*')` (and the similar check around the skip-line logic at the other
occurrence) are wrong; change both closures to use `b !=` (e.g., `|&&b| b !=
b'*'`) so the take_while/count and skip logic correctly detect `'*'`.
| .note(markup! { | ||
| {member_name}" is an alias for "{suggested_name}"." | ||
| }), |
There was a problem hiding this comment.
Diagnostic note has a small quoting typo.
The note currently renders without the opening quote before the current method name.
💡 Suggested patch
.note(markup! {
- {member_name}" is an alias for "{suggested_name}"."
+ "\"" {member_name} "\" is an alias for \"" {suggested_name} "\"."
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .note(markup! { | |
| {member_name}" is an alias for "{suggested_name}"." | |
| }), | |
| .note(markup! { | |
| "\"" {member_name} "\" is an alias for \"" {suggested_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/style/use_trim_start_end.rs` around lines
118 - 120, The diagnostic note in the .note(...) call inside
use_trim_start_end.rs is missing the opening quote before the current method
name (it currently renders {member_name}" is an alias for "{suggested_name}".);
update the markup string so it includes the opening quote before {member_name}
(e.g., "\"{member_name}\" is an alias for \"{suggested_name}.\"") in the
.note(markup! { ... }) expression to correctly quote both names; locate the
.note invocation that references member_name and suggested_name and fix the
markup there.
| // should this also cover web/node js globals? | ||
| if crate::globals::is_js_language_global(name) { | ||
| Some(State { |
There was a problem hiding this comment.
Please resolve the rule-scope ambiguity before merge.
Line 63 leaves an open behaviour question in code (web/node globals vs language globals). For a lint rule, this should be decided explicitly and backed by snapshots to avoid silent drift.
As per coding guidelines: "All code changes MUST include appropriate tests: lint rules require snapshot tests in tests/specs/{group}/{rule}/."
#!/bin/bash
# Verify current scope and snapshot coverage for no-shadow-restricted-names.
# 1) Inspect helper usage and available global classifiers.
fd no_shadow_restricted_names.rs
rg -n "is_js_language_global|is_language_global|ES_BUILTIN" crates/biome_js_analyze/src crates/biome_js_analyze/src/globals
# 2) Inspect snapshots for this rule and covered identifiers.
fd no-shadow-restricted-names crates/biome_js_analyze/tests/specs
rg -n "NaN|Set|Array|Object|JSON|window|document|process|Buffer|globalThis" crates/biome_js_analyze/tests/specs
# Expected:
# - Clear intended helper/scope choice.
# - Snapshot cases that document the chosen scope.In ESLint, exactly which identifiers are covered by `no-shadow-restricted-names`? Is it language-restricted names only, or does it include browser/Node globals too?
🤖 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_shadow_restricted_names.rs`
around lines 63 - 65, Decide and make the rule-scope explicit for
no_shadow_restricted_names: choose whether the rule should match only JS
language-restricted names or also include environment globals (browser/Node),
then update the check from is_js_language_global(name) to the chosen helper
(e.g., is_language_global(name) for language-only or a combined helper like
is_env_or_language_global(name)), adjust State creation in
no_shadow_restricted_names to use that explicit helper, and add snapshot tests
under tests/specs/suspicious/no-shadow-restricted-names showing expected matches
for representative identifiers (NaN, Set, Array, Object, JSON for language;
window, document for browser; process, Buffer, globalThis for Node) so the
chosen scope is documented and locked by tests.
Summary
same objective as #10088
Generated by gpt 5.4, didn't have to tweak much.
Test Plan
no snapshot changes
Docs