project: Fix code_actions_on_format edits being reverted when no formatter available#51605
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @mvanhorn on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
@cla-bot check |
|
We require contributors to sign our Contributor License Agreement, and we don't have @mvanhorn on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
That's a great investigation under the link posted.
It seems that the main idea is that the loop for formatter in formatters must be always carried up to the end, and here we try to fix some of the ? that forbid it.
Yet, things like extend_formatting_transaction are still ?-marked and would need a similar treatment?
Overall, the whole approach seems not very sustainable, we would be better off extracting the entire loop's body into its own function/method.
We could return an anyhow::Result<std::ops::ControlFlow> there and cover all various ? explicitly, once, by logging an error with the context and moving on.
Besides that, the usual ask are the tests: formatting is rather intricate and would definitely benefit from covering this scenario in e.g. editor_tests.rs
Restructure the formatter loop to extract each formatter's application logic into a separate function that returns anyhow::Result. On failure, the loop logs the error and continues to the next formatter instead of aborting the entire formatting chain. This addresses the sustainability concern: instead of converting individual ? operators to match blocks, all error points are covered by the function boundary.
642e7ae to
db9886b
Compare
📏 PR Size: 902 lines changed (size/XL)Please note: this PR exceeds the 400 LOC soft limit.
|
|
Restructured in db9886b - extracted the entire formatter loop body into This covers all Re: tests - I'll add a test in |
Verifies that when one formatter in the chain fails (returns an error), subsequent formatters still apply their edits successfully. This covers the scenario described in zed-industries#51490 where code_actions_on_format edits were lost when a formatter was unavailable.
|
Added a test in 106c346 covering the case where one formatter fails and subsequent formatters still apply their edits. On the |
|
Thanks for the merge! Happy this code_actions_on_format fix landed. |
…atter available (zed-industries#51605) Fixes zed-industries#51490 ## Problem When `code_actions_on_format` is configured (e.g., `source.fixAll.phpcs`) and `formatter` is not explicitly set, code action edits are applied but then reverted during format-on-save. The root cause is in the formatting pipeline's error handling. Code actions and formatters share a single transaction via `.chain()`. When `Formatter::Auto` resolves to `Formatter::Prettier` (PHP defaults to `prettier.allowed: true`), and prettier is not installed or fails, the error propagates via `?` out of the formatter loop, aborting the entire `format_buffer_locally` function. This causes the formatting transaction - which already contains the successfully applied code action edits - to be lost. The workaround of setting `"formatter": []` works because it removes all formatters from the chain, so no formatter can fail and trigger the abort. ## Fix Change error handling for individual formatters (Prettier, External command, Language Server) from propagating errors via `?` to logging the error and continuing to the next formatter. This treats "formatter failed" as a no-op for that specific formatter rather than aborting the entire pipeline. This matches the existing pattern used when `LanguageServer(Current)` finds no server that supports formatting - it already logs and `continue`s rather than erroring. ## Testing - `cargo check -p project` passes - `cargo fmt -p project -- --check` passes Release Notes: - Fixed `code_actions_on_format` edits being reverted when `formatter: "auto"` resolves to an unavailable formatter. --------- Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Kirill Bulatov <kirill@zed.dev>
Fixes #51490
Problem
When
code_actions_on_formatis configured (e.g.,source.fixAll.phpcs) andformatteris not explicitly set, code action edits are applied but then reverted during format-on-save.The root cause is in the formatting pipeline's error handling. Code actions and formatters share a single transaction via
.chain(). WhenFormatter::Autoresolves toFormatter::Prettier(PHP defaults toprettier.allowed: true), and prettier is not installed or fails, the error propagates via?out of the formatter loop, aborting the entireformat_buffer_locallyfunction. This causes the formatting transaction - which already contains the successfully applied code action edits - to be lost.The workaround of setting
"formatter": []works because it removes all formatters from the chain, so no formatter can fail and trigger the abort.Fix
Change error handling for individual formatters (Prettier, External command, Language Server) from propagating errors via
?to logging the error and continuing to the next formatter. This treats "formatter failed" as a no-op for that specific formatter rather than aborting the entire pipeline.This matches the existing pattern used when
LanguageServer(Current)finds no server that supports formatting - it already logs andcontinues rather than erroring.Testing
cargo check -p projectpassescargo fmt -p project -- --checkpassesRelease Notes:
code_actions_on_formatedits being reverted whenformatter: "auto"resolves to an unavailable formatter.