Rust: Tracking issue for unstable `--json-rendered` flag

[alexcrichton]

This issue is intended to be a tracking issue for the --json-rendered flag implemented in https://github.com/rust-lang/rust/pull/59128. The --json-rendered flag can either take plain or termcolor, and affects whether ANSI color codes are embedded into the rendered field of JSON diagnostics.

Known issues with this:

• [ ] flag not supported in rustdoc yet

Future features:

• [ ] Html output (discussion: https://github.com/rust-lang/rust/pull/59128#discussion_r265675833)
• [ ] Short output (equal to --error-format=short)

cc @oli-obk, original author of the feature

[alexcrichton]

I'd like to propose that this feature is stabilized in an effort to stabilize pipelined compilation. As a brief summary of how this flag works today, when passed to the compiler the --json-rendered flag will affect the "rendered" field of JSON diagnostics printed by the compiler.

I would propose the following stabilization:

• Add a new flag, --json-rendered
• Hide this flag from rustc -h unless the --verbose flag is passed
• Accept the following values for --json-rendered=value:
  
  
  
  • plain - whatever rustc would print on the console but without colors
  
  
  • ansi-colors - whatever rustc would print on the console with ANSI color codes embedded in the string
  
  
  • short - whatever rustc would print on the console for --error-format=short, but without colors
  
  
  • short-ansi-colors - combination of the two options above
  
  

This should be sufficient for all of Cargo's usage, and we'd mirror the option into rustdoc as well as rustc for processing there.

[alexcrichton]

Oh and cc @rust-lang/compiler, could I persuade one of y'all to kick of FCP for stabilization? I wrote a summary above for what I'm proposing to be stabilized here.

[eddyb]

I feel like we're accumulating a bunch of flags that are related but not explicitly grouped (like -C flags are), maybe there is a better way to set this up? Other than that I'm on board!

[oli-obk]

Instead of having a a non-color flag and a color flag for each variant we can also reuse the --color flag that modifies how non-json output is colored.

[jsgf]

Would it be possible to have an option to include all rendering styles in the case where it is being cached and we don't know how its going to be used or consumed?

Is color vs. non-color just a matter of stripping out ctrl characters?

[eddyb]

@jsgf With the ANSI output, you'd need to have a small parser to find where the escape sequence stops (i.e. it always starts with a \x1b but can be followed by a varying number of characters).
While possible, it might be too much work, so it could be a good idea to have a different field, rendered_ansi_colors which contains the colored version.
This would also be backwards-compatible, if that's a concern.

[ehuss]

@eddyb Cargo already implements ansi parsing and stripping when necessary. It uses the strip_ansi_escapes package for stripping and fwdansi for converting to old-Windows color controls.

[ehuss]

I'm a little concerned about stabilizing this without having a strategy for supporting the caching use case. It needs a single JSON format that can be converted to all other output formats. Currently it works for everything except short messages. I see a few options here:

1. Support ability to include multiple rendered formats in JSON, which could influence the format of the --json-rendered flag.
2. Use an external library for rendering messages, and share that between rustc and cargo. This would probably require a new JSON format altogether. Once this is in place, cargo could stop using --json-rendered.

ping @rust-lang/wg-diagnostics. I'm curious what the feasibility of 2 is. Is it on the table as an option? Is it something that can happen within a year?

I also think it's a little strange to have separate color controls from the --color flag.

[jsgf]

@ehuss Yes, that's exactly my concern - I'd like to cache the json output and be able to generate any of the other forms on demand, without having separate implementation of diagnostics rendering.

[oli-obk]

@rust-lang/wg-diagnostics is working on integrating the annotate-snippet-rs crate into rustc and making it the go-to rendering engine (see https://github.com/rust-lang/rust/issues/59346). Implementing a json -> rendered scheme is not on the table at the momemnt, but I see nothing that would speak against it. All relevant information should be available.

We could even extend --json-rendered to also have a not/none variant that just produces null for the rendered field (or an empty string) in order to reduce the amount of data in json that you don't need.

So it definitely is doable, but we have our hands full with just using annotate-snippet-rs in the compiler at all.

[alexcrichton]

@jsgf and @ehuss can y'all explain more why you think that a diagnostic JSON message should contain all possible renderings? I understand the idea of wanting it cached, but why would you want it cached and dynamically switched between builds? This seems like a setting that is rarely flipped and wouldn't really benefit from an A+ user-experience of caching as much as possible.

I would personally be fine interpreting --color always as simply "put ansi codes into the json rendered field". This is largely a means to an end of pipelined compilation, and I wish that nothing had to be stabilized anyway.

To that end could I propose that we simply stabilize the intent of interpreting --color always as "put ansi codes into the json rendered field" and table the question of caching short/long at the same time?

[ehuss]

There are some use cases where switching outputs is common. For example, my editor uses JSON, whereas when I run cargo manually in a terminal it uses human output. It would be bad if the cached output spit JSON into the terminal. The cache implementation in cargo handles this, except for the "short" case. It would be weird if you asked for "short" output and suddenly got the verbose human output, or vice-versa.

That being said, I wonder how often the short output is used. I suspect it is extremely rare, in which case perhaps leaving it a little broken might be ok.

Thinking about it more, I think my concern can be addressed later and independently. Such as making --json-rendered a comma-separated list of values, or using an external library; neither should affect this, so I'm OK with this proposal.

One nitpick about naming, perhaps use human instead of plain to be consistent with --error-format?

Also, making it use the --color flag might be slightly tricky. Cargo always issues that flag. Cargo will need to be updated to not do that if the user requests the JSON format, and instead only if --color is passed. Otherwise the JSON will suddenly start including color codes for everyone. A minor issue, but needs to be handled if we go that route.

[alexcrichton]

Ah ok I think I see what you mean as well with caching and stale messages. It's a good point as well with --color and being a bit trickier with Cargo but that seems surmountable. I would personally be most in favor of that strategy!

[jsgf]

In our environment, Buck caches are very persistent and shared among multiple users. A cache entry could have been generated by CI, and then reused as part of a user's manually invoked build, or in a rustfix-style linter build. In other words, the original cached form must be reusable for all these other use-cases.

[ehuss]

@jsgrf I'm curious, does Buck normally cache the console output from compilers, or is this something unique for the rust integration?

[jsgf]

@ehuss It doesn't at present, but it's something I'd like to add. I've been writing tooling to auto-apply fix suggestions in error messages, and its very annoying to have to deliberately break caching in order to get diagnostics. Also it falls out of distributed builds (Remote Execution) pretty naturally, because all the output artifacts are transferred via a content store, including diagnostics.

[luser]

@eddyb Cargo already implements ansi parsing and stripping when necessary. It uses the strip_ansi_escapes package for stripping and fwdansi for converting to old-Windows color controls.

FWIW, this use case is the exact reason I wrote the strip-ansi-escapes crate in the first place--so sccache could invoke compilers with --color=always and cache their output, and then replay that output with or without colors depending on the situation.

[alexcrichton]

Ok in an attempt to prod this again, I'd like to propose that this feature is stabilized in an effort to stabilize pipelined compilation. Given the discussion here I would propose a different stabilization path than the one I mentioned above, namely:

• If --color always is passed to the compiler and --error-format=json is also specified then the rendered field of the JSON blob for error message will embed ANSI color codes.

That should be all that's necessary for Cargo (and this would be mirrored in rustdoc). No new flags or anything like that, just reinterpreting what we're already given.

[nikomatsakis]

@rfcbot fcp merge

I am going to propose that we stabilize @alexcrichton's proposal, where --color=always affects the rendering of JSON output:

If --color=always is passed to the compiler and --error-format=json is also specified then the rendered field of the JSON blob for error message will embed ANSI color codes.

We are going however to need:

• [ ] Implementation
• [ ] Tweaks to the documentation for --color=always
• [ ] Ideally an update to the rustc documentation

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

• [x] @Zoxc
• [x] @cramertj
• [ ] @eddyb
• [ ] @estebank
• [ ] @michaelwoerister
• [ ] @nagisa
• [x] @nikomatsakis
• [x] @oli-obk
• [ ] @petrochenkov
• [ ] @pnkfelix
• [ ] @varkor

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@rfcbot fcp cancel

On second though, I'm going to cancel this proposal because I feel like it is mildly backwards incompatible, since --color and JSON output can already be combined.

[rfcbot]

@nikomatsakis proposal cancelled.

[nikomatsakis]

@rfcbot fcp merge

Instead I am going to propose the we stabilize @alexcrichton's original proposal.

Key points:

• --json-rendered is the flag for controlling details of how json output is rendered. It is not expected to be encountered by most users.
• It accepts a small set of strings initially, as described by @alexcrichton, but may be extended in the future to support a comma separated list. This could also be used to describe other kinds of JSON output in the future, as @Zoxc describes here.

There was a short discussion on Zulip between @oli-obk and @Zoxc and I in which we discuss a few mild variations. I would be happy with any of these but I'm going to propose that we go forward with what we have today in the interests of expediency, and because all of these changes seem "backwards compatible" or just not especially important. Still, I think if somebody opened a PR for one of the changes below, we could accept that and still stabilize.

• Rename to --json instead of --json-rendered
• Possibly tweak initial values to begin with diagnostic-foo instead of just foo (e.g., diagnostic-plain instead of plain). This would leave room for future values that are unrelated to diagnostics (e.g., to emit json artifacts of another kind).
• Error if --json-rendered is combined with --color.

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

• [ ] @Zoxc
• [x] @cramertj
• [x] @eddyb
• [x] @estebank
• [x] @michaelwoerister
• [x] @nagisa
• [x] @nikomatsakis
• [x] @oli-obk
• [x] @petrochenkov
• [x] @pnkfelix
• [x] @varkor

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[nikomatsakis]

(I tagged @michaelwoerister while they're on PTO)

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[alexcrichton]

While we're waiting for FCP to finish, I've proposed an implementation of this stabilization at https://github.com/rust-lang/rust/pull/62766

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.