refactor: Move diagnostic printing to Shell #13813

Muscraft · 2024-04-27T18:37:43Z

As I have been working on cargo's diagnostic system, I have disliked copying around the code for a new Renderer, and then emitting the diagnostic:

let renderer = Renderer::styled().term_width(
    gctx.shell()
        .err_width()
        .diagnostic_terminal_width()
        .unwrap_or(annotate_snippets::renderer::DEFAULT_TERM_WIDTH),
);
writeln!(gctx.shell().err(), "{}", renderer.render(message))?;

Moving to a method on Shell helps mitigate the risk of updating duplicate code and missing one. It should also make it nearly impossible to get into scenarios where gctx.shell() is borrowed twice, leading to panics. This problem was one I ran into early on as I tried to write messages to stderr like so:

writeln!(
    gctx.shell().err(),
    "{}",
    Renderer::styled()
        .term_width(
            gctx.shell()
                .err_width()
                .diagnostic_terminal_width()
                .unwrap_or(annotate_snippets::renderer::DEFAULT_TERM_WIDTH),
        )
        .render(message)
)?;

rustbot · 2024-04-27T18:37:49Z

r? @epage

rustbot has assigned @epage.
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

weihanglo

Overall looks good to me, especially for the prevention of double borrowing RefCell!

However, I wonder if we need diagnostic with to be computed every time before Cargo prints messages. I guess the answer is no, we don't care people resize their terminal width during a build.

epage · 2024-04-29T15:02:45Z

I'm a bit uncomfortable generating this in Context::new because it can be quite easy for us to add config for controlling this (honestly, I'm surprised it doesn't exist).

