-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Show multiline spans in full if short enough #37369
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
// If the span is multi-line, simplify down to the span of one character | ||
if lo.line != hi.line { | ||
// If the span is long multi-line, simplify down to the span of one character | ||
let max_multiline_span_length = 20; |
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.
This will need to be much lower. I propose a value between 5 and 10.
af0d31c
to
5a81d01
Compare
5a81d01
to
3ff63dd
Compare
Hi there! I think this is nice work but I am unsure if this is the right way. I would suggest to use the box-drawing characters for the lines. This would result in a cleaner look. Like this
|
@dawirstejeck as per @nikomatsakis in the original PR:
I think it'd be neat if there were a way to probe for unicode awareness and use ascii safe characters or unicode accordingly, but that's beyond the scope of this PR :) |
6f80da2
to
41c9950
Compare
…-back, r=nikomatsakis Include type of missing trait methods in error Provide either a span pointing to the original definition of missing trait items, or a message with the inferred definitions. Fixes rust-lang#24626. Follow up to PR rust-lang#36371. If PR rust-lang#37369 lands, missing trait items that present a multiline span will be able to show the entirety of the item definition on the error itself, instead of just the first line.
Carrying over reviewer from the original PR. Looks like the conclusion there was to close in favor of an internals thread and open up a new one upon reaching consensus. With my reading of the internals thread it looks like we haven't quite reached consensus yet, but @nikomatsakis seems to have been following more closely than I and may know more! @jonathandturner you were also interested in the first PR so you're likely to be interested in this one :) |
Neat feature! |
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.
Hmm, so I see that the #[test]
infrastructure for testing snippet highlighting has been removed and move to UI tests. That's a bit unfortunate here, as it would have been really easy to make a bunch of tests for edge cases. I'm trying to think now if there is an easy way to reproduce it. Perhaps just adding back the #[test]
stuff, but as @jonathandturner pointed out, it IS kind of a pain.
Let me poke around a bit. I'd love to land this PR and don't want to block it on extensive work to build up a nice unit-testing infrastructure -- on the other hand, I'd feel a lot more comfortable if we could just write down tests for various edge cases (even if they don't occur in rustc...yet). The code seems pretty clean but oveall this formatting logic is easier to verify as a block-box, frankly.
|
||
// Find overlapping multiline annotations, put them at different depths | ||
multiline_annotations.sort_by(|a, b| a.1.start_col.cmp(&b.1.start_col)); | ||
for item in multiline_annotations.clone() { |
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.
This code confuses me a bit -- why doesn't it have to consider lines at all?
What if the input was like
X0
...
X1
Y0
...
Y1
With two multi-line spans stretching from X0..X1 and Y0..Y1... wouldn't that cause it to indent the Y case even though they do not in fact overlap?
I think I would expect this indentation to be done on the basis not of start_col
and end_col
but rather a (line, col) pair, or something like that.
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.
Only the line needs to be taken into account. I probably just made a mistake here that, as I didn't have any overlapping example to test against, I didn't catch.
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.
Fixed.
|
||
let mut max_depth = 0; // max overlapping multiline spans | ||
for (file, ann) in multiline_annotations { | ||
if let AnnotationType::Multiline {line_start, line_end, depth} = ann.annotation_type { |
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.
all the annotations in this vector should be multiline, right? If so, can you add else { panic!("non-multiline annotation in
multiline_annotations!"); }
to make this clearer?
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.
Added.
Bah, I didn't get time to finish implementing that test harness. =( The basic idea was just going to be adding some |
OK, so, I'm sorry I've been letting the perfect be the enemy of the good here. Let's land this PR and hack on a test harness separately. I've been toying with some stuff locally but the setup is kind of a pain so I don't want to block things on it. @bors r+ |
📌 Commit 41c9950 has been approved by |
@bors r- Actually, I'd still like to know the answer to this question and have this nit addressed. @estebank, you around? |
@nikomatsakis sure thing. I'll update the PR this afternoon. |
If you publish branch I can use as a base to help, please let me know. Also, #12182 covers exactly that work, I believe. |
@estebank ok I got something working in this branch: https://github.com/nikomatsakis/rust/tree/multiline-span In particular, you can just cherry-pick this commit: The tests are all calls to this To run the tests, do |
Awesome!
It actually looks exactly how I expected it to look, as in, failing in the exact ways I expected it to be failing :)
I would like to arrive with you to what the appropriate output for this case would be, where the multispans start (or end) both on the same line. I'm thinking something along the lines of:
or
Both cases seem a bit messy, to honest. This might be not as accurate but more composable:
Which for the PR's example would look like:
|
Yes, but this is an extreme case. I mostly want us to do "something sensible". I wonder if we can provide a bit of extra info, though, to help people disambiguate, like a number. Something like this (riffing on my preferred variant):
Maybe we only show these if things overlap? |
Oh, and I removed the |
2dc9cfc
to
935c0df
Compare
When dealing with multiline spans that span few lines, show the complete span instead of restricting to the first character of the first line. For example, instead of: ``` % ./rustc foo.rs error[E0277]: the trait bound `{integer}: std::ops::Add<()>` is not satisfied --> foo.rs:13:9 | 13 | foo(1 + bar(x, | ^ trait `{integer}: std::ops::Add<()>` not satisfied | ``` show ``` % ./rustc foo.rs error[E0277]: the trait bound `{integer}: std::ops::Add<()>` is not satisfied --> foo.rs:13:9 | 13 | foo(1 + bar(x, | ________^ starting here... 14 | | y), | |_____________^ ...ending here: trait `{integer}: std::ops::Add<()>` not satisfied | ```
935c0df
to
eb53ca3
Compare
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.
Left one suggestion. One other thing I'd like: can you give a test with two disjoint ranges (touching distinct lines)?
I confess I didn't read the actual code in detail, but your comments gave me confidence that I could read it if ever I had to. =) The tests seem quite good though.
Basically r=me modulo the suggested refactoring and test (and feel free to tell me if you think my suggestion is not a good one).
} | ||
|
||
// Find overlapping multiline annotations, put them at different depths | ||
multiline_annotations.sort_by(|a, b| { |
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.
Nit: this code might be cleaner if you made a struct for MultilineAnnotation
, and then adjusted the enum to wrap the struct. Something like:
enum Annotation {
...
Multiline(MultilineAnnotation),
...
}
struct MultilineAnnotation {
depth: usize,
line_start: usize,
line_end: usize,
}
in that case, the type of multiline_annotations
can be Vec<MultilineAnnotation>
.
// on a different line as the underline. | ||
// | ||
// After this we will have: | ||
// |
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.
❤️ loving the comments! ❤️
There's some duplication of fields now for this to work, but I don't think it'll make the code any harder to follow.
Other than the issue mentioned above, the suggestion makes sense. |
1ef8ede
to
d9c370e
Compare
d9c370e
to
b7982bb
Compare
@bors r+ Very nice. |
📌 Commit b7982bb has been approved by |
Show multiline spans in full if short enough When dealing with multiline spans that span few lines, show the complete span instead of restricting to the first character of the first line. For example, instead of: ``` % ./rustc file2.rs error[E0277]: the trait bound `{integer}: std::ops::Add<()>` is not satisfied --> file2.rs:13:9 | 13 | foo(1 + bar(x, | ^ trait `{integer}: std::ops::Add<()>` not satisfied | ``` show ``` % ./rustc file2.rs error[E0277]: the trait bound `{integer}: std::ops::Add<()>` is not satisfied --> file2.rs:13:9 | 13 | foo(1 + bar(x, | ________^ starting here... 14 | | y), | |_____________^ ...ending here: trait `{integer}: std::ops::Add<()>` not satisfied | ``` The [proposal in internals](https://internals.rust-lang.org/t/proposal-for-multiline-span-comments/4242/6) outlines the reasoning behind this.
After the fix of rust-lang#37453 in PR rust-lang#37369, instead of pointing at only the cast type, point at the full cast span when a cast needs a dereference: ``` error: casting `&{float}` as `f32` is invalid --> ../../../src/test/ui/mismatched_types/cast-rfc0401.rs:81:30 | 81 | vec![0.0].iter().map(|s| s as f32).collect::<Vec<f32>>(); | ^^^^^^^^ cannot cast `&{float}` as `f32` | help: did you mean `*s`? --> ../../../src/test/ui/mismatched_types/cast-rfc0401.rs:81:30 | 81 | vec![0.0].iter().map(|s| s as f32).collect::<Vec<f32>>(); | ^ ``` instead of ``` error: casting `&{float}` as `f32` is invalid --> ../../../src/test/ui/mismatched_types/cast-rfc0401.rs:81:35 | 81 | vec![0.0].iter().map(|s| s as f32).collect::<Vec<f32>>(); | - ^^^ | | | | | did you mean `*s`? | cannot cast `&{float}` as `f32` ```
When cast needs a dereference point at full cast After the fix of #37453 in PR #37369, instead of pointing at only the cast type, point at the full cast span when a cast needs a dereference, as well as assign the error label to the correct span for proper coloring: <img width="471" alt="error span pointing at the entire cast" src="https://cloud.githubusercontent.com/assets/1606434/21024245/8797fc2e-bd38-11e6-82c1-66c281c656c1.png"> instead of <img width="471" alt="error span pointing at the type of the cast" src="https://cloud.githubusercontent.com/assets/1606434/21023777/d4814aa6-bd36-11e6-9fc3-b2a0ea5ee15d.png"> Move `compile-fail` test to `ui` test.
…atsakis When cast needs a dereference point at full cast After the fix of rust-lang#37453 in PR rust-lang#37369, instead of pointing at only the cast type, point at the full cast span when a cast needs a dereference, as well as assign the error label to the correct span for proper coloring: <img width="471" alt="error span pointing at the entire cast" src="https://cloud.githubusercontent.com/assets/1606434/21024245/8797fc2e-bd38-11e6-82c1-66c281c656c1.png"> instead of <img width="471" alt="error span pointing at the type of the cast" src="https://cloud.githubusercontent.com/assets/1606434/21023777/d4814aa6-bd36-11e6-9fc3-b2a0ea5ee15d.png"> Move `compile-fail` test to `ui` test.
When dealing with multiline spans that span few lines, show the complete span instead of restricting to the first character of the first line.
For example, instead of:
show
The proposal in internals outlines the reasoning behind this.