From 5aab1a9a88dd30d622a5303f26d9ad213c551473 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 23 Jun 2020 17:32:06 -0700 Subject: [PATCH 1/3] Tweak binop errors * Suggest potentially missing binop trait bound (fix #73416) * Use structured suggestion for dereference in binop --- src/librustc_typeck/check/op.rs | 135 +++++++++++++----- src/test/ui/binary-op-on-double-ref.fixed | 9 ++ src/test/ui/binary-op-on-double-ref.rs | 1 + src/test/ui/binary-op-on-double-ref.stderr | 7 +- src/test/ui/issues/issue-35668.stderr | 6 + src/test/ui/issues/issue-5239-1.stderr | 2 +- .../missing-trait-bound-for-op.fixed | 13 ++ .../suggestions/missing-trait-bound-for-op.rs | 13 ++ .../missing-trait-bound-for-op.stderr | 17 +++ .../trait-resolution-in-overloaded-op.stderr | 6 + 10 files changed, 169 insertions(+), 40 deletions(-) create mode 100644 src/test/ui/binary-op-on-double-ref.fixed create mode 100644 src/test/ui/suggestions/missing-trait-bound-for-op.fixed create mode 100644 src/test/ui/suggestions/missing-trait-bound-for-op.rs create mode 100644 src/test/ui/suggestions/missing-trait-bound-for-op.stderr diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs index fe5087091164..7acf88431857 100644 --- a/src/librustc_typeck/check/op.rs +++ b/src/librustc_typeck/check/op.rs @@ -9,7 +9,9 @@ use rustc_middle::ty::adjustment::{ Adjust, Adjustment, AllowTwoPhase, AutoBorrow, AutoBorrowMutability, }; use rustc_middle::ty::TyKind::{Adt, Array, Char, FnDef, Never, Ref, Str, Tuple, Uint}; -use rustc_middle::ty::{self, suggest_constraining_type_param, Ty, TyCtxt, TypeFoldable}; +use rustc_middle::ty::{ + self, suggest_constraining_type_param, Ty, TyCtxt, TypeFoldable, TypeVisitor, +}; use rustc_span::symbol::Ident; use rustc_span::Span; use rustc_trait_selection::infer::InferCtxtExt; @@ -254,6 +256,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if !lhs_ty.references_error() && !rhs_ty.references_error() { let source_map = self.tcx.sess.source_map(); + let note = |err: &mut DiagnosticBuilder<'_>, missing_trait| { + err.note(&format!( + "the trait `{}` is not implemented for `{}`", + missing_trait, lhs_ty + )); + }; match is_assign { IsAssign::Yes => { let mut err = struct_span_err!( @@ -286,10 +294,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { rty.peel_refs(), lstring, ); - err.span_suggestion( - lhs_expr.span, + err.span_suggestion_verbose( + lhs_expr.span.shrink_to_lo(), msg, - format!("*{}", lstring), + "*".to_string(), rustc_errors::Applicability::MachineApplicable, ); suggested_deref = true; @@ -310,6 +318,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { _ => None, }; if let Some(missing_trait) = missing_trait { + let mut visitor = TypeParamVisitor(vec![]); + visitor.visit_ty(lhs_ty); + + let mut sugg = false; if op.node == hir::BinOpKind::Add && self.check_str_addition( lhs_expr, rhs_expr, lhs_ty, rhs_ty, &mut err, true, op, @@ -318,18 +330,33 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // This has nothing here because it means we did string // concatenation (e.g., "Hello " += "World!"). This means // we don't want the note in the else clause to be emitted - } else if let ty::Param(p) = lhs_ty.kind { - suggest_constraining_param( - self.tcx, - self.body_id, - &mut err, - lhs_ty, - rhs_ty, - missing_trait, - p, - false, - ); - } else if !suggested_deref { + sugg = true; + } else if let [ty] = &visitor.0[..] { + if let ty::Param(p) = ty.kind { + // FIXME: This *guesses* that constraining the type param + // will make the operation available, but this is only true + // when the corresponding trait has a blanked + // implementation, like the following: + // `impl<'a> PartialEq for &'a [T] where T: PartialEq {}` + // The correct thing to do would be to verify this + // projection would hold. + if *ty != lhs_ty { + note(&mut err, missing_trait); + } + suggest_constraining_param( + self.tcx, + self.body_id, + &mut err, + ty, + rhs_ty, + missing_trait, + p, + false, + ); + sugg = true; + } + } + if !sugg && !suggested_deref { suggest_impl_missing(&mut err, lhs_ty, &missing_trait); } } @@ -458,18 +485,27 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { .is_ok() } { if let Ok(lstring) = source_map.span_to_snippet(lhs_expr.span) { - err.help(&format!( - "`{}` can be used on '{}', you can \ - dereference `{2}`: `*{2}`", - op.node.as_str(), - rty.peel_refs(), - lstring - )); + err.span_suggestion_verbose( + lhs_expr.span.shrink_to_lo(), + &format!( + "`{}` can be used on `{}`, you can dereference \ + `{}`", + op.node.as_str(), + rty.peel_refs(), + lstring, + ), + "*".to_string(), + Applicability::MachineApplicable, + ); suggested_deref = true; } } } if let Some(missing_trait) = missing_trait { + let mut visitor = TypeParamVisitor(vec![]); + visitor.visit_ty(lhs_ty); + + let mut sugg = false; if op.node == hir::BinOpKind::Add && self.check_str_addition( lhs_expr, rhs_expr, lhs_ty, rhs_ty, &mut err, false, op, @@ -478,18 +514,33 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // This has nothing here because it means we did string // concatenation (e.g., "Hello " + "World!"). This means // we don't want the note in the else clause to be emitted - } else if let ty::Param(p) = lhs_ty.kind { - suggest_constraining_param( - self.tcx, - self.body_id, - &mut err, - lhs_ty, - rhs_ty, - missing_trait, - p, - use_output, - ); - } else if !suggested_deref && !involves_fn { + sugg = true; + } else if let [ty] = &visitor.0[..] { + if let ty::Param(p) = ty.kind { + // FIXME: This *guesses* that constraining the type param + // will make the operation available, but this is only true + // when the corresponding trait has a blanked + // implementation, like the following: + // `impl<'a> PartialEq for &'a [T] where T: PartialEq {}` + // The correct thing to do would be to verify this + // projection would hold. + if *ty != lhs_ty { + note(&mut err, missing_trait); + } + suggest_constraining_param( + self.tcx, + self.body_id, + &mut err, + ty, + rhs_ty, + missing_trait, + p, + use_output, + ); + sugg = true; + } + } + if !sugg && !suggested_deref && !involves_fn { suggest_impl_missing(&mut err, lhs_ty, &missing_trait); } } @@ -928,8 +979,7 @@ fn suggest_impl_missing(err: &mut DiagnosticBuilder<'_>, ty: Ty<'_>, missing_tra if let Adt(def, _) = ty.peel_refs().kind { if def.did.is_local() { err.note(&format!( - "an implementation of `{}` might \ - be missing for `{}`", + "an implementation of `{}` might be missing for `{}`", missing_trait, ty )); } @@ -975,3 +1025,14 @@ fn suggest_constraining_param( err.span_label(span, msg); } } + +struct TypeParamVisitor<'tcx>(Vec>); + +impl<'tcx> TypeVisitor<'tcx> for TypeParamVisitor<'tcx> { + fn visit_ty(&mut self, ty: Ty<'tcx>) -> bool { + if let ty::Param(_) = ty.kind { + self.0.push(ty); + } + ty.super_visit_with(self) + } +} diff --git a/src/test/ui/binary-op-on-double-ref.fixed b/src/test/ui/binary-op-on-double-ref.fixed new file mode 100644 index 000000000000..de9dc19af29b --- /dev/null +++ b/src/test/ui/binary-op-on-double-ref.fixed @@ -0,0 +1,9 @@ +// run-rustfix +fn main() { + let v = vec![1, 2, 3, 4, 5, 6, 7, 8, 9]; + let vr = v.iter().filter(|x| { + *x % 2 == 0 + //~^ ERROR cannot mod `&&{integer}` by `{integer}` + }); + println!("{:?}", vr); +} diff --git a/src/test/ui/binary-op-on-double-ref.rs b/src/test/ui/binary-op-on-double-ref.rs index 67e01b9327db..2616c560cbef 100644 --- a/src/test/ui/binary-op-on-double-ref.rs +++ b/src/test/ui/binary-op-on-double-ref.rs @@ -1,3 +1,4 @@ +// run-rustfix fn main() { let v = vec![1, 2, 3, 4, 5, 6, 7, 8, 9]; let vr = v.iter().filter(|x| { diff --git a/src/test/ui/binary-op-on-double-ref.stderr b/src/test/ui/binary-op-on-double-ref.stderr index 6c405333ec68..02b0488488c5 100644 --- a/src/test/ui/binary-op-on-double-ref.stderr +++ b/src/test/ui/binary-op-on-double-ref.stderr @@ -1,12 +1,15 @@ error[E0369]: cannot mod `&&{integer}` by `{integer}` - --> $DIR/binary-op-on-double-ref.rs:4:11 + --> $DIR/binary-op-on-double-ref.rs:5:11 | LL | x % 2 == 0 | - ^ - {integer} | | | &&{integer} | - = help: `%` can be used on '{integer}', you can dereference `x`: `*x` +help: `%` can be used on `{integer}`, you can dereference `x` + | +LL | *x % 2 == 0 + | ^ error: aborting due to previous error diff --git a/src/test/ui/issues/issue-35668.stderr b/src/test/ui/issues/issue-35668.stderr index 98e8e6366b99..c8f1f88a4708 100644 --- a/src/test/ui/issues/issue-35668.stderr +++ b/src/test/ui/issues/issue-35668.stderr @@ -5,6 +5,12 @@ LL | a.iter().map(|a| a*a) | -^- &T | | | &T + | + = note: the trait `std::ops::Mul` is not implemented for `&T` +help: consider restricting type parameter `T` + | +LL | fn func<'a, T: std::ops::Mul>(a: &'a [T]) -> impl Iterator { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: aborting due to previous error diff --git a/src/test/ui/issues/issue-5239-1.stderr b/src/test/ui/issues/issue-5239-1.stderr index f4f0f17d0019..d0c71e73463c 100644 --- a/src/test/ui/issues/issue-5239-1.stderr +++ b/src/test/ui/issues/issue-5239-1.stderr @@ -9,7 +9,7 @@ LL | let x = |ref x: isize| { x += 1; }; help: `+=` can be used on 'isize', you can dereference `x` | LL | let x = |ref x: isize| { *x += 1; }; - | ^^ + | ^ error: aborting due to previous error diff --git a/src/test/ui/suggestions/missing-trait-bound-for-op.fixed b/src/test/ui/suggestions/missing-trait-bound-for-op.fixed new file mode 100644 index 000000000000..02886bb845cf --- /dev/null +++ b/src/test/ui/suggestions/missing-trait-bound-for-op.fixed @@ -0,0 +1,13 @@ +// run-rustfix + +pub fn strip_prefix<'a, T: std::cmp::PartialEq>(s: &'a [T], prefix: &[T]) -> Option<&'a [T]> { + let n = prefix.len(); + if n <= s.len() { + let (head, tail) = s.split_at(n); + if head == prefix { //~ ERROR binary operation `==` cannot be applied to type `&[T]` + return Some(tail); + } + } + None +} +fn main() {} diff --git a/src/test/ui/suggestions/missing-trait-bound-for-op.rs b/src/test/ui/suggestions/missing-trait-bound-for-op.rs new file mode 100644 index 000000000000..aa4ef467360d --- /dev/null +++ b/src/test/ui/suggestions/missing-trait-bound-for-op.rs @@ -0,0 +1,13 @@ +// run-rustfix + +pub fn strip_prefix<'a, T>(s: &'a [T], prefix: &[T]) -> Option<&'a [T]> { + let n = prefix.len(); + if n <= s.len() { + let (head, tail) = s.split_at(n); + if head == prefix { //~ ERROR binary operation `==` cannot be applied to type `&[T]` + return Some(tail); + } + } + None +} +fn main() {} diff --git a/src/test/ui/suggestions/missing-trait-bound-for-op.stderr b/src/test/ui/suggestions/missing-trait-bound-for-op.stderr new file mode 100644 index 000000000000..dab4e575be1f --- /dev/null +++ b/src/test/ui/suggestions/missing-trait-bound-for-op.stderr @@ -0,0 +1,17 @@ +error[E0369]: binary operation `==` cannot be applied to type `&[T]` + --> $DIR/missing-trait-bound-for-op.rs:7:17 + | +LL | if head == prefix { + | ---- ^^ ------ &[T] + | | + | &[T] + | + = note: the trait `std::cmp::PartialEq` is not implemented for `&[T]` +help: consider restricting type parameter `T` + | +LL | pub fn strip_prefix<'a, T: std::cmp::PartialEq>(s: &'a [T], prefix: &[T]) -> Option<&'a [T]> { + | ^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0369`. diff --git a/src/test/ui/traits/trait-resolution-in-overloaded-op.stderr b/src/test/ui/traits/trait-resolution-in-overloaded-op.stderr index 29216f36f5f3..1c424ce7da66 100644 --- a/src/test/ui/traits/trait-resolution-in-overloaded-op.stderr +++ b/src/test/ui/traits/trait-resolution-in-overloaded-op.stderr @@ -5,6 +5,12 @@ LL | a * b | - ^ - f64 | | | &T + | + = note: the trait `std::ops::Mul` is not implemented for `&T` +help: consider further restricting this bound + | +LL | fn foo + std::ops::Mul>(a: &T, b: f64) -> f64 { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: aborting due to previous error From 09af1845d7b43c0a17c1b98f6be92c2c0fbc202b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 24 Jun 2020 14:16:41 -0700 Subject: [PATCH 2/3] review comments: clean up code * deduplicate logic * fix typos * remove unnecessary state --- src/librustc_typeck/check/method/mod.rs | 3 +- src/librustc_typeck/check/method/probe.rs | 4 +- src/librustc_typeck/check/op.rs | 502 +++++++++------------- src/test/ui/issues/issue-5239-1.stderr | 2 +- 4 files changed, 214 insertions(+), 297 deletions(-) diff --git a/src/librustc_typeck/check/method/mod.rs b/src/librustc_typeck/check/method/mod.rs index ac3fa15417e9..ad980fd2f121 100644 --- a/src/librustc_typeck/check/method/mod.rs +++ b/src/librustc_typeck/check/method/mod.rs @@ -296,8 +296,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { opt_input_types: Option<&[Ty<'tcx>]>, ) -> Option>> { debug!( - "lookup_in_trait_adjusted(self_ty={:?}, \ - m_name={}, trait_def_id={:?})", + "lookup_in_trait_adjusted(self_ty={:?}, m_name={}, trait_def_id={:?})", self_ty, m_name, trait_def_id ); diff --git a/src/librustc_typeck/check/method/probe.rs b/src/librustc_typeck/check/method/probe.rs index 93bcd5cf2914..093f76be0086 100644 --- a/src/librustc_typeck/check/method/probe.rs +++ b/src/librustc_typeck/check/method/probe.rs @@ -379,8 +379,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self.tcx.sess, span, E0699, - "the type of this value must be known \ - to call a method on a raw pointer on it" + "the type of this value must be known to call a method on a raw pointer on \ + it" ) .emit(); } else { diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs index 7acf88431857..0184c00c475b 100644 --- a/src/librustc_typeck/check/op.rs +++ b/src/librustc_typeck/check/op.rs @@ -251,303 +251,222 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { method.sig.output() } + // error types are considered "builtin" + Err(()) if lhs_ty.references_error() || rhs_ty.references_error() => { + self.tcx.ty_error() + } Err(()) => { - // error types are considered "builtin" - if !lhs_ty.references_error() && !rhs_ty.references_error() { - let source_map = self.tcx.sess.source_map(); - - let note = |err: &mut DiagnosticBuilder<'_>, missing_trait| { - err.note(&format!( - "the trait `{}` is not implemented for `{}`", - missing_trait, lhs_ty - )); - }; - match is_assign { - IsAssign::Yes => { - let mut err = struct_span_err!( - self.tcx.sess, - expr.span, - E0368, - "binary assignment operation `{}=` cannot be applied to type `{}`", - op.node.as_str(), + let source_map = self.tcx.sess.source_map(); + let (mut err, missing_trait, use_output, involves_fn) = match is_assign { + IsAssign::Yes => { + let mut err = struct_span_err!( + self.tcx.sess, + expr.span, + E0368, + "binary assignment operation `{}=` cannot be applied to type `{}`", + op.node.as_str(), + lhs_ty, + ); + err.span_label( + lhs_expr.span, + format!("cannot use `{}=` on type `{}`", op.node.as_str(), lhs_ty), + ); + let missing_trait = match op.node { + hir::BinOpKind::Add => Some("std::ops::AddAssign"), + hir::BinOpKind::Sub => Some("std::ops::SubAssign"), + hir::BinOpKind::Mul => Some("std::ops::MulAssign"), + hir::BinOpKind::Div => Some("std::ops::DivAssign"), + hir::BinOpKind::Rem => Some("std::ops::RemAssign"), + hir::BinOpKind::BitAnd => Some("std::ops::BitAndAssign"), + hir::BinOpKind::BitXor => Some("std::ops::BitXorAssign"), + hir::BinOpKind::BitOr => Some("std::ops::BitOrAssign"), + hir::BinOpKind::Shl => Some("std::ops::ShlAssign"), + hir::BinOpKind::Shr => Some("std::ops::ShrAssign"), + _ => None, + }; + (err, missing_trait, false, false) + } + IsAssign::No => { + let (message, missing_trait, use_output) = match op.node { + hir::BinOpKind::Add => ( + format!("cannot add `{}` to `{}`", rhs_ty, lhs_ty), + Some("std::ops::Add"), + true, + ), + hir::BinOpKind::Sub => ( + format!("cannot subtract `{}` from `{}`", rhs_ty, lhs_ty), + Some("std::ops::Sub"), + true, + ), + hir::BinOpKind::Mul => ( + format!("cannot multiply `{}` to `{}`", rhs_ty, lhs_ty), + Some("std::ops::Mul"), + true, + ), + hir::BinOpKind::Div => ( + format!("cannot divide `{}` by `{}`", lhs_ty, rhs_ty), + Some("std::ops::Div"), + true, + ), + hir::BinOpKind::Rem => ( + format!("cannot mod `{}` by `{}`", lhs_ty, rhs_ty), + Some("std::ops::Rem"), + true, + ), + hir::BinOpKind::BitAnd => ( + format!("no implementation for `{} & {}`", lhs_ty, rhs_ty), + Some("std::ops::BitAnd"), + true, + ), + hir::BinOpKind::BitXor => ( + format!("no implementation for `{} ^ {}`", lhs_ty, rhs_ty), + Some("std::ops::BitXor"), + true, + ), + hir::BinOpKind::BitOr => ( + format!("no implementation for `{} | {}`", lhs_ty, rhs_ty), + Some("std::ops::BitOr"), + true, + ), + hir::BinOpKind::Shl => ( + format!("no implementation for `{} << {}`", lhs_ty, rhs_ty), + Some("std::ops::Shl"), + true, + ), + hir::BinOpKind::Shr => ( + format!("no implementation for `{} >> {}`", lhs_ty, rhs_ty), + Some("std::ops::Shr"), + true, + ), + hir::BinOpKind::Eq | hir::BinOpKind::Ne => ( + format!( + "binary operation `{}` cannot be applied to type `{}`", + op.node.as_str(), + lhs_ty + ), + Some("std::cmp::PartialEq"), + false, + ), + hir::BinOpKind::Lt + | hir::BinOpKind::Le + | hir::BinOpKind::Gt + | hir::BinOpKind::Ge => ( + format!( + "binary operation `{}` cannot be applied to type `{}`", + op.node.as_str(), + lhs_ty + ), + Some("std::cmp::PartialOrd"), + false, + ), + _ => ( + format!( + "binary operation `{}` cannot be applied to type `{}`", + op.node.as_str(), + lhs_ty + ), + None, + false, + ), + }; + let mut err = + struct_span_err!(self.tcx.sess, op.span, E0369, "{}", message.as_str()); + let mut involves_fn = false; + if !lhs_expr.span.eq(&rhs_expr.span) { + involves_fn |= self.add_type_neq_err_label( + &mut err, + lhs_expr.span, lhs_ty, + rhs_ty, + op, + is_assign, ); - err.span_label( - lhs_expr.span, - format!("cannot use `{}=` on type `{}`", op.node.as_str(), lhs_ty), + involves_fn |= self.add_type_neq_err_label( + &mut err, + rhs_expr.span, + rhs_ty, + lhs_ty, + op, + is_assign, ); - let mut suggested_deref = false; - if let Ref(_, rty, _) = lhs_ty.kind { - if { - self.infcx.type_is_copy_modulo_regions( - self.param_env, - rty, - lhs_expr.span, - ) && self - .lookup_op_method(rty, &[rhs_ty], Op::Binary(op, is_assign)) - .is_ok() - } { - if let Ok(lstring) = source_map.span_to_snippet(lhs_expr.span) { - let msg = &format!( - "`{}=` can be used on '{}', you can dereference `{}`", - op.node.as_str(), - rty.peel_refs(), - lstring, - ); - err.span_suggestion_verbose( - lhs_expr.span.shrink_to_lo(), - msg, - "*".to_string(), - rustc_errors::Applicability::MachineApplicable, - ); - suggested_deref = true; - } - } - } - let missing_trait = match op.node { - hir::BinOpKind::Add => Some("std::ops::AddAssign"), - hir::BinOpKind::Sub => Some("std::ops::SubAssign"), - hir::BinOpKind::Mul => Some("std::ops::MulAssign"), - hir::BinOpKind::Div => Some("std::ops::DivAssign"), - hir::BinOpKind::Rem => Some("std::ops::RemAssign"), - hir::BinOpKind::BitAnd => Some("std::ops::BitAndAssign"), - hir::BinOpKind::BitXor => Some("std::ops::BitXorAssign"), - hir::BinOpKind::BitOr => Some("std::ops::BitOrAssign"), - hir::BinOpKind::Shl => Some("std::ops::ShlAssign"), - hir::BinOpKind::Shr => Some("std::ops::ShrAssign"), - _ => None, - }; - if let Some(missing_trait) = missing_trait { - let mut visitor = TypeParamVisitor(vec![]); - visitor.visit_ty(lhs_ty); - - let mut sugg = false; - if op.node == hir::BinOpKind::Add - && self.check_str_addition( - lhs_expr, rhs_expr, lhs_ty, rhs_ty, &mut err, true, op, - ) - { - // This has nothing here because it means we did string - // concatenation (e.g., "Hello " += "World!"). This means - // we don't want the note in the else clause to be emitted - sugg = true; - } else if let [ty] = &visitor.0[..] { - if let ty::Param(p) = ty.kind { - // FIXME: This *guesses* that constraining the type param - // will make the operation available, but this is only true - // when the corresponding trait has a blanked - // implementation, like the following: - // `impl<'a> PartialEq for &'a [T] where T: PartialEq {}` - // The correct thing to do would be to verify this - // projection would hold. - if *ty != lhs_ty { - note(&mut err, missing_trait); - } - suggest_constraining_param( - self.tcx, - self.body_id, - &mut err, - ty, - rhs_ty, - missing_trait, - p, - false, - ); - sugg = true; - } - } - if !sugg && !suggested_deref { - suggest_impl_missing(&mut err, lhs_ty, &missing_trait); - } - } - err.emit(); } - IsAssign::No => { - let (message, missing_trait, use_output) = match op.node { - hir::BinOpKind::Add => ( - format!("cannot add `{}` to `{}`", rhs_ty, lhs_ty), - Some("std::ops::Add"), - true, - ), - hir::BinOpKind::Sub => ( - format!("cannot subtract `{}` from `{}`", rhs_ty, lhs_ty), - Some("std::ops::Sub"), - true, - ), - hir::BinOpKind::Mul => ( - format!("cannot multiply `{}` to `{}`", rhs_ty, lhs_ty), - Some("std::ops::Mul"), - true, - ), - hir::BinOpKind::Div => ( - format!("cannot divide `{}` by `{}`", lhs_ty, rhs_ty), - Some("std::ops::Div"), - true, - ), - hir::BinOpKind::Rem => ( - format!("cannot mod `{}` by `{}`", lhs_ty, rhs_ty), - Some("std::ops::Rem"), - true, - ), - hir::BinOpKind::BitAnd => ( - format!("no implementation for `{} & {}`", lhs_ty, rhs_ty), - Some("std::ops::BitAnd"), - true, - ), - hir::BinOpKind::BitXor => ( - format!("no implementation for `{} ^ {}`", lhs_ty, rhs_ty), - Some("std::ops::BitXor"), - true, - ), - hir::BinOpKind::BitOr => ( - format!("no implementation for `{} | {}`", lhs_ty, rhs_ty), - Some("std::ops::BitOr"), - true, - ), - hir::BinOpKind::Shl => ( - format!("no implementation for `{} << {}`", lhs_ty, rhs_ty), - Some("std::ops::Shl"), - true, - ), - hir::BinOpKind::Shr => ( - format!("no implementation for `{} >> {}`", lhs_ty, rhs_ty), - Some("std::ops::Shr"), - true, - ), - hir::BinOpKind::Eq | hir::BinOpKind::Ne => ( - format!( - "binary operation `{}` cannot be applied to type `{}`", - op.node.as_str(), - lhs_ty - ), - Some("std::cmp::PartialEq"), - false, - ), - hir::BinOpKind::Lt - | hir::BinOpKind::Le - | hir::BinOpKind::Gt - | hir::BinOpKind::Ge => ( - format!( - "binary operation `{}` cannot be applied to type `{}`", - op.node.as_str(), - lhs_ty - ), - Some("std::cmp::PartialOrd"), - false, - ), - _ => ( - format!( - "binary operation `{}` cannot be applied to type `{}`", - op.node.as_str(), - lhs_ty - ), - None, - false, - ), - }; - let mut err = struct_span_err!( - self.tcx.sess, - op.span, - E0369, - "{}", - message.as_str() + (err, missing_trait, use_output, involves_fn) + } + }; + let mut suggested_deref = false; + if let Ref(_, rty, _) = lhs_ty.kind { + if { + self.infcx.type_is_copy_modulo_regions(self.param_env, rty, lhs_expr.span) + && self + .lookup_op_method(rty, &[rhs_ty], Op::Binary(op, is_assign)) + .is_ok() + } { + if let Ok(lstring) = source_map.span_to_snippet(lhs_expr.span) { + let msg = &format!( + "`{}{}` can be used on `{}`, you can dereference `{}`", + op.node.as_str(), + match is_assign { + IsAssign::Yes => "=", + IsAssign::No => "", + }, + rty.peel_refs(), + lstring, ); - - let mut involves_fn = false; - if !lhs_expr.span.eq(&rhs_expr.span) { - involves_fn |= self.add_type_neq_err_label( - &mut err, - lhs_expr.span, - lhs_ty, - rhs_ty, - op, - is_assign, - ); - involves_fn |= self.add_type_neq_err_label( - &mut err, - rhs_expr.span, - rhs_ty, - lhs_ty, - op, - is_assign, - ); - } - - let mut suggested_deref = false; - if let Ref(_, rty, _) = lhs_ty.kind { - if { - self.infcx.type_is_copy_modulo_regions( - self.param_env, - rty, - lhs_expr.span, - ) && self - .lookup_op_method(rty, &[rhs_ty], Op::Binary(op, is_assign)) - .is_ok() - } { - if let Ok(lstring) = source_map.span_to_snippet(lhs_expr.span) { - err.span_suggestion_verbose( - lhs_expr.span.shrink_to_lo(), - &format!( - "`{}` can be used on `{}`, you can dereference \ - `{}`", - op.node.as_str(), - rty.peel_refs(), - lstring, - ), - "*".to_string(), - Applicability::MachineApplicable, - ); - suggested_deref = true; - } - } - } - if let Some(missing_trait) = missing_trait { - let mut visitor = TypeParamVisitor(vec![]); - visitor.visit_ty(lhs_ty); - - let mut sugg = false; - if op.node == hir::BinOpKind::Add - && self.check_str_addition( - lhs_expr, rhs_expr, lhs_ty, rhs_ty, &mut err, false, op, - ) - { - // This has nothing here because it means we did string - // concatenation (e.g., "Hello " + "World!"). This means - // we don't want the note in the else clause to be emitted - sugg = true; - } else if let [ty] = &visitor.0[..] { - if let ty::Param(p) = ty.kind { - // FIXME: This *guesses* that constraining the type param - // will make the operation available, but this is only true - // when the corresponding trait has a blanked - // implementation, like the following: - // `impl<'a> PartialEq for &'a [T] where T: PartialEq {}` - // The correct thing to do would be to verify this - // projection would hold. - if *ty != lhs_ty { - note(&mut err, missing_trait); - } - suggest_constraining_param( - self.tcx, - self.body_id, - &mut err, - ty, - rhs_ty, - missing_trait, - p, - use_output, - ); - sugg = true; - } - } - if !sugg && !suggested_deref && !involves_fn { - suggest_impl_missing(&mut err, lhs_ty, &missing_trait); - } + err.span_suggestion_verbose( + lhs_expr.span.shrink_to_lo(), + msg, + "*".to_string(), + rustc_errors::Applicability::MachineApplicable, + ); + suggested_deref = true; + } + } + } + if let Some(missing_trait) = missing_trait { + let mut visitor = TypeParamVisitor(vec![]); + visitor.visit_ty(lhs_ty); + + if op.node == hir::BinOpKind::Add + && self.check_str_addition( + lhs_expr, rhs_expr, lhs_ty, rhs_ty, &mut err, is_assign, op, + ) + { + // This has nothing here because it means we did string + // concatenation (e.g., "Hello " + "World!"). This means + // we don't want the note in the else clause to be emitted + } else if let [ty] = &visitor.0[..] { + if let ty::Param(p) = ty.kind { + // FIXME: This *guesses* that constraining the type param + // will make the operation available, but this is only true + // when the corresponding trait has a blanket + // implementation, like the following: + // `impl<'a> PartialEq for &'a [T] where T: PartialEq {}` + // The correct thing to do would be to verify this + // projection would hold. + if *ty != lhs_ty { + err.note(&format!( + "the trait `{}` is not implemented for `{}`", + missing_trait, lhs_ty + )); } - err.emit(); + suggest_constraining_param( + self.tcx, + self.body_id, + &mut err, + ty, + rhs_ty, + missing_trait, + p, + use_output, + ); + } else { + bug!("type param visitor stored a non type param: {:?}", ty.kind); } + } else if !suggested_deref && !involves_fn { + suggest_impl_missing(&mut err, lhs_ty, &missing_trait); } } + err.emit(); self.tcx.ty_error() } }; @@ -621,7 +540,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { lhs_ty: Ty<'tcx>, rhs_ty: Ty<'tcx>, err: &mut rustc_errors::DiagnosticBuilder<'_>, - is_assign: bool, + is_assign: IsAssign, op: hir::BinOp, ) -> bool { let source_map = self.tcx.sess.source_map(); @@ -644,7 +563,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { &format!("{:?}", rhs_ty) == "&&str" ) => { - if !is_assign { // Do not supply this message if `&str += &str` + if let IsAssign::No = is_assign { // Do not supply this message if `&str += &str` err.span_label( op.span, "`+` cannot be used to concatenate two `&str` strings", @@ -685,7 +604,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { source_map.span_to_snippet(rhs_expr.span), is_assign, ) { - (Ok(l), Ok(r), false) => { + (Ok(l), Ok(r), IsAssign::No) => { let to_string = if l.starts_with('&') { // let a = String::new(); let b = String::new(); // let _ = &a + b; @@ -738,8 +657,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { err.span_label( ex.span, format!( - "cannot apply unary \ - operator `{}`", + "cannot apply unary operator `{}`", op.as_str() ), ); diff --git a/src/test/ui/issues/issue-5239-1.stderr b/src/test/ui/issues/issue-5239-1.stderr index d0c71e73463c..078a7ef2173b 100644 --- a/src/test/ui/issues/issue-5239-1.stderr +++ b/src/test/ui/issues/issue-5239-1.stderr @@ -6,7 +6,7 @@ LL | let x = |ref x: isize| { x += 1; }; | | | cannot use `+=` on type `&isize` | -help: `+=` can be used on 'isize', you can dereference `x` +help: `+=` can be used on `isize`, you can dereference `x` | LL | let x = |ref x: isize| { *x += 1; }; | ^ From 8f40dae93b3a66a9cdd0f940244da7f602618fba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 24 Jun 2020 16:17:04 -0700 Subject: [PATCH 3/3] Suggest type param trait bound for binop only when appropriate Verify that the binop trait *is* implemented for the types *if* all the involved type parameters are replaced with fresh inferred types. When this is the case, it means that the type parameter was indeed missing a trait bound. If this is not the case, provide a generic `note` refering to the type that doesn't implement the expected trait. --- src/librustc_typeck/check/op.rs | 67 +++++++++++++------ src/test/ui/issues/issue-35668.stderr | 1 - src/test/ui/suggestions/invalid-bin-op.rs | 7 ++ src/test/ui/suggestions/invalid-bin-op.stderr | 13 ++++ .../missing-trait-bound-for-op.fixed | 12 +--- .../suggestions/missing-trait-bound-for-op.rs | 12 +--- .../missing-trait-bound-for-op.stderr | 15 ++--- .../trait-resolution-in-overloaded-op.stderr | 1 - 8 files changed, 78 insertions(+), 50 deletions(-) create mode 100644 src/test/ui/suggestions/invalid-bin-op.rs create mode 100644 src/test/ui/suggestions/invalid-bin-op.stderr diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs index 0184c00c475b..e333b706e745 100644 --- a/src/librustc_typeck/check/op.rs +++ b/src/librustc_typeck/check/op.rs @@ -8,6 +8,7 @@ use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKi use rustc_middle::ty::adjustment::{ Adjust, Adjustment, AllowTwoPhase, AutoBorrow, AutoBorrowMutability, }; +use rustc_middle::ty::fold::TypeFolder; use rustc_middle::ty::TyKind::{Adt, Array, Char, FnDef, Never, Ref, Str, Tuple, Uint}; use rustc_middle::ty::{ self, suggest_constraining_type_param, Ty, TyCtxt, TypeFoldable, TypeVisitor, @@ -436,29 +437,36 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // we don't want the note in the else clause to be emitted } else if let [ty] = &visitor.0[..] { if let ty::Param(p) = ty.kind { - // FIXME: This *guesses* that constraining the type param - // will make the operation available, but this is only true - // when the corresponding trait has a blanket - // implementation, like the following: - // `impl<'a> PartialEq for &'a [T] where T: PartialEq {}` - // The correct thing to do would be to verify this - // projection would hold. - if *ty != lhs_ty { + // Check if the method would be found if the type param wasn't + // involved. If so, it means that adding a trait bound to the param is + // enough. Otherwise we do not give the suggestion. + let mut eraser = TypeParamEraser(&self, expr.span); + let needs_bound = self + .lookup_op_method( + eraser.fold_ty(lhs_ty), + &[eraser.fold_ty(rhs_ty)], + Op::Binary(op, is_assign), + ) + .is_ok(); + if needs_bound { + suggest_constraining_param( + self.tcx, + self.body_id, + &mut err, + ty, + rhs_ty, + missing_trait, + p, + use_output, + ); + } else if *ty != lhs_ty { + // When we know that a missing bound is responsible, we don't show + // this note as it is redundant. err.note(&format!( "the trait `{}` is not implemented for `{}`", missing_trait, lhs_ty )); } - suggest_constraining_param( - self.tcx, - self.body_id, - &mut err, - ty, - rhs_ty, - missing_trait, - p, - use_output, - ); } else { bug!("type param visitor stored a non type param: {:?}", ty.kind); } @@ -656,10 +664,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ); err.span_label( ex.span, - format!( - "cannot apply unary operator `{}`", - op.as_str() - ), + format!("cannot apply unary operator `{}`", op.as_str()), ); match actual.kind { Uint(_) if op == hir::UnOp::UnNeg => { @@ -954,3 +959,21 @@ impl<'tcx> TypeVisitor<'tcx> for TypeParamVisitor<'tcx> { ty.super_visit_with(self) } } + +struct TypeParamEraser<'a, 'tcx>(&'a FnCtxt<'a, 'tcx>, Span); + +impl TypeFolder<'tcx> for TypeParamEraser<'_, 'tcx> { + fn tcx(&self) -> TyCtxt<'tcx> { + self.0.tcx + } + + fn fold_ty(&mut self, ty: Ty<'tcx>) -> Ty<'tcx> { + match ty.kind { + ty::Param(_) => self.0.next_ty_var(TypeVariableOrigin { + kind: TypeVariableOriginKind::MiscVariable, + span: self.1, + }), + _ => ty.super_fold_with(self), + } + } +} diff --git a/src/test/ui/issues/issue-35668.stderr b/src/test/ui/issues/issue-35668.stderr index c8f1f88a4708..600cacc23aef 100644 --- a/src/test/ui/issues/issue-35668.stderr +++ b/src/test/ui/issues/issue-35668.stderr @@ -6,7 +6,6 @@ LL | a.iter().map(|a| a*a) | | | &T | - = note: the trait `std::ops::Mul` is not implemented for `&T` help: consider restricting type parameter `T` | LL | fn func<'a, T: std::ops::Mul>(a: &'a [T]) -> impl Iterator { diff --git a/src/test/ui/suggestions/invalid-bin-op.rs b/src/test/ui/suggestions/invalid-bin-op.rs new file mode 100644 index 000000000000..bea1b9155864 --- /dev/null +++ b/src/test/ui/suggestions/invalid-bin-op.rs @@ -0,0 +1,7 @@ +pub fn foo(s: S, t: S) { + let _ = s == t; //~ ERROR binary operation `==` cannot be applied to type `S` +} + +struct S(T); + +fn main() {} diff --git a/src/test/ui/suggestions/invalid-bin-op.stderr b/src/test/ui/suggestions/invalid-bin-op.stderr new file mode 100644 index 000000000000..7668eddf6070 --- /dev/null +++ b/src/test/ui/suggestions/invalid-bin-op.stderr @@ -0,0 +1,13 @@ +error[E0369]: binary operation `==` cannot be applied to type `S` + --> $DIR/invalid-bin-op.rs:2:15 + | +LL | let _ = s == t; + | - ^^ - S + | | + | S + | + = note: the trait `std::cmp::PartialEq` is not implemented for `S` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0369`. diff --git a/src/test/ui/suggestions/missing-trait-bound-for-op.fixed b/src/test/ui/suggestions/missing-trait-bound-for-op.fixed index 02886bb845cf..6b24375e4150 100644 --- a/src/test/ui/suggestions/missing-trait-bound-for-op.fixed +++ b/src/test/ui/suggestions/missing-trait-bound-for-op.fixed @@ -1,13 +1,7 @@ // run-rustfix -pub fn strip_prefix<'a, T: std::cmp::PartialEq>(s: &'a [T], prefix: &[T]) -> Option<&'a [T]> { - let n = prefix.len(); - if n <= s.len() { - let (head, tail) = s.split_at(n); - if head == prefix { //~ ERROR binary operation `==` cannot be applied to type `&[T]` - return Some(tail); - } - } - None +pub fn foo(s: &[T], t: &[T]) { + let _ = s == t; //~ ERROR binary operation `==` cannot be applied to type `&[T]` } + fn main() {} diff --git a/src/test/ui/suggestions/missing-trait-bound-for-op.rs b/src/test/ui/suggestions/missing-trait-bound-for-op.rs index aa4ef467360d..df47be070c9e 100644 --- a/src/test/ui/suggestions/missing-trait-bound-for-op.rs +++ b/src/test/ui/suggestions/missing-trait-bound-for-op.rs @@ -1,13 +1,7 @@ // run-rustfix -pub fn strip_prefix<'a, T>(s: &'a [T], prefix: &[T]) -> Option<&'a [T]> { - let n = prefix.len(); - if n <= s.len() { - let (head, tail) = s.split_at(n); - if head == prefix { //~ ERROR binary operation `==` cannot be applied to type `&[T]` - return Some(tail); - } - } - None +pub fn foo(s: &[T], t: &[T]) { + let _ = s == t; //~ ERROR binary operation `==` cannot be applied to type `&[T]` } + fn main() {} diff --git a/src/test/ui/suggestions/missing-trait-bound-for-op.stderr b/src/test/ui/suggestions/missing-trait-bound-for-op.stderr index dab4e575be1f..0e0d397d6fc1 100644 --- a/src/test/ui/suggestions/missing-trait-bound-for-op.stderr +++ b/src/test/ui/suggestions/missing-trait-bound-for-op.stderr @@ -1,16 +1,15 @@ error[E0369]: binary operation `==` cannot be applied to type `&[T]` - --> $DIR/missing-trait-bound-for-op.rs:7:17 + --> $DIR/missing-trait-bound-for-op.rs:4:15 | -LL | if head == prefix { - | ---- ^^ ------ &[T] - | | - | &[T] +LL | let _ = s == t; + | - ^^ - &[T] + | | + | &[T] | - = note: the trait `std::cmp::PartialEq` is not implemented for `&[T]` help: consider restricting type parameter `T` | -LL | pub fn strip_prefix<'a, T: std::cmp::PartialEq>(s: &'a [T], prefix: &[T]) -> Option<&'a [T]> { - | ^^^^^^^^^^^^^^^^^^^^^ +LL | pub fn foo(s: &[T], t: &[T]) { + | ^^^^^^^^^^^^^^^^^^^^^ error: aborting due to previous error diff --git a/src/test/ui/traits/trait-resolution-in-overloaded-op.stderr b/src/test/ui/traits/trait-resolution-in-overloaded-op.stderr index 1c424ce7da66..507d53dc07c4 100644 --- a/src/test/ui/traits/trait-resolution-in-overloaded-op.stderr +++ b/src/test/ui/traits/trait-resolution-in-overloaded-op.stderr @@ -6,7 +6,6 @@ LL | a * b | | | &T | - = note: the trait `std::ops::Mul` is not implemented for `&T` help: consider further restricting this bound | LL | fn foo + std::ops::Mul>(a: &T, b: f64) -> f64 {