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 Unicode block-drawing compiler output support #126597

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jun 17, 2024

Add nightly-only theming support to rustc output using Unicode box
drawing characters instead of ASCII-art to draw the terminal UI.

In order to enable, the flags -Zunstable-options=yes --error-format=human-unicode must be passed in.

After:

error: foo
  ╭▸ test.rs:3:3
  │
3 │       X0 Y0 Z0
  │   ┌───╿──│──┘
  │  ┌│───│──┘
  │ ┏││━━━┙
  │ ┃││
4 │ ┃││   X1 Y1 Z1
5 │ ┃││   X2 Y2 Z2
  │ ┃│└────╿──│──┘ `Z` label
  │ ┃└─────│──┤
  │ ┗━━━━━━┥  `Y` is a good letter too
  │        `X` is a good letter
  ╰╴
note: bar
  ╭▸ test.rs:4:3
  │
4 │ ┏   X1 Y1 Z1
5 │ ┃   X2 Y2 Z2
6 │ ┃   X3 Y3 Z3
  │ ┗━━━━━━━━━━┛
  ├ note: bar
  ╰ note: baz
note: qux
  ╭▸ test.rs:4:3
  │
4 │   X1 Y1 Z1
  ╰╴  ━━━━━━━━

Before:

error: foo
 --> test.rs:3:3
  |
3 |       X0 Y0 Z0
  |    ___^__-__-
  |   |___|__|
  |  ||___|
  | |||
4 | |||   X1 Y1 Z1
5 | |||   X2 Y2 Z2
  | |||____^__-__- `Z` label
  | ||_____|__|
  | |______|  `Y` is a good letter too
  |        `X` is a good letter
  |
note: bar
 --> test.rs:4:3
  |
4 | /   X1 Y1 Z1
5 | |   X2 Y2 Z2
6 | |   X3 Y3 Z3
  | |__________^
  = note: bar
  = note: baz
note: qux
 --> test.rs:4:3
  |
4 |   X1 Y1 Z1
  |   ^^^^^^^^

After:

rustc output with unicode box drawing characters

Before:
current rustc output with ASCII art

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 19, 2024
@estebank estebank force-pushed the unicode-output branch 2 times, most recently from 0fb5d30 to 3b7a44f Compare June 20, 2024 02:04
@estebank estebank changed the title [Experiment] Repalce ASCII compiler output with Unicode block-drawing Add Unicode block-drawing compiler output support Jun 20, 2024
@estebank estebank marked this pull request as ready for review June 20, 2024 02:16
@rust-log-analyzer

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Jun 20, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment was marked as resolved.

@rustbot
Copy link
Collaborator

rustbot commented Jun 20, 2024

Some changes occurred in tests/ui/check-cfg

cc @Urgau

@estebank
Copy link
Contributor Author

r? compiler

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@estebank estebank added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2024
@bors

This comment was marked as resolved.

@estebank estebank force-pushed the unicode-output branch 2 times, most recently from 57ff435 to 6fe519d Compare June 24, 2024 17:17
Jake-Shadle added a commit to EmbarkStudios/cargo-deny that referenced this pull request Jul 25, 2024
This change (IMO) improves the diagnostic output by changing the
characters that codespan uses when outputting diagnostics.

Inspired by rust-lang/rust#126597.
@apiraino
Copy link
Contributor

apiraino commented Aug 1, 2024

@estebank do we know what the situation is, about Unicode support (on terminals) for all the platforms that rustc supports?
I'm a fan of UTF-8 but I wonder if this would break diags on some Tier 2/3 we don't actively test. Or on some terminals.

Can you say more about the reasoning behind this change? Why do we want to do this? thanks

@estebank
Copy link
Contributor Author

estebank commented Aug 4, 2024

@apiraino

do we know what the situation is, about Unicode support (on terminals) for all the platforms that rustc supports? I'm a fan of UTF-8 but I wonder if this would break diags on some Tier 2/3 we don't actively test. Or on some terminals.

It's spotty at best. I've tested a few different terminals in different platforms and it is honestly quite a terrible status-quo. Mac terminals seem to all support this output correctly. Windows terminals it's hit or miss. Linux is almost always terrible (unless you have a good combination of terminal + fonts, Konsole for example looks ok).

