-
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
introduce "type must be valid for" into lexical region solver #55988
Conversation
@bors r+ |
📌 Commit 4b4e074fdc64cc1e189ae86a813f0c30cb8ab337 has been approved by |
cc @rust-lang/compiler -- I'm nominating this for beta backport, since it fixes a regression. |
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 |
@bors r-
|
@bors r=eddyb p=1 |
📌 Commit 134c7d1019623aaa96f92d574428303845474fd4 has been approved by |
Giving p=1 because this is a Rust 2018 blocker. |
@bors r- Huh. Apparently this broke some tests. |
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 |
OK, so, the errors are sort of interesting. These are test cases that also break with NLL (I guess that makes sense). They have to do with closures whose signatures contain invalid types -- currently, regionck doesn't give an error for this, but it seems wrong. An example is: || {
&mut x //~ ERROR cannot infer
}; Why, you ask, is this wrong? Well, the answer is that this is an I suspect we can get away with breaking these tests -- NLL will break them away, and they are seim-broken -- but it makes me a bit nervous. We could also try one of the alternative fixes I had in mind. (Hmm, I thought that the NLL breakage was noticed in the wild (#49824) but it turns out that this breakage was noticed by me from one of our other tests which was purposefully probing at extreme scenarios. That makes me feel better.) One test that used to give errors also now started to work, interestingly -- that test seems clearly "ok" to me (it's an |
On balance, given that the broken tests are basically invalid and the fixed test seems valid, I'm inclined to run with it. |
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 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 |
@bors r=eddyb |
📌 Commit 752feb7814fd98c383c344dccb6cd9c328fbcf5e has been approved by |
752feb7
to
29ec15a
Compare
@nikomatsakis with #56043 landed and backported is still still 2018 edition critical? (aka on the milestone) |
@alexcrichton it is not |
@bors p=0 |
@bors r=eddyb |
📌 Commit 29ec15a has been approved by |
@bors r- Actually, second guessing. I want to think about whether to land this now or let NLL fix these test cases. |
Ping from triage @nikomatsakis: What is the status of this PR? |
☔ The latest upstream changes (presumably #56502) made this pull request unmergeable. Please resolve the merge conflicts. |
@nikomatsakis I'd like us to consider landing this now. Minimizing the number of subtle discrepancies between AST-borrowck and NLL simplifies our internal processes for evaluating the correctness of NLL itself. |
@pnkfelix I would prefer landing this if it gets us closer to removing the AST borrowck (is there a full post-2018 transition plan spelled out anywhere?). |
ping from triage @nikomatsakis @pnkfelix @eddyb any updates on this? |
Sorry, I've been busy. I'm going to try and rebase this and/or close it this week though =) |
Ping from triage @nikomatsakis Have you been able to make any progress on this PR? |
Not yet but still hope to =)
|
Ping from triage, @nikomatsakis -- have you been able to make progress? r? @pnkfelix |
ping from triage @nikomatsakis closing this due to inactivity. Thanks |
This is a fix for #55756 -- it makes the lexical region solver impose constraints more like the ones that NLL does, which overcomes a shortcoming in the logic for handling
<T as Foo<'a>>::Bar: 'b
outlives bounds. As part of the NLL check, that logic itself was fixed to avoid "overconstraining" -- i.e., adding stricter region checks than were necessary. Unfortunately, the logic now has the potential to underconstrain (it always did, but it's worse). This is caused #55756. This change matches NLL's behavior, which winds up requiring that if a value of typeT
is live during some scope S, then every region inT
must outliveS
(which is stricter than outlives when normalization is involved).As part of the Chalk and Polonius work, I plan to revisit this whole system, and hopefully move to a more precise solver that can accommodate the kinds of outlives constraints we encounter in progress. However, this patch seems like a reasonably conservative step that also solves the regressions at hand.
More discussion in this comment.
Fixes #55756
r? @eddyb