-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Implement MultiSpan error reporting #30411
Conversation
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
☔ The latest upstream changes (presumably #30410) made this pull request unmergeable. Please resolve the merge conflicts. |
(rebased) |
for sp in msp.spans.as_slice().iter() { | ||
let hi = cm.lookup_char_pos(sp.hi); | ||
if hi.line < (last_line.line_index + 1) { | ||
continue |
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 is a bit unfortunate.
3817189
to
06dda69
Compare
☔ The latest upstream changes (presumably #30457) made this pull request unmergeable. Please resolve the merge conflicts. |
8e858e8
to
bce9176
Compare
/// and point into the same FileMap. | ||
#[derive(Clone)] | ||
pub struct MultiSpan { | ||
pub spans: SmallVector<Span> |
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.
Using SmallVector
here seemed nice at first because
- converting a
Span
into aMultiSpan
is cheap const COMMAND_LINE_MSP
can exist and be used inside patterns (well, after the next snapshot -> Const fn causes ICE in a pattern #30117)
Since this only matters once we actually want to report something this should probably be Vec<Span>
?
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 don't think perf in the error case is super-important, so just using Vec is probably better (and save all the changes to SmallVec, which I would otherwise say need some tests).
I've now implemented interleaved span/code-line rendering, making this possible:
But eliding lines like this would make it difficult to find the line in the source once the error messages have been restyled (#3533, #29989), because the line numbers (here line 5) would be missing. Because of this spans that are automatically grouped would render like this:
|
☔ The latest upstream changes (presumably #30542) made this pull request unmergeable. Please resolve the merge conflicts. |
59ea72d
to
83e50b5
Compare
(rebased) |
I've reverted the no-line-elision behaviour mentioned in my last comment because I've acted prematurely (sorry for all the noise) since the line numbers may stay after all, and I think that this nicely reduces clutter.
|
review ping @sfackler |
r? @nrc I think you've been working with diagnostic output. |
☔ The latest upstream changes (presumably #31214) made this pull request unmergeable. Please resolve the merge conflicts. |
buf | ||
} | ||
|
||
pub fn custom_group_spans<F>(&self, mut spans: Vec<Span>, push: F) -> Vec<MultiSpan> |
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.
could you add a comment please?
I think the overall approach here is good, but have a few questions about the details. |
2e396a8
to
4a7f008
Compare
(updated) |
Apologies, I may have spoken too soon. These (group_*) are purely functions for turning many spans into a single MulitSpan? In which case, I think keeping them with the other Span functions in codemap.rs is fine (we might want to break out all the spans stuff into its own module since that file is getting pretty big, but that can wait till later).
|
Ok, I've limited it to a single span + suggestion, but left the backend support (-> |
@mitaa looks good, thanks for making the changes! Could you squash the commits please? Then r+ |
This allows to render multiple spans on one line, or to splice multiple replacements into a code suggestion.
(squashed) Thanks @nrc! |
@bors: r+ |
📌 Commit 727f959 has been approved by |
This allows to render multiple spans on one line, or to splice multiple replacements into a code suggestion. fixes #28124
This passed all tests but buildbot lost a slave so homu didn't merge, gonna merge manually as I'm restarting buildbot anyway. |
This allows to render multiple spans on one line, or to splice multiple replacements into a code suggestion.
fixes #28124