Skip to content
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

Split implied and super predicate queries, then allow elaborator to filter only supertraits #107614

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Feb 2, 2023

Split the super_predicates_of query into a new implied_predicates_of query. The former now only returns the real supertraits of a trait alias, and the latter now returns the implied predicates (which include all of the where clauses of the trait alias). The behavior of these queries is identical for regular traits.

Now that the two queries are split, we can add a new filter method to the elaborator, filter_only_self(), which can be used in instances that we need only the supertrait predicates, such as during the elaboration used in closure signature deduction. This toggles the usage of super_predicates_of instead of implied_predicates_of during elaboration of a trait predicate.

This supersedes #104745, and fixes the four independent bugs identified in that PR.
Fixes #104719
Fixes #106238
Fixes #110023
Fixes #109514

r? types

@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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 2, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 2, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@jackh726
Copy link
Member

jackh726 commented Feb 3, 2023

I like this. I'll try to review it this weekend if @oli-obk doesn't beat me to it.

Thanks for putting up with my (likely not that helpful) comment of "seems like a there's a better solution".

(I still think there's probably some way to not have to filter Self predicates in a bunch of places, but I think the improvement here is enough to just land this.)

@compiler-errors
Copy link
Member Author

(I still think there's probably some way to not have to filter Self predicates in a bunch of places, but I think the improvement here is enough to just land this.)

I'd be happy to hear what you have in mind, but I'm not sure if I fully understand -- the filtering has to happen somewhere, it's just that I'm able to move this filtering "into" these two queries, instead of having to do it after elaboration.

Anyways, this also probably needs a perf run.

@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 Feb 3, 2023
@bors
Copy link
Contributor

bors commented Feb 3, 2023

⌛ Trying commit af7d9b076bbbd92e19184af024a637826afe9201 with merge 90322d178945ecd686f6c925d195723e108d67dd...

@jackh726
Copy link
Member

jackh726 commented Feb 3, 2023

I'd be happy to hear what you have in mind, but I'm not sure if I fully understand -- the filtering has to happen somewhere, it's just that I'm able to move this filtering "into" these two queries, instead of having to do it after elaboration.

idk, maybe two different Elaborator traits? Or two different interfaces to that?

@bors
Copy link
Contributor

bors commented Feb 3, 2023

☀️ Try build successful - checks-actions
Build commit: 90322d178945ecd686f6c925d195723e108d67dd (90322d178945ecd686f6c925d195723e108d67dd)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (90322d178945ecd686f6c925d195723e108d67dd): comparison URL.

Overall result: no relevant changes - no 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.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

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

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.8% [2.4%, 3.2%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 3, 2023
@lcnr
Copy link
Contributor

lcnr commented Feb 3, 2023

another alternative would be to rip out trait aliases entirely? 😁

Making a PR and making an lang FCP to unaccept the RFC

@pnkfelix
Copy link
Member

pnkfelix commented Mar 2, 2023

@rustbot label: -T-compiler +T-types

@rustbot rustbot added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 2, 2023
@compiler-errors
Copy link
Member Author

@rustbot author

Waiting to answer the question about "do we want trait aliases at all"

@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 Mar 2, 2023
@rust-cloud-vms rust-cloud-vms bot force-pushed the allow-elaborator-to-filter-only-super-traits branch from af7d9b0 to 9ea18d7 Compare April 6, 2023 23:55
@bors
Copy link
Contributor

bors commented Apr 12, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 12, 2023
@compiler-errors
Copy link
Member Author

---- [ui] tests/ui/unique/expr-block-generic-unique1.rs stdout ----

error: test run failed!
status: exit status: 101
command: RUST_TEST_THREADS="16" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "run" "0" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/unique/expr-block-generic-unique1/a"
--- stdout -------------------------------
uploaded "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/unique/expr-block-generic-unique1/a", waiting for result
------------------------------------------
--- stderr -------------------------------
thread 'main' panicked at 'client.read_exact(&mut header) failed with Connection reset by peer (os error 104)', src/tools/remote-test-client/src/main.rs:310:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
------------------------------------------

@bors retry

@bors 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 Apr 12, 2023
@bors
Copy link
Contributor

bors commented Apr 12, 2023

⌛ Testing commit 7ec72ef with merge 9be9b5e...

@bors
Copy link
Contributor

bors commented Apr 12, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 9be9b5e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 12, 2023
@bors bors merged commit 9be9b5e into rust-lang:master Apr 12, 2023
@rustbot rustbot added this to the 1.70.0 milestone Apr 12, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9be9b5e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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.

mean range count
Regressions ❌
(primary)
1.0% [0.4%, 3.5%] 7
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-0.4%, -0.4%] 1
Improvements ✅
(secondary)
-3.7% [-3.7%, -3.7%] 1
All ❌✅ (primary) 0.8% [-0.4%, 3.5%] 8

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.

mean range count
Regressions ❌
(primary)
0.5% [0.5%, 0.6%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [-0.5%, 0.6%] 3

@rust-log-analyzer
Copy link
Collaborator

The job armhf-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
---- [ui] tests/ui/unique/expr-block-generic-unique1.rs stdout ----

error: test run failed!
status: exit status: 101
command: RUST_TEST_THREADS="16" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "run" "0" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/unique/expr-block-generic-unique1/a"
uploaded "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/unique/expr-block-generic-unique1/a", waiting for result
------------------------------------------
--- stderr -------------------------------
thread 'main' panicked at 'client.read_exact(&mut header) failed with Connection reset by peer (os error 104)', src/tools/remote-test-client/src/main.rs:310:9

@compiler-errors compiler-errors deleted the allow-elaborator-to-filter-only-super-traits branch August 11, 2023 19:54
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2024
Ignore own item bounds in parent alias types in `for_each_item_bound`

Fixes rust-lang#120912

I want to get a vibe check on this approach, which is very obviously a hack, but I believe something that is forwards-compatible with a more thorough solution and "good enough for now".

The problem here is that for a really deep rigid associated type, we are now repeatedly considering unrelated item bounds from the parent alias types, meaning we're doing a *lot* of extra work in the MIR inliner for deeply substituted rigid projections.

This feels intimately related to rust-lang#107614. In that PR, we split *supertrait* bounds (bound which share the same `Self` type as the predicate which is being elaborated) and *implied* bounds (anything that is implied by elaborating the predicate).

The problem here is related to the fact that we don't maintain the split between these two for `item_bounds`. If we did, then when recursing into a parent alias type, we'd want to consider only the bounds that are given by [`PredicateFilter::All`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir_analysis/astconv/enum.PredicateFilter.html#variant.SelfOnly) **except** those given by [`PredicateFilter::SelfOnly`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir_analysis/astconv/enum.PredicateFilter.html#variant.SelfOnly).
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 14, 2024
Ignore own item bounds in parent alias types in `for_each_item_bound`

Fixes rust-lang#120912

I want to get a vibe check on this approach, which is very obviously a hack, but I believe something that is forwards-compatible with a more thorough solution and "good enough for now".

The problem here is that for a really deep rigid associated type, we are now repeatedly considering unrelated item bounds from the parent alias types, meaning we're doing a *lot* of extra work in the MIR inliner for deeply substituted rigid projections.

This feels intimately related to rust-lang#107614. In that PR, we split *supertrait* bounds (bound which share the same `Self` type as the predicate which is being elaborated) and *implied* bounds (anything that is implied by elaborating the predicate).

The problem here is related to the fact that we don't maintain the split between these two for `item_bounds`. If we did, then when recursing into a parent alias type, we'd want to consider only the bounds that are given by [`PredicateFilter::All`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir_analysis/astconv/enum.PredicateFilter.html#variant.SelfOnly) **except** those given by [`PredicateFilter::SelfOnly`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir_analysis/astconv/enum.PredicateFilter.html#variant.SelfOnly).
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2024
…oli-obk

Split an item bounds and an item's super predicates

This is the moral equivalent of rust-lang#107614, but instead for predicates this applies to **item bounds**. This PR splits out the item bounds (i.e. *all* predicates that are assumed to hold for the alias) from the item *super predicates*, which are the subset of item bounds which share the same self type as the alias.

## Why?

Much like rust-lang#107614, there are places in the compiler where we *only* care about super-predicates, and considering predicates that possibly don't have anything to do with the alias is problematic. This includes things like closure signature inference (which is at its core searching for `Self: Fn(..)` style bounds), but also lints like `#[must_use]`, error reporting for aliases, computing type outlives predicates.

Even in cases where considering all of the `item_bounds` doesn't lead to bugs, unnecessarily considering irrelevant bounds does lead to a regression (rust-lang#121121) due to doing extra work in the solver.

## Example 1 - Trait Aliases

This is best explored via an example:

```
type TAIT<T> = impl TraitAlias<T>;

trait TraitAlias<T> = A + B where T: C;
```

The item bounds list for `Tait<T>` will include:
* `Tait<T>: A`
* `Tait<T>: B`
* `T: C`

While `item_super_predicates` query will include just the first two predicates.

Side-note: You may wonder why `T: C` is included in the item bounds for `TAIT`? This is because when we elaborate `TraitAlias<T>`, we will also elaborate all the predicates on the trait.

## Example 2 - Associated Type Bounds

```
type TAIT<T> = impl Iterator<Item: A>;
```

The `item_bounds` list for `TAIT<T>` will include:
* `Tait<T>: Iterator`
* `<Tait<T> as Iterator>::Item: A`

But the `item_super_predicates` will just include the first bound, since that's the only bound that is relevant to the *alias* itself.

## So what

This leads to some diagnostics duplication just like rust-lang#107614, but none of it will be user-facing. We only see it in the UI test suite because we explicitly disable diagnostic deduplication.

Regarding naming, I went with `super_predicates` kind of arbitrarily; this can easily be changed, but I'd consider better names as long as we don't block this PR in perpetuity.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2024
…cates-always, r=<try>

Encode implied predicates for traits

In rust-lang#112629, we decided to make associated type bounds in the "supertrait" AST position *implied* even though they're not supertraits themselves.

This means that the `super_predicates` and `implied_predicates` queries now differ for regular traits. The assumption that they didn't differ was hard-coded in rust-lang#107614, so in cross-crate positions this means that we forget the implied predicates from associated type bounds.

This isn't unsound, just kind of annoying. This should be backported since associated type bounds are slated to stabilize for 1.78 -- either that, or associated type bounds can be reverted on beta and re-shipped in 1.79 with this patch.

Fixes rust-lang#122859
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2024
…cates-always, r=oli-obk

Encode implied predicates for traits

In rust-lang#112629, we decided to make associated type bounds in the "supertrait" AST position *implied* even though they're not supertraits themselves.

This means that the `super_predicates` and `implied_predicates` queries now differ for regular traits. The assumption that they didn't differ was hard-coded in rust-lang#107614, so in cross-crate positions this means that we forget the implied predicates from associated type bounds.

This isn't unsound, just kind of annoying. This should be backported since associated type bounds are slated to stabilize for 1.78 -- either that, or associated type bounds can be reverted on beta and re-shipped in 1.79 with this patch.

Fixes rust-lang#122859
RenjiSann pushed a commit to RenjiSann/rust that referenced this pull request Mar 25, 2024
…cates-always, r=oli-obk

Encode implied predicates for traits

In rust-lang#112629, we decided to make associated type bounds in the "supertrait" AST position *implied* even though they're not supertraits themselves.

This means that the `super_predicates` and `implied_predicates` queries now differ for regular traits. The assumption that they didn't differ was hard-coded in rust-lang#107614, so in cross-crate positions this means that we forget the implied predicates from associated type bounds.

This isn't unsound, just kind of annoying. This should be backported since associated type bounds are slated to stabilize for 1.78 -- either that, or associated type bounds can be reverted on beta and re-shipped in 1.79 with this patch.

Fixes rust-lang#122859
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 4, 2024
…oli-obk

Split an item bounds and an item's super predicates

This is the moral equivalent of rust-lang#107614, but instead for predicates this applies to **item bounds**. This PR splits out the item bounds (i.e. *all* predicates that are assumed to hold for the alias) from the item *super predicates*, which are the subset of item bounds which share the same self type as the alias.

## Why?

Much like rust-lang#107614, there are places in the compiler where we *only* care about super-predicates, and considering predicates that possibly don't have anything to do with the alias is problematic. This includes things like closure signature inference (which is at its core searching for `Self: Fn(..)` style bounds), but also lints like `#[must_use]`, error reporting for aliases, computing type outlives predicates.

Even in cases where considering all of the `item_bounds` doesn't lead to bugs, unnecessarily considering irrelevant bounds does lead to a regression (rust-lang#121121) due to doing extra work in the solver.

## Example 1 - Trait Aliases

This is best explored via an example:

```
type TAIT<T> = impl TraitAlias<T>;

trait TraitAlias<T> = A + B where T: C;
```

The item bounds list for `Tait<T>` will include:
* `Tait<T>: A`
* `Tait<T>: B`
* `T: C`

While `item_super_predicates` query will include just the first two predicates.

Side-note: You may wonder why `T: C` is included in the item bounds for `TAIT`? This is because when we elaborate `TraitAlias<T>`, we will also elaborate all the predicates on the trait.

## Example 2 - Associated Type Bounds

```
type TAIT<T> = impl Iterator<Item: A>;
```

The `item_bounds` list for `TAIT<T>` will include:
* `Tait<T>: Iterator`
* `<Tait<T> as Iterator>::Item: A`

But the `item_super_predicates` will just include the first bound, since that's the only bound that is relevant to the *alias* itself.

## So what

This leads to some diagnostics duplication just like rust-lang#107614, but none of it will be user-facing. We only see it in the UI test suite because we explicitly disable diagnostic deduplication.

Regarding naming, I went with `super_predicates` kind of arbitrarily; this can easily be changed, but I'd consider better names as long as we don't block this PR in perpetuity.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 21, 2024
…e1-dead,fmease

Assert that `explicit_super_predicates_of` and `explicit_item_super_predicates` truly only contains bounds for the type itself

We distinguish _implied_ predicates (anything that is implied from elaborating a trait bound) from _super_ predicates, which are are the subset of implied predicates that share the same self type as the trait predicate we're elaborating. This was originally done in rust-lang#107614, which fixed a large class of ICEs and strange errors where the compiler expected the self type of a trait predicate not to change when elaborating super predicates.

Specifically, super predicates are special for various reasons: they're the valid candidates for trait upcasting, are the only predicates we elaborate when doing closure signature inference, etc. So making sure that we get this list correct and don't accidentally "leak" any other predicates into this list is quite important.

This PR adds some debug assertions that we're in fact not doing so, and it fixes an oversight in the effect desugaring rework.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Sep 21, 2024
…e1-dead,fmease

Assert that `explicit_super_predicates_of` and `explicit_item_super_predicates` truly only contains bounds for the type itself

We distinguish _implied_ predicates (anything that is implied from elaborating a trait bound) from _super_ predicates, which are are the subset of implied predicates that share the same self type as the trait predicate we're elaborating. This was originally done in rust-lang#107614, which fixed a large class of ICEs and strange errors where the compiler expected the self type of a trait predicate not to change when elaborating super predicates.

Specifically, super predicates are special for various reasons: they're the valid candidates for trait upcasting, are the only predicates we elaborate when doing closure signature inference, etc. So making sure that we get this list correct and don't accidentally "leak" any other predicates into this list is quite important.

This PR adds some debug assertions that we're in fact not doing so, and it fixes an oversight in the effect desugaring rework.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 22, 2024
Rollup merge of rust-lang#130666 - compiler-errors:super-bounds, r=fee1-dead,fmease

Assert that `explicit_super_predicates_of` and `explicit_item_super_predicates` truly only contains bounds for the type itself

We distinguish _implied_ predicates (anything that is implied from elaborating a trait bound) from _super_ predicates, which are are the subset of implied predicates that share the same self type as the trait predicate we're elaborating. This was originally done in rust-lang#107614, which fixed a large class of ICEs and strange errors where the compiler expected the self type of a trait predicate not to change when elaborating super predicates.

Specifically, super predicates are special for various reasons: they're the valid candidates for trait upcasting, are the only predicates we elaborate when doing closure signature inference, etc. So making sure that we get this list correct and don't accidentally "leak" any other predicates into this list is quite important.

This PR adds some debug assertions that we're in fact not doing so, and it fixes an oversight in the effect desugaring rework.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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-types Relevant to the types team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
9 participants