"Fix" an overflow in byte position math #89046
Conversation
rust-highfive
added
the
S-waiting-on-review
| @@ -782,13 +785,25 @@ impl Span { | |||
| /// ^^^^^^^^^^^^^^^^^ | |||
| /// ``` | |||
| pub fn until(self, end: Span) -> Span { | |||
| let span = self.data(); | |||
| let end = end.data(); | |||
| let span_data = self.data(); | |||
There was a problem hiding this comment.
Most of this function's body is copied from to. We can't just do self.to(end.shrink_to_lo()), because to also does some magic where it uses min/max so it can handle overlapping spans (I think, the logic is a bit wonky).
There was a problem hiding this comment.
Add a comment saying so or refactor the common logic to a new method.
compiler/rustc_errors/src/lib.rs
Outdated
| if prev_hi.line == cur_lo.line { | ||
| // Account for the difference between the width of the current code and the | ||
| // snippet being suggested, so that the *later* suggestions are correctly | ||
| // aligned on the screen. | ||
| trace!(?len); | ||
| acc += len as isize - (cur_hi.col.0 - cur_lo.col.0) as isize; |
There was a problem hiding this comment.
The overflow happens here, because the lines of cur_hi and cur_lo are never checked against each other, so if lo starts in an earlier line but with a higher column, we get an overflow.
Now, we can properly fix this, and probably should, but the span that we have here is nonsensical to begin with. It goes from bar( inside the macro to the start of true at the call site of the macro.
There was a problem hiding this comment.
Ah! Yes, that's why we are getting these malformed spans. Yeah, macros screw everything up. Maybe we should have an assert (not an assert, a check) for well formed spans before we even try to emit or add a suggestion to the DiagnosticBuilder? That would make it so we never show suggestions like this one, while also not breaking the emitter logic. We should probably log that though.
There was a problem hiding this comment.
I hate Spans sourced from macros 🥺
There was a problem hiding this comment.
We could add an actual assert! here. At least if anyone hits it we'll get a traceback.
There was a problem hiding this comment.
By the time we get here, the span doesn't look malformed anymore. The fault is either at the until method or at the caller to it. Maybe we should return an option instead of a span? Or a dummy span in case there are macros involved?
There was a problem hiding this comment.
We could tell that lo > hi, right? And in the Span wrangling methods we should check that both spans belong to the same Ctxt, and if not die in nightly and return only one of the original spans in beta/stable. Changing this to Option would be a royal pain in the neck.
There was a problem hiding this comment.
yea, debug asserting this would be best. We'll have to fix a bunch of diagnostics code that currently relies on it.
There was a problem hiding this comment.
I'm bumping into this overflow (again) in Clippy. Still not sure why it hasn't shown up in CI. But I narrowed it down to a suggestion that replaces an else block, so the block starts at a higher column than where it ends.
There was a problem hiding this comment.
@camsteffen yeah, didn't ping you on this PR when oli noticed this, but it is likely the same issue and this PR should help (but likely give... incorrect suggestions).
There was a problem hiding this comment.
I don't think the problem is only with invalid spans. Here is a minimal reproduction (triggers anonymous_parameters).
trait X {
fn test(x: u32, (
)) {}
}
|
@bors r=estebank |
|
📌 Commit c9fe093 has been approved by |
bors
added
S-waiting-on-bors
"Fix" an overflow in byte position math r? `@estebank` help! I fixed the ICE only to brick the diagnostic. I mean, it was wrong previously (using an already expanded macro span), but it is really bad now XD
Rollup of 8 pull requests Successful merges: - rust-lang#89036 (Fix missing `no_global_oom_handling` cfg-gating) - rust-lang#89041 (Work around invalid DWARF bugs for fat LTO) - rust-lang#89046 ("Fix" an overflow in byte position math) - rust-lang#89127 (Re-enable the `src/test/debuginfo/mutex.rs` test on Windows) - rust-lang#89133 (Fix ICE with `--cap-lints=allow` and `-Zfuel=...=0`) - rust-lang#89162 (rustc_index: Add some map-like APIs to `IndexVec`) - rust-lang#89164 (Document `--show-type-layout` in the rustdoc book) - rust-lang#89170 (Disable the leak sanitizer on Macos aarch64 for now) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
r? @estebank
help! I fixed the ICE only to brick the diagnostic.
I mean, it was wrong previously (using an already expanded macro span), but it is really bad now XD