As for the terminal width changing, that is more likely to come up when it comes to cargo watch support (#9339).

So far, a Renderer is "cheap". We could instead make the renderer on every call.

weihanglo · 2024-04-29T16:58:59Z

src/cargo/util/context/mod.rs

@@ -412,6 +413,22 @@ impl GlobalContext {
        self.shell.borrow_mut()
    }

+    /// Prints the passed in [Message] to stderr
+    pub fn renderer_diagnostic(&self, message: Message<'_>) -> std::io::Result<()> {


Any reason that this method is on GlobalContext instead of Shell?

None that I can think of, I went ahead and moved it to Shell

One of the problems with #12875 is that it put diagnostic tracking in Shell. For me, I see Shell as lower level than that. I feel like putting diagnotic rendering in Shell could encourage that kind of thinking.

I agree diagnostic tracking should not be in Shell, but I am not as worried about encouraging that thinking with this change. This change moves to a function where things get rendered and then emitted, similar to:

cargo/src/cargo/core/shell.rs

Lines 387 to 393 in ba6fb40

pub fn print_json<T: serde::ser::Serialize>(&mut self, obj: &T) -> CargoResult<()> {

// Path may fail to serialize to JSON ...

let encoded = serde_json::to_string(&obj)?;

// ... but don't fail due to a closed pipe.

drop(writeln!(self.out(), "{}", encoded));

Ok(())

}

The JSON gets "rendered" by serde_json::to_string and then printed to stdout.

The fact Renderer gets created inside of emit_diagnostic is an implementation detail.

@epage do you consider it as a blocker? I am fine with either way.

I agree diagnostic tracking should not be in Shell, but I am not as worried about encouraging that thinking with this change.

By putting it here, it implicitly states diagnostics live here and it is the starting point for any thought process on how to evolve this code.

do you consider it as a blocker? I am fine with either way.

I'm mixed. In terms of "this is all internal and easily changed", its not a blocker. It just sets us down the wrong direction for future changes unless someone thinks beyond the code in front of them which we don't always do.

Maybe we could put some design principles in the module-level for shell, so at least this discussion won't just disappear in the air.

The current naming is low level enough that I'm fine with moving this forward

weihanglo · 2024-04-30T15:23:21Z

src/cargo/core/shell.rs

@@ -1,3 +1,4 @@
+use annotate_snippets::{Message, Renderer};


nit: we don't really have a style convention for imports, though I believe if this was added by auto-import via Rust Analyzer, it would be grouped together with anstream and `anstyle.

Nice catch! Normally, things get added correctly; I'm not sure why it didn't in this case.

I wish group_imports and imports_granularity were stable so we could use them.

src/cargo/core/shell.rs

weihanglo · 2024-04-30T18:24:44Z

@bors r+

Thank you all :)

bors · 2024-04-30T18:24:47Z

📌 Commit 5415b04 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-04-30T18:25:54Z

⌛ Testing commit 5415b04 with merge a1f8e45...

bors · 2024-04-30T18:55:50Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing a1f8e45 to master...

Update cargo 15 commits in b60a1555155111e962018007a6d0ef85207db463..6087566b3fa73bfda29702632493e938b12d19e5 2024-04-26 16:37:29 +0000 to 2024-04-30 20:45:20 +0000 - fix(cargo-fix): dont fix into standard library (rust-lang/cargo#13792) - refactor: Move diagnostic printing to Shell (rust-lang/cargo#13813) - Populate git information when building Cargo from Rust's source tarball (rust-lang/cargo#13832) - docs: fix several typos found by `typos-cli` (rust-lang/cargo#13831) - fix(alias): Aliases without subcommands should not panic (rust-lang/cargo#13819) - fix(toml): Improve granularity of traces (rust-lang/cargo#13830) - fix(toml): Warn, rather than fail publish, if a target is excluded (rust-lang/cargo#13713) - test(cargo-lints): Add a test to ensure cap-lints works (rust-lang/cargo#13829) - fix(toml)!: Remove support for inheriting badges (rust-lang/cargo#13788) - chore(ci): Don't check `cargo` against beta channel (rust-lang/cargo#13827) - Fix target entry in .gitignore (rust-lang/cargo#13817) - Bump to 0.81.0; update changelog (rust-lang/cargo#13823) - Add failing test: artifact_dep_target_specified (rust-lang/cargo#13816) - fix(cargo-lints): Don't always inherit workspace lints (rust-lang/cargo#13812) - Update SleepTraker returns_in_order unit test (rust-lang/cargo#13811) r? ghost

rustbot assigned epage Apr 27, 2024

rustbot added A-configuration Area: cargo config files and env vars A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2024

weihanglo reviewed Apr 28, 2024

View reviewed changes

Muscraft force-pushed the add-global-renderer branch from 73f7796 to 1601b6e Compare April 29, 2024 15:51

weihanglo reviewed Apr 29, 2024

View reviewed changes

Muscraft force-pushed the add-global-renderer branch from 1601b6e to 986a3ee Compare April 29, 2024 17:26

rustbot added the A-console-output Area: Terminal output, colors, progress bar, etc. label Apr 29, 2024

Muscraft force-pushed the add-global-renderer branch from 986a3ee to 7d45802 Compare April 29, 2024 17:28

Muscraft changed the title ~~Move to a global Renderer~~ Move Renderer creation to Shell Apr 29, 2024

Muscraft force-pushed the add-global-renderer branch from 7d45802 to 6fb7a9b Compare April 29, 2024 17:38

Muscraft changed the title ~~Move Renderer creation to Shell~~ refactor: Move diagnostic printing to Shell Apr 29, 2024

weihanglo approved these changes Apr 30, 2024

View reviewed changes

Muscraft force-pushed the add-global-renderer branch from 6fb7a9b to 5a22441 Compare April 30, 2024 15:37

epage reviewed Apr 30, 2024

View reviewed changes

src/cargo/core/shell.rs Outdated Show resolved Hide resolved

refactor: Move diagnostic printing to Shell

5415b04

Muscraft force-pushed the add-global-renderer branch from 5a22441 to 5415b04 Compare April 30, 2024 16:38

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 Apr 30, 2024

bors merged commit a1f8e45 into rust-lang:master Apr 30, 2024
21 checks passed

bors mentioned this pull request Apr 30, 2024

[experiment] patch with patch files #13779

Closed

Muscraft deleted the add-global-renderer branch April 30, 2024 19:10

weihanglo mentioned this pull request May 1, 2024

Update cargo rust-lang/rust#124572

Merged

rustbot added this to the 1.80.0 milestone May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Move diagnostic printing to Shell #13813

refactor: Move diagnostic printing to Shell #13813

Muscraft commented Apr 27, 2024 •

edited

Loading

rustbot commented Apr 27, 2024

weihanglo left a comment

epage commented Apr 29, 2024

weihanglo Apr 29, 2024

Muscraft Apr 29, 2024

epage Apr 29, 2024

Muscraft Apr 29, 2024

weihanglo Apr 30, 2024

epage Apr 30, 2024

weihanglo Apr 30, 2024

epage Apr 30, 2024

weihanglo Apr 30, 2024

Muscraft Apr 30, 2024

weihanglo commented Apr 30, 2024

bors commented Apr 30, 2024

bors commented Apr 30, 2024

bors commented Apr 30, 2024

	pub fn print_json<T: serde::ser::Serialize>(&mut self, obj: &T) -> CargoResult<()> {
	// Path may fail to serialize to JSON ...
	let encoded = serde_json::to_string(&obj)?;
	// ... but don't fail due to a closed pipe.
	drop(writeln!(self.out(), "{}", encoded));
	Ok(())
	}

refactor: Move diagnostic printing to Shell #13813

refactor: Move diagnostic printing to Shell #13813

Conversation

Muscraft commented Apr 27, 2024 • edited Loading

rustbot commented Apr 27, 2024

weihanglo left a comment

Choose a reason for hiding this comment

epage commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weihanglo commented Apr 30, 2024

bors commented Apr 30, 2024

bors commented Apr 30, 2024

bors commented Apr 30, 2024

Muscraft commented Apr 27, 2024 •

edited

Loading