Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add simple markdown formatting to rustc --explain output #112697

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jun 16, 2023

This is a second attempt at #104540, which is #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)

image

After this patch, sample output from the same file:

image

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 if printing with formatting to stdout fails, fallback to printing without formatting
  • Follow --color always/never if supplied
  • 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)

@rustbot rustbot added 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 Jun 16, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jun 16, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@tgross35
Copy link
Contributor Author

I need to tweak the wrapping text printer, it's almost there but sometimes prints slightly too much to the terminal. Do we have a standard algorithm for this anywhere already?

To test with/without the pager, you can override PAGER, PAGER=fake ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --explain E0038 or $env:PAGER='fake'; .\build\x86_64-pc-windows-msvc\stage1\bin\rustc --explain E0038

@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 force-pushed the explain-markdown branch 2 times, most recently from c85f24a to 0ee3834 Compare June 16, 2023 07:13
@bjorn3
Copy link
Member

bjorn3 commented Jun 16, 2023

If formatting is not possible can you keep the current markdown output? I think it is clearer than not having anything to identify code blocks.

@tgross35
Copy link
Contributor Author

That is what it currently does - if --color never is passed or if there is no TTY and the pager doesn't support it, it just uses the raw text

@bjorn3
Copy link
Member

bjorn3 commented Jun 16, 2023

Right, I misread the code.


#[test]
fn test_output() {
let bless = std::env::var("MD_BLESS").unwrap_or_default() == "1";
Copy link
Contributor

Choose a reason for hiding this comment

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

can you connect this to however x test --bless works?

Or make it a ui test that passes --explain and --color=something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adjusted bootstrap to set RUST_BOOTSTRAP_BLESS if --bless is passed, so this and other tests can hook into that if needed. I think this should be an alright way to do it, but let me know if you have something better in mind.

@bors
Copy link
Contributor

bors commented Jun 28, 2023

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

@tgross35 tgross35 force-pushed the explain-markdown branch 2 times, most recently from c050bfa to 8e27acb Compare June 30, 2023 06:41
@oli-obk
Copy link
Contributor

oli-obk commented Jun 30, 2023

@rustbot author

did anything change that I should review?

@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 Jun 30, 2023
@tgross35
Copy link
Contributor Author

Sorry no not yet, I was trying to figure out the best thing for that test (and got some help on zulip).

@tgross35 tgross35 force-pushed the explain-markdown branch from 8e27acb to 39a53e0 Compare July 2, 2023 04:44
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jul 2, 2023
@tgross35 tgross35 force-pushed the explain-markdown branch 2 times, most recently from 29001ab to d8f1c4e Compare July 2, 2023 04:57
@tgross35
Copy link
Contributor Author

tgross35 commented Jul 2, 2023

Alright, with that test change I think this should be good again

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 2, 2023
@@ -2217,6 +2217,11 @@ fn prepare_cargo_test(
) -> Command {
let mut cargo = cargo.into();

// If bless is passed, give downstream crates a way to use it
if std::env::args().any(|arg| arg == "--bless") {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a bless flag you can check: builder.config.cmd.bless()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually came up on Zulip the other day, Clippy and Miri kind of wanted to unite their variables that do the same thing. I did that change in #113298 and requested you for review.

I'll just checkout the relevant parts of that to this one so there's no conflict.

Copy link
Contributor Author

@tgross35 tgross35 Jul 3, 2023

Choose a reason for hiding this comment

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

I take this back, there wound up being some more refactoring in that that I don't want to pull into here. So I just made the change you suggested, I can adjust the other PR if this lands first. (should be good to go here again)

@tgross35 tgross35 force-pushed the explain-markdown branch from d8f1c4e to 917e5b2 Compare July 3, 2023 19:53
Currently, the output of `rustc --explain foo` displays the raw markdown in a
pager. This is acceptable, but using actual formatting makes it easier to
understand.

This patch consists of three major components:

1.  A markdown parser. This is an extremely simple non-backtracking recursive
    implementation that requires normalization of the final token stream
2.  A utility to write the token stream to an output buffer
3.  Configuration within rustc_driver_impl to invoke this combination for
    `--explain`. Like the current implementation, it first attempts to print to
    a pager with a fallback colorized terminal, and standard print as a last
    resort.

    If color is disabled, or if the output does not support it, or if printing
    with color fails, it will write the raw markdown (which matches current
    behavior).

    Pagers known to support color are: `less` (with `-r`), `bat` (aka `catbat`),
    and `delta`.

The markdown parser does not support the entire markdown specification, but
should support the following with reasonable accuracy:

-   Headings, including formatting
-   Comments
-   Code, inline and fenced block (no indented block)
-   Strong, emphasis, and strikethrough formatted text
-   Links, anchor, inline, and reference-style
-   Horizontal rules
-   Unordered and ordered list items, including formatting

This parser and writer should be reusable by other systems if ever needed.
@tgross35 tgross35 force-pushed the explain-markdown branch from 917e5b2 to 6a1c10b Compare July 3, 2023 20:05
@oli-obk
Copy link
Contributor

oli-obk commented Jul 4, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jul 4, 2023

📌 Commit 6a1c10b has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 Jul 4, 2023
@bors
Copy link
Contributor

bors commented Jul 4, 2023

⌛ Testing commit 6a1c10b with merge 2e746c34b26ca2c001c862925a4c90ecd672b406...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jul 4, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 4, 2023
@tgross35
Copy link
Contributor Author

tgross35 commented Jul 4, 2023

@oli-obk could you retry? It looks like check failed downloading nodejs, server must have been iffy or something

@oli-obk
Copy link
Contributor

oli-obk commented Jul 4, 2023

@bors retry

@bors 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 Jul 4, 2023
@bors
Copy link
Contributor

bors commented Jul 5, 2023

⌛ Testing commit 6a1c10b with merge 6dab6dc...

@bors
Copy link
Contributor

bors commented Jul 5, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6dab6dc to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 5, 2023
@bors bors merged commit 6dab6dc into rust-lang:master Jul 5, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jul 5, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6dab6dc): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.2% [-3.5%, -2.9%] 2
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 654.835s -> 655.25s (0.06%)

@tgross35 tgross35 deleted the explain-markdown branch July 5, 2023 16:04
@lqd lqd mentioned this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

7 participants