Skip to content

Commit

Permalink
Rollup merge of #84014 - estebank:cool-bears-hot-tip, r=varkor
Browse files Browse the repository at this point in the history 
Improve trait/impl method discrepancy errors

* Use more accurate spans
* Clean up some code by removing previous hack
* Provide structured suggestions

Structured suggestions are particularly useful for cases where arbitrary self types are used, like in custom `Future`s, because the way to write `self: Pin<&mut Self>` is not necessarily self-evident when first encountered.
  • Loading branch information
@Dylan-DPC
Dylan-DPC authored Apr 11, 2021
2 parents b6780b3 + bb502c4 commit c905e9d
Show file tree
Hide file tree
Showing 17 changed files with 258 additions and 117 deletions.
13 changes: 8 additions & 5 deletions compiler/rustc_middle/src/ty/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ pub enum TypeError<'tcx> {
UnsafetyMismatch(ExpectedFound<hir::Unsafety>),
AbiMismatch(ExpectedFound<abi::Abi>),
Mutability,
ArgumentMutability(usize),
TupleSize(ExpectedFound<usize>),
FixedArraySize(ExpectedFound<u64>),
ArgCount,
Expand All @@ -46,6 +47,7 @@ pub enum TypeError<'tcx> {
RegionsPlaceholderMismatch,

