Do not retain `Normalization` goal errors in nested goals for `BestObligationVisitor:: non_trivial_candidates ` by LorrensP-2158466 · Pull Request #162443 · rust-lang/rust

LorrensP-2158466 · 2026-09-07T18:27:03Z

When getting the non_trivial_candidates, also consider candidates whose nested candidates have a TypeRelating failure as well, instead of only impl-where clauses.

Should fix #161882
related: #t-types/call-for-participation > diagnostics proof tree visitor is too eager

r? @lcnr

rustbot · 2026-09-07T18:27:06Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

lcnr · 2026-09-17T11:04:15Z

                                            | GoalSource::AliasBoundConstCondition
                                            | GoalSource::AliasWellFormed
+                                            | GoalSource::TypeRelating
                                    ) && nested_goal.result().is_err()


this is confusing 🤔 we keep candidates which have a nested goal which fails with such an error.

should we not do it the other way around. Only keep candidates which have a failing where-bound, so if a TypeRelating candidate fails, we drop it?

Should we do it the other way around, making sure that a candidate does not fail because there's a non ImplWhereBound which fails?

What exactly are the goals + candidates here

View changes since the review

this is confusing 🤔 we keep candidates which have a nested goal which fails with such an error.

yeah, this was just the simplest way I could make this example work :D.

the heavily simplified trace of the non WF errors:

main goal: MyError: From<<Self as MaybeError>::ComposeWithOther> 2 candidates: [No Solution: `NonZero impl`, No Solution: a `From impl`]: candidate 1: `NonZero impl`, 3 nested goals: nested_goal 1: <Self as MaybeError>::ComposeWithOther = NonZero<MyError> source: TypeRelating result: NoSolution nested_goal 2: `MyError: Sized` source: `ImplWhereBound` result: `Yes` nested_goal 3: `MyError: ZeroablePrimitive` source: `ImplWhereBound` result: `NoSolution` candidate 2: `From impl`, 2 nested goals nested_goal 1: `<Self as MaybeError>::ComposeWithOther = MyError` source: TypeRelating result: NoSolution nested_goal 2: `MyError: Sized` source: ImplWhereBound result: Yes

Candidate 1 is kept because one of its nested goals is a ImplWhereBound with NoSolution. Candidate 2 is removed because it does not match any of the criteria in the retain closure.

So we keep going with candidate 1, which leads us to the confusing error.

my fix also removed that candidate.

should we not do it the other way around. Only keep candidates which have a failing where-bound, so if a TypeRelating candidate fails, we drop it?

Should we do it the other way around, making sure that a candidate does not fail because there's a non ImplWhereBound which fails?

so yeah, i think we should only keep those that only have the failing ones :

GoalSource::ImplWhereBound | GoalSource::AliasBoundConstCondition | GoalSource::AliasWellFormed

LorrensP-2158466 · 2026-09-21T16:01:38Z

+                            let nested_goals = candidate.instantiate_nested_goals(self.span());
+                            let not_boring = nested_goals.iter().any(|nested_goal| {
+                                matches!(
+                                    nested_goal.source(),
+                                    GoalSource::ImplWhereBound
+                                        | GoalSource::AliasBoundConstCondition
+                                        | GoalSource::AliasWellFormed
+                                ) && nested_goal.result().is_err()
+                            });
+                            let interesting = nested_goals.iter().all(|g| {
+                                g.result().is_ok()
+                                    || matches!(
+                                        g.source(),
                                        GoalSource::ImplWhereBound
                                            | GoalSource::AliasBoundConstCondition
                                            | GoalSource::AliasWellFormed
-                                    ) && nested_goal.result().is_err()
-                                },
-                            )
+                                    )
+                            });
+                            not_boring && interesting
                        })


ah hmm 🤔. did you mean that we just drop any that has a failing typerelating goal? i did it a bit more generic i think.

View changes since the review

I am still confused here. Can you state the intended behavior in words. What I would expect is

