Rollup merge of #134977 - estebank:issue-112357, r=BoxyUwU

Detect `mut arg: &Ty` meant to be `arg: &mut Ty` and provide structured suggestion

When a newcomer attempts to use an "out parameter" using borrows, they sometimes get confused and instead of mutating the borrow they try to mutate the function-local binding instead. This leads to either type errors (due to assigning an owned value to a mutable binding of reference type) or a multitude of lifetime errors and unused binding warnings.

This change adds a suggestion to the type error

```
error[E0308]: mismatched types
  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:6:14
   |
LL | fn change_object(mut object: &Object) {
   |                              ------- expected due to this parameter type
LL |     let object2 = Object;
LL |     object = object2;
   |              ^^^^^^^ expected `&Object`, found `Object`
   |
help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding
   |
LL ~ fn change_object(object: &mut Object) {
LL |     let object2 = Object;
LL ~     *object = object2;
   |
```
and to the unused assignment lint
```
error: value assigned to `object` is never read
  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:11:5
   |
LL |     object = &object2;
   |     ^^^^^^
   |
note: the lint level is defined here
  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:1:9
   |
LL | #![deny(unused_assignments, unused_variables)]
   |         ^^^^^^^^^^^^^^^^^^
help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding
   |
LL ~ fn change_object2(object: &mut Object) {
LL |     let object2 = Object;
LL ~     *object = object2;
   |
```

Fix #112357.

Author: jhpratt

compiler/rustc_hir_typeck/src/demand.rs

@@ -85,6 +85,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
         self.annotate_expected_due_to_let_ty(err, expr, error);
         self.annotate_loop_expected_due_to_inference(err, expr, error);
+        if self.annotate_mut_binding_to_immutable_binding(err, expr, error) {
+            return;
+        }
 
         // FIXME(#73154): For now, we do leak check when coercing function
         // pointers in typeck, instead of only during borrowck. This can lead
@@ -795,6 +798,98 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         }
     }
 
