Auto merge of #7570 - HKalbasi:derivable-impls, r=camsteffen

Add the `derivable_impls` lint

Fix #7550

changelog: Add new derivable_impls lint. mem_replace_with_default now covers non constructor cases.
This commit is contained in:
bors 2021-09-04 21:40:04 +00:00
commit a8c269ad50
11 changed files with 516 additions and 58 deletions

View file

@ -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

View file

@ -0,0 +1,108 @@
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::{DefKind, Res},
Body, Expr, ExprKind, Impl, ImplItemKind, Item, ItemKind, Node, QPath,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::TypeFoldable;
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
/// }
/// ```
///
/// ### Known problems
/// Derive macros [sometimes use incorrect bounds](https://github.com/rust-lang/rust/issues/26925)
/// in generic types and the user defined `impl` maybe is more generalized or
/// specialized than what derive will produce. This lint can't detect the manual `impl`
/// has exactly equal bounds, and therefore this lint is disabled for types with
/// generic parameters.
///
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(..) | Res::Def(DefKind::Ctor(..), _))
} 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],
..
}) = 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 Some(adt_def) = cx.tcx.type_of(item.def_id).ty_adt_def();
then {
if cx.tcx.type_of(item.def_id).definitely_has_param_types_or_consts(cx.tcx) {
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 = cx.tcx.def_path_str(adt_def.did);
span_lint_and_help(
cx,
DERIVABLE_IMPLS,
item.span,
"this `impl` can be derived",
None,
&format!("try annotating `{}` with `#[derive(Default)]`", path_string),
);
}
}
}
}
}

View file

@ -187,6 +187,7 @@ mod dbg_macro;
mod default;
mod default_numeric_fallback;
mod dereference;
mod derivable_impls;
mod derive;
mod disallowed_method;
mod disallowed_script_idents;
@ -586,6 +587,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,
@ -1204,6 +1206,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),
@ -1589,6 +1592,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),
@ -1937,6 +1941,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| Box::new(panic_unimplemented::PanicUnimplemented));
store.register_late_pass(|| Box::new(strings::StringLitAsBytes));
store.register_late_pass(|| Box::new(derive::Derive));
store.register_late_pass(|| Box::new(derivable_impls::DerivableImpls));
store.register_late_pass(|| Box::new(get_last_with_len::GetLastWithLen));
store.register_late_pass(|| Box::new(drop_forget_ref::DropForgetRef));
store.register_late_pass(|| Box::new(empty_enum::EmptyEnum));

View file

@ -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_non_aggregate_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};
@ -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_non_aggregate_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,
);
}
);
}
},
);
}
}

View file

@ -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,
@ -644,6 +644,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.

View file

@ -224,6 +224,13 @@ fn is_normalizable_helper<'tcx>(
result
}
/// Returns true iff the given type is a non aggregate primitive (a bool or char, any integer or
/// floating-point number type). For checking aggregation of primitive types (e.g. tuples and slices
/// of primitive type) see `is_recursively_primitive_type`
pub fn is_non_aggregate_primitive_type(ty: Ty<'_>) -> bool {
matches!(ty.kind(), ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_))
}
/// Returns true iff the given type is a primitive (a bool or char, any integer or floating-point
/// number type, a str, or an array, slice, or tuple of those types).
pub fn is_recursively_primitive_type(ty: Ty<'_>) -> bool {

170
tests/ui/derivable_impls.rs Normal file
View file

@ -0,0 +1,170 @@
use std::collections::HashMap;
struct FooDefault<'a> {
a: bool,
b: i32,
c: u64,
d: Vec<i32>,
e: FooND1,
f: FooND2,
g: HashMap<i32, i32>,
h: (i32, Vec<i32>),
i: [Vec<i32>; 3],
j: [i32; 5],
k: Option<i32>,
l: &'a [i32],
}
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],
k: None,
l: &[],
}
}
}
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<i32>);
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);
macro_rules! mac {
() => {
0
};
($e:expr) => {
struct X(u32);
impl Default for X {
fn default() -> Self {
Self($e)
}
}
};
}
mac!(0);
struct Y(u32);
impl Default for Y {
fn default() -> Self {
Self(mac!())
}
}
struct RustIssue26925<T> {
a: Option<T>,
}
// We should watch out for cases where a manual impl is needed because a
// derive adds different type bounds (https://github.com/rust-lang/rust/issues/26925).
// For example, a struct with Option<T> does not require T: Default, but a derive adds
// that type bound anyways. So until #26925 get fixed we should disable lint
// for the following case
impl<T> Default for RustIssue26925<T> {
fn default() -> Self {
Self { a: None }
}
}
struct SpecializedImpl<A, B> {
a: A,
b: B,
}
impl<T: Default> Default for SpecializedImpl<T, T> {
fn default() -> Self {
Self {
a: T::default(),
b: T::default(),
}
}
}
struct WithoutSelfCurly {
a: bool,
}
impl Default for WithoutSelfCurly {
fn default() -> Self {
WithoutSelfCurly { a: false }
}
}
struct WithoutSelfParan(bool);
impl Default for WithoutSelfParan {
fn default() -> Self {
WithoutSelfParan(false)
}
}
fn main() {}

