normalise host effect sub-obligations from builtin impls #143391
Conversation
Obligations produced by builtin impls need to be normalised so as not to later be compared to normalised candidates. For example, when introducing const sizedness then during the compilation of `std`, there will be a `thread::Inner as const MetaSized` obligation, the builtin impl will create a `<u32 as AtomicPrimitive>::AtomicInner as const MetaSized` obligation, which if unnormalised, will end up in `evaluate_host_effect_from_item_bounds` and then `match_candidate` being compared with a normalised candidate and failing.
rustbot
added
S-waiting-on-review
compiler-errors
changed the title
| obligation.param_env, | ||
| obligation.cause.clone(), | ||
| obligation.recursion_depth, | ||
| obligation, |
There was a problem hiding this comment.
Don't normalize the whole obligation; it's not really needed.
| obligations = obligations | ||
| .drain(..) | ||
| .map(|mut obligation| { | ||
| obligation.recursion_depth += 1; |
There was a problem hiding this comment.
We don't do recursion depth incrementation anywhere else, why did you add this here? Needs a comment at least.
There was a problem hiding this comment.
On my branch with const sizedness, if I don't add this then it will just recurse indefinitely. It mirrors the increment that happens with trait predicates before it calls collect_predicates_for_types which is where those are normalised.
| } | ||
| }?; | ||
|
|
||
| let mut obligations_from_normalization = thin_vec![]; |
There was a problem hiding this comment.
Uplift this normalization into evaluate_host_effect_for_destruct_goal. Each built-in obligation should be responsible for normalizing its own sub-obligations if needed.
There was a problem hiding this comment.
Specifically, we only need to normalize the const condition part of the obligation before we create the obligation itself, so normalizing it there is more clear what it's doing.
There was a problem hiding this comment.
I did this in evaluate_host_effect_from_builtin_impls mirroring what happened for trait predicates, where this happened for every kind of builtin impl, but also because I'm not certain whether evaluate_host_effect_for_destruct_goal needs it, but I know that the evaluate_host_effect_for_sizedness_goal that exists in the branch that I spun this out of does need it, and putting it here did both.
rust/compiler/rustc_trait_selection/src/traits/select/confirmation.rs
Lines 284 to 290 in a413f77
That's also why I do this on the whole obligation returned by this function, because it wasn't trivial to get the same return type out of both destruct and sizedness functions that wasn't Obligation. I could probably do that if we wanted but it was cleaner this way.
There was a problem hiding this comment.
I'd rather be explicit rather than just normalizing everything at the end. I would recommend not modeling things off of the old solver; it's not great architecturally, and a lot of its behavior is just how things grew organically over the last 10 years. Sorry for the code duplication, but this normalization is subtle and right now it feels like kinda a large hammer.
There was a problem hiding this comment.
That's totally fair. As I don't have a justification for doing normalisation for the destruct goal and only know it is necessary for the const sizedness goal, there's not really anything to do in this PR, I don't have a function in master to add the normalisation to - so I'll close this. I'll incorporate your feedback for these changes into my local working branch though.
rustbot
added
S-waiting-on-author
Obligations produced by builtin impls need to be normalised so as not to later be compared to normalised candidates and fail.
For example, when introducing const sizedness then during the compilation of
std, there will be athread::Inner as const MetaSizedobligation, the builtin impl will create a<u32 as AtomicPrimitive>::AtomicInner as const MetaSizedobligation, which if unnormalised, will end up inevaluate_host_effect_from_item_boundsand thenmatch_candidatebeing compared with a normalised candidate and failing.r? @compiler-errors