Skip to content
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

Preserve substitutions when making trait obligations for suggestions #71618

Merged
merged 7 commits into from
May 24, 2020
33 changes: 22 additions & 11 deletions src/librustc_trait_selection/traits/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1000,12 +1000,15 @@ trait InferCtxtPrivExt<'tcx> {
trait_ref: &ty::PolyTraitRef<'tcx>,
);

fn mk_obligation_for_def_id(
/// Creates a `PredicateObligation` with `new_self_ty` replacing the existing type in the
/// `trait_ref`.
///
/// For this to work, `new_self_ty` must have no escaping bound variables.
fn mk_trait_obligation_with_new_self_ty(
&self,
def_id: DefId,
output_ty: Ty<'tcx>,
cause: ObligationCause<'tcx>,
param_env: ty::ParamEnv<'tcx>,
trait_ref: &ty::PolyTraitRef<'tcx>,
new_self_ty: Ty<'tcx>,
) -> PredicateObligation<'tcx>;

fn maybe_report_ambiguity(
Expand Down Expand Up @@ -1380,16 +1383,24 @@ impl<'a, 'tcx> InferCtxtPrivExt<'tcx> for InferCtxt<'a, 'tcx> {
}
}

fn mk_obligation_for_def_id(
fn mk_trait_obligation_with_new_self_ty(
&self,
def_id: DefId,
output_ty: Ty<'tcx>,
cause: ObligationCause<'tcx>,
param_env: ty::ParamEnv<'tcx>,
trait_ref: &ty::PolyTraitRef<'tcx>,
new_self_ty: Ty<'tcx>,
) -> PredicateObligation<'tcx> {
let new_trait_ref =
ty::TraitRef { def_id, substs: self.tcx.mk_substs_trait(output_ty, &[]) };
Obligation::new(cause, param_env, new_trait_ref.without_const().to_predicate(self.tcx))
assert!(!new_self_ty.has_escaping_bound_vars());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should comment this function and clarify that new_self_ty cannot have escaping bound variables


let trait_ref = trait_ref.map_bound_ref(|tr| ty::TraitRef {
substs: self.tcx.mk_substs_trait(new_self_ty, &tr.substs[1..]),
..*tr
});

Obligation::new(
ObligationCause::dummy(),
param_env,
trait_ref.without_const().to_predicate(self.tcx),
)
}

fn maybe_report_ambiguity(
Expand Down
62 changes: 31 additions & 31 deletions src/librustc_trait_selection/traits/error_reporting/suggestions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -532,14 +532,17 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
};
let msg = format!("use parentheses to call the {}", callable);

let obligation = self.mk_obligation_for_def_id(
trait_ref.def_id(),
output_ty.skip_binder(),
obligation.cause.clone(),
obligation.param_env,
);
// `mk_trait_obligation_with_new_self_ty` only works for types with no escaping bound
// variables, so bail out if we have any.
let output_ty = match output_ty.no_bound_vars() {
Some(ty) => ty,
None => return,
};

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

