Skip to content

Commit

Permalink
Clean fix for rust-lang#96223
Browse files Browse the repository at this point in the history 
- Modified `InferCtxt::mk_trait_obligation_with_new_self_ty` to take as
  argument a `Binder<(TraitPredicate, Ty)>` instead of a
  `Binder<TraitPredicate>` and a separate `Ty` with no bound vars.

- Modified all call places to avoid calling `Binder::no_bounds_var` or
  `Binder::skip_binder` when it is not safe.
  • Loading branch information
@ricked-twice
ricked-twice committed May 17, 2022
1 parent 3655175 commit 4d7e014
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 115 deletions.
12 changes: 4 additions & 8 deletions compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1384,8 +1384,7 @@ trait InferCtxtPrivExt<'hir, 'tcx> {
fn mk_trait_obligation_with_new_self_ty(
&self,
param_env: ty::ParamEnv<'tcx>,
trait_ref: ty::PolyTraitPredicate<'tcx>,
new_self_ty: Ty<'tcx>,
trait_ref_and_ty: ty::Binder<'tcx, (ty::TraitPredicate<'tcx>, Ty<'tcx>)>,
) -> PredicateObligation<'tcx>;

fn maybe_report_ambiguity(
Expand Down Expand Up @@ -1923,14 +1922,11 @@ impl<'a, 'tcx> InferCtxtPrivExt<'a, 'tcx> for InferCtxt<'a, 'tcx> {
fn mk_trait_obligation_with_new_self_ty(
&self,
param_env: ty::ParamEnv<'tcx>,
trait_ref: ty::PolyTraitPredicate<'tcx>,
new_self_ty: Ty<'tcx>,
trait_ref_and_ty: ty::Binder<'tcx, (ty::TraitPredicate<'tcx>, Ty<'tcx>)>,
) -> PredicateObligation<'tcx> {
assert!(!new_self_ty.has_escaping_bound_vars());

let trait_pred = trait_ref.map_bound_ref(|tr| ty::TraitPredicate {
let trait_pred = trait_ref_and_ty.map_bound_ref(|(tr, new_self_ty)| ty::TraitPredicate {
trait_ref: ty::TraitRef {
substs: self.tcx.mk_substs_trait(new_self_ty, &tr.trait_ref.substs[1..]),
substs: self.tcx.mk_substs_trait(*new_self_ty, &tr.trait_ref.substs[1..]),
..tr.trait_ref
},
..*tr
Expand Down
224 changes: 117 additions & 107 deletions compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -637,8 +637,10 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
if let Some(steps) = autoderef.find_map(|(ty, steps)| {
// Re-add the `&`
let ty = self.tcx.mk_ref(region, TypeAndMut { ty, mutbl });
let obligation =
self.mk_trait_obligation_with_new_self_ty(param_env, real_trait_pred, ty);
let real_trait_pred_and_ty =
real_trait_pred.map_bound(|inner_trait_pred| (inner_trait_pred, ty));
let obligation = self
.mk_trait_obligation_with_new_self_ty(param_env, real_trait_pred_and_ty);
Some(steps).filter(|_| self.predicate_may_hold(&obligation))
}) {
if steps > 0 {
Expand All @@ -659,10 +661,11 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
}
} else if real_trait_pred != trait_pred {
// This branch addresses #87437.
let real_trait_pred_and_base_ty =
real_trait_pred.map_bound(|inner_trait_pred| (inner_trait_pred, base_ty));
let obligation = self.mk_trait_obligation_with_new_self_ty(
param_env,
real_trait_pred,
base_ty,
real_trait_pred_and_base_ty,
);
if self.predicate_may_hold(&obligation) {
err.span_suggestion_verbose(
Expand Down Expand Up @@ -720,9 +723,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
err: &mut Diagnostic,
trait_pred: ty::PolyTraitPredicate<'tcx>,
) -> bool {
let Some(self_ty) = trait_pred.self_ty().no_bound_vars() else {
return false;
};
let self_ty = trait_pred.self_ty().skip_binder();

let (def_id, output_ty, callable) = match *self_ty.kind() {
ty::Closure(def_id, substs) => (def_id, substs.as_closure().sig().output(), "closure"),
Expand All @@ -731,14 +732,12 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
};
let msg = format!("use parentheses to call the {}", callable);

// `mk_trait_obligation_with_new_self_ty` only works for types with no escaping bound
// variables, so bail out if we have any.
let Some(output_ty) = output_ty.no_bound_vars() else {
return false;
};
let output_ty = self.tcx.liberate_late_bound_regions(def_id, output_ty);

let trait_pred_and_self = trait_pred.map_bound(|trait_pred| (trait_pred, output_ty));

let new_obligation =
self.mk_trait_obligation_with_new_self_ty(obligation.param_env, trait_pred, output_ty);
self.mk_trait_obligation_with_new_self_ty(obligation.param_env, trait_pred_and_self);

match self.evaluate_obligation(&new_obligation) {
Ok(
Expand Down Expand Up @@ -842,96 +841,102 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
let param_env = obligation.param_env;

// Try to apply the original trait binding obligation by borrowing.
let mut try_borrowing = |old_pred: ty::PolyTraitPredicate<'tcx>,
blacklist: &[DefId]|
-> bool {
if blacklist.contains(&old_pred.def_id()) {
return false;
}
let mut try_borrowing =
|old_pred: ty::PolyTraitPredicate<'tcx>, blacklist: &[DefId]| -> bool {
if blacklist.contains(&old_pred.def_id()) {
return false;
}
// We map bounds to `&T` and `&mut T`
let trait_pred_and_imm_ref = old_pred.map_bound(|trait_pred| {
(
trait_pred,
self.tcx.mk_imm_ref(self.tcx.lifetimes.re_static, trait_pred.self_ty()),
)
});
let trait_pred_and_mut_ref = old_pred.map_bound(|trait_pred| {
(
trait_pred,
self.tcx.mk_mut_ref(self.tcx.lifetimes.re_static, trait_pred.self_ty()),
)
});

// This is a quick fix to resolve an ICE (#96223).
// This change should probably be deeper.
// As suggested by @jackh726, `mk_trait_obligation_with_new_self_ty` could take a `Binder<(TraitRef, Ty)>
// instead of `Binder<Ty>` leading to some changes to its call places.
let Some(orig_ty) = old_pred.self_ty().no_bound_vars() else {
return false;
};
let mk_result = |new_ty| {
let obligation =
self.mk_trait_obligation_with_new_self_ty(param_env, old_pred, new_ty);
self.predicate_must_hold_modulo_regions(&obligation)
};
let imm_result = mk_result(self.tcx.mk_imm_ref(self.tcx.lifetimes.re_static, orig_ty));
let mut_result = mk_result(self.tcx.mk_mut_ref(self.tcx.lifetimes.re_static, orig_ty));

if imm_result || mut_result {
if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
// We have a very specific type of error, where just borrowing this argument
// might solve the problem. In cases like this, the important part is the
// original type obligation, not the last one that failed, which is arbitrary.
// Because of this, we modify the error to refer to the original obligation and
// return early in the caller.

let msg = format!(
"the trait bound `{}: {}` is not satisfied",
orig_ty,
old_pred.print_modifiers_and_trait_path(),
);
if has_custom_message {
err.note(&msg);
} else {
err.message =
vec![(rustc_errors::DiagnosticMessage::Str(msg), Style::NoStyle)];
}
if snippet.starts_with('&') {
// This is already a literal borrow and the obligation is failing
// somewhere else in the obligation chain. Do not suggest non-sense.
return false;
}
err.span_label(
span,
&format!(
"expected an implementor of trait `{}`",
let mk_result = |trait_pred_and_new_ty| {
let obligation =
self.mk_trait_obligation_with_new_self_ty(param_env, trait_pred_and_new_ty);
self.predicate_must_hold_modulo_regions(&obligation)
};
let imm_result = mk_result(trait_pred_and_imm_ref);
let mut_result = mk_result(trait_pred_and_mut_ref);

if imm_result || mut_result {
if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
// We have a very specific type of error, where just borrowing this argument
// might solve the problem. In cases like this, the important part is the
// original type obligation, not the last one that failed, which is arbitrary.
// Because of this, we modify the error to refer to the original obligation and
// return early in the caller.

let msg = format!(
"the trait bound `{}: {}` is not satisfied",
// Safe to skip binder here
old_pred.self_ty().skip_binder(),
old_pred.print_modifiers_and_trait_path(),
),
);

// This if is to prevent a special edge-case
if matches!(
span.ctxt().outer_expn_data().kind,
ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop)
) {
// We don't want a borrowing suggestion on the fields in structs,
// ```
// struct Foo {
// the_foos: Vec<Foo>
// }
// ```

if imm_result && mut_result {
err.span_suggestions(
span.shrink_to_lo(),
"consider borrowing here",
["&".to_string(), "&mut ".to_string()].into_iter(),
Applicability::MaybeIncorrect,
);
);
if has_custom_message {
err.note(&msg);
} else {
err.span_suggestion_verbose(
span.shrink_to_lo(),
&format!(
"consider{} borrowing here",
if mut_result { " mutably" } else { "" }
),
format!("&{}", if mut_result { "mut " } else { "" }),
Applicability::MaybeIncorrect,
);
err.message =
vec![(rustc_errors::DiagnosticMessage::Str(msg), Style::NoStyle)];
}
if snippet.starts_with('&') {
// This is already a literal borrow and the obligation is failing
// somewhere else in the obligation chain. Do not suggest non-sense.
return false;
}
err.span_label(
span,
&format!(
"expected an implementor of trait `{}`",
old_pred.print_modifiers_and_trait_path(),
),
);

// This if is to prevent a special edge-case
if matches!(
span.ctxt().outer_expn_data().kind,
ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop)
) {
// We don't want a borrowing suggestion on the fields in structs,
// ```
// struct Foo {
// the_foos: Vec<Foo>
// }
// ```

if imm_result && mut_result {
err.span_suggestions(
span.shrink_to_lo(),
"consider borrowing here",
["&".to_string(), "&mut ".to_string()].into_iter(),
Applicability::MaybeIncorrect,
);
} else {
err.span_suggestion_verbose(
span.shrink_to_lo(),
&format!(
"consider{} borrowing here",
if mut_result { " mutably" } else { "" }
),
format!("&{}", if mut_result { "mut " } else { "" }),
Applicability::MaybeIncorrect,
);
}
}
return true;
}
return true;
}
}
return false;
};
return false;
};

if let ObligationCauseCode::ImplDerivedObligation(cause) = &*code {
try_borrowing(cause.derived.parent_trait_pred, &[])
Expand Down Expand Up @@ -992,20 +997,22 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
return false;
}

let Some(mut suggested_ty) = trait_pred.self_ty().no_bound_vars() else {
return false;
};
// We skip binder here
let mut suggested_ty = trait_pred.self_ty().skip_binder();

for refs_remaining in 0..refs_number {
let ty::Ref(_, inner_ty, _) = suggested_ty.kind() else {
break;
};
suggested_ty = *inner_ty;

// We remap bounds here
let trait_pred_and_suggested_ty =
trait_pred.map_bound(|trait_pred| (trait_pred, suggested_ty));

let new_obligation = self.mk_trait_obligation_with_new_self_ty(
obligation.param_env,
trait_pred,
suggested_ty,
trait_pred_and_suggested_ty,
);

if self.predicate_may_hold(&new_obligation) {
Expand Down Expand Up @@ -1141,10 +1148,12 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
hir::Mutability::Not => self.tcx.mk_mut_ref(region, t_type),
};

let trait_pred_and_suggested_ty =
trait_pred.map_bound(|trait_pred| (trait_pred, suggested_ty));

let new_obligation = self.mk_trait_obligation_with_new_self_ty(
obligation.param_env,
trait_pred,
suggested_ty,
trait_pred_and_suggested_ty,
);
let suggested_ty_would_satisfy_obligation = self
.evaluate_obligation_no_overflow(&new_obligation)
Expand Down Expand Up @@ -1195,7 +1204,9 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
// Only suggest this if the expression behind the semicolon implements the predicate
&& let Some(typeck_results) = self.in_progress_typeck_results
&& let Some(ty) = typeck_results.borrow().expr_ty_opt(expr)
&& self.predicate_may_hold(&self.mk_trait_obligation_with_new_self_ty(obligation.param_env, trait_pred, ty))
&& self.predicate_may_hold(&self.mk_trait_obligation_with_new_self_ty(
obligation.param_env, trait_pred.map_bound(|trait_pred| (trait_pred, ty))
))
{
err.span_label(
expr.span,
Expand Down Expand Up @@ -2727,8 +2738,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
);
let try_obligation = self.mk_trait_obligation_with_new_self_ty(
obligation.param_env,
trait_pred,
normalized_ty.ty().unwrap(),
trait_pred.map_bound(|trait_pred| (trait_pred, normalized_ty.ty().unwrap())),
);
debug!("suggest_await_before_try: try_trait_obligation {:?}", try_obligation);
if self.predicate_may_hold(&try_obligation)
Expand Down

0 comments on commit 4d7e014

Please sign in to comment.