Remove precise placeholder tracking from region inference by amandasystems · Pull Request #130227 · rust-lang/rust

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

This PR does shotgun surgery on borrowck to remove most special handling of placeholders, 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.

Placeholder-related errors are now detected earlier and tagged using error variants, making them explicit. The downstream plumbing in error_region does not fully utilise this fact and could presumably be simplified a lot. In particular, there is an elaborate reconstruction of computed universes that could and should get removed.

Some placeholder related metadata remains for evaluation of member constraints and type tests. Assuming those can be handled elsewhere, region inference will no longer need to take placeholders into account.

There are a bunch of rather nice bonuses:

We now don't need region indices in MirTypeckRegionConstraints; placeholders are now mapped to regions straight from a private hash map further up the chain. Presumably this opens the door to splitting it up into a build phase followed by a read-only use phase, but where that change happens beats me.
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.
Once member constraints are made into regular graph constraints, constraint search and blame choice is wholly dependent on the region graph and can be moved out of region inference.

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)

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

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

amandasystems · 2025-03-10T13:30:15Z

r? lcnr

amandasystems · 2025-03-11T11:29:57Z

NICE #137655 looked like it would make this complicated at first, while in fact it was a much more elegant implementation of a mechanism I hacked in. Cool!

bors · 2025-03-15T12:00:40Z

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

lcnr

have to take a bit more time to properly review this, some thoughts as i skimmed over it

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

bors · 2025-03-22T10:23:02Z

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

bors · 2025-05-01T19:30:31Z

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

Revise the extra `r: 'static` constraints added upon universe issues to add an explanation, and use that explanation during constraint blame search. This greatly simplifies the region inference logic, which now does not need to reverse-engineer the event that caused a region to outlive 'static.

amandasystems · 2025-06-24T15:37:28Z

Update: now rebased on the other two PRs!

Placeholders are now handled mostly by rewriting constraints as a pre-computation step, so precisely tracking which placeholder end up in which region is no longer necessary.

…traints`

bors · 2025-06-26T16:37:08Z

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

amandasystems · 2025-07-06T10:17:23Z

@rustbot blocked

Dylan-DPC · 2025-07-07T05:49:39Z

@amandasystems if you can add a reason why this is blocked, it will be useful for future tracking and triage purposes. Thanks

amandasystems · 2025-07-07T08:56:06Z

Or course! It’s blocked waiting for #140737 and #142623, which it’s based on.

amandasystems · 2026-01-22T16:15:25Z

This PR is fundamentally incompatible with future trait solver development, so we've decided to abandon it. Read more in my closing comment on #142623

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 removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 7, 2025

rustbot assigned lcnr and unassigned nikomatsakis Mar 10, 2025

lcnr reviewed Mar 19, 2025

View reviewed changes

This was referenced Apr 17, 2025

Add tests for two untested cases of placeholder relations #139960

Merged

Decouple SCC annotations from SCCs #139965

Merged

This was referenced Apr 24, 2025

Some drive-by housecleaning in rustc_borrowck #140257

Closed

Move placeholder handling to a proper preprocessing step #140466

Merged

amandasystems mentioned this pull request May 7, 2025

Region inference: Use outlives-static constraints in constraint search #140737

Merged

amandasystems mentioned this pull request Jun 17, 2025

Move placeholder error handling to before region inference #142623

Closed

amandasystems added 4 commits June 17, 2025 16:33

Move handling of placeholder errors to before region inference

07c226b

Bless omitted error messages

96b5ee8

Revise the tests for coroutine/auto-trait-regions

199f961

amandasystems added 3 commits June 25, 2025 12:00

Remove precise placeholder tracking during region inference

4958ec8

Placeholders are now handled mostly by rewriting constraints as a pre-computation step, so precisely tracking which placeholder end up in which region is no longer necessary.

Move the placeholder-to-region vid mapping out of `MirTypckRegionCons…

3212751

…traints`

Simplify now that placeholders are gone

23e3ffa

lqd mentioned this pull request Jun 27, 2025

Simplify polonius location-sensitive analysis #143093

Merged

Uh oh!

Conversation

amandasystems commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 11, 2024

Uh oh!

amandasystems commented Sep 11, 2024

Uh oh!

amandasystems commented Sep 11, 2024

Uh oh!

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

Uh oh!

lqd commented Sep 27, 2024

Uh oh!

This comment has been minimized.

bors commented Sep 27, 2024

Uh oh!

amandasystems commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

bors commented Sep 27, 2024

Uh oh!

This comment has been minimized.

bors commented Mar 9, 2025

Uh oh!

amandasystems commented Mar 10, 2025

Uh oh!

amandasystems commented Mar 11, 2025

Uh oh!

bors commented Mar 15, 2025

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented Mar 22, 2025

Uh oh!

bors commented May 1, 2025

Uh oh!

amandasystems commented Jun 24, 2025

Uh oh!

bors commented Jun 26, 2025

Uh oh!

amandasystems commented Jul 6, 2025

Uh oh!

Dylan-DPC commented Jul 7, 2025

Uh oh!

amandasystems commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amandasystems commented Jan 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

amandasystems commented Sep 11, 2024 •

edited

Loading

amandasystems commented Sep 27, 2024 •

edited

Loading

amandasystems commented Jul 7, 2025 •

edited

Loading