add derivable impls lint

HKalbasi · HKalbasi · commit 9e0f00242bba · 2021-08-20T02:50:55.000+04:30
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2483,6 +2483,7 @@ Released 2018-09-13
 [`deprecated_cfg_attr`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_cfg_attr
 [`deprecated_semver`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_semver
 [`deref_addrof`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_addrof
+[`derivable_impls`]: https://rust-lang.github.io/rust-clippy/master/index.html#derivable_impls
 [`derive_hash_xor_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
 [`derive_ord_xor_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
 [`disallowed_method`]: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_method
diff --git a/clippy_lints/src/derivable_impls.rs b/clippy_lints/src/derivable_impls.rs
@@ -0,0 +1,114 @@
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::{in_macro, is_automatically_derived, is_default, match_def_path, paths};
+use rustc_hir::{Block, Expr, ExprField, ExprKind, Impl, ImplItemKind, Item, ItemKind, Node, QPath};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for double comparisons that could be simplified to a single expression.
+    ///
+    ///
+    /// ### Why is this bad?
+    /// Readability.
+    ///
+    /// ### Example
+    /// ```rust
+    /// # let x = 1;
+    /// # let y = 2;
+    /// if x == y || x < y {}
+    /// ```
+    ///
+    /// Could be written as:
+    ///
+    /// ```rust
+    /// # let x = 1;
+    /// # let y = 2;
+    /// if x <= y {}
+    /// ```
+    pub DERIVABLE_IMPLS,
+    complexity,
+    "manual implementation of a trait which is equal to a derive"
+}
+
+declare_lint_pass!(DerivableImpls => [DERIVABLE_IMPLS]);
+
+fn struct_fields<'hir>(e: &'hir Expr<'hir>) -> Option<&'hir [ExprField<'hir>]> {
+    match e.kind {
+        ExprKind::Struct(_, fields, _) => Some(fields),
+        ExprKind::Block(Block { expr, .. }, _) => expr.and_then(struct_fields),
+        _ => None,
+    }
+}
+
+fn is_path_self(e: &Expr<'_>) -> bool {
+    if let ExprKind::Path(QPath::Resolved(_, p)) = e.kind {
+        if p.segments.len() != 1 {
+            return false;
+        }
+        if p.segments[0].ident.name == rustc_span::symbol::kw::SelfUpper {
+            return true;
+        }
+    }
+    false
+}
+
+fn tuple_fields<'hir>(e: &'hir Expr<'hir>) -> Option<&'hir [Expr<'hir>]> {
+    match e.kind {
+        ExprKind::Tup(fields) => Some(fields),
+        ExprKind::Call(callee, args) => {
+            if is_path_self(callee) {
+                Some(args)
+            } else {
+                None
+            }
+        },
+        ExprKind::Block(Block { expr, .. }, _) => expr.and_then(tuple_fields),
+        _ => None,
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for DerivableImpls {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
+        if let ItemKind::Impl(Impl {
+            of_trait: Some(ref trait_ref),
+            items,
+            ..
+        }) = item.kind
+        {
+            let attrs = cx.tcx.hir().attrs(item.hir_id());
+            if is_automatically_derived(attrs) {
+                return;
+            }
+            if in_macro(item.span) {
+                return;
+            }
+            if let Some(def_id) = trait_ref.trait_def_id() {
+                if !match_def_path(cx, def_id, &paths::DEFAULT_TRAIT) {
+                    return;
+                }
+            }
+            let impl_item_hir = items[0].id.hir_id();
+            let func_expr = match cx.tcx.hir().find(impl_item_hir) {
+                Some(Node::ImplItem(x)) => match &x.kind {
+                    ImplItemKind::Fn(_, b) => match cx.tcx.hir().find(b.hir_id) {
+                        Some(Node::Expr(e)) => e,
+                        _ => return,
+                    },
+                    _ => return,
+                },
+                _ => return,
+            };
+            let should_emit = if let Some(s) = struct_fields(func_expr) {
+                s.iter().all(|ef| is_default(cx, ef.expr))
+            } else if let Some(s) = tuple_fields(func_expr) {
+                s.iter().all(|e| is_default(cx, e))
+            } else {
+                return;
+            };
+            if should_emit {
+                span_lint(cx, DERIVABLE_IMPLS, item.span, "derivable impl of trait Default:");
+            }
+        }
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -189,6 +189,7 @@ mod dbg_macro;
 mod default;
 mod default_numeric_fallback;
 mod dereference;
+mod derivable_impls;
 mod derive;
 mod disallowed_method;
 mod disallowed_script_idents;
@@ -587,6 +588,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         default::FIELD_REASSIGN_WITH_DEFAULT,
         default_numeric_fallback::DEFAULT_NUMERIC_FALLBACK,
         dereference::EXPLICIT_DEREF_METHODS,
+        derivable_impls::DERIVABLE_IMPLS,
         derive::DERIVE_HASH_XOR_EQ,
         derive::DERIVE_ORD_XOR_PARTIAL_ORD,
         derive::EXPL_IMPL_CLONE_ON_COPY,
@@ -1200,6 +1202,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(copies::IFS_SAME_COND),
         LintId::of(copies::IF_SAME_THEN_ELSE),
         LintId::of(default::FIELD_REASSIGN_WITH_DEFAULT),
+        LintId::of(derivable_impls::DERIVABLE_IMPLS),
         LintId::of(derive::DERIVE_HASH_XOR_EQ),
         LintId::of(derive::DERIVE_ORD_XOR_PARTIAL_ORD),
         LintId::of(doc::MISSING_SAFETY_DOC),
@@ -1583,6 +1586,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(casts::CHAR_LIT_AS_U8),
         LintId::of(casts::UNNECESSARY_CAST),
         LintId::of(copies::BRANCHES_SHARING_CODE),
+        LintId::of(derivable_impls::DERIVABLE_IMPLS),
         LintId::of(double_comparison::DOUBLE_COMPARISONS),
         LintId::of(double_parens::DOUBLE_PARENS),
         LintId::of(duration_subsec::DURATION_SUBSEC),
@@ -1922,6 +1926,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box panic_unimplemented::PanicUnimplemented);
     store.register_late_pass(|| box strings::StringLitAsBytes);
     store.register_late_pass(|| box derive::Derive);
+    store.register_late_pass(|| box derivable_impls::DerivableImpls);
     store.register_late_pass(|| box get_last_with_len::GetLastWithLen);
     store.register_late_pass(|| box drop_forget_ref::DropForgetRef);
     store.register_late_pass(|| box empty_enum::EmptyEnum);
diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs
@@ -1,9 +1,8 @@
 use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::source::{snippet, snippet_with_applicability};
-use clippy_utils::{in_macro, is_diag_trait_item, is_lang_ctor, match_def_path, meets_msrv, msrvs, paths};
+use clippy_utils::{in_macro, is_default, is_lang_ctor, match_def_path, meets_msrv, msrvs, paths};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
-use rustc_hir::def_id::DefId;
 use rustc_hir::LangItem::OptionNone;
 use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, QPath};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
@@ -194,64 +193,26 @@ fn check_replace_with_uninit(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'
     }
 }
 
-/// Returns true if the `def_id` associated with the `path` is recognized as a "default-equivalent"
-/// constructor from the std library
-fn is_default_equivalent_ctor(cx: &LateContext<'_>, def_id: DefId, path: &QPath<'_>) -> bool {
-    let std_types_symbols = &[
-        sym::string_type,
-        sym::vec_type,
-        sym::vecdeque_type,
-        sym::LinkedList,
-        sym::hashmap_type,
-        sym::BTreeMap,
-        sym::hashset_type,
-        sym::BTreeSet,
-        sym::BinaryHeap,
-    ];
-
-    if let QPath::TypeRelative(_, method) = path {
-        if method.ident.name == sym::new {
-            if let Some(impl_did) = cx.tcx.impl_of_method(def_id) {
-                if let Some(adt) = cx.tcx.type_of(impl_did).ty_adt_def() {
-                    return std_types_symbols
-                        .iter()
-                        .any(|&symbol| cx.tcx.is_diagnostic_item(symbol, adt.did));
-                }
-            }
-        }
-    }
-    false
-}
-
 fn check_replace_with_default(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
-    if_chain! {
-        if let ExprKind::Call(repl_func, _) = src.kind;
-        if !in_external_macro(cx.tcx.sess, expr_span);
-        if let ExprKind::Path(ref repl_func_qpath) = repl_func.kind;
-        if let Some(repl_def_id) = cx.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id();
-        if is_diag_trait_item(cx, repl_def_id, sym::Default)
-            || is_default_equivalent_ctor(cx, repl_def_id, repl_func_qpath);
-
-        then {
-            span_lint_and_then(
-                cx,
-                MEM_REPLACE_WITH_DEFAULT,
-                expr_span,
-                "replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`",
-                |diag| {
-                    if !in_macro(expr_span) {
-                        let suggestion = format!("std::mem::take({})", snippet(cx, dest.span, ""));
-
-                        diag.span_suggestion(
-                            expr_span,
-                            "consider using",
-                            suggestion,
-                            Applicability::MachineApplicable
-                        );
-                    }
+    if is_default(cx, src) && !in_external_macro(cx.tcx.sess, expr_span) {
+        span_lint_and_then(
+            cx,
+            MEM_REPLACE_WITH_DEFAULT,
+            expr_span,
+            "replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`",
+            |diag| {
+                if !in_macro(expr_span) {
+                    let suggestion = format!("std::mem::take({})", snippet(cx, dest.span, ""));
+
+                    diag.span_suggestion(
+                        expr_span,
+                        "consider using",
+                        suggestion,
+                        Applicability::MachineApplicable,
+                    );
                 }
-            );
-        }
+            },
+        );
     }
 }
 
diff --git a/clippy_lints/src/redundant_else.rs b/clippy_lints/src/redundant_else.rs
@@ -120,7 +120,7 @@ impl<'ast> Visitor<'ast> for BreakVisitor {
 impl BreakVisitor {
     fn check<T>(&mut self, item: T, visit: fn(&mut Self, T)) -> bool {
         visit(self, item);
-        std::mem::replace(&mut self.is_break, false)
+        std::mem::take(&mut self.is_break)
     }
 
     fn check_block(&mut self, block: &Block) -> bool {
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
@@ -606,6 +606,61 @@ pub fn can_mut_borrow_both(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>) -
     false
 }
 
+/// Returns true if the `def_id` associated with the `path` is recognized as a "default-equivalent"
+/// constructor from the std library
+fn is_default_equivalent_ctor(cx: &LateContext<'_>, def_id: DefId, path: &QPath<'_>) -> bool {
+    let std_types_symbols = &[
+        sym::string_type,
+        sym::vec_type,
+        sym::vecdeque_type,
+        sym::LinkedList,
+        sym::hashmap_type,
+        sym::BTreeMap,
+        sym::hashset_type,
+        sym::BTreeSet,
+        sym::BinaryHeap,
+    ];
+
+    if let QPath::TypeRelative(_, method) = path {
+        if method.ident.name == sym::new {
+            if let Some(impl_did) = cx.tcx.impl_of_method(def_id) {
+                if let Some(adt) = cx.tcx.type_of(impl_did).ty_adt_def() {
+                    return std_types_symbols
+                        .iter()
+                        .any(|&symbol| cx.tcx.is_diagnostic_item(symbol, adt.did));
+                }
+            }
+        }
+    }
+    false
+}
+
+pub fn is_default(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
+    match &e.kind {
+        ExprKind::Lit(lit) => match lit.node {
+            LitKind::Bool(x) => !x,
+            LitKind::Int(x, _) => x == 0,
+            LitKind::Str(s, _) => s.is_empty(),
+            _ => false,
+        },
+        ExprKind::Tup(items) | ExprKind::Array(items) => items.iter().all(|x| is_default(cx, x)),
+        ExprKind::Repeat(x, _) => is_default(cx, x),
+        ExprKind::Call(repl_func, _) => if_chain! {
+            if let ExprKind::Path(ref repl_func_qpath) = repl_func.kind;
+            if let Some(repl_def_id) = cx.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id();
+            if is_diag_trait_item(cx, repl_def_id, sym::Default)
+                || is_default_equivalent_ctor(cx, repl_def_id, repl_func_qpath);
+            then {
+                true
+            }
+            else {
+                false
+            }
+        },
+        _ => false,
+    }
+}
+
 /// Checks if the top level expression can be moved into a closure as is.
 pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, jump_targets: &[HirId]) -> bool {
     match expr.kind {
diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs
@@ -31,6 +31,7 @@ pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"];
 pub const CMP_MIN: [&str; 3] = ["core", "cmp", "min"];
 pub const COW: [&str; 3] = ["alloc", "borrow", "Cow"];
 pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"];
+pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"];
 pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"];
 pub const DEREF_MUT_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "DerefMut", "deref_mut"];
 /// Preferably use the diagnostic item `sym::deref_method` where possible
diff --git a/tests/ui/derivable_impls.rs b/tests/ui/derivable_impls.rs
diff --git a/tests/ui/derivable_impls.stderr b/tests/ui/derivable_impls.stderr

Original file line number	Diff line number	Diff line change
`@@ -120,7 +120,7 @@ impl<'ast> Visitor<'ast> for BreakVisitor {`
`120`	`120`	`impl BreakVisitor {`
`121`	`121`	`fn check<T>(&mut self, item: T, visit: fn(&mut Self, T)) -> bool {`
`122`	`122`	`visit(self, item);`
`123`		`- std::mem::replace(&mut self.is_break, false)`
	`123`	`+ std::mem::take(&mut self.is_break)`
`124`	`124`	`}`
`125`	`125`
`126`	`126`	`fn check_block(&mut self, block: &Block) -> bool {`