Don't try to prove opaque type bounds twice on the same types

[oli-obk]

fixes #109268

The other alternative I came up with was to filter obligations out by trying to prove them on their own first. Basically <Foo<'_> as Fn() -> Foo<'_>>::Output = Foo<'_> should always hold without needing to reveal the opaque type or without trying to register a hidden type for it.

r? @compiler-errors cc @lcnr

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[compiler-errors]

how does this behave on the new trait solver? I think the new trait solver does re-prove bounds even if it's unifying with an existing opaque:

rust/compiler/rustc_trait_selection/src/solve/eval_ctxt.rs

Lines 896 to 902 in bd9785c

	ecx.eq(param_env, candidate_ty, ty)?;
	ecx.add_item_bounds_for_hidden_type(
	candidate_key.def_id.to_def_id(),
	candidate_key.args,
	param_env,
	candidate_ty,
	);

[oli-obk]

it works (see new commit), but I haven't run any logs yet to see what it is doing, and whether anything is different.

[compiler-errors]

Wonder why it's not cycling. I bet it's because of lazy norm. Anyways, I guess it shouldn't matter, unless we make sure we don't commit to something that breaks the new solver.

[lcnr]

I don't like this approach but can't quite put into words why 😅 want to look into this in more detail later this/next week

[oli-obk]

I don't like this approach but can't quite put into words why

Well, I don't like it because it makes the order of hidden type registrations meaningful.

I looked into the original proposal you had, but it didn't look like we could actually do that without adding special casing for opaque types $somewhere.

[oli-obk]

@bors try @rust-timer queue

[bors]

⌛ Trying commit ab01463 with merge 3c4e2f5ff62c4193137fa44aa1dc9373aca1f4cc...

[bors]

☀️ Try build successful - checks-actions
Build commit: 3c4e2f5ff62c4193137fa44aa1dc9373aca1f4cc (3c4e2f5ff62c4193137fa44aa1dc9373aca1f4cc)

[rust-timer]

Finished benchmarking commit (3c4e2f5ff62c4193137fa44aa1dc9373aca1f4cc): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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)	5.5%	[5.5%, 5.5%]	1
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.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-	-	0

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

Binary size

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

Bootstrap: 651.412s -> 650.446s (-0.15%)

[lcnr]

Finally getting to this 😅 sorry for taking so long.

As you said, this makes stuff order dependent and I don't think there's a theoretical reason for why this is correct. I don't want to have this behavior in the new solver so this is a chance for the old and new solver to diverge in behavior which is very :/

I personally don't think #109268 should block the stabilization of TAIT, so for me it would be acceptable to not fix this in the old solver at all.

I tried to instead only replace opaques with infer vars if can_eq would otherwise fail: master...lcnr:rust:project-opaques. This doesn't seem to work because we don't normalize the opaque when checking its trait bounds. The hack is necessary to constrain the projection_ty infer var to the hidden type instead of the opaque :<

I also can't think of how we'd fix this except by adding more lazy norm for opaques. This is difficult as long as we have Bubble 😁

[oli-obk]

sgtm. I marked the issue as "can do after stabilization"