Skip to content

small refactor to new projection code #107348

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

Merged
merged 2 commits into from
Feb 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 29 additions & 5 deletions compiler/rustc_trait_selection/src/solve/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ impl<'tcx> InferCtxtEvalExt<'tcx> for InferCtxt<'tcx> {
search_graph: &mut search_graph,
infcx: self,
var_values: CanonicalVarValues::dummy(),
in_projection_eq_hack: false,
}
.evaluate_goal(goal);

Expand All @@ -187,6 +188,10 @@ struct EvalCtxt<'a, 'tcx> {
var_values: CanonicalVarValues<'tcx>,

search_graph: &'a mut search_graph::SearchGraph<'tcx>,

/// This field is used by a debug assertion in [`EvalCtxt::evaluate_goal`],
/// see the comment in that method for more details.
in_projection_eq_hack: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Thiis hack would be also useful for other let (_, certainty) = evaluate_goal() calls in candidate assembly, maybe generalize the name and put it in a helper like evaluate_goal_expect_stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why would it be useful there? All other evaluate_goal shouldn't result in cycles when rerunning with substitutions applied (that's at least what I believe and why I've added the debug assert in the first place)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you meant for evaluate_goal added in the future? We only really hit this issue if we branch on the inference state where we actually drop info, I don't think this will happen for anything apart from hacks for better caching

}

impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
Expand All @@ -213,7 +218,8 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
loop {
let (ref infcx, goal, var_values) =
tcx.infer_ctxt().build_with_canonical(DUMMY_SP, &canonical_goal);
let mut ecx = EvalCtxt { infcx, var_values, search_graph };
let mut ecx =
EvalCtxt { infcx, var_values, search_graph, in_projection_eq_hack: false };
let result = ecx.compute_goal(goal);

// FIXME: `Response` should be `Copy`
Expand Down Expand Up @@ -243,10 +249,28 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
let canonical_goal = self.infcx.canonicalize_query(goal, &mut orig_values);
let canonical_response =
EvalCtxt::evaluate_canonical_goal(self.tcx(), self.search_graph, canonical_goal)?;
Ok((
!canonical_response.value.var_values.is_identity(),
instantiate_canonical_query_response(self.infcx, &orig_values, canonical_response),
))

let has_changed = !canonical_response.value.var_values.is_identity();
let certainty =
instantiate_canonical_query_response(self.infcx, &orig_values, canonical_response);

// Check that rerunning this query with its inference constraints applied
// doesn't result in new inference constraints and has the same result.
//
// If we have projection goals like `<T as Trait>::Assoc == u32` we recursively
// call `exists<U> <T as Trait>::Assoc == U` to enable better caching. This goal
// could constrain `U` to `u32` which would cause this check to result in a
// solver cycle.
if cfg!(debug_assertions) && has_changed && !self.in_projection_eq_hack {
let mut orig_values = OriginalQueryValues::default();
let canonical_goal = self.infcx.canonicalize_query(goal, &mut orig_values);
let canonical_response =
EvalCtxt::evaluate_canonical_goal(self.tcx(), self.search_graph, canonical_goal)?;
assert!(canonical_response.value.var_values.is_identity());
assert_eq!(certainty, canonical_response.value.certainty);
}

Ok((has_changed, certainty))
}

fn compute_goal(&mut self, goal: Goal<'tcx, ty::Predicate<'tcx>>) -> QueryResult<'tcx> {
Expand Down
91 changes: 52 additions & 39 deletions compiler/rustc_trait_selection/src/solve/project_goals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
projection_ty: goal.predicate.projection_ty,
term: unconstrained_rhs,
});
let (_has_changed, normalize_certainty) =
self.evaluate_goal(goal.with(self.tcx(), unconstrained_predicate))?;
let (_has_changed, normalize_certainty) = self.in_projection_eq_hack(|this| {
this.evaluate_goal(goal.with(this.tcx(), unconstrained_predicate))
})?;

let nested_eq_goals =
self.infcx.eq(goal.param_env, unconstrained_rhs, predicate.term)?;
Expand All @@ -55,6 +56,15 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
}
}

