Merge branch 'master' into manual_memcpy_with_counter

Author: rail-rain

clippy_lints/src/await_holding_lock.rs

@@ -11,7 +11,7 @@ declare_clippy_lint! {
     /// non-async-aware MutexGuard.
     ///
     /// **Why is this bad?** The Mutex types found in syd::sync and parking_lot
-    /// are not designed to operator in an async context across await points.
+    /// are not designed to operate in an async context across await points.
     ///
     /// There are two potential solutions. One is to use an asynx-aware Mutex
     /// type. Many asynchronous foundation crates provide such a Mutex type. The

clippy_lints/src/methods/mod.rs

@@ -2041,7 +2041,7 @@ fn lint_clone_on_copy(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, arg: &hir:
         }
         span_lint_and_then(cx, CLONE_ON_COPY, expr.span, "using `clone` on a `Copy` type", |diag| {
             if let Some((text, snip)) = snip {
-                diag.span_suggestion(expr.span, text, snip, Applicability::Unspecified);
+                diag.span_suggestion(expr.span, text, snip, Applicability::MachineApplicable);
             }
         });
     }

clippy_lints/src/misc.rs

@@ -3,11 +3,11 @@ use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::FnKind;
 use rustc_hir::{
-    def, BinOpKind, BindingAnnotation, Body, Expr, ExprKind, FnDecl, HirId, Mutability, PatKind, Stmt, StmtKind, Ty,
-    TyKind, UnOp,
+    self as hir, def, BinOpKind, BindingAnnotation, Body, Expr, ExprKind, FnDecl, HirId, Mutability, PatKind, Stmt,
+    StmtKind, TyKind, UnOp,
 };
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty;
+use rustc_middle::ty::{self, Ty};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::hygiene::DesugaringKind;
 use rustc_span::source_map::{ExpnKind, Span};
