Skip to content

Commit

Permalink
add derivable impls lint
Browse files Browse the repository at this point in the history
  • Loading branch information
HKalbasi committed Aug 30, 2021
1 parent 15b1c6f commit f9c4a27
Show file tree
Hide file tree
Showing 10 changed files with 448 additions and 58 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2613,6 +2613,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
Expand Down
113 changes: 113 additions & 0 deletions clippy_lints/src/derivable_impls.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::{in_macro, is_automatically_derived, is_default_equivalent, remove_blocks};
use rustc_hir::{
def::Res, Body, Expr, ExprKind, GenericArg, Impl, ImplItemKind, Item, ItemKind, Node, QPath, Ty, TyKind,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::sym;

declare_clippy_lint! {
/// ### What it does
/// Detects manual `std::default::Default` implementations that are identical to a derived implementation.
///
/// ### Why is this bad?
/// It is less concise.
///
/// ### Example
/// ```rust
/// struct Foo {
/// bar: bool
/// }
///
/// impl std::default::Default for Foo {
/// fn default() -> Self {
/// Self {
/// bar: false
/// }
/// }
/// }
/// ```
///
/// Could be written as:
///
/// ```rust
/// #[derive(Default)]
/// struct Foo {
/// bar: bool
/// }
/// ```
pub DERIVABLE_IMPLS,
complexity,
"manual implementation of the `Default` trait which is equal to a derive"
}

declare_lint_pass!(DerivableImpls => [DERIVABLE_IMPLS]);

fn is_path_self(e: &Expr<'_>) -> bool {
if let ExprKind::Path(QPath::Resolved(_, p)) = e.kind {
matches!(p.res, Res::SelfCtor(..))
} else {
false
}
}

impl<'tcx> LateLintPass<'tcx> for DerivableImpls {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
if_chain! {
if let ItemKind::Impl(Impl {
of_trait: Some(ref trait_ref),
items: [child],
self_ty,
..
}) = item.kind;
if let attrs = cx.tcx.hir().attrs(item.hir_id());
if !is_automatically_derived(attrs);
if !in_macro(item.span);
if let Some(def_id) = trait_ref.trait_def_id();
if cx.tcx.is_diagnostic_item(sym::Default, def_id);
if let impl_item_hir = child.id.hir_id();
if let Some(Node::ImplItem(impl_item)) = cx.tcx.hir().find(impl_item_hir);
if let ImplItemKind::Fn(_, b) = &impl_item.kind;
if let Body { value: func_expr, .. } = cx.tcx.hir().body(*b);
if let Ty {
kind: TyKind::Path(QPath::Resolved(_, self_ty_path)),
..
} = self_ty;
then {
if let Some(generic) = self_ty_path.segments.last().and_then(|s| s.args) {
for arg in generic.args {
if !matches!(arg, GenericArg::Lifetime(_)) {
return;
}
}
}
let should_emit = match remove_blocks(func_expr).kind {
ExprKind::Tup(fields) => fields.iter().all(|e| is_default_equivalent(cx, e)),
ExprKind::Call(callee, args)
if is_path_self(callee) => args.iter().all(|e| is_default_equivalent(cx, e)),
ExprKind::Struct(_, fields, _) => fields.iter().all(|ef| is_default_equivalent(cx, ef.expr)),
_ => false,
};
if should_emit {
let path_string = self_ty_path
.segments
.iter()
.map(|segment| {
segment.ident.to_string()
})
.collect::<Vec<_>>()
.join("::");
span_lint_and_help(
cx,
DERIVABLE_IMPLS,
item.span,
"this `impl` can be derived",
None,
&format!("try annotating `{}` with `#[derive(Default)]`", path_string),
);
}
}
}
}
}
5 changes: 5 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -588,6 +589,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,
Expand Down Expand Up @@ -1203,6 +1205,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),
Expand Down Expand Up @@ -1588,6 +1591,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(booleans::NONMINIMAL_BOOL),
LintId::of(casts::CHAR_LIT_AS_U8),
LintId::of(casts::UNNECESSARY_CAST),
LintId::of(derivable_impls::DERIVABLE_IMPLS),
LintId::of(double_comparison::DOUBLE_COMPARISONS),
LintId::of(double_parens::DOUBLE_PARENS),
LintId::of(duration_subsec::DURATION_SUBSEC),
Expand Down Expand Up @@ -1937,6 +1941,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);
Expand Down
85 changes: 29 additions & 56 deletions clippy_lints/src/mem_replace.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
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::ty::is_recursively_primitive_type;
use clippy_utils::{in_macro, is_default_equivalent, 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};
Expand Down Expand Up @@ -194,64 +194,37 @@ 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));
}
}
fn check_replace_with_default(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
// disable lint for primitives
let expr_type = cx.typeck_results().expr_ty_adjusted(src);
if is_recursively_primitive_type(expr_type) {
return;
}
// disable lint for Option since it is covered in another lint
if let ExprKind::Path(q) = &src.kind {
if is_lang_ctor(cx, q, OptionNone) {
return;
}
}
false
}
if is_default_equivalent(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, ""));

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
);
}
diag.span_suggestion(
expr_span,
"consider using",
suggestion,
Applicability::MachineApplicable,
);
}
);
}
},
);
}
}

Expand Down
61 changes: 60 additions & 1 deletion clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::DefId;
use rustc_hir::hir_id::{HirIdMap, HirIdSet};
use rustc_hir::intravisit::{self, walk_expr, ErasedMap, FnKind, NestedVisitorMap, Visitor};
use rustc_hir::LangItem::{ResultErr, ResultOk};
use rustc_hir::LangItem::{OptionNone, ResultErr, ResultOk};
use rustc_hir::{
def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl,
ImplItem, ImplItemKind, IsAsync, Item, ItemKind, LangItem, Local, MatchSource, Mutability, Node, Param, Pat,
Expand Down Expand Up @@ -630,6 +630,65 @@ 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
}

/// Returns true if the expr is equal to `Default::default()` of it's type when evaluated.
/// It doesn't cover all cases, for example indirect function calls (some of std
/// functions are supported) but it is the best we have.
pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
match &e.kind {
ExprKind::Lit(lit) => match lit.node {
LitKind::Bool(false) | LitKind::Int(0, _) => true,
LitKind::Str(s, _) => s.is_empty(),
_ => false,
},
ExprKind::Tup(items) | ExprKind::Array(items) => items.iter().all(|x| is_default_equivalent(cx, x)),
ExprKind::Repeat(x, _) => is_default_equivalent(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
}
},
ExprKind::Path(qpath) => is_lang_ctor(cx, qpath, OptionNone),
ExprKind::AddrOf(rustc_hir::BorrowKind::Ref, _, expr) => matches!(expr.kind, ExprKind::Array([])),
_ => false,
}
}

/// Checks if the top level expression can be moved into a closure as is.
/// Currently checks for:
/// * Break/Continue outside the given loop HIR ids.
Expand Down
Loading

0 comments on commit f9c4a27

Please sign in to comment.