cargo
cargo copied to clipboard
feat: Add `rustc` style errors for manifest parsing
#12235 is tracking user control over warnings. One part of that is making cargo
's diagnostic system output messages in the style of rustc
. To make it so that cargo
didn't have to manage a formatter and renderer, I decided to use annotate-snippets
, which matches rustc
's style well (at one time it was meant to be the formatted for rustc
).
To get this work kicked off, it was suggested to me that we should start with styling manifest parsing errors, and that is what his PR does.
What manifest parsing errors look like after this change:
Note: cargo
does not currently match rustc
in color (#12740), rustc
specifies their colors here and here. I matched what cargo
uses currently for color, but it might be jarring to see two different sets of colors for similar styled error messages. We should probably unify the colors that are used between rustc
and cargo
(and my hope is that annotate-snippets
can help with that in the long run).
Note: the error messages come directly from toml
and can be quite long. It would be nice if we could put some of the message below the highlight but this would require changes to toml
.
Example:
error: invalid type: integer `3`
--> Cargo.toml:7:7
|
7 | bar = 3∅
| ^ expected a version string like "0.9.8" or a detailed dependency like { version = "0.9.8" }
|
r? @ehuss
(rustbot has picked a reviewer for you, use r? to override)
:umbrella: The latest upstream changes (presumably #13166) made this pull request unmergeable. Please resolve the merge conflicts.
This PR probably isn't the best place to have a broader discussion, but I was wondering about what the longer-term higher-level design approach will be with this. Some questions I have:
- Is the error-chain approach cargo currently uses incompatible with the annotated diagnostics? Is there some way the two can coexist?
- Is it possible to have an annotated diagnostic encoded in a type that impls
Error
so that it can be returned like a normal error, and have cargo's top-level error handler (display_error
) display the diagnostics correctly?- I'm a little concerned about what looks like a special-cased
AlreadyPrintedError
.
- I'm a little concerned about what looks like a special-cased
- If there is an annotated structured
Error
type, is it possible to also have that work with JSON output (#8283). - If it isn't possible to have an annotated
Error
type, would it make sense to have something like rustc'semit_err
and keep track of whether or not errors have been encountered in a centralized place?
I'm happy to have a sync discussion sometime if that would be a better way to discuss the design ideas.
For myself, I see this PR as picking the smallest subsection of cargo to allow us to start introducing rustc-style diagnostics and most larger discussions (how to spread this pattern to other parts of the code, json output) would be handled in incremental steps on top. I'm willing to accept some level of hacks (AlreadyPrintedError
) to help keep the scope of this experiment small.
The rough roadmap I see is
- Render TOML errors like rustc
- Try to turn all
toml/mod.rs
errors into rustc-style errors - Create a new warning (implicit features) that leverages a new
emit_diagnostic
(intentionally unstable feature so its not shown to users while we work out the kinks) - Identify
shell().warn
candidates to migrate toemit_diagnostic
- Stabilize
[lints.cargo]
(doesn't have to block on all warnings being turned into diagnostics but we'll likely want some)
At this point, depending on priorities, we can better evaluate
-
emit_err
: afteremit_diagnostic
as that will need to handledeny
and isn't as critical to normal operations. We'll need to discover how we want to handle "exit immediate" vs "fail on exit" (ie error recovery) in both cases. - json output as this isn't in the critical path towards the goal we're working towards but having more structured information will help!
r? @epage
@bors r+
:pushpin: Commit 0d62ae2fc359b2dc3beb8ccf3f743566936ecbf7 has been approved by epage
It is now in the queue for this repository.
:hourglass: Testing commit 0d62ae2fc359b2dc3beb8ccf3f743566936ecbf7 with merge 48601a362dc7f49ad82c6097e8dc7324f7654b63...
:boom: Test timed out
@bors retry
macos never started
:hourglass: Testing commit 0d62ae2fc359b2dc3beb8ccf3f743566936ecbf7 with merge 4eef543a4d49cfdbd92d1f930fd66f08a90547b3...
:sunny: Test successful - checks-actions Approved by: epage Pushing 4eef543a4d49cfdbd92d1f930fd66f08a90547b3 to master...