diff --git a/CHANGELOG.md b/CHANGELOG.md index acbefc8064dd..f6ee8e2aaf38 100644 --- 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 new file mode 100644 index 000000000000..b5cefbd40d22 --- /dev/null +++ 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 index f49b382c5ea3..a6cc7f0e4fc3 100644 --- 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 index 3d071c9081be..4dea1ca0d4ab 100644 --- 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_utils/src/lib.rs b/clippy_utils/src/lib.rs index e930338270cb..f977bd8a1604 100644 --- 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 index b0c3fe1e5a71..7a9b2c247ccb 100644 --- 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 new file mode 100644 index 000000000000..535def4d9123 --- /dev/null +++ b/tests/ui/derivable_impls.rs @@ -0,0 +1,96 @@ +use std::collections::HashMap; + +struct FooDefault { + a: bool, + b: i32, + c: u64, + d: Vec, + e: FooND1, + f: FooND2, + g: HashMap, + h: (i32, Vec), + i: [Vec; 3], + j: [i32; 5], +} + +impl std::default::Default for FooDefault { + fn default() -> Self { + Self { + a: false, + b: 0, + c: 0u64, + d: vec![], + e: Default::default(), + f: FooND2::default(), + g: HashMap::new(), + h: (0, vec![]), + i: [vec![], vec![], vec![]], + j: [0; 5], + } + } +} + +struct TupleDefault(bool, i32, u64); + +impl std::default::Default for TupleDefault { + fn default() -> Self { + Self(false, 0, 0u64) + } +} + +struct FooND1 { + a: bool, +} + +impl std::default::Default for FooND1 { + fn default() -> Self { + Self { a: true } + } +} + +struct FooND2 { + a: i32, +} + +impl std::default::Default for FooND2 { + fn default() -> Self { + Self { a: 5 } + } +} + +struct FooNDNew { + a: bool, +} + +impl FooNDNew { + fn new() -> Self { + Self { a: true } + } +} + +impl Default for FooNDNew { + fn default() -> Self { + Self::new() + } +} + +struct FooNDVec(Vec); + +impl Default for FooNDVec { + fn default() -> Self { + Self(vec![5, 12]) + } +} + +struct StrDefault<'a>(&'a str); + +impl Default for StrDefault<'_> { + fn default() -> Self { + Self("") + } +} + +#[derive(Default)] +struct AlreadyDerived(i32, bool); + +fn main() {} diff --git a/tests/ui/derivable_impls.stderr b/tests/ui/derivable_impls.stderr new file mode 100644 index 000000000000..dbd4bbcda4c9 --- /dev/null +++ b/tests/ui/derivable_impls.stderr @@ -0,0 +1,36 @@ +error: derivable impl of trait Default: + --> $DIR/derivable_impls.rs:16:1 + | +LL | / impl std::default::Default for FooDefault { +LL | | fn default() -> Self { +LL | | Self { +LL | | a: false, +... | +LL | | } +LL | | } + | |_^ + | + = note: `-D clippy::derivable-impls` implied by `-D warnings` + +error: derivable impl of trait Default: + --> $DIR/derivable_impls.rs:35:1 + | +LL | / impl std::default::Default for TupleDefault { +LL | | fn default() -> Self { +LL | | Self(false, 0, 0u64) +LL | | } +LL | | } + | |_^ + +error: derivable impl of trait Default: + --> $DIR/derivable_impls.rs:87:1 + | +LL | / impl Default for StrDefault<'_> { +LL | | fn default() -> Self { +LL | | Self("") +LL | | } +LL | | } + | |_^ + +error: aborting due to 3 previous errors +