Skip to content

Remove placeholders completely #130227

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

Already on GitHub? Sign in to your account

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

amandasystems
Copy link
Contributor

@amandasystems amandasystems commented Sep 11, 2024

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:

  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

@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2024

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

@rustbot 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
@amandasystems
Copy link
Contributor Author

r? nikomatsakis

@rustbot rustbot assigned nikomatsakis and unassigned TaKO8Ki Sep 11, 2024
@amandasystems
Copy link
Contributor Author

@rustbot author

@rustbot 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
@rust-log-analyzer

This comment has been minimized.

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

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Sep 23, 2024

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

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

lqd commented Sep 27, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

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

bors commented Sep 27, 2024

⌛ Trying commit 4633c56 with merge b8cc49b...

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
@amandasystems
Copy link
Contributor Author

amandasystems commented Sep 27, 2024

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

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Sep 27, 2024

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

@rust-timer

This comment has been minimized.

@amandasystems amandasystems force-pushed the remove-placeholders-completely branch from 733bb16 to 55e292d Compare March 19, 2025 15:42
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

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

@bors
Copy link
Collaborator

bors commented Mar 22, 2025

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

amandasystems added a commit to amandasystems/rust that referenced this pull request Apr 17, 2025
During work on rust-lang#130227, I discovered several situations not covered by any
previously existing UI test. This commit introudces tests to cover that.
amandasystems added a commit to amandasystems/rust that referenced this pull request Apr 17, 2025
This rewires SCC annotations to have them be a separate,
visitor-type data structure. It was broken out of rust-lang#130227,
which needed them to be able to remove unused annotations
after computation without recomputing the SCCs themselves.

As a drive-by it also removes some redundant code from
the hot loop in SCC construction for a performance improvement.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2025
…c-annotations, r=<try>

Decouple SCC annotations from SCCs

This rewires SCC annotations to have them be a separate, visitor-type data structure. It was broken out of rust-lang#130227, which needed them to be able to remove unused annotations after computation without recomputing the SCCs themselves.

As a drive-by it also removes some redundant code from the hot loop in SCC construction for a performance improvement.

r? lcnr
amandasystems added a commit to amandasystems/rust that referenced this pull request Apr 17, 2025
During work on rust-lang#130227, I discovered several situations not covered by any
previously existing UI test. This commit introudces tests to cover that.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 17, 2025
…, r=lcnr

Add tests for two untested cases of placeholder relations

During work on rust-lang#130227, I discovered several situations not covered by any previously existing UI test. This commit introudces tests to cover that.

r? lcnr
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 17, 2025
…, r=lcnr

Add tests for two untested cases of placeholder relations

During work on rust-lang#130227, I discovered several situations not covered by any previously existing UI test. This commit introudces tests to cover that.

r? lcnr
amandasystems added a commit to amandasystems/rust that referenced this pull request Apr 22, 2025
During work on rust-lang#130227, I discovered several situations not covered by any
previously existing UI test. This commit introudces tests to cover that.
amandasystems added a commit to amandasystems/rust that referenced this pull request Apr 22, 2025
This rewires SCC annotations to have them be a separate,
visitor-type data structure. It was broken out of rust-lang#130227,
which needed them to be able to remove unused annotations
after computation without recomputing the SCCs themselves.

As a drive-by it also removes some redundant code from
the hot loop in SCC construction for a performance improvement.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 22, 2025
…r=lcnr

Add tests for two untested cases of placeholder relations

During work on rust-lang#130227, I discovered several situations not covered by any previously existing UI test. This commit introudces tests to cover that.

r? lcnr
joshlf pushed a commit to joshlf/rust that referenced this pull request Apr 22, 2025
During work on rust-lang#130227, I discovered several situations not covered by any
previously existing UI test. This commit introudces tests to cover that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants