[WIP] Remove placeholders completely #130227
Open
+933
−769
Conversation
This file contains 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
rustbot
added
S-waiting-on-review
|
r? nikomatsakis |
|
@rustbot author |
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
amandasystems
force-pushed
the
remove-placeholders-completely
branch
from
07b10ca
to
6ab45ce
Compare
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.
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.
|
☔ The latest upstream changes (presumably #130724) made this pull request unmergeable. Please resolve the merge conflicts. |
amandasystems
force-pushed
the
remove-placeholders-completely
branch
from
da9ccef
to
4633c56
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 27, 2024
…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
|
Failing test cases left: "for any two lifetimes" becomes "for any one", with the other replaced by the user-supplied region variable
missing "note: due to current limitations in the borrow checker, this implies a
|
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
rustbot
removed
the
S-waiting-on-perf
amandasystems
force-pushed
the
remove-placeholders-completely
branch
from
6d284ce
to
40f3aea
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 13, 2024
…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 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
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (4bb6bd0): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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. @bors rollup=never Instruction countThis 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.
Max RSS (memory usage)Results (primary -1.4%, secondary 1.4%)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.
CyclesResults (secondary -1.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 sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 781.228s -> 782.066s (0.11%) |
rustbot
removed
S-waiting-on-perf
3 tasks
amandasystems
force-pushed
the
remove-placeholders-completely
branch
from
40f3aea
to
9cbe0a8
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #132581) made this pull request unmergeable. Please resolve the merge conflicts. |
This version is maximally naive, and currently unsound. Handle more placeholder errors. This update 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 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. Use annotions on the outlives-static constraints to help the search for who is to blame. Invalid placeholder relation constraints are redirects This commit makes the search for blamable outlives constraints treat an added `x: 'static` edge as a redirect to figure out why it reached an invalid placeholder. 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. This version mostly works! I've switched to encoding our new outlives-static constraints with two extra parameters: the source and drain of a path in the original constraint graph that caused a placeholder issue. This handles all of the soundness issues, leaving 16 failing diagnostics. Remove some stuff we don't need or can't use anymore Figure out better names for opaque types! Fix tidy error Fix most of the cases of missing "implied 'static" warnings Fix documentation that build-failed
This seems to hit the sweet spot between enabling debugging and being efficient. Somewhat cleaner representation that separates concerns Avoid handling placeholders for MIR bodies without them Determine once and for all if there are higher-kinded concerns and nope out This also changes how annotations for SCCs work. This version finally does away with all kinds of universe handling It does so in a preprocessing step that now also removes higher-ranked concerns from type tests. Fix handling of opaque regions
amandasystems
force-pushed
the
remove-placeholders-completely
branch
from
9cbe0a8
to
372912b
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 6, 2024
…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 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
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (f258cb4): comparison URL. Overall result: ✅ improvements - BENCHMARK(S) FAILEDBenchmarking 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. @bors rollup=never ❗ ❗ ❗ ❗ ❗
❗ ❗ ❗ ❗ ❗ Instruction countThis 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.
Max RSS (memory usage)Results (primary -0.4%, secondary -0.8%)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.
CyclesResults (secondary -6.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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 781.244s -> 779.734s (-0.19%) |
rustbot
removed
the
S-waiting-on-perf
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #123720 started.
The new method works like this:
r - (from: to_invalid) - > 'staticis added whenris the representative of an SCC andto_invalidis 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 caseto_invalidis the region with a too large universe). In either case,fromis alsor.reachesinr's SCC reaches another placeholder,reached, in which case the added constraint isr -> (reaches: reached) 'static. Through clever choice of defaults (chosing minimum elements),reachedwill berif 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: toand reach an edge withreaches: invalidwe stop the search and start followingreaches: invalidinstead. When doing this the implicit edgesx: 'staticfor every regionxare 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!There are a bunch of rather nice bonuses:
MirTypeckRegionConstraintsto the entire crate. The only entry point isplaceholder_region()so correctness of the indices is now guaranteedfind_constraint_path_to()isfind_sub_region_live_at(), which may or may not be possible to work around.r? nikomatsakis