Auto merge of #9791 - smoelius:issues-9739-9782, r=Jarcho

Address issues 9739 and 9782 This PR fixes #9739 in the manner I suggested in #9739 (comment). This PR also fixes the compilation failures in #9782 (but doesn't address `@e00E's` other objections). Fixes #9739 r? `@Jarcho` changelog: Fix two `needless_borrow` false positives, one involving borrows in `if`-`else`s, the other involving qualified function calls
rust-lang · Nov 8, 2022 · b9ca319 · b9ca319
2 parents 4e31877 + 50f63a0
commit b9ca319
Show file tree

Hide file tree

Showing 4 changed files with 233 additions and 24 deletions.
diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs
@@ -805,30 +805,39 @@ fn walk_parents<'tcx>(
                     .position(|arg| arg.hir_id == child_id)
                     .zip(expr_sig(cx, func))
                     .and_then(|(i, sig)| {
-                        sig.input_with_hir(i).map(|(hir_ty, ty)| match hir_ty {
-                            // Type inference for closures can depend on how they're called. Only go by the explicit
-                            // types here.
-                            Some(hir_ty) => binding_ty_auto_deref_stability(cx, hir_ty, precedence, ty.bound_vars()),
-                            None => {
-                                if let ty::Param(param_ty) = ty.skip_binder().kind() {
-                                    needless_borrow_impl_arg_position(
-                                        cx,
-                                        possible_borrowers,
-                                        parent,
-                                        i,
-                                        *param_ty,
-                                        e,
-                                        precedence,
-                                        msrv,
-                                    )
-                                } else {
-                                    ty_auto_deref_stability(cx, cx.tcx.erase_late_bound_regions(ty), precedence)
-                                        .position_for_arg()
-                                }
-                            },
+                        sig.input_with_hir(i).map(|(hir_ty, ty)| {
+                            match hir_ty {
+                                // Type inference for closures can depend on how they're called. Only go by the explicit
+                                // types here.
+                                Some(hir_ty) => {
+                                    binding_ty_auto_deref_stability(cx, hir_ty, precedence, ty.bound_vars())
+                                },
+                                None => {
+                                    // `e.hir_id == child_id` for https://github.com/rust-lang/rust-clippy/issues/9739
+                                    // `!call_is_qualified(func)` for https://github.com/rust-lang/rust-clippy/issues/9782
+                                    if e.hir_id == child_id
+                                        && !call_is_qualified(func)
+                                        && let ty::Param(param_ty) = ty.skip_binder().kind()
+                                    {
+                                        needless_borrow_impl_arg_position(
+                                            cx,
+                                            possible_borrowers,
+                                            parent,
+                                            i,
+                                            *param_ty,
+                                            e,
+                                            precedence,
+                                            msrv,
+                                        )
+                                    } else {
+                                        ty_auto_deref_stability(cx, cx.tcx.erase_late_bound_regions(ty), precedence)
+                                            .position_for_arg()
+                                    }
+                                },
+                            }
                         })
                     }),
-                ExprKind::MethodCall(_, receiver, args, _) => {
+                ExprKind::MethodCall(method, receiver, args, _) => {
                     let id = cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap();
                     if receiver.hir_id == child_id {
                         // Check for calls to trait methods where the trait is implemented on a reference.
@@ -866,7 +875,9 @@ fn walk_parents<'tcx>(
                     }
                     args.iter().position(|arg| arg.hir_id == child_id).map(|i| {
                         let ty = cx.tcx.fn_sig(id).skip_binder().inputs()[i + 1];
-                        if let ty::Param(param_ty) = ty.kind() {
+                        // `e.hir_id == child_id` for https://github.com/rust-lang/rust-clippy/issues/9739
+                        // `method.args.is_none()` for https://github.com/rust-lang/rust-clippy/issues/9782
+                        if e.hir_id == child_id && method.args.is_none() && let ty::Param(param_ty) = ty.kind() {
                             needless_borrow_impl_arg_position(
                                 cx,
                                 possible_borrowers,
@@ -1044,6 +1055,18 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool {
     v.0
 }
 
+fn call_is_qualified(expr: &Expr<'_>) -> bool {
+    if let ExprKind::Path(path) = &expr.kind {
+        match path {
+            QPath::Resolved(_, path) => path.segments.last().map_or(false, |segment| segment.args.is_some()),
+            QPath::TypeRelative(_, segment) => segment.args.is_some(),
+            QPath::LangItem(..) => false,
+        }
+    } else {
+        false
+    }
+}
+
 // Checks whether:
 // * child is an expression of the form `&e` in an argument position requiring an `impl Trait`
 // * `e`'s type implements `Trait` and is copyable

diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed
@@ -420,3 +420,93 @@ mod issue_9710 {
 
     fn f<T: AsRef<str>>(_: T) {}
 }
+
+#[allow(dead_code)]
+mod issue_9739 {
+    fn foo<D: std::fmt::Display>(_it: impl IntoIterator<Item = D>) {}
+
+    fn main() {
+        foo(if std::env::var_os("HI").is_some() {
+            &[0]
+        } else {
+            &[] as &[u32]
+        });
+    }
+}
+
+#[allow(dead_code)]
+mod issue_9739_method_variant {
+    struct S;
+
+    impl S {
+        fn foo<D: std::fmt::Display>(&self, _it: impl IntoIterator<Item = D>) {}
+    }
+
+    fn main() {
+        S.foo(if std::env::var_os("HI").is_some() {
+            &[0]
+        } else {
+            &[] as &[u32]
+        });
+    }
+}
+
+#[allow(dead_code)]
+mod issue_9782 {
+    fn foo<T: AsRef<[u8]>>(t: T) {
+        println!("{}", std::mem::size_of::<T>());
+        let _t: &[u8] = t.as_ref();
+    }
+
+    fn main() {
+        let a: [u8; 100] = [0u8; 100];
+
+        // 100
+        foo::<[u8; 100]>(a);
+        foo(a);
+
+        // 16
+        foo::<&[u8]>(&a);
+        foo(a.as_slice());
+
+        // 8
+        foo::<&[u8; 100]>(&a);
+        foo(a);
+    }
+}
+
+#[allow(dead_code)]
+mod issue_9782_type_relative_variant {
+    struct S;
+
+    impl S {
+        fn foo<T: AsRef<[u8]>>(t: T) {
+            println!("{}", std::mem::size_of::<T>());
+            let _t: &[u8] = t.as_ref();
+        }
+    }
+
+    fn main() {
+        let a: [u8; 100] = [0u8; 100];
+
+        S::foo::<&[u8; 100]>(&a);
+    }
+}
+
+#[allow(dead_code)]
+mod issue_9782_method_variant {
+    struct S;
+
+    impl S {
+        fn foo<T: AsRef<[u8]>>(&self, t: T) {
+            println!("{}", std::mem::size_of::<T>());
+            let _t: &[u8] = t.as_ref();
+        }
+    }
+
+    fn main() {
+        let a: [u8; 100] = [0u8; 100];
+
+        S.foo::<&[u8; 100]>(&a);
+    }
+}
diff --git a/tests/ui/needless_borrow.rs b/tests/ui/needless_borrow.rs
@@ -420,3 +420,93 @@ mod issue_9710 {
 
     fn f<T: AsRef<str>>(_: T) {}
 }
+
+#[allow(dead_code)]
+mod issue_9739 {
+    fn foo<D: std::fmt::Display>(_it: impl IntoIterator<Item = D>) {}
+
+    fn main() {
+        foo(if std::env::var_os("HI").is_some() {
+            &[0]
+        } else {
+            &[] as &[u32]
+        });
+    }
+}
+
+#[allow(dead_code)]
+mod issue_9739_method_variant {
+    struct S;
+
+    impl S {
+        fn foo<D: std::fmt::Display>(&self, _it: impl IntoIterator<Item = D>) {}
+    }
+
+    fn main() {
+        S.foo(if std::env::var_os("HI").is_some() {
+            &[0]
+        } else {
+            &[] as &[u32]
+        });
+    }
+}
+
+#[allow(dead_code)]
+mod issue_9782 {
+    fn foo<T: AsRef<[u8]>>(t: T) {
+        println!("{}", std::mem::size_of::<T>());
+        let _t: &[u8] = t.as_ref();
+    }
+
+    fn main() {
+        let a: [u8; 100] = [0u8; 100];
+
+        // 100
+        foo::<[u8; 100]>(a);
+        foo(a);
+
+        // 16
+        foo::<&[u8]>(&a);
+        foo(a.as_slice());
+
+        // 8
+        foo::<&[u8; 100]>(&a);
+        foo(&a);
+    }
+}
+
+#[allow(dead_code)]
+mod issue_9782_type_relative_variant {
+    struct S;
+
+    impl S {
+        fn foo<T: AsRef<[u8]>>(t: T) {
+            println!("{}", std::mem::size_of::<T>());
+            let _t: &[u8] = t.as_ref();
+        }
+    }
+
+    fn main() {
+        let a: [u8; 100] = [0u8; 100];
+
+        S::foo::<&[u8; 100]>(&a);
+    }
+}
+
+#[allow(dead_code)]
+mod issue_9782_method_variant {
+    struct S;
+
+    impl S {
+        fn foo<T: AsRef<[u8]>>(&self, t: T) {
+            println!("{}", std::mem::size_of::<T>());
+            let _t: &[u8] = t.as_ref();
+        }
+    }
+
+    fn main() {
+        let a: [u8; 100] = [0u8; 100];
+
+        S.foo::<&[u8; 100]>(&a);
+    }
+}
diff --git a/tests/ui/needless_borrow.stderr b/tests/ui/needless_borrow.stderr
@@ -210,5 +210,11 @@ error: the borrowed expression implements the required traits
 LL |         use_x(&x);
    |               ^^ help: change this to: `x`
 
-error: aborting due to 35 previous errors
+error: the borrowed expression implements the required traits
+  --> $DIR/needless_borrow.rs:474:13
+   |
+LL |         foo(&a);
+   |             ^^ help: change this to: `a`
+
+error: aborting due to 36 previous errors