Skip to content
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

lifetime order breaks member constraints #104639

Closed
aliemjay opened this issue Nov 20, 2022 · 2 comments · Fixed by #104765 or #105300
Closed

lifetime order breaks member constraints #104639

aliemjay opened this issue Nov 20, 2022 · 2 comments · Fixed by #104765 or #105300
Assignees
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@aliemjay
Copy link
Member

aliemjay commented Nov 20, 2022

The following code should pass regardless of the declaration order of lifetimes: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=88e227f9f7a5b516bffe21e09b18cd5e

async fn fail<'a, 'b, 'c>(_: &'static str) where 'a: 'c, 'b: 'c, {}
async fn pass<'a, 'c, 'b>(_: &'static str) where 'a: 'c, 'b: 'c, {}

This is related to #63033 but it's different in that we do have a lower bound region, 'c, but we fail to recognize that because the algorithm here, which is responsible for calculating min_choice from choice_regions = ['static, 'a, 'b, 'c], assumes that the outlive relation is a total order, which is not true.

Here is the graph of self.universal_region_relations: lXMdKPzIUUNTRnjy

So we need an efficient algorithm that works for partial order relations, but the one that comes to mind is O(n^2). The perf may not be really important here but I'm curious to see the alternatives.

@rustbot label C-bug T-compiler A-borrow-checker E-mentor E-help-wanted

@rustbot rustbot added A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 20, 2022
@chenyukang
Copy link
Member

@rustbot claim

@aliemjay
Copy link
Member Author

Here is the code of concern

// Otherwise, we need to find the minimum remaining choice, if
// any, and take that.
debug!("choice_regions remaining are {:#?}", choice_regions);
let min = |r1: ty::RegionVid, r2: ty::RegionVid| -> Option<ty::RegionVid> {
let r1_outlives_r2 = self.universal_region_relations.outlives(r1, r2);
let r2_outlives_r1 = self.universal_region_relations.outlives(r2, r1);
match (r1_outlives_r2, r2_outlives_r1) {
(true, true) => Some(r1.min(r2)),
(true, false) => Some(r2),
(false, true) => Some(r1),
(false, false) => None,
}
};
let mut min_choice = choice_regions[0];
for &other_option in &choice_regions[1..] {
debug!(?min_choice, ?other_option,);
match min(min_choice, other_option) {
Some(m) => min_choice = m,
None => {
debug!(?min_choice, ?other_option, "incomparable; no min choice",);
return false;
}
}
}

The code should calculate the min_choice from a set of universal regions (choice_regions). The relations are encoded in self.universal_region_relations and are guaranteed to be transitive and reflexive. The relation graph may have cycles and can be sparse.

The goal here is to find a unique minimum choice that is known to be outlived by all other regions. If none can be found, the function returns false.

Regions that have cycles between them are considered equivalent so we can pick either one of them as a unique choice. e.g. async fn<'a, 'b> f(_: &'static str) where 'a: 'b, 'b: 'a, {}, we can pick either one of ['a, 'b] as a minimum choice.

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 15, 2022
…e-check, r=oli-obk

Find the right lower bound region in the scenario of partial order relations

Fixes rust-lang#104639
@bors bors closed this as completed in b0d39c6 Dec 15, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
…e-check, r=oli-obk

Find the right lower bound region in the scenario of partial order relations

Fixes rust-lang#104639
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 14, 2023
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
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 15, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
3 participants