Skip to content

Commit 93390d4

Browse files
HKalbasiHKalbasi
committed
add derivable impls lint
1 parent a8c2c7b commit 93390d4

11 files changed

+524
-58
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -2613,6 +2613,7 @@ Released 2018-09-13
26132613
[`deprecated_cfg_attr`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_cfg_attr
26142614
[`deprecated_semver`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_semver
26152615
[`deref_addrof`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_addrof
2616+
[`derivable_impls`]: https://rust-lang.github.io/rust-clippy/master/index.html#derivable_impls
26162617
[`derive_hash_xor_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
26172618
[`derive_ord_xor_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
26182619
[`disallowed_method`]: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_method

‎clippy_lints/src/derivable_impls.rs

+116
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
use clippy_utils::diagnostics::span_lint_and_help;
2+
use clippy_utils::{in_macro, is_automatically_derived, is_default_equivalent, remove_blocks};
3+
use rustc_hir::{
4+
def::{DefKind, Res},
5+
Body, Expr, ExprKind, GenericArg, Impl, ImplItemKind, Item, ItemKind, Node, QPath, Ty, TyKind,
6+
};
7+
use rustc_lint::{LateContext, LateLintPass};
8+
use rustc_session::{declare_lint_pass, declare_tool_lint};
9+
use rustc_span::sym;
10+
11+
declare_clippy_lint! {
12+
/// ### What it does
13+
/// Detects manual `std::default::Default` implementations that are identical to a derived implementation.
14+
///
15+
/// ### Why is this bad?
16+
/// It is less concise.
17+
///
18+
/// ### Example
19+
/// ```rust
20+
/// struct Foo {
21+
/// bar: bool
22+
/// }
23+
///
24+
/// impl std::default::Default for Foo {
25+
/// fn default() -> Self {
26+
/// Self {
27+
/// bar: false
28+
/// }
29+
/// }
30+
/// }
31+
/// ```
32+
///
33+
/// Could be written as:
34+
///
35+
/// ```rust
36+
/// #[derive(Default)]
37+
/// struct Foo {
38+
/// bar: bool
39+
/// }
40+
/// ```
41+
///
42+
/// ### Known problems
43+
/// Derive macros [sometimes use incorrect bounds](https://github.com/rust-lang/rust/issues/26925)
44+
/// in generic types and the user defined `impl` maybe is more generalized or
45+
/// specialized than what derive will produce. This lint can't detect the manual `impl`
46+
/// has exactly equal bounds, and therefore this lint is disabled for types with
47+
/// generic parameters.
48+
///
49+
pub DERIVABLE_IMPLS,
50+
complexity,
51+
"manual implementation of the `Default` trait which is equal to a derive"
52+
}
53+
54+
declare_lint_pass!(DerivableImpls => [DERIVABLE_IMPLS]);
55+
56+
fn is_path_self(e: &Expr<'_>) -> bool {
57+
if let ExprKind::Path(QPath::Resolved(_, p)) = e.kind {
58+
matches!(p.res, Res::SelfCtor(..) | Res::Def(DefKind::Ctor(..), _))
59+
} else {
60+
false
61+
}
62+
}
63+
64+
impl<'tcx> LateLintPass<'tcx> for DerivableImpls {
65+
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
66+
if_chain! {
67+
if let ItemKind::Impl(Impl {
68+
of_trait: Some(ref trait_ref),
69+
items: [child],
70+
self_ty,
71+
..
72+
}) = item.kind;
73+
if let attrs = cx.tcx.hir().attrs(item.hir_id());
74+
if !is_automatically_derived(attrs);
75+
if !in_macro(item.span);
76+
if let Some(def_id) = trait_ref.trait_def_id();
77+
if cx.tcx.is_diagnostic_item(sym::Default, def_id);
78+
if let impl_item_hir = child.id.hir_id();
79+
if let Some(Node::ImplItem(impl_item)) = cx.tcx.hir().find(impl_item_hir);
80+
if let ImplItemKind::Fn(_, b) = &impl_item.kind;
81+
if let Body { value: func_expr, .. } = cx.tcx.hir().body(*b);
82+
if let Ty {
83+
kind: TyKind::Path(QPath::Resolved(_, self_ty_path)),
84+
..
85+
} = self_ty;
86+
if let Some(adt_def) = cx.tcx.type_of(item.def_id).ty_adt_def();
87+
then {
88+
if let Some(generic) = self_ty_path.segments.last().and_then(|s| s.args) {
89+
for arg in generic.args {
90+
if !matches!(arg, GenericArg::Lifetime(_)) {
91+
return;
92+
}
93+
}
94+
}
95+
let should_emit = match remove_blocks(func_expr).kind {
96+
ExprKind::Tup(fields) => fields.iter().all(|e| is_default_equivalent(cx, e)),
97+
ExprKind::Call(callee, args)
98+
if is_path_self(callee) => args.iter().all(|e| is_default_equivalent(cx, e)),
99+
ExprKind::Struct(_, fields, _) => fields.iter().all(|ef| is_default_equivalent(cx, ef.expr)),
100+
_ => false,
101+
};
102+
if should_emit {
103+
let path_string = cx.tcx.def_path_str(adt_def.did);
104+
span_lint_and_help(
105+
cx,
106+
DERIVABLE_IMPLS,
107+
item.span,
108+
"this `impl` can be derived",
109+
None,
110+
&format!("try annotating `{}` with `#[derive(Default)]`", path_string),
111+
);
112+
}
113+
}
114+
}
115+
}
116+
}

‎clippy_lints/src/lib.rs

+5
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,7 @@ mod dbg_macro;
187187
mod default;
188188
mod default_numeric_fallback;
189189
mod dereference;
190+
mod derivable_impls;
190191
mod derive;
191192
mod disallowed_method;
192193
mod disallowed_script_idents;
@@ -586,6 +587,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
586587
default::FIELD_REASSIGN_WITH_DEFAULT,
587588
default_numeric_fallback::DEFAULT_NUMERIC_FALLBACK,
588589
dereference::EXPLICIT_DEREF_METHODS,
590+
derivable_impls::DERIVABLE_IMPLS,
589591
derive::DERIVE_HASH_XOR_EQ,
590592
derive::DERIVE_ORD_XOR_PARTIAL_ORD,
591593
derive::EXPL_IMPL_CLONE_ON_COPY,
@@ -1204,6 +1206,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
12041206
LintId::of(copies::IFS_SAME_COND),
12051207
LintId::of(copies::IF_SAME_THEN_ELSE),
12061208
LintId::of(default::FIELD_REASSIGN_WITH_DEFAULT),
1209+
LintId::of(derivable_impls::DERIVABLE_IMPLS),
12071210
LintId::of(derive::DERIVE_HASH_XOR_EQ),
12081211
LintId::of(derive::DERIVE_ORD_XOR_PARTIAL_ORD),
12091212
LintId::of(doc::MISSING_SAFETY_DOC),
@@ -1589,6 +1592,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
15891592
LintId::of(booleans::NONMINIMAL_BOOL),
15901593
LintId::of(casts::CHAR_LIT_AS_U8),
15911594
LintId::of(casts::UNNECESSARY_CAST),
1595+
LintId::of(derivable_impls::DERIVABLE_IMPLS),
15921596
LintId::of(double_comparison::DOUBLE_COMPARISONS),
15931597
LintId::of(double_parens::DOUBLE_PARENS),
15941598
LintId::of(duration_subsec::DURATION_SUBSEC),
@@ -1937,6 +1941,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
19371941
store.register_late_pass(|| Box::new(panic_unimplemented::PanicUnimplemented));
19381942
store.register_late_pass(|| Box::new(strings::StringLitAsBytes));
19391943
store.register_late_pass(|| Box::new(derive::Derive));
1944+
store.register_late_pass(|| Box::new(derivable_impls::DerivableImpls));
19401945
store.register_late_pass(|| Box::new(get_last_with_len::GetLastWithLen));
19411946
store.register_late_pass(|| Box::new(drop_forget_ref::DropForgetRef));
19421947
store.register_late_pass(|| Box::new(empty_enum::EmptyEnum));

‎clippy_lints/src/mem_replace.rs

+29-56
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
22
use clippy_utils::source::{snippet, snippet_with_applicability};
3-
use clippy_utils::{in_macro, is_diag_trait_item, is_lang_ctor, match_def_path, meets_msrv, msrvs, paths};
3+
use clippy_utils::ty::is_non_aggregate_primitive_type;
4+
use clippy_utils::{in_macro, is_default_equivalent, is_lang_ctor, match_def_path, meets_msrv, msrvs, paths};
45
use if_chain::if_chain;
56
use rustc_errors::Applicability;
6-
use rustc_hir::def_id::DefId;
77
use rustc_hir::LangItem::OptionNone;
88
use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, QPath};
99
use rustc_lint::{LateContext, LateLintPass, LintContext};
@@ -194,64 +194,37 @@ fn check_replace_with_uninit(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'
194194
}
195195
}
196196

197-
/// Returns true if the `def_id` associated with the `path` is recognized as a "default-equivalent"
198-
/// constructor from the std library
199-
fn is_default_equivalent_ctor(cx: &LateContext<'_>, def_id: DefId, path: &QPath<'_>) -> bool {
200-
let std_types_symbols = &[
201-
sym::string_type,
202-
sym::vec_type,
203-
sym::vecdeque_type,
204-
sym::LinkedList,
205-
sym::hashmap_type,
206-
sym::BTreeMap,
207-
sym::hashset_type,
208-
sym::BTreeSet,
209-
sym::BinaryHeap,
210-
];
211-
212-
if let QPath::TypeRelative(_, method) = path {
213-
if method.ident.name == sym::new {
214-
if let Some(impl_did) = cx.tcx.impl_of_method(def_id) {
215-
if let Some(adt) = cx.tcx.type_of(impl_did).ty_adt_def() {
216-
return std_types_symbols
217-
.iter()
218-
.any(|&symbol| cx.tcx.is_diagnostic_item(symbol, adt.did));
219-
}
220-
}
197+
fn check_replace_with_default(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
198+
// disable lint for primitives
199+
let expr_type = cx.typeck_results().expr_ty_adjusted(src);
200+
if is_non_aggregate_primitive_type(expr_type) {
201+
return;
202+
}
203+
// disable lint for Option since it is covered in another lint
204+
if let ExprKind::Path(q) = &src.kind {
205+
if is_lang_ctor(cx, q, OptionNone) {
206+
return;
221207
}
222208
}
223-
false
224-
}
209+
if is_default_equivalent(cx, src) && !in_external_macro(cx.tcx.sess, expr_span) {
210+
span_lint_and_then(
211+
cx,
212+
MEM_REPLACE_WITH_DEFAULT,
213+
expr_span,
214+
"replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`",
215+
|diag| {
216+
if !in_macro(expr_span) {
217+
let suggestion = format!("std::mem::take({})", snippet(cx, dest.span, ""));
225218

226-
fn check_replace_with_default(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
227-
if_chain! {
228-
if let ExprKind::Call(repl_func, _) = src.kind;
229-
if !in_external_macro(cx.tcx.sess, expr_span);
230-
if let ExprKind::Path(ref repl_func_qpath) = repl_func.kind;
231-
if let Some(repl_def_id) = cx.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id();
232-
if is_diag_trait_item(cx, repl_def_id, sym::Default)
233-
|| is_default_equivalent_ctor(cx, repl_def_id, repl_func_qpath);
234-
235-
then {
236-
span_lint_and_then(
237-
cx,
238-
MEM_REPLACE_WITH_DEFAULT,
239-
expr_span,
240-
"replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`",
241-
|diag| {
242-
if !in_macro(expr_span) {
243-
let suggestion = format!("std::mem::take({})", snippet(cx, dest.span, ""));
244-
245-
diag.span_suggestion(
246-
expr_span,
247-
"consider using",
248-
suggestion,
249-
Applicability::MachineApplicable
250-
);
251-
}
219+
diag.span_suggestion(
220+
expr_span,
221+
"consider using",
222+
suggestion,
223+
Applicability::MachineApplicable,
224+
);
252225
}
253-
);
254-
}
226+
},
227+
);
255228
}
256229
}
257230

‎clippy_utils/src/lib.rs

+60-1
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ use rustc_hir::def::{DefKind, Res};
7070
use rustc_hir::def_id::DefId;
7171
use rustc_hir::hir_id::{HirIdMap, HirIdSet};
7272
use rustc_hir::intravisit::{self, walk_expr, ErasedMap, FnKind, NestedVisitorMap, Visitor};
73-
use rustc_hir::LangItem::{ResultErr, ResultOk};
73+
use rustc_hir::LangItem::{OptionNone, ResultErr, ResultOk};
7474
use rustc_hir::{
7575
def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl,
7676
ImplItem, ImplItemKind, IsAsync, Item, ItemKind, LangItem, Local, MatchSource, Mutability, Node, Param, Pat,
@@ -644,6 +644,65 @@ pub fn can_mut_borrow_both(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>) -
644644
false
645645
}
646646

647+
/// Returns true if the `def_id` associated with the `path` is recognized as a "default-equivalent"
648+
/// constructor from the std library
649+
fn is_default_equivalent_ctor(cx: &LateContext<'_>, def_id: DefId, path: &QPath<'_>) -> bool {
650+
let std_types_symbols = &[
651+
sym::string_type,
652+
sym::vec_type,
653+
sym::vecdeque_type,
654+
sym::LinkedList,
655+
sym::hashmap_type,
656+
sym::BTreeMap,
657+
sym::hashset_type,
658+
sym::BTreeSet,
659+
sym::BinaryHeap,
660+
];
661+
662+
if let QPath::TypeRelative(_, method) = path {
663+
if method.ident.name == sym::new {
664+
if let Some(impl_did) = cx.tcx.impl_of_method(def_id) {
665+
if let Some(adt) = cx.tcx.type_of(impl_did).ty_adt_def() {
666+
return std_types_symbols
667+
.iter()
668+
.any(|&symbol| cx.tcx.is_diagnostic_item(symbol, adt.did));
669+
}
670+
}
671+
}
672+
}
673+
false
674+
}
675+
676+
/// Returns true if the expr is equal to `Default::default()` of it's type when evaluated.
677+
/// It doesn't cover all cases, for example indirect function calls (some of std
678+
/// functions are supported) but it is the best we have.
679+
pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
680+
match &e.kind {
681+
ExprKind::Lit(lit) => match lit.node {
682+
LitKind::Bool(false) | LitKind::Int(0, _) => true,
683+
LitKind::Str(s, _) => s.is_empty(),
684+
_ => false,
685+
},
686+
ExprKind::Tup(items) | ExprKind::Array(items) => items.iter().all(|x| is_default_equivalent(cx, x)),
687+
ExprKind::Repeat(x, _) => is_default_equivalent(cx, x),
688+
ExprKind::Call(repl_func, _) => if_chain! {
689+
if let ExprKind::Path(ref repl_func_qpath) = repl_func.kind;
690+
if let Some(repl_def_id) = cx.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id();
691+
if is_diag_trait_item(cx, repl_def_id, sym::Default)
692+
|| is_default_equivalent_ctor(cx, repl_def_id, repl_func_qpath);
693+
then {
694+
true
695+
}
696+
else {
697+
false
698+
}
699+
},
700+
ExprKind::Path(qpath) => is_lang_ctor(cx, qpath, OptionNone),
701+
ExprKind::AddrOf(rustc_hir::BorrowKind::Ref, _, expr) => matches!(expr.kind, ExprKind::Array([])),
702+
_ => false,
703+
}
704+
}
705+
647706
/// Checks if the top level expression can be moved into a closure as is.
648707
/// Currently checks for:
649708
/// * Break/Continue outside the given loop HIR ids.

‎clippy_utils/src/ty.rs

+7
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,13 @@ fn is_normalizable_helper<'tcx>(
224224
result
225225
}
226226

227+
/// Returns true iff the given type is a non aggregate primitive (a bool or char, any integer or
228+
/// floating-point number type). For checking aggregation of primitive types (e.g. tuples and slices
229+
/// of primitive type) see `is_recursively_primitive_type`
230+
pub fn is_non_aggregate_primitive_type(ty: Ty<'_>) -> bool {
231+
matches!(ty.kind(), ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_))
232+
}
233+
227234
/// Returns true iff the given type is a primitive (a bool or char, any integer or floating-point
228235
/// number type, a str, or an array, slice, or tuple of those types).
229236
pub fn is_recursively_primitive_type(ty: Ty<'_>) -> bool {

0 commit comments

Comments
 (0)