match self.evaluate_obligation(&obligation) {
match self.evaluate_obligation(&new_obligation) {
Ok(
EvaluationResult::EvaluatedToOk
| EvaluationResult::EvaluatedToOkModuloRegions
Expand Down Expand Up @@ -694,7 +697,6 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
err: &mut DiagnosticBuilder<'_>,
trait_ref: &ty::Binder<ty::TraitRef<'tcx>>,
) {
let trait_ref = trait_ref.skip_binder();
let span = obligation.cause.span;

if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
Expand All @@ -705,17 +707,16 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
return;
}

let mut trait_type = trait_ref.self_ty();
let mut suggested_ty = trait_ref.self_ty();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused how the types work out here. This is a Binders<Ty>, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a plain Ty if that helps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I noticed that recently. Hiding the skip_binder call like that is...just so wrong. But it explains how this code type-checks, anyway.


for refs_remaining in 0..refs_number {
if let ty::Ref(_, t_type, _) = trait_type.kind {
trait_type = t_type;
if let ty::Ref(_, inner_ty, _) = suggested_ty.kind {
Copy link
Contributor

Choose a reason for hiding this comment

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

...but I must be wrong, because how do we access the suggested_ty.kind then?

suggested_ty = inner_ty;

let new_obligation = self.mk_obligation_for_def_id(
trait_ref.def_id,
trait_type,
ObligationCause::dummy(),
let new_obligation = self.mk_trait_obligation_with_new_self_ty(
obligation.param_env,
trait_ref,
suggested_ty,
Comment on lines -715 to +719
Copy link
Contributor

Choose a reason for hiding this comment

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

ignoring the previous obligation entirely? We used to use the same cause, right?

Is this what causes the "note: std::convert::From<std::string::String> is implemented for &mut str, but not for &str" to go away?

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Apr 27, 2020

Choose a reason for hiding this comment

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

No. That comes from propagating substitutions correctly. From<String> is not implemented for any of &mut str or &str.

My impression was that the cause for an obligation is where it originates from in the source. There is no underlying cause for the new obligation we create, since we're just trying various "self" types for diagnostic hints. From this POV, obligations that are only for speculation should have no cause code.

);

if self.predicate_may_hold(&new_obligation) {
Expand Down Expand Up @@ -782,20 +783,20 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
return;
}

let trait_type = match mutability {
let suggested_ty = match mutability {
hir::Mutability::Mut => self.tcx.mk_imm_ref(region, t_type),
hir::Mutability::Not => self.tcx.mk_mut_ref(region, t_type),
};

let new_obligation = self.mk_obligation_for_def_id(
trait_ref.skip_binder().def_id,
trait_type,
ObligationCause::dummy(),
let new_obligation = self.mk_trait_obligation_with_new_self_ty(
obligation.param_env,
&trait_ref,
suggested_ty,
Copy link
Contributor

Choose a reason for hiding this comment

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

as above I don't 100% understand where this type is coming from or how we can be sure that it has no bound references in it -- doesn't it come from the trait_ref?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PolyTraitRef::self_ty is equivalent to self.skip_binder().substs.type_at(0). It seems we are assuming that the self type has no escaping bound variables. Is this assumption correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One other thing that I found confusing: According to the docs for TraitRef,

[it] introduces a level of region binding, to account for higher-ranked trait bounds like T: for<'a> Foo<&'a U> or higher-ranked object types.

If that's true, why do we use PolyTraitRef in so many places? Is it just for higher-ranked self types? If so, why is PolyTraitRef::self_ty, which calls skip_binder, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

These docs are wrong. Binders are always represented explicitly by a Binder -- so TraitRef alone cannot account for for<'a>, only a PolyTraitRef can.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this assumption correct?

I think this is sort of what I was asking. I doubt it's correct. I think a reasonable thing to do might be to test the assumption by using no_bound_vars instead of relying on skip_binder (and, if the assumption is false, not generating the suggestions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like we want to change PolyTraitRef::self_ty to return a Binder<Ty>. Is this correct? This will have a significant amount of fallout I think. Do you want me to open an issue to track this? I could check the result of self_ty for no_bound_vars in this PR, but we need to be doing this across all diagnostics code, not just here.

);

if self.evaluate_obligation_no_overflow(&new_obligation).must_apply_modulo_regions()
{
let suggested_ty_would_satisfy_obligation = self
.evaluate_obligation_no_overflow(&new_obligation)
.must_apply_modulo_regions();
if suggested_ty_would_satisfy_obligation {
let sp = self
.tcx
.sess
Expand All @@ -812,7 +813,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
err.note(&format!(
"`{}` is implemented for `{:?}`, but not for `{:?}`",
trait_ref.print_only_trait_path(),
trait_type,
suggested_ty,
trait_ref.skip_binder().self_ty(),
));
}
Expand Down Expand Up @@ -1891,7 +1892,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
span: Span,
) {
debug!(
"suggest_await_befor_try: obligation={:?}, span={:?}, trait_ref={:?}, trait_ref_self_ty={:?}",
"suggest_await_before_try: obligation={:?}, span={:?}, trait_ref={:?}, trait_ref_self_ty={:?}",
obligation,
span,
trait_ref,
Expand Down Expand Up @@ -1946,16 +1947,15 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
);

debug!(
"suggest_await_befor_try: normalized_projection_type {:?}",
"suggest_await_before_try: normalized_projection_type {:?}",
self.resolve_vars_if_possible(&normalized_ty)
);
let try_obligation = self.mk_obligation_for_def_id(
trait_ref.def_id(),
normalized_ty,
obligation.cause.clone(),
let try_obligation = self.mk_trait_obligation_with_new_self_ty(
obligation.param_env,
trait_ref,
normalized_ty,
);
debug!("suggest_await_befor_try: try_trait_obligation {:?}", try_obligation);
debug!("suggest_await_before_try: try_trait_obligation {:?}", try_obligation);
if self.predicate_may_hold(&try_obligation) && impls_future {
if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
if snippet.ends_with('?') {
Expand Down
1 change: 0 additions & 1 deletion src/test/ui/suggestions/into-str.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ LL | foo(String::new());
| ^^^ the trait `std::convert::From<std::string::String>` is not implemented for `&str`
|
= note: to coerce a `std::string::String` into a `&str`, use `&*` as a prefix
= note: `std::convert::From<std::string::String>` is implemented for `&mut str`, but not for `&str`
= note: required because of the requirements on the impl of `std::convert::Into<&str>` for `std::string::String`

error: aborting due to previous error
Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/suggestions/issue-71394-no-from-impl.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
fn main() {
let data: &[u8] = &[0; 10];
let _: &[i8] = data.into();
//~^ ERROR the trait bound `&[i8]: std::convert::From<&[u8]>` is not satisfied
}
11 changes: 11 additions & 0 deletions src/test/ui/suggestions/issue-71394-no-from-impl.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error[E0277]: the trait bound `&[i8]: std::convert::From<&[u8]>` is not satisfied
--> $DIR/issue-71394-no-from-impl.rs:3:25
|
LL | let _: &[i8] = data.into();
| ^^^^ the trait `std::convert::From<&[u8]>` is not implemented for `&[i8]`
|
= note: required because of the requirements on the impl of `std::convert::Into<&[i8]>` for `&[u8]`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.