feat: Add `rustc` style errors for manifest parsing #13172

Muscraft · 2023-12-14T23:13:57Z

#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 doesn'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 used annotate-snippets default colors which match what rustc currently uses for colors. I did this as a stopgap while I work to unify the colors used between rustc and cargo.

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

rustbot · 2023-12-14T23:14:02Z

r? @ehuss

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

src/cargo/util/toml/mod.rs

epage · 2023-12-15T01:06:05Z

src/cargo/util/toml/mod.rs

+                // Add back the new line that was stripped by `.lines()`
+                if line_count > line + 1 {
+                    source.push('\n');
+                // If we are trying to highlight past the end of the file, add a
+                // placeholder character to the end of the line.
+                } else if span.end >= source.len() - 1 {
+                    source.push('∅');
+                }


This should be in annotate-snippets, not cargo

The '\n' should be "\\n" so it gets rendered

These should be Dimmed to indicate they aren't literal text

These should only show up when they are being highlighted

I'm also not clear why the ∅ is being added, can you say more about the motivation there? I'm a little concerned about any potential confusion if users are not familiar with what that symbol means. I don't think rustc typically displays errors like that, and I think it is reasonably clear without a special character. For example:

error: this file contains an unclosed delimiter --> src/main.rs:4:10 | 4 | fn x() { | - ^ | | | unclosed delimiter

or

error: expected one of `->`, `where`, or `{`, found `<eof>` --> src/main.rs:4:9 | 4 | fn main() | ^ expected one of `->`, `where`, or `{`

I'm trying to find the conversation around invisible characters but it is a bit buried.

I think an aspect is that TOML is newline sensitive, unlike Rust, which elevates the importance of invisible characters and makes the chance of an error more frequent.

I removed cargo's ability to render invisible characters, as it is something that should be discussed in another issue

src/cargo/util/toml/mod.rs

Cargo.toml

bors · 2023-12-15T18:37:40Z

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

ehuss · 2023-12-28T20:39:05Z

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 (Provide Cargo messages as JSON messages #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.

epage · 2023-12-28T20:55:30Z

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 to emit_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: 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!

tests/testsuite/bad_config.rs

tests/testsuite/alt_registry.rs

src/cargo/util/toml/mod.rs

ehuss · 2024-01-08T17:42:30Z

r? @epage

epage · 2024-01-10T22:41:29Z

@bors r+

bors · 2024-01-10T22:41:32Z

📌 Commit 0d62ae2 has been approved by epage

It is now in the queue for this repository.

bors · 2024-01-10T22:42:38Z

⌛ Testing commit 0d62ae2 with merge 48601a3...

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` doesn't have to manage a formatter and renderer, I decided to use [`annotate-snippets`](https://crates.io/crates/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: ![image](https://github.com/rust-lang/cargo/assets/23045215/0eb388b9-8d72-48ad-84a9-585160995078) --- Note: `cargo` does not currently match `rustc` in color (#12740), `rustc` specifies their colors [here](https://github.com/rust-lang/rust/blob/740cea81d6e34fc03c4495890e29f1c5ecb40b96/compiler/rustc_errors/src/lib.rs#L1755-L1768) and [here](https://github.com/rust-lang/rust/blob/740cea81d6e34fc03c4495890e29f1c5ecb40b96/compiler/rustc_errors/src/emitter.rs#L2689-L2723). I used `annotate-snippets` default colors which match what `rustc` currently uses for colors. I did this as a stopgap while I work to unify the colors used between `rustc` and `cargo`. --- 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" } | ```

bors · 2024-01-11T00:42:38Z

💥 Test timed out

ehuss · 2024-01-11T01:06:29Z

@bors retry

macos never started

bors · 2024-01-11T01:07:38Z

⌛ Testing commit 0d62ae2 with merge 4eef543...

bors · 2024-01-11T01:38:02Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 4eef543 to master...

Update cargo 8 commits in 3e428a38a34e820a461d2cc082e726d3bda71bcb..84976cd699f4aea56cb3a90ce3eedeed9e20d5a5 2024-01-09 20:46:36 +0000 to 2024-01-12 15:55:43 +0000 - fix(resolver): do not panic when sorting empty summaries (rust-lang/cargo#13287) - Implementation of shallow libgit2 fetches behind an unstable flag (rust-lang/cargo#13252) - Add documentation entry for unstable `--output-format` flag (rust-lang/cargo#13284) - doc: add `public` info in `cargo-add` man page. (rust-lang/cargo#13272) - More docs on prerelease compat (rust-lang/cargo#13286) - Add unstable `--output-format` option to `cargo rustdoc` (rust-lang/cargo#12252) - feat: Add `rustc` style errors for manifest parsing (rust-lang/cargo#13172) - Document why `du` function uses mutex (rust-lang/cargo#13273) r? ghost

The fix was backported to 1.77 (rust-lang#13393) which is when it got introduced (rust-lang#13172).

docs(changelog): Slight cleanup ### What does this PR try to resolve? For strip, I felt the nuance of where this change applies isn't as obvious and might give people the wrong impression, so I tried to make that front and center. For the panic, the fix was backported to 1.77 (#13393) which is when it got introduced (#13172). ### How should we test and review this PR? ### Additional information

The fix was backported to 1.77 (rust-lang#13393) which is when it got introduced (rust-lang#13172).

rustbot assigned ehuss Dec 14, 2023

rustbot added A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2023

Muscraft force-pushed the diagnostic-system branch 3 times, most recently from 8a0c58a to 31b833d Compare December 14, 2023 23:16

epage reviewed Dec 15, 2023

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

epage reviewed Dec 15, 2023

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

weihanglo reviewed Dec 15, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

weihanglo added the A-diagnostics Area: Error and warning messages generated by Cargo itself. label Dec 15, 2023

feat: Add rustc style errors for manifest parsing

0d62ae2

epage mentioned this pull request Jan 2, 2024

cargo fix support for exported-private-dependencies #13095

Open

Muscraft force-pushed the diagnostic-system branch from 31b833d to e7d43d7 Compare January 5, 2024 17:50

epage reviewed Jan 5, 2024

View reviewed changes

tests/testsuite/bad_config.rs Show resolved Hide resolved

tests/testsuite/alt_registry.rs Outdated Show resolved Hide resolved

Muscraft force-pushed the diagnostic-system branch from e7d43d7 to 7a4baec Compare January 5, 2024 22:21

epage reviewed Jan 6, 2024

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

epage reviewed Jan 6, 2024

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

epage reviewed Jan 6, 2024

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

epage reviewed Jan 6, 2024

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

epage reviewed Jan 6, 2024

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

epage reviewed Jan 6, 2024

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

Muscraft mentioned this pull request Jan 7, 2024

Running 'cargo new' with 'workspace.package.name' set creates an invalid manifest #13258

Open

rustbot assigned epage and unassigned ehuss Jan 8, 2024

Muscraft force-pushed the diagnostic-system branch 2 times, most recently from 0186fd2 to 0d62ae2 Compare January 10, 2024 22:39

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2024

epage mentioned this pull request Jan 10, 2024

User control over cargo warnings #12235

Open

6 tasks

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 11, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2024

bors merged commit 4eef543 into rust-lang:master Jan 11, 2024
22 checks passed

Muscraft deleted the diagnostic-system branch January 11, 2024 01:38

weihanglo mentioned this pull request Jan 12, 2024

Update cargo rust-lang/rust#119909

Merged

weihanglo added this to the 1.77.0 milestone Jan 31, 2024

weihanglo mentioned this pull request Jan 31, 2024

OOB string indexing panic with incorrect Cargo.toml #13374

Closed

epage added a commit to epage/cargo that referenced this pull request Feb 5, 2024

docs(changelog): Remove panic entry

0d2ebbb

The fix was backported to 1.77 (rust-lang#13393) which is when it got introduced (rust-lang#13172).

epage mentioned this pull request Feb 5, 2024

docs(changelog): Slight cleanup #13396

Merged

stupendoussuperpowers pushed a commit to stupendoussuperpowers/cargo that referenced this pull request Feb 28, 2024

docs(changelog): Remove panic entry

465b573

The fix was backported to 1.77 (rust-lang#13393) which is when it got introduced (rust-lang#13172).

charmitro pushed a commit to charmitro/cargo that referenced this pull request Sep 13, 2024

docs(changelog): Remove panic entry

59df000

The fix was backported to 1.77 (rust-lang#13393) which is when it got introduced (rust-lang#13172).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `rustc` style errors for manifest parsing #13172

feat: Add `rustc` style errors for manifest parsing #13172

Muscraft commented Dec 14, 2023 •

edited

Loading

rustbot commented Dec 14, 2023

epage Dec 15, 2023 •

edited

Loading

ehuss Dec 28, 2023

epage Dec 28, 2023

Muscraft Jan 5, 2024

bors commented Dec 15, 2023

ehuss commented Dec 28, 2023

epage commented Dec 28, 2023

ehuss commented Jan 8, 2024

epage commented Jan 10, 2024

bors commented Jan 10, 2024

bors commented Jan 10, 2024

bors commented Jan 11, 2024

ehuss commented Jan 11, 2024

bors commented Jan 11, 2024

bors commented Jan 11, 2024

feat: Add rustc style errors for manifest parsing #13172

feat: Add rustc style errors for manifest parsing #13172

Conversation

Muscraft commented Dec 14, 2023 • edited Loading

rustbot commented Dec 14, 2023

epage Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

ehuss Dec 28, 2023

Choose a reason for hiding this comment

epage Dec 28, 2023

Choose a reason for hiding this comment

Muscraft Jan 5, 2024

Choose a reason for hiding this comment

bors commented Dec 15, 2023

ehuss commented Dec 28, 2023

epage commented Dec 28, 2023

ehuss commented Jan 8, 2024

epage commented Jan 10, 2024

bors commented Jan 10, 2024

bors commented Jan 10, 2024

bors commented Jan 11, 2024

ehuss commented Jan 11, 2024

bors commented Jan 11, 2024

bors commented Jan 11, 2024

feat: Add `rustc` style errors for manifest parsing #13172

feat: Add `rustc` style errors for manifest parsing #13172

Muscraft commented Dec 14, 2023 •

edited

Loading

epage Dec 15, 2023 •

edited

Loading