@@ -371,8 +371,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints {
                 if op.is_comparison() {
                     check_nan(cx, left, expr);
                     check_nan(cx, right, expr);
-                    check_to_owned(cx, left, right);
-                    check_to_owned(cx, right, left);
+                    check_to_owned(cx, left, right, true);
+                    check_to_owned(cx, right, left, false);
                 }
                 if (op == BinOpKind::Eq || op == BinOpKind::Ne) && (is_float(cx, left) || is_float(cx, right)) {
                     if is_allowed(cx, left) || is_allowed(cx, right) {
@@ -570,19 +570,38 @@ fn is_array(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
     matches!(&walk_ptrs_ty(cx.tables.expr_ty(expr)).kind, ty::Array(_, _))
 }
 
-fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>) {
+fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>, left: bool) {
+    #[derive(Default)]
+    struct EqImpl {
+        ty_eq_other: bool,
+        other_eq_ty: bool,
+    }
+
+    impl EqImpl {
+        fn is_implemented(&self) -> bool {
+            self.ty_eq_other || self.other_eq_ty
+        }
+    }
+
+    fn symmetric_partial_eq<'tcx>(cx: &LateContext<'_, 'tcx>, ty: Ty<'tcx>, other: Ty<'tcx>) -> Option<EqImpl> {
+        cx.tcx.lang_items().eq_trait().map(|def_id| EqImpl {
+            ty_eq_other: implements_trait(cx, ty, def_id, &[other.into()]),
+            other_eq_ty: implements_trait(cx, other, def_id, &[ty.into()]),
+        })
+    }
+
     let (arg_ty, snip) = match expr.kind {
         ExprKind::MethodCall(.., ref args, _) if args.len() == 1 => {
             if match_trait_method(cx, expr, &paths::TO_STRING) || match_trait_method(cx, expr, &paths::TO_OWNED) {
-                (cx.tables.expr_ty_adjusted(&args[0]), snippet(cx, args[0].span, ".."))
+                (cx.tables.expr_ty(&args[0]), snippet(cx, args[0].span, ".."))
             } else {
                 return;
             }
         },
         ExprKind::Call(ref path, ref v) if v.len() == 1 => {
             if let ExprKind::Path(ref path) = path.kind {
                 if match_qpath(path, &["String", "from_str"]) || match_qpath(path, &["String", "from"]) {
-                    (cx.tables.expr_ty_adjusted(&v[0]), snippet(cx, v[0].span, ".."))
+                    (cx.tables.expr_ty(&v[0]), snippet(cx, v[0].span, ".."))
                 } else {
                     return;
                 }
@@ -593,28 +612,19 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>) {
         _ => return,
     };
 
-    let other_ty = cx.tables.expr_ty_adjusted(other);
-    let partial_eq_trait_id = match cx.tcx.lang_items().eq_trait() {
-        Some(id) => id,
-        None => return,
-    };
+    let other_ty = cx.tables.expr_ty(other);
 
-    let deref_arg_impl_partial_eq_other = arg_ty.builtin_deref(true).map_or(false, |tam| {
-        implements_trait(cx, tam.ty, partial_eq_trait_id, &[other_ty.into()])
-    });
-    let arg_impl_partial_eq_deref_other = other_ty.builtin_deref(true).map_or(false, |tam| {
-        implements_trait(cx, arg_ty, partial_eq_trait_id, &[tam.ty.into()])
-    });
-    let arg_impl_partial_eq_other = implements_trait(cx, arg_ty, partial_eq_trait_id, &[other_ty.into()]);
+    let without_deref = symmetric_partial_eq(cx, arg_ty, other_ty).unwrap_or_default();
+    let with_deref = arg_ty
+        .builtin_deref(true)
+        .and_then(|tam| symmetric_partial_eq(cx, tam.ty, other_ty))
+        .unwrap_or_default();
 
-    if !deref_arg_impl_partial_eq_other && !arg_impl_partial_eq_deref_other && !arg_impl_partial_eq_other {
+    if !with_deref.is_implemented() && !without_deref.is_implemented() {
         return;
     }
 
-    let other_gets_derefed = match other.kind {
-        ExprKind::Unary(UnOp::UnDeref, _) => true,
-        _ => false,
-    };
+    let other_gets_derefed = matches!(other.kind, ExprKind::Unary(UnOp::UnDeref, _));
 
     let lint_span = if other_gets_derefed {
         expr.span.to(other.span)
@@ -634,18 +644,34 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr<'_>, other: &Expr<'_>) {
                 return;
             }
 
-            let try_hint = if deref_arg_impl_partial_eq_other {
-                // suggest deref on the left
-                format!("*{}", snip)
+            let expr_snip;
+            let eq_impl;
+            if with_deref.is_implemented() {
+                expr_snip = format!("*{}", snip);
+                eq_impl = with_deref;
             } else {
-                // suggest dropping the to_owned on the left
-                snip.to_string()
+                expr_snip = snip.to_string();
+                eq_impl = without_deref;
             };
 
+            let span;
+            let hint;
+            if (eq_impl.ty_eq_other && left) || (eq_impl.other_eq_ty && !left) {
+                span = expr.span;
+                hint = expr_snip;
+            } else {
+                span = expr.span.to(other.span);
+                if eq_impl.ty_eq_other {
+                    hint = format!("{} == {}", expr_snip, snippet(cx, other.span, ".."));
+                } else {
+                    hint = format!("{} == {}", snippet(cx, other.span, ".."), expr_snip);
+                }
+            }
+
             diag.span_suggestion(
-                lint_span,
+                span,
                 "try",
-                try_hint,
+                hint,
                 Applicability::MachineApplicable, // snippet
             );
         },
@@ -694,7 +720,7 @@ fn non_macro_local(cx: &LateContext<'_, '_>, res: def::Res) -> bool {
     }
 }
 
-fn check_cast(cx: &LateContext<'_, '_>, span: Span, e: &Expr<'_>, ty: &Ty<'_>) {
+fn check_cast(cx: &LateContext<'_, '_>, span: Span, e: &Expr<'_>, ty: &hir::Ty<'_>) {
     if_chain! {
         if let TyKind::Ptr(ref mut_ty) = ty.kind;
         if let ExprKind::Lit(ref lit) = e.kind;

tests/ui/clone_on_copy.fixed

@@ -0,0 +1,40 @@
+// run-rustfix
+
+#![allow(
+    unused,
+    clippy::redundant_clone,
+    clippy::deref_addrof,
+    clippy::no_effect,
+    clippy::unnecessary_operation
+)]
+
+use std::cell::RefCell;
+use std::rc::{self, Rc};
+use std::sync::{self, Arc};
+
+fn main() {}
+
+fn is_ascii(ch: char) -> bool {
+    ch.is_ascii()
+}
+
+fn clone_on_copy() {
+    42;
+
+    vec![1].clone(); // ok, not a Copy type
+    Some(vec![1]).clone(); // ok, not a Copy type
+    *(&42);
+
+    let rc = RefCell::new(0);
+    *rc.borrow();
+
+    // Issue #4348
+    let mut x = 43;
+    let _ = &x.clone(); // ok, getting a ref
+    'a'.clone().make_ascii_uppercase(); // ok, clone and then mutate
+    is_ascii('z');
+
+    // Issue #5436
+    let mut vec = Vec::new();
+    vec.push(42);
+}

tests/ui/clone_on_copy.rs

@@ -0,0 +1,40 @@
+// run-rustfix
+
+#![allow(
+    unused,
+    clippy::redundant_clone,
+    clippy::deref_addrof,
+    clippy::no_effect,
+    clippy::unnecessary_operation
+)]
+
+use std::cell::RefCell;
+use std::rc::{self, Rc};
+use std::sync::{self, Arc};
+
+fn main() {}
+
+fn is_ascii(ch: char) -> bool {
+    ch.is_ascii()
+}
+
+fn clone_on_copy() {
+    42.clone();
+
+    vec![1].clone(); // ok, not a Copy type
+    Some(vec![1]).clone(); // ok, not a Copy type
+    (&42).clone();
+
+    let rc = RefCell::new(0);
+    rc.borrow().clone();
+
+    // Issue #4348
+    let mut x = 43;
+    let _ = &x.clone(); // ok, getting a ref
+    'a'.clone().make_ascii_uppercase(); // ok, clone and then mutate
+    is_ascii('z'.clone());
+
+    // Issue #5436
+    let mut vec = Vec::new();
+    vec.push(42.clone());
+}

tests/ui/clone_on_copy.stderr

@@ -0,0 +1,34 @@
+error: using `clone` on a `Copy` type
+  --> $DIR/clone_on_copy.rs:22:5
+   |
+LL |     42.clone();
+   |     ^^^^^^^^^^ help: try removing the `clone` call: `42`
+   |
+   = note: `-D clippy::clone-on-copy` implied by `-D warnings`
+
+error: using `clone` on a `Copy` type
+  --> $DIR/clone_on_copy.rs:26:5
+   |
+LL |     (&42).clone();
+   |     ^^^^^^^^^^^^^ help: try dereferencing it: `*(&42)`
+
+error: using `clone` on a `Copy` type
+  --> $DIR/clone_on_copy.rs:29:5
+   |
+LL |     rc.borrow().clone();
+   |     ^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: `*rc.borrow()`
+
+error: using `clone` on a `Copy` type
+  --> $DIR/clone_on_copy.rs:35:14
+   |
+LL |     is_ascii('z'.clone());
+   |              ^^^^^^^^^^^ help: try removing the `clone` call: `'z'`
+
+error: using `clone` on a `Copy` type
+  --> $DIR/clone_on_copy.rs:39:14
+   |
+LL |     vec.push(42.clone());
+   |              ^^^^^^^^^^ help: try removing the `clone` call: `42`
+
+error: aborting due to 5 previous errors
+

tests/ui/cmp_owned/asymmetric_partial_eq.fixed

@@ -0,0 +1,93 @@
+// run-rustfix
+#![allow(unused, clippy::redundant_clone)] // See #5700
+
+// Define the types in each module to avoid trait impls leaking between modules.
+macro_rules! impl_types {
+    () => {
+        #[derive(PartialEq)]
+        pub struct Owned;
+
+        pub struct Borrowed;
+
+        impl ToOwned for Borrowed {
+            type Owned = Owned;
+            fn to_owned(&self) -> Owned {
+                Owned {}
+            }
+        }
+
+        impl std::borrow::Borrow<Borrowed> for Owned {
+            fn borrow(&self) -> &Borrowed {
+                static VALUE: Borrowed = Borrowed {};
+                &VALUE
+            }
+        }
+    };
+}
+
+// Only Borrowed == Owned is implemented
+mod borrowed_eq_owned {
+    impl_types!();
+
+    impl PartialEq<Owned> for Borrowed {
+        fn eq(&self, _: &Owned) -> bool {
+            true
+        }
+    }
+
+    pub fn compare() {
+        let owned = Owned {};
+        let borrowed = Borrowed {};
+
+        if borrowed == owned {}
+        if borrowed == owned {}
+    }
+}
+
+// Only Owned == Borrowed is implemented
+mod owned_eq_borrowed {
+    impl_types!();
+
+    impl PartialEq<Borrowed> for Owned {
+        fn eq(&self, _: &Borrowed) -> bool {
+            true
+        }
+    }
+
+    fn compare() {
+        let owned = Owned {};
+        let borrowed = Borrowed {};
+
+        if owned == borrowed {}
+        if owned == borrowed {}
+    }
+}
+
+mod issue_4874 {
+    impl_types!();
+
+    // NOTE: PartialEq<Borrowed> for T can't be implemented due to the orphan rules
+    impl<T> PartialEq<T> for Borrowed
+    where
+        T: AsRef<str> + ?Sized,
+    {
+        fn eq(&self, _: &T) -> bool {
+            true
+        }
+    }
+
+    impl std::fmt::Display for Borrowed {
+        fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+            write!(f, "borrowed")
+        }
+    }
+
+    fn compare() {
+        let borrowed = Borrowed {};
+
+        if borrowed == "Hi" {}
+        if borrowed == "Hi" {}
+    }
+}
+
+fn main() {}