+    /// Detect the following case
+    ///
+    /// ```text
+    /// fn change_object(mut a: &Ty) {
+    ///     let a = Ty::new();
+    ///     b = a;
+    /// }
+    /// ```
+    ///
+    /// where the user likely meant to modify the value behind there reference, use `a` as an out
+    /// parameter, instead of mutating the local binding. When encountering this we suggest:
+    ///
+    /// ```text
+    /// fn change_object(a: &'_ mut Ty) {
+    ///     let a = Ty::new();
+    ///     *b = a;
+    /// }
+    /// ```
+    fn annotate_mut_binding_to_immutable_binding(
+        &self,
+        err: &mut Diag<'_>,
+        expr: &hir::Expr<'_>,
+        error: Option<TypeError<'tcx>>,
+    ) -> bool {
+        if let Some(TypeError::Sorts(ExpectedFound { expected, found })) = error
+            && let ty::Ref(_, inner, hir::Mutability::Not) = expected.kind()
+
+            // The difference between the expected and found values is one level of borrowing.
+            && self.can_eq(self.param_env, *inner, found)
+
+            // We have an `ident = expr;` assignment.
+            && let hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Assign(lhs, rhs, _), .. }) =
+                self.tcx.parent_hir_node(expr.hir_id)
+            && rhs.hir_id == expr.hir_id
+
+            // We are assigning to some binding.
+            && let hir::ExprKind::Path(hir::QPath::Resolved(
+                None,
+                hir::Path { res: hir::def::Res::Local(hir_id), .. },
+            )) = lhs.kind
+            && let hir::Node::Pat(pat) = self.tcx.hir_node(*hir_id)
+
+            // The pattern we have is an fn argument.
+            && let hir::Node::Param(hir::Param { ty_span, .. }) =
+                self.tcx.parent_hir_node(pat.hir_id)
+            && let item = self.tcx.hir().get_parent_item(pat.hir_id)
+            && let item = self.tcx.hir_owner_node(item)
+            && let Some(fn_decl) = item.fn_decl()
+
+            // We have a mutable binding in the argument.
+            && let hir::PatKind::Binding(hir::BindingMode::MUT, _hir_id, ident, _) = pat.kind
+
+            // Look for the type corresponding to the argument pattern we have in the argument list.
+            && let Some(ty_sugg) = fn_decl
+                .inputs
+                .iter()
+                .filter_map(|ty| {
+                    if ty.span == *ty_span
+                        && let hir::TyKind::Ref(lt, x) = ty.kind
+                    {
+                        // `&'name Ty` -> `&'name mut Ty` or `&Ty` -> `&mut Ty`
+                        Some((
+                            x.ty.span.shrink_to_lo(),
+                            format!(
+                                "{}mut ",
+                                if lt.ident.span.lo() == lt.ident.span.hi() { "" } else { " " }
+                            ),
+                        ))
+                    } else {
+                        None
+                    }
+                })
+                .next()
+        {
+            let sugg = vec![
+                ty_sugg,
+                (pat.span.until(ident.span), String::new()),
+                (lhs.span.shrink_to_lo(), "*".to_string()),
+            ];
+            // We suggest changing the argument from `mut ident: &Ty` to `ident: &'_ mut Ty` and the
+            // assignment from `ident = val;` to `*ident = val;`.
+            err.multipart_suggestion_verbose(
+                "you might have meant to mutate the pointed at value being passed in, instead of \
+                changing the reference in the local binding",
+                sugg,
+                Applicability::MaybeIncorrect,
+            );
+            return true;
+        }
+        false
+    }
+
     fn annotate_alternative_method_deref(
         &self,
         err: &mut Diag<'_>,

compiler/rustc_passes/messages.ftl

@@ -799,6 +799,9 @@ passes_unused_assign = value assigned to `{$name}` is never read
 passes_unused_assign_passed = value passed to `{$name}` is never read
     .help = maybe it is overwritten before being read?
 
+passes_unused_assign_suggestion =
+    you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding
+
 passes_unused_capture_maybe_capture_ref = value captured by `{$name}` is never read
     .help = did you mean to capture by reference instead?
 

compiler/rustc_passes/src/errors.rs

@@ -1787,9 +1787,26 @@ pub(crate) struct IneffectiveUnstableImpl;
 
 #[derive(LintDiagnostic)]
 #[diag(passes_unused_assign)]
-#[help]
 pub(crate) struct UnusedAssign {
     pub name: String,
+    #[subdiagnostic]
+    pub suggestion: Option<UnusedAssignSuggestion>,
+    #[help]
+    pub help: bool,
+}
+
+#[derive(Subdiagnostic)]
+#[multipart_suggestion(passes_unused_assign_suggestion, applicability = "maybe-incorrect")]
+pub(crate) struct UnusedAssignSuggestion {
+    pub pre: &'static str,
+    #[suggestion_part(code = "{pre}mut ")]
+    pub ty_span: Span,
+    #[suggestion_part(code = "")]
+    pub ty_ref_span: Span,
+    #[suggestion_part(code = "*")]
+    pub ident_span: Span,
+    #[suggestion_part(code = "")]
+    pub expr_ref_span: Span,
 }
 
 #[derive(LintDiagnostic)]

compiler/rustc_passes/src/liveness.rs

@@ -1360,7 +1360,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Liveness<'a, 'tcx> {
     fn visit_local(&mut self, local: &'tcx hir::LetStmt<'tcx>) {
         self.check_unused_vars_in_pat(local.pat, None, None, |spans, hir_id, ln, var| {
             if local.init.is_some() {
-                self.warn_about_dead_assign(spans, hir_id, ln, var);
+                self.warn_about_dead_assign(spans, hir_id, ln, var, None);
             }
         });
 
@@ -1460,7 +1460,8 @@ impl<'tcx> Liveness<'_, 'tcx> {
                     // as being used.
                     let ln = self.live_node(expr.hir_id, expr.span);
                     let var = self.variable(var_hid, expr.span);
-                    self.warn_about_dead_assign(vec![expr.span], expr.hir_id, ln, var);
+                    let sugg = self.annotate_mut_binding_to_immutable_binding(var_hid, expr);
+                    self.warn_about_dead_assign(vec![expr.span], expr.hir_id, ln, var, sugg);
                 }
             }
             _ => {
@@ -1585,6 +1586,70 @@ impl<'tcx> Liveness<'_, 'tcx> {
         }
     }
 
