Fix marker trait winnowing depending on impl order by traviscross · Pull Request #153847 · rust-lang/rust

traviscross · 2026-03-14T06:46:31Z

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 · 2026-03-15T08:40:36Z

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 · 2026-03-22T05:43:32Z

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 · 2026-03-30T12:17:25Z

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 · 2026-05-14T14:25:39Z

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

lcnr · 2026-05-14T16:55:28Z

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

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. This fixes Rust issue 109481. See PR 153847. This same symptom was originally filed as issue 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, issue 109481 was filed. It noted the connection to PR 102472 -- that the bug only appeared after it -- but the duplicate condition was not noticed.

It's harder than one would imagine to demonstrate that the `has_non_region_infer()` check in the `TypeOutlives` handler is load bearing (even though the check seems right analytically). Fortunately, we did find a way to show this. Let's add that test. In working that out, we found two other interesting ways of showing that the `has_non_region_param()` check matters. Let's add those too. Though we're concerned here with code in the old solver, we test against both.

rustbot · 2026-05-20T20:35:05Z

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 · 2026-05-20T20:35:37Z

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 · 2026-06-09T12:10:02Z

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

@bors r+ rollup

rust-bors · 2026-06-09T12:10:06Z

📌 Commit 9a122c9 has been approved by lcnr

It is now in the queue for this repository.

@lcnr

…ssing-param-check, r=lcnr Fix marker trait winnowing depending on impl order 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 rust-lang#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 rust-lang#109481. This same symptom was originally filed as rust-lang#84917 and fixed in PR rust-lang#88139. Then PR rust-lang#102472 rewrote the `TypeOutlives` handler, introducing the duplicate `has_non_region_infer()` and losing the param check, regressing this. Around this same time, rust-lang#109481 was filed. It noted the connection to rust-lang#102472 -- the bug only appeared after it -- but the duplicate condition was not noticed. r? @lcnr cc @oli-obk @nikomatsakis

@lcnr

…ssing-param-check, r=lcnr Fix marker trait winnowing depending on impl order 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 rust-lang#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 rust-lang#109481. This same symptom was originally filed as rust-lang#84917 and fixed in PR rust-lang#88139. Then PR rust-lang#102472 rewrote the `TypeOutlives` handler, introducing the duplicate `has_non_region_infer()` and losing the param check, regressing this. Around this same time, rust-lang#109481 was filed. It noted the connection to rust-lang#102472 -- the bug only appeared after it -- but the duplicate condition was not noticed. r? @lcnr cc @oli-obk @nikomatsakis

Rollup of 18 pull requests Successful merges: - #152852 (Remove driver_lint_caps) - #157166 (Change type of async context parameter after state transform.) - #157335 (bootstrap: Handle dotted table keys when parsing bootstrap.toml) - #157503 (Disable `tests/debuginfo/pretty-std.rs` `OsString` cdb check) - #157571 (Remove ProcMacro enum from proc macro ABI) - #148183 (rustdoc: Test & document `test_harness` code block attribute) - #153847 (Fix marker trait winnowing depending on impl order) - #156067 (Fix async drop glue for Box<T>) - #156399 (fix improper ctypes in Znext solver) - #157338 (Make `Literal::byte_character_value` work with bytes as well) - #157410 (Implement rustc_public::CrateDef{,Type} for FieldDef) - #157605 (Arg splat experiment - syntax impl) - #157630 (Add multibyte JSON diagnostic regression test) - #157633 (Reorder `impl` restriction rendering and add bottom margin) - #157642 (Report duplicate relaxed bounds during ast lowering) - #157652 (fix doc for unicode normalization faq on `casefold` APIs) - #157661 (Update to ar_archive_writer v0.5.2) - #157668 (Add test for matches in `rustc_must_match_exhaustively`) Failed merges: - #157670 (Rename `errors.rs` file to `diagnostics.rs` (4/N))

Rollup of 17 pull requests Successful merges: - #157166 (Change type of async context parameter after state transform.) - #157335 (bootstrap: Handle dotted table keys when parsing bootstrap.toml) - #157503 (Disable `tests/debuginfo/pretty-std.rs` `OsString` cdb check) - #157571 (Remove ProcMacro enum from proc macro ABI) - #148183 (rustdoc: Test & document `test_harness` code block attribute) - #153847 (Fix marker trait winnowing depending on impl order) - #156067 (Fix async drop glue for Box<T>) - #156399 (fix improper ctypes in Znext solver) - #157338 (Make `Literal::byte_character_value` work with bytes as well) - #157410 (Implement rustc_public::CrateDef{,Type} for FieldDef) - #157605 (Arg splat experiment - syntax impl) - #157630 (Add multibyte JSON diagnostic regression test) - #157633 (Reorder `impl` restriction rendering and add bottom margin) - #157642 (Report duplicate relaxed bounds during ast lowering) - #157652 (fix doc for unicode normalization faq on `casefold` APIs) - #157661 (Update to ar_archive_writer v0.5.2) - #157668 (Add test for matches in `rustc_must_match_exhaustively`) Failed merges: - #157670 (Rename `errors.rs` file to `diagnostics.rs` (4/N))