"we only go into candidates if they do not have a failing TypeRelating"

looking at #162443 (comment) we should not recurse into either of the candidates, should we?

Yeah, my change is confusing now that I am reading it again. I wrongly assumed that we only wanted such failing nested goals.

interesting should be a check that there is no failing TypeRelating goal present.

I am still confused here. Can you state the intended behavior in words. What I would expect is

"we only go into candidates if they do not have a failing TypeRelating"why do we need to check both interesting and not_boring? Partially as a review question, partially as a "please write a comment"

I am gonna start with information you already know, but it makes it easier for me :D.

The goal (pun intended) of the BestObligation visitor is to descend into the proof tree of some goal to find the most likely source that it failed such that the user can fix their code.

It does this by looking at the candidates of the goal and filtering them to ones that are actually useful: currently this is done by looking at the nested goals of those candidates and making sure that a failing "non boring" one is present:

/// Filter out the candidates that aren't interesting to visit for the /// purposes of reporting errors. For ambiguities, we only consider /// candidates that may hold. For errors, we only consider candidates that /// *don't* hold and which have impl-where clauses that also don't hold. fn non_trivial_candidates<'a>( ... // If we have >1 candidate, one may still be due to "boring" reasons, like // an alias-relate that failed to hold when deeply evaluated. We really // don't care about reasons like this. matches!( nested_goal.source(), // nested goal of candidate // non boring sources GoalSource::ImplWhereBound | GoalSource::AliasBoundConstCondition | GoalSource::AliasWellFormed ) && nested_goal.result().is_err()

So we are ignoring GoalSource::Misc and GoalSource::Normalization here. This was already here before I came, but I assume its because these have no affect on the user and are useless to them. Its also tied to actually visiting the nested goals later on if we filterd to only 1 candidate of the goal, because we don't go into failed nested goals with Misc or Normalization.

Reason of confusing error

But this breaks with the reproducer:

Goal: MyError: From<<Self as MaybeError>::ComposeWithOther> Candidate 1: impl<T: ZeroablePrimitive> From<NonZero<T>> for T (T = MyError) <Self as MaybeError>::ComposeWithOther == NonZero<MyError> Normalization fails MyError: Sized ImplWhereBound ok MyError: ZeroablePrimitive ImplWhereBound fails Candidate 2: impl<T> From<T> for T (T = MyError) <Self as MaybeError>::ComposeWithOther == MyError Normalization fails MyError: Sized ImplWhereBound ok

The second candidate is removed because it does not have a failing ImplWhereBound.

Candidate 1 has a failing ImplWhereBound: MyError: ZeroablePrimitive and thus we use that candidate. Then we look at the nested goals again to figure out what the next goal to visit should be, Normalization failures are not visited, thus we visit MyError: ZeroablePrimitive, which gives us the error that does not make sense.

My reasoning

I feel like we should not use candidates who contain failing nested Normalization goals (maybe even Misc? idk what exactly that is), because it makes the candidate wrong to use, regardless of the fact that a failing ImplWhereBound is present.

I presume that in all cases that this would occur, the failing Normalization and ImplWhereBound are related semantically. I mean, why would we look at impl<T: ZeroablePrimitive> From<NonZero<T>> for T to obtain diagnostics in our test case? But it does cause the failing ImplWhereBound and Normalization goals.

why do we need to check both interesting and not_boring?

Because not_boring is the existing test (which needs to be there) and interesting makes sure we are only using candidates that actually make sense.

(interesting is maybe a bad wording)

I hope that i am making some sense :D.

So we are ignoring GoalSource::Misc and GoalSource::Normalization here. This was already here before I came, but I assume its because these have no affect on the user and are useless to them. Its also tied to actually visiting the nested goals later on if we filterd to only 1 candidate of the goal, because we don't go into failed nested goals with Misc or Normalization.

what are the tests which change if we drop the not_boring check?