+    /// Detect the following case
+    ///
+    /// ```text
+    /// fn change_object(mut a: &Ty) {
+    ///     let a = Ty::new();
+    ///     b = &a;
+    /// }
+    /// ```
+    ///
+    /// where the user likely meant to modify the value behind there reference, use `a` as an out
+    /// parameter, instead of mutating the local binding. When encountering this we suggest:
+    ///
+    /// ```text
+    /// fn change_object(a: &'_ mut Ty) {
+    ///     let a = Ty::new();
+    ///     *b = a;
+    /// }
+    /// ```
+    fn annotate_mut_binding_to_immutable_binding(
+        &self,
+        var_hid: HirId,
+        expr: &'tcx Expr<'tcx>,
+    ) -> Option<errors::UnusedAssignSuggestion> {
+        if let hir::Node::Expr(parent) = self.ir.tcx.parent_hir_node(expr.hir_id)
+            && let hir::ExprKind::Assign(_, rhs, _) = parent.kind
+            && let hir::ExprKind::AddrOf(borrow_kind, _mut, inner) = rhs.kind
+            && let hir::BorrowKind::Ref = borrow_kind
+            && let hir::Node::Pat(pat) = self.ir.tcx.hir_node(var_hid)
+            && let hir::Node::Param(hir::Param { ty_span, .. }) =
+                self.ir.tcx.parent_hir_node(pat.hir_id)
+            && let item_id = self.ir.tcx.hir().get_parent_item(pat.hir_id)
+            && let item = self.ir.tcx.hir_owner_node(item_id)
+            && let Some(fn_decl) = item.fn_decl()
+            && let hir::PatKind::Binding(hir::BindingMode::MUT, _hir_id, ident, _) = pat.kind
+            && let Some((ty_span, pre)) = fn_decl
+                .inputs
+                .iter()
+                .filter_map(|ty| {
+                    if ty.span == *ty_span
+                        && let hir::TyKind::Ref(lt, mut_ty) = ty.kind
+                    {
+                        // `&'name Ty` -> `&'name mut Ty` or `&Ty` -> `&mut Ty`
+                        Some((
+                            mut_ty.ty.span.shrink_to_lo(),
+                            if lt.ident.span.lo() == lt.ident.span.hi() { "" } else { " " },
+                        ))
+                    } else {
+                        None
+                    }
+                })
+                .next()
+        {
+            Some(errors::UnusedAssignSuggestion {
+                ty_span,
+                pre,
+                ty_ref_span: pat.span.until(ident.span),
+                ident_span: expr.span.shrink_to_lo(),
+                expr_ref_span: rhs.span.until(inner.span),
+            })
+        } else {
+            None
+        }
+    }
+
     #[instrument(skip(self), level = "INFO")]
     fn report_unused(
         &self,
@@ -1738,15 +1803,23 @@ impl<'tcx> Liveness<'_, 'tcx> {
         suggs
     }
 
