refactor: Move diagnostic printing to Shell

[Muscraft]

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]

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]

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]

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

[Muscraft]

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

[epage]

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.

[Muscraft]

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.

[weihanglo]

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

[epage]

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.

[weihanglo]

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.

[epage]

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

[weihanglo]

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.

[Muscraft]

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.

[weihanglo]

@bors r+

Thank you all :)

[bors]

📌 Commit 5415b04 has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit 5415b04 with merge a1f8e45...

[bors]

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