Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
28 changes: 18 additions & 10 deletions compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,11 @@ pub(super) enum AssembleCandidatesFrom {
/// user-written and built-in impls. We only expect `ParamEnv` and `AliasBound`
/// candidates to be assembled.
EnvAndBounds,
/// FIXME: only for the fast path of normalizes-to goal. The fast path is incomplete and
/// we shouldn't force ambiguity if no candidate exist when assembling for opaque self_ty.
/// See `try_assemble_bounds_via_registered_opaques`.
EnvAndBoundsFastPath,
Object,
Impl,
}

Expand All @@ -368,6 +373,8 @@ impl AssembleCandidatesFrom {
match self {
AssembleCandidatesFrom::All => true,
AssembleCandidatesFrom::EnvAndBounds => false,
AssembleCandidatesFrom::EnvAndBoundsFastPath => false,
AssembleCandidatesFrom::Object => false,
AssembleCandidatesFrom::Impl => true,
}
}
Expand Down Expand Up @@ -458,25 +465,21 @@ where
self.assemble_object_bound_candidates(goal, &mut candidates);
}
}
AssembleCandidatesFrom::EnvAndBounds => {
AssembleCandidatesFrom::EnvAndBounds | AssembleCandidatesFrom::EnvAndBoundsFastPath => {
self.assemble_alias_bound_candidates(goal, &mut candidates);
self.assemble_param_env_candidates(
goal,
&mut candidates,
&mut failed_candidate_info,
);
// This is somewhat inconsistent and may make #57893 slightly easier to exploit.
// However, it matches the behavior of the old solver. See
// `tests/ui/traits/next-solver/normalization-shadowing/use_object_if_empty_env.rs`.
if matches!(normalized_self_ty.kind(), ty::Dynamic(..))
&& !candidates.iter().any(|c| matches!(c.source, CandidateSource::ParamEnv(_)))
{
self.assemble_object_bound_candidates(goal, &mut candidates);
}
}
AssembleCandidatesFrom::Object => {
self.assemble_object_bound_candidates(goal, &mut candidates);
}
AssembleCandidatesFrom::Impl => {
self.assemble_builtin_impl_candidates(goal, &mut candidates);
self.assemble_impl_candidates(goal, &mut candidates);
self.assemble_object_bound_candidates(goal, &mut candidates);
}
}

Expand Down Expand Up @@ -1096,7 +1099,12 @@ where
});
}

if candidates.is_empty() {
// If we're on the fast path, it's possible that we still have applicable impl candidates.
// We should return empty list here rather than bail out with fallback candidate.
if candidates.is_empty()
&& !matches!(assemble_from, AssembleCandidatesFrom::EnvAndBoundsFastPath)
{
debug!("no candidates found via registered opaques for self ty {self_ty:?}");
let source = CandidateSource::BuiltinImpl(BuiltinImplSource::Misc);
let certainty = Certainty::Maybe {
cause: MaybeCause::Ambiguity,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ where
&mut self,
goal: Goal<I, ty::NormalizesTo<I>>,
) -> QueryResult<I> {
// Fast path via preferred env, alias bound and object candidates.
// Fast path via preferred env and alias bound candidates.
if let Some(result) = self.assemble_and_merge_env_candidates(goal) {
return result;
}
Expand All @@ -64,6 +64,18 @@ where
if let Some(proven_via) = proven_via {
match proven_via {
TraitGoalProvenVia::ParamEnv | TraitGoalProvenVia::AliasBound => {
// This is somewhat inconsistent and may make #57893 slightly easier to exploit.
// However, it matches the behavior of the old solver. See
// `tests/ui/traits/next-solver/normalization-shadowing/use_object_if_empty_env.rs`.
// FIXME: predicates with opaque self type rely on assembly call to force
// ambiguous fallback candidate. It happens to be this object assembly call
// here.
let (candidates, _) =
self.assemble_and_evaluate_candidates(goal, AssembleCandidatesFrom::Object);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved object candidate out of the fast path to avoid coherence issues.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

which coherence issue? what's the difference between EnvAndBounds and EnvAndBoundsFastPath?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah I see. That structure feels odd and unintuitive 🤔

maybe we should do an even larger refactoring 🤔 make normalize self ty + opaque types jank a sub-fn returning an Option for the "self ty is infer" case, and then use that directly.

Would need to think about the structure more myself, but I do think having a bunch of control flow inside of our functions makes code hard to understand and duplicating (parts of) the behavior may be better here.

if !candidates.is_empty() {
debug_assert_eq!(candidates.len(), 1);
return Ok(candidates[0].result);
}
// If the trait goal has been proven by using the environment, we want to treat
// aliases as rigid if there are no applicable projection bounds in the environment.
self.probe(|&result| ProbeKind::RigidAlias { result }).enter(|this| {
Expand Down Expand Up @@ -95,8 +107,8 @@ where
// Even when a trait bound has been proven using a where-bound, we
// still need to consider alias-bounds for normalization, see
// `tests/ui/next-solver/alias-bound-shadowed-by-env.rs`.
let (mut candidates, _) =
self.assemble_and_evaluate_candidates(goal, AssembleCandidatesFrom::EnvAndBounds);
let (mut candidates, _) = self
.assemble_and_evaluate_candidates(goal, AssembleCandidatesFrom::EnvAndBoundsFastPath);
debug!(?candidates);

if candidates.is_empty() {
Expand Down