Match Rust's multiline annotation output #133

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

Jump to bottom

Merged

Muscraft merged 6 commits into rust-lang:master from Muscraft:merge-lines

Jul 10, 2024

Merged

Match Rust's multiline annotation output #133

Muscraft merged 6 commits into rust-lang:master from Muscraft:merge-lines

Jul 10, 2024

Conversation

Muscraft

Copy link

Member

@Muscraft Muscraft commented Jun 19, 2024 •

edited

Loading

One big difference between Rust's output and annotate-snippets is that annotate-snippets did not work well when multiple annotations were on a single line. To show this, I added tests for multiple annotations per line and tests from Rust's parser tests. Then, in the next few commits I worked to match Rust's output.

Note: Most of the code to make this work was adapted from Rust's Emitter. This has the added benefit of making our output much closer to Rust's.

@Muscraft


 test: Add tests for multiple annotations per line

52bf734

epage

epage reviewed

Jun 20, 2024

View reviewed changes

tests/formatter.rs Outdated Show resolved Hide resolved

epage

epage reviewed

Jun 20, 2024

View reviewed changes

src/snippet.rs Outdated Show resolved Hide resolved

epage

epage reviewed

Jun 20, 2024

View reviewed changes

tests/fixtures/no-color/multiple_annotations.svg Outdated Show resolved Hide resolved

epage

epage reviewed

Jun 20, 2024

View reviewed changes

examples/expected_type.svg Outdated Show resolved Hide resolved

epage

epage reviewed

Jun 20, 2024

View reviewed changes

tests/formatter.rs Outdated

Comment on lines 803 to 797

4 | bar = { version = "0.1.0", optional = true }

| __________________________________________^

| --------------- info: This should also be long but not too long

| ____________________________--------------^

| | |

| | info: This should also be long but not too long

Copy link

Contributor

@epage epage Jun 20, 2024 •

edited

Loading

Choose a reason for hiding this comment

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

Am I reading this right that the annotation spans overlap but the rendering is not showing that?

~~(削除) Also, whats with the trailing caret? (削除ここまで)~~ start/end of multi-line

Copy link

Member Author

@Muscraft Muscraft Jun 20, 2024

Choose a reason for hiding this comment

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

Yes, they do overlap, but it is being shown. Multiline annotation underlines get written first, then any - and ^. In this case it is showing they overlap with the - intersecting the multiline annotation.

epage

epage reviewed

Jun 20, 2024

View reviewed changes

tests/formatter.rs

Comment on lines 846 to 848

4 | bar = { version = "0.1.0", optional = true }

| __________________________________________^

| _________^

| --------------- info: This should also be long but not too long

| _________^__________________--------------^

| | | |

| |_________| info: This should also be long but not too long

| ||

5 | || this is another line

6 | || so is this

7 | || bar = { version = "0.1.0", optional = true }

| ||__________________________________________^ I need this to be really long so I can test overlaps

| ||_________________________^ I need this to be really long so I can test overlaps

| ||_________________________^________________^ I need this to be really long so I can test overlaps

| |__________________________|

| I need this to be really long so I can test overlaps

Copy link

Contributor

@epage epage Jun 20, 2024

Choose a reason for hiding this comment

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

Is the diff being funky or is this really confusing to see the spans highlighted?

Copy link

Member Author

@Muscraft Muscraft Jun 20, 2024

Choose a reason for hiding this comment

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

Maybe I have the curse of knowledge, but it makes sense to me; what part is confusing?

Copy link

@estebank estebank Jun 20, 2024

Choose a reason for hiding this comment

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

The diff makes it harder to read, but the new output matches rustc's

@Muscraft Muscraft force-pushed the merge-lines branch from 91875dd to 0e283cc Compare

June 20, 2024 03:28

@Muscraft Muscraft changed the title ~~(削除) Properly handle multiple annotations on one line (削除ここまで)~~ (追記) Match Rust's multiline annotation output (追記ここまで)

Jun 20, 2024

estebank

estebank reviewed

Jun 20, 2024

