Only check outlives goals on impl compared to trait

[jackh726]

Fixes #108544

r? @compiler-errors

[jackh726]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 7760182c6632aee71f02847169d13607b4b19b64 with merge 6979a2ddb0b3594e3bb078e1b252b5959c54be65...

[compiler-errors]

approach looks fine to me, one nit. let's see what perf says.

[bors]

☀️ Try build successful - checks-actions
Build commit: 6979a2ddb0b3594e3bb078e1b252b5959c54be65 (6979a2ddb0b3594e3bb078e1b252b5959c54be65)

[rust-timer]

Finished benchmarking commit (6979a2ddb0b3594e3bb078e1b252b5959c54be65): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.2%	[1.2%, 1.2%]	1
Improvements ✅ (primary)	-0.9%	[-1.4%, -0.3%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-1.4%, -0.3%]	2

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.9%	[0.4%, 1.1%]	3
Regressions ❌ (secondary)	3.9%	[3.9%, 3.9%]	1
Improvements ✅ (primary)	-1.1%	[-2.5%, -0.4%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-2.5%, 1.1%]	6

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.6%	[0.5%, 0.7%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.6%, -0.4%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.6%, 0.7%]	6

[jackh726]

Had to add FxHashSet because of infinite cycle. Let's recheck perf.

@bors try @rust-timer queue

[bors]

⌛ Trying commit eadcd271de548dc676c928e1562021d281ed9584 with merge 6df34432f4d6a6382a2434b3976482ce6e7fb836...

[bors]

☀️ Try build successful - checks-actions
Build commit: 6df34432f4d6a6382a2434b3976482ce6e7fb836 (6df34432f4d6a6382a2434b3976482ce6e7fb836)

[rust-timer]

Finished benchmarking commit (6df34432f4d6a6382a2434b3976482ce6e7fb836): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.4%	[0.4%, 0.4%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.2%, -0.9%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-1.2%, 0.4%]	4

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.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.2%, -0.8%]	2
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-0.4%	[-1.2%, 0.7%]	3

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

[compiler-errors]

I think this is noise -- anyways, this is also for correctness.

@bors r+

[bors]

📌 Commit eadcd271de548dc676c928e1562021d281ed9584 has been approved by compiler-errors

It is now in the queue for this repository.

[lcnr]

this should be unsound?

for some argument to be well formed, we could have a trait obligation with a nested outlives constraint which only holds because of the implied bounds of the method?

i could imagine only checking the outlives bounds returned by https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.implied_outlives_bounds, but even that feels dangerous.

I don't think we should keep an unsoundness to patch a deficiency of the current trait solver, especially if that deficiency can be solved in a different way.

Can we instead strip trivial clauses from the trait method param env when substituting? That should solve this issue without adding any soundness concerns? 🤔

[lcnr]

opened #109491 to experiment with that approach

[jackh726]

I don't think this is unsound (though, arguably, imperfect).

We check that everything is well formed elsewhere, so there's no unsoundness related to ill-formed types.

The only implied bounds we currently have are those involving regions (or super trait bounds, but those should also be checked elsewhere).

Therefore, we don't necessarily have to care about non-region predicates for this lint.

That being said, while in theory, skipping "trivial clauses" is arguably a "nicer" solution, the implementation in #109491 is not simple. And I think that approach may also have issues when the param env predicates introduce ambiguity that doesn't exist in an empty environment. Though, I'm not sure if this would be caught by other checks.

[lcnr]

I don't think this is unsound (though, arguably, imperfect).

We check that everything is well formed elsewhere, so there's no unsoundness related to ill-formed types.

The only implied bounds we currently have are those involving regions (or super trait bounds, but those should also be checked elsewhere).

Therefore, we don't necessarily have to care about non-region predicates for this lint.

We do care about them if these other predicates end up causing us to add additional outlives predicates. Considering that we end up computing Projection obligations inside of compute_implied_outlives_bounds, this can be the case.

rust/compiler/rustc_traits/src/implied_outlives_bounds.rs

Lines 86 to 94 in 8a7ca93

	if obligation.predicate.has_non_region_infer() {
	match obligation.predicate.kind().skip_binder() {
	ty::PredicateKind::Clause(ty::Clause::Projection(..))
	| ty::PredicateKind::AliasRelate(..) => {
	ocx.register_obligation(obligation.clone());
	}
	_ => {}
	}
	}

