fix(shell): Make warn/note/error consistent with lints/toml errors #15917

epage · 2025-09-03T21:25:24Z

What does this PR try to resolve?

This hardens Shell::print_report to work for Shell::warn, note, and error and migrates them to it so we get a more consistent experience and prep for switching the callers over to Shell::print_report.

In terms of end-user behavior changes,

messages now have trailing lines indented.
error is now bright red instead of red
note is now green instead of cyan
the : after the level is no longer bolded

How to test and review this PR?

rustbot · 2025-09-03T21:25:30Z

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

epage · 2025-09-03T21:33:01Z

src/cargo/core/shell.rs

@@ -222,12 +222,18 @@ impl Shell {

    /// Prints an amber 'warning' message.
    pub fn warn<T: fmt::Display>(&mut self, message: T) -> CargoResult<()> {
-        self.print(&"warning", Some(&message), &WARN, false)
+        let report = &[annotate_snippets::Group::with_title(
+            annotate_snippets::Level::WARNING.secondary_title(message.to_string()),


I used secondary_title (custom styling) as a short term hack because the callers may be passing in styled text to workaround the fact that we don't have annotate snippets. As we migrate things, we'll be able to use primary_title (no custom styling) as we should.

…#15920) ### What does this PR try to resolve? This is preparation for changes like #15917 where we start to use `annotate_snippets::Report` for handling more of the rendering of our user messages to be more consistent with rustc and more feature rich in what we are able to render. ### How to test and review this PR?

weihanglo

Do you want to rebase and fix the CI failure?

epage · 2025-09-04T17:58:14Z

Do you want to rebase and fix the CI failure?

Might be good to give any feedback in parallel so I know its worth putting in time to fix things :)

weihanglo

Generally no objection to this change, though I'd like to understand the plan and where we are heading towards.

weihanglo · 2025-09-04T20:04:53Z

tests/testsuite/build_dir.rs

-
-[HELP] available template variables are `{workspace-root}`, `{cargo-cache-home}`, `{workspace-path-hash}`
+       
+       [HELP] available template variables are `{workspace-root}`, `{cargo-cache-home}`, `{workspace-path-hash}`


This seems a bit off. I would expect Help: start without leading spaces.

Yes, we'd need to update the call site to call print_report directly so that it can add a help message to this Report's Group.

weihanglo · 2025-09-04T20:22:01Z

tests/testsuite/freshness_checksum.rs

@@ -21,7 +21,7 @@ fn non_nightly_fails() {
    p.cargo("build -Zchecksum-freshness")
        .with_stderr_data(str![[r#"
 [ERROR] the `-Z` flag is only accepted on the nightly channel of Cargo, but this is the `stable` channel
-See https://doc.rust-lang.org/book/appendix-07-nightly-rust.html for more information about Rust release channels.
+       See https://doc.rust-lang.org/book/appendix-07-nightly-rust.html for more information about Rust release channels.


This See stuff should start from its own line, or have a note:

Yes, now that we have the full capabilities of annotate-snippets, we'll have to re-evaluate our warn/notes to decide if and how we should re-format them. The question is which would be blocking for this PR vs things we do over time. My hope is one like this could be non-blocking.

weihanglo · 2025-09-04T20:24:32Z

src/cargo/core/shell.rs

I feel like we will need to rework on most of our messages. Not all the help and note should be under the same group.

weihanglo · 2025-09-04T21:25:48Z

src/cargo/core/shell.rs

Couldn't find a concrete example, but conceptually adding indentations to trailing lines would add efforts for people to remove those indentations when copy-pasting.

epage · 2025-09-04T21:39:07Z

tests/testsuite/cargo_add/add_toolchain/stderr.term.svg

Let's merge #15928 first

epage · 2025-09-04T21:51:14Z

Generally no objection to this change, though I'd like to understand the plan and where we are heading towards.

My long term vision is

Multiple errors report by cargo that matches rustc
as many warnings as possible converted to lints
as many errors, warnings, notes, and lints as possible show code snippets
All of this match rustc in style

This gets our existing output to be more like rustc and paves a way for using more features.

ehuss · 2025-09-05T22:57:55Z

src/cargo/core/shell.rs

-        )
+        let rendered = Renderer::styled().term_width(term_width).render(report);
+        self.err().write_all(rendered.as_bytes())?;
+        self.err().write_all("\n".as_bytes())?;


minor nit, take it if you will

Suggested change

self.err().write_all("\n".as_bytes())?;

self.err().write_all(b"\n")?;

Posted #15937 to take care of this since this was actually from a PR that has already been merged.

ehuss · 2025-09-05T23:03:58Z

tests/testsuite/collisions.rs

 [WARNING] output filename collision.
-The lib target `foo_macro` in package `foo-macro v1.0.0` has the same output filename as the lib target `foo_macro` in package `foo-macro v1.0.0 ([ROOT]/foo/foo-macro)`.
-Colliding filename is: [ROOT]/foo/target/doc/foo_macro/index.html
-The targets should have unique names.
-This is a known bug where multiple crates with the same name use
-the same path; see <https://github.com/rust-lang/cargo/issues/6313>.
 [CHECKING] foo-macro v1.0.0
 [DOCUMENTING] foo-macro v1.0.0
 [CHECKING] abc v1.0.0 ([ROOT]/foo/abc)
 [DOCUMENTING] foo-macro v1.0.0 ([ROOT]/foo/foo-macro)
 [DOCUMENTING] abc v1.0.0 ([ROOT]/foo/abc)
 [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
 [GENERATED] [ROOT]/foo/target/doc/abc/index.html and 1 other file
+         The lib target `foo_macro` in package `foo-macro v1.0.0` has the same output filename as the lib target `foo_macro` in package `foo-macro v1.0.0 ([ROOT]/foo/foo-macro)`.
+         Colliding filename is: [ROOT]/foo/target/doc/foo_macro/index.html
+         The targets should have unique names.
+         This is a known bug where multiple crates with the same name use
+         the same path; see <https://github.com/rust-lang/cargo/issues/6313>.


Some of these warnings seem to have some ordering issues where the message is split? Like the other messages are showing up in the middle of the warning.

### What does this PR try to resolve? Fixes #12740 This is also prep for #15917, #15922 Changes: - `note` changes from Cyan to Bright Green - `cargo info` had to be changed because the header and context colors became the same - Switch to Bright colors which is usually redundant with Bold but that is theme dependent - `warning` only changes to Bright on Windows which is to work around a shell issue on some Windows versions - except for `PLACEHOLDER` to tell it apart from `LITERAL` - the `:` in `note:` is no longer bolded to match rustc ### How to test and review this PR? ### Notes While annotate-snippets does not have a style for every one of our styles, I updated our manual styles to be similar to annotate snippets. For annotate snippets color definitions, see https://github.com/rust-lang/annotate-snippets-rs/blob/d38b08b81d7d574369ecfd30124195a970b37fd3/src/renderer/mod.rs#L41-L78

rustbot · 2025-09-08T04:47:00Z

☔ The latest upstream changes (possibly 3ceb2cb) made this pull request unmergeable. Please resolve the merge conflicts.

This is from some review feedback left on rust-lang#15917 which was merged in rust-lang#15920.

### What does this PR try to resolve? This is from some review feedback left on #15917 which was merged in #15920. ### How to test and review this PR?

epage added 7 commits September 3, 2025 15:44

refactor(shell): Remove redundant verbosity check

da303f7

refactor(shell): Make print_report consistent with message_stder

d9d6c4a

fix(shell): Add trailing newline to reports

30e00d9

fix(shell): Clear lines for Reports

e3e2b07

refactor(shell): Allow callers to opt-in to quiet filtering

642411f

fix(shell): Switch to annotate snippets for warn/note

d8ed402

fix(shell): Switch to annotate snippets for error

0904b40

rustbot assigned ehuss Sep 3, 2025

rustbot added A-console-output Area: Terminal output, colors, progress bar, etc. A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 3, 2025

epage commented Sep 3, 2025

View reviewed changes

epage force-pushed the annotate branch 4 times, most recently from 625330c to f1be881 Compare September 3, 2025 21:54

epage mentioned this pull request Sep 4, 2025

refactor(shell): Prepare for Reports being generated in more places #15920

Merged

epage force-pushed the annotate branch 2 times, most recently from 7079ccf to 2605d32 Compare September 4, 2025 01:17

epage force-pushed the annotate branch from 2605d32 to 7b535da Compare September 4, 2025 01:38

epage mentioned this pull request Sep 4, 2025

fix(cli): Use Shell::print_report for error chains #15922

Draft

epage force-pushed the annotate branch from 7b535da to 0904b40 Compare September 4, 2025 01:53

weihanglo reviewed Sep 4, 2025

View reviewed changes

epage mentioned this pull request Sep 4, 2025

fix(cli): Adjust messages to match rustc #15928

Merged

epage commented Sep 4, 2025

View reviewed changes

tests/testsuite/cargo_add/add_toolchain/stderr.term.svg

Copy link

Contributor Author

epage Sep 4, 2025

Choose a reason for hiding this comment

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

Let's merge #15928 first

ehuss reviewed Sep 5, 2025

View reviewed changes

epage added a commit to epage/cargo that referenced this pull request Sep 8, 2025

refactor(shell): Simplify some code

334f660

This is from some review feedback left on rust-lang#15917 which was merged in rust-lang#15920.

epage mentioned this pull request Sep 8, 2025

refactor(shell): Simplify some code #15937

Merged

github-merge-queue bot pushed a commit that referenced this pull request Sep 8, 2025

refactor(shell): Simplify some code (#15937)

9eb3240

### What does this PR try to resolve? This is from some review feedback left on #15917 which was merged in #15920. ### How to test and review this PR?


		[HELP] available template variables are `{workspace-root}`, `{cargo-cache-home}`, `{workspace-path-hash}`

		[HELP] available template variables are `{workspace-root}`, `{cargo-cache-home}`, `{workspace-path-hash}`

	self.err().write_all("\n".as_bytes())?;
	self.err().write_all(b"\n")?;

fix(shell): Make warn/note/error consistent with lints/toml errors #15917

Are you sure you want to change the base?

fix(shell): Make warn/note/error consistent with lints/toml errors #15917

Uh oh!

Conversation

epage commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How to test and review this PR?

Uh oh!

rustbot commented Sep 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

epage commented Sep 4, 2025

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage commented Sep 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Sep 8, 2025

Uh oh!

Uh oh!

epage commented Sep 3, 2025 •

edited

Loading