/// This sets a flag used by a debug assert in [`EvalCtxt::evaluate_goal`],
/// see the comment in that method for more details.
fn in_projection_eq_hack<T>(&mut self, f: impl FnOnce(&mut Self) -> T) -> T {
self.in_projection_eq_hack = true;
let result = f(self);
self.in_projection_eq_hack = false;
result
}

/// Is the projection predicate is of the form `exists<T> <Ty as Trait>::Assoc = T`.
///
/// This is the case if the `term` is an inference variable in the innermost universe
Expand Down Expand Up @@ -122,6 +132,28 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
&& goal.param_env.visit_with(&mut visitor).is_continue()
}

/// After normalizing the projection to `normalized_alias` with the given
/// `normalization_certainty`, constrain the inference variable `term` to it
/// and return a query response.
fn eq_term_and_make_canonical_response(
&mut self,
goal: Goal<'tcx, ProjectionPredicate<'tcx>>,
normalization_certainty: Certainty,
normalized_alias: impl Into<ty::Term<'tcx>>,
) -> QueryResult<'tcx> {
// The term of our goal should be fully unconstrained, so this should never fail.
//
// It can however be ambiguous when the `normalized_alias` contains a projection.
let nested_goals = self
.infcx
.eq(goal.param_env, goal.predicate.term, normalized_alias.into())
.expect("failed to unify with unconstrained term");
let rhs_certainty =
self.evaluate_all(nested_goals).expect("failed to unify with unconstrained term");

self.make_canonical_response(normalization_certainty.unify_and(rhs_certainty))
}

fn merge_project_candidates(
&mut self,
mut candidates: Vec<Candidate<'tcx>>,
Expand Down Expand Up @@ -218,7 +250,7 @@ impl<'tcx> assembly::GoalKind<'tcx> for ProjectionPredicate<'tcx> {
.map(|pred| goal.with(tcx, pred));

nested_goals.extend(where_clause_bounds);
let trait_ref_certainty = ecx.evaluate_all(nested_goals)?;
let match_impl_certainty = ecx.evaluate_all(nested_goals)?;

// In case the associated item is hidden due to specialization, we have to
// return ambiguity this would otherwise be incomplete, resulting in
Expand All @@ -230,7 +262,7 @@ impl<'tcx> assembly::GoalKind<'tcx> for ProjectionPredicate<'tcx> {
goal.predicate.def_id(),
impl_def_id
)? else {
return ecx.make_canonical_response(trait_ref_certainty.unify_and(Certainty::AMBIGUOUS));
return ecx.make_canonical_response(match_impl_certainty.unify_and(Certainty::AMBIGUOUS));
};

if !assoc_def.item.defaultness(tcx).has_value() {
Expand Down Expand Up @@ -277,17 +309,7 @@ impl<'tcx> assembly::GoalKind<'tcx> for ProjectionPredicate<'tcx> {
ty.map_bound(|ty| ty.into())
};

// The term of our goal should be fully unconstrained, so this should never fail.
//
// It can however be ambiguous when the resolved type is a projection.
let nested_goals = ecx
.infcx
.eq(goal.param_env, goal.predicate.term, term.subst(tcx, substs))
.expect("failed to unify with unconstrained term");
let rhs_certainty =
ecx.evaluate_all(nested_goals).expect("failed to unify with unconstrained term");

ecx.make_canonical_response(trait_ref_certainty.unify_and(rhs_certainty))
ecx.eq_term_and_make_canonical_response(goal, match_impl_certainty, term.subst(tcx, substs))
})
}

Expand All @@ -307,18 +329,11 @@ impl<'tcx> assembly::GoalKind<'tcx> for ProjectionPredicate<'tcx> {
)?;
let subst_certainty = ecx.evaluate_all(nested_goals)?;

// The term of our goal should be fully unconstrained, so this should never fail.
//
// It can however be ambiguous when the resolved type is a projection.
let nested_goals = ecx
.infcx
.eq(goal.param_env, goal.predicate.term, assumption_projection_pred.term)
.expect("failed to unify with unconstrained term");
let rhs_certainty = ecx
.evaluate_all(nested_goals)
.expect("failed to unify with unconstrained term");

