make member constraints pick static if no upper bounds by nikomatsakis · Pull Request #89056 · rust-lang/rust

nikomatsakis · 2021-09-17T22:01:38Z

The current member constraint algorithm has a failure mode when it encounters a
variable '0 and the only constraint is that ':static: '0. In that case,
there are no upper bounds, or at least no non-trivial upper bounds. As a
result, we are not able to rule out any of the choices, so if you have a
constraint like '0 member ['a, 'b, 'static], where 'a and 'b are
unrelated, then the algorithm gets stuck as there is no 'least choice' from
that set.

The tweak in this commit changes the algorithm so that if there are no upper
bounds (and hence '0 can get as large as desired without creating a region
check error), it will just pick 'static. This should only occur in cases
where the data is flowing out from a 'static value.

This change is probably not right for impl Trait in let bindings, but those
are challenging with member constraints anyway, and not currently supported.
Furthermore, this change is not needed in a polonius-like formulation, which
effectively permits "ad-hoc intersections" of lifetimes as the value for a
region, and hence could give a value like 'a ^ 'b as the resulting lifetime.
Therefore I think there isn't forwards compat danger here. (famous last words?)

r? @lqd
cc @tmandry @eholk @jackh726 @pnkfelix

I was thinking it might be nice to schedule a recorded zoom session to talk over this PR and how this bit of the code works, as I imagine it has a ... low bus factor. =)

Fixes #63033

The current member constraint algorithm has a failure mode when it encounters a variable `'0` and the only constraint is that `':static: '0`. In that case, there are no upper bounds, or at least no non-trivial upper bounds. As a result, we are not able to rule out any of the choices, so if you have a constraint like `'0 member ['a, 'b, 'static]`, where `'a` and `'b` are unrelated, then the algorithm gets stuck as there is no 'least choice' from that set. The tweak in this commit changes the algorithm so that *if* there are no upper bounds (and hence `'0` can get as large as desired without creating a region check error), it will just pick `'static`. This should only occur in cases where the data is flowing out from a `'static` value. This change is probably *not* right for impl Trait in let bindings, but those are challenging with member constraints anyway, and not currently supported. Furthermore, this change is not needed in a polonius-like formulation, which effectively permits "ad-hoc intersections" of lifetimes as the value for a region, and hence could give a value like `'a ^ 'b` as the resulting lifetime. Therefore I think there isn't forwards compat danger here. (famous last words?)

danielhenrymantilla · 2021-09-17T23:41:19Z

+
+        // If there are no upper bounds, and static is a choice (in practice, it always is),
+        // then we should just pick static.
+        if member_upper_bounds.is_empty()


I notice the check here is simpler than the one with any_upper_bounds done above (e.g., non-empty case with only 'statics inside): does collect_bounding_regions never yield 'statics?

Conjecture on my part: I wouldn't expect it to ? It wouldn't constrain the variable to add it as an upper bound, it's already the ⊤ element.

We'll see when we do the Zoom session ^^

Was the zoom session recorded somewhere btw?

It'll likely happen tomorrow

lqd · 2021-09-18T20:41:24Z

I was thinking it might be nice to schedule a recorded zoom session to talk over this PR and how this bit of the code works, as I imagine it has a ... low bus factor. =)

Sounds good. Even if the docs on member constraints are excellent, diving into the code and matching it to the prose will still be enlightening, I expect.

nikomatsakis · 2021-09-20T13:13:09Z

@bors try

Seems like a good idea to do a check crater run

bors · 2021-09-20T13:13:21Z

⌛ Trying commit b893cff with merge 8de4b308b608cded0ebccf658a0fc719bce87d4e...

bors · 2021-09-20T14:30:35Z

☀️ Try build successful - checks-actions
Build commit: 8de4b308b608cded0ebccf658a0fc719bce87d4e (8de4b308b608cded0ebccf658a0fc719bce87d4e)

lqd · 2021-09-20T17:49:50Z

Seems like a good idea to do a check crater run

Let's do that, the queue is empty !

@craterbot check

craterbot · 2021-09-20T17:49:56Z

👌 Experiment pr-89056 created and queued.
🤖 Automatically detected try build 8de4b308b608cded0ebccf658a0fc719bce87d4e
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2021-09-20T17:50:00Z

🚧 Experiment pr-89056 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2021-09-22T16:51:19Z

