cargo icon indicating copy to clipboard operation
cargo copied to clipboard

feat: Add `rustc` style errors for manifest parsing

Open Muscraft opened this issue 6 months ago • 2 comments

#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: Screenshot from 2023-12-14 15-59-09


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" }
  |

Muscraft avatar Dec 14 '23 23:12 Muscraft

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

rustbot avatar Dec 14 '23 23:12 rustbot

:umbrella: The latest upstream changes (presumably #13166) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Dec 15 '23 18:12 bors

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.
  • 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's emit_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.

ehuss avatar Dec 28 '23 20:12 ehuss

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

  1. Render TOML errors like rustc
  2. Try to turn all toml/mod.rs errors into rustc-style errors
  3. 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)
  4. Identify shell().warn candidates to migrate to emit_diagnostic
  5. 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: after emit_diagnostic as that will need to handle deny 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!

epage avatar Dec 28 '23 20:12 epage

r? @epage

ehuss avatar Jan 08 '24 17:01 ehuss

@bors r+

epage avatar Jan 10 '24 22:01 epage

:pushpin: Commit 0d62ae2fc359b2dc3beb8ccf3f743566936ecbf7 has been approved by epage

It is now in the queue for this repository.

bors avatar Jan 10 '24 22:01 bors

:hourglass: Testing commit 0d62ae2fc359b2dc3beb8ccf3f743566936ecbf7 with merge 48601a362dc7f49ad82c6097e8dc7324f7654b63...

bors avatar Jan 10 '24 22:01 bors

:boom: Test timed out

bors avatar Jan 11 '24 00:01 bors

@bors retry

macos never started

ehuss avatar Jan 11 '24 01:01 ehuss

:hourglass: Testing commit 0d62ae2fc359b2dc3beb8ccf3f743566936ecbf7 with merge 4eef543a4d49cfdbd92d1f930fd66f08a90547b3...

bors avatar Jan 11 '24 01:01 bors

:sunny: Test successful - checks-actions Approved by: epage Pushing 4eef543a4d49cfdbd92d1f930fd66f08a90547b3 to master...

bors avatar Jan 11 '24 01:01 bors