[WIP] Remove placeholders completely #130227

amandasystems · 2024-09-11T13:55:54Z

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 #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:

during SCC construction, some information about SCC membership and reachability is retained
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.
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

rustbot · 2024-09-11T13:56:03Z

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
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

amandasystems · 2024-09-11T13:57:12Z

r? nikomatsakis

amandasystems · 2024-09-11T13:58:10Z

@rustbot author

bors · 2024-09-23T05:19:22Z

☔ The latest upstream changes (presumably #130724) made this pull request unmergeable. Please resolve the merge conflicts.

lqd · 2024-09-27T14:58:15Z

@bors try @rust-timer queue

bors · 2024-09-27T14:59:26Z

⌛ Trying commit 4633c56 with merge b8cc49b...

…mpletely, r=<try> [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`. 2. 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

amandasystems · 2024-09-27T15:06:30Z

Failing test cases left:

"for any two lifetimes" becomes "for any one", with the other replaced by the user-supplied region variable

tests/ui/associated-types/associated-types-eq-hr.rs
tests/ui/async-await/return-type-notation/issue-110963-early.rs
tests/ui/coroutine/auto-trait-regions.rs
tests/ui/higher-ranked/trait-bounds/hrtb-conflate-regions.rs

missing "note: due to current limitations in the borrow checker, this implies a 'static lifetime":

tests/ui/generic-associated-types/bugs/hrtb-implied-2.rs

bors · 2024-09-27T16:42:45Z

☀️ Try build successful - checks-actions
Build commit: b8cc49b (b8cc49b4b6432618a26e2a74fec6b342ee94484e)

rust-timer · 2025-02-28T19:51:31Z

Finished benchmarking commit (a87ebda): 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
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	2
Improvements ✅ (secondary)	-0.2%	[-0.3%, -0.1%]	9
All ❌✅ (primary)	-0.2%	[-0.3%, -0.2%]	2

Max RSS (memory usage)

Results (primary -3.2%, secondary 1.0%)

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)	2.9%	[2.7%, 3.0%]	2
Improvements ✅ (primary)	-3.2%	[-4.9%, -0.9%]	4
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	-3.2%	[-4.9%, -0.9%]	4

Cycles

Results (secondary 1.6%)

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)	3.2%	[3.0%, 3.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-1.5%, -1.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 771.133s -> 770.759s (-0.05%)
Artifact size: 361.91 MiB -> 361.98 MiB (0.02%)

This commit moves all handling of placeholders, including those in type-tests, into a pre-processing step of the borrowcheck. Along the way it extends the SCC metadata tracking a lot and uses the extra information to add p: 'static for any placeholder that reaches another placeholder. It also rewrites type-tests that have the same behaviour. Error reporting is handled by introducing a new constraint kind that remembers which original outlives constraint caused an outlives-static. These work a lot like a HTTP redirect response during constraint search. `RegionInferenceContext` is now a lot simpler and only tracks opaque regions without caring about their origin. It also inlines the few measly bits of `init_free_and_bound_regions()` that still remain as relevant. This increases the constructor for `RegionInferenceContext`s somewhat, but I still think it's readable. The documentation for `init_free_and_bound_regions()` was out of date, and the correct, up to date version is available in the various places where the logic was moved. As a drive-by it also refactors the blame search somewhat, renames a few methods, and allows iterating over outgoing constraints without the implied edges from 'static.

The change consists of a few changed spans and a reordering of emitted errors. This seems benign to me, and moreover very difficult to fix since the old behaviour relies on information that is no longer retained.

lqd · 2025-03-07T12:49:04Z

@bors try @rust-timer queue

…mpletely, r=<try> [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`. 2. 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 · 2025-03-07T12:50:17Z

⌛ Trying commit 91e5a1f with merge ae2232c5832648e304c96fb77aa4768170f5f5dc...

bors · 2025-03-07T14:50:04Z

☀️ Try build successful - checks-actions
Build commit: ae2232c (ae2232c5832648e304c96fb77aa4768170f5f5dc)

rust-timer · 2025-03-07T16:20:05Z

Finished benchmarking commit (ae2232c): 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
Regressions ❌ (secondary)	0.5%	[0.4%, 1.0%]	7
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.2%]	3
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.2%]	8
All ❌✅ (primary)	-0.3%	[-0.3%, -0.2%]	3

Max RSS (memory usage)

