Skip to content

fix(ops): correct error messages for cargo fix --broken-code#16681

Open
raushan728 wants to merge 1 commit intorust-lang:masterfrom
raushan728:fix/cargo-fix-broken-code-message
Open

fix(ops): correct error messages for cargo fix --broken-code#16681
raushan728 wants to merge 1 commit intorust-lang:masterfrom
raushan728:fix/cargo-fix-broken-code-message

Conversation

@raushan728
Copy link
Contributor

@raushan728 raushan728 commented Feb 28, 2026 •
edited
Loading

Fixes #10955

Updates the error message when cargo fix applies suggestions but the code still fails to compile.

  • Changes the warning to accurately state: "fixes were applied, but the code is still in a broken state".
  • Stops redundantly suggesting --broken-code if the flag is already passed.
  • Switches the output to use structured warnings.
  • Updates relevant test assertions.

@rustbot rustbot added Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2026
@rustbot
Copy link
Collaborator

rustbot commented Feb 28, 2026

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @ehuss, @epage, @weihanglo
  • @ehuss, @epage, @weihanglo expanded to ehuss, epage, weihanglo
  • Random selection from ehuss, epage, weihanglo

@raushan728 raushan728 force-pushed the fix/cargo-fix-broken-code-message branch from 3973cf9 to 68ced81 Compare February 28, 2026 13:58
@raushan728
Copy link
Contributor Author

raushan728 commented Feb 28, 2026

r? epage

@rustbot rustbot assigned epage and unassigned ehuss Feb 28, 2026
@epage
Copy link
Contributor

epage commented Feb 28, 2026

A couple of quick notes

  • the issue is marked S-needs-design
  • this is set to close the issue but it had two parts under the assumption that they would be fixed together
  • writing directly to stderr is rare and we need to consider whether this fits
  • the wording feels off and doesn't conform to our diagnostic style guide

@raushan728
Copy link
Contributor Author

raushan728 commented Mar 1, 2026

thanks for the review @epage
a few clarification:

  1. Regarding s-needs-design — i was not sure about the expected design upfront. could u point me to what the agreed design should look like, or share any prior discussion? i will align my implementation accordingly

  2. for the two-part issue — i focused on the --broken-code case (example 1). should example 2 also be addressed in this PR, or as a separate one?

  3. for the stderr writing approch — is there a preferred way to emit this kind of msg in cargo'a diagnostic system? happy to refactor if there is a better pattern.

  4. for the wording — could u share the diagnostic style guide or point to an example msg that follows it? i will update the text to match.

Happy to iterate on this!

@epage
Copy link
Contributor

epage commented Mar 2, 2026

Regarding s-needs-design — i was not sure about the expected design upfront. could u point me to what the agreed design should look like, or share any prior discussion? i will align my implementation accordingly

Thats the intention of that label: it needs that design work done.

for the two-part issue — i focused on the --broken-code case (example 1). should example 2 also be addressed in this PR, or as a separate one?

I'm less concerned about the number of PRs and more about this closing the Issue without fully resolving it

for the stderr writing approch — is there a preferred way to emit this kind of msg in cargo'a diagnostic system? happy to refactor if there is a better pattern.

shell notes, warns, etc

for the wording — could u share the diagnostic style guide or point to an example msg that follows it? i will update the text to match.

See our contrib docs: https://doc.crates.io/contrib/implementation/console.html

@raushan728
Copy link
Contributor Author

raushan728 commented Mar 3, 2026

thanks for docs @epage
my plan to fix both problems:

  1. replace writeln!(stderr) with shell.warn() to follow cargo's output conventions
  2. fix the msg fot both cases:
    • when --broken-code is already used -> explain code was saved but is broken
    • when --broken-code is not used -> suggest trying --broken-code
  3. keep wording lowercase and brief as par style guide

this should fully resolve both example from the issue. will push update soon!

@raushan728 raushan728 force-pushed the fix/cargo-fix-broken-code-message branch from 68ced81 to 8c3c403 Compare March 4, 2026 05:39
@raushan728 raushan728 changed the title Fix poor error message when cargo fix --broken-code fails fix(ops): correct error messages for cargo fix --broken-code Mar 4, 2026
@raushan728
Copy link
Contributor Author

raushan728 commented Mar 4, 2026

ready for re-review @epage

@epage epage mentioned this pull request Mar 4, 2026
Merged
@epage
Copy link
Contributor

epage commented Mar 5, 2026

I realized there were some misunderstandings on both our parts on this code and am working through some prep PRs to help straighten that out

epage
epage reviewed Mar 5, 2026
View reviewed changes
tests/testsuite/fix_n_times.rs Outdated
Note that you may be able to make some more progress in the near-term
fixing code with the `--broken-code` flag

[WARNING] could not apply fixes; the code is already in a broken state. consider using `--broken-code` to save partial progress
Copy link
Contributor

@epage epage Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error does not look like the case in Example 2. Are we sure we have the right situation?

Copy link
Contributor

@epage epage Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we no longer addressing Example 2?

epage
epage reviewed Mar 5, 2026
View reviewed changes
src/cargo/util/diagnostic_server.rs Outdated
Comment on lines +174 to +178
self.gctx.shell().warn(
"could not apply fixes; the code is already in a \
broken state. broken code was saved as requested \
with `--broken-code`",
)?;
Copy link
Contributor

@epage epage Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this message is accurate. It looks like the fixes are applied but we want to report that it still fails

github-merge-queue bot pushed a commit that referenced this pull request Mar 5, 2026
@epage
refactor(fix): Prep for annotate-snippets (#16702)
292e9c2 
### What does this PR try to resolve?

Instead of printing a warning as a
- a warning
- many individual blocks of text

this switches us to print it all as a single warning. This will make it
easier to adapt to annotate-snippets in the future (work is in
progress).

This was noticed while reviewing #16681 which led us down the wrong path
there.

### How to test and review this PR?
@rustbot

This comment has been minimized.

Sign in to view
@raushan728 raushan728 force-pushed the fix/cargo-fix-broken-code-message branch from bcf4239 to dffe9cf Compare March 6, 2026 05:22
@rustbot

This comment has been minimized.

Sign in to view
@raushan728
Copy link
Contributor Author

raushan728 commented Mar 6, 2026

Hey @epage, conflict resolved i have also updated the warning message wording and fixed the fix_n_times test assertions based on your feedback

(Note: The current CI failures seem to be unrelated nightly script breakages)

@raushan728 raushan728 requested a review from epage March 6, 2026 05:43
@epage
Copy link
Contributor

epage commented Mar 6, 2026

Hey @epage, conflict resolved i have also updated the warning message wording and fixed the fix_n_times test assertions based on your feedback

A review was requested my I don't think my comments were addressed

@rustbot

This comment has been minimized.

Sign in to view
@epage
Copy link
Contributor

epage commented Mar 6, 2026

#16711 is now merged and it is safe to rebase

@raushan728 raushan728 force-pushed the fix/cargo-fix-broken-code-message branch from dffe9cf to 23ac6a1 Compare March 7, 2026 10:24
@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2026

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@raushan728
Copy link
Contributor Author

raushan728 commented Mar 7, 2026

@epage sry about the confusion earlier i had a git mix-up pushed the stashed code yest.

i just rebased. ur previous feedback now addressed

  1. updated the warning msg wording
  2. reverted the fix_n_times test assertions since u were right about example 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@epage epage Awaiting requested review from epage

At least 1 approving review is required to merge this pull request.

Assignees

@epage epage

Labels

Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Poor error messages with cargo fix with broken code

4 participants

@raushan728 @rustbot @epage @ehuss