Rewrite handling of universe-leaking placeholder regions into outlives constraints by amandasystems · Pull Request #123720 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation76 Commits3 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This commit prepares for Polonius by moving handling of leak check/universe errors out of the inference step by rewriting any universe error into an outlives-static constraint.

This variant is a work in progress but seems to pass most tests.

Note that a few debug assertions no longer hold; a few extra eyes on those changes are appreciated!

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Apr 10, 2024

?r @nikomatsakis

(it's not 100% ready for review yet probably maybe, but I figured I might as well start the PR now)

This comment has been minimized.

the syntax is r? nikomatsakis 😆

the syntax is r? nikomatsakis 😆

That’s funny because I tried that first, thought I made a mistake, and went back to “fix” it

Update: I had a brainwave during my investigations of the failing UI tests (there's 20-ish of them if you enable the new logic everywhere, depending on whether you count duplicates). I tried adding a new constraint category so that I could easily not blame it (ever, actually), and that reduces the errors down to one, tiny span diff:

---- [ui] tests/ui/higher-ranked/trait-bounds/due-to-where-clause.rs stdout ----
diff of stderr:

2         --> $DIR/due-to-where-clause.rs:2:5
3          |
4       LL |     test::<FooS>(&mut 42);
-          |     ^^^^^^^^^^^^ implementation of `Foo` is not general enough
+          |     ^^^^^^^^^^^^^^^^^^^^^ implementation of `Foo` is not general enough
6          |
7          = note: `FooS<'_>` must implement `Foo<'0>`, for any lifetime `'0`...
8          = note: ...but `FooS<'_>` actually implements `Foo<'1>`, for some specific lifetime `'1`

I'm pretty confident this works now. Removing the WIP label!

amandasystems changed the title[WIP] Rewrite handling of universe-leaking placeholder regions into outlives constraints Rewrite handling of universe-leaking placeholder regions into outlives constraints

Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Efficiency nit

rustbot 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

Apr 12, 2024

I think it's ready for a perf run, @lqd!

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Apr 12, 2024

…=

Rewrite handling of universe-leaking placeholder regions into outlives constraints

This commit prepares for Polonius by moving handling of leak check/universe errors out of the inference step by rewriting any universe error into an outlives-static constraint.

This variant is a work in progress but seems to pass most tests.

Note that a few debug assertions no longer hold; a few extra eyes on those changes are appreciated!

This comment has been minimized.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so, I reviewed and left a (tiny) nit. I don't know that a perf run is really needed, this is pretty clearly grossly inefficient. That's precisely what I expected, of course.

r=me but I'd like to hand this off to somebody else to drive over the finish line

☀️ Try build successful - checks-actions
Build commit: 1b9e1e6 (1b9e1e65acb35d4d1d734c6461ace5c4e50a9fd0)

This comment has been minimized.

Finished benchmarking commit (1b9e1e6): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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

Warning ⚠: The following benchmark(s) failed to build:

• rustc
• wg-grammar
• webrender-2022

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

Max RSS (memory usage)

Results

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.

Cycles

Results

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.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: missing data
Artifact size: 315.97 MiB -> 305.14 MiB (-3.43%)

This comment has been minimized.

r=me with a nit

Nit addressed (and I agree, that's just the sort of bad naming I know has confused me before)

📌 Commit 9be3a3d has been approved by nikomatsakis

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jul 2, 2024

Finished benchmarking commit (67f0d43): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (primary 2.2%)

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.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 696.719s -> 695.519s (-0.17%)
Artifact size: 327.57 MiB -> 327.72 MiB (0.05%)

This is my first crater run so I'm not sure how to read the logs, but most of those errors look spurious to me, if not all of them.

Agreed, most of the 12 "regressions" look like crater errors. Since these crates not tested in the run however, I've checked them all locally: they build with this PR without issues.

We'll keep an eye out for possible new issues, and beta crater run regressions, that could be bisected to this PR, just in case the "spurious regressed" contains real effects (these can also be crater errors, but also contains OOMs).

But things look currently fine in my opinion.

lqd mentioned this pull request

Sep 1, 2024

18 tasks

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 27, 2024

…mpletely, r=

[WIP] Remove placeholders completely

This PR does shotgun surgery on borrowck to remove all special handling of placeholders, completely replacing them with a preprocessing step that rewrites placeholder leaks into constraints, removing constraint propagation of placeholders and all logic used to detect placeholder violations during error reporting. This finishes what rust-lang#123720 started.

The new method works like this:

1. during SCC construction, some information about SCC membership and reachability is retained
2. just after SCC construction, a constraint r - (from: to_invalid) - > 'static is added when r is the representative of an SCC and
   1. that SCC either has had its universe shrunk because it reaches a region with a smaller one (in which case to_invalid is the smallest-universed region it reaches), or if it reaches a region with a too large universe that isn't part of the SCC (in which case to_invalid is the region with a too large universe). In either case, from is also r.
3. some region reaches in r's SCC reaches another placeholder, reached, in which case the added constraint is r -> (reaches: reached) 'static. Through clever choice of defaults (chosing minimum elements), reached will be r if at all possible.

When tracing errors for diagnostics one of these special constraints along a path are treated much like a HTTP redirect: if we are explaining from: to and reach an edge with reaches: invalid we stop the search and start following reaches: invalid instead. When doing this the implicit edges x: 'static for every region x are ignored, since the search would otherwise be able to cheat by going through 'static and re-find the same edge again.

A bunch of optimisations are possible:

• Conservatively add constraints, e.g. one per SCC. May worsen error tracing!
• as a final pass, allow fusing the annotations for the SCC after adding the extra constraints to remove unnecessary information and save memory. This could be done cheaply since we already iterate over the entire SCC.
• currently, if constraints are added the entire set of SCCs are recomputed. This is of course rather wasteful, and we could maybe do better.

There are a bunch of rather nice bonuses:

• We now don't need to expose region indices in MirTypeckRegionConstraints to the entire crate. The only entry point is placeholder_region() so correctness of the indices is now guaranteed
• A lot of things that were previously iterations over lists is now a single lookup
• The constraint graph search functions are simple and at least one of them can now take a proper region as target rather than a predicate function. The only case that needs the predicate argument to find_constraint_path_to() is find_sub_region_live_at(), which may or may not be possible to work around.

r​? nikomatsakis

bors added a commit to rust-lang-ci/rust that referenced this pull request

Oct 1, 2024

…mpletely, r=

[WIP] Remove placeholders completely

This PR does shotgun surgery on borrowck to remove all special handling of placeholders, completely replacing them with a preprocessing step that rewrites placeholder leaks into constraints, removing constraint propagation of placeholders and all logic used to detect placeholder violations during error reporting. This finishes what rust-lang#123720 started.

The new method works like this:

1. during SCC construction, some information about SCC membership and reachability is retained
2. just after SCC construction, a constraint r - (from: to_invalid) - > 'static is added when r is the representative of an SCC and
   1. that SCC either has had its universe shrunk because it reaches a region with a smaller one (in which case to_invalid is the smallest-universed region it reaches), or if it reaches a region with a too large universe that isn't part of the SCC (in which case to_invalid is the region with a too large universe). In either case, from is also r.
3. some region reaches in r's SCC reaches another placeholder, reached, in which case the added constraint is r -> (reaches: reached) 'static. Through clever choice of defaults (chosing minimum elements), reached will be r if at all possible.

When tracing errors for diagnostics one of these special constraints along a path are treated much like a HTTP redirect: if we are explaining from: to and reach an edge with reaches: invalid we stop the search and start following reaches: invalid instead. When doing this the implicit edges x: 'static for every region x are ignored, since the search would otherwise be able to cheat by going through 'static and re-find the same edge again.

A bunch of optimisations are possible:

• Conservatively add constraints, e.g. one per SCC. May worsen error tracing!
• as a final pass, allow fusing the annotations for the SCC after adding the extra constraints to remove unnecessary information and save memory. This could be done cheaply since we already iterate over the entire set of SCCs.
• currently, if constraints are added the entire set of SCCs are recomputed. This is of course rather wasteful, and we could do better. Especially since SCCs are added in dependency order. This would require a fully separate SCC module since the dynamic SCC combo we'd need now shares almost no properties with regular SCC computation. Given that this is meant to be a temporary work-around, that seems like too much work.

There are a bunch of rather nice bonuses:

• We now don't need to expose region indices in MirTypeckRegionConstraints to the entire crate. The only entry point is placeholder_region() so correctness of the indices is now guaranteed
• A lot of things that were previously iterations over lists is now a single lookup
• The constraint graph search functions are simple and at least one of them can now take a proper region as target rather than a predicate function. The only case that needs the predicate argument to find_constraint_path_to() is find_sub_region_live_at(), which may or may not be possible to work around.

r​? nikomatsakis

bors added a commit to rust-lang-ci/rust that referenced this pull request

Oct 4, 2024

…mpletely, r=

[WIP] Remove placeholders completely

This PR does shotgun surgery on borrowck to remove all special handling of placeholders, completely replacing them with a preprocessing step that rewrites placeholder leaks into constraints, removing constraint propagation of placeholders and all logic used to detect placeholder violations during error reporting. This finishes what rust-lang#123720 started.

The new method works like this:

1. during SCC construction, some information about SCC membership and reachability is retained
2. just after SCC construction, a constraint r - (from: to_invalid) - > 'static is added when r is the representative of an SCC and
   1. that SCC either has had its universe shrunk because it reaches a region with a smaller one (in which case to_invalid is the smallest-universed region it reaches), or if it reaches a region with a too large universe that isn't part of the SCC (in which case to_invalid is the region with a too large universe). In either case, from is also r.
3. some region reaches in r's SCC reaches another placeholder, reached, in which case the added constraint is r -> (reaches: reached) 'static. Through clever choice of defaults (chosing minimum elements), reached will be r if at all possible.

When tracing errors for diagnostics one of these special constraints along a path are treated much like a HTTP redirect: if we are explaining from: to and reach an edge with reaches: invalid we stop the search and start following reaches: invalid instead. When doing this the implicit edges x: 'static for every region x are ignored, since the search would otherwise be able to cheat by going through 'static and re-find the same edge again.

A bunch of optimisations are possible:

• Conservatively add constraints, e.g. one per SCC. May worsen error tracing!
• as a final pass, allow fusing the annotations for the SCC after adding the extra constraints to remove unnecessary information and save memory. This could be done cheaply since we already iterate over the entire set of SCCs.
• currently, if constraints are added the entire set of SCCs are recomputed. This is of course rather wasteful, and we could do better. Especially since SCCs are added in dependency order. This would require a fully separate SCC module since the dynamic SCC combo we'd need now shares almost no properties with regular SCC computation. Given that this is meant to be a temporary work-around, that seems like too much work.

There are a bunch of rather nice bonuses:

• We now don't need to expose region indices in MirTypeckRegionConstraints to the entire crate. The only entry point is placeholder_region() so correctness of the indices is now guaranteed
• A lot of things that were previously iterations over lists is now a single lookup
• The constraint graph search functions are simple and at least one of them can now take a proper region as target rather than a predicate function. The only case that needs the predicate argument to find_constraint_path_to() is find_sub_region_live_at(), which may or may not be possible to work around.

r​? nikomatsakis

bors added a commit to rust-lang-ci/rust that referenced this pull request

Oct 13, 2024

…mpletely, r=

[WIP] Remove placeholders completely

This PR does shotgun surgery on borrowck to remove all special handling of placeholders, completely replacing them with a preprocessing step that rewrites placeholder leaks into constraints, removing constraint propagation of placeholders and all logic used to detect placeholder violations during error reporting. This finishes what rust-lang#123720 started.

The new method works like this:

1. during SCC construction, some information about SCC membership and reachability is retained
2. just after SCC construction, a constraint r - (from: to_invalid) - > 'static is added when r is the representative of an SCC and
   1. that SCC either has had its universe shrunk because it reaches a region with a smaller one (in which case to_invalid is the smallest-universed region it reaches), or if it reaches a region with a too large universe that isn't part of the SCC (in which case to_invalid is the region with a too large universe). In either case, from is also r.
3. some region reaches in r's SCC reaches another placeholder, reached, in which case the added constraint is r -> (reaches: reached) 'static. Through clever choice of defaults (chosing minimum elements), reached will be r if at all possible.

When tracing errors for diagnostics one of these special constraints along a path are treated much like a HTTP redirect: if we are explaining from: to and reach an edge with reaches: invalid we stop the search and start following reaches: invalid instead. When doing this the implicit edges x: 'static for every region x are ignored, since the search would otherwise be able to cheat by going through 'static and re-find the same edge again.

A bunch of optimisations are possible:

• Conservatively add constraints, e.g. one per SCC. May worsen error tracing!
• as a final pass, allow fusing the annotations for the SCC after adding the extra constraints to remove unnecessary information and save memory. This could be done cheaply since we already iterate over the entire set of SCCs.
• currently, if constraints are added the entire set of SCCs are recomputed. This is of course rather wasteful, and we could do better. Especially since SCCs are added in dependency order. This would require a fully separate SCC module since the dynamic SCC combo we'd need now shares almost no properties with regular SCC computation. Given that this is meant to be a temporary work-around, that seems like too much work.

There are a bunch of rather nice bonuses:

• We now don't need to expose region indices in MirTypeckRegionConstraints to the entire crate. The only entry point is placeholder_region() so correctness of the indices is now guaranteed
• A lot of things that were previously iterations over lists is now a single lookup
• The constraint graph search functions are simple and at least one of them can now take a proper region as target rather than a predicate function. The only case that needs the predicate argument to find_constraint_path_to() is find_sub_region_live_at(), which may or may not be possible to work around.

r​? nikomatsakis

bors added a commit to rust-lang-ci/rust that referenced this pull request

Nov 6, 2024

…mpletely, r=

[WIP] Remove placeholders completely

This PR does shotgun surgery on borrowck to remove all special handling of placeholders, completely replacing them with a preprocessing step that rewrites placeholder leaks into constraints, removing constraint propagation of placeholders and all logic used to detect placeholder violations during error reporting. This finishes what rust-lang#123720 started.

The new method works like this:

1. during SCC construction, some information about SCC membership and reachability is retained
2. just after SCC construction, a constraint r - (from: to_invalid) - > 'static is added when r is the representative of an SCC and
   1. that SCC either has had its universe shrunk because it reaches a region with a smaller one (in which case to_invalid is the smallest-universed region it reaches), or if it reaches a region with a too large universe that isn't part of the SCC (in which case to_invalid is the region with a too large universe). In either case, from is also r.
3. some region reaches in r's SCC reaches another placeholder, reached, in which case the added constraint is r -> (reaches: reached) 'static. Through clever choice of defaults (chosing minimum elements), reached will be r if at all possible.

When tracing errors for diagnostics one of these special constraints along a path are treated much like a HTTP redirect: if we are explaining from: to and reach an edge with reaches: invalid we stop the search and start following reaches: invalid instead. When doing this the implicit edges x: 'static for every region x are ignored, since the search would otherwise be able to cheat by going through 'static and re-find the same edge again.

A bunch of optimisations are possible:

• Conservatively add constraints, e.g. one per SCC. May worsen error tracing!
• as a final pass, allow fusing the annotations for the SCC after adding the extra constraints to remove unnecessary information and save memory. This could be done cheaply since we already iterate over the entire set of SCCs.
• currently, if constraints are added the entire set of SCCs are recomputed. This is of course rather wasteful, and we could do better. Especially since SCCs are added in dependency order. This would require a fully separate SCC module since the dynamic SCC combo we'd need now shares almost no properties with regular SCC computation. Given that this is meant to be a temporary work-around, that seems like too much work.

There are a bunch of rather nice bonuses:

• We now don't need to expose region indices in MirTypeckRegionConstraints to the entire crate. The only entry point is placeholder_region() so correctness of the indices is now guaranteed
• A lot of things that were previously iterations over lists is now a single lookup
• The constraint graph search functions are simple and at least one of them can now take a proper region as target rather than a predicate function. The only case that needs the predicate argument to find_constraint_path_to() is find_sub_region_live_at(), which may or may not be possible to work around.

r​? nikomatsakis

bors added a commit to rust-lang-ci/rust that referenced this pull request

Nov 15, 2024

…mpletely, r=

[WIP] Remove placeholders completely

This PR does shotgun surgery on borrowck to remove all special handling of placeholders, completely replacing them with a preprocessing step that rewrites placeholder leaks into constraints, removing constraint propagation of placeholders and all logic used to detect placeholder violations during error reporting. This finishes what rust-lang#123720 started.

The new method works like this:

1. during SCC construction, some information about SCC membership and reachability is retained
2. just after SCC construction, a constraint r - (from: to_invalid) - > 'static is added when r is the representative of an SCC and
   1. that SCC either has had its universe shrunk because it reaches a region with a smaller one (in which case to_invalid is the smallest-universed region it reaches), or if it reaches a region with a too large universe that isn't part of the SCC (in which case to_invalid is the region with a too large universe). In either case, from is also r.
3. some region reaches in r's SCC reaches another placeholder, reached, in which case the added constraint is r -> (reaches: reached) 'static. Through clever choice of defaults (chosing minimum elements), reached will be r if at all possible.

When tracing errors for diagnostics one of these special constraints along a path are treated much like a HTTP redirect: if we are explaining from: to and reach an edge with reaches: invalid we stop the search and start following reaches: invalid instead. When doing this the implicit edges x: 'static for every region x are ignored, since the search would otherwise be able to cheat by going through 'static and re-find the same edge again.

A bunch of optimisations are possible:

• Conservatively add constraints, e.g. one per SCC. May worsen error tracing!
• as a final pass, allow fusing the annotations for the SCC after adding the extra constraints to remove unnecessary information and save memory. This could be done cheaply since we already iterate over the entire set of SCCs.
• currently, if constraints are added the entire set of SCCs are recomputed. This is of course rather wasteful, and we could do better. Especially since SCCs are added in dependency order. This would require a fully separate SCC module since the dynamic SCC combo we'd need now shares almost no properties with regular SCC computation. Given that this is meant to be a temporary work-around, that seems like too much work.

There are a bunch of rather nice bonuses:

• We now don't need to expose region indices in MirTypeckRegionConstraints to the entire crate. The only entry point is placeholder_region() so correctness of the indices is now guaranteed
• A lot of things that were previously iterations over lists is now a single lookup
• The constraint graph search functions are simple and at least one of them can now take a proper region as target rather than a predicate function. The only case that needs the predicate argument to find_constraint_path_to() is find_sub_region_live_at(), which may or may not be possible to work around.

r​? nikomatsakis

bors added a commit to rust-lang-ci/rust that referenced this pull request

Feb 28, 2025

…mpletely, r=

[WIP] Remove placeholders completely

This PR does shotgun surgery on borrowck to remove all special handling of placeholders, completely replacing them with a preprocessing step that rewrites placeholder leaks into constraints, removing constraint propagation of placeholders and most of the logic used to detect placeholder violations during error reporting. This finishes what rust-lang#123720 started. Due to the already sprawling scope of this PR, not all the breaks are clean. In particular, some of the error reporting code can almost certainly be further simplified.

The new method works like this:

1. during SCC construction, some information about SCC membership and reachability is retained
2. just after SCC construction, a constraint r - (from: to_invalid) - > 'static is added when r is the representative of an SCC and
   1. that SCC either has had its universe shrunk because it reaches a region with a smaller one (in which case to_invalid is the smallest-universed region it reaches), or if it reaches a region with a too large universe that isn't part of the SCC (in which case to_invalid is the region with a too large universe). In either case, from is also r.
3. some region reaches in r's SCC reaches another placeholder, reached, in which case the added constraint is r -> (reaches: reached) 'static. Through clever choice of defaults (chosing minimum elements), reached will be r if at all possible.

When tracing errors for diagnostics one of these special constraints along a path are treated much like a HTTP redirect: if we are explaining from: to and reach an edge with reaches: invalid we stop the search and start following reaches: invalid instead. When doing this the implicit edges x: 'static for every region x are ignored, since the search would otherwise be able to cheat by going through 'static and re-find the same edge again.

Type-tests are also rewritten to account for placeholder issues. In particular, if a bound implies : 'static, this is flagged using a new variant, and if a test is guaranteed to always fail (e.g. if an equals bound reaches different placeholders), it is replaced with a bound that is always unsatisfied.

A bunch of optimisations are possible:

• Conservatively add constraints, e.g. one per SCC. May worsen error tracing!
• as a final pass, allow fusing the annotations for the SCC after adding the extra constraints to remove unnecessary information and save memory. This could be done cheaply since we already iterate over the entire set of SCCs.
• currently, if constraints are added the entire set of SCCs are recomputed. This is of course rather wasteful, and we could do better. Especially since SCCs are added in dependency order. This would require a fully separate SCC module since the dynamic SCC combo we'd need now shares almost no properties with regular SCC computation. Given that this is meant to be a temporary work-around, that seems like too much work.

There are a bunch of rather nice bonuses:

• We now don't need to expose region indices in MirTypeckRegionConstraints to the entire crate. The only entry point is placeholder_region() so correctness of the indices is now guaranteed
• A lot of things that were previously iterations over lists is now a single lookup
• The constraint graph search functions are simple and at least one of them can now take a proper region as target rather than a predicate function. The only case that needs the predicate argument to find_constraint_path_to() is find_sub_region_live_at(), which may or may not be possible to work around.

r​? nikomatsakis

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 7, 2025

…mpletely, r=

[WIP] Remove placeholders completely

This PR does shotgun surgery on borrowck to remove all special handling of placeholders, completely replacing them with a preprocessing step that rewrites placeholder leaks into constraints, removing constraint propagation of placeholders and most of the logic used to detect placeholder violations during error reporting. This finishes what rust-lang#123720 started. Due to the already sprawling scope of this PR, not all the breaks are clean. In particular, some of the error reporting code can almost certainly be further simplified.

The new method works like this:

1. during SCC construction, some information about SCC membership and reachability is retained
2. just after SCC construction, a constraint r - (from: to_invalid) - > 'static is added when r is the representative of an SCC and
   1. that SCC either has had its universe shrunk because it reaches a region with a smaller one (in which case to_invalid is the smallest-universed region it reaches), or if it reaches a region with a too large universe that isn't part of the SCC (in which case to_invalid is the region with a too large universe). In either case, from is also r.
3. some region reaches in r's SCC reaches another placeholder, reached, in which case the added constraint is r -> (reaches: reached) 'static. Through clever choice of defaults (chosing minimum elements), reached will be r if at all possible.

When tracing errors for diagnostics one of these special constraints along a path are treated much like a HTTP redirect: if we are explaining from: to and reach an edge with reaches: invalid we stop the search and start following reaches: invalid instead. When doing this the implicit edges x: 'static for every region x are ignored, since the search would otherwise be able to cheat by going through 'static and re-find the same edge again.

Type-tests are also rewritten to account for placeholder issues. In particular, if a bound implies : 'static, this is flagged using a new variant, and if a test is guaranteed to always fail (e.g. if an equals bound reaches different placeholders), it is replaced with a bound that is always unsatisfied.

A bunch of optimisations are possible:

• Conservatively add constraints, e.g. one per SCC. May worsen error tracing!
• as a final pass, allow fusing the annotations for the SCC after adding the extra constraints to remove unnecessary information and save memory. This could be done cheaply since we already iterate over the entire set of SCCs.
• currently, if constraints are added the entire set of SCCs are recomputed. This is of course rather wasteful, and we could do better. Especially since SCCs are added in dependency order. This would require a fully separate SCC module since the dynamic SCC combo we'd need now shares almost no properties with regular SCC computation. Given that this is meant to be a temporary work-around, that seems like too much work.

There are a bunch of rather nice bonuses:

• We now don't need to expose region indices in MirTypeckRegionConstraints to the entire crate. The only entry point is placeholder_region() so correctness of the indices is now guaranteed
• A lot of things that were previously iterations over lists is now a single lookup
• The constraint graph search functions are simple and at least one of them can now take a proper region as target rather than a predicate function. The only case that needs the predicate argument to find_constraint_path_to() is find_sub_region_live_at(), which may or may not be possible to work around.

r​? nikomatsakis

Labels

merged-by-bors

This PR was explicitly merged by bors.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.