-
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
[WIP] remove Location::All
#50938
[WIP] remove Location::All
#50938
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
377430c
to
a904c55
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #51082) made this pull request unmergeable. Please resolve the merge conflicts. |
I found the above interesting. Do you think the way we might address that concern is perhaps related to finding a complete resolution to #47184 ? |
@@ -112,23 +108,19 @@ impl<'cx, 'tcx> SubtypeConstraintGenerator<'cx, 'tcx> { | |||
// reverse direction, because `regioncx` talks about | |||
// "outlives" (`>=`) whereas the region constraints | |||
// talk about `<=`. | |||
self.regioncx.add_outlives(span, b_vid, a_vid, at_location); | |||
self.regioncx.add_outlives(span, b_vid, a_vid); |
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.
Am I right in my understanding that one side-effect of no longer passing at_location
into add_outlives
is that our diagnostics stop being able to point to this location as the place where a particular constraint was introduced, which is what is leading to problems like the regression in diagnostic quality for e.g. a904c55#diff-40124ba66f06953794b406e3bb131b95L6 ?)
(And if that hypothesis is correct, is your thinking that we're going to have to solve that problem more generally anyway, perhaps as part of #51188, but that finding a solution to that problem should not block landing PR's like this one?)
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.
hey @nikomatsakis do you have an answer to my Q above ?
); | ||
if let Some(mir_yield_ty) = mir.yield_ty { | ||
let ur_yield_ty = universal_regions.yield_ty.unwrap(); | ||
self.equate_normalized_input_or_output(ur_yield_ty, mir_yield_ty); | ||
self.equate_normalized_input_or_output(Locations::Yield, ur_yield_ty, mir_yield_ty); |
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.
I think I need a comment here saying this is the place where effectively you are equating at all points. (or am I even right about that?)
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.
@nikomatsakis I still would like to see a comment in the source code about the effect of this change. (Either my hypothesis is right, which means its a subtle effect. Or my hypothesis is wrong, which means I need to figure out where the equate-at-all-points is happening for Yield...)
So this all seems acceptable to me. Its a shame about the regressions in diagnostic quality, but we have big problems to solve there on other test cases anyway, and thus I'm willing to assume we'll work on getting the quality back in these cases when we address those. |
There aren't that many universal regions. This was a premature optimization and it makes the code much messier.
a904c55
to
fd78a2f
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ping from triage @pnkfelix! The author pushed new commits. |
LL | &*x | ||
| ^^^ lifetime `ReStatic` required | ||
LL | fn foo(x: &u32) -> &'static u32 { | ||
| ^ - consider changing the type of `x` to `&ReStatic u32` |
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 sort of thing is the reason I'm not really thrilled with ui/
tests that use -Z verbose
... though I guess seeing ReStatic
in supposed user output is not as egregious as other potential leakage of internal details...)
LL | x | ||
| ^ lifetime `ReEarlyBound(0, 'a)` required | ||
LL | fn foo<'a, T>(x: &T) -> impl Foo<'a> { | ||
| ^ - consider changing the type of `x` to `&ReEarlyBound(0, 'a) T` |
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.
Like a similar comment elsewhere: this sort of thing is the reason I'm not really thrilled with ui/
tests that use -Z verbose
I recognize its a pre-existing issue with the test, but I think it would clean things up if you took the -Z verbose
out the test file. (Unless you think the output is serving a useful purpose here? Provides hint to compiler devs about the internal region structure in play?)
This all still seems fine. I had some questions about the code. and of course travis is faillng (I think due to a panic from But mostly I think the PR is still in @nikomatsakis 's court. Its still marked as WIP. |
Ping from triage @nikomatsakis! It's been a while since we heard from you on this, will you have time to work on this again? |
☔ The latest upstream changes (presumably #51460) made this pull request unmergeable. Please resolve the merge conflicts. |
I will get back to it, but right now it's relatively low priority as this is more concerned with polonius than anything else I guess. |
Ping from triage @nikomatsakis! Are you ok with closing this until you have time for it? |
I'm going to close this PR until I have time to get back to it 😿 |
When adding the facts for the new analysis, I added this idea of a
Locations::All
which enforces an outlives relation at all points in the CFG. This was kind of a hack to deal with a problem in the return slot_0
. What would happen is this. We would unify the type of_0
with the return type from the signature at the entry point of the function. Then later, we would write to_0
:This means that
_0
— which is otherwise treated like a regular local variable — would be dead at the entry point. In the existing analysis, in part because of its imprecision, this still works out ok, but in the new analysis it did not: in effect, the type of_0
was severed from the signature. So the hack I added basically re-equates the type of_0
(and all parameters) with the signature at all points in the CFG. But this winds up adding a ton of redundant outlives relations (hence rust-lang/polonius#24).So now I am taking a different approach. I equate the return type with the type from the signature only at the return location. This has the same soundness effect but without a hacky notion like
Location::All
. In the case of yields, I equate at all points. The argument types are equated on function entry.I think this is the right general direction; we may want to think about the use of liveness in general, as it does mean that something like this will type-check:
This might seem surprising, since the type of
x
is declared as&'static u32
, but&p
cannot live for'static
lifetime. But it works becausex
is dead at the pointx = &p
and hence it effectively gets a "fresh set of regions" from its initial value.I think we can address this concern in other ways, though, and not via the
Locations::All
hack (e.g., perhaps we should not use liveness to decide when regions in local variable types go dead, but rather going out of scope?). But anyway I'd rather we focus on that question in a separate PR.I'm marking this PR as WIP, because IIRC it still had a few problems in travis, but I wanted to get it posted for feedback purposes.
r? @pnkfelix
cc @rust-lang/wg-compiler-nll