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

Match Rust's multiline annotation output #133

Merged
merged 6 commits into from
Jul 10, 2024

Conversation

Muscraft
Copy link
Member

@Muscraft Muscraft commented Jun 19, 2024

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.

tests/formatter.rs Outdated Show resolved Hide resolved
src/snippet.rs Outdated Show resolved Hide resolved
examples/expected_type.svg Outdated Show resolved Hide resolved
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

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

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.

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

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

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?

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 changed the title Properly handle multiple annotations on one line Match Rust's multiline annotation output Jun 20, 2024
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
|
"#]];

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

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

tests/rustc_tests.rs Outdated Show resolved Hide resolved
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
13 checks passed
@Muscraft Muscraft deleted the merge-lines branch July 10, 2024 15:26
@estebank
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants