feat(biome_service): support applying GritQL plugin rewrites via --write#9073
feat(biome_service): support applying GritQL plugin rewrites via --write#9073chocky335 wants to merge 32 commits intobiomejs:nextfrom
Conversation
🦋 Changeset detectedLatest commit: 11f2bc5 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-CLI
WalkthroughAdds support for fixable GritQL plugin rewrites. Introduces public types 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)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/biome_grit_patterns/src/linearization.rs`:
- Around line 47-50: The code in linearize_binding uses expect("binding should
have a range") on binding.range(language), which can legitimately be None for
some Binding variants (e.g. File or Constant); replace the panicking expect with
proper error propagation by returning a GritResult::Err when range is None.
Specifically, in function linearize_binding locate the two places that call
binding.range(language) and instead handle the Option by using ok_or_else(...)
or match to produce a descriptive GritError (or use ? after converting with
.ok_or(...)), then use the obtained byte_range for
replacements.push((byte_range.start, byte_range.end, cached_text.clone())).
Ensure both occurrences (the one around replacements.push((byte_range.start,
byte_range.end, cached_text.clone())) and the second occurrence later in the
function) are changed to propagate a GritResult error instead of panicking.
In `@crates/biome_service/src/file_handlers/mod.rs`:
- Around line 845-853: record_text_edit_fix currently ignores the boolean
returned by growth_guard.check(), so runaway plugin rewrites can bypass the same
conflict detection used in process_action; update record_text_edit_fix to call
growth_guard.check(new_text_len) and handle a false result the same way as
process_action (e.g., return a Result and propagate/return a
ConflictingRuleFixesError or run the same error path), or perform the check
before pushing the FixAction and abort (return Err) when check returns false;
reference the function names record_text_edit_fix and growth_guard.check (and
mirror process_action's error handling) so the file growth guard behavior is
consistent.
🧹 Nitpick comments (7)
crates/biome_cli/tests/commands/check.rs (1)
3489-3531: Missing result assertion.The other two new tests assert
result.is_ok(), but this one silently drops the result status. Since the plugin emits a warning-level diagnostic and no--writeis passed, it would be good to assert the expected outcome (likelyis_err) for clarity and to catch regressions.Proposed fix
let (fs, result) = run_cli_with_server_workspace( fs, &mut console, Args::from(["check", file_path.as_str()].as_slice()), ); + assert!(result.is_err(), "run_cli returned {result:?}"); assert_file_contents(&fs, file_path, "console.log(\"hello\");\n");crates/biome_plugin_loader/src/analyzer_grit_plugin.rs (2)
74-99: Consider extracting the duplicated closure into a helper.The three match arms perform identical work (grab range + text + send-node), differing only in the downcast type. This is a pre-existing pattern, but the new fields make the duplication more visible.
Not blocking — just a future readability win.
140-146: All rewrite effects are converted to actions — even if the diagnostic has no span.If a Grit pattern produces a rewrite effect but
register_diagnosticwas never called (or was called without a span), the action still gets created with the root-levelsource_range. This seems intentional for the current design, but worth noting that the action'ssource_rangealways covers the entire root node, not the matched sub-expression.crates/biome_analyze/src/analyzer_plugin.rs (1)
127-153: Each diagnostic signal receives a clone of all actions — O(N×M) duplication.When a plugin produces N diagnostics and M rewrite effects, every signal gets all M actions cloned onto it. For 3
console.logmatches this means 3 signals × 3 actions = 9AnalyzerActions emitted, even though only 3 are meaningful.The fix-all loop and no-op guard likely handle this correctly, but:
- It's wasteful (N full
Vecclones).- It can confuse action counts in diagnostics/UX.
A future improvement could pair each diagnostic with its corresponding action(s) — e.g. by correlating the diagnostic span with the rewrite's source range.
Not blocking this PR, but worth tracking.
crates/biome_analyze/src/signals.rs (1)
188-196: Good doc comment ontext_edit.One small note: the
unwrap_or_default()at the end of theor_elsechain (lines 229 and 243) means a completely empty(TextRange::default(), TextEdit::default())is produced if neither path yields a result. This is a safe fallback but could mask bugs where an action was expected to carry edits. Probably fine for now — just flagging.crates/biome_grit_patterns/src/linearization.rs (1)
84-91: Overlapping or out-of-order replacements silently produce garbled output.If two replacements overlap (i.e.
*start < cursorafter processing the previous replacement), the gap copy is silently skipped but the replacement text is still appended, leading to corrupted output. Consider adding a diagnostic or assertion when*start < cursor.Defensive guard
for (start, end, replacement) in &replacements { + if *start < cursor { + // Overlapping replacements — skip or error + continue; + } // Copy the gap from cursor to this replacement. if *start > cursor { result.push_str(&source[cursor..*start]); }crates/biome_grit_patterns/src/grit_resolved_pattern.rs (1)
593-671:linearized_text()— clean delegation across all variants.One small observation: the
Listvariant joins with","(no space) on line 636, while theMapvariant joins with", "(with space) on line 653. Thetext()method (line 772) also uses","for lists and", "for maps, so this is at least internally consistent. Just flagging in case the comma-only separator for lists was unintentional.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_service/src/file_handlers/mod.rs`:
- Around line 845-872: record_text_edit_fix currently pushes FixAction with
rule_name: None which makes the ConflictingRuleFixesError list empty when recent
actions are all plugin edits; update record_text_edit_fix (and the FixAction
creation) or the error-collection logic so the error preserves context: either
set a synthetic rule_name when creating the FixAction (e.g.,
Some("plugin:<plugin-id>" or Some("plugin:anonymous")) so these edits appear in
the reversed .iter().rev().filter_map(...) collection, or keep rule_name as None
but change the error construction after growth_guard.check to also count
anonymous actions and include a summary string like "plugin_edits:N" in the
rules vector passed to ConflictingRuleFixesError (modify the block that builds
rules from self.actions.iter().rev().take(10) to collect both Some(rule_name)
values and a count of None entries and include that synthetic entry in the
resulting Vec).
🧹 Nitpick comments (2)
crates/biome_grit_patterns/src/linearization.rs (2)
84-91: Defensive guard for overlapping replacements.If two replacements ever overlap (i.e.
*start < cursor), the current loop silently stitches garbled text.get_top_level_effectsshould prevent this by contract, but adebug_assert!or skip would be cheap insurance against subtle bugs during future refactors.Suggested defensive check
for (start, end, replacement) in &replacements { - // Copy the gap from cursor to this replacement. - if *start > cursor { + // Skip overlapping replacements (should not happen with top-level effects). + if *start < cursor { + debug_assert!(false, "overlapping replacements detected: start={start}, cursor={cursor}"); + continue; + } + if *start > cursor { result.push_str(&source[cursor..*start]); }
11-16: Consider extracting args into a struct to avoid thetoo_many_argumentssuppression.Seven parameters is a lot to thread around. A small context struct (e.g.
LinearizationContext { language, effects, files, memo, logs }) would tidy call sites and remove the need for#[expect(clippy::too_many_arguments)]. No rush — just something to keep in mind if the signature grows further.
…r plugin rewrites
…ts in linearization
Merging this PR will not alter performance
Comparing 
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/biome_grit_patterns/src/linearization.rs (1)
122-122:source.len() as u32silently truncates for files > 4 GiB.Unlikely to matter in practice, but a
u32::try_fromwith a propagated error (or at least adebug_assert) would make the invariant explicit.♻️ Suggested change
- let range = CodeRange::new(0, source.len() as u32, source); + let len = u32::try_from(source.len()).map_err(|_| { + GritPatternError::new("source file too large for GritQL linearisation (> 4 GiB)") + })?; + let range = CodeRange::new(0, len, source);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_grit_patterns/src/linearization.rs` at line 122, The current cast in the CodeRange construction silently truncates large files; replace `source.len() as u32` with a checked conversion using `u32::try_from(source.len())` and propagate the error (or return a Result) to avoid silent truncation—i.e., compute `let len = u32::try_from(source.len())?; let range = CodeRange::new(0, len, source);` (if changing the signature isn’t feasible, at minimum add `debug_assert!(source.len() <= u32::MAX as usize)` before calling `CodeRange::new` to make the invariant explicit).
🤖 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/linearization.rs`:
- Around line 87-96: The debug_assert! in linearization that currently checks
"*start >= cursor" must be turned into a runtime error so overlapping
replacements fail in release builds: replace the debug_assert! with an explicit
check (e.g., if *start < cursor) that returns a GritResult::Err (or the crate's
appropriate GritError) with a clear message including start and cursor; keep the
subsequent copying logic (the if *start > cursor branch,
result.push_str(replacement), and cursor = *end) unchanged so normal
non-overlapping behavior remains intact.
- Around line 44-59: The debug_assert that checks for overlapping replacements
should be replaced with a hard error return: when you detect an overlap (i.e.,
*start < cursor) in the code that builds `replacements` and moves `cursor`,
return a GritResult::Err (construct a GritPatternError with a clear message
about overlapping replacements and include the affected byte ranges) instead of
relying on debug-only asserts so release builds don't silently drop bytes; also
make the cycle-detection sentinel robust by handling memo.get(br) == Some(None)
explicitly—when you see Some(None) (the in-progress marker inserted by
`memo.insert(br.clone(), None)`), `continue` to short-circuit recursion rather
than relying on `get_top_level_effects` to filter it out so you avoid re-entry
and potential infinite recursion.
---
Nitpick comments:
In `@crates/biome_grit_patterns/src/linearization.rs`:
- Line 122: The current cast in the CodeRange construction silently truncates
large files; replace `source.len() as u32` with a checked conversion using
`u32::try_from(source.len())` and propagate the error (or return a Result) to
avoid silent truncation—i.e., compute `let len = u32::try_from(source.len())?;
let range = CodeRange::new(0, len, source);` (if changing the signature isn’t
feasible, at minimum add `debug_assert!(source.len() <= u32::MAX as usize)`
before calling `CodeRange::new` to make the invariant explicit).
arendjr
left a comment
arendjr
left a comment
There was a problem hiding this comment.
Amazing work, thank you!
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
This is exciting! Thank you @chocky335 for the PR. I left some comments. Here is what needs to be done to bring the PR to the finish line:
- Please restore our PR template. In fact, by wiping it out, you missed an important part: documentation. You'll need to send a PR to
biomejs/website, against thenextbranch - This is a new feature, so the PR needs to go to the
nextbranch as per the contribution guidelines - We should at least add an LSP test, so we can validate that the diagnostics are correctly emitted
- The CLI test covers only JavaScript, and I bet CSS and JSON don't work, as highlighted in one of my messages. I believe it's best to add tests for those too.
It's not clear why users need --unsafe, but it would be nice to have a test with --write only, and the snapshost should show that there's an unsafe fix from the plugin
chocky335
force-pushed
the
feat/fixable-gritql-plugins
branch
from
6a060cc to
faa7979
Compare
|
Thank you. Would you mind tracking these things an issue (task template) |
|
@ematipico Thanks for the follow-up. Small correction to my previous message: I initially said all plugin fixes were unsafe and that fix safety would be a follow-up. That is now implemented in this PR. Current status:
I also pushed one additional hardening fix in Docs update for |
|
@ematipico Opened the website docs follow-up PR against : biomejs/website#3988 |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
This is going be an amazing feature! Biome plugins will be much more powerful, thank you!
I left some comments. It's mostly good, I would like to add some tests to catch some error paths
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_grit_patterns/src/linearization.rs (1)
5-5:ResolvedPatternis still unused.It's never referenced by name;
effect.pattern.linearized_text(…)at line 67 resolves via the type system without requiring the import.just lwould catch this.🗑️ Proposed fix
-use grit_pattern_matcher::pattern::{FileRegistry, ResolvedPattern, get_top_level_effects}; +use grit_pattern_matcher::pattern::{FileRegistry, get_top_level_effects};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_grit_patterns/src/linearization.rs` at line 5, The import list in linearization.rs includes ResolvedPattern but it is never referenced; remove ResolvedPattern from the use statement (leaving FileRegistry and get_top_level_effects) so the unused import is eliminated and the compiler warning goes away; locate the use line that currently mentions ResolvedPattern and delete that identifier from the comma-separated import list.
🧹 Nitpick comments (1)
crates/biome_grit_patterns/src/linearization.rs (1)
66-79: Consider checkingbinding.range()before callinglinearized_text.
linearized_textis called unconditionally (and mutatesmemo) even whenbinding.range(language)will returnNoneand force an earlyErrat line 76. Resolving the range first lets you skip the recursive call entirely forFile/Empty/Constantbindings.♻️ Proposed refactor
+ // Resolve range early; skip expensive linearization for bindings without one. + let byte_range = binding.range(language).ok_or_else(|| { + GritPatternError::new("failed to apply rewrite: binding has no byte range") + })?; + // Recursively linearize the replacement pattern. let res = effect .pattern .linearized_text(language, effects, files, memo, false, logs)?; if let Some(ref br) = binding_range && matches!(effect.kind, EffectKind::Rewrite) { memo.insert(br.clone(), Some(res.to_string())); } - let byte_range = binding.range(language).ok_or_else(|| { - GritPatternError::new("failed to apply rewrite: binding has no byte range") - })?; replacements.push((byte_range.start, byte_range.end, res.into_owned()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_grit_patterns/src/linearization.rs` around lines 66 - 79, Check binding.range(language) first and return early if it is None so you avoid calling effect.pattern.linearized_text(...) unnecessarily; move the byte_range lookup (binding.range) above the linearized_text call, only call linearized_text when you have a valid byte_range, and only insert into memo (binding_range and memo.insert(...)) after computing res in the cases where EffectKind::Rewrite applies and the binding has a range; this prevents recursing for File/Empty/Constant bindings and keeps replacements.push((byte_range.start, byte_range.end, res.into_owned())) safe.
🤖 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 146-173: Diagnostics are currently zipped with actions in order,
which can mispair rewrites when log/missing-span diagnostics are present; change
the logic that builds entries by first splitting diagnostics into "real"
diagnostics that can accept an action (e.g., those with a valid span or that
originated from query diagnostics) and "other" diagnostics (logs/missing span),
then drain actions and assign them only to the real diagnostics (update
PluginActionData.applicability as done with action_iter), and finally
reconstruct the full entries list by mapping real diagnostics to
PluginDiagnosticEntry { diagnostic, action } and other diagnostics to
PluginDiagnosticEntry { diagnostic, action: None } preserving original order;
update code around variables result.effects, actions, diagnostics, action_iter
and PluginDiagnosticEntry to implement this stable pairing.
---
Duplicate comments:
In `@crates/biome_grit_patterns/src/linearization.rs`:
- Line 5: The import list in linearization.rs includes ResolvedPattern but it is
never referenced; remove ResolvedPattern from the use statement (leaving
FileRegistry and get_top_level_effects) so the unused import is eliminated and
the compiler warning goes away; locate the use line that currently mentions
ResolvedPattern and delete that identifier from the comma-separated import list.
---
Nitpick comments:
In `@crates/biome_grit_patterns/src/linearization.rs`:
- Around line 66-79: Check binding.range(language) first and return early if it
is None so you avoid calling effect.pattern.linearized_text(...) unnecessarily;
move the byte_range lookup (binding.range) above the linearized_text call, only
call linearized_text when you have a valid byte_range, and only insert into memo
(binding_range and memo.insert(...)) after computing res in the cases where
EffectKind::Rewrite applies and the binding has a range; this prevents recursing
for File/Empty/Constant bindings and keeps replacements.push((byte_range.start,
byte_range.end, res.into_owned())) safe.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/biome_cli/tests/snapshots/main_commands_check/check_plugin_invalid_fix_kind.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_commands_check/check_plugin_invalid_severity.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
crates/biome_cli/tests/commands/check.rscrates/biome_grit_patterns/src/linearization.rscrates/biome_plugin_loader/src/analyzer_grit_plugin.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 153-168: The current check uses entries (which includes log-only
entries) to detect missing spans and can produce false positives; change the
logic to only inspect diagnostic entries before adding the
PluginDiagnosticEntry. Concretely, collect or iterate the diagnostic iterator
(diag_entries) separately (e.g., into a vec or by peeking its items) and run the
is_none() span check on those diagnostic entries only; only if that
diagnostic-only check returns true, push the PluginDiagnosticEntry (the
RuleDiagnostic message referencing name) into entries, leaving log_entries
untouched.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_plugin_loader/src/analyzer_grit_plugin.rs (1)
121-131: Potential memory bloat from cloning full source text per action.
Each rewrite clonesoriginal_text, so many rewrites in large files can balloon memory. Consider sharing the text or storing it once and referencing it.🤖 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 121 - 131, The code clones full original_text for every PluginActionData, causing memory bloat; fix by sharing the text (e.g., create a single Arc<String> or Rc<String> from original_text before building actions and store that in PluginActionData.original_text instead of String) and update the creation site inside the GritQueryEffect::Rewrite handling (where actions/let mut actions is built) to use Arc::clone(&shared_text) so only cheap clones occur; also adjust PluginActionData's original_text type to Arc<String> (or &str with appropriate lifetimes) to avoid per-action String allocations.
🤖 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 233-245: The match in analyzer_grit_plugin.rs that sets the
default diagnostic severity currently returns Severity::Error when severity is
None; change that default to the enum's default variant (Severity::Information)
so register_diagnostic() aligns with the Severity enum's #[default]. Update the
None arm in the match that assigns to the local variable named severity (the
block starting with "let severity = match severity.as_ref()") to use
Severity::Information instead of Severity::Error.
---
Nitpick comments:
In `@crates/biome_plugin_loader/src/analyzer_grit_plugin.rs`:
- Around line 121-131: The code clones full original_text for every
PluginActionData, causing memory bloat; fix by sharing the text (e.g., create a
single Arc<String> or Rc<String> from original_text before building actions and
store that in PluginActionData.original_text instead of String) and update the
creation site inside the GritQueryEffect::Rewrite handling (where actions/let
mut actions is built) to use Arc::clone(&shared_text) so only cheap clones
occur; also adjust PluginActionData's original_text type to Arc<String> (or &str
with appropriate lifetimes) to avoid per-action String allocations.
|
@morgante do you maybe have some time looking at this PR? When I was porting to Biome, some of the rewrite logic from Grit also went over my head 😅 |
|
I can take a look this weekend. |
4 participants
Summary
Closes #5687.
Completes support for fixable GritQL plugin rewrites by wiring plugin rewrite text edits through
biome check --writefor JavaScript, CSS, and JSON.register_diagnostic()now supports optionalfix_kind:fix_kind = "unsafe"): rewrite is an unsafe fix and requires--write --unsafefix_kind = "safe": rewrite is treated as safe and can be applied with--writeTest Plan
CLI integration tests in
biome_cli/tests/commands/check.rs:check_plugin_apply_rewrite--write --unsafeapplies a plugin rewritecheck_plugin_rewrite_no_write--write, diagnostic shown but file not modifiedcheck_plugin_rewrite_write_without_unsafe--writewithout--unsafedoes not apply unsafe fixcheck_plugin_multiple_rewritescheck_plugin_apply_rewrite_css--write --unsafecheck_plugin_apply_rewrite_json--write --unsafecheck_plugin_safe_fix_write--writecheck_plugin_safe_fix_no_write--writeplugin_rewrite_pull_diagnosticsWarningwith code"plugin"Additional local verification:
Docs
Documentation updates have been submitted: biomejs/website#3988