I agree that we shouldn't look at candidates with failing Normalization goals. Why are we not going into candidates without failing GoalSource::ImplWhereBound | GoalSource::AliasBoundConstCondition | GoalSource::AliasWellFormed 🤔 I guess the question is: what are the reasons that candidates we "properly consider" fails if its not due to norm or ImplWhereBound... are these candidates with failing Misc goals?

what are the tests which change if we drop the not_boring check?

I can answer that one now, just ran ./x test ui while i had dinner and to my surprise, none of the tests changed.

I guess the question is: what are the reasons that candidates we "properly consider" fails if its not due to norm or ImplWhereBound... are these candidates with failing Misc goals?

I am not sure I understand you correctly, you want me to check if need to look at failing Misc nested goals of candidates as well?

I can answer that one now, just ran ./x test ui while i had dinner and to my surprise, none of the tests changed.

that was my question: why do we have this check and what tests are affected.

the answer here is: apparently there's no reason to keep it

Alright, thats clear, i'll remove it.

lcnr · 2026-10-01T13:32:54Z

@rustbot author

rustbot · 2026-10-01T13:32:59Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

rustbot · 2026-10-06T19:42:25Z

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.

LorrensP-2158466 · 2026-10-06T23:14:53Z

ah, some things changed to the next solver by your pr in #163496.

TypeRelating was removed, but looking at the traces and the pr itself, it seems we now need to check for Normalization instead of TypeRelating.

should be good now (i hope). @rustbot ready

lcnr · 2026-10-07T14:34:51Z

#162443 (comment) is still relevant.

LorrensP-2158466 · 2026-10-08T20:52:29Z

@rustbot ready

i also squashed

…dates who have a failed `Normalization` nested goal. The previous condition on `ImplWhereBound` and co is now removed as it is unessecary. + add test of issue

lcnr · 2026-10-09T13:42:57Z

@bors r+ rollup

rust-bors · 2026-10-09T13:43:00Z

📌 Commit f21a12a has been approved by lcnr

It is now in the queue for this repository.

@lcnr

…on-visitor, r=lcnr Do not retain `Normalization` goal errors in nested goals for `BestObligationVisitor:: non_trivial_candidates ` When getting the `non_trivial_candidates`, also consider candidates whose nested candidates have a `TypeRelating` failure as well, instead of only impl-where clauses. Should fix rust-lang#161882 related: [#t-types/call-for-participation > diagnostics proof tree visitor is too eager](https://rust-lang.zulipchat.com/#narrow/channel/618216-t-types.2Fcall-for-participation/topic/diagnostics.20proof.20tree.20visitor.20is.20too.20eager/with/622349291) r? @lcnr

@lcnr

…on-visitor, r=lcnr Do not retain `Normalization` goal errors in nested goals for `BestObligationVisitor:: non_trivial_candidates ` When getting the `non_trivial_candidates`, also consider candidates whose nested candidates have a `TypeRelating` failure as well, instead of only impl-where clauses. Should fix rust-lang#161882 related: [#t-types/call-for-participation > diagnostics proof tree visitor is too eager](https://rust-lang.zulipchat.com/#narrow/channel/618216-t-types.2Fcall-for-participation/topic/diagnostics.20proof.20tree.20visitor.20is.20too.20eager/with/622349291) r? @lcnr