-    fn warn_about_dead_assign(&self, spans: Vec<Span>, hir_id: HirId, ln: LiveNode, var: Variable) {
+    fn warn_about_dead_assign(
+        &self,
+        spans: Vec<Span>,
+        hir_id: HirId,
+        ln: LiveNode,
+        var: Variable,
+        suggestion: Option<errors::UnusedAssignSuggestion>,
+    ) {
         if !self.live_on_exit(ln, var)
             && let Some(name) = self.should_warn(var)
         {
+            let help = suggestion.is_none();
             self.ir.tcx.emit_node_span_lint(
                 lint::builtin::UNUSED_ASSIGNMENTS,
                 hir_id,
                 spans,
-                errors::UnusedAssign { name },
+                errors::UnusedAssign { name, suggestion, help },
             );
         }
     }

tests/ui/fn/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.fixed

@@ -0,0 +1,22 @@
+//@ run-rustfix
+#![deny(unused_assignments, unused_variables)]
+struct Object;
+
+fn change_object(object: &mut Object) { //~ HELP you might have meant to mutate
+    let object2 = Object;
+    *object = object2; //~ ERROR mismatched types
+}
+
+fn change_object2(object: &mut Object) { //~ ERROR variable `object` is assigned to, but never used
+    //~^ HELP you might have meant to mutate
+    let object2 = Object;
+    *object = object2;
+    //~^ ERROR `object2` does not live long enough
+    //~| ERROR value assigned to `object` is never read
+}
+
+fn main() {
+    let mut object = Object;
+    change_object(&mut object);
+    change_object2(&mut object);
+}

tests/ui/fn/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs

@@ -0,0 +1,22 @@
+//@ run-rustfix
+#![deny(unused_assignments, unused_variables)]
+struct Object;
+
+fn change_object(mut object: &Object) { //~ HELP you might have meant to mutate
+    let object2 = Object;
+    object = object2; //~ ERROR mismatched types
+}
+
+fn change_object2(mut object: &Object) { //~ ERROR variable `object` is assigned to, but never used
+    //~^ HELP you might have meant to mutate
+    let object2 = Object;
+    object = &object2;
+    //~^ ERROR `object2` does not live long enough
+    //~| ERROR value assigned to `object` is never read
+}
+
+fn main() {
+    let mut object = Object;
+    change_object(&mut object);
+    change_object2(&mut object);
+}

tests/ui/fn/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.stderr

@@ -0,0 +1,69 @@
+error[E0308]: mismatched types
+  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:7:14
+   |
+LL | fn change_object(mut object: &Object) {
+   |                              ------- expected due to this parameter type
+LL |     let object2 = Object;
+LL |     object = object2;
+   |              ^^^^^^^ expected `&Object`, found `Object`
+   |
+help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding
+   |
+LL ~ fn change_object(object: &mut Object) {
+LL |     let object2 = Object;
+LL ~     *object = object2;
+   |
+
+error: value assigned to `object` is never read
+  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:13:5
+   |
+LL |     object = &object2;
+   |     ^^^^^^
+   |
+note: the lint level is defined here
+  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:2:9
+   |
+LL | #![deny(unused_assignments, unused_variables)]
+   |         ^^^^^^^^^^^^^^^^^^
+help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding
+   |
+LL ~ fn change_object2(object: &mut Object) {
+LL |
+LL |     let object2 = Object;
+LL ~     *object = object2;
+   |
+
+error: variable `object` is assigned to, but never used
+  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:10:23
+   |
+LL | fn change_object2(mut object: &Object) {
+   |                       ^^^^^^
+   |
+   = note: consider using `_object` instead
+note: the lint level is defined here
+  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:2:29
+   |
+LL | #![deny(unused_assignments, unused_variables)]
+   |                             ^^^^^^^^^^^^^^^^
+
+error[E0597]: `object2` does not live long enough
+  --> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:13:14
+   |
+LL | fn change_object2(mut object: &Object) {
+   |                               - let's call the lifetime of this reference `'1`
+LL |
+LL |     let object2 = Object;
+   |         ------- binding `object2` declared here
+LL |     object = &object2;
+   |     ---------^^^^^^^^
+   |     |        |
+   |     |        borrowed value does not live long enough
+   |     assignment requires that `object2` is borrowed for `'1`
+...
+LL | }
+   | - `object2` dropped here while still borrowed
+
+error: aborting due to 4 previous errors
+
+Some errors have detailed explanations: E0308, E0597.
+For more information about an error, try `rustc --explain E0308`.