support revealing uses of opaques by lcnr · Pull Request #139587 · rust-lang/rust (original) (raw)
the following now works, needs a bit of cleanup to actually land :3
fn foo(x: &u32) -> impl Sized + '_ {
let _ = || {
let temp = 1;
// normalization of the return type results
// in opaque<'local1> = &'local2 u32.
foo(&temp);
};
x
}
final breakage?
fn proj<F: FnOnce() -> P, P: Iterator<Item = Q>, Q>(_: F) {} struct Lts<'a, 'b>(&'a (), &'b ()); fn expectations<'a>() -> impl Iterator<Item = Lts<'a, 'static>> + use<'a> { // opaque<'a> = Map<std::iter::Empty<Lts<'0, '1>>, {closure@src/main.rs:5:28: 5:31}>`` proj::<_, _, Lts<'a, 'static>>(|| expectations()); std::iter::empty().map(|x| x) }
struct Foo<F, T>(T, F); trait Trait { type Assoc; } impl<F: FnOnce(T) -> R, T, R> Trait for Foo<F, T> { type Assoc = R; }
fn new_defining_use<F: FnOnce() -> R, R>(f: F) {} fn rpit<'a>() -> impl Trait<Assoc = &'static ()> + use<'a> { new_defining_use(rpit::<'a>); Foo(&(), |x: &'static ()| x) }
use std::convert::identity; struct Foo<F, T>(T, F); trait Trait { type Assoc; } impl<F: FnOnce(T) -> R, T, R> Trait for Foo<F, T> { type Assoc = R; }
fn new_defining_use<F: FnOnce() -> R, R>(: F) {} fn rpit<'a>() -> impl Trait<Assoc = &'static ()> + use<'a> { new_defining_use(rpit::<'a>); Foo(&(), identity as fn() -> _) }
struct Inv<'a, 'b>(*mut (&'a (), &'b ())); fn new_defining_use<F: FnOnce() -> R, R>(_: F) {} fn rpit<'a>() -> impl Sized + use<'a> { new_defining_use(rpit::<'a>); Inv::<'a, 'static>(std::ptr::null_mut()) }
fn new_defining_use<F: FnOnce(T) -> R, T, R>(_: F) {} fn rpit<'a, 'b: 'b>(x: &'b ()) -> impl Sized + use<'a, 'b> { new_defining_use(rpit::<'a, 'b>); x }
rustbot added S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
This comment has been minimized.
lcnr mentioned this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
support revealing uses of opaques in closures
based on rust-lang#139484
the following now works, needs a bit of cleanup to actually land :3
fn foo(x: &u32) -> impl Sized + '_ {
let _ = || {
let temp = 1;
// normalization of the return type results
// in `opaque<'local1> = &'local2 u32`.
foo(&temp);
};
x
}r? @compiler-errors
This comment has been minimized.
☀️ Try build successful - checks-actions
Build commit: 14ea441 (14ea4416942169f85851de68119f43a59ef8801f)
This comment has been minimized.
Finished benchmarking commit (14ea441): comparison URL.
Overall result: ❌✅ regressions and improvements - please read the text below
Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.
Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.
@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression
Instruction count
This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | 0.6% | [0.2%, 1.1%] | 62 |
| Regressions ❌ (secondary) | 0.5% | [0.3%, 0.8%] | 16 |
| Improvements ✅ (primary) | -0.3% | [-0.3%, -0.2%] | 2 |
| Improvements ✅ (secondary) | -0.5% | [-1.1%, -0.2%] | 27 |
| All ❌✅ (primary) | 0.5% | [-0.3%, 1.1%] | 64 |
Max RSS (memory usage)
Results (primary 2.0%, secondary -1.3%)
This 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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | 2.0% | [1.2%, 2.8%] | 5 |
| Regressions ❌ (secondary) | 3.1% | [2.5%, 3.6%] | 2 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | -3.0% | [-7.2%, -1.2%] | 5 |
| All ❌✅ (primary) | 2.0% | [1.2%, 2.8%] | 5 |
Cycles
Results (primary 1.1%, secondary 1.5%)
This 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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | 1.1% | [0.8%, 1.3%] | 12 |
| Regressions ❌ (secondary) | 1.5% | [1.2%, 1.9%] | 10 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | 1.1% | [0.8%, 1.3%] | 12 |
Binary size
Results (secondary -0.1%)
This 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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | -0.1% | [-0.1%, -0.1%] | 6 |
| All ❌✅ (primary) | - | - | 0 |
Bootstrap: 780.245s -> 782.612s (0.30%)
Artifact size: 366.19 MiB -> 366.30 MiB (0.03%)
bors added a commit to rust-lang-ci/rust that referenced this pull request
lcnr mentioned this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
cleanup mir_borrowck
Cleanup pulled out of rust-lang#139587. Best reviewed commit by commit.
r? compiler-errors
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
lcnr mentioned this pull request
bors added a commit that referenced this pull request
apply_member_constraints: fix placeholder check
Checking whether the member region is an existential region from a higher universe is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before #140466.
I've encountered this issue separately while working on #139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc #140569/#142073. However, as we always skipped these member regions in apply_member_constraints the skipped region is guaranteed to cause an error in check_member_constraints later on.
bors added a commit that referenced this pull request
apply_member_constraints: fix placeholder check
Checking whether the member region is an existential region from a higher universe is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before #140466.
I've encountered this issue separately while working on #139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc #140569/#142073. However, as we always skipped these member regions in apply_member_constraints the skipped region is guaranteed to cause an error in check_member_constraints later on.
rust-cloud-vms bot pushed a commit to makai410/rustc_public that referenced this pull request
rust-cloud-vms bot pushed a commit to makai410/rustc_public that referenced this pull request
apply_member_constraints: fix placeholder check
Checking whether the member region is an existential region from a higher universe is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before rust-lang/rust#140466.
I've encountered this issue separately while working on rust-lang/rust#139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc rust-lang/rust#140569/rust-lang/rust#142073. However, as we always skipped these member regions in apply_member_constraints the skipped region is guaranteed to cause an error in check_member_constraints later on.
github-actions bot pushed a commit to rust-lang/stdarch that referenced this pull request
move type_check out of compute_regions
A step towards rust-lang/rust#139587. I don't think there's a clear reason for why MIR type check should be in compute_regions and this simplifies future PRs here.
github-actions bot pushed a commit to rust-lang/stdarch that referenced this pull request
apply_member_constraints: fix placeholder check
Checking whether the member region is an existential region from a higher universe is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before rust-lang/rust#140466.
I've encountered this issue separately while working on rust-lang/rust#139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc rust-lang/rust#140569/rust-lang/rust#142073. However, as we always skipped these member regions in apply_member_constraints the skipped region is guaranteed to cause an error in check_member_constraints later on.
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request
apply_member_constraints: fix placeholder check
Checking whether the member region is an existential region from a higher universe is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before rust-lang/rust#140466.
I've encountered this issue separately while working on rust-lang/rust#139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc rust-lang/rust#140569/rust-lang/rust#142073. However, as we always skipped these member regions in apply_member_constraints the skipped region is guaranteed to cause an error in check_member_constraints later on.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request
apply_member_constraints: fix placeholder check
Checking whether the member region is an existential region from a higher universe is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before rust-lang/rust#140466.
I've encountered this issue separately while working on rust-lang/rust#139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc rust-lang/rust#140569/rust-lang/rust#142073. However, as we always skipped these member regions in apply_member_constraints the skipped region is guaranteed to cause an error in check_member_constraints later on.
bors added a commit that referenced this pull request
…nce, r=BoxyUwU
support non-defining uses of opaques in borrowck
Reimplements the first part of #139587, but limited to only the new solver. To do so I also entirely rewrite the way we handle opaque types in borrowck even on stable. This should not impact behavior however.
We now support revealing uses during MIR borrowck with the new solver:
fn foo<'a>(x: &'a u32) -> impl Sized + use<'a> {
let local = 1;
foo::<'_>(&local);
x
}How do opaque types work right now
Whenever we use an opaque type during type checking, we remember this use in the opaque_type_storage of the InferCtxt.
Right now, we collect all member constraints at the end of MIR type check by looking at all uses from the opaque_type_storage. We then apply these constraints while computing the region values for each SCC. This does not add actual region constraints but directly updates the final region values.
This means we need to manually handle any constraints from member constraints for diagnostics. We do this by separately tracking applied_member_constraints in the RegionInferenceContext.
After we've finished computing the region values, it is now immutable and we check whether all member constraints hold. If not, we error.
We now map the hidden types of our defining uses to the defining scope. This assumes that all member constraints apply. To handle non-member regions, we simply map any region in the hidden type we fail to map to a choice region to 'erased https://github.com/rust-lang/rust/blob/b1b26b834d85e84b46aa8f8f3ce210a1627aa85f/compiler/rustc_borrowck/src/region_infer/opaque_types.rs#L126-L132
How do we handle opaque types with this PR
MIR type check still works the same by populating the opaque_type_storage whenever we use an opaque type.
We now have a new step fn handle_opaque_type_uses which happens between MIR type check and compute_regions.
This step looks at all opaque type uses in the storage and first checks whether they are defining: are the arguments of the opaque_type_key unique region parameters. With the new solver we silently ignore any non-defining uses here. The old solver emits an errors.
fn compute_concrete_opaque_types: We then collect all member constraints for the defining uses and apply them just like we did before. However, we do it on a temporary region graph which is only used while computing the concrete opaque types. We then use this region graph to compute the concrete type which we then store in the root_cx.
fn apply_computed_concrete_opaque_types: Now that we know the final concrete type of each opaque type and have mapped them to the definition of the opaque. We iterate over all opaque type uses and equate their hidden type with the instantiated final concrete type. This is the step which actually mutates the region graph.
The actual region checking can now entirely ignores opaque types (outside of the ConstraintCategory from checking the opaque type uses).
Diagnostics issue (chill)
Because we now simply use type equality to "apply member constraints" we get ordinary OutlivesConstraints, even if the regions were already related to another.
This is generally not an issue, expect that it can hide the actual region constraints which resulted in the final value of the opaque. The constraints we get from checking against the final opaque type definition relies on constraints we used to compute that definition.
I mostly handle this by updating find_constraint_path_between_regions to first ignore member constraints in its search and only if that does not find a path, retry while considering member constraints.
Diagnostics issue (not chill)
A separate issue is that find_constraint_paths_between_regions currently looks up member constraints by their scc, not by region value:
https://github.com/rust-lang/rust/blob/2c1ac85679678dfe5cce7ea8037735b0349ceaf3/compiler/rustc_borrowck/src/region_infer/mod.rs#L1768-L1775
This means that in the borrowck-4 test, the resulting constraint path is currently
('?2: '?5) due to Single(bb0[5]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?5: '?3) due to Single(bb0[6]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?3: '?0) due to All(src/main.rs:15:5: 15:6 (#0)) (None) (OpaqueType) (ConstraintSccIndex(1): ConstraintSccIndex(1))Here '?3 is equal to '?4, but the reason why it's in the opaque is that it's related to '?4. With my PR this will be correctly tracked so we end up with
('?2: '?5) due to Single(bb0[5]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?5: '?3) due to Single(bb0[6]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?3: '?4) due to Single(bb0[6]) (None) (Assignment) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?4: '?0) due to All(src/main.rs:15:5: 15:6 (#0)) (None) (OpaqueType) (ConstraintSccIndex(1): ConstraintSccIndex(1)),This additional Assignment step then worsens the error message as we stop talking about the fact that the closures is returned from the function. Fixing this is hard. I've looked into this and it's making me sad :< Properly handling this requires some deeper changes to MIR borrowck diagnostics and that seems like too much for this PR. Given that this only impacts a single test, it seems acceptable to me.
r? @ghost
rust-cloud-vms bot pushed a commit to makai410/rustc_public that referenced this pull request
rust-cloud-vms bot pushed a commit to makai410/rustc_public that referenced this pull request
apply_member_constraints: fix placeholder check
Checking whether the member region is an existential region from a higher universe is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before rust-lang/rust#140466.
I've encountered this issue separately while working on rust-lang/rust#139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc rust-lang/rust#140569/rust-lang/rust#142073. However, as we always skipped these member regions in apply_member_constraints the skipped region is guaranteed to cause an error in check_member_constraints later on.
bors added a commit that referenced this pull request
…nce, r=BoxyUwU
support non-defining uses of opaques in borrowck
Reimplements the first part of #139587, but limited to only the new solver. To do so I also entirely rewrite the way we handle opaque types in borrowck even on stable. This should not impact behavior however.
We now support revealing uses during MIR borrowck with the new solver:
fn foo<'a>(x: &'a u32) -> impl Sized + use<'a> {
let local = 1;
foo::<'_>(&local);
x
}How do opaque types work right now
Whenever we use an opaque type during type checking, we remember this use in the opaque_type_storage of the InferCtxt.
Right now, we collect all member constraints at the end of MIR type check by looking at all uses from the opaque_type_storage. We then apply these constraints while computing the region values for each SCC. This does not add actual region constraints but directly updates the final region values.
This means we need to manually handle any constraints from member constraints for diagnostics. We do this by separately tracking applied_member_constraints in the RegionInferenceContext.
After we've finished computing the region values, it is now immutable and we check whether all member constraints hold. If not, we error.
We now map the hidden types of our defining uses to the defining scope. This assumes that all member constraints apply. To handle non-member regions, we simply map any region in the hidden type we fail to map to a choice region to 'erased https://github.com/rust-lang/rust/blob/b1b26b834d85e84b46aa8f8f3ce210a1627aa85f/compiler/rustc_borrowck/src/region_infer/opaque_types.rs#L126-L132
How do we handle opaque types with this PR
MIR type check still works the same by populating the opaque_type_storage whenever we use an opaque type.
We now have a new step fn handle_opaque_type_uses which happens between MIR type check and compute_regions.
This step looks at all opaque type uses in the storage and first checks whether they are defining: are the arguments of the opaque_type_key unique region parameters. With the new solver we silently ignore any non-defining uses here. The old solver emits an errors.
fn compute_concrete_opaque_types: We then collect all member constraints for the defining uses and apply them just like we did before. However, we do it on a temporary region graph which is only used while computing the concrete opaque types. We then use this region graph to compute the concrete type which we then store in the root_cx.
fn apply_computed_concrete_opaque_types: Now that we know the final concrete type of each opaque type and have mapped them to the definition of the opaque. We iterate over all opaque type uses and equate their hidden type with the instantiated final concrete type. This is the step which actually mutates the region graph.
The actual region checking can now entirely ignores opaque types (outside of the ConstraintCategory from checking the opaque type uses).
Diagnostics issue (chill)
Because we now simply use type equality to "apply member constraints" we get ordinary OutlivesConstraints, even if the regions were already related to another.
This is generally not an issue, expect that it can hide the actual region constraints which resulted in the final value of the opaque. The constraints we get from checking against the final opaque type definition relies on constraints we used to compute that definition.
I mostly handle this by updating find_constraint_path_between_regions to first ignore member constraints in its search and only if that does not find a path, retry while considering member constraints.
Diagnostics issue (not chill)
A separate issue is that find_constraint_paths_between_regions currently looks up member constraints by their scc, not by region value:
https://github.com/rust-lang/rust/blob/2c1ac85679678dfe5cce7ea8037735b0349ceaf3/compiler/rustc_borrowck/src/region_infer/mod.rs#L1768-L1775
This means that in the borrowck-4 test, the resulting constraint path is currently
('?2: '?5) due to Single(bb0[5]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?5: '?3) due to Single(bb0[6]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?3: '?0) due to All(src/main.rs:15:5: 15:6 (#0)) (None) (OpaqueType) (ConstraintSccIndex(1): ConstraintSccIndex(1))Here '?3 is equal to '?4, but the reason why it's in the opaque is that it's related to '?4. With my PR this will be correctly tracked so we end up with
('?2: '?5) due to Single(bb0[5]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?5: '?3) due to Single(bb0[6]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?3: '?4) due to Single(bb0[6]) (None) (Assignment) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?4: '?0) due to All(src/main.rs:15:5: 15:6 (#0)) (None) (OpaqueType) (ConstraintSccIndex(1): ConstraintSccIndex(1)),This additional Assignment step then worsens the error message as we stop talking about the fact that the closures is returned from the function. Fixing this is hard. I've looked into this and it's making me sad :< Properly handling this requires some deeper changes to MIR borrowck diagnostics and that seems like too much for this PR. Given that this only impacts a single test, it seems acceptable to me.
r? @ghost
bors added a commit that referenced this pull request
…nce, r=BoxyUwU
support non-defining uses of opaques in borrowck
Reimplements the first part of #139587, but limited to only the new solver. To do so I also entirely rewrite the way we handle opaque types in borrowck even on stable. This should not impact behavior however.
We now support revealing uses during MIR borrowck with the new solver:
fn foo<'a>(x: &'a u32) -> impl Sized + use<'a> {
let local = 1;
foo::<'_>(&local);
x
}How do opaque types work right now
Whenever we use an opaque type during type checking, we remember this use in the opaque_type_storage of the InferCtxt.
Right now, we collect all member constraints at the end of MIR type check by looking at all uses from the opaque_type_storage. We then apply these constraints while computing the region values for each SCC. This does not add actual region constraints but directly updates the final region values.
This means we need to manually handle any constraints from member constraints for diagnostics. We do this by separately tracking applied_member_constraints in the RegionInferenceContext.
After we've finished computing the region values, it is now immutable and we check whether all member constraints hold. If not, we error.
We now map the hidden types of our defining uses to the defining scope. This assumes that all member constraints apply. To handle non-member regions, we simply map any region in the hidden type we fail to map to a choice region to 'erased https://github.com/rust-lang/rust/blob/b1b26b834d85e84b46aa8f8f3ce210a1627aa85f/compiler/rustc_borrowck/src/region_infer/opaque_types.rs#L126-L132
How do we handle opaque types with this PR
MIR type check still works the same by populating the opaque_type_storage whenever we use an opaque type.
We now have a new step fn handle_opaque_type_uses which happens between MIR type check and compute_regions.
This step looks at all opaque type uses in the storage and first checks whether they are defining: are the arguments of the opaque_type_key unique region parameters. With the new solver we silently ignore any non-defining uses here. The old solver emits an errors.
fn compute_concrete_opaque_types: We then collect all member constraints for the defining uses and apply them just like we did before. However, we do it on a temporary region graph which is only used while computing the concrete opaque types. We then use this region graph to compute the concrete type which we then store in the root_cx.
fn apply_computed_concrete_opaque_types: Now that we know the final concrete type of each opaque type and have mapped them to the definition of the opaque. We iterate over all opaque type uses and equate their hidden type with the instantiated final concrete type. This is the step which actually mutates the region graph.
The actual region checking can now entirely ignores opaque types (outside of the ConstraintCategory from checking the opaque type uses).
Diagnostics issue (chill)
Because we now simply use type equality to "apply member constraints" we get ordinary OutlivesConstraints, even if the regions were already related to another.
This is generally not an issue, expect that it can hide the actual region constraints which resulted in the final value of the opaque. The constraints we get from checking against the final opaque type definition relies on constraints we used to compute that definition.
I mostly handle this by updating find_constraint_path_between_regions to first ignore member constraints in its search and only if that does not find a path, retry while considering member constraints.
Diagnostics issue (not chill)
A separate issue is that find_constraint_paths_between_regions currently looks up member constraints by their scc, not by region value:
https://github.com/rust-lang/rust/blob/2c1ac85679678dfe5cce7ea8037735b0349ceaf3/compiler/rustc_borrowck/src/region_infer/mod.rs#L1768-L1775
This means that in the borrowck-4 test, the resulting constraint path is currently
('?2: '?5) due to Single(bb0[5]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?5: '?3) due to Single(bb0[6]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?3: '?0) due to All(src/main.rs:15:5: 15:6 (#0)) (None) (OpaqueType) (ConstraintSccIndex(1): ConstraintSccIndex(1))Here '?3 is equal to '?4, but the reason why it's in the opaque is that it's related to '?4. With my PR this will be correctly tracked so we end up with
('?2: '?5) due to Single(bb0[5]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?5: '?3) due to Single(bb0[6]) (None) (Boring) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?3: '?4) due to Single(bb0[6]) (None) (Assignment) (ConstraintSccIndex(1): ConstraintSccIndex(1)),
('?4: '?0) due to All(src/main.rs:15:5: 15:6 (#0)) (None) (OpaqueType) (ConstraintSccIndex(1): ConstraintSccIndex(1)),This additional Assignment step then worsens the error message as we stop talking about the fact that the closures is returned from the function. Fixing this is hard. I've looked into this and it's making me sad :< Properly handling this requires some deeper changes to MIR borrowck diagnostics and that seems like too much for this PR. Given that this only impacts a single test, it seems acceptable to me.
r? @ghost
lcnr mentioned this pull request
bors added a commit that referenced this pull request
-Znext-solver: support non-defining uses in closures
Cleaned up version of #139587, finishing the implementation of rust-lang/types-team#129. This does not affect stable. The reasoning for why this is the case is subtle however.
What does it do
We split do_mir_borrowck into borrowck_collect_region_constraints and borrowck_check_region_constraints, where borrowck_collect_region_constraints returns an enormous CollectRegionConstraintsResult struct which contains all the relevant data to actually handle opaque type uses and to check the region constraints later on.
query mir_borrowck now simply calls BorrowCheckRootCtxt::do_mir_borrowck which starts by iterating over all nested bodies of the current function - visiting nested bodies before their parents - and computing their CollectRegionConstraintsResult.
After we've collected all constraints it's time to actually compute the concrete types for the opaques defined by this function. With this PR we now compute the concrete types of opaques for each body before using them to check the non-defining uses of any of them.
After we've computed the concrete types by using all bodies, we use apply_computed_concrete_opaque_types for each body to constrain non-defining uses, before finally finishing with borrowck_check_region_constraints. We always visit nested bodies before their parents when doing this.
ClosureRegionRequirements
As we only call borrowck_collect_region_constraints for nested bodies before type checking the parent, we can't simply use the final ClosureRegionRequirements of the nested body during MIR type check. We instead track that we need to apply these requirements in deferred_closure_requirements.
We now manually apply the final closure requirements to each body after handling opaque types.
This works, except that we may need the region constraints of nested bodies to successfully define an opaque type in the parent. This is handled by using a new fn compute_closure_requirements_modulo_opaques which duplicates region checking - while ignoring any errors - before we've added the constraints from apply_computed_concrete_opaque_types. This is necessary for a lot of async tests, as pretty much the entire function is inside of an async block while the opaque type gets defined in the parent.
As an performance optimization we only use fn compute_closure_requirements_modulo_opaques in case the nested body actually depends on any opaque types. Otherwise we eagerly call borrowck_check_region_constraints and apply the final closure region requirements right away.
Impact on stable code
Handling the opaque type uses in the parent function now only uses the closure requirements modulo opaques, while it previously also considered member constraints from nested bodies. External regions are never valid choice regions. Also, member constraints will never constrain a member region if it is required to be outlived by an external region, as that fails the upper-bound check. https://github.com/rust-lang/rust/blob/564ee219127b796d56f74767366fd359758b97de/compiler/rustc_borrowck/src/region_infer/opaque_types/member_constraints.rs#L90-L96
Member constraints therefore never add constraints for external regions :>
r? @BoxyUwU
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request
-Znext-solver: support non-defining uses in closures
Cleaned up version of rust-lang/rust#139587, finishing the implementation of rust-lang/types-team#129. This does not affect stable. The reasoning for why this is the case is subtle however.
What does it do
We split do_mir_borrowck into borrowck_collect_region_constraints and borrowck_check_region_constraints, where borrowck_collect_region_constraints returns an enormous CollectRegionConstraintsResult struct which contains all the relevant data to actually handle opaque type uses and to check the region constraints later on.
query mir_borrowck now simply calls BorrowCheckRootCtxt::do_mir_borrowck which starts by iterating over all nested bodies of the current function - visiting nested bodies before their parents - and computing their CollectRegionConstraintsResult.
After we've collected all constraints it's time to actually compute the concrete types for the opaques defined by this function. With this PR we now compute the concrete types of opaques for each body before using them to check the non-defining uses of any of them.
After we've computed the concrete types by using all bodies, we use apply_computed_concrete_opaque_types for each body to constrain non-defining uses, before finally finishing with borrowck_check_region_constraints. We always visit nested bodies before their parents when doing this.
ClosureRegionRequirements
As we only call borrowck_collect_region_constraints for nested bodies before type checking the parent, we can't simply use the final ClosureRegionRequirements of the nested body during MIR type check. We instead track that we need to apply these requirements in deferred_closure_requirements.
We now manually apply the final closure requirements to each body after handling opaque types.
This works, except that we may need the region constraints of nested bodies to successfully define an opaque type in the parent. This is handled by using a new fn compute_closure_requirements_modulo_opaques which duplicates region checking - while ignoring any errors - before we've added the constraints from apply_computed_concrete_opaque_types. This is necessary for a lot of async tests, as pretty much the entire function is inside of an async block while the opaque type gets defined in the parent.
As an performance optimization we only use fn compute_closure_requirements_modulo_opaques in case the nested body actually depends on any opaque types. Otherwise we eagerly call borrowck_check_region_constraints and apply the final closure region requirements right away.
Impact on stable code
Handling the opaque type uses in the parent function now only uses the closure requirements modulo opaques, while it previously also considered member constraints from nested bodies. External regions are never valid choice regions. Also, member constraints will never constrain a member region if it is required to be outlived by an external region, as that fails the upper-bound check. https://github.com/rust-lang/rust/blob/564ee219127b796d56f74767366fd359758b97de/compiler/rustc_borrowck/src/region_infer/opaque_types/member_constraints.rs#L90-L96
Member constraints therefore never add constraints for external regions :>
r? @BoxyUwU
github-actions bot pushed a commit to rust-lang/compiler-builtins that referenced this pull request
-Znext-solver: support non-defining uses in closures
Cleaned up version of rust-lang/rust#139587, finishing the implementation of rust-lang/types-team#129. This does not affect stable. The reasoning for why this is the case is subtle however.
What does it do
We split do_mir_borrowck into borrowck_collect_region_constraints and borrowck_check_region_constraints, where borrowck_collect_region_constraints returns an enormous CollectRegionConstraintsResult struct which contains all the relevant data to actually handle opaque type uses and to check the region constraints later on.
query mir_borrowck now simply calls BorrowCheckRootCtxt::do_mir_borrowck which starts by iterating over all nested bodies of the current function - visiting nested bodies before their parents - and computing their CollectRegionConstraintsResult.
After we've collected all constraints it's time to actually compute the concrete types for the opaques defined by this function. With this PR we now compute the concrete types of opaques for each body before using them to check the non-defining uses of any of them.
After we've computed the concrete types by using all bodies, we use apply_computed_concrete_opaque_types for each body to constrain non-defining uses, before finally finishing with borrowck_check_region_constraints. We always visit nested bodies before their parents when doing this.
ClosureRegionRequirements
As we only call borrowck_collect_region_constraints for nested bodies before type checking the parent, we can't simply use the final ClosureRegionRequirements of the nested body during MIR type check. We instead track that we need to apply these requirements in deferred_closure_requirements.
We now manually apply the final closure requirements to each body after handling opaque types.
This works, except that we may need the region constraints of nested bodies to successfully define an opaque type in the parent. This is handled by using a new fn compute_closure_requirements_modulo_opaques which duplicates region checking - while ignoring any errors - before we've added the constraints from apply_computed_concrete_opaque_types. This is necessary for a lot of async tests, as pretty much the entire function is inside of an async block while the opaque type gets defined in the parent.
As an performance optimization we only use fn compute_closure_requirements_modulo_opaques in case the nested body actually depends on any opaque types. Otherwise we eagerly call borrowck_check_region_constraints and apply the final closure region requirements right away.
Impact on stable code
Handling the opaque type uses in the parent function now only uses the closure requirements modulo opaques, while it previously also considered member constraints from nested bodies. External regions are never valid choice regions. Also, member constraints will never constrain a member region if it is required to be outlived by an external region, as that fails the upper-bound check. https://github.com/rust-lang/rust/blob/564ee219127b796d56f74767366fd359758b97de/compiler/rustc_borrowck/src/region_infer/opaque_types/member_constraints.rs#L90-L96
Member constraints therefore never add constraints for external regions :>
r? @BoxyUwU
github-actions bot pushed a commit to rust-lang/rust-analyzer that referenced this pull request
apply_member_constraints: fix placeholder check
Checking whether the member region is an existential region from a higher universe is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before rust-lang/rust#140466.
I've encountered this issue separately while working on rust-lang/rust#139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc rust-lang/rust#140569/rust-lang/rust#142073. However, as we always skipped these member regions in apply_member_constraints the skipped region is guaranteed to cause an error in check_member_constraints later on.
rust-cloud-vms bot pushed a commit to makai410/rustc_public that referenced this pull request
-Znext-solver: support non-defining uses in closures
Cleaned up version of rust-lang/rust#139587, finishing the implementation of rust-lang/types-team#129. This does not affect stable. The reasoning for why this is the case is subtle however.
What does it do
We split do_mir_borrowck into borrowck_collect_region_constraints and borrowck_check_region_constraints, where borrowck_collect_region_constraints returns an enormous CollectRegionConstraintsResult struct which contains all the relevant data to actually handle opaque type uses and to check the region constraints later on.
query mir_borrowck now simply calls BorrowCheckRootCtxt::do_mir_borrowck which starts by iterating over all nested bodies of the current function - visiting nested bodies before their parents - and computing their CollectRegionConstraintsResult.
After we've collected all constraints it's time to actually compute the concrete types for the opaques defined by this function. With this PR we now compute the concrete types of opaques for each body before using them to check the non-defining uses of any of them.
After we've computed the concrete types by using all bodies, we use apply_computed_concrete_opaque_types for each body to constrain non-defining uses, before finally finishing with borrowck_check_region_constraints. We always visit nested bodies before their parents when doing this.
ClosureRegionRequirements
As we only call borrowck_collect_region_constraints for nested bodies before type checking the parent, we can't simply use the final ClosureRegionRequirements of the nested body during MIR type check. We instead track that we need to apply these requirements in deferred_closure_requirements.
We now manually apply the final closure requirements to each body after handling opaque types.
This works, except that we may need the region constraints of nested bodies to successfully define an opaque type in the parent. This is handled by using a new fn compute_closure_requirements_modulo_opaques which duplicates region checking - while ignoring any errors - before we've added the constraints from apply_computed_concrete_opaque_types. This is necessary for a lot of async tests, as pretty much the entire function is inside of an async block while the opaque type gets defined in the parent.
As an performance optimization we only use fn compute_closure_requirements_modulo_opaques in case the nested body actually depends on any opaque types. Otherwise we eagerly call borrowck_check_region_constraints and apply the final closure region requirements right away.
Impact on stable code
Handling the opaque type uses in the parent function now only uses the closure requirements modulo opaques, while it previously also considered member constraints from nested bodies. External regions are never valid choice regions. Also, member constraints will never constrain a member region if it is required to be outlived by an external region, as that fails the upper-bound check. https://github.com/rust-lang/rust/blob/564ee219127b796d56f74767366fd359758b97de/compiler/rustc_borrowck/src/region_infer/opaque_types/member_constraints.rs#L90-L96
Member constraints therefore never add constraints for external regions :>
r? @BoxyUwU
makai410 pushed a commit to makai410/rust that referenced this pull request
makai410 pushed a commit to makai410/rust that referenced this pull request
apply_member_constraints: fix placeholder check
Checking whether the member region is an existential region from a higher universe is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before rust-lang#140466.
I've encountered this issue separately while working on rust-lang#139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc rust-lang#140569/rust-lang#142073. However, as we always skipped these member regions in apply_member_constraints the skipped region is guaranteed to cause an error in check_member_constraints later on.
makai410 pushed a commit to makai410/rust that referenced this pull request
makai410 pushed a commit to makai410/rust that referenced this pull request
apply_member_constraints: fix placeholder check
Checking whether the member region is an existential region from a higher universe is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before rust-lang#140466.
I've encountered this issue separately while working on rust-lang#139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc rust-lang#140569/rust-lang#142073. However, as we always skipped these member regions in apply_member_constraints the skipped region is guaranteed to cause an error in check_member_constraints later on.
makai410 pushed a commit to makai410/rust that referenced this pull request
…yUwU
-Znext-solver: support non-defining uses in closures
Cleaned up version of rust-lang#139587, finishing the implementation of rust-lang/types-team#129. This does not affect stable. The reasoning for why this is the case is subtle however.
What does it do
We split do_mir_borrowck into borrowck_collect_region_constraints and borrowck_check_region_constraints, where borrowck_collect_region_constraints returns an enormous CollectRegionConstraintsResult struct which contains all the relevant data to actually handle opaque type uses and to check the region constraints later on.
query mir_borrowck now simply calls BorrowCheckRootCtxt::do_mir_borrowck which starts by iterating over all nested bodies of the current function - visiting nested bodies before their parents - and computing their CollectRegionConstraintsResult.
After we've collected all constraints it's time to actually compute the concrete types for the opaques defined by this function. With this PR we now compute the concrete types of opaques for each body before using them to check the non-defining uses of any of them.
After we've computed the concrete types by using all bodies, we use apply_computed_concrete_opaque_types for each body to constrain non-defining uses, before finally finishing with borrowck_check_region_constraints. We always visit nested bodies before their parents when doing this.
ClosureRegionRequirements
As we only call borrowck_collect_region_constraints for nested bodies before type checking the parent, we can't simply use the final ClosureRegionRequirements of the nested body during MIR type check. We instead track that we need to apply these requirements in deferred_closure_requirements.
We now manually apply the final closure requirements to each body after handling opaque types.
This works, except that we may need the region constraints of nested bodies to successfully define an opaque type in the parent. This is handled by using a new fn compute_closure_requirements_modulo_opaques which duplicates region checking - while ignoring any errors - before we've added the constraints from apply_computed_concrete_opaque_types. This is necessary for a lot of async tests, as pretty much the entire function is inside of an async block while the opaque type gets defined in the parent.
As an performance optimization we only use fn compute_closure_requirements_modulo_opaques in case the nested body actually depends on any opaque types. Otherwise we eagerly call borrowck_check_region_constraints and apply the final closure region requirements right away.
Impact on stable code
Handling the opaque type uses in the parent function now only uses the closure requirements modulo opaques, while it previously also considered member constraints from nested bodies. External regions are never valid choice regions. Also, member constraints will never constrain a member region if it is required to be outlived by an external region, as that fails the upper-bound check. https://github.com/rust-lang/rust/blob/564ee219127b796d56f74767366fd359758b97de/compiler/rustc_borrowck/src/region_infer/opaque_types/member_constraints.rs#L90-L96
Member constraints therefore never add constraints for external regions :>
r? @BoxyUwU
makai410 pushed a commit to makai410/rustc_public that referenced this pull request
makai410 pushed a commit to makai410/rustc_public that referenced this pull request
apply_member_constraints: fix placeholder check
Checking whether the member region is an existential region from a higher universe is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before rust-lang/rust#140466.
I've encountered this issue separately while working on rust-lang/rust#139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc rust-lang/rust#140569/rust-lang/rust#142073. However, as we always skipped these member regions in apply_member_constraints the skipped region is guaranteed to cause an error in check_member_constraints later on.