Auto merge of #94034 - willcrichton:fix-trait-suggestion-for-binops, r=estebank

Fix incorrect suggestion for trait bounds involving binary operators

This PR fixes #93927, #92347, #93744 by replacing the bespoke trait-suggestion logic in `op.rs` with a more common code path.

The downside is that this fix causes some suggestions to not include an `Output=` type, reducing their usefulness.

Note that this causes one case in the `missing-bounds.rs` test to fail rustfix. So I would need to move that code into a separate non-fix test if this PR is otherwise acceptable.

Author: bors

compiler/rustc_typeck/src/check/op.rs

@@ -11,13 +11,12 @@ use rustc_middle::ty::adjustment::{
 };
 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,
-};
+use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable, TypeVisitor};
 use rustc_span::source_map::Spanned;
 use rustc_span::symbol::{sym, Ident};
 use rustc_span::Span;
 use rustc_trait_selection::infer::InferCtxtExt;
+use rustc_trait_selection::traits::error_reporting::suggestions::InferCtxtExt as _;
 use rustc_trait_selection::traits::{FulfillmentError, TraitEngine, TraitEngineExt};
 
 use std::ops::ControlFlow;
@@ -266,7 +265,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             Err(_) if lhs_ty.references_error() || rhs_ty.references_error() => self.tcx.ty_error(),
             Err(errors) => {
                 let source_map = self.tcx.sess.source_map();
-                let (mut err, missing_trait, use_output) = match is_assign {
+                let (mut err, missing_trait, _use_output) = match is_assign {
                     IsAssign::Yes => {
                         let mut err = struct_span_err!(
                             self.tcx.sess,
@@ -449,39 +448,39 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                         // 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() {
-                            // 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),
-                                    Some(eraser.fold_ty(rhs_ty)),
-                                    Some(rhs_expr),
-                                    Op::Binary(op, is_assign),
-                                )
-                                .is_ok();
-                            if needs_bound {
-                                suggest_constraining_param(
-                                    self.tcx,
-                                    self.body_id,
+                        // Look for a TraitPredicate in the Fulfillment errors,
+                        // and use it to generate a suggestion.
+                        //
+                        // Note that lookup_op_method must be called again but
+                        // with a specific rhs_ty instead of a placeholder so
+                        // the resulting predicate generates a more specific
+                        // suggestion for the user.
+                        let errors = self
+                            .lookup_op_method(
+                                lhs_ty,
+                                Some(rhs_ty),
+                                Some(rhs_expr),
+                                Op::Binary(op, is_assign),
+                            )
+                            .unwrap_err();
+                        let predicates = errors
+                            .into_iter()
+                            .filter_map(|error| error.obligation.predicate.to_opt_poly_trait_pred())
+                            .collect::<Vec<_>>();
+                        if !predicates.is_empty() {
+                            for pred in predicates {
+                                self.infcx.suggest_restricting_param_bound(
                                     &mut err,
-                                    *ty,
-                                    rhs_ty,
-                                    missing_trait,
-                                    p,
-                                    use_output,
+                                    pred,
+                                    self.body_id,
                                 );
-                            } 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 `{missing_trait}` is not implemented for `{lhs_ty}`"
-                                ));
                             }
-                        } else {
-                            bug!("type param visitor stored a non type param: {:?}", ty.kind());
+                        } 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 `{missing_trait}` is not implemented for `{lhs_ty}`"
+                            ));
                         }
                     }
                 }
@@ -671,24 +670,22 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                         ex.span,
                         format!("cannot apply unary operator `{}`", op.as_str()),
                     );
-                    let missing_trait = match op {
-                        hir::UnOp::Deref => unreachable!("check unary op `-` or `!` only"),
-                        hir::UnOp::Not => "std::ops::Not",
-                        hir::UnOp::Neg => "std::ops::Neg",
-                    };
+
                     let mut visitor = TypeParamVisitor(vec![]);
                     visitor.visit_ty(operand_ty);
-                    if let [ty] = &visitor.0[..] && let ty::Param(p) = *operand_ty.kind() {
-                        suggest_constraining_param(
-                            self.tcx,
-                            self.body_id,
-                            &mut err,
-                            *ty,
-                            operand_ty,
-                            missing_trait,
-                            p,
-                            true,
-                        );
+                    if let [_] = &visitor.0[..] && let ty::Param(_) = *operand_ty.kind() {
+                        let predicates = errors
+                            .iter()
+                            .filter_map(|error| {
+                                error.obligation.predicate.clone().to_opt_poly_trait_pred()
+                            });
+                        for pred in predicates {
+                            self.infcx.suggest_restricting_param_bound(
+                                &mut err,
+                                pred,
+                                self.body_id,
+                            );
+                        }
                     }
 
                     let sp = self.tcx.sess.source_map().start_point(ex.span);
@@ -973,46 +970,6 @@ fn is_builtin_binop<'tcx>(lhs: Ty<'tcx>, rhs: Ty<'tcx>, op: hir::BinOp) -> bool
     }
 }
 