Results (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)	-	-	0
Regressions ❌ (secondary)	3.1%	[2.9%, 3.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.9%, -2.6%]	5
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -1.9%)

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)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 765.289s -> 766.369s (0.14%)
Artifact size: 362.07 MiB -> 362.09 MiB (0.01%)

bors · 2025-03-09T18:53:08Z

☔ The latest upstream changes (presumably #137655) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned TaKO8Ki Sep 11, 2024

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 Sep 11, 2024

rustbot assigned nikomatsakis and unassigned TaKO8Ki Sep 11, 2024

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 Sep 11, 2024

This comment has been minimized.

Sign in to view

amandasystems force-pushed the remove-placeholders-completely branch from 07b10ca to 6ab45ce Compare September 12, 2024 13:52

This comment has been minimized.

Sign in to view

amandasystems force-pushed the remove-placeholders-completely branch from da9ccef to 4633c56 Compare September 27, 2024 14:56

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 27, 2024

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 28, 2025

amandasystems force-pushed the remove-placeholders-completely branch from f6d7c6f to 22b9286 Compare March 3, 2025 12:40

This comment has been minimized.

Sign in to view

amandasystems force-pushed the remove-placeholders-completely branch from ad07f6a to 291bc8e Compare March 3, 2025 16:09

amandasystems added 5 commits March 5, 2025 14:18

An attempt at rebasing on upstream

cf5ed30

Fix build failure

e143ec4

Rejigger handling of VerifyIfEq

0702a42

Clean up some of the error handling code

9fd80b0

amandasystems force-pushed the remove-placeholders-completely branch from 291bc8e to 9fd80b0 Compare March 5, 2025 16:11

amandasystems added 2 commits March 5, 2025 17:24

Inline add_placeholder_violation_constraint

fe76f37

Clean up error handling FIXMEs

ac05bf2

amandasystems force-pushed the remove-placeholders-completely branch from 89c0235 to ac05bf2 Compare March 6, 2025 11:50

amandasystems added 3 commits March 6, 2025 13:15

More cleanup

81ced87

Bless the new output of nll/ice-106874.rs

178f357

The change consists of a few changed spans and a reordering of emitted errors. This seems benign to me, and moreover very difficult to fix since the old behaviour relies on information that is no longer retained.

Remove universes from expected MIR output in tests

91e5a1f

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 7, 2025

This comment has been minimized.

Sign in to view

Simplify report_erroneous_element

815d58c

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Remove placeholders completely #130227

[WIP] Remove placeholders completely #130227

amandasystems commented Sep 11, 2024 •

edited

Loading

rustbot commented Sep 11, 2024

amandasystems commented Sep 11, 2024

amandasystems commented Sep 11, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Sep 23, 2024

lqd commented Sep 27, 2024

This comment has been minimized.

bors commented Sep 27, 2024

amandasystems commented Sep 27, 2024 •

edited

Loading

This comment has been minimized.

bors commented Sep 27, 2024

This comment has been minimized.

rust-timer commented Feb 28, 2025

This comment has been minimized.

This comment has been minimized.

lqd commented Mar 7, 2025

This comment has been minimized.

bors commented Mar 7, 2025

bors commented Mar 7, 2025

This comment has been minimized.

rust-timer commented Mar 7, 2025

bors commented Mar 9, 2025

[WIP] Remove placeholders completely #130227

Are you sure you want to change the base?

[WIP] Remove placeholders completely #130227

Conversation

amandasystems commented Sep 11, 2024 • edited Loading

rustbot commented Sep 11, 2024

amandasystems commented Sep 11, 2024

amandasystems commented Sep 11, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Sep 23, 2024

lqd commented Sep 27, 2024

This comment has been minimized.

bors commented Sep 27, 2024

amandasystems commented Sep 27, 2024 • edited Loading

This comment has been minimized.

bors commented Sep 27, 2024

This comment has been minimized.

rust-timer commented Feb 28, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

This comment has been minimized.

This comment has been minimized.

lqd commented Mar 7, 2025

This comment has been minimized.

bors commented Mar 7, 2025

bors commented Mar 7, 2025

This comment has been minimized.

rust-timer commented Mar 7, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

bors commented Mar 9, 2025

amandasystems commented Sep 11, 2024 •

edited

Loading

amandasystems commented Sep 27, 2024 •

edited

Loading