Fix erroneous span for borrowck error #98022
Conversation
rustbot
added
the
T-compiler
|
r? @wesleywiser (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Correct me if I'm wrong, but I don't think this PR in its current state fixes #97997 as that issue is about the duplicate error, not the wrong location. Looking at the new test, this PR expect the error to be duplicated, which means merging this shouldn't mark the issue fixed. |
|
@bugadani: The error is deduplicated by the diagnostic printing logic, except for in UI tests, and renders as one if you invoke rustc normally. |
|
The reason it shows up as two separate errors when you include that extra unrelated line in the original github issue was precisely because the spans for the two copies of the error (that are always printed) differ, meaning that they don't get deduplicated by the compiler. |
|
I was more curious about getting feedback from someone who's an expert on MIR than an expert on diagnostics. My understanding is that this isn't really a diagnosics issue per se, but an issue with Location type which is a MIR concept. |
| // HACK(compiler-errors): Constants that are gathered into Body.required_consts | ||
| // have their locations erased... | ||
| let locations = if location != Location::START { | ||
| location.to_locations() | ||
| } else { | ||
| Locations::All(constant.span) | ||
| }; |
There was a problem hiding this comment.
It seems like you've identified the right course of action (modifying Body.required_consts instead of here).
I'd be ok with landing this as a temporary hack, but I'd rather getting sign off from somebody immediately related to the const generics effort. I'd punt to @oli-obk, but he'll be somewhat unavailable.
There was a problem hiding this comment.
I wonder if we should just unconditionally use the constant's span, it seems like that span will always be better than what we can guess from the location (or we'll just end up fetching the constant's span at the location anyway). At which point it begs the question of whether we should be passing a location to visit_constant at all.
A quick skim of all visit_constant and visit_const suggest that we're only really interested in the span, not the actual mir location.
anyway. Leaving the hack comment and fixing this in a follow up PR seems the quickest way forward.
There was a problem hiding this comment.
Yeah, I looked into that refactoring, but we also use Location (and not Locations) as a key to some different HashMaps in different MIR stages? I'll see if I can give the refactoring another go, though.
|
r? @oli-obk |
|
@bors r+ rollup |
|
📌 Commit ccf6124 has been approved by |
bors
added
S-waiting-on-bors
…span, r=oli-obk Fix erroneous span for borrowck error I am not confident that this is the correct fix, but it does the job. Open to suggestions for a real fix instead. Fixes rust-lang#97997 The issue is that we pass a [dummy location](https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_middle/mir/visit.rs.html#302) when type-checking the ["required consts"](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.Body.html#structfield.required_consts) that are needed by the MIR body during borrowck. This means that when we fail to evaluate the constant, we use the span of `bb0[0]`, instead of the actual span of the constant. There are quite a few other places that use `START_BLOCK.start_location()`, `Location::START`, etc. when calling for a random/unspecified `Location` value. This is because, unlike (for example) `Span`, we don't have a dummy/miscellaneous value to use instead. I would appreciate guidance (either in this PR, or a follow-up) on what needs to be done to clean this up in general.
Rollup of 11 pull requests Successful merges: - rust-lang#94033 (Improve docs for `is_running` to explain use case) - rust-lang#97269 (adjust transmute const stabilization version) - rust-lang#97805 (Add proper tracing spans to rustc_trait_selection::traits::error_reporting) - rust-lang#98022 (Fix erroneous span for borrowck error) - rust-lang#98124 (Improve loading of crates.js and sidebar-items.js) - rust-lang#98278 (Some token stream cleanups) - rust-lang#98306 (`try_fold_unevaluated` for infallible folders) - rust-lang#98313 (Remove lies in comments.) - rust-lang#98323 (:arrow_up: rust-analyzer) - rust-lang#98329 (Avoid an ICE and instead let the compiler report a useful error) - rust-lang#98330 (update ioslice docs to use shared slices) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
I am not confident that this is the correct fix, but it does the job. Open to suggestions for a real fix instead.
Fixes #97997
The issue is that we pass a dummy location when type-checking the "required consts" that are needed by the MIR body during borrowck. This means that when we fail to evaluate the constant, we use the span of
bb0[0], instead of the actual span of the constant.There are quite a few other places that use
START_BLOCK.start_location(),Location::START, etc. when calling for a random/unspecifiedLocationvalue. This is because, unlike (for example)Span, we don't have a dummy/miscellaneous value to use instead. I would appreciate guidance (either in this PR, or a follow-up) on what needs to be done to clean this up in general.