-fn suggest_constraining_param(
-    tcx: TyCtxt<'_>,
-    body_id: hir::HirId,
-    mut err: &mut Diagnostic,
-    lhs_ty: Ty<'_>,
-    rhs_ty: Ty<'_>,
-    missing_trait: &str,
-    p: ty::ParamTy,
-    set_output: bool,
-) {
-    let hir = tcx.hir();
-    let msg = &format!("`{lhs_ty}` might need a bound for `{missing_trait}`");
-    // Try to find the def-id and details for the parameter p. We have only the index,
-    // so we have to find the enclosing function's def-id, then look through its declared
-    // generic parameters to get the declaration.
-    let def_id = hir.body_owner_def_id(hir::BodyId { hir_id: body_id });
-    let generics = tcx.generics_of(def_id);
-    let param_def_id = generics.type_param(&p, tcx).def_id;
-    if let Some(generics) = param_def_id
-        .as_local()
-        .map(|id| hir.local_def_id_to_hir_id(id))
-        .and_then(|id| hir.find_by_def_id(hir.get_parent_item(id)))
-        .as_ref()
-        .and_then(|node| node.generics())
-    {
-        let output = if set_output { format!("<Output = {rhs_ty}>") } else { String::new() };
-        suggest_constraining_type_param(
-            tcx,
-            generics,
-            &mut err,
-            &lhs_ty.to_string(),
-            &format!("{missing_trait}{output}"),
-            None,
-        );
-    } else {
-        let span = tcx.def_span(param_def_id);
-        err.span_label(span, msg);
-    }
-}
-
 struct TypeParamVisitor<'tcx>(Vec<Ty<'tcx>>);
 
 impl<'tcx> TypeVisitor<'tcx> for TypeParamVisitor<'tcx> {

src/test/ui/binop/issue-93927.rs

@@ -0,0 +1,20 @@
+// Regression test for #93927: suggested trait bound for T should be Eq, not PartialEq
+struct MyType<T>(T);
+
+impl<T> PartialEq for MyType<T>
+where
+    T: Eq,
+{
+    fn eq(&self, other: &Self) -> bool {
+        true
+    }
+}
+
+fn cond<T: PartialEq>(val: MyType<T>) -> bool {
+    val == val
+    //~^ ERROR binary operation `==` cannot be applied to type `MyType<T>`
+}
+
+fn main() {
+    cond(MyType(0));
+}

src/test/ui/binop/issue-93927.stderr

