Skip to content

Commit f973ec6

Browse files
HKalbasiHKalbasi
committed
add derivable impls lint
1 parent 15b1c6f commit f973ec6

11 files changed

+479
-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

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

‎clippy_lints/src/lib.rs

+5
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,7 @@ mod dbg_macro;
189189
mod default;
190190
mod default_numeric_fallback;
191191
mod dereference;
192+
mod derivable_impls;
192193
mod derive;
193194
mod disallowed_method;
194195
mod disallowed_script_idents;
@@ -588,6 +589,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
588589
default::FIELD_REASSIGN_WITH_DEFAULT,
589590
default_numeric_fallback::DEFAULT_NUMERIC_FALLBACK,
590591
dereference::EXPLICIT_DEREF_METHODS,
592+
derivable_impls::DERIVABLE_IMPLS,
591593
derive::DERIVE_HASH_XOR_EQ,
592594
derive::DERIVE_ORD_XOR_PARTIAL_ORD,
593595
derive::EXPL_IMPL_CLONE_ON_COPY,
@@ -1203,6 +1205,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
12031205
LintId::of(copies::IFS_SAME_COND),
12041206
LintId::of(copies::IF_SAME_THEN_ELSE),
12051207
LintId::of(default::FIELD_REASSIGN_WITH_DEFAULT),
1208+
LintId::of(derivable_impls::DERIVABLE_IMPLS),
12061209
LintId::of(derive::DERIVE_HASH_XOR_EQ),
12071210
LintId::of(derive::DERIVE_ORD_XOR_PARTIAL_ORD),
12081211
LintId::of(doc::MISSING_SAFETY_DOC),
@@ -1588,6 +1591,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
15881591
LintId::of(booleans::NONMINIMAL_BOOL),
15891592
LintId::of(casts::CHAR_LIT_AS_U8),
15901593
LintId::of(casts::UNNECESSARY_CAST),
1594+
LintId::of(derivable_impls::DERIVABLE_IMPLS),
15911595
LintId::of(double_comparison::DOUBLE_COMPARISONS),
15921596
LintId::of(double_parens::DOUBLE_PARENS),
15931597
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 panic_unimplemented::PanicUnimplemented);
19381942
store.register_late_pass(|| box strings::StringLitAsBytes);
19391943
store.register_late_pass(|| box derive::Derive);
1944+
store.register_late_pass(|| box derivable_impls::DerivableImpls);
19401945
store.register_late_pass(|| box get_last_with_len::GetLastWithLen);
19411946
store.register_late_pass(|| box drop_forget_ref::DropForgetRef);
19421947
store.register_late_pass(|| box 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,
@@ -630,6 +630,65 @@ pub fn can_mut_borrow_both(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>) -
630630
false
631631
}
632632

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