Skip to content

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Nov 17, 2022

The goal here is to provide some sort of super basic markdown formatting to rustc --explain output when available. This is a minimal alternative to #63128 without additiional dependencies.

There's a lot of tweaking to do but I'm creating this draft PR because I could use some guidance on things like:

  • What crate does this belong in? Currently it's in rustc_driver because that's where handle_explain is called, but it might make sense in rustc_errors which currently does some output formatting
  • Should this tie in with WritableDst in rustc_errors/src/emitter.rs? It seems like this is used to indicate TTY-ness, but I can't find it in use anywhere
  • What is the correct way to figure out if this is a TTY?

Currently, the output just displays raw markdown. This works of course, but it really doesn't look very elegant. (output is rustc --explain E0038)

image

After this patch, sample output from the same file:

image

@rustbot
Copy link
Collaborator

rustbot commented Nov 17, 2022

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 17, 2022
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 21, 2022

cc @rust-lang/wg-diagnostics

@estebank
Copy link
Contributor

I feel like it'd be reasonable to pull in a markdown parser if there was a reasonable one (small, maintained) with a compatible license for this.

Also, you might be interested in seeing if this very old PR would be still usable for code highlighting: #39300.

@estebank
Copy link
Contributor

After talking with Oli, I'm ok with using regexes for this, given that there will be no adversarial content.

@estebank
Copy link
Contributor

What crate does this belong in? Currently it's in rustc_driver because that's where handle_explain is called, but it might make sense in rustc_errors which currently does some output formatting

rustc_errors makes sense to me

Should this tie in with WritableDst in rustc_errors/src/emitter.rs? It seems like this is used to indicate TTY-ness, but I can't find it in use anywhere

I think what you want to look at is EmitterWriter::stderr.

What is the correct way to figure out if this is a TTY?

This is what rustc does

ColorConfig::Auto if io::stderr().is_terminal() => ColorChoice::Auto,

@oli-obk
Copy link
Contributor

oli-obk commented Dec 13, 2022

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 13, 2022
@tgross35
Copy link
Contributor Author

tgross35 commented Dec 19, 2022

I have a few tweaks to make but I'll mark this as ready for review to get some feedback at this point.

It turned out that a super simple parser was somewhat easier to get right than markdown, so I went that route instead (still use regex to parse [...](...) and <...> links for now). So far support works for:

  • Plain text
  • Italics
  • Bold
  • Inline code
  • Headers 1-4 (allows nesting of any of the above)
  • Unordered lists
  • Inline links [...](...) and <...>

Still todo:

  • Separate links ([...]: ... later in file)
  • Strikethrough (this is blocked by termcolor not supporting it)
  • Enable for some different pagers that support color formatting (currently only less with -r option)

Example output from E0038:

image

And generic example:

image

I saw your comment @estebank about 39300 for code formatting, but I'd like to reintroduce that in a separate PR if that's ok, just to keep this one scoped. It will be easy to plug in separately

@tgross35 tgross35 marked this pull request as ready for review December 19, 2022 21:22
Comment on lines +4 to +5
#![warn(clippy::pedantic)]
#![warn(clippy::nursery)]
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, we don't use clippy on rustc yet.

@bors
Copy link
Collaborator

bors commented Jan 6, 2023

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

@albertlarsan68

This comment was marked as off-topic.

@rustbot rustbot removed the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 18, 2023
@JohnCSimon
Copy link
Member

@tgross35

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Mar 17, 2023
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Mar 17, 2023
@tgross35
Copy link
Contributor Author

Sorry about that, it's been on my todo list but I just haven't gotten to it. Good decision, I will revisit at a later time 👍

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 5, 2023
Add simple markdown formatting to `rustc --explain` output

This is a second attempt at rust-lang#104540, which is rust-lang#63128 without dependencies.

This PR adds basic markdown formatting to `rustc --explain` output when available. Currently, the output just displays raw markdown: this works of course, but it really doesn't look very elegant. (output is `rustc --explain E0038`)

<img width="583" alt="image" src="https://github.com/rust-lang/rust/assets/13724985/ea418117-47af-455b-83c0-6fc59276efee">

After this patch, sample output from the same file:

<img width="693" alt="image" src="https://github.com/rust-lang/rust/assets/13724985/12f7bf9b-a3fe-4104-b74b-c3e5227f3de9">

This also obeys the `--color always/auto/never` command option. Behavior:

- If pager is available and supports color, print with formatting to the pager
- If pager is not available or fails print with formatting to stdout - otherwise without formatting
- Follow `--color always/never` if suppied
- If everything fails, just print plain text to stdout

r? `@oli-obk`
cc `@estebank`
(since the two of you were involved in the previous discussion)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants