trait_selection: Preserve eager normalization failures

[Dnreikronos]

Fixes #160875

This started from #161407 and the Zulip discussion around eager normalization failures.

The nested inherent associated type case takes two paths. Eager normalization handles the inner alias first, then the outer alias fails the leak check. The old fallback throws that attempt away and retries the original aliases. That retry can succeed because the lifetime constraints come back through nested goals and the parent leak check does not see them.

My first take was to resolve the inference variable after fulfillment. That made the debug assertion go away, but it was only hiding the failed relation. I do not think we should return a value from a path that already failed the leak check.

normalize_with_universes now returns the failed obligation. Deep normalization passes that failure back instead of falling into recovery. Regular normalize stays infallible and keeps the fallback behavior because its callers already expect obligations.

The fallback still has to rebuild those obligations. For higher ranked aliases, ReplaceAliasWithInfer leaves the alias in the folded value, replaces bound vars with placeholders only for the Projection obligation, and does not put the fresh inference term in the returned type. I missed that part in my first version and dropped too much.

I think this is the cleanest split. Deep normalization gets the strict behavior it promises, while regular normalization still recovers in the same general way as before.

The UI tests cover the nested IAT case and the affected diagnostics.

[lcnr]

sgtm, this is now 2 unrelated changes, is it?

• we change ReplaceAliasesWithInfer to keep hr aliases around
• we change the inner normalize to return Err

I think we should do both. Generally try to only do changes in the same PR if they build on each other

[lcnr]

ah, with the ReplaceAliasesWithInfer. I think we still want to emit a Projection obligation, just not return an infer var 🤔

[Dnreikronos]

sgtm, this is now 2 unrelated changes, is it?

• we change ReplaceAliasesWithInfer to keep hr aliases around
• we change the inner normalize to return Err

I think we should do both. Generally try to only do changes in the same PR if they build on each other

Yeah, I can split them. My thinking was that once we kept the eager failure, we already had an error to report, so I dropped the extra obligations for higher-ranked aliases in the fallback. That's why I bundled the two changes. I'd prefer to keep this one focused on normalize_with_universes returning Err and work through the visitor behavior separately.

ah, with the ReplaceAliasesWithInfer. I think we still want to emit a Projection obligation, just not return an infer var 🤔

yep, I removed too much there. I figured keeping the eager failure gave us the error we needed, so I dropped the higher-ranked projection obligations during recovery ;/. But that first failure doesn't cover every alias the visitor walks. I'll keep emitting the Projection obligation while leaving the alias in the returned type. I think handling that separately will make it easier to reason about, so I'll keep this PR focused on normalize_with_universes returning Err.
Sounds good for you?

[lcnr]

My thinking was that once we kept the eager failure, we already had an error to report, so I dropped the extra obligations for higher-ranked aliases in the fallback.

we entirely drop all failed obligations from the first normalizaiton attempt, don't we?

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

Lines 81 to 83 in a4c1445

	let mut replacer = ReplaceAliasWithInfer { at, obligations: Default::default(), universes };
	let value = original_value.fold_with(&mut replacer);
	Normalized { value, obligations: replacer.obligations }

so to make sure we actually error, ReplaceRegionsWithInfer needs to also add a Projection obligation which doesn't hold

[Dnreikronos]

My thinking was that once we kept the eager failure, we already had an error to report, so I dropped the extra obligations for higher-ranked aliases in the fallback.

we entirely drop all failed obligations from the first normalizaiton attempt, don't we?

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

Lines 81 to 83 in a4c1445

	let mut replacer = ReplaceAliasWithInfer { at, obligations: Default::default(), universes };
	let value = original_value.fold_with(&mut replacer);
	Normalized { value, obligations: replacer.obligations }

so to make sure we actually error, ReplaceRegionsWithInfer needs to also add a Projection obligation which doesn't hold

Yep, I was mixing up two paths here.

For deep normalization we want to return the eager failure. For the infallible normalize path, though, the first failed attempt is dropped and the fallback has to recreate the obligations from the original aliases. I changed it back to work that way.

ReplaceAliasWithInfer now keeps a higher-ranked alias in the folded value, but it still replaces its bound vars with placeholders and registers a Projection obligation. The fresh inference term only belongs to that obligation. It never becomes the returned type. I did the same for const aliases.

I think this split makes more sense. Deep normalization gets a real error instead of hiding it, while regular normalization keeps its recovery behavior and lets fulfillment report the projection failure. It also gets rid of the duplicate diagnostic we were seeing.

[Dnreikronos]

hey @lcnr, I did the split you asked for, so here's where this ended up

the visitor is back to what main does. in my first version I dropped the projection obligations for higher ranked aliases in the fallback, because I thought the eager failure was already enough to get an error. but like you said, the failed obligations from the first attempt get thrown away, so without that obligation nothing actually errors. so I put it back. ReplaceAliasWithInfer keeps the higher ranked alias in the returned type, swaps the bound vars for placeholders and registers the Projection obligation, and the fresh infer var only lives inside that obligation

the only real change left is normalize_with_universes returning a Result with the failed obligation. deeply_normalize just passes that error up instead of going into the fallback. plain normalize still can't fail, so when it gets an error it does the same fallback as before and lets fulfillment report it

why I went this way.. in the nested IAT case the eager attempt fails the leak check on the outer alias, and the old fallback retried the original aliases and got away with it because the lifetime stuff ends up in nested goals the parent leak check never sees. that's where the fn(?3t) came from. resolving vars after fulfillment (what I did in #161407) made the assert go away, but it was just hiding a relation that already failed, so I don't think we should return anything from that path

I think this is the cleanest split. deep normalization gets the strict behavior it says it has, and regular normalization keeps recovering like it always did. it also got rid of the duplicate diagnostic we had. one thing I'm not super happy with is the new error on the next solver, it says "type mismatch resolving" which is less clear than the old solver's "one type is more general than the other", but I'd rather not mess with diagnostics in this PR

also, earlier you said the visitor leaks placeholders. do you want a separate PR for that, or is keeping the Projection obligation enough for now?

r? lcnr
@rustbot ready

[rustbot]

Some changes occurred to the core trait solver

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

[rustbot]

r? @adwinwhite

rustbot has assigned @adwinwhite.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 77 candidates
• Random selection from 21 candidates

[lcnr]

@bors r+ rollup

[rust-bors]

📌 Commit 26be04b has been approved by lcnr

It is now in the queue for this repository.