Skip to content

normalise host effect sub-obligations from builtin impls #143391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
+20 −2
Closed

normalise host effect sub-obligations from builtin impls #143391

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions compiler/rustc_trait_selection/src/traits/effects.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,10 +301,28 @@ fn evaluate_host_effect_from_builtin_impls<'tcx>(
selcx: &mut SelectionContext<'_, 'tcx>,
obligation: &HostEffectObligation<'tcx>,
) -> Result<ThinVec<PredicateObligation<'tcx>>, EvaluationFailure> {
match selcx.tcx().as_lang_item(obligation.predicate.def_id()) {
let mut obligations = match selcx.tcx().as_lang_item(obligation.predicate.def_id()) {
Some(LangItem::Destruct) => evaluate_host_effect_for_destruct_goal(selcx, obligation),
_ => Err(EvaluationFailure::NoSolution),
}
}?;

let mut obligations_from_normalization = thin_vec![];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@davidtwco davidtwco Jul 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.collect_predicates_for_types(
obligation.param_env,
cause,
obligation.recursion_depth + 1,
trait_def,
types,
)

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

obligations = obligations
.drain(..)
.map(|mut obligation| {
obligation.recursion_depth += 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do recursion depth incrementation anywhere else, why did you add this here? Needs a comment at least.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rust/compiler/rustc_trait_selection/src/traits/select/confirmation.rs

Line 287 in a413f77

obligation.recursion_depth + 1,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then add a comment pls

normalize_with_depth_to(
selcx,
obligation.param_env,
obligation.cause.clone(),
obligation.recursion_depth,
obligation,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't normalize the whole obligation; it's not really needed.

&mut obligations_from_normalization,
)
})
.collect();
obligations.extend(obligations_from_normalization.drain(..));
Ok(obligations)
}

// NOTE: Keep this in sync with `const_conditions_for_destruct` in the new solver.
Expand Down
Loading