-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
(re-)tighten sourceinfo span of adjustments in MIR #112945
(re-)tighten sourceinfo span of adjustments in MIR #112945
Conversation
This may affect miri diagnostic spans, too. r=me with CI clean (should run miri, right?) and the comment removed |
This comment has been minimized.
This comment has been minimized.
1796124
to
71698ee
Compare
The Miri subtree was changed cc @rust-lang/miri |
I'll keep this open for a few days in case someone wants to speak up. |
The Miri diagnostic changes seem rather troubling to me. The changes here are all diagnostics trying to point at the creation of the receiver as the problem. Now the span points only to the instance that the method is called on, where previously I think it was pretty clear that we were pointing at the method call. |
@saethlin: The receiver is the problem here, though, or more specifically whatever adjustments of the receiver are applied before the method call. I don't think the right span to highlight on "receiver is auto-ref'd before a call happens" is the whole call span. |
It's arguable whether or not the method call is the important part in explaining that an adjustment happens, and I'm kinda skeptical (hence this PR) that borrowck errors (or other errors derived from MIR spans, like miri diagnostics) should point there instead. That's presumably what #89110 was trying to restore from the pre-NLL borrowck, though. Ideally, we'd pass down richer information than just a single source span per MIR statement for reasons like this :/ |
Given that the issue occurs before the actual call, I to agree the old spans were not great - we definitely shouldn't be pointing at the whole call expression including arguments ("rec.method(arg)").
A point could be made that we should point at "rec.method" and not just at "rec". I don't have a strong preference either way.
|
☔ The latest upstream changes (presumably #113151) made this pull request unmergeable. Please resolve the merge conflicts. |
71698ee
to
a74db1a
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (da1d099): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 657.918s -> 657.774s (-0.02%) |
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
andshrink_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
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 #89110, which was debated somewhat in #90286. cc @Aaron1011 who originally made this change.
r? diagnostics
Fixes #113547
Fixes #111016