-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Fix how warnings/errors are displayed to avoid a crash #2251
Conversation
Hm, so in this case, after DCA, we essentially remove any warnings that have spans that got eliminated? Or, shorten any spans that have regions which got eliminated? I'm not sure this is the best way forward. Ideally, we can perform DCA maybe after rendering warnings, so they're not lost based on the result of optimizations. Compile errors and warnings are effectively outputs of the compiler, and optimizations should not cause them to get malformed or omitted. |
I agree that something fishy is going on. I don't know that the warning and its span have been modified... it's just that in this file, we construct a window into the code (input) based on the span and display that but this is a case where the calculations were a bit off. This particular warning is from DCA before any optimizations happen. Do you think there is something wrong with the formation of the warning itself? Or is the warning being modified later somewhere? Or do you think the problem is in construct_window? My fix to construct_window is a bit of a bandaid fix. |
|
||
// The length of input must be at least as big as *end_ix. This is required by the | ||
// annotate_snippets library | ||
&input[calculated_start_ix..std::cmp::max(calculated_start_ix + *end_ix, calculated_end_ix)] |
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 whole function is hard to read. Is end_ix
an index or a length? Adding two indices together doesn't make sense does it? I can't tell what's going on.
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.
You know what? I'm going to study that whole function tomorrow and rewrite it. This change is a bandaid fix and will probably lead to more issues in the future.
The code linked to in #2240 crashes with the following panic:
from https://github.com/rust-lang/annotate-snippets-rs/blob/448b80421b5a9c39c3ed1a708636014aec82eac1/src/display_list/from_snippet.rs#L287
This wasn't happening in
0.17.0
because the problematic warning was dropped in DCA but now show up due to #2230.The requirement is here is that the length of
input
is at least as big as*end_ix
.With this change, the problematic warning shows up as intended:
Honestly, I'm not 100% sure that this is the best way to fix this. I don't fully understand how
construct_window()
works 🤔 . Soliciting help if someone has a better idea.