Rollup merge of rust-lang#133292 - dianne:e0277-suggest-deref, r=estebank

E0277: suggest dereferencing function arguments in more cases

This unifies and generalizes some of the logic in `TypeErrCtxt::suggest_dereferences` so that it will suggest dereferencing arguments to function/method calls in order to satisfy trait bounds in more cases.

Previously it would only fire on reference types, and it had two separate cases (one specifically to get through custom `Deref` impls when passing by-reference, and one specifically to catch rust-lang#87437). I've based the new checks loosely on what's done for `E0308` in `FnCtxt::suggest_deref_or_ref`: it will suggest dereferences to satisfy trait bounds whenever the referent is `Copy`, is boxed (& so can be moved out of the boxes), or is being passed by reference.

This doesn't make the suggestion fire in contexts other than function arguments or binary operators (which are in a separate case that this doesn't touch), and doesn't make it suggest a combination of `&`-removal and dereferences. Those would require a bit more restructuring, so I figured just doing this would be a decent first step.

Closes rust-lang#90997

Author: Zalathar

compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs

@@ -445,9 +445,8 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
         }
     }
 
-    /// When after several dereferencing, the reference satisfies the trait
-    /// bound. This function provides dereference suggestion for this
-    /// specific situation.
+    /// Provide a suggestion to dereference arguments to functions and binary operators, if that
+    /// would satisfy trait bounds.
     pub(super) fn suggest_dereferences(
         &self,
         obligation: &PredicateObligation<'tcx>,
@@ -461,127 +460,100 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
             && let Some(arg_ty) = typeck_results.expr_ty_adjusted_opt(expr)
         {
             // Suggest dereferencing the argument to a function/method call if possible
+
+            // Get the root obligation, since the leaf obligation we have may be unhelpful (#87437)
             let mut real_trait_pred = trait_pred;
             while let Some((parent_code, parent_trait_pred)) = code.parent() {
                 code = parent_code;
                 if let Some(parent_trait_pred) = parent_trait_pred {
                     real_trait_pred = parent_trait_pred;
                 }
+            }
 
-                // We `instantiate_bound_regions_with_erased` here because `make_subregion` does not handle
-                // `ReBound`, and we don't particularly care about the regions.
-                let real_ty =
-                    self.tcx.instantiate_bound_regions_with_erased(real_trait_pred.self_ty());
+            // We `instantiate_bound_regions_with_erased` here because `make_subregion` does not handle
+            // `ReBound`, and we don't particularly care about the regions.
+            let real_ty = self.tcx.instantiate_bound_regions_with_erased(real_trait_pred.self_ty());
+            if !self.can_eq(obligation.param_env, real_ty, arg_ty) {
+                return false;
+            }
 
-                if self.can_eq(obligation.param_env, real_ty, arg_ty)
-                    && let ty::Ref(region, base_ty, mutbl) = *real_ty.kind()
+            // Potentially, we'll want to place our dereferences under a `&`. We don't try this for
+            // `&mut`, since we can't be sure users will get the side-effects they want from it.
+            // If this doesn't work, we'll try removing the `&` in `suggest_remove_reference`.
+            // FIXME(dianne): this misses the case where users need both to deref and remove `&`s.
+            // This method could be combined with `TypeErrCtxt::suggest_remove_reference` to handle
+            // that, similar to what `FnCtxt::suggest_deref_or_ref` does.
+            let (is_under_ref, base_ty, span) = match expr.kind {
+                hir::ExprKind::AddrOf(hir::BorrowKind::Ref, hir::Mutability::Not, subexpr)
+                    if let &ty::Ref(region, base_ty, hir::Mutability::Not) = real_ty.kind() =>
                 {
-                    let autoderef = (self.autoderef_steps)(base_ty);
-                    if let Some(steps) =
-                        autoderef.into_iter().enumerate().find_map(|(steps, (ty, obligations))| {
-                            // Re-add the `&`
-                            let ty = Ty::new_ref(self.tcx, region, ty, mutbl);
-
-                            // Remapping bound vars here
-                            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(
-                                obligation.param_env,
-                                real_trait_pred_and_ty,
-                            );
-                            let may_hold = obligations
-                                .iter()
-                                .chain([&obligation])
-                                .all(|obligation| self.predicate_may_hold(obligation))
-                                .then_some(steps);
+                    (Some(region), base_ty, subexpr.span)
+                }
+                // Don't suggest `*&mut`, etc.
+                hir::ExprKind::AddrOf(..) => return false,
+                _ => (None, real_ty, obligation.cause.span),
+            };
 
-                            may_hold
-                        })
-                    {
-                        if steps > 0 {
-                            // Don't care about `&mut` because `DerefMut` is used less
-                            // often and user will not expect that an autoderef happens.
-                            if let hir::Node::Expr(hir::Expr {
-                                kind:
-                                    hir::ExprKind::AddrOf(
-                                        hir::BorrowKind::Ref,
-                                        hir::Mutability::Not,
-                                        expr,
-                                    ),
-                                ..
-                            }) = self.tcx.hir_node(*arg_hir_id)
-                            {
-                                let derefs = "*".repeat(steps);
-                                err.span_suggestion_verbose(
-                                    expr.span.shrink_to_lo(),
-                                    "consider dereferencing here",
-                                    derefs,
-                                    Applicability::MachineApplicable,
-                                );
-                                return true;
-                            }
-                        }
-                    } else if real_trait_pred != trait_pred {
-                        // This branch addresses #87437.
-
-                        let span = obligation.cause.span;
-                        // Remapping bound vars here
-                        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(
-                            obligation.param_env,
-                            real_trait_pred_and_base_ty,
-                        );
-                        let sized_obligation = Obligation::new(
-                            self.tcx,
-                            obligation.cause.clone(),
-                            obligation.param_env,
-                            ty::TraitRef::new(
-                                self.tcx,
-                                self.tcx.require_lang_item(
-                                    hir::LangItem::Sized,
-                                    Some(obligation.cause.span),
-                                ),
-                                [base_ty],
-                            ),
-                        );
-                        if self.predicate_may_hold(&obligation)
-                            && self.predicate_must_hold_modulo_regions(&sized_obligation)
-                            // Do not suggest * if it is already a reference,
-                            // will suggest removing the borrow instead in that case.
-                            && !matches!(expr.kind, hir::ExprKind::AddrOf(..))
-                        {
-                            let call_node = self.tcx.hir_node(*call_hir_id);
-                            let msg = "consider dereferencing here";
-                            let is_receiver = matches!(
-                                call_node,
-                                Node::Expr(hir::Expr {
-                                    kind: hir::ExprKind::MethodCall(_, receiver_expr, ..),
-                                    ..
-                                })
-                                if receiver_expr.hir_id == *arg_hir_id
-                            );
-                            if is_receiver {
-                                err.multipart_suggestion_verbose(
-                                    msg,
-                                    vec![
-                                        (span.shrink_to_lo(), "(*".to_string()),
-                                        (span.shrink_to_hi(), ")".to_string()),
-                                    ],
-                                    Applicability::MachineApplicable,
-                                )
-                            } else {
-                                err.span_suggestion_verbose(
-                                    span.shrink_to_lo(),
-                                    msg,
-                                    '*',
-                                    Applicability::MachineApplicable,
-                                )
-                            };
-                            return true;
-                        }
-                    }
+            let autoderef = (self.autoderef_steps)(base_ty);
+            let mut is_boxed = base_ty.is_box();
+            if let Some(steps) = autoderef.into_iter().position(|(mut ty, obligations)| {
+                // Ensure one of the following for dereferencing to be valid: we're passing by
+                // reference, `ty` is `Copy`, or we're moving out of a (potentially nested) `Box`.
+                let can_deref = is_under_ref.is_some()
+                    || self.type_is_copy_modulo_regions(obligation.param_env, ty)
+                    || ty.is_numeric() // for inference vars (presumably but not provably `Copy`)
+                    || is_boxed && self.type_is_sized_modulo_regions(obligation.param_env, ty);
+                is_boxed &= ty.is_box();
+
+                // Re-add the `&` if necessary
+                if let Some(region) = is_under_ref {
+                    ty = Ty::new_ref(self.tcx, region, ty, hir::Mutability::Not);
                 }
+
+                // Remapping bound vars here
+                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(
+                    obligation.param_env,
+                    real_trait_pred_and_ty,
+                );
+
+                can_deref
+                    && obligations
+                        .iter()
+                        .chain([&obligation])
+                        .all(|obligation| self.predicate_may_hold(obligation))
+            }) && steps > 0
+            {
+                let derefs = "*".repeat(steps);
+                let msg = "consider dereferencing here";
+                let call_node = self.tcx.hir_node(*call_hir_id);
+                let is_receiver = matches!(
+                    call_node,
+                    Node::Expr(hir::Expr {
+                        kind: hir::ExprKind::MethodCall(_, receiver_expr, ..),
+                        ..
+                    })
+                    if receiver_expr.hir_id == *arg_hir_id
+                );
+                if is_receiver {
+                    err.multipart_suggestion_verbose(
+                        msg,
+                        vec![
+                            (span.shrink_to_lo(), format!("({derefs}")),
+                            (span.shrink_to_hi(), ")".to_string()),
+                        ],
+                        Applicability::MachineApplicable,
+                    )
+                } else {
+                    err.span_suggestion_verbose(
+                        span.shrink_to_lo(),
+                        msg,
+                        derefs,
+                        Applicability::MachineApplicable,
+                    )
+                };
+                return true;
             }
         } else if let (
             ObligationCauseCode::BinOp { lhs_hir_id, rhs_hir_id: Some(rhs_hir_id), .. },

tests/ui/no_send-rc.stderr

@@ -12,6 +12,10 @@ note: required by a bound in `bar`
    |
 LL | fn bar<T: Send>(_: T) {}
    |           ^^^^ required by this bound in `bar`
+help: consider dereferencing here
+   |
+LL |     bar(*x);
+   |         +
 
 error: aborting due to 1 previous error
 

tests/ui/suggestions/issue-84973-blacklist.stderr

@@ -86,6 +86,10 @@ note: required by a bound in `f_send`
    |
 LL | fn f_send<T: Send>(t: T) {}
    |              ^^^^ required by this bound in `f_send`
+help: consider dereferencing here
+   |
+LL |     f_send(*rc);
+   |            +
 
 error: aborting due to 5 previous errors
 

tests/ui/traits/suggest-dereferences/deref-argument.fixed

@@ -0,0 +1,37 @@
+//@ run-rustfix
+//! diagnostic test for #90997.
+//! test that E0277 suggests dereferences to satisfy bounds when the referent is `Copy` or boxed.
+use std::ops::Deref;
+
+trait Test {
+    fn test(self);
+}
+fn consume_test(x: impl Test) { x.test() }
+
+impl Test for u32 {
+    fn test(self) {}
+}
+struct MyRef(u32);
+impl Deref for MyRef {
+    type Target = u32;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+struct NonCopy;
+impl Test for NonCopy {
+    fn test(self) {}
+}
+
+fn main() {
+    let my_ref = MyRef(0);
+    consume_test(*my_ref);
+    //~^ ERROR the trait bound `MyRef: Test` is not satisfied
+    //~| SUGGESTION *
+
+    let nested_box = Box::new(Box::new(Box::new(NonCopy)));
+    consume_test(***nested_box);
+    //~^ ERROR the trait bound `Box<Box<Box<NonCopy>>>: Test` is not satisfied
+    //~| SUGGESTION ***
+}

tests/ui/traits/suggest-dereferences/deref-argument.rs

@@ -0,0 +1,37 @@
+//@ run-rustfix
+//! diagnostic test for #90997.
+//! test that E0277 suggests dereferences to satisfy bounds when the referent is `Copy` or boxed.
+use std::ops::Deref;
+
+trait Test {
+    fn test(self);
+}
+fn consume_test(x: impl Test) { x.test() }
+
+impl Test for u32 {
+    fn test(self) {}
+}
+struct MyRef(u32);
+impl Deref for MyRef {
+    type Target = u32;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+struct NonCopy;
+impl Test for NonCopy {
+    fn test(self) {}
+}
+
+fn main() {
+    let my_ref = MyRef(0);
+    consume_test(my_ref);
+    //~^ ERROR the trait bound `MyRef: Test` is not satisfied
+    //~| SUGGESTION *
+
+    let nested_box = Box::new(Box::new(Box::new(NonCopy)));
+    consume_test(nested_box);
+    //~^ ERROR the trait bound `Box<Box<Box<NonCopy>>>: Test` is not satisfied
+    //~| SUGGESTION ***
+}

tests/ui/traits/suggest-dereferences/deref-argument.stderr

@@ -0,0 +1,39 @@
+error[E0277]: the trait bound `MyRef: Test` is not satisfied
+  --> $DIR/deref-argument.rs:29:18
+   |
+LL |     consume_test(my_ref);
+   |     ------------ ^^^^^^ the trait `Test` is not implemented for `MyRef`
+   |     |
+   |     required by a bound introduced by this call
+   |
+note: required by a bound in `consume_test`
+  --> $DIR/deref-argument.rs:9:25
+   |
+LL | fn consume_test(x: impl Test) { x.test() }
+   |                         ^^^^ required by this bound in `consume_test`
+help: consider dereferencing here
+   |
+LL |     consume_test(*my_ref);
+   |                  +
+
+error[E0277]: the trait bound `Box<Box<Box<NonCopy>>>: Test` is not satisfied
+  --> $DIR/deref-argument.rs:34:18
+   |
+LL |     consume_test(nested_box);
+   |     ------------ ^^^^^^^^^^ the trait `Test` is not implemented for `Box<Box<Box<NonCopy>>>`
+   |     |
+   |     required by a bound introduced by this call
+   |
+note: required by a bound in `consume_test`
+  --> $DIR/deref-argument.rs:9:25
+   |
+LL | fn consume_test(x: impl Test) { x.test() }
+   |                         ^^^^ required by this bound in `consume_test`
+help: consider dereferencing here
+   |
+LL |     consume_test(***nested_box);
+   |                  +++
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0277`.