@@ -0,0 +1,16 @@
+error[E0369]: binary operation `==` cannot be applied to type `MyType<T>`
+  --> $DIR/issue-93927.rs:14:9
+   |
+LL |     val == val
+   |     --- ^^ --- MyType<T>
+   |     |
+   |     MyType<T>
+   |
+help: consider further restricting this bound
+   |
+LL | fn cond<T: PartialEq + std::cmp::Eq>(val: MyType<T>) -> bool {
+   |                      ++++++++++++++
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0369`.

src/test/ui/generic-associated-types/missing-bounds.fixed

@@ -1,5 +1,3 @@
-// run-rustfix
-
 use std::ops::Add;
 
 struct A<B>(B);

src/test/ui/generic-associated-types/missing-bounds.rs

@@ -1,5 +1,5 @@
 error: equality constraints are not yet supported in `where` clauses
-  --> $DIR/missing-bounds.rs:37:33
+  --> $DIR/missing-bounds.rs:35:33
    |
 LL | impl<B: Add> Add for E<B> where <B as Add>::Output = B {
    |                                 ^^^^^^^^^^^^^^^^^^^^^^ not supported
@@ -11,7 +11,7 @@ LL | impl<B: Add> Add for E<B> where B: Add<Output = B> {
    |                                 ~~~~~~~~~~~~~~~~~~
 
 error[E0308]: mismatched types
-  --> $DIR/missing-bounds.rs:11:11
+  --> $DIR/missing-bounds.rs:9:11
    |
 LL | impl<B> Add for A<B> where B: Add {
    |      - this type parameter
@@ -24,7 +24,7 @@ LL |         A(self.0 + rhs.0)
    = note: expected type parameter `B`
              found associated type `<B as Add>::Output`
 note: tuple struct defined here
-  --> $DIR/missing-bounds.rs:5:8
+  --> $DIR/missing-bounds.rs:3:8
    |
 LL | struct A<B>(B);
    |        ^
@@ -34,7 +34,7 @@ LL | impl<B> Add for A<B> where B: Add + Add<Output = B> {
    |                                   +++++++++++++++++
 
 error[E0308]: mismatched types
-  --> $DIR/missing-bounds.rs:21:14
+  --> $DIR/missing-bounds.rs:19:14
    |
 LL | impl<B: Add> Add for C<B> {
    |      - this type parameter
@@ -47,7 +47,7 @@ LL |         Self(self.0 + rhs.0)
    = note: expected type parameter `B`
              found associated type `<B as Add>::Output`
 note: tuple struct defined here
-  --> $DIR/missing-bounds.rs:15:8
+  --> $DIR/missing-bounds.rs:13:8
    |
 LL | struct C<B>(B);
    |        ^
@@ -57,7 +57,7 @@ LL | impl<B: Add + Add<Output = B>> Add for C<B> {
    |             +++++++++++++++++
 
 error[E0369]: cannot add `B` to `B`
-  --> $DIR/missing-bounds.rs:31:21
+  --> $DIR/missing-bounds.rs:29:21
    |
 LL |         Self(self.0 + rhs.0)
    |              ------ ^ ----- B
@@ -66,11 +66,11 @@ LL |         Self(self.0 + rhs.0)
    |
 help: consider restricting type parameter `B`
    |
-LL | impl<B: std::ops::Add<Output = B>> Add for D<B> {
-   |       +++++++++++++++++++++++++++
+LL | impl<B: std::ops::Add> Add for D<B> {
+   |       +++++++++++++++
 
 error[E0308]: mismatched types
-  --> $DIR/missing-bounds.rs:42:14
+  --> $DIR/missing-bounds.rs:40:14
    |
 LL | impl<B: Add> Add for E<B> where <B as Add>::Output = B {
    |      - this type parameter
@@ -83,7 +83,7 @@ LL |         Self(self.0 + rhs.0)
    = note: expected type parameter `B`
              found associated type `<B as Add>::Output`
 note: tuple struct defined here
-  --> $DIR/missing-bounds.rs:35:8
+  --> $DIR/missing-bounds.rs:33:8
    |
 LL | struct E<B>(B);
    |        ^

src/test/ui/generic-associated-types/missing-bounds.stderr

@@ -6,10 +6,10 @@ LL |     a.iter().map(|a| a*a)
    |                      |
    |                      &T
    |
-help: consider restricting type parameter `T`
+help: consider introducing a `where` bound, but there might be an alternative better way to express this requirement
    |
-LL | fn func<'a, T: std::ops::Mul<Output = &T>>(a: &'a [T]) -> impl Iterator<Item=&'a T> {
-   |              ++++++++++++++++++++++++++++
+LL | fn func<'a, T>(a: &'a [T]) -> impl Iterator<Item=&'a T> where &T: Mul<&T> {
+   |                                                         +++++++++++++++++
 
 error: aborting due to previous error
 

src/test/ui/issues/issue-35668.stderr

@@ -11,11 +11,14 @@ note: an implementation of `PartialEq<_>` might be missing for `S<T>`
    |
 LL | struct S<T>(T);
    | ^^^^^^^^^^^^^^^ must implement `PartialEq<_>`
-   = note: the trait `std::cmp::PartialEq` is not implemented for `S<T>`
 help: consider annotating `S<T>` with `#[derive(PartialEq)]`
    |
 LL | #[derive(PartialEq)]
    |
+help: consider introducing a `where` bound, but there might be an alternative better way to express this requirement
+   |
+LL | pub fn foo<T>(s: S<T>, t: S<T>) where S<T>: PartialEq {
+   |                                 +++++++++++++++++++++
 
 error: aborting due to previous error
 

src/test/ui/suggestions/invalid-bin-op.stderr

@@ -6,10 +6,10 @@ LL |     a * b
    |     |
    |     &T
    |
-help: consider further restricting this bound
+help: consider introducing a `where` bound, but there might be an alternative better way to express this requirement
    |
-LL | fn foo<T: MyMul<f64, f64> + std::ops::Mul<Output = f64>>(a: &T, b: f64) -> f64 {
-   |                           +++++++++++++++++++++++++++++
+LL | fn foo<T: MyMul<f64, f64>>(a: &T, b: f64) -> f64 where &T: Mul<f64> {
+   |                                                  ++++++++++++++++++
 
 error: aborting due to previous error