Fix marker trait winnowing depending on impl order

[traviscross]

Story: I was looking through this code for an unrelated reason and happened to notice the duplicate condition. That seemed obviously wrong, especially in light of the comment, so I worked backward to what it affected, wrote a test that failed, and then found that the test matched an existing known-bug test, which pointed me to #109481.

---

The TypeOutlives handler in evaluate_predicate_recursively means to check whether a type in a T: 'a predicate has free regions, bound regions, non-region inference variables, or non-region generic parameters -- and if so, to return EvaluatedToOkModuloRegions rather than EvaluatedToOk. Correspondingly, the comment says "no free lifetimes or generic parameters". But the code was mistakenly checking has_non_region_infer() twice instead of checking both has_non_region_infer() and has_non_region_param().

This meant that TypeOutlives(T, 'static) where T is a type parameter returned EvaluatedToOk -- claiming the result holds unconditionally -- when the correct answer is EvaluatedToOkModuloRegions.

The distinction matters during marker trait winnowing in prefer_lhs_over_victim, which uses must_apply_considering_regions() (true only for EvaluatedToOk) to decide whether one impl beats another when there are multiple candidates. With the bug, a T: 'static-bounded impl appeared equally as strong as an unrestricted impl, making the winner depend on source order. This caused spurious E0310 errors when the more-constrained impl happened to appear after the less-constrained one.

Fixes #109481.

This same symptom was originally filed as #84917 and fixed in PR #88139. Then PR #102472 rewrote the TypeOutlives handler, introducing the duplicate has_non_region_infer() and losing the param check, regressing this. Around this same time, #109481 was filed. It noted the connection to #102472 -- the bug only appeared after it -- but the duplicate condition was not noticed.

r? @lcnr

cc @oli-obk @nikomatsakis

[lcnr]

Good catch. We actually need to check for both.

If we have ?x: 'a, ?x could later be inferred to be T after all

[traviscross]

Good catch. We actually need to check for both.

If we have ?x: 'a, ?x could later be inferred to be T after all

That makes sense to me. On the other hand, I commented out the pred.0.has_non_region_infer() check and all the tests still pass. I timed out in trying to come up with a test that would fail. I think maybe things are being equated and normalized before we get here (but you'd know better on this). Even still, it would seem more correct to me to not lean on that and to check for both.

[lcnr]

I timed out in trying to come up with a test that would fail. I think maybe things are being equated and normalized before we get here (but you'd know better on this). Even still, it would seem more correct to me to not lean on that and to check for both.

We don't even try to prove things if the self type is an inference variable, so you need sth like

#![feature(marker_trait_attr)]
#[marker]
trait Marker<T> {}

impl<T: 'static> Marker<T> for u32 {}
impl<T> Marker<T> for u32 {} 

fn foo<T: Marker<U>, U>() -> U { todo!() }

fn bar<'a>() {
    let x = foo::<u32, _>();
    1i32.abs(); // guarantees we do trait solving before constraining `U`
    let _: *mut &'a () = x;
}

would need to look at the logging in select/mod.rs to see whether this does what u need

[wesleywiser]

Just checking in from compiler team triage, I think this is waiting on a review from @lcnr. Is that right?

[lcnr]

I don't necessarily think so, either me or @traviscross needs to look into my example #153847 (comment) and figure out whether that's works as a test

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[traviscross]

Thanks. I looked into that example. It didn't work as a test of the has_non_region_infer() check (in the TypeOutlives handler), but I added (a variant of) it as an interesting test of the has_non_region_param() side of the disjunction. I did eventually construct something that worked for exercising the has_non_region_infer() side. It's pretty subtle.

@rustbot review

[lcnr]

Thanks! Good job coming up with the tests and sorry for the late review

@bors r+ rollup

[rust-bors]

📌 Commit 9a122c9 has been approved by lcnr

It is now in the queue for this repository.

[JonathanBrouwer]

@rust-timer build b11367c

[rust-timer]

Finished benchmarking commit (b11367c): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

Results (primary 2.3%, secondary 2.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[2.3%, 2.3%]	1

Cycles

Results (secondary -4.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.6%	[-6.3%, -2.9%]	2
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 525.47s -> 517.45s (-1.53%)
Artifact size: 400.80 MiB -> 402.84 MiB (0.51%)