-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Assemble const bounds via normal item bounds in old solver too #143235
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -44,6 +44,12 @@ pub fn evaluate_host_effect_obligation<'tcx>( | |||||
Err(EvaluationFailure::NoSolution) => {} | ||||||
} | ||||||
|
||||||
match evaluate_host_effect_from_conditionally_const_item_bounds(selcx, obligation) { | ||||||
Ok(result) => return Ok(result), | ||||||
Err(EvaluationFailure::Ambiguous) => return Err(EvaluationFailure::Ambiguous), | ||||||
Err(EvaluationFailure::NoSolution) => {} | ||||||
} | ||||||
|
||||||
match evaluate_host_effect_from_item_bounds(selcx, obligation) { | ||||||
Ok(result) => return Ok(result), | ||||||
Err(EvaluationFailure::Ambiguous) => return Err(EvaluationFailure::Ambiguous), | ||||||
|
@@ -153,7 +159,9 @@ fn evaluate_host_effect_from_bounds<'tcx>( | |||||
} | ||||||
} | ||||||
|
||||||
fn evaluate_host_effect_from_item_bounds<'tcx>( | ||||||
/// Assembles constness bounds from `~const` item bounds on alias types, which only | ||||||
/// hold if the `~const` where bounds also hold and the parent trait is `~const`. | ||||||
fn evaluate_host_effect_from_conditionally_const_item_bounds<'tcx>( | ||||||
selcx: &mut SelectionContext<'_, 'tcx>, | ||||||
obligation: &HostEffectObligation<'tcx>, | ||||||
) -> Result<ThinVec<PredicateObligation<'tcx>>, EvaluationFailure> { | ||||||
|
@@ -232,6 +240,63 @@ fn evaluate_host_effect_from_item_bounds<'tcx>( | |||||
} | ||||||
} | ||||||
|
||||||
/// Assembles constness bounds "normal" item bounds on aliases, which may include | ||||||
/// unconditionally `const` bounds that are *not* conditional and thus always hold. | ||||||
fn evaluate_host_effect_from_item_bounds<'tcx>( | ||||||
selcx: &mut SelectionContext<'_, 'tcx>, | ||||||
obligation: &HostEffectObligation<'tcx>, | ||||||
) -> Result<ThinVec<PredicateObligation<'tcx>>, EvaluationFailure> { | ||||||
let infcx = selcx.infcx; | ||||||
let tcx = infcx.tcx; | ||||||
let drcx = DeepRejectCtxt::relate_rigid_rigid(selcx.tcx()); | ||||||
let mut candidate = None; | ||||||
|
||||||
let mut consider_ty = obligation.predicate.self_ty(); | ||||||
while let ty::Alias(kind @ (ty::Projection | ty::Opaque), alias_ty) = *consider_ty.kind() { | ||||||
for clause in tcx.item_bounds(alias_ty.def_id).iter_instantiated(tcx, alias_ty.args) { | ||||||
let bound_clause = clause.kind(); | ||||||
let ty::ClauseKind::HostEffect(data) = bound_clause.skip_binder() else { | ||||||
continue; | ||||||
}; | ||||||
let data = bound_clause.rebind(data); | ||||||
if data.skip_binder().trait_ref.def_id != obligation.predicate.trait_ref.def_id { | ||||||
continue; | ||||||
} | ||||||
|
||||||
if !drcx.args_may_unify( | ||||||
obligation.predicate.trait_ref.args, | ||||||
data.skip_binder().trait_ref.args, | ||||||
) { | ||||||
continue; | ||||||
} | ||||||
|
||||||
let is_match = | ||||||
infcx.probe(|_| match_candidate(selcx, obligation, data, true, |_, _| {}).is_ok()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
In trying to use this patch locally with my experimental const sizedness changes, I find that I get errors in I don't have a minimal example unfortunately, but the issue was when:
I don't know that it's the correct solution but changing this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, that change is not right in general and would cause additional failures later on since predicates that we get from the item bounds need to be normalized after substitution. I think you should identify where the error was being emitted, what phase/step of checking we're doing, the param-env, why the goal predicate is not being normalized, etc, since it doesn't have much to do with this PR I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Went back and looked into this further, much easier now I've got a sense of what to look for. Unnormalised obligations were coming from the builtin impl for const sizedness that I was adding, in particular when the final field of a struct was a projection. On that branch, I've started normalising the obligations in I would submit a patch for that separately but not sure how to craft a test case for it with only the builtin impl for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @davidtwco: You can just put up a PR without a test. I can either come up with a test or I'll approve it without one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I still need to work out some issues with the change and recursive structs, but will put a patch up after resolving that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
|
||||||
if is_match { | ||||||
if candidate.is_some() { | ||||||
return Err(EvaluationFailure::Ambiguous); | ||||||
} else { | ||||||
candidate = Some(data); | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
if kind != ty::Projection { | ||||||
break; | ||||||
} | ||||||
|
||||||
consider_ty = alias_ty.self_ty(); | ||||||
} | ||||||
|
||||||
if let Some(data) = candidate { | ||||||
Ok(match_candidate(selcx, obligation, data, true, |_, _| {}) | ||||||
.expect("candidate matched before, so it should match again")) | ||||||
} else { | ||||||
Err(EvaluationFailure::NoSolution) | ||||||
} | ||||||
} | ||||||
|
||||||
fn evaluate_host_effect_from_builtin_impls<'tcx>( | ||||||
selcx: &mut SelectionContext<'_, 'tcx>, | ||||||
obligation: &HostEffectObligation<'tcx>, | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,19 @@ | ||||||
//@ check-pass | ||||||
//@ revisions: current next | ||||||
//@ ignore-compare-mode-next-solver (explicit revisions) | ||||||
//@[next] compile-flags: -Znext-solver | ||||||
|
||||||
#![feature(const_trait_impl)] | ||||||
|
||||||
#[const_trait] | ||||||
trait Bar {} | ||||||
|
||||||
trait Baz: const Bar {} | ||||||
|
||||||
trait Foo { | ||||||
// Well-formedenss of `Baz` requires `<Self as Foo>::Bar: const Bar`. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// Make sure we assemble a candidate for that via the item bounds. | ||||||
type Bar: Baz; | ||||||
} | ||||||
|
||||||
fn main() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't this use the
is_conditionally_const
"fast path"?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the constness of these item bounds is not dependent on the constness of the associated item. If I have:
<T as Foo>::Assoc
is alwaysconst A
, but onlyconst B
ifT: const Foo
.