Register wf obligation before normalizing in wfcheck

[compiler-errors]

Fixes #100041.

Why are we normalizing in wfcheck at all? Should we just be dealing w unnormalized types? I guess whether we normalize or not at all has effects on implied bounds...

cc @rust-lang/types

r? types

[compiler-errors]

This shouldn't be necessary once @lcnr's PR #99217 lands, and we should use obligation.predicate.allow_normalization() here instead.

[lcnr]

this is a breaking change again, for similar reasons to #99217.

I think we would want to merge these two PRs and get another crater run in 😅

apart from that lgtm
r? @lcnr

[lcnr]

Why are we normalizing in wfcheck at all?

anything that's not wf checking pretty much expects normalized inputs 😅 without lazy norm having unnormalized projections tends to be bad™️. we do normalize in fulfill so for obligations that requirement doesn't really exist anymore¹, but e.g. combine (infcx.at(cause, param_env).eq(a, b)) still expect a and b to be normalized as much as possible.

Footnotes

1. maybe we do want to completely stop normalizing obligations before adding them as that should improve error messages, not sure about the perf impact. ↩

[compiler-errors]

@lcnr yeah, I'm more curious about whether it's correct/important to have normalized types before we add them to the implied bounds list, like here: https://github.com/rust-lang/rust/blob/master/compiler/rustc_typeck/src/check/wfcheck.rs#L1523

This PR doesn't affect that, for example (just moves the WF obligation on that fn sig's types to before they get normalized). Should we be normalizing types there at all?

[lcnr]

@lcnr yeah, I'm more curious about whether it's correct/important to have normalized types before we add them to the implied bounds list, like here: https://github.com/rust-lang/rust/blob/master/compiler/rustc_typeck/src/check/wfcheck.rs#L1523

This PR doesn't affect that, for example (just moves the WF obligation on that fn sig's types to before they get normalized). Should we be normalizing types there at all?

With the changes to normalization in fulfill, and maybe some other ones of #99217, we are allowed to (and should) add both. We do need to add the normalized ones as we currently don't elaborate implied bounds. If you have <T as Trait<'a>>::Assoc which normalizes to &'a T, just adding <T as Trait<'a>>::Assoc as an implied bound doesn't allow you to assume T: 'a inside of the function. If we merge your PR with #99217 we should check that every time we add implied bounds, we add both normalized and unnormalized ones (alternatively, add unnormalized ones and also add the normalized ones in add_implied_bounds 😁)

[compiler-errors]

Alright. That makes sense, thanks for the explanation.

As a first step I can cherry-pick #99217 onto this branch so we can do another crater run of the merged code.

I'll think a bit more about the best way that we can collect both the normalized and un-normalized forms of those types in wfcheck.

[compiler-errors]

@bors try

[bors]

⌛ Trying commit 854a9eb2d1feaa755ed9827216f314bd29cd7854 with merge d76952bc2a74b981479554f7e1e2c6f8a8c3e3bf...

[bors]

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

[compiler-errors]

@craterbot check

[craterbot]

👌 Experiment pr-100046 created and queued.
🤖 Automatically detected try build d76952bc2a74b981479554f7e1e2c6f8a8c3e3bf
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-100046 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-100046 is completed!
📊 31 regressed and 3 fixed (240511 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[compiler-errors]

We do need to add the normalized ones as we currently don't elaborate implied bounds

ok crater shows clearly that yeah i need to do this then

[nikomatsakis]

@rfcbot reviewed

Agreed we should be checking WF before normalization.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[compiler-errors]

Lol, so this PR cannot land as-is anymore because of #100676 and a few other PRs. I need to rework this now.

[compiler-errors]

@lcnr I think this is blocked on your work to get both the unnormalized and normalized versions of a type to be considered implied wf types, correct? Since e6b8a2129fdfe4fcde1e418a8b3ec479a23c87f1 is obsolete (we don't collect implied wf tys here anymore).

[bors]

☔ The latest upstream changes (presumably #101225) made this pull request unmergeable. Please resolve the merge conflicts.

[lcnr]

already dealt with by try_normalize_with_depth_to, no need to check for allow_normalization here

[compiler-errors]

Oh cool

[lcnr]

blocked on implied bounds for unnormalized types, should hopefully implement this over the next few weeks

[aliemjay]

#105948 should probably be fixed first. Otherwise this will break the following arguably correct code:

trait Trait { type Ty; }
impl<T> Trait for T { type Ty = (); }
fn pass<T>(_: <&'static T as Trait>::Ty) {}

[lcnr]

closing this PR as #100046 (comment) is blocked on -Ztrait-solver=next plus additional work, so that's going to take a while

[aliemjay]

It is not clear to me why this is blocked on the next trait solver. Is it because of the regression in this ui test? If so it can be simply unblocked by fixing #105948 first.

[lcnr]

we need the unnormalized implied bounds for impl headers as well. Fixing #105948 would only fix it in function signatures 🤔 I think the following test breaks with this PR:

trait Trait {}

trait ToUnit {
    type Assoc;
}
impl<T> ToUnit for T {
    type Assoc = ();
}

impl<'a, 'b> Trait for <&'a &'b () as ToUnit>::Assoc {}