🎉 Experiment pr-89056 is completed!
📊 131 regressed and 7 fixed (184295 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

lqd · 2021-09-22T20:36:53Z

I've been looking at the results, and as mentioned on Zulip, here's the bb8-0.3.x regression minimized:

trait Future {}
impl Future for () {}

fn sink_error1<'a, F: 'a>(f: F) -> impl Future + 'a {
    sink_error2(f) // error: `F` may not live long enough
}
fn sink_error2<'a, F: 'a>(_: F) -> impl Future + 'a {}

danielhenrymantilla · 2021-09-23T09:48:56Z

+                    (true, true) => Some(r1.min(r2)),
+                    (true, false) => Some(r2),


Btw, unrelated to this PR, just out of curiosity / to make sure I understand this part correctly: would this second branch already cover the first? (at the cost of breaking the "symmetry" in the code)

nikomatsakis · 2021-09-23T21:41:36Z

@lqd that's an interesting regression. I guess the problem is that our region inference here is kind of "missing" some of the actual bounds. Let me think about that.

nikomatsakis · 2021-09-23T22:55:40Z

@bors try

Pushed a fix for that crater case. We can re-run crater, perhaps.

bors · 2021-09-23T22:55:49Z

⌛ Trying commit 92b2be2 with merge 11084977fccce286b348dbb5473938f22b430250...

jackh726 · 2021-09-23T22:59:39Z

We should be able to only rerun crater on the regressed crates. I don't know if we really need to do a full crater run. (In fact, if we do the subset, it might be done by the meeting tomorrow since the queue is empty)

@Mark-Simulacrum can you help here? I'm not entirely sure how to run with a subset.

Mark-Simulacrum · 2021-09-23T23:19:43Z

#88827 (comment) is an example of queueing the regressions from a prior crater run, you can probably duplicate that once the try is done here.

bors · 2021-09-24T00:18:35Z

☀️ Try build successful - checks-actions
Build commit: 11084977fccce286b348dbb5473938f22b430250 (11084977fccce286b348dbb5473938f22b430250)

jackh726 · 2021-09-24T02:49:22Z

@craterbot run name=pr-89056-2 start=db1fb85cff63ad5fffe435e17128f99f9e1d970c end=8de4b308b608cded0ebccf658a0fc719bce87d4e mode=build-and-test crates=https://crater-reports.s3.amazonaws.com/pr-89056/retry-regressed-list.txt

craterbot · 2021-09-24T02:49:25Z

👌 Experiment pr-89056-2 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2021-09-24T11:47:31Z