ecx.make_canonical_response(subst_certainty.unify_and(rhs_certainty))
ecx.eq_term_and_make_canonical_response(
goal,
subst_certainty,
assumption_projection_pred.term,
)
})
} else {
Err(NoSolution)
Expand Down Expand Up @@ -434,14 +449,12 @@ impl<'tcx> assembly::GoalKind<'tcx> for ProjectionPredicate<'tcx> {
[ty::GenericArg::from(goal.predicate.self_ty())],
));

let mut nested_goals = ecx.infcx.eq(
goal.param_env,
goal.predicate.term.ty().unwrap(),
let is_sized_certainty = ecx.evaluate_goal(goal.with(tcx, sized_predicate))?.1;
return ecx.eq_term_and_make_canonical_response(
goal,
is_sized_certainty,
tcx.types.unit,
)?;
nested_goals.push(goal.with(tcx, sized_predicate));

return ecx.evaluate_all_and_make_canonical_response(nested_goals);
);
}

ty::Adt(def, substs) if def.is_struct() => {
Expand All @@ -453,7 +466,8 @@ impl<'tcx> assembly::GoalKind<'tcx> for ProjectionPredicate<'tcx> {
tcx,
ty::Binder::dummy(goal.predicate.with_self_ty(tcx, self_ty)),
);
return ecx.evaluate_all_and_make_canonical_response(vec![new_goal]);
let (_, certainty) = ecx.evaluate_goal(new_goal)?;
Copy link
Member

@compiler-errors compiler-errors Jan 27, 2023

Choose a reason for hiding this comment

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

Why do we throw away changed here, but evaluate_all runs in a loop until changed is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because evaluate_all has multiple goals which can influence each other.

added a second commit which checks that the result of evaluate_goal is stable and we don't get any additional information by rerunning a goal after applying its substitutions.

Will have to change the is_identity check to ignore regions once they are implemented but that doesn't yet matter.

return ecx.make_canonical_response(certainty);
}
}
}
Expand All @@ -466,7 +480,8 @@ impl<'tcx> assembly::GoalKind<'tcx> for ProjectionPredicate<'tcx> {
tcx,
ty::Binder::dummy(goal.predicate.with_self_ty(tcx, self_ty)),
);
return ecx.evaluate_all_and_make_canonical_response(vec![new_goal]);
let (_, certainty) = ecx.evaluate_goal(new_goal)?;
return ecx.make_canonical_response(certainty);
}
},

Expand All @@ -479,9 +494,7 @@ impl<'tcx> assembly::GoalKind<'tcx> for ProjectionPredicate<'tcx> {
),
};

let nested_goals =
ecx.infcx.eq(goal.param_env, goal.predicate.term.ty().unwrap(), metadata_ty)?;
ecx.evaluate_all_and_make_canonical_response(nested_goals)
ecx.eq_term_and_make_canonical_response(goal, Certainty::Yes, metadata_ty)
})
}

Expand Down
6 changes: 5 additions & 1 deletion compiler/rustc_trait_selection/src/solve/search_graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ impl<'tcx> SearchGraph<'tcx> {
/// Tries putting the new goal on the stack, returning an error if it is already cached.
///
/// This correctly updates the provisional cache if there is a cycle.
#[instrument(level = "debug", skip(self, tcx), ret)]
pub(super) fn try_push_stack(
&mut self,
tcx: TyCtxt<'tcx>,
Expand Down Expand Up @@ -79,8 +80,10 @@ impl<'tcx> SearchGraph<'tcx> {
Entry::Occupied(entry_index) => {
let entry_index = *entry_index.get();

cache.add_dependency_of_leaf_on(entry_index);
let stack_depth = cache.depth(entry_index);
debug!("encountered cycle with depth {stack_depth:?}");

cache.add_dependency_of_leaf_on(entry_index);

self.stack[stack_depth].has_been_used = true;
// NOTE: The goals on the stack aren't the only goals involved in this cycle.
Expand Down Expand Up @@ -117,6 +120,7 @@ impl<'tcx> SearchGraph<'tcx> {
/// updated the provisional cache and we have to recompute the current goal.
///
/// FIXME: Refer to the rustc-dev-guide entry once it exists.
#[instrument(level = "debug", skip(self, tcx, actual_goal), ret)]
pub(super) fn try_finalize_goal(
&mut self,
tcx: TyCtxt<'tcx>,
Expand Down