-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Use larger span for adjustment THIR expressions #89110
Conversation
r? @wesleywiser (rust-highfive has picked a reviewer for you, use r? to override) |
r? @estebank |
Note that we don't currently explain that an autoderef/autoborrow was the cause of the borrow that resulted in an error. Doing so would require tracking additional information about THIR expressions / MIR statements, since we can't otherwise distinguish between a user-written However, I don't think this change makes the situation any more confusing. Previously, a user might be lead to incorrectly ask "Why does writing |
This comment has been minimized.
This comment has been minimized.
df00c68
to
45b4d33
Compare
This comment has been minimized.
This comment has been minimized.
r=me after fixing the tests |
45b4d33
to
e489fb9
Compare
This comment has been minimized.
This comment has been minimized.
It looks like storing the additional span in |
If you somehow keep two spans around, I would love it if we could point at both the receiver and the call with different labels to maybe explain what's happening better, but that might be too involved for this PR. |
☔ The latest upstream changes (presumably #88804) made this pull request unmergeable. Please resolve the merge conflicts. |
e489fb9
to
f33117f
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit f33117fb9d8b1559497dd8d617e088d08e6e3345 with merge ac898de96c454463a122001d8e531e7be605a152... |
☀️ Try build successful - checks-actions |
Queued ac898de96c454463a122001d8e531e7be605a152 with parent 043972f, future comparison URL. |
Finished benchmarking commit (ac898de96c454463a122001d8e531e7be605a152): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. @bors rollup=never |
Currently, we use a relatively 'small' span for THIR expressions generated by an 'adjustment' (e.g. an autoderef, autoborrow, unsizing). As a result, if a borrow generated by an adustment ends up causing a borrowcheck error, for example: ```rust let mut my_var = String::new(); let my_ref = &my_var my_var.push('a'); my_ref; ``` then the span for the mutable borrow may end up referring to only the base expression (e.g. `my_var`), rather than the method call which triggered the mutable borrow (e.g. `my_var.push('a')`) Due to a quirk of the MIR borrowck implementation, this doesn't always get exposed in migration mode, but it does in many cases. This commit makes THIR building consistently use 'larger' spans for adjustment expressions The intent of this change it make it clearer to users when it's the specific way in which a variable is used (for example, in a method call) that produdes a borrowcheck error. For example, an error message claiming that a 'mutable borrow occurs here' might be confusing if it just points at a usage of a variable (e.g. `my_var`), when no `&mut` is in sight. Pointing at the entire expression should help to emphasize that the method call itself is responsible for the mutable borrow. In several cases, this makes the `#![feature(nll)]` diagnostic output match up exactly with the default (migration mode) output. As a result, several `.nll.stderr` files end up getting removed entirely.
f33117f
to
4d66986
Compare
@estebank: I rewrote the implementation entirely - only |
@bors r+ |
⌛ Testing commit 4d66986 with merge 85de977ecd19afc104105755aba47eb2521d2347... |
💥 Test timed out |
@bors retry |
⌛ Testing commit 4d66986 with merge 700edf49ebfdb401f007379f83aebd2d6c8cb752... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
@bors retry |
@bors treeclosed- |
☀️ Test successful - checks-actions |
Finished benchmarking commit (4aa7879): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
…stment-error, r=oli-obk (re-)tighten sourceinfo span of adjustments in MIR Diagnostics rely on the spans of MIR statements being (approximately) correct in order to give suggestions relative to that span (i.e. `shrink_to_hi` and `shrink_to_lo`). I discovered that we're *intentionally* lowering THIR exprs with their parent expr's span if they come from adjustments that are due to a parent expression. While I understand why that may be desirable to demonstrate the relationship of an adjustment and the expression that requires it, it leads to 1. very verbose borrowck output 2. incorrect spans for suggestions Some diagnostics get around that by giving suggestions relative to other spans we've collected during MIR lowering, such as the span of the method's identifier (e.g. `name` in `.name()`), but this doesn't work too well when things come from desugaring. I assume it also has lead to numerous tweaks and complications to diagnostics code down the road, which this PR doesn't necessarily aim to fix but may open the gates to fixing later... The last three commits are simplifications due to the fact that we can assume that the move span actually points to what is being moved (and a test). This regressed in rust-lang#89110, which was debated somewhat in rust-lang#90286. cc `@Aaron1011` who originally made this change. r? diagnostics Fixes rust-lang#113547 Fixes rust-lang#111016
Currently, we use a relatively 'small' span for THIR
expressions generated by an 'adjustment' (e.g. an autoderef,
autoborrow, unsizing). As a result, if a borrow generated
by an adustment ends up causing a borrowcheck error, for example:
then the span for the mutable borrow may end up referring
to only the base expression (e.g.
my_var
), rather thanthe method call which triggered the mutable borrow
(e.g.
my_var.push('a')
)Due to a quirk of the MIR borrowck implementation,
this doesn't always get exposed in migration mode,
but it does in many cases.
This commit makes THIR building consistently use 'larger'
spans for adjustment expressions. These spans are recoded
when we first create the adjustment during typecheck. For
example, an autoref adjustment triggered by a method call
will record the span of the entire method call.
The intent of this change it make it clearer to users
when it's the specific way in which a variable is
used (for example, in a method call) that produdes
a borrowcheck error. For example, an error message
claiming that a 'mutable borrow occurs here' might
be confusing if it just points at a usage of a variable
(e.g.
my_var
), when no&mut
is in sight. Pointingat the entire expression should help to emphasize
that the method call itself is responsible for
the mutable borrow.
In several cases, this makes the
#![feature(nll)]
diagnosticoutput match up exactly with the default (migration mode) output.
As a result, several
.nll.stderr
files end up getting removedentirely.