🎉 Experiment pr-89056-3 is completed!
📊 0 regressed and 55 fixed (131 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Mark-Simulacrum · 2021-09-24T22:33:27Z

@craterbot abort name=pr-89056-2

craterbot · 2021-09-24T22:33:28Z

🗑️ Experiment pr-89056-2 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

nikomatsakis · 2021-09-30T20:31:47Z

This PR is on hold while @oli-obk explores intersection regions.

oli-obk · 2022-06-22T10:49:28Z

I haven't gotten beyond code reading on the intersection regions. Let's unblock this, as afaict this is strictly forwards compatible with intersection regions

oli-obk · 2022-06-22T10:49:45Z

@rustbot review

lqd · 2022-07-14T21:06:04Z

This PR has been brought up during today's t-compiler meeting as part of the "oldest PRs waiting for review".

I don't think we've published the walkthrough on youtube we did back then, right ? I would have loved to rewatch it, and bring back some of that context in cache.

If we're going with this PR rather than intersection regions, then

it's going to need a rebase. That will also make sure this doesn't change diagnostics, as we expect.
a new crater run (a bunch of async code must have been published to crates.io in the meantime, so it will be good to check) but it will likely come back clean.
I believe @jackh726's concern at the time was resolved by Pick one possible lifetime in case there are multiple choices #89327
I remember the walkthrough making sense to me at the time within the context of member constraints, and this PR looking good, especially with the concern above having been resolved.

Here's a @rust-lang/types ping to notify interested parties who weren't involved when this PR was initially made, and a @rust-lang/wg-nll one for good measure.

With that: @rustbot author.

aliemjay · 2022-12-05T13:16:55Z

cc #105300

lcnr · 2023-01-18T15:19:07Z

unnominating in favor of an fcp in #105300

rework min_choice algorithm of member constraints See [this comment](rust-lang#105300 (comment)) for the description of the new algorithm. Fixes rust-lang#63033 Fixes rust-lang#104639 This uses a more general algorithm than rust-lang#89056 that doesn't treat `'static` as a special case. It thus accepts more code. For example: ```rust async fn test2<'s>(_: &'s u8, _: &'_ &'s u8, _: &'_ &'s u8) {} ``` I claim it's more correct as well because it fixes rust-lang#104639. cc `@nikomatsakis` `@lqd` `@tmandry` `@eholk` `@chenyukang` `@oli-obk` r? types

rework min_choice algorithm of member constraints See [this comment](rust-lang#105300 (comment)) for the description of the new algorithm. Fixes rust-lang#63033 Fixes rust-lang#104639 This uses a more general algorithm than rust-lang#89056 that doesn't treat `'static` as a special case. It thus accepts more code. For example: ```rust async fn test2<'s>(_: &'s u8, _: &'_ &'s u8, _: &'_ &'s u8) {} ``` I claim it's more correct as well because it fixes rust-lang#104639. cc ``@nikomatsakis`` ``@lqd`` ``@tmandry`` ``@eholk`` ``@chenyukang`` ``@oli-obk`` r? types

jackh726 · 2023-02-18T23:12:56Z

Closing since #105300 has landed and is a more general version of this.

rust-highfive assigned lqd Sep 17, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 17, 2021

nikomatsakis mentioned this pull request Sep 17, 2021

Async fn does not compile if lifetime does not appear in bounds (sometimes) #63033

Closed

danielhenrymantilla reviewed Sep 17, 2021

View reviewed changes

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 20, 2021

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Sep 22, 2021

danielhenrymantilla reviewed Sep 23, 2021

View reviewed changes

rework so that we pick static as a last resort only

92b2be2

craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Sep 24, 2021

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Sep 24, 2021

jackh726 mentioned this pull request Sep 28, 2021

Pick one possible lifetime in case there are multiple choices #89327

Merged

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021

jackh726 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2021

oli-obk removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jun 22, 2022

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2022

lqd mentioned this pull request Jul 14, 2022

async fn doesn't compile if it includes at least two generic lifetimes and at least one 'static lifetime #99190

Closed

lqd added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 14, 2022

jackh726 added the I-types-nominated Nominated for discussion during a types team meeting. label Nov 24, 2022

aliemjay mentioned this pull request Dec 5, 2022

rework min_choice algorithm of member constraints #105300

Merged

lcnr added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed I-types-nominated Nominated for discussion during a types team meeting. labels Jan 18, 2023

jackh726 closed this Feb 18, 2023

Uh oh!

Conversation

nikomatsakis commented Sep 17, 2021

Uh oh!

danielhenrymantilla Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

lqd Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

danielhenrymantilla Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

lqd Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

lqd commented Sep 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Sep 20, 2021

Uh oh!

bors commented Sep 20, 2021

Uh oh!

bors commented Sep 20, 2021

Uh oh!

lqd commented Sep 20, 2021

Uh oh!

craterbot commented Sep 20, 2021

Uh oh!

craterbot commented Sep 20, 2021

Uh oh!

craterbot commented Sep 22, 2021

Uh oh!

lqd commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielhenrymantilla Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Sep 23, 2021

Uh oh!

nikomatsakis commented Sep 23, 2021

Uh oh!

bors commented Sep 23, 2021

Uh oh!

jackh726 commented Sep 23, 2021

Uh oh!

Mark-Simulacrum commented Sep 23, 2021

Uh oh!

bors commented Sep 24, 2021

Uh oh!

jackh726 commented Sep 24, 2021

Uh oh!

craterbot commented Sep 24, 2021

Uh oh!

craterbot commented Sep 24, 2021

Uh oh!

Mark-Simulacrum commented Sep 24, 2021

Uh oh!

craterbot commented Sep 24, 2021

Uh oh!

nikomatsakis commented Sep 30, 2021

Uh oh!

oli-obk commented Jun 22, 2022

Uh oh!

oli-obk commented Jun 22, 2022

Uh oh!

lqd commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aliemjay commented Dec 5, 2022

Uh oh!

lcnr commented Jan 18, 2023

Uh oh!

jackh726 commented Feb 18, 2023

Uh oh!

Reviewers

Assignees

Labels

lqd commented Sep 18, 2021 •

edited

Loading

lqd commented Sep 22, 2021 •

edited

Loading

danielhenrymantilla Sep 23, 2021 •

edited

Loading

lqd commented Jul 14, 2022 •

edited

Loading