Skip to content

Commit c2ae386

Browse files
committed
On E0308, detect mut arg: &Ty meant to be arg: &mut Ty
``` 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; | ``` This might be the first thing someone tries to write to mutate the value *behind* an argument. We avoid suggesting `object = &object2;`, as that is less likely to be what was intended.
1 parent 57f9f8f commit c2ae386

File tree

2 files changed

+110
-3
lines changed

2 files changed

+110
-3
lines changed

compiler/rustc_hir_typeck/src/demand.rs

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
8585

8686
self.annotate_expected_due_to_let_ty(err, expr, error);
8787
self.annotate_loop_expected_due_to_inference(err, expr, error);
88+
if self.annotate_mut_binding_to_immutable_binding(err, expr, error) {
89+
return;
90+
}
8891

8992
// FIXME(#73154): For now, we do leak check when coercing function
9093
// pointers in typeck, instead of only during borrowck. This can lead
@@ -795,6 +798,108 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
795798
}
796799
}
797800

801+
/// Detect the following case
802+
///
803+
/// ```text
804+
/// fn change_object(mut a: &Ty) {
805+
/// let a = Ty::new();
806+
/// b = a;
807+
/// }
808+
/// ```
809+
///
810+
/// where the user likely meant to modify the value behind there reference, use `a` as an out
811+
/// parameter, instead of mutating the local binding. When encountering this we suggest:
812+
///
813+
/// ```text
814+
/// fn change_object(a: &'_ mut Ty) {
815+
/// let a = Ty::new();
816+
/// *b = a;
817+
/// }
818+
/// ```
819+
fn annotate_mut_binding_to_immutable_binding(
820+
&self,
821+
err: &mut Diag<'_>,
822+
expr: &hir::Expr<'_>,
823+
error: Option<TypeError<'tcx>>,
824+
) -> bool {
825+
let Some(TypeError::Sorts(ExpectedFound { expected, found })) = error else { return false };
826+
let ty::Ref(_, inner, hir::Mutability::Not) = expected.kind() else { return false };
827+
if !self.can_eq(self.param_env, *inner, found) {
828+
// The difference between the expected and found values isn't one level of borrowing.
829+
return false;
830+
}
831+
// We have an `ident = expr;` assignment.
832+
let hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Assign(lhs, rhs, _), .. }) =
833+
self.tcx.parent_hir_node(expr.hir_id)
834+
else {
835+
return false;
836+
};
837+
if rhs.hir_id != expr.hir_id || expected.is_closure() {
838+
return false;
839+
}
840+
// We are assigning to some binding.
841+
let hir::ExprKind::Path(hir::QPath::Resolved(
842+
None,
843+
hir::Path { res: hir::def::Res::Local(hir_id), .. },
844+
)) = lhs.kind
845+
else {
846+
return false;
847+
};
848+
let hir::Node::Pat(pat) = self.tcx.hir_node(*hir_id) else { return false };
849+
// The pattern we have is an fn argument.
850+
let hir::Node::Param(hir::Param { ty_span, .. }) = self.tcx.parent_hir_node(pat.hir_id)
851+
else {
852+
return false;
853+
};
854+
let item = self.tcx.hir().get_parent_item(pat.hir_id);
855+
let item = self.tcx.hir_owner_node(item);
856+
let Some(fn_decl) = item.fn_decl() else { return false };
857+
858+
// We have a mutable binding in the argument.
859+
let hir::PatKind::Binding(hir::BindingMode::MUT, _hir_id, ident, _) = pat.kind else {
860+
return false;
861+
};
862+
863+
// Look for the type corresponding to the argument pattern we have in the argument list.
864+
let Some(ty_sugg) = fn_decl
865+
.inputs
866+
.iter()
867+
.filter_map(|ty| {
868+
if ty.span == *ty_span
869+
&& let hir::TyKind::Ref(lt, mut_ty) = ty.kind
870+
{
871+
// `&'name Ty` -> `&'name mut Ty` or `&Ty` -> `&mut Ty`
872+
Some((
873+
mut_ty.ty.span.shrink_to_lo(),
874+
format!(
875+
"{}mut ",
876+
if lt.ident.span.lo() == lt.ident.span.hi() { "" } else { " " }
877+
),
878+
))
879+
} else {
880+
None
881+
}
882+
})
883+
.next()
884+
else {
885+
return false;
886+
};
887+
let sugg = vec![
888+
ty_sugg,
889+
(pat.span.until(ident.span), String::new()),
890+
(lhs.span.shrink_to_lo(), "*".to_string()),
891+
];
892+
// We suggest changing the argument from `mut ident: &Ty` to `ident: &'_ mut Ty` and the
893+
// assignment from `ident = val;` to `*ident = val;`.
894+
err.multipart_suggestion_verbose(
895+
"you might have meant to mutate the pointed at value being passed in, instead of \
896+
changing the reference in the local binding",
897+
sugg,
898+
Applicability::MaybeIncorrect,
899+
);
900+
return true;
901+
}
902+
798903
fn annotate_alternative_method_deref(
799904
&self,
800905
err: &mut Diag<'_>,

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,12 @@ LL | let object2 = Object;
77
LL | object = object2;
88
| ^^^^^^^ expected `&Object`, found `Object`
99
|
10-
help: consider borrowing here
10+
help: you might have meant to mutate the pointed at value being passed in, instead of changing the reference in the local binding
11+
|
12+
LL ~ fn change_object(object: &mut Object) {
13+
LL | let object2 = Object;
14+
LL ~ *object = object2;
1115
|
12-
LL | object = &object2;
13-
| +
1416

1517
error: value assigned to `object` is never read
1618
--> $DIR/mut-arg-of-borrowed-type-meant-to-be-arg-of-mut-borrow.rs:11:5

0 commit comments

Comments
 (0)