Auto merge of #11900 - Enselic:needless-borrow-drop, r=Manishearth

bors · bors · commit 4a56563154f9 · 2023-12-05T19:10:09.000Z
needless_borrows_for_generic_args: Handle when field operand impl Drop

Before this fix, the lint had a false positive, namely when a reference was taken to a field when the field operand implements a custom Drop. The compiler will refuse to partially move a type that implements Drop, because that would put the type in a weird state.

## False Positive Example (Fixed)

```rs
struct CustomDrop(String);

impl Drop for CustomDrop {
    fn drop(&amp;mut self) {}
}

fn check_str&lt;P: AsRef&lt;str&gt;&gt;(_to: P) {}

fn test() {
    let owner = CustomDrop(String::default());
    check_str(&amp;owner.0); // Don't lint. `owner` can't be partially moved because it impl Drop
}
```

changelog: [`needless_borrows_for_generic_args`]: Handle when field operand impl Drop
diff --git a/clippy_lints/src/needless_borrows_for_generic_args.rs b/clippy_lints/src/needless_borrows_for_generic_args.rs
@@ -2,7 +2,7 @@ use clippy_config::msrvs::{self, Msrv};
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::mir::{enclosing_mir, expr_local, local_assignments, used_exactly_once, PossibleBorrowerMap};
 use clippy_utils::source::snippet_with_context;
-use clippy_utils::ty::is_copy;
+use clippy_utils::ty::{implements_trait, is_copy};
 use clippy_utils::{expr_use_ctxt, peel_n_hir_expr_refs, DefinedTy, ExprUseNode};
 use rustc_errors::Applicability;
 use rustc_hir::def::{DefKind, Res};
@@ -169,6 +169,7 @@ fn needless_borrow_count<'tcx>(
 ) -> usize {
     let destruct_trait_def_id = cx.tcx.lang_items().destruct_trait();
     let sized_trait_def_id = cx.tcx.lang_items().sized_trait();
+    let drop_trait_def_id = cx.tcx.lang_items().drop_trait();
 
     let fn_sig = cx.tcx.fn_sig(fn_id).instantiate_identity().skip_binder();
     let predicates = cx.tcx.param_env(fn_id).caller_bounds();
@@ -223,7 +224,14 @@ fn needless_borrow_count<'tcx>(
     // elements are modified each time `check_referent` is called.
     let mut args_with_referent_ty = callee_args.to_vec();
 
-    let mut check_reference_and_referent = |reference, referent| {
+    let mut check_reference_and_referent = |reference: &Expr<'tcx>, referent: &Expr<'tcx>| {
+        if let ExprKind::Field(base, _) = &referent.kind {
+            let base_ty = cx.typeck_results().expr_ty(base);
+            if drop_trait_def_id.map_or(false, |id| implements_trait(cx, base_ty, id, &[])) {
+                return false;
+            }
+        }
+
         let referent_ty = cx.typeck_results().expr_ty(referent);
 
         if !is_copy(cx, referent_ty)
diff --git a/tests/ui/needless_borrows_for_generic_args.fixed b/tests/ui/needless_borrows_for_generic_args.fixed
@@ -284,4 +284,19 @@ fn main() {
         {
         }
     }
+    // address of field when operand impl Drop
+    {
+        struct CustomDrop(String);
+
+        impl Drop for CustomDrop {
+            fn drop(&mut self) {}
+        }
+
+        fn check_str<P: AsRef<str>>(_to: P) {}
+
+        fn test() {
+            let owner = CustomDrop(String::default());
+            check_str(&owner.0); // Don't lint. `owner` can't be partially moved because it impl Drop
+        }
+    }
 }
diff --git a/tests/ui/needless_borrows_for_generic_args.rs b/tests/ui/needless_borrows_for_generic_args.rs
@@ -284,4 +284,19 @@ fn main() {
         {
         }
     }
+    // address of field when operand impl Drop
+    {
+        struct CustomDrop(String);
+
+        impl Drop for CustomDrop {
+            fn drop(&mut self) {}
+        }
+
+        fn check_str<P: AsRef<str>>(_to: P) {}
+
+        fn test() {
+            let owner = CustomDrop(String::default());
+            check_str(&owner.0); // Don't lint. `owner` can't be partially moved because it impl Drop
+        }
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -284,4 +284,19 @@ fn main() {`
`284`	`284`	`{`
`285`	`285`	`}`
`286`	`286`	`}`
	`287`	`+ // address of field when operand impl Drop`
	`288`	`+ {`
	`289`	`+ struct CustomDrop(String);`
	`290`	`+`
	`291`	`+ impl Drop for CustomDrop {`
	`292`	`+ fn drop(&mut self) {}`
	`293`	`+ }`
	`294`	`+`
	`295`	`+ fn check_str<P: AsRef<str>>(_to: P) {}`
	`296`	`+`
	`297`	`+ fn test() {`
	`298`	`+ let owner = CustomDrop(String::default());`
	`299`	+ check_str(&owner.0); // Don't lint. `owner` can't be partially moved because it impl Drop
	`300`	`+ }`
	`301`	`+ }`
`287`	`302`	`}`