View reviewed changes

tests/rustc_tests.rs

Comment on lines 426 to 476

fn multiple_labels_primary_without_message_2() {

let source = r#"

fn foo() {

a { b { c } d }

}

"#;

let input = Level::Error.title("foo").snippet(

Snippet::source(source)

.line_start(1)

.origin("test.rs")

.fold(true)

.annotation(Level::Error.span(18..25).label("`b` is a good letter"))

.annotation(Level::Error.span(14..27).label(""))

.annotation(Level::Error.span(22..23).label("")),

);

// This is all `^` until we support primary and secondary labels

let expected = str![[r#"

error: foo

--> test.rs:3:7

3 | a { b { c } d }

| ^^^^^^^^^^^^^

| |

| `b` is a good letter

"#]];

Copy link

@estebank estebank Jun 20, 2024

Choose a reason for hiding this comment

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

For this case, you really want to have different label levels, so that you can confirm that overlapping spans of different levels do not "hide" each other. In rustc we ensure we print from widest to shortest span, so that this ends up like ^^^^-----^^^^ or ----^^^^----. This isn't perfect but it is the best we can do when restricted to ASCII and a single line of text.

Copy link

Contributor

@epage epage Jul 10, 2024

Choose a reason for hiding this comment

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

... yeah, I think my comment from the other issue stands. I think rustc is being too "smart" and a lot of these overlapping spans and weird cases are bonkers for a user to decipher

estebank

estebank reviewed

Jun 20, 2024

View reviewed changes

tests/rustc_tests.rs Outdated Show resolved Hide resolved

Muscraft added 4 commits

June 20, 2024 15:24

@Muscraft


 test: Add some of Rust's parser tests

d1d3a62

@Muscraft


 fix: Properly handle multiple annotations on one line

9ec2939

@Muscraft


 feat: Merge multiline annotations with matching spans

@Muscraft


 feat: Match Rust's multiline start special case

66bbd82

@Muscraft Muscraft force-pushed the merge-lines branch from 0e283cc to 66bbd82 Compare

June 20, 2024 21:39

@Muscraft


 feat: Match Rust's overlapping multiline starts

b25bd3e

estebank

estebank approved these changes

Jun 24, 2024

View reviewed changes

epage

epage approved these changes

Jul 10, 2024

View reviewed changes

Copy link

Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

To be blunt, I think this PR is a major regression and that rustc is in the wrong to render overlapping spans. However, rustc is the "gold standard" and matching it is the goal, so I'm reluctantly fine with this moving forward.

@Muscraft Muscraft merged commit 106f70c into rust-lang:master

Jul 10, 2024

@Muscraft Muscraft deleted the merge-lines branch

July 10, 2024 15:26

@estebank

Copy link

estebank commented Jul 11, 2024

@epage I think it is reasonable for annotate-snippets to (eventually) allow configuring this (as other tools might want different behavior).

For the future, I'd want something more bold: being able to emit SVG output with much fancier output that can cram multiple different separated underlines in the space a single line of text would fit, and as an even bigger stretch, use sixels for rendering the underlines. But none of these things are reasonable to attempt 1) now or 2) in rustc first.

@epage

Copy link

Contributor

epage commented Jul 11, 2024

Configuration breeds complexity. Just for what is in this PR, I don't think configuration is worth it.

Labels

None yet

3 participants

@Muscraft @estebank @epage

Match Rust's multiline annotation output #133

Match Rust's multiline annotation output #133

Uh oh!

Conversation

@Muscraft Muscraft commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

@epage epage Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@Muscraft Muscraft Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

@epage epage Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

@Muscraft Muscraft Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

@estebank estebank Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

@estebank estebank Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

@epage epage Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@epage epage left a comment

Choose a reason for hiding this comment

Uh oh!

estebank commented Jul 11, 2024

Uh oh!

epage commented Jul 11, 2024

Uh oh!

Uh oh!

@Muscraft Muscraft commented Jun 19, 2024 •

edited

Loading

@epage epage Jun 20, 2024 •

edited

Loading