View file

@ -0,0 +1,77 @@
error: this `impl` can be derived
--> $DIR/derivable_impls.rs:18: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`
= help: try annotating `FooDefault` with `#[derive(Default)]`
error: this `impl` can be derived
--> $DIR/derivable_impls.rs:39:1
|
LL | / impl std::default::Default for TupleDefault {
LL | | fn default() -> Self {
LL | | Self(false, 0, 0u64)
LL | | }
LL | | }
| |_^
|
= help: try annotating `TupleDefault` with `#[derive(Default)]`
error: this `impl` can be derived
--> $DIR/derivable_impls.rs:91:1
|
LL | / impl Default for StrDefault<'_> {
LL | | fn default() -> Self {
LL | | Self("")
LL | | }
LL | | }
| |_^
|
= help: try annotating `StrDefault` with `#[derive(Default)]`
error: this `impl` can be derived
--> $DIR/derivable_impls.rs:117:1
|
LL | / impl Default for Y {
LL | | fn default() -> Self {
LL | | Self(mac!())
LL | | }
LL | | }
| |_^
|
= help: try annotating `Y` with `#[derive(Default)]`
error: this `impl` can be derived
--> $DIR/derivable_impls.rs:156:1
|
LL | / impl Default for WithoutSelfCurly {
LL | | fn default() -> Self {
LL | | WithoutSelfCurly { a: false }
LL | | }
LL | | }
| |_^
|
= help: try annotating `WithoutSelfCurly` with `#[derive(Default)]`
error: this `impl` can be derived
--> $DIR/derivable_impls.rs:164:1
|
LL | / impl Default for WithoutSelfParan {
LL | | fn default() -> Self {
LL | | WithoutSelfParan(false)
LL | | }
LL | | }
| |_^
|
= help: try annotating `WithoutSelfParan` with `#[derive(Default)]`
error: aborting due to 6 previous errors

View file

@ -51,9 +51,29 @@ fn replace_with_default() {
let mut binary_heap: BinaryHeap<i32> = BinaryHeap::new();
let _ = std::mem::take(&mut binary_heap);
let mut tuple = (vec![1, 2], BinaryHeap::<i32>::new());
let _ = std::mem::take(&mut tuple);
let mut refstr = "hello";
let _ = std::mem::take(&mut refstr);
let mut slice: &[i32] = &[1, 2, 3];
let _ = std::mem::take(&mut slice);
}
// lint is disabled for primitives because in this case `take`
// has no clear benefit over `replace` and sometimes is harder to read
fn dont_lint_primitive() {
let mut pbool = true;
let _ = std::mem::replace(&mut pbool, false);
let mut pint = 5;
let _ = std::mem::replace(&mut pint, 0);
}
fn main() {
replace_option_with_none();
replace_with_default();
dont_lint_primitive();
}

View file

@ -51,9 +51,29 @@ fn replace_with_default() {
let mut binary_heap: BinaryHeap<i32> = BinaryHeap::new();
let _ = std::mem::replace(&mut binary_heap, BinaryHeap::new());
let mut tuple = (vec![1, 2], BinaryHeap::<i32>::new());
let _ = std::mem::replace(&mut tuple, (vec![], BinaryHeap::new()));
let mut refstr = "hello";
let _ = std::mem::replace(&mut refstr, "");
let mut slice: &[i32] = &[1, 2, 3];
let _ = std::mem::replace(&mut slice, &[]);
}
// lint is disabled for primitives because in this case `take`
// has no clear benefit over `replace` and sometimes is harder to read
fn dont_lint_primitive() {
let mut pbool = true;
let _ = std::mem::replace(&mut pbool, false);
let mut pint = 5;
let _ = std::mem::replace(&mut pint, 0);
}
fn main() {
replace_option_with_none();
replace_with_default();
dont_lint_primitive();
}

View file

@ -98,5 +98,23 @@ error: replacing a value of type `T` with `T::default()` is better expressed usi
LL | let _ = std::mem::replace(&mut binary_heap, BinaryHeap::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut binary_heap)`
error: aborting due to 16 previous errors
error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:56:13
|
LL | let _ = std::mem::replace(&mut tuple, (vec![], BinaryHeap::new()));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut tuple)`
error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:59:13
|
LL | let _ = std::mem::replace(&mut refstr, "");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut refstr)`
error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:62:13
|
LL | let _ = std::mem::replace(&mut slice, &[]);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut slice)`
error: aborting due to 19 previous errors