The idea for the output is to be configurable in ~/.cargo/config.toml, maybe with different default per-platform, but to make this something that individual users can set and forget.

Can you say more about the reasoning behind this change? Why do we want to do this? thanks

There are a couple of things behind this:

  • I keep finding people that have small misunderstandings with the ASCII output: it is not quite as readable as I'd want. I find that the new output is way more readable, particularly for overlapping multiline spans
  • I find the unicode output more pleasing to the eyes, and communicates the same amount of information with slightly more compact output in some cases
  • I wanted to experiment with what was possible while still being constrained by the terminal
  • I wanted to identify what the base components would be if we wanted to ever support theming of the output: I implemented this in such a way that an alternative way of rendering this output is actually possible
  • we've got a goal to migrate to annotate-snippets, I prototyped this here but I want for all this code to eventually just live there
  • I want annotate-snippets to have this rendering option as a carrot for people to try out the new rendering, I know people would jump at the option
  • I wanted to exercise the cargo/rustc communication in order to enable this. I encountered a bunch of places where we were failing to properly propagate the information through for anything other than short error messages (particularly the interaction with json output)
  • "I just think it's neat"

If this lands before we move to annotate-snippets, we'd change the setting for this to actually affect annotate-snippets, so anyone that starts opting-into unicode output will be "enrolled" in the testing of annotate-snippets when we do that move.

@estebank estebank force-pushed the unicode-output branch 2 times, most recently from f3d1fce to bcd2412 Compare August 8, 2024 01:53
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 10, 2024

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

@fmease fmease added I-compiler-nominated Nominated for discussion during a compiler team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 4, 2024
@apiraino
Copy link
Contributor

apiraino commented Sep 5, 2024

Discussed in t-compiler triage on Zulip

Green light 🚦 since it will be nightly-only and opt-in anyway

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Sep 5, 2024
@estebank estebank removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Sep 23, 2024
@bors
Copy link
Contributor

bors commented Sep 25, 2024

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

@bors

This comment was marked as resolved.

Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

r=me after rebase. sorry for the abysmal delay. I have some small nits but they shouldn't really block this PR. esp. since annotate-snippet will likely remedy them

