Split an item bounds and an item's super predicates #121123

compiler-errors · 2024-02-15T01:59:15Z

This is the moral equivalent of #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 #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 (#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 #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.

compiler-errors · 2024-02-15T01:59:22Z

@bors try @rust-timer queue

bors · 2024-02-15T02:00:32Z

⌛ Trying commit 4146340 with merge 224e8f5...

…<try> Split an item bounds and an item's own assumptions uwu r? `@ghost`

bors · 2024-02-15T03:27:32Z

☀️ Try build successful - checks-actions
Build commit: 224e8f5 (224e8f50ad87cfc891a68119c9962e1a49d3e67b)

rust-timer · 2024-02-15T06:43:11Z

Finished benchmarking commit (224e8f5): comparison URL.

Overall result: ❌ regressions - 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 is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.3%	[1.2%, 1.4%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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.2%	[0.0%, 0.9%]	84
Regressions ❌ (secondary)	1.0%	[0.1%, 1.9%]	24
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.0%, 0.9%]	84

Bootstrap: 636.575s -> 635.79s (-0.12%)
Artifact size: 306.16 MiB -> 306.22 MiB (0.02%)

bors · 2024-03-06T02:33:06Z

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

rustbot · 2024-03-20T17:02:01Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler-errors · 2024-03-20T17:02:02Z

Ok, I think this is ready to go.

compiler-errors · 2024-03-20T17:02:36Z

r? types

Does anyone want to review this? Like @lcnr or @jackh726 or @oli-obk :D

compiler-errors

See first commit for actual changes, second commit should just be blessing duplicated error messages due to the fact that we call astconv on the HIR in two cases rather than one.

compiler-errors · 2024-03-20T17:04:02Z

tests/ui/associated-type-bounds/dont-imply-atb-in-closure-inference.rs

@@ -0,0 +1,21 @@
+//@ check-pass


This is one of two user-facing consequences of this change, but I don't expect this to be replicatable with RPIT on stable (at least not easily, e.g. w/o a recursive call) since we eagerly replace opaques with infer vars in the old solver.

compiler-errors · 2024-03-20T17:06:11Z

tests/ui/lint/unused/unused-associated-item.rs

@@ -0,0 +1,21 @@
+//@ check-pass


This is a user-facing consequence of this change and how it interacts w/ associated type bounds.

Previously the must_use implementation used item_bounds, which means we see the Future trait in the associated item bound of impl Factory<Output: Future> even if it's not bounding the RPIT itself.

oli-obk · 2024-03-21T05:42:38Z

we call astconv on the HIR in two cases rather than one.

annoying, but also not too important. May be worth exploring whether filtering item_bounds can be done without significant code duplication.

That the bounds stress test regresses comes to no surprise, but also isn't going to be relevant in practice. There is no exponential work happening here that could cause real world extreme bound users to get a perf regression.

@bors r+ rollup=never

bors · 2024-03-21T05:42:41Z

📌 Commit ce5f8c9 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2024-03-21T06:12:28Z

⌛ Testing commit ce5f8c9 with merge 47dd709...

bors · 2024-03-21T08:13:15Z

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

rust-timer · 2024-03-21T09:26:43Z

Finished benchmarking commit (47dd709): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.0%	[0.9%, 1.0%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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

Cycles

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

Binary size

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.2%	[0.0%, 1.0%]	91
Regressions ❌ (secondary)	0.9%	[0.0%, 1.9%]	27
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.0%, 1.0%]	91

Bootstrap: 669.528s -> 669.21s (-0.05%)
Artifact size: 312.72 MiB -> 312.83 MiB (0.03%)

…und-tests, r=lcnr Add tests for shortcomings of associated type bounds Adds the test in rust-lang#122791 (comment) Turns out that rust-lang#121123 is what breaks `tests/ui/associated-type-bounds/cant-see-copy-bound-from-child-rigid.rs` (passes on nightly), but given that associated type bounds haven't landed anywhere yet, I'm happy with breaking it. This is unrelated to rust-lang#122791, which just needed that original commit e6b64c6 stacked on top of it so that it wouldn't have tests failing. r? lcnr

Rollup merge of rust-lang#122826 - compiler-errors:associated-type-bound-tests, r=lcnr Add tests for shortcomings of associated type bounds Adds the test in rust-lang#122791 (comment) Turns out that rust-lang#121123 is what breaks `tests/ui/associated-type-bounds/cant-see-copy-bound-from-child-rigid.rs` (passes on nightly), but given that associated type bounds haven't landed anywhere yet, I'm happy with breaking it. This is unrelated to rust-lang#122791, which just needed that original commit e6b64c6 stacked on top of it so that it wouldn't have tests failing. r? lcnr