@lcnr

Rollup merge of #153847 - traviscross:TC/fix-typeoutlives-missing-param-check, r=lcnr Fix marker trait winnowing depending on impl order 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

Rollup of 17 pull requests Successful merges: - rust-lang/rust#157166 (Change type of async context parameter after state transform.) - rust-lang/rust#157335 (bootstrap: Handle dotted table keys when parsing bootstrap.toml) - rust-lang/rust#157503 (Disable `tests/debuginfo/pretty-std.rs` `OsString` cdb check) - rust-lang/rust#157571 (Remove ProcMacro enum from proc macro ABI) - rust-lang/rust#148183 (rustdoc: Test & document `test_harness` code block attribute) - rust-lang/rust#153847 (Fix marker trait winnowing depending on impl order) - rust-lang/rust#156067 (Fix async drop glue for Box<T>) - rust-lang/rust#156399 (fix improper ctypes in Znext solver) - rust-lang/rust#157338 (Make `Literal::byte_character_value` work with bytes as well) - rust-lang/rust#157410 (Implement rustc_public::CrateDef{,Type} for FieldDef) - rust-lang/rust#157605 (Arg splat experiment - syntax impl) - rust-lang/rust#157630 (Add multibyte JSON diagnostic regression test) - rust-lang/rust#157633 (Reorder `impl` restriction rendering and add bottom margin) - rust-lang/rust#157642 (Report duplicate relaxed bounds during ast lowering) - rust-lang/rust#157652 (fix doc for unicode normalization faq on `casefold` APIs) - rust-lang/rust#157661 (Update to ar_archive_writer v0.5.2) - rust-lang/rust#157668 (Add test for matches in `rustc_must_match_exhaustively`) Failed merges: - rust-lang/rust#157670 (Rename `errors.rs` file to `diagnostics.rs` (4/N))

JonathanBrouwer · 2026-06-13T10:40:28Z

@rust-timer build b11367c

rust-timer · 2026-06-13T11:20:58Z

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%)

rustbot assigned lcnr Mar 14, 2026

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 Mar 14, 2026

This comment was marked as resolved.

Sign in to view

traviscross force-pushed the TC/fix-typeoutlives-missing-param-check branch 5 times, most recently from 411054e to 85b6665 Compare March 14, 2026 07:08

traviscross added T-types Relevant to the types team, which will review and decide on the PR/issue. F-marker_trait_attr `#![feature(marker_trait_attr)]` labels Mar 14, 2026

oli-obk reviewed Mar 14, 2026

View reviewed changes

Comment thread tests/ui/marker_trait_attr/overlapping-impl-1-modulo-regions.rs

traviscross force-pushed the TC/fix-typeoutlives-missing-param-check branch from 85b6665 to 516f56b Compare March 14, 2026 07:47

lcnr 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 May 14, 2026

traviscross force-pushed the TC/fix-typeoutlives-missing-param-check branch from 516f56b to 7259e03 Compare May 20, 2026 20:31

This comment was marked as resolved.

Sign in to view

traviscross added 2 commits May 20, 2026 20:34

traviscross force-pushed the TC/fix-typeoutlives-missing-param-check branch from 7259e03 to 9a122c9 Compare May 20, 2026 20:35

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 20, 2026

rust-bors Bot 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 Jun 9, 2026

GuillaumeGomez mentioned this pull request Jun 9, 2026

Rollup of 15 pull requests #157673

Closed

jhpratt mentioned this pull request Jun 9, 2026

Rollup of 18 pull requests #157679

Closed

jhpratt mentioned this pull request Jun 9, 2026

Rollup of 17 pull requests #157683

Merged

rust-bors Bot merged commit 0be51f4 into rust-lang:main Jun 10, 2026
11 checks passed

rustbot added this to the 1.98.0 milestone Jun 10, 2026

This comment has been minimized.

Sign in to view

Uh oh!

Conversation

traviscross commented Mar 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

lcnr commented Mar 15, 2026

Uh oh!

traviscross commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lcnr commented Mar 30, 2026

Uh oh!

wesleywiser commented May 14, 2026

Uh oh!

lcnr commented May 14, 2026

Uh oh!

This comment was marked as resolved.

rustbot commented May 20, 2026

Uh oh!

traviscross commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lcnr commented Jun 9, 2026

Uh oh!

rust-bors Bot commented Jun 9, 2026

Uh oh!

Uh oh!

JonathanBrouwer commented Jun 13, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Jun 13, 2026

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

traviscross commented Mar 14, 2026 •

edited

Loading

traviscross commented Mar 22, 2026 •

edited

Loading

traviscross commented May 20, 2026 •

edited

Loading