…uwer Rollup of 24 pull requests Successful merges: - #161998 ( Support type-relative assoc item paths in generic param defaults & const param types) - #162106 (Helpful suggestions for incorrect address-of mutability (2)) - #162652 (Syntactically reject leading parenthesized precise capturing lists in bare trait object types (`(use<…>)+`)) - #163337 (MIR move elimination [3/6]: PreciseLiveness) - #163938 (-Zassumptions-on-binders: rewrite alias outlives constraints more goodly) - #163939 (Better debug impls for some assumptions on binders types) - #163954 (fix(bootstrap/darwin): fix rpath for distributed LLD) - #163956 (Pass the unremapped path to the `rustc` invocation for doctests) - #164042 (Allow testing cg-gcc on any target) - #162443 (Do not retain `Normalization` goal errors in nested goals for `BestObligationVisitor:: non_trivial_candidates `) - #162908 (Fix - const parameters rejected when identical) - #163193 (cfi: mangle `f128` as `e` rather than `g` on platforms without `_Float128`) - #163634 (move overflow lint computation into decorator) - #163666 (Updates the expect message library/core/src/time.rs) - #163727 (rigid aliases to non-rigid for fully normalized check) - #163745 (replace `fully_monomorphized` with `cx.typing_env()`) - #163912 (Fix debug assert failure in `note_obligation_cause_code_inner`) - #163950 (don't treat inherited opaques as defining) - #163972 (const-eval: ICE when we hit a non-const fn) - #164000 (When mentioning that closure doesn't implement trait, point at closure) - #164007 ([rustdoc] Prefer local paths over remote ones when foreign item is locally reexported) - #164008 (properly ignore the current goal's usages) - #164017 (cg_llvm: Avoid some explicit casts to `*const c_char`) - #164025 (Less `CanonicalVarValues`)

@lcnr

Rollup merge of #162443 - LorrensP-2158466:fix-best-obligation-visitor, r=lcnr Do not retain `Normalization` goal errors in nested goals for `BestObligationVisitor:: non_trivial_candidates ` When getting the `non_trivial_candidates`, also consider candidates whose nested candidates have a `TypeRelating` failure as well, instead of only impl-where clauses. Should fix #161882 related: [#t-types/call-for-participation > diagnostics proof tree visitor is too eager](https://rust-lang.zulipchat.com/#narrow/channel/618216-t-types.2Fcall-for-participation/topic/diagnostics.20proof.20tree.20visitor.20is.20too.20eager/with/622349291) r? @lcnr

…uwer Rollup of 24 pull requests Successful merges: - rust-lang/rust#161998 ( Support type-relative assoc item paths in generic param defaults & const param types) - rust-lang/rust#162106 (Helpful suggestions for incorrect address-of mutability (2)) - rust-lang/rust#162652 (Syntactically reject leading parenthesized precise capturing lists in bare trait object types (`(use<…>)+`)) - rust-lang/rust#163337 (MIR move elimination [3/6]: PreciseLiveness) - rust-lang/rust#163938 (-Zassumptions-on-binders: rewrite alias outlives constraints more goodly) - rust-lang/rust#163939 (Better debug impls for some assumptions on binders types) - rust-lang/rust#163954 (fix(bootstrap/darwin): fix rpath for distributed LLD) - rust-lang/rust#163956 (Pass the unremapped path to the `rustc` invocation for doctests) - rust-lang/rust#164042 (Allow testing cg-gcc on any target) - rust-lang/rust#162443 (Do not retain `Normalization` goal errors in nested goals for `BestObligationVisitor:: non_trivial_candidates `) - rust-lang/rust#162908 (Fix - const parameters rejected when identical) - rust-lang/rust#163193 (cfi: mangle `f128` as `e` rather than `g` on platforms without `_Float128`) - rust-lang/rust#163634 (move overflow lint computation into decorator) - rust-lang/rust#163666 (Updates the expect message library/core/src/time.rs) - rust-lang/rust#163727 (rigid aliases to non-rigid for fully normalized check) - rust-lang/rust#163745 (replace `fully_monomorphized` with `cx.typing_env()`) - rust-lang/rust#163912 (Fix debug assert failure in `note_obligation_cause_code_inner`) - rust-lang/rust#163950 (don't treat inherited opaques as defining) - rust-lang/rust#163972 (const-eval: ICE when we hit a non-const fn) - rust-lang/rust#164000 (When mentioning that closure doesn't implement trait, point at closure) - rust-lang/rust#164007 ([rustdoc] Prefer local paths over remote ones when foreign item is locally reexported) - rust-lang/rust#164008 (properly ignore the current goal's usages) - rust-lang/rust#164017 (cg_llvm: Avoid some explicit casts to `*const c_char`) - rust-lang/rust#164025 (Less `CanonicalVarValues`)

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 Sep 7, 2026