That being said, while in theory, skipping "trivial clauses" is arguably a "nicer" solution, the implementation in #109491 is not simple.

I disagree, the implementation in #109491 is (after enabling the use of the canonical solver mode to get non-fatal overflow) simple. We're checking whether a goal holds in an empty environment. While this needs more lines of code, the behavior of these is clear and also relied on in other areas of the compiler. That approach is also far more obvious to be sound as we don't drop requirements, we only drop assumptions.

And I think that approach may also have issues when the param env predicates introduce ambiguity that doesn't exist in an empty environment. Though, I'm not sure if this would be caught by other checks.

You mean that the approach in #109491 does not completely fix the issue of incorrect ambiguity/selecting the wrong candidate when proving the wf bounds? That is true and something that can be incrementally improved if people encounter that issue .

I very strongly prefer trivially sound (given that the rest of the trait solver is sound) over maybe sound but may accept slightly more code.

I've spend a few hours looking at the code and experimenting with some tests to convince myself that your approach is sound (or to get a counter example) and wasn't able to do either of these. Apparently removing the ocx.register_obligation doesn't actually break any existing tests but an assertion that the obligation has no inference variables does fail. cc @aliemjay as you may know whether that register_obligation is currently still needed.

[aliemjay]

We do care about them if these other predicates end up causing us to add additional outlives predicates. Considering that we end up computing Projection obligations inside of compute_implied_outlives_bounds, this can be the case.

In implied_outlives_bounds query, ocx.register_obligations will add additional predicates as requirements not assumptions. i.e. the additional outlives predicates will end up in QueryResponse::region_constraints rather than QueryResponse::value.

Apparently removing the ocx.register_obligation doesn't actually break any existing tests but an assertion that the obligation has no inference variables does fail. cc @aliemjay as you may know whether that register_obligation is currently still needed.

After fiddling with it for a while I managed to come up with a test:

pub trait Iter {
    type Item;
}

impl<X, I> Iter for I
where
    I: IntoIterator<Item = X>,
{
    type Item = X;
}

pub struct Map<I>(I)
where
    I: Iter,
    I::Item: 'static;

fn test<X>(_: Map<Vec<X>>) {}

and filed an issue #109799 for why the condition predicate.has_non_region_infer() is unnecessary.

[lcnr]

Extending the approach in #109491 to deal with generic bounds ended up causing weird trait solver errors so I changed my opinion here. I think that - at least with the current solver - this PR is the better choice.

Please add the following as a test.

// check-pass

// Similar to issue-108544.rs except that we have a generic `T` which
// previously caused an overeager fast-path to trigger.
use std::borrow::Cow;

pub trait Trait<T: Clone> {
    fn method(self) -> Option<Cow<'static, T>>
    where
        Self: Sized;
}

impl<'a, T: Clone> Trait<T> for Cow<'a, T> {
    fn method(self) -> Option<Cow<'static, T>> {
        None
    }
}

fn main() {}

[lcnr]

also please add a fixme to look into reverting this once the new solver is stable

Suggested change

	// region outlives obligations.
	// region outlives obligations.
	//
	// FIXME(-Ztrait-solver=next): Try removing this hack again once
	// the new solver is stable.

[lcnr]

@bors r- for now because of the above comment

[compiler-errors]

@jackh726: Are you planning on rebasing this? Ideally we'd bump deny implied_bounds_entailment to a hard error, but I think it's blocked on this.

(cc #105572 for tracking)

[jackh726]

Ah yes. Completely forgot about this

[jackh726]

@bors r=lcnr

[bors]

📌 Commit 3028dc4 has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit 3028dc4 with merge f1b8548...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing f1b8548 to master...

[Kobzol]

The result above was caused by an issue in the perf. bot, it doesn't have anything to do with this PR. Sorry for the noise.

@rustbot label: +perf-regression-triaged

[rust-timer]

Finished benchmarking commit (f1b8548): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)	0.4%	[0.4%, 0.4%]	2
Improvements ✅ (primary)	-0.9%	[-1.6%, -0.6%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-1.6%, -0.6%]	6

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

Binary size

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

Bootstrap: 632.009s -> 632.973s (0.15%)

[Skepfyr]

This looks like it's causing ICEs on the latest nightly: #114783

[pnkfelix]

• regression is solely to the opt and debug profiles externs secondary benchmark, by a small amount.
• marked as triaged

@rustbot label: +perf-regression-triaged

(those ICEs from issue #114783 might lead us to revert this though.)