Sorts(ExpectedFound<Ty<'tcx>>),
ArgumentSorts(ExpectedFound<Ty<'tcx>>, usize),
IntMismatch(ExpectedFound<ty::IntVarValue>),
FloatMismatch(ExpectedFound<ty::FloatTy>),
Traits(ExpectedFound<DefId>),
Expand Down Expand Up @@ -110,7 +112,7 @@ impl<'tcx> fmt::Display for TypeError<'tcx> {
AbiMismatch(values) => {
write!(f, "expected {} fn, found {} fn", values.expected, values.found)
}
Mutability => write!(f, "types differ in mutability"),
ArgumentMutability(_) | Mutability => write!(f, "types differ in mutability"),
TupleSize(values) => write!(
f,
"expected a tuple with {} element{}, \
Expand Down Expand Up @@ -142,7 +144,7 @@ impl<'tcx> fmt::Display for TypeError<'tcx> {
br_string(br)
),
RegionsPlaceholderMismatch => write!(f, "one type is more general than the other"),
Sorts(values) => ty::tls::with(|tcx| {
ArgumentSorts(values, _) | Sorts(values) => ty::tls::with(|tcx| {
report_maybe_different(
f,
&values.expected.sort_string(tcx),
Expand Down Expand Up @@ -199,10 +201,11 @@ impl<'tcx> TypeError<'tcx> {
use self::TypeError::*;
match self {
CyclicTy(_) | CyclicConst(_) | UnsafetyMismatch(_) | Mismatch | AbiMismatch(_)
| FixedArraySize(_) | Sorts(_) | IntMismatch(_) | FloatMismatch(_)
| VariadicMismatch(_) | TargetFeatureCast(_) => false,
| FixedArraySize(_) | ArgumentSorts(..) | Sorts(_) | IntMismatch(_)
| FloatMismatch(_) | VariadicMismatch(_) | TargetFeatureCast(_) => false,

Mutability
| ArgumentMutability(_)
| TupleSize(_)
| ArgCount
| RegionsDoesNotOutlive(..)
Expand Down Expand Up @@ -339,7 +342,7 @@ impl<'tcx> TyCtxt<'tcx> {
use self::TypeError::*;
debug!("note_and_explain_type_err err={:?} cause={:?}", err, cause);
match err {
Sorts(values) => {
ArgumentSorts(values, _) | Sorts(values) => {
match (values.expected.kind(), values.found.kind()) {
(ty::Closure(..), ty::Closure(..)) => {
db.note("no two closures, even if identical, have the same type");
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_middle/src/ty/relate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,12 @@ impl<'tcx> Relate<'tcx> for ty::FnSig<'tcx> {
} else {
relation.relate_with_variance(ty::Contravariant, a, b)
}
})
.enumerate()
.map(|(i, r)| match r {
Err(TypeError::Sorts(exp_found)) => Err(TypeError::ArgumentSorts(exp_found, i)),
Err(TypeError::Mutability) => Err(TypeError::ArgumentMutability(i)),
r => r,
});
Ok(ty::FnSig {
inputs_and_output: tcx.mk_type_list(inputs_and_output)?,
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_middle/src/ty/structural_impls.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,7 @@ impl<'a, 'tcx> Lift<'tcx> for ty::error::TypeError<'a> {
UnsafetyMismatch(x) => UnsafetyMismatch(x),
AbiMismatch(x) => AbiMismatch(x),
Mutability => Mutability,
ArgumentMutability(i) => ArgumentMutability(i),
TupleSize(x) => TupleSize(x),
FixedArraySize(x) => FixedArraySize(x),
ArgCount => ArgCount,
Expand All @@ -607,6 +608,7 @@ impl<'a, 'tcx> Lift<'tcx> for ty::error::TypeError<'a> {
CyclicTy(t) => return tcx.lift(t).map(|t| CyclicTy(t)),
CyclicConst(ct) => return tcx.lift(ct).map(|ct| CyclicConst(ct)),
ProjectionMismatched(x) => ProjectionMismatched(x),
ArgumentSorts(x, i) => return tcx.lift(x).map(|x| ArgumentSorts(x, i)),
Sorts(x) => return tcx.lift(x).map(Sorts),
ExistentialMismatch(x) => return tcx.lift(x).map(ExistentialMismatch),
ConstMismatch(x) => return tcx.lift(x).map(ConstMismatch),
Expand Down
168 changes: 87 additions & 81 deletions compiler/rustc_typeck/src/check/compare_method.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,9 +278,8 @@ fn compare_predicate_entailment<'tcx>(
if let Err(terr) = sub_result {
debug!("sub_types failed: impl ty {:?}, trait ty {:?}", impl_fty, trait_fty);

let (impl_err_span, trait_err_span) = extract_spans_for_error_reporting(
&infcx, param_env, &terr, &cause, impl_m, impl_sig, trait_m, trait_sig,
);
let (impl_err_span, trait_err_span) =
extract_spans_for_error_reporting(&infcx, &terr, &cause, impl_m, trait_m);

cause.make_mut().span = impl_err_span;

Expand All @@ -291,18 +290,79 @@ fn compare_predicate_entailment<'tcx>(
"method `{}` has an incompatible type for trait",
trait_m.ident
);
if let TypeError::Mutability = terr {
if let Some(trait_err_span) = trait_err_span {
if let Ok(trait_err_str) = tcx.sess.source_map().span_to_snippet(trait_err_span)
match &terr {
TypeError::ArgumentMutability(0) | TypeError::ArgumentSorts(_, 0)
if trait_m.fn_has_self_parameter =>
{
let ty = trait_sig.inputs()[0];
let sugg = match ExplicitSelf::determine(ty, |_| ty == impl_trait_ref.self_ty())
{
ExplicitSelf::ByValue => "self".to_owned(),
ExplicitSelf::ByReference(_, hir::Mutability::Not) => "&self".to_owned(),
ExplicitSelf::ByReference(_, hir::Mutability::Mut) => {
"&mut self".to_owned()
}
_ => format!("self: {}", ty),
};

// When the `impl` receiver is an arbitrary self type, like `self: Box<Self>`, the
// span points only at the type `Box<Self`>, but we want to cover the whole
// argument pattern and type.
let impl_m_hir_id =
tcx.hir().local_def_id_to_hir_id(impl_m.def_id.expect_local());
let span = match tcx.hir().expect_impl_item(impl_m_hir_id).kind {
ImplItemKind::Fn(ref sig, body) => tcx
.hir()
.body_param_names(body)
.zip(sig.decl.inputs.iter())
.map(|(param, ty)| param.span.to(ty.span))
.next()
.unwrap_or(impl_err_span),
_ => bug!("{:?} is not a method", impl_m),
};

diag.span_suggestion(
span,
"change the self-receiver type to match the trait",
sugg,
Applicability::MachineApplicable,
);
}
TypeError::ArgumentMutability(i) | TypeError::ArgumentSorts(_, i) => {
if trait_sig.inputs().len() == *i {
// Suggestion to change output type. We do not suggest in `async` functions
// to avoid complex logic or incorrect output.
let impl_m_hir_id =
tcx.hir().local_def_id_to_hir_id(impl_m.def_id.expect_local());
match tcx.hir().expect_impl_item(impl_m_hir_id).kind {
ImplItemKind::Fn(ref sig, _)
if sig.header.asyncness == hir::IsAsync::NotAsync =>
{
let msg = "change the output type to match the trait";
let ap = Applicability::MachineApplicable;
match sig.decl.output {
hir::FnRetTy::DefaultReturn(sp) => {
let sugg = format!("-> {} ", trait_sig.output());
diag.span_suggestion_verbose(sp, msg, sugg, ap);
}
hir::FnRetTy::Return(hir_ty) => {
let sugg = trait_sig.output().to_string();
diag.span_suggestion(hir_ty.span, msg, sugg, ap);
}
};
}
_ => {}
};
} else if let Some(trait_ty) = trait_sig.inputs().get(*i) {
diag.span_suggestion(
impl_err_span,
"consider changing the mutability to match the trait",
trait_err_str,
"change the parameter type to match the trait",
trait_ty.to_string(),
Applicability::MachineApplicable,
);
}
}
_ => {}
}

infcx.note_type_err(
Expand Down Expand Up @@ -385,86 +445,35 @@ fn check_region_bounds_on_impl_item<'tcx>(

fn extract_spans_for_error_reporting<'a, 'tcx>(
infcx: &infer::InferCtxt<'a, 'tcx>,
param_env: ty::ParamEnv<'tcx>,
terr: &TypeError<'_>,
cause: &ObligationCause<'tcx>,
impl_m: &ty::AssocItem,
impl_sig: ty::FnSig<'tcx>,
trait_m: &ty::AssocItem,
trait_sig: ty::FnSig<'tcx>,
) -> (Span, Option<Span>) {
let tcx = infcx.tcx;
let impl_m_hir_id = tcx.hir().local_def_id_to_hir_id(impl_m.def_id.expect_local());
let (impl_m_output, impl_m_iter) = match tcx.hir().expect_impl_item(impl_m_hir_id).kind {
ImplItemKind::Fn(ref impl_m_sig, _) => {
(&impl_m_sig.decl.output, impl_m_sig.decl.inputs.iter())
let mut impl_args = match tcx.hir().expect_impl_item(impl_m_hir_id).kind {
ImplItemKind::Fn(ref sig, _) => {
sig.decl.inputs.iter().map(|t| t.span).chain(iter::once(sig.decl.output.span()))
}
_ => bug!("{:?} is not a method", impl_m),
};

match *terr {
TypeError::Mutability => {
if let Some(def_id) = trait_m.def_id.as_local() {
let trait_m_hir_id = tcx.hir().local_def_id_to_hir_id(def_id);
let trait_m_iter = match tcx.hir().expect_trait_item(trait_m_hir_id).kind {
TraitItemKind::Fn(ref trait_m_sig, _) => trait_m_sig.decl.inputs.iter(),
_ => bug!("{:?} is not a TraitItemKind::Fn", trait_m),
};

iter::zip(impl_m_iter, trait_m_iter)
.find(|&(ref impl_arg, ref trait_arg)| {
match (&impl_arg.kind, &trait_arg.kind) {
(
&hir::TyKind::Rptr(_, ref impl_mt),
&hir::TyKind::Rptr(_, ref trait_mt),
)
| (&hir::TyKind::Ptr(ref impl_mt), &hir::TyKind::Ptr(ref trait_mt)) => {
impl_mt.mutbl != trait_mt.mutbl
}
_ => false,
}
})
.map(|(ref impl_arg, ref trait_arg)| (impl_arg.span, Some(trait_arg.span)))
.unwrap_or_else(|| (cause.span(tcx), tcx.hir().span_if_local(trait_m.def_id)))
} else {
(cause.span(tcx), tcx.hir().span_if_local(trait_m.def_id))
let trait_args = trait_m.def_id.as_local().map(|def_id| {
let trait_m_hir_id = tcx.hir().local_def_id_to_hir_id(def_id);
match tcx.hir().expect_trait_item(trait_m_hir_id).kind {
TraitItemKind::Fn(ref sig, _) => {
sig.decl.inputs.iter().map(|t| t.span).chain(iter::once(sig.decl.output.span()))
}
_ => bug!("{:?} is not a TraitItemKind::Fn", trait_m),
}
TypeError::Sorts(ExpectedFound { .. }) => {
if let Some(def_id) = trait_m.def_id.as_local() {
let trait_m_hir_id = tcx.hir().local_def_id_to_hir_id(def_id);
let (trait_m_output, trait_m_iter) =
match tcx.hir().expect_trait_item(trait_m_hir_id).kind {
TraitItemKind::Fn(ref trait_m_sig, _) => {
(&trait_m_sig.decl.output, trait_m_sig.decl.inputs.iter())
}
_ => bug!("{:?} is not a TraitItemKind::Fn", trait_m),
};
});

let impl_iter = impl_sig.inputs().iter();
let trait_iter = trait_sig.inputs().iter();
iter::zip(iter::zip(impl_iter, trait_iter), iter::zip(impl_m_iter, trait_m_iter))
.find_map(|((&impl_arg_ty, &trait_arg_ty), (impl_arg, trait_arg))| match infcx
.at(&cause, param_env)
.sub(trait_arg_ty, impl_arg_ty)
{
Ok(_) => None,
Err(_) => Some((impl_arg.span, Some(trait_arg.span))),
})
.unwrap_or_else(|| {
if infcx
.at(&cause, param_env)
.sup(trait_sig.output(), impl_sig.output())
.is_err()
{
(impl_m_output.span(), Some(trait_m_output.span()))
} else {
(cause.span(tcx), tcx.hir().span_if_local(trait_m.def_id))
}
})
} else {
(cause.span(tcx), tcx.hir().span_if_local(trait_m.def_id))
}
match *terr {
TypeError::ArgumentMutability(i) => {
(impl_args.nth(i).unwrap(), trait_args.and_then(|mut args| args.nth(i)))
}
TypeError::ArgumentSorts(ExpectedFound { .. }, i) => {
(impl_args.nth(i).unwrap(), trait_args.and_then(|mut args| args.nth(i)))
}
_ => (cause.span(tcx), tcx.hir().span_if_local(trait_m.def_id)),
}
Expand Down Expand Up @@ -514,8 +523,7 @@ fn compare_self_type<'tcx>(
tcx.sess,
impl_m_span,
E0185,
"method `{}` has a `{}` declaration in the impl, but \
not in the trait",
"method `{}` has a `{}` declaration in the impl, but not in the trait",
trait_m.ident,
self_descr
);
Expand All @@ -535,8 +543,7 @@ fn compare_self_type<'tcx>(
tcx.sess,
impl_m_span,
E0186,
"method `{}` has a `{}` declaration in the trait, but \
not in the impl",
"method `{}` has a `{}` declaration in the trait, but not in the impl",
trait_m.ident,
self_descr
);
Expand Down Expand Up @@ -993,8 +1000,7 @@ crate fn compare_const_impl<'tcx>(
tcx.sess,
cause.span,
E0326,
"implemented const `{}` has an incompatible type for \
trait",
"implemented const `{}` has an incompatible type for trait",
trait_c.ident
);

Expand Down
10 changes: 8 additions & 2 deletions src/test/ui/associated-types/defaults-specialization.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ LL | fn make() -> Self::Ty {
| -------- type in trait
...
LL | fn make() -> u8 { 0 }
| ^^ expected associated type, found `u8`
| ^^
| |
| expected associated type, found `u8`
| help: change the output type to match the trait: `<A<T> as Tr>::Ty`
|
= note: expected fn pointer `fn() -> <A<T> as Tr>::Ty`
found fn pointer `fn() -> u8`
Expand All @@ -30,7 +33,10 @@ LL | default type Ty = bool;
| ----------------------- expected this associated type
LL |
LL | fn make() -> bool { true }
| ^^^^ expected associated type, found `bool`
| ^^^^
| |
| expected associated type, found `bool`
| help: change the output type to match the trait: `<B<T> as Tr>::Ty`
|
= note: expected fn pointer `fn() -> <B<T> as Tr>::Ty`
found fn pointer `fn() -> bool`
Expand Down
26 changes: 26 additions & 0 deletions src/test/ui/compare-method/bad-self-type.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
use std::future::Future;
use std::task::{Context, Poll};

fn main() {}

struct MyFuture {}

impl Future for MyFuture {
type Output = ();
fn poll(self, _: &mut Context<'_>) -> Poll<()> {
//~^ ERROR method `poll` has an incompatible type for trait
todo!()
}
}

trait T {
fn foo(self);
fn bar(self) -> Option<()>;
}

impl T for MyFuture {
fn foo(self: Box<Self>) {}
//~^ ERROR method `foo` has an incompatible type for trait
fn bar(self) {}
//~^ ERROR method `bar` has an incompatible type for trait
}
Loading

0 comments on commit c905e9d

Please sign in to comment.