Skip to content

Commit

Permalink
review
Browse files Browse the repository at this point in the history
  • Loading branch information
@lcnr
lcnr committed Sep 21, 2023
1 parent 8167a25 commit 614760f
Showing 1 changed file with 75 additions and 78 deletions.
153 changes: 75 additions & 78 deletions compiler/rustc_trait_selection/src/traits/coherence.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,76 +214,69 @@ fn overlap<'tcx>(
let mut obligations = equate_impl_headers(selcx.infcx, &impl1_header, &impl2_header)?;
debug!("overlap: unification check succeeded");

if !overlap_mode.use_implicit_negative() {
let impl_header = selcx.infcx.resolve_vars_if_possible(impl1_header);
return Some(OverlapResult {
impl_header,
intercrate_ambiguity_causes: Default::default(),
involves_placeholder: false,
});
};

obligations.extend(
[&impl1_header.predicates, &impl2_header.predicates].into_iter().flatten().map(
|&predicate| Obligation::new(infcx.tcx, ObligationCause::dummy(), param_env, predicate),
),
);

for mode in [TreatInductiveCycleAs::Ambig, TreatInductiveCycleAs::Recur] {
if let Some(failing_obligation) = selcx.with_treat_inductive_cycle_as(mode, |selcx| {
impl_intersection_has_impossible_obligation(selcx, &obligations)
}) {
if matches!(mode, TreatInductiveCycleAs::Recur) {
let first_local_impl = impl1_header
.impl_def_id
.as_local()
.or(impl2_header.impl_def_id.as_local())
.expect("expected one of the impls to be local");
infcx.tcx.struct_span_lint_hir(
COINDUCTIVE_OVERLAP_IN_COHERENCE,
infcx.tcx.local_def_id_to_hir_id(first_local_impl),
infcx.tcx.def_span(first_local_impl),
format!(
"implementations {} will conflict in the future",
match impl1_header.trait_ref {
Some(trait_ref) => {
let trait_ref = infcx.resolve_vars_if_possible(trait_ref);
format!(
"of `{}` for `{}`",
trait_ref.print_only_trait_path(),
trait_ref.self_ty()
)
}
None => format!(
"for `{}`",
infcx.resolve_vars_if_possible(impl1_header.self_ty)
),
if overlap_mode.use_implicit_negative() {
for mode in [TreatInductiveCycleAs::Ambig, TreatInductiveCycleAs::Recur] {
if let Some(failing_obligation) = selcx.with_treat_inductive_cycle_as(mode, |selcx| {
impl_intersection_has_impossible_obligation(selcx, &obligations)
}) {
if matches!(mode, TreatInductiveCycleAs::Recur) {
let first_local_impl = impl1_header
.impl_def_id
.as_local()
.or(impl2_header.impl_def_id.as_local())
.expect("expected one of the impls to be local");
infcx.tcx.struct_span_lint_hir(
COINDUCTIVE_OVERLAP_IN_COHERENCE,
infcx.tcx.local_def_id_to_hir_id(first_local_impl),
infcx.tcx.def_span(first_local_impl),
format!(
"implementations {} will conflict in the future",
match impl1_header.trait_ref {
Some(trait_ref) => {
let trait_ref = infcx.resolve_vars_if_possible(trait_ref);
format!(
"of `{}` for `{}`",
trait_ref.print_only_trait_path(),
trait_ref.self_ty()
)
}
None => format!(
"for `{}`",
infcx.resolve_vars_if_possible(impl1_header.self_ty)
),
},
),
|lint| {
lint.note(
"impls that are not considered to overlap may be considered to \
overlap in the future",
)
.span_label(
infcx.tcx.def_span(impl1_header.impl_def_id),
"the first impl is here",
)
.span_label(
infcx.tcx.def_span(impl2_header.impl_def_id),
"the second impl is here",
);
lint.note(format!(
"`{}` may be considered to hold in future releases, \
causing the impls to overlap",
infcx.resolve_vars_if_possible(failing_obligation.predicate)
));
lint
},
),
|lint| {
lint.note(
"impls that are not considered to overlap may be considered to \
overlap in the future",
)
.span_label(
infcx.tcx.def_span(impl1_header.impl_def_id),
"the first impl is here",
)
.span_label(
infcx.tcx.def_span(impl2_header.impl_def_id),
"the second impl is here",
);
lint.note(format!(
"`{}` may be considered to hold in future releases, \
causing the impls to overlap",
infcx.resolve_vars_if_possible(failing_obligation.predicate)
));
lint
},
);
}
);
}

return None;
return None;
}
}
}

Expand All @@ -294,7 +287,9 @@ fn overlap<'tcx>(
return None;
}

let intercrate_ambiguity_causes = if infcx.next_trait_solver() {
let intercrate_ambiguity_causes = if !overlap_mode.use_implicit_negative() {
Default::default()
} else if infcx.next_trait_solver() {
compute_intercrate_ambiguity_causes(&infcx, &obligations)
} else {
selcx.take_intercrate_ambiguity_causes()
Expand Down Expand Up @@ -932,6 +927,8 @@ impl<'a, 'tcx> ProofTreeVisitor<'tcx> for AmbiguityCausesVisitor<'a> {

let Goal { param_env, predicate } = goal.goal();

// For bound predicates we simply call `infcx.replace_bound_vars_with_placeholders`
// and then prove the resulting predicate as a nested goal.
let trait_ref = match predicate.kind().no_bound_vars() {
Some(ty::PredicateKind::Clause(ty::ClauseKind::Trait(tr))) => tr.trait_ref,
Some(ty::PredicateKind::Clause(ty::ClauseKind::Projection(proj))) => {
Expand All @@ -942,21 +939,6 @@ impl<'a, 'tcx> ProofTreeVisitor<'tcx> for AmbiguityCausesVisitor<'a> {

let mut ambiguity_cause = None;
for cand in goal.candidates() {
match cand.result() {
Ok(Certainty::Maybe(_)) => {}
// We only add intercrate ambiguity causes if the goal would
// otherwise result in an error.
//
// FIXME: this isn't quite right. Changing a goal from YES with
// inference contraints to AMBIGUOUS can also cause a goal to not
// fail.
Ok(Certainty::Yes) => {
ambiguity_cause = None;
break;
}
Err(NoSolution) => continue,
}

// FIXME: boiiii, using string comparisions here sure is scuffed.
if let inspect::ProbeKind::MiscCandidate { name: "coherence unknowable", result: _ } =
cand.kind()
Expand Down Expand Up @@ -1011,6 +993,21 @@ impl<'a, 'tcx> ProofTreeVisitor<'tcx> for AmbiguityCausesVisitor<'a> {
}
}
})
} else {
match cand.result() {
// We only add an ambiguity cause if the goal would otherwise
// result in an error.
//
// FIXME: While this matches the behavior of the
// old solver, it is not the only way in which the unknowable
// candidates *weaken* coherence, they can also force otherwise
// sucessful normalization to be ambiguous.
Ok(Certainty::Maybe(_) | Certainty::Yes) => {
ambiguity_cause = None;
break;
}
Err(NoSolution) => continue,
}
}
}

Expand Down

0 comments on commit 614760f

Please sign in to comment.