if margin.was_cut_left() {
// We have stripped some code/whitespace from the beginning, make it clear.
buffer.puts(line_offset, code_offset, "...", Style::LineNumber);
buffer.puts(line_offset, code_offset, placeholder, Style::LineNumber);
}
if margin.was_cut_right(line_len) {
Copy link
Member

Choose a reason for hiding this comment

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

Not super important: You could slightly tweak the comment & code inside Margin::was_cut_right? The comment is outdated (refers to "code above" and "...") and hard-codes the offset (it's 6 right now; it that 2 * width(padding)?).

I think was_cut_right is still correct because "...".len() == 3 == "…".len() but otoh I'm not sure if it the usizes represent byte offsets for indexing or for displaying ("len vs width"). So might be worth double checking.

Copy link
Member

Choose a reason for hiding this comment

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

Note that below, you use placeholder.chars().map(|ch| char_width(ch)).sum() to robustly calculate the padding of the placeholder.

Copy link
Member

Choose a reason for hiding this comment

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

… however given the plan to push annotate-snippets forward and eventually use it in the compiler over emitter.rs, I'm sure if these issues matter ^^'.

if margin.was_cut_left() {
// We have stripped some code/whitespace from the beginning, make it clear.
buffer.puts(line_offset, code_offset, "...", Style::LineNumber);
buffer.puts(line_offset, code_offset, placeholder, Style::LineNumber);
Copy link
Member

Choose a reason for hiding this comment

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

I haven't spent too much thought on this but I wonder if things would break (offsets, alignments) if OutputTheme::Ascii.margin().len() != OutputTheme::Unicode.margin().len() (which coincidentally isn't the case for ... and so things just work out). Anyway, see comment chain directly below.

Comment on lines +1071 to +1085
if let AnnotationType::MultilineLine(_) = annotation.annotation_type {
buffer.putc(
p,
(code_offset + annotation.start_col.display).saturating_sub(left),
underline.multiline_vertical,
underline.style,
);
} else {
buffer.putc(
p,
(code_offset + annotation.start_col.display).saturating_sub(left),
underline.vertical_text_line,
underline.style,
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let AnnotationType::MultilineLine(_) = annotation.annotation_type {
buffer.putc(
p,
(code_offset + annotation.start_col.display).saturating_sub(left),
underline.multiline_vertical,
underline.style,
);
} else {
buffer.putc(
p,
(code_offset + annotation.start_col.display).saturating_sub(left),
underline.vertical_text_line,
underline.style,
);
}
buffer.putc(
p,
(code_offset + annotation.start_col.display).saturating_sub(left),
match annotation.annotation_style {
AnnotationType::MultilineLine(_) => underline.multiline_vertical,
_ => underline.vertical_text_line,
},
underline.style,
);

Comment on lines +1087 to 1104
if let AnnotationType::MultilineStart(_) = annotation.annotation_type {
buffer.putc(
p,
line_offset + pos,
(code_offset + annotation.start_col.display).saturating_sub(left),
'|',
style,
underline.bottom_right,
underline.style,
);
}
if let AnnotationType::MultilineEnd(_) = annotation.annotation_type
&& annotation.has_label()
{
buffer.putc(
line_offset + pos,
(code_offset + annotation.start_col.display).saturating_sub(left),
underline.multiline_bottom_right_with_text,
underline.style,
);
}
Copy link
Member

Choose a reason for hiding this comment

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

similarly

@@ -1102,7 +1155,11 @@ impl HumanEmitter {
let style =
if annotation.is_primary { Style::LabelPrimary } else { Style::LabelSecondary };
let (pos, col) = if pos == 0 {
(pos + 1, (annotation.end_col.display + 1).saturating_sub(left))
if annotation.end_col.display == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Could you maybe leave a comment why this is necessary?

@@ -2082,7 +2249,7 @@ impl HumanEmitter {
buffer.putc(
row_num,
(padding as isize + p) as usize,
if part.is_addition(sm) { '+' } else { '~' },
if part.is_addition(sm) { '+' } else { self.diff() },
Copy link
Member

Choose a reason for hiding this comment

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

Side note: For annotate-snippet, it would be nice if symbols like + that currently don't vary by OutputTheme get moved to the "palette".

@fmease fmease 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 Oct 10, 2024
@bors

This comment was marked as resolved.

Add nightly-only theming support to rustc output using Unicode box
drawing characters instead of ASCII-art to draw the terminal UI:

After:

```
error: foo
  ╭▸ test.rs:3:3
  │
3 │       X0 Y0 Z0
  │   ┌───╿──│──┘
  │  ┌│───│──┘
  │ ┏││━━━┙
  │ ┃││
4 │ ┃││   X1 Y1 Z1
5 │ ┃││   X2 Y2 Z2
  │ ┃│└────╿──│──┘ `Z` label
  │ ┃└─────│──┤
  │ ┗━━━━━━┥  `Y` is a good letter too
  │        `X` is a good letter
  ╰╴
note: bar
  ╭▸ test.rs:4:3
  │
4 │ ┏   X1 Y1 Z1
5 │ ┃   X2 Y2 Z2
6 │ ┃   X3 Y3 Z3
  │ ┗━━━━━━━━━━┛
  ├ note: bar
  ╰ note: baz
note: qux
  ╭▸ test.rs:4:3
  │
4 │   X1 Y1 Z1
  ╰╴  ━━━━━━━━
```

Before:

```
error: foo
 --> test.rs:3:3
  |
3 |       X0 Y0 Z0
  |    ___^__-__-
  |   |___|__|
  |  ||___|
  | |||
4 | |||   X1 Y1 Z1
5 | |||   X2 Y2 Z2
  | |||____^__-__- `Z` label
  | ||_____|__|
  | |______|  `Y` is a good letter too
  |        `X` is a good letter
  |
note: bar
 --> test.rs:4:3
  |
4 | /   X1 Y1 Z1
5 | |   X2 Y2 Z2
6 | |   X3 Y3 Z3
  | |__________^
  = note: bar
  = note: baz
note: qux
 --> test.rs:4:3
  |
4 |   X1 Y1 Z1
  |   ^^^^^^^^
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-rustdoc Relevant to the rustdoc 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