rustbot assigned lcnr Sep 7, 2026

rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Sep 7, 2026

This comment has been minimized.

Sign in to view

LorrensP-2158466 force-pushed the fix-best-obligation-visitor branch from aed52c9 to f365966 Compare September 9, 2026 17:47

lcnr reviewed Sep 17, 2026

View reviewed changes

LorrensP-2158466 commented Sep 21, 2026

View reviewed changes

rustbot 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 Oct 1, 2026

This comment has been minimized.

Sign in to view

LorrensP-2158466 force-pushed the fix-best-obligation-visitor branch from c7f5201 to df6b412 Compare October 6, 2026 19:42

This comment has been minimized.

Sign in to view

LorrensP-2158466 force-pushed the fix-best-obligation-visitor branch from df6b412 to 5063e2e Compare October 6, 2026 23:12

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 Oct 6, 2026

LorrensP-2158466 changed the title ~~Retain TypeRelating errors in nested goals for BestObligationVisitor:: non_trivial_candidates~~ Do not retain Normalization goal errors in nested goals for BestObligationVisitor:: non_trivial_candidates Oct 6, 2026

This comment has been minimized.

Sign in to view

LorrensP-2158466 force-pushed the fix-best-obligation-visitor branch from 2361023 to 7d105d2 Compare October 8, 2026 20:52

lcnr reviewed Oct 9, 2026

View reviewed changes

Comment thread compiler/rustc_trait_selection/src/solve/fulfill/derive_errors.rs Outdated

lcnr reviewed Oct 9, 2026

View reviewed changes

Comment thread compiler/rustc_trait_selection/src/solve/fulfill/derive_errors.rs

In the BestObligation prooftree visitor, we now only skip any candi…

f21a12a

…dates who have a failed `Normalization` nested goal. The previous condition on `ImplWhereBound` and co is now removed as it is unessecary. + add test of issue

LorrensP-2158466 force-pushed the fix-best-obligation-visitor branch from 7d105d2 to f21a12a Compare October 9, 2026 10:53

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 Oct 9, 2026

JonathanBrouwer mentioned this pull request Oct 9, 2026

Rollup of 10 pull requests #164048

Closed

This was referenced Oct 9, 2026

Rollup of 10 pull requests #164050

Closed

Rollup of 24 pull requests #164052

Merged

rust-bors Bot merged commit cea8afc into rust-lang:main Oct 9, 2026
14 checks passed

rustbot added this to the 1.101.0 milestone Oct 9, 2026

Conversation

LorrensP-2158466 commented Sep 7, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 7, 2026

Uh oh!

This comment has been minimized.

lcnr Sep 17, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LorrensP-2158466 Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LorrensP-2158466 Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LorrensP-2158466 Sep 21, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Reason of confusing error

My reasoning

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcnr commented Oct 1, 2026

Uh oh!

rustbot commented Oct 1, 2026

Uh oh!

This comment has been minimized.

rustbot commented Oct 6, 2026

Uh oh!

This comment has been minimized.

LorrensP-2158466 commented Oct 6, 2026

Uh oh!

This comment has been minimized.

lcnr commented Oct 7, 2026

Uh oh!

LorrensP-2158466 commented Oct 8, 2026

Uh oh!

Uh oh!

Uh oh!

lcnr commented Oct 9, 2026

Uh oh!

rust-bors Bot commented Oct 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LorrensP-2158466 commented Sep 7, 2026 •

edited by rustbot

Loading

lcnr Sep 17, 2026 •

edited by rustbot

Loading

LorrensP-2158466 Sep 21, 2026 •

edited

Loading

LorrensP-2158466 Sep 21, 2026 •

edited

Loading

LorrensP-2158466 Sep 21, 2026 •

edited by rustbot

Loading