-
Notifications
You must be signed in to change notification settings - Fork 36
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
Conversation
tests/formatter.rs
Outdated
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
tests/formatter.rs
Outdated
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 | ||
| |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
tests/rustc_tests.rs
Outdated
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 | ||
| | ||
"#]]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
@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. |
Configuration breeds complexity. Just for what is in this PR, I don't think configuration is worth it. |
One big difference between Rust's output and
annotate-snippets
is thatannotate-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.