…ays, r=lcnr Make inductive cycles always ambiguous This makes inductive cycles always result in ambiguity rather than be treated like a stack-dependent error. This has some interactions with specialization, and so breaks a few UI tests that I don't agree should've ever worked in the first place, and also breaks a handful of crates in a way that I don't believe is a problem. On the bright side, it puts us in a better spot when it comes to eventually enabling coinduction everywhere. ## Results This was cratered in rust-lang#116494 (comment), which boils down to two regressions: * `lu_packets` - This code should have never compiled in the first place. More below. * **ALL** other regressions are due to `[email protected]` (edit: and `[email protected]`) - This actually seems to be fixed in version `0.11.0-beta.5`, which is the *most* up to date version, but it's still prerelease on crates.io so I don't think cargo ends up picking `beta.5` when building dependent crates. ### `lu_packets` Firstly, this crate uses specialization, so I think it's automatically worth breaking. However, I've minimized [the regression](https://crater-reports.s3.amazonaws.com/pr-116494-3/try%23d614ed876e31a5f3ad1d0fbf848fcdab3a29d1d8/gh/lcdr.lu_packets/log.txt) to: ```rust // Upstream crate pub trait Serialize {} impl Serialize for &() {} impl<S> Serialize for &[S] where for<'a> &'a S: Serialize {} // ----------------------------------------------------------------------- // // Downstream crate #![feature(specialization)] #![allow(incomplete_features, unused)] use upstream::Serialize; trait Replica { fn serialize(); } impl<T> Replica for T { default fn serialize() {} } impl<T> Replica for Option<T> where for<'a> &'a T: Serialize, { fn serialize() {} } ``` Specifically this fails when computing the specialization graph for the `downstream` crate. The code ends up cycling on `&[?0]: Serialize` when we equate `&?0 = &[?1]` during impl matching, which ends up needing to prove `&[?1]: Serialize`, which since cycles are treated like ambiguity, ends up in a **fatal overflow**. For some reason this requires two crates, squashing them into one crate doesn't work. Side-note: This code is subtly order dependent. When minimizing, I ended up having the code start failing on `nightly` very easily after removing and reordering impls. This seems to me all the more reason to remove this behavior altogether. ## Side-note: Item Bounds (edit: this was fixed independently in rust-lang#121123) Due to the changes in rust-lang#120584 where we now consider an alias's item bounds *and* all the item bounds of the alias's nested self type aliases, I've had to add e6b64c6 which is a hack to make sure we're not eagerly normalizing bounds that have nothing to do with the predicate we're trying to solve, and which result in. This is fixed in a more principled way in rust-lang#121123. --- r? lcnr for an initial review

…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.

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 Feb 15, 2024

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 Feb 15, 2024

compiler-errors mentioned this pull request Feb 15, 2024

Test polymorphic_recursion.rs takes a long time to compile with a debug-assertions=true compiler #121121

Open

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 15, 2024

Auto merge of rust-lang#121123 - compiler-errors:item-assumptions, r=…

224e8f5

…<try> Split an item bounds and an item's own assumptions uwu r? `@ghost`

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 Feb 15, 2024

rust-cloud-vms bot force-pushed the item-assumptions branch from 4146340 to 2ab927e Compare March 2, 2024 19:42

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the item-assumptions branch 2 times, most recently from 5b1ceaf to 359084c Compare March 20, 2024 16:53

compiler-errors added 2 commits March 20, 2024 13:00

Split item bounds and item super predicates

aa39dbb

Bless test fallout (duplicate diagnostics)

ce5f8c9

compiler-errors force-pushed the item-assumptions branch from 359084c to ce5f8c9 Compare March 20, 2024 17:01

compiler-errors marked this pull request as ready for review March 20, 2024 17:01

rustbot added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Mar 20, 2024

rustbot assigned oli-obk Mar 20, 2024

compiler-errors commented Mar 20, 2024

View reviewed changes

compiler-errors changed the title ~~Split an item bounds and an item's own assumptions~~ Split an item bounds and an item's super predicates Mar 20, 2024

compiler-errors mentioned this pull request Mar 20, 2024

Make inductive cycles always ambiguous #122791

Merged

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 Mar 21, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 21, 2024

bors merged commit 47dd709 into rust-lang:master Mar 21, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Mar 21, 2024

This was referenced Mar 21, 2024

[MCP 723] Rename astconv::AstConv and related items #120926

Merged

Collect relevant item bounds from trait clauses for nested rigid projections #120752

Merged

Suggest using type args directly instead of equality constraint #122591

Merged

compiler-errors deleted the item-assumptions branch March 21, 2024 13:57

compiler-errors mentioned this pull request Mar 21, 2024

Add tests for shortcomings of associated type bounds #122826

Merged

compiler-errors mentioned this pull request Apr 4, 2024

circular set of associated types cause cargo build to hang #123448

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split an item bounds and an item's super predicates #121123

Split an item bounds and an item's super predicates #121123

compiler-errors commented Feb 15, 2024 •

edited

Loading

compiler-errors commented Feb 15, 2024

This comment has been minimized.

bors commented Feb 15, 2024

This comment has been minimized.

bors commented Feb 15, 2024

This comment has been minimized.

rust-timer commented Feb 15, 2024

This comment has been minimized.

bors commented Mar 6, 2024

rustbot commented Mar 20, 2024

compiler-errors commented Mar 20, 2024

compiler-errors commented Mar 20, 2024

compiler-errors left a comment

compiler-errors Mar 20, 2024

compiler-errors Mar 20, 2024

oli-obk commented Mar 21, 2024

bors commented Mar 21, 2024

bors commented Mar 21, 2024

bors commented Mar 21, 2024

rust-timer commented Mar 21, 2024

Split an item bounds and an item's super predicates #121123

Split an item bounds and an item's super predicates #121123

Conversation

compiler-errors commented Feb 15, 2024 • edited Loading

Why?

Example 1 - Trait Aliases

Example 2 - Associated Type Bounds

So what

compiler-errors commented Feb 15, 2024

This comment has been minimized.

bors commented Feb 15, 2024

This comment has been minimized.

bors commented Feb 15, 2024

This comment has been minimized.

rust-timer commented Feb 15, 2024

Overall result: ❌ regressions - no action needed

This comment has been minimized.

bors commented Mar 6, 2024

rustbot commented Mar 20, 2024

compiler-errors commented Mar 20, 2024

compiler-errors commented Mar 20, 2024

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Mar 21, 2024

bors commented Mar 21, 2024

bors commented Mar 21, 2024

bors commented Mar 21, 2024

rust-timer commented Mar 21, 2024

Overall result: ❌ regressions - no action needed

compiler-errors commented Feb 15, 2024 •

edited

Loading