feat(linter): add includes option for plugin file scoping#9171
Open
chocky335 wants to merge 16 commits intobiomejs:nextfrom
Open
feat(linter): add includes option for plugin file scoping#9171chocky335 wants to merge 16 commits intobiomejs:nextfrom
chocky335 wants to merge 16 commits intobiomejs:nextfrom
Conversation
🦋 Changeset detectedLatest commit: 570979d 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-Project
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughAdds per-file plugin scoping: introduces 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_plugin_loader/src/lib.rs (1)
82-95:⚠️ Potential issue | 🟠 MajorJS plugins bypass includes/excludes—fix or document the inconsistency
AnalyzerJsPlugin::load()doesn't acceptincludes/excludesparameters, unlikeAnalyzerGritPlugin. This means JS plugins will run on all files regardless of the patterns passed toBiomePlugin::load(). Either pass these parameters through toAnalyzerJsPlugin(and store them for use inapplies_to_file()) or add a comment explaining why JS plugins don't support scoping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/lib.rs` around lines 82 - 95, The JS branch in BiomePlugin::load calls AnalyzerJsPlugin::load(plugin_path) without forwarding the includes/excludes used by AnalyzerGritPlugin, so JS plugins ignore scoping; update AnalyzerJsPlugin::load to accept the same includes/excludes parameters (or an options struct), pass the includes/excludes from BiomePlugin::load when constructing the AnalyzerJsPlugin, and ensure the AnalyzerJsPlugin instance stores these patterns and uses them in its applies_to_file() implementation; alternatively, if scoping is intentionally unsupported, add a clear comment near the AnalyzerJsPlugin::load call and on the AnalyzerJsPlugin type documenting that includes/excludes are not applied to JS plugins.
🧹 Nitpick comments (7)
.changeset/plugin-perf-anchor-dispatch.md (1)
8-9: Changeset descriptions should stay end-user-focused — trim the implementation internals.Lines 8 and 9 leak internal symbols (
PluginVisitor,BatchPluginVisitor,Contains) that end users don't interact with. The changelog should describe the observable benefit, not the mechanism.As per coding guidelines, changeset descriptions should be end-user-focused.
✏️ Suggested rewording
- - Replaced per-plugin `PluginVisitor` instances with a single `BatchPluginVisitor` that evaluates all plugins in one tree walk, reducing visitor dispatch overhead. - - Added anchor-kind dispatch to `GritQuery`, which extracts target syntax kinds from the compiled pattern tree and only executes the pattern at matching nodes instead of walking the entire AST via `Contains`. + - Reduced visitor dispatch overhead by evaluating all plugins in a single tree walk instead of one walk per plugin. + - Restricted GritQL pattern matching to only the relevant node kinds, avoiding a full AST walk and cutting unnecessary work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/plugin-perf-anchor-dispatch.md around lines 8 - 9, The changeset currently leaks implementation details (PluginVisitor, BatchPluginVisitor, Contains, GritQuery internals); update the two lines to a user-facing summary that describes the observable benefits only (e.g., reduced plugin dispatch overhead and faster/targeted query execution) without naming internal types or methods—replace mentions of PluginVisitor/BatchPluginVisitor/Contains and the internal dispatch behavior with phrasing like "improved plugin evaluation performance" and "queries now run faster by targeting relevant syntax nodes" so the description is end-user-focused.crates/biome_plugin_loader/src/lib.rs (1)
40-48: Silently skipping invalid glob patterns could surprise users.If a user misconfigures an include/exclude pattern (e.g.,
"[invalid"), it's silently dropped with no diagnostic. Consider logging a warning or returning a diagnostic for malformed patterns so users aren't puzzled when their scoping doesn't work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/lib.rs` around lines 40 - 48, parse_globs currently drops invalid patterns silently; change it to collect both parsed Globs and parse errors, emit a warning for each malformed pattern (including the original pattern string and parse error) using the crate logging/tracing facility, then return Some(valid_globs) or None as before; locate the logic in parse_globs and adjust the iterator over patterns (patterns.iter()) to match on p.parse() results so you can warn on Err and keep Ok(Glob).crates/biome_grit_patterns/tests/quick_test.rs (2)
144-164: Debug-format comparison is serviceable but fragile.Comparing effects via
format!("{e:?}")works for equivalence but will break silently if theDebugoutput changes without the underlying data changing (or vice versa). SinceGritQueryEffectderivesEqandPartialEq, you could compare theVec<GritQueryEffect>directly for a stronger assertion.Compare effects directly
- let ranges1: Vec<_> = result1.effects.iter().map(|e| format!("{e:?}")).collect(); - let ranges2: Vec<_> = result2.effects.iter().map(|e| format!("{e:?}")).collect(); - assert_eq!( - ranges1, ranges2, - "execute and execute_optimized should produce the same effects for query: {query_source}" - ); + assert_eq!( + result1.effects, result2.effects, + "execute and execute_optimized should produce the same effects for query: {query_source}" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_grit_patterns/tests/quick_test.rs` around lines 144 - 164, The test currently compares debug-formatted effects strings which is fragile; instead collect and compare the actual Vec<GritQueryEffect> values. In assert_optimized_matches_execute replace the mapped format!("{e:?}") collections (ranges1, ranges2) with direct effect collections (e.g. result1.effects.iter().cloned().collect::<Vec<_>>() and same for result2.effects) and assert_eq! those Vec<GritQueryEffect> values so the test uses the derived Eq/PartialEq on GritQueryEffect rather than Debug output, keeping the existing assert message using query_source.
102-203: Good baseline coverage. Consider adding aNotpattern test.Given the
Pattern::Notconcern raised ingrit_query.rs, a test like`console.log($x)` where { $x <: not `"debug"` }executed against code containing both"debug"and"hello"arguments would help verify correctness of the optimised path for negation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_grit_patterns/tests/quick_test.rs` around lines 102 - 203, Add a new test in quick_test.rs that exercises Pattern::Not by calling assert_optimized_matches_execute with a query like "`console.log($x)` where { $x <: not `\"debug\"` }" and code containing both console.log("debug") and console.log("hello") so the negation branch is exercised; name the test (e.g., execute_optimized_not_pattern) and use JsFileSource::js_module() to ensure it runs the same compare path as the other tests, verifying execute and execute_optimized produce identical effects for the negated where-clause.crates/biome_grit_patterns/src/grit_query.rs (1)
542-542:Pattern::Andshould intersect kinds, not union them.
flat_mapcollects kinds from all conjuncts. Forand { A, B }, both A and B must match the same node. The correct anchor set is the intersection — if A targets kind X and B targets kind Y (X ≠ Y), no node can match both, so we'd visit nodes that can never match. This isn't a correctness bug (the inner pattern will reject them), but it undermines the optimisation.Intersect kinds for And patterns
- Pattern::And(and) => and.patterns.iter().flat_map(extract_anchor_kinds).collect(), + Pattern::And(and) => { + let mut iter = and.patterns.iter().map(extract_anchor_kinds).filter(|k| !k.is_empty()); + match iter.next() { + Some(first) => { + let first_set: std::collections::BTreeSet<_> = first.into_iter().collect(); + iter.fold(first_set, |acc, kinds| { + let set: std::collections::BTreeSet<_> = kinds.into_iter().collect(); + acc.intersection(&set).copied().collect() + }) + .into_iter() + .collect() + } + None => vec![], + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_grit_patterns/src/grit_query.rs` at line 542, Pattern::And currently unions anchor kinds by flat_mapping over and.patterns, which is wrong — it should compute the intersection of anchor kinds from each conjunct so only kinds common to all are returned. Modify extract_anchor_kinds handling for Pattern::And to fold over and.patterns: for each subpattern call extract_anchor_kinds, convert each result into a set of kinds, and intersect it with an accumulator set (start with the first subpattern's set or None→set). If any intersection becomes empty return an empty Vec; otherwise return the accumulator converted back to a Vec. Keep references to Pattern::And, and.patterns and extract_anchor_kinds when making the change.crates/biome_analyze/src/analyzer_plugin.rs (2)
104-113:applies_to_filecheck should come after the kind check for consistency and performance.In
PluginVisitor,applies_to_file(which may do glob matching) is called before the cheapHashSet::containskind check. InBatchPluginVisitor(line 219), you correctly check the kind first. Align these for consistency and to avoid unnecessary glob calls on non-matching nodes.♻️ Proposed fix
- // Skip if plugin doesn't apply to this file - if !self.plugin.applies_to_file(&ctx.options.file_path) { - return; - } - // TODO: Integrate to [`VisitorContext::match_query`]? let kind = node.kind(); if !self.query.contains(&kind) { return; } + + // Skip if plugin doesn't apply to this file + if !self.plugin.applies_to_file(&ctx.options.file_path) { + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_analyze/src/analyzer_plugin.rs` around lines 104 - 113, Move the cheap kind filter ahead of the file-glob filter in PluginVisitor: call let kind = node.kind(); if !self.query.contains(&kind) { return; } before invoking self.plugin.applies_to_file(&ctx.options.file_path). This mirrors BatchPluginVisitor’s order and avoids expensive glob matching for nodes that don't match the query; update the block in PluginVisitor (the applies_to_file and query.contains checks) accordingly.
218-247:applies_to_fileis called per-node – consider caching the result.For a file with many matching nodes,
applies_to_file(which does glob matching inAnalyzerGritPlugin) is invoked on every node for every plugin. Since the file path is constant during a single analysis run, the result could be computed once per plugin and cached (e.g. in aVec<bool>alongsideself.plugins, lazily initialised on first visit).Given this PR is specifically about reducing plugin overhead, this seems like low-hanging fruit – especially with 84 plugins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_analyze/src/analyzer_plugin.rs` around lines 218 - 247, The loop that calls plugin.applies_to_file(&ctx.options.file_path) for every node is wasting work; compute and cache the per-plugin boolean once per analyzed file and consult the cache inside the loop. Add a cache (e.g. Vec<bool> aligned with self.plugins or a HashMap keyed by plugin id) stored on the Analyzer/AnalyzerPlugin instance and lazily initialize it when you start processing a new file (use ctx.options.file_path to detect file changes); replace the direct applies_to_file call in the for (query, plugin) in &self.plugins loop with a lookup of the cached boolean so applies_to_file is invoked at most once per plugin per file. Ensure the cache is cleared or recomputed when file_path changes.
🤖 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_grit_patterns/src/grit_query.rs`:
- Around line 543-546: The Pattern::Not match arm currently calls
extract_anchor_kinds(¬.pattern), which yields anchor kinds for the negated
pattern and can incorrectly restrict execute_optimized; change the Pattern::Not
arm in the extract_anchor_kinds implementation to return an empty vector (vec![]
/ Vec::new()) so callers (like execute_optimized / execute) will fall back to
the non-optimized path instead of filtering by the negated pattern's kinds.
- Around line 132-247: The code in execute_optimized currently calls unwrap() on
inner after checking inner.is_none(); replace that pattern with an idiomatic
guard using let Some(inner) = inner else { return self.execute(file); } to avoid
unwrap and make the early return clearer; locate the variable inner in the
execute_optimized function (after extract_contains_inner(&self.pattern)) and
replace the two-step is_none()/unwrap() sequence with a single let Some(...)
else guard while leaving the rest of the method (anchor_kinds logic, tree
creation, loop over anchor_nodes, and state handling) unchanged.
---
Outside diff comments:
In `@crates/biome_plugin_loader/src/lib.rs`:
- Around line 82-95: The JS branch in BiomePlugin::load calls
AnalyzerJsPlugin::load(plugin_path) without forwarding the includes/excludes
used by AnalyzerGritPlugin, so JS plugins ignore scoping; update
AnalyzerJsPlugin::load to accept the same includes/excludes parameters (or an
options struct), pass the includes/excludes from BiomePlugin::load when
constructing the AnalyzerJsPlugin, and ensure the AnalyzerJsPlugin instance
stores these patterns and uses them in its applies_to_file() implementation;
alternatively, if scoping is intentionally unsupported, add a clear comment near
the AnalyzerJsPlugin::load call and on the AnalyzerJsPlugin type documenting
that includes/excludes are not applied to JS plugins.
---
Nitpick comments:
In @.changeset/plugin-perf-anchor-dispatch.md:
- Around line 8-9: The changeset currently leaks implementation details
(PluginVisitor, BatchPluginVisitor, Contains, GritQuery internals); update the
two lines to a user-facing summary that describes the observable benefits only
(e.g., reduced plugin dispatch overhead and faster/targeted query execution)
without naming internal types or methods—replace mentions of
PluginVisitor/BatchPluginVisitor/Contains and the internal dispatch behavior
with phrasing like "improved plugin evaluation performance" and "queries now run
faster by targeting relevant syntax nodes" so the description is
end-user-focused.
In `@crates/biome_analyze/src/analyzer_plugin.rs`:
- Around line 104-113: Move the cheap kind filter ahead of the file-glob filter
in PluginVisitor: call let kind = node.kind(); if !self.query.contains(&kind) {
return; } before invoking self.plugin.applies_to_file(&ctx.options.file_path).
This mirrors BatchPluginVisitor’s order and avoids expensive glob matching for
nodes that don't match the query; update the block in PluginVisitor (the
applies_to_file and query.contains checks) accordingly.
- Around line 218-247: The loop that calls
plugin.applies_to_file(&ctx.options.file_path) for every node is wasting work;
compute and cache the per-plugin boolean once per analyzed file and consult the
cache inside the loop. Add a cache (e.g. Vec<bool> aligned with self.plugins or
a HashMap keyed by plugin id) stored on the Analyzer/AnalyzerPlugin instance and
lazily initialize it when you start processing a new file (use
ctx.options.file_path to detect file changes); replace the direct
applies_to_file call in the for (query, plugin) in &self.plugins loop with a
lookup of the cached boolean so applies_to_file is invoked at most once per
plugin per file. Ensure the cache is cleared or recomputed when file_path
changes.
In `@crates/biome_grit_patterns/src/grit_query.rs`:
- Line 542: Pattern::And currently unions anchor kinds by flat_mapping over
and.patterns, which is wrong — it should compute the intersection of anchor
kinds from each conjunct so only kinds common to all are returned. Modify
extract_anchor_kinds handling for Pattern::And to fold over and.patterns: for
each subpattern call extract_anchor_kinds, convert each result into a set of
kinds, and intersect it with an accumulator set (start with the first
subpattern's set or None→set). If any intersection becomes empty return an empty
Vec; otherwise return the accumulator converted back to a Vec. Keep references
to Pattern::And, and.patterns and extract_anchor_kinds when making the change.
In `@crates/biome_grit_patterns/tests/quick_test.rs`:
- Around line 144-164: The test currently compares debug-formatted effects
strings which is fragile; instead collect and compare the actual
Vec<GritQueryEffect> values. In assert_optimized_matches_execute replace the
mapped format!("{e:?}") collections (ranges1, ranges2) with direct effect
collections (e.g. result1.effects.iter().cloned().collect::<Vec<_>>() and same
for result2.effects) and assert_eq! those Vec<GritQueryEffect> values so the
test uses the derived Eq/PartialEq on GritQueryEffect rather than Debug output,
keeping the existing assert message using query_source.
- Around line 102-203: Add a new test in quick_test.rs that exercises
Pattern::Not by calling assert_optimized_matches_execute with a query like
"`console.log($x)` where { $x <: not `\"debug\"` }" and code containing both
console.log("debug") and console.log("hello") so the negation branch is
exercised; name the test (e.g., execute_optimized_not_pattern) and use
JsFileSource::js_module() to ensure it runs the same compare path as the other
tests, verifying execute and execute_optimized produce identical effects for the
negated where-clause.
In `@crates/biome_plugin_loader/src/lib.rs`:
- Around line 40-48: parse_globs currently drops invalid patterns silently;
change it to collect both parsed Globs and parse errors, emit a warning for each
malformed pattern (including the original pattern string and parse error) using
the crate logging/tracing facility, then return Some(valid_globs) or None as
before; locate the logic in parse_globs and adjust the iterator over patterns
(patterns.iter()) to match on p.parse() results so you can warn on Err and keep
Ok(Glob).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/biome_grit_patterns/Cargo.toml (1)
50-54: Cfg conditions here don't fully align with those in the bench file.The Cargo.toml guards jemalloc with
target_family="unix"(covers FreeBSD, OpenBSD, etc.) but the bench file's#[global_allocator]only fires fortarget_os = "macos"or"linux". On those other Unix targets the crate is compiled in but never wired up as the global allocator — a harmless no-op, but confusing.Consider aligning the conditions (e.g., using
target_family="unix"in both), or at minimum adding a comment explaining the intentional divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_grit_patterns/Cargo.toml` around lines 50 - 54, The Cargo.toml currently enables tikv-jemallocator under the cfg target_family="unix" while the bench file only installs it as the #[global_allocator] for target_os = "linux" or "macos"; align these guards to avoid compiling the allocator without using it or add an explanatory comment: either change the bench's #[global_allocator] cfg to target_family="unix" to match the Cargo.toml dev-dependency on tikv-jemallocator, or restrict the Cargo.toml entry to target_os = "linux" and "macos"; alternatively add a clear comment next to the tikv-jemallocator dev-dependency and the #[global_allocator] attribute explaining the intentional mismatch.crates/biome_grit_patterns/benches/grit_query.rs (1)
104-134: Parsing insideb.iter()inflates both measurements equally, masking the actual execute/execute_optimized delta.
biome_js_parser::parse+GritTargetFile::neware re-run on every benchmark iteration. If parsing dominates the iteration time, the difference you're trying to measure (execute vs execute_optimized) can become statistically invisible. Useiter_batchedto separate setup from the hot path:♻️ Proposed refactor using
iter_batched- group.bench_with_input(BenchmarkId::new("execute", name), &query, |b, query| { - b.iter(|| { - let parsed = biome_js_parser::parse( - JS_CODE, - JsFileSource::js_module(), - JsParserOptions::default(), - ); - let file = GritTargetFile::new("test.js", parsed.into()); - black_box(query.execute(file).expect("execute failed")); - }); - }); + group.bench_with_input(BenchmarkId::new("execute", name), &query, |b, query| { + b.iter_batched( + || { + let parsed = biome_js_parser::parse( + JS_CODE, + JsFileSource::js_module(), + JsParserOptions::default(), + ); + GritTargetFile::new("test.js", parsed.into()) + }, + |file| black_box(query.execute(file).expect("execute failed")), + criterion::BatchSize::SmallInput, + ); + });Apply the same pattern to the
execute_optimizedarm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_grit_patterns/benches/grit_query.rs` around lines 104 - 134, Parsing and file-creation are currently done inside the hot loop (group.bench_with_input -> b.iter) which masks the real difference between query.execute and query.execute_optimized; change both benchmark arms (the one using query.execute and the one using query.execute_optimized) to use b.iter_batched (or iter_batched_ref) so that biome_js_parser::parse(...) and GritTargetFile::new("test.js", ...) are done in the setup closure and only query.execute(file) / query.execute_optimized(file) run in the measurement closure, choosing an appropriate BatchSize so setup is not included in timing.
🤖 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_grit_patterns/Cargo.toml`:
- Line 46: Replace the floating dependency for codspeed-criterion-compat with a
pinned version: update the criterion dependency entry that currently uses
package = "codspeed-criterion-compat", version = "*" to specify the exact
version "4.1.0" so the Cargo.toml maps criterion -> codspeed-criterion-compat at
the pinned semver 4.1.0 instead of allowing any release.
---
Duplicate comments:
In `@crates/biome_grit_patterns/benches/grit_query.rs`:
- Around line 7-20: Multiple platform configurations leave gaps so no global
allocator is defined for some targets; consolidate the allocator selection into
a single, exhaustive cfg_if/#[cfg] block so exactly one static GLOBAL is defined
for every target. Update the file to cover all combinations by using a cfg_if!
or ordered #[cfg(...)] branches referencing the existing statics (GLOBAL,
mimalloc::MiMalloc, tikv_jemallocator::Jemalloc, std::alloc::System) and add a
fallback case (e.g., System) for any unmatched targets to ensure a single global
allocator is always compiled.
---
Nitpick comments:
In `@crates/biome_grit_patterns/benches/grit_query.rs`:
- Around line 104-134: Parsing and file-creation are currently done inside the
hot loop (group.bench_with_input -> b.iter) which masks the real difference
between query.execute and query.execute_optimized; change both benchmark arms
(the one using query.execute and the one using query.execute_optimized) to use
b.iter_batched (or iter_batched_ref) so that biome_js_parser::parse(...) and
GritTargetFile::new("test.js", ...) are done in the setup closure and only
query.execute(file) / query.execute_optimized(file) run in the measurement
closure, choosing an appropriate BatchSize so setup is not included in timing.
In `@crates/biome_grit_patterns/Cargo.toml`:
- Around line 50-54: The Cargo.toml currently enables tikv-jemallocator under
the cfg target_family="unix" while the bench file only installs it as the
#[global_allocator] for target_os = "linux" or "macos"; align these guards to
avoid compiling the allocator without using it or add an explanatory comment:
either change the bench's #[global_allocator] cfg to target_family="unix" to
match the Cargo.toml dev-dependency on tikv-jemallocator, or restrict the
Cargo.toml entry to target_os = "linux" and "macos"; alternatively add a clear
comment next to the tikv-jemallocator dev-dependency and the #[global_allocator]
attribute explaining the intentional mismatch.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/biome_grit_patterns/src/grit_query.rs (1)
190-201: Three bare.unwrap()s; prefer a singleexpect()(or one binding to the scope).All three dereferences through
last_mut()panic identically on a missing global scope. A single binding expresses intent and gives a useful panic message if things ever go wrong.♻️ Suggested tidy-up
- state.bindings[GLOBAL_VARS_SCOPE_INDEX as usize] - .last_mut() - .unwrap()[FILENAME_INDEX] - .value = Some(name_val); - state.bindings[GLOBAL_VARS_SCOPE_INDEX as usize] - .last_mut() - .unwrap()[PROGRAM_INDEX] - .value = Some(program_val); - state.bindings[GLOBAL_VARS_SCOPE_INDEX as usize] - .last_mut() - .unwrap()[ABSOLUTE_PATH_INDEX] - .value = Some(abs_path_val); + let global_scope = state.bindings[GLOBAL_VARS_SCOPE_INDEX as usize] + .last_mut() + .expect("global scope is always initialised"); + global_scope[FILENAME_INDEX].value = Some(name_val); + global_scope[PROGRAM_INDEX].value = Some(program_val); + global_scope[ABSOLUTE_PATH_INDEX].value = Some(abs_path_val);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_grit_patterns/src/grit_query.rs` around lines 190 - 201, The three identical .last_mut().unwrap() calls should be collapsed into a single binding with an explanatory expect() to avoid repeated panics; retrieve the mutable top scope once (e.g., let scope = state.bindings[GLOBAL_VARS_SCOPE_INDEX as usize].last_mut().expect("missing global vars scope")), then set scope[FILENAME_INDEX].value = Some(name_val), scope[PROGRAM_INDEX].value = Some(program_val), and scope[ABSOLUTE_PATH_INDEX].value = Some(abs_path_val) to replace the three unwrap usages in grit_query.rs.
🤖 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_grit_patterns/src/grit_query.rs`:
- Around line 589-598: The Sequential arm in extract_contains_inner only
inspects the first step (seq.first()) while extract_anchor_kinds scans all
steps, which creates an asymmetry; change the Sequential handling in
extract_contains_inner to iterate steps (e.g., seq.iter().find_map(...) calling
extract_contains_inner on each step.pattern) so it finds a Contains in any step
like extract_anchor_kinds does, and add a short comment near
extract_contains_inner and Pattern::Sequential noting the intended dependence on
traversal order/auto-wrap to avoid future regressions.
- Around line 541-543: The Or/Any arms currently flat_map the branch results
which silently drops branches that return [] (meaning "universal"); change
Pattern::Or and Pattern::Any handling to iterate each branch calling
extract_anchor_kinds and if any branch returns an empty Vec then return an empty
Vec immediately (propagate the universal constraint), otherwise union the
non-empty kind sets (deduplicate) and return that union; keep Pattern::And
as-is. Reference: modify the Pattern::Or / Pattern::Any match arms in
grit_query.rs and use extract_anchor_kinds and the same kind-union logic used
elsewhere so execute_optimized sees an empty anchor_kinds when a branch is
universal.
---
Nitpick comments:
In `@crates/biome_grit_patterns/src/grit_query.rs`:
- Around line 190-201: The three identical .last_mut().unwrap() calls should be
collapsed into a single binding with an explanatory expect() to avoid repeated
panics; retrieve the mutable top scope once (e.g., let scope =
state.bindings[GLOBAL_VARS_SCOPE_INDEX as usize].last_mut().expect("missing
global vars scope")), then set scope[FILENAME_INDEX].value = Some(name_val),
scope[PROGRAM_INDEX].value = Some(program_val), and
scope[ABSOLUTE_PATH_INDEX].value = Some(abs_path_val) to replace the three
unwrap usages in grit_query.rs.
Contributor
Author
|
Hey @arendjr @ematipico @siketyan — would love your eyes on this when you get a chance. This tackles the plugin execution overhead that shows up when you load many GritQL plugins (related: #6210, #7020, #2463). Three changes:
Benchmarks in the PR description show 5-8x per-query speedup from anchor dispatch alone. Real-world: 84 plugins on ~400 files went from ~25s to ~2s. @arendjr you built most of the plugin architecture and GritQL bindings so your review would be especially valuable here. @ematipico the config/loading changes touch your area. @siketyan the visitor dispatch rework builds on your Website docs PR: biomejs/website#3993 |
This was referenced Feb 21, 2026
Merging this PR will not alter performance
Comparing 
Footnotes
|
dyc3
requested changes
Feb 21, 2026
Member
|
I agree with @dyc3 that we could land the perf part in a separate PR as a patch! It doesn't stop us :) |
dyc3
requested changes
Feb 21, 2026
chocky335
force-pushed
the
perf/plugin-anchor-dispatch
branch
from
1f48d98 to
3a22a85
Compare
chocky335
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/biome_plugin_loader/src/configuration.rs (1)
171-189: Consider asserting the actual glob value is preserved, not justis_some().Line 188 checks
is_some()but doesn't verify the includes content survived normalisation unchanged. A small tightening:🧪 Suggested assertion
// includes should be unchanged - assert!(plugins.0[0].includes().is_some()); + let includes = plugins.0[0].includes().expect("includes should be present"); + assert_eq!(includes.len(), 1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/configuration.rs` around lines 171 - 189, The test normalize_relative_paths_with_options currently only asserts includes() is Some() but doesn't verify the actual glob was preserved; update the test for Plugins/PluginConfiguration::PathWithOptions (PluginWithOptions) to assert the includes vector's content equals the original glob (e.g., the first includes entry matches "src/**/*.ts" or the same parsed Glob) so the normalization step leaves includes unchanged.crates/biome_plugin_loader/src/lib.rs (1)
68-81: Note: JS plugins don't receiveincludes— intentional but worth a TODO?
AnalyzerJsPlugin::loadat line 74 doesn't acceptincludes. The defaultapplies_to_filereturningtruemeans file scoping silently doesn't apply to JS plugins. If this is intentional and deferred, a brief comment would save future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/lib.rs` around lines 68 - 81, The JS plugin loader currently calls AnalyzerJsPlugin::load without passing any file-scoping "includes", so JS plugins never receive includes and rely on the default applies_to_file() == true; either update AnalyzerJsPlugin::load signature and its caller here to accept and forward the includes (and ensure the resulting AnalyzerPlugin implementation uses includes in applies_to_file), or if this behavior is intentional, add a concise clarifying comment next to the AnalyzerJsPlugin::load call (and/or on AnalyzerJsPlugin::load) stating that JS plugins do not support includes/scoping and why; reference AnalyzerJsPlugin::load, applies_to_file, and the analyzer_plugins construction in this block when making the change.crates/biome_plugin_loader/src/analyzer_grit_plugin.rs (1)
83-88: Tiny nit: double reference on line 87.
pathis already&Utf8Path, so&pathis&&Utf8Path. Works fine via auto-deref but reads oddly.✏️ Nit
- CandidatePath::new(&path).matches_with_exceptions(includes) + CandidatePath::new(path).matches_with_exceptions(includes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/analyzer_grit_plugin.rs` around lines 83 - 88, In applies_to_file, remove the unnecessary double reference when constructing CandidatePath: pass path directly to CandidatePath::new instead of &path so you don't create a &&Utf8Path; keep the rest of the logic (applies_to_file, includes check, and matches_with_exceptions call) unchanged.
🤖 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_analyze/src/analyzer_plugin.rs`:
- Around line 110-112: The applies_to_file check is being recomputed for every
matching node; cache it on PluginVisitor instead. Add a field (e.g.,
file_applicable: Option<bool>) to PluginVisitor and set it once at the start of
traversal (or lazily on first visit) by calling
self.plugin.applies_to_file(&ctx.options.file_path), then in visit use that
cached boolean instead of calling applies_to_file repeatedly; update any
references to self.plugin.applies_to_file(...) to read from the cached field
(ensure the cache is initialized before querying child nodes).
---
Nitpick comments:
In `@crates/biome_plugin_loader/src/analyzer_grit_plugin.rs`:
- Around line 83-88: In applies_to_file, remove the unnecessary double reference
when constructing CandidatePath: pass path directly to CandidatePath::new
instead of &path so you don't create a &&Utf8Path; keep the rest of the logic
(applies_to_file, includes check, and matches_with_exceptions call) unchanged.
In `@crates/biome_plugin_loader/src/configuration.rs`:
- Around line 171-189: The test normalize_relative_paths_with_options currently
only asserts includes() is Some() but doesn't verify the actual glob was
preserved; update the test for Plugins/PluginConfiguration::PathWithOptions
(PluginWithOptions) to assert the includes vector's content equals the original
glob (e.g., the first includes entry matches "src/**/*.ts" or the same parsed
Glob) so the normalization step leaves includes unchanged.
In `@crates/biome_plugin_loader/src/lib.rs`:
- Around line 68-81: The JS plugin loader currently calls AnalyzerJsPlugin::load
without passing any file-scoping "includes", so JS plugins never receive
includes and rely on the default applies_to_file() == true; either update
AnalyzerJsPlugin::load signature and its caller here to accept and forward the
includes (and ensure the resulting AnalyzerPlugin implementation uses includes
in applies_to_file), or if this behavior is intentional, add a concise
clarifying comment next to the AnalyzerJsPlugin::load call (and/or on
AnalyzerJsPlugin::load) stating that JS plugins do not support includes/scoping
and why; reference AnalyzerJsPlugin::load, applies_to_file, and the
analyzer_plugins construction in this block when making the change.
0a4bbc9 to
1bd549c
Compare
chocky335
force-pushed
the
perf/plugin-anchor-dispatch
branch
from
1bd549c to
2960a3b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/biome_plugin_loader/src/lib.rs (1)
45-50: LGTM — clean API extension.The
includesparameter is correctly threaded to both grit and JS plugin loaders. One minor doc note: worth calling out in the/// The optional includes patterns...comment that an emptyincludes: Some(&[])means the plugin never runs on any file (since there are no positive patterns to match). Easy to accidentally misconfigure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/lib.rs` around lines 45 - 50, Update the doc comment for the load function's includes parameter to explicitly state that passing Some(&[]) (an empty slice) results in no positive include patterns and therefore the plugin will never run on any file; locate the comment above pub fn load (the /// The optional includes patterns... doc) and add a short sentence clarifying that an empty Some indicates "no files matched" behavior to prevent accidental misconfiguration.crates/biome_plugin_loader/src/analyzer_js_plugin.rs (2)
87-92: Optional: Extract the duplicatedapplies_to_filebody into a crate-local helper.The implementation here is byte-for-byte identical to
AnalyzerGritPlugin::applies_to_file. Since both types live in the same crate, a small free function would eliminate the duplication.♻️ Suggested helper (e.g. in `lib.rs` or a new `util.rs`)
+pub(crate) fn file_matches_includes(includes: &Option<Box<[NormalizedGlob]>>, path: &Utf8Path) -> bool { + match includes { + None => true, + Some(globs) => CandidatePath::new(path).matches_with_exceptions(globs), + } +}Then both plugins reduce to:
fn applies_to_file(&self, path: &Utf8Path) -> bool { - let Some(includes) = &self.includes else { - return true; - }; - CandidatePath::new(path).matches_with_exceptions(includes) + file_matches_includes(&self.includes, path) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/analyzer_js_plugin.rs` around lines 87 - 92, applies_to_file implementations in AnalyzerJsPlugin and AnalyzerGritPlugin are identical; extract the shared logic into a crate-local helper function (e.g. fn applies_to_path(path: &Utf8Path, includes: &Option<...>) -> bool) and have AnalyzerJsPlugin::applies_to_file and AnalyzerGritPlugin::applies_to_file call that helper; the helper should perform the same early-return when includes is None and delegate to CandidatePath::new(path).matches_with_exceptions(includes) so you only keep the unique type impls and remove the duplicated bodies.
140-215: Consider addingapplies_to_fileunit tests forAnalyzerJsPlugin.
AnalyzerGritPluginships with four dedicated tests covering theNone, matching, non-matching, and negated-glob cases.AnalyzerJsPluginhas none — which is a shame given the caching thatPluginVisitorwill do based on this result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/analyzer_js_plugin.rs` around lines 140 - 215, Add unit tests for AnalyzerJsPlugin that exercise the applies_to_file behavior (covering None, matching glob, non-matching glob, and negated-glob cases) similar to AnalyzerGritPlugin tests; locate and load plugins via AnalyzerJsPlugin::load (using MemoryFileSystem/Arc as in existing tests), then call AnalyzerJsPlugin::applies_to_file (or PluginVisitor path-resolution if needed) with appropriate file paths to assert true/false outcomes and ensure caching-relevant behavior is validated by invoking applies_to_file multiple times to confirm stable results.
🤖 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_plugin_loader/src/analyzer_grit_plugin.rs`:
- Around line 220-241: Add tests that verify applies_to_file correctly handles
absolute paths: update the existing test cases in analyzer_grit_plugin.rs (the
tests using load_test_plugin and applies_to_file with NormalizedGlob patterns)
to include absolute paths like "/project/src/main.ts" and "/project/test/foo.ts"
for the matching and non-matching scenarios respectively; ensure the
applies_to_file assertions mirror the relative-path assertions but pass absolute
Utf8Path values so the glob "src/**/*.ts" is validated against absolute inputs
as well.
---
Duplicate comments:
In `@crates/biome_plugin_loader/src/analyzer_grit_plugin.rs`:
- Around line 83-88: The applies_to_file implementation in AnalyzerGritPlugin
duplicates logic from AnalyzerJsPlugin::applies_to_file; refactor by extracting
the common behavior into a shared helper (e.g., a method or free function) that
takes &Utf8Path and Option<&IncludesType> and returns bool, then have
AnalyzerGritPlugin::applies_to_file and AnalyzerJsPlugin::applies_to_file call
that helper instead of duplicating the
CandidatePath::new(path).matches_with_exceptions(includes) +
early-return-on-None logic; update references to the includes variable and
CandidatePath::new usage to use the shared helper.
---
Nitpick comments:
In `@crates/biome_plugin_loader/src/analyzer_js_plugin.rs`:
- Around line 87-92: applies_to_file implementations in AnalyzerJsPlugin and
AnalyzerGritPlugin are identical; extract the shared logic into a crate-local
helper function (e.g. fn applies_to_path(path: &Utf8Path, includes:
&Option<...>) -> bool) and have AnalyzerJsPlugin::applies_to_file and
AnalyzerGritPlugin::applies_to_file call that helper; the helper should perform
the same early-return when includes is None and delegate to
CandidatePath::new(path).matches_with_exceptions(includes) so you only keep the
unique type impls and remove the duplicated bodies.
- Around line 140-215: Add unit tests for AnalyzerJsPlugin that exercise the
applies_to_file behavior (covering None, matching glob, non-matching glob, and
negated-glob cases) similar to AnalyzerGritPlugin tests; locate and load plugins
via AnalyzerJsPlugin::load (using MemoryFileSystem/Arc as in existing tests),
then call AnalyzerJsPlugin::applies_to_file (or PluginVisitor path-resolution if
needed) with appropriate file paths to assert true/false outcomes and ensure
caching-relevant behavior is validated by invoking applies_to_file multiple
times to confirm stable results.
In `@crates/biome_plugin_loader/src/lib.rs`:
- Around line 45-50: Update the doc comment for the load function's includes
parameter to explicitly state that passing Some(&[]) (an empty slice) results in
no positive include patterns and therefore the plugin will never run on any
file; locate the comment above pub fn load (the /// The optional includes
patterns... doc) and add a short sentence clarifying that an empty Some
indicates "no files matched" behavior to prevent accidental misconfiguration.
Contributor
Author
|
Addressed the latest CodeRabbit review items in c3ba15f and 555fc38:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/biome_plugin_loader/src/lib.rs (1)
131-143: Consider usingOption<&[NormalizedGlob]>instead of&Option<Box<[NormalizedGlob]>>.The parameter type
&Option<Box<[NormalizedGlob]>>works but is mildly unidiomatic in Rust. The more conventional signature would beOption<&[NormalizedGlob]>, with callers passingself.includes.as_deref(). This avoids borrowing theOptionwrapper itself and is the pattern Clippy'sclippy::ref_optionlint encourages.Not a blocker — entirely up to you.
♻️ Suggested refactor
-pub(crate) fn file_matches_includes( - includes: &Option<Box<[NormalizedGlob]>>, - path: &Utf8Path, -) -> bool { - let Some(includes) = includes else { +pub(crate) fn file_matches_includes( + includes: Option<&[NormalizedGlob]>, + path: &Utf8Path, +) -> bool { + let Some(includes) = includes else { return true; }; CandidatePath::new(path).matches_with_exceptions(includes) }Then at call sites (e.g. in
analyzer_grit_plugin.rsandanalyzer_js_plugin.rs):file_matches_includes(self.includes.as_deref(), path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/lib.rs` around lines 131 - 143, The function signature file_matches_includes currently takes &Option<Box<[NormalizedGlob]>> which is unidiomatic; change it to accept Option<&[NormalizedGlob]> and update callers to pass self.includes.as_deref() (e.g., where analyzer_grit_plugin.rs and analyzer_js_plugin.rs call file_matches_includes) so the function becomes file_matches_includes(includes: Option<&[NormalizedGlob]>, path: &Utf8Path) and continues to delegate to CandidatePath::new(path).matches_with_exceptions(includes) (adjust matches_with_exceptions to accept the same slice reference if necessary).crates/biome_service/src/workspace.tests.rs (1)
844-955: Consider adding a negated-glob branch to cover the exclusion half of the feature.The PR's headline example is
"includes": ["src/**/*.ts", "!**/*.test.ts"]. This test only exercises the positive-match case. The plugin-loader unit tests may already cover negation, but an end-to-end workspace test that asserts a file matching the base glob but excluded by a!pattern receives zero diagnostics would give meaningful confidence from the top of the stack.Happy to draft the extra case if you'd like — just say the word.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/workspace.tests.rs` around lines 844 - 955, Add a negated-glob branch to the existing correctly_scope_plugin_with_includes test: modify the PluginWithOptions includes (the vector passed to Plugins/PluginConfiguration::PathWithOptions) to include a negation like "!**/*.test.ts" alongside the existing "**/src/**/*.ts", add a file under "/project/src" that matches the positive glob but is excluded (e.g., "/project/src/foo.test.ts" with the same Object.assign usage), and add an assertion case in the final loop that expects 0 plugin diagnostics for that test-file path (use the same OpenFileParams / pull_diagnostics flow and the same plugin_diagnostics filtering used in the test).
🤖 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_plugin_loader/src/lib.rs`:
- Around line 131-143: The function signature file_matches_includes currently
takes &Option<Box<[NormalizedGlob]>> which is unidiomatic; change it to accept
Option<&[NormalizedGlob]> and update callers to pass self.includes.as_deref()
(e.g., where analyzer_grit_plugin.rs and analyzer_js_plugin.rs call
file_matches_includes) so the function becomes file_matches_includes(includes:
Option<&[NormalizedGlob]>, path: &Utf8Path) and continues to delegate to
CandidatePath::new(path).matches_with_exceptions(includes) (adjust
matches_with_exceptions to accept the same slice reference if necessary).
In `@crates/biome_service/src/workspace.tests.rs`:
- Around line 844-955: Add a negated-glob branch to the existing
correctly_scope_plugin_with_includes test: modify the PluginWithOptions includes
(the vector passed to Plugins/PluginConfiguration::PathWithOptions) to include a
negation like "!**/*.test.ts" alongside the existing "**/src/**/*.ts", add a
file under "/project/src" that matches the positive glob but is excluded (e.g.,
"/project/src/foo.test.ts" with the same Object.assign usage), and add an
assertion case in the final loop that expects 0 plugin diagnostics for that
test-file path (use the same OpenFileParams / pull_diagnostics flow and the same
plugin_diagnostics filtering used in the test).
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The implementation is solid. Nice work tightening up the API surface and test coverage! 🚀 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/biome_plugin_loader/src/analyzer_js_plugin.rs (2)
57-62:Debugimpl omitsincludes— fine for now, but worth noting.
finish_non_exhaustive()hides theincludesfield from debug output. If someone's debugging why a plugin isn't running on a file, seeing the globs would help. Not blocking, just something to keep in mind.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/analyzer_js_plugin.rs` around lines 57 - 62, The Debug implementation for AnalyzerJsPlugin currently hides the includes field by calling finish_non_exhaustive(); update the fmt method in the Debug impl for AnalyzerJsPlugin to include the includes field (e.g., add .field("includes", &self.includes) and use .finish() or otherwise ensure includes is part of the debug output) so globs are visible when debugging.
161-195: Tests are solid. Consider adding negated-glob coverage for parity with grit plugin tests.The grit plugin has
applies_with_negated_glob_exclusionandglob_does_not_match_absolute_paths_without_prefixtests. Since both plugins sharefile_matches_includes, it's not strictly necessary to duplicate them, but it wouldn't hurt for completeness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/analyzer_js_plugin.rs` around lines 161 - 195, Add two unit tests to mirror grit plugin coverage: create a test (e.g., applies_with_negated_glob_exclusion) that calls load_test_plugin(Some(&globs)) where globs includes a positive pattern like "src/**/*.ts" and a negated pattern like "!src/nested/**" and assert that AnalyzerJsPlugin::applies_to_file returns true for "src/main.ts" but false for "src/nested/file.ts"; and add a test (e.g., glob_does_not_match_absolute_paths_without_prefix) that verifies applies_to_file on an absolute path without the expected prefix does not match, ensuring you exercise load_test_plugin, AnalyzerJsPlugin::load, and applies_to_file to validate file_matches_includes behavior with negations and absolute paths.crates/biome_service/src/workspace.tests.rs (1)
960-963: Minor: snapshot name includes/characters which may cause issues on some systems.The
format!("scoped_plugin_diagnostics_{path}")at line 961 will produce snapshot names likescoped_plugin_diagnostics_/project/src/foo.ts. Theinstacrate typically handles this, but path separators in snapshot names can be surprising. The existing test at line 838 uses the same pattern, so this is at least consistent with the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/workspace.tests.rs` around lines 960 - 963, The snapshot name currently uses format!("scoped_plugin_diagnostics_{path}") which can include path separators; update the naming to sanitize the path before formatting (e.g., replace path separators with underscores or use path.file_name() if appropriate) so snapshot_name contains no '/' characters; locate the code around expect_diagnosis_count, snapshot_name, and plugin_diagnostics and apply the sanitization when building snapshot_name.crates/biome_plugin_loader/src/analyzer_grit_plugin.rs (1)
206-261: Good test coverage — consider adding a test forSome(&[])(empty includes).The doc comment on
BiomePlugin::load(line 45 oflib.rs) explicitly states thatSome(&[])means the plugin never matches any file. It'd be nice to have a unit test confirming this edge case, since it's a subtle semantic distinction fromNone.💡 Suggested test
+ #[test] + fn empty_includes_matches_nothing() { + let globs: Vec<NormalizedGlob> = vec![]; + let plugin = load_test_plugin(Some(&globs)); + assert!(!plugin.applies_to_file(Utf8Path::new("src/main.ts"))); + assert!(!plugin.applies_to_file(Utf8Path::new("any/file.js"))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/analyzer_grit_plugin.rs` around lines 206 - 261, Add a unit test in the existing tests module that calls AnalyzerGritPlugin::load with Some(&[]) (an empty slice of NormalizedGlob) and asserts that plugin.applies_to_file(...) returns false for representative relative and absolute paths; this verifies the documented behavior from BiomePlugin::load that Some(&[]) means “never match.” Locate the test alongside the other applies_to_* tests in the tests mod and use load_test_plugin(Some(&[])) to create the plugin and then assert !plugin.applies_to_file(Utf8Path::new("src/main.ts")) and another absolute path check.
🤖 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_plugin_loader/src/analyzer_grit_plugin.rs`:
- Around line 206-261: Add a unit test in the existing tests module that calls
AnalyzerGritPlugin::load with Some(&[]) (an empty slice of NormalizedGlob) and
asserts that plugin.applies_to_file(...) returns false for representative
relative and absolute paths; this verifies the documented behavior from
BiomePlugin::load that Some(&[]) means “never match.” Locate the test alongside
the other applies_to_* tests in the tests mod and use
load_test_plugin(Some(&[])) to create the plugin and then assert
!plugin.applies_to_file(Utf8Path::new("src/main.ts")) and another absolute path
check.
In `@crates/biome_plugin_loader/src/analyzer_js_plugin.rs`:
- Around line 57-62: The Debug implementation for AnalyzerJsPlugin currently
hides the includes field by calling finish_non_exhaustive(); update the fmt
method in the Debug impl for AnalyzerJsPlugin to include the includes field
(e.g., add .field("includes", &self.includes) and use .finish() or otherwise
ensure includes is part of the debug output) so globs are visible when
debugging.
- Around line 161-195: Add two unit tests to mirror grit plugin coverage: create
a test (e.g., applies_with_negated_glob_exclusion) that calls
load_test_plugin(Some(&globs)) where globs includes a positive pattern like
"src/**/*.ts" and a negated pattern like "!src/nested/**" and assert that
AnalyzerJsPlugin::applies_to_file returns true for "src/main.ts" but false for
"src/nested/file.ts"; and add a test (e.g.,
glob_does_not_match_absolute_paths_without_prefix) that verifies applies_to_file
on an absolute path without the expected prefix does not match, ensuring you
exercise load_test_plugin, AnalyzerJsPlugin::load, and applies_to_file to
validate file_matches_includes behavior with negations and absolute paths.
In `@crates/biome_service/src/workspace.tests.rs`:
- Around line 960-963: The snapshot name currently uses
format!("scoped_plugin_diagnostics_{path}") which can include path separators;
update the naming to sanitize the path before formatting (e.g., replace path
separators with underscores or use path.file_name() if appropriate) so
snapshot_name contains no '/' characters; locate the code around
expect_diagnosis_count, snapshot_name, and plugin_diagnostics and apply the
sanitization when building snapshot_name.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_plugin_loader/src/analyzer_grit_plugin.rs (1)
27-29: Minor doc gap:Some(&[])edge-case worth a one-liner.The comment documents the
Nonearm but silently skips theSome([])case. The PR description explicitly called this out, and there's already a test for it — a single added sentence makes the field self-explaining for future readers.📝 Suggested doc addition
/// Glob patterns that restrict which files this plugin runs on. /// `None` means the plugin runs on all files. + /// `Some(&[])` (an empty list) means the plugin never runs on any file. includes: Option<Box<[NormalizedGlob]>>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/analyzer_grit_plugin.rs` around lines 27 - 29, The doc comment for the includes field (includes: Option<Box<[NormalizedGlob]>>) currently documents the None case but omits the Some([]) edge-case; update the comment for the includes field to add a one-line note that Some(&[]) (an empty slice of NormalizedGlob) means the plugin is restricted to no files (i.e., matches nothing), so readers understand that None means "all files" while Some(empty) means "no files."
🤖 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_plugin_loader/src/analyzer_grit_plugin.rs`:
- Around line 27-29: The doc comment for the includes field (includes:
Option<Box<[NormalizedGlob]>>) currently documents the None case but omits the
Some([]) edge-case; update the comment for the includes field to add a one-line
note that Some(&[]) (an empty slice of NormalizedGlob) means the plugin is
restricted to no files (i.e., matches nothing), so readers understand that None
means "all files" while Some(empty) means "no files."
Contributor
|
Tip For best results, initiate chat on the files or code changes.
All four points are solid. Appreciate you adding that edge-case coverage! 🚀 🧠 Learnings used |
ematipico
requested changes
Feb 23, 2026
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_plugin_loader/src/lib.rs (1)
131-140: Consider pinning the empty-includes contract with a tiny unit test.You document that
Some(&[])should match nothing; a direct test here would make that guarantee explicit and future-proof.Proposed test addition
#[cfg(test)] mod test { use super::*; @@ + #[test] + fn empty_includes_match_nothing() { + let includes: Vec<NormalizedGlob> = vec![]; + assert!(!file_matches_includes( + Some(&includes), + Utf8Path::new("src/main.ts"), + )); + } + #[test] fn load_plugin() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_plugin_loader/src/lib.rs` around lines 131 - 140, Add a tiny unit test to pin the documented contract: inside the crate tests (or a #[cfg(test)] mod) add a test that calls file_matches_includes(Some(&[]), &Utf8Path::new("some/path")) and asserts it returns false, and optionally assert file_matches_includes(None, &Utf8Path::new("some/path")) returns true; this ensures the behavior of file_matches_includes, CandidatePath::new and CandidatePath::matches_with_exceptions is explicitly verified.
🤖 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_plugin_loader/src/lib.rs`:
- Around line 131-140: Add a tiny unit test to pin the documented contract:
inside the crate tests (or a #[cfg(test)] mod) add a test that calls
file_matches_includes(Some(&[]), &Utf8Path::new("some/path")) and asserts it
returns false, and optionally assert file_matches_includes(None,
&Utf8Path::new("some/path")) returns true; this ensures the behavior of
file_matches_includes, CandidatePath::new and
CandidatePath::matches_with_exceptions is explicitly verified.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (3)
crates/biome_plugin_loader/src/analyzer_js_plugin.rscrates/biome_plugin_loader/src/lib.rscrates/biome_service/src/workspace.tests.rs
Contributor
Author
|
The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add
includesoption to plugin configuration, allowing plugins to be scoped to specific files using glob patterns. Negated globs serve as exclusions, matching the existing Biome convention.Configuration
Plugins can now be specified as either a plain path (existing behavior) or an object with path and includes:
{ "plugins": [ "global-plugin.grit", { "path": "ts-only-plugin.grit", "includes": ["src/**/*.ts", "!**/*.test.ts"] } ] }Changes
PluginConfiguration— Extended enum withPathWithOptionsvariant containingpathand optionalincludes: Vec<NormalizedGlob>.AnalyzerPlugin::applies_to_file()— New trait method (defaulttrue) that checks whether a plugin should run on a given file.AnalyzerGritPlugin— Stores includes and implementsapplies_to_file()usingCandidatePath::matches_with_exceptions().PluginVisitor— Cachesapplies_to_fileresult per visitor so the glob match runs once per file, not per node.BiomePlugin::load()— Threads includes through plugin loading.PluginWithOptionstype.Related
Test Plan
PluginConfiguration: plain string, object with/without includes, mixed listapplies_to_fileunit tests: no includes (applies to all), matching files, non-matching filesnormalize_relative_pathstests includingPathWithOptionsvariantcargo clippyclean across all affected cratescargo codegen-schemaDocs
Website documentation: biomejs/website#3993