Skip to content

Commit e932dd3

Browse files
committed
Use MIR body to identify more "default equivalent" calls
When looking for `Default` impls that could be derived, we look at the body of their `fn default()` and if it is an fn call or literal we check if they are equivalent to what `#[derive(Default)]` would have used. Now, when checking those fn calls in the `fn default()` body, we also compare against the corresponding type's `Default::default` body to see if our call is equivalent to that one. For example, given ```rust struct S; impl S { fn new() -> S { S } } impl Default for S { fn default() -> S { S::new() } } ``` `<S as Default>::default()` and `S::new()` are considered equivalent. Given that, if the user also writes ```rust struct R { s: S, } impl Default for R { fn default() -> R { R { s: S::new() } } } ``` the `derivable_impls` lint will now trigger.
1 parent ab55d3f commit e932dd3

File tree

8 files changed

+265
-35
lines changed

8 files changed

+265
-35
lines changed

clippy_lints/src/methods/or_fun_call.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ pub(super) fn check<'tcx>(
104104
if (is_new(fun) && output_type_implements_default(fun))
105105
|| match call_expr {
106106
Some(call_expr) => is_default_equivalent(cx, call_expr),
107-
None => is_default_equivalent_call(cx, fun) || closure_body_returns_empty_to_string(cx, fun),
107+
None => is_default_equivalent_call(cx, fun, None) || closure_body_returns_empty_to_string(cx, fun),
108108
}
109109
{
110110
span_lint_and_sugg(

clippy_utils/src/lib.rs

+89-9
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ use rustc_hir::{
112112
use rustc_lexer::{TokenKind, tokenize};
113113
use rustc_lint::{LateContext, Level, Lint, LintContext};
114114
use rustc_middle::hir::place::PlaceBase;
115-
use rustc_middle::mir::Const;
115+
use rustc_middle::mir::{AggregateKind, Const, Operand, RETURN_PLACE, Rvalue, StatementKind, TerminatorKind};
116116
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow};
117117
use rustc_middle::ty::fast_reject::SimplifiedType;
118118
use rustc_middle::ty::layout::IntegerExt;
@@ -897,22 +897,102 @@ fn is_default_equivalent_ctor(cx: &LateContext<'_>, def_id: DefId, path: &QPath<
897897
}
898898

899899
/// Returns true if the expr is equal to `Default::default` when evaluated.
900-
pub fn is_default_equivalent_call(cx: &LateContext<'_>, repl_func: &Expr<'_>) -> bool {
900+
pub fn is_default_equivalent_call(
901+
cx: &LateContext<'_>,
902+
repl_func: &Expr<'_>,
903+
whole_call_expr: Option<&Expr<'_>>,
904+
) -> bool {
901905
if let ExprKind::Path(ref repl_func_qpath) = repl_func.kind
902906
&& let Some(repl_def_id) = cx.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id()
903907
&& (is_diag_trait_item(cx, repl_def_id, sym::Default)
904908
|| is_default_equivalent_ctor(cx, repl_def_id, repl_func_qpath))
905909
{
906-
true
907-
} else {
908-
false
910+
return true;
911+
}
912+
913+
// Get the type of the whole method call expression, find the exact method definition, look at
914+
// its body and check if it is similar to the corresponding `Default::default()` body.
915+
let Some(e) = whole_call_expr else { return false };
916+
let Some(default_fn_def_id) = cx.tcx.get_diagnostic_item(sym::default_fn) else {
917+
return false;
918+
};
919+
let Some(ty) = cx.tcx.typeck(e.hir_id.owner.def_id).expr_ty_adjusted_opt(e) else {
920+
return false;
921+
};
922+
let args = rustc_ty::GenericArgs::for_item(cx.tcx, default_fn_def_id, |param, _| {
923+
if let rustc_ty::GenericParamDefKind::Lifetime = param.kind {
924+
cx.tcx.lifetimes.re_erased.into()
925+
} else if param.index == 0 && param.name == kw::SelfUpper {
926+
ty.into()
927+
} else {
928+
param.to_error(cx.tcx)
929+
}
930+
});
931+
let instance = rustc_ty::Instance::try_resolve(cx.tcx, cx.typing_env(), default_fn_def_id, args);
932+
933+
let Ok(Some(instance)) = instance else { return false };
934+
if let rustc_ty::InstanceKind::Item(def) = instance.def
935+
&& !cx.tcx.is_mir_available(def)
936+
{
937+
// Avoid ICE while running rustdoc for not providing `optimized_mir` query.
938+
return false;
939+
}
940+
let ExprKind::Path(ref repl_func_qpath) = repl_func.kind else {
941+
return false;
942+
};
943+
let Some(repl_def_id) = cx.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id() else {
944+
return false;
945+
};
946+
947+
// Get the MIR Body for the `<Ty as Default>::default()` function.
948+
// If it is a value or call (either fn or ctor), we compare its `DefId` against the one for the
949+
// resolution of the expression we had in the path. This lets us identify, for example, that
950+
// the body of `<Vec<T> as Default>::default()` is a `Vec::new()`, and the field was being
951+
// initialized to `Vec::new()` as well.
952+
let body = cx.tcx.instance_mir(instance.def);
953+
for block_data in body.basic_blocks.iter() {
954+
if block_data.statements.len() == 1
955+
&& let StatementKind::Assign(assign) = &block_data.statements[0].kind
956+
&& assign.0.local == RETURN_PLACE
957+
&& let Rvalue::Aggregate(kind, _places) = &assign.1
958+
&& let AggregateKind::Adt(did, variant_index, _, _, _) = &**kind
959+
&& let def = cx.tcx.adt_def(did)
960+
&& let variant = &def.variant(*variant_index)
961+
&& variant.fields.is_empty()
962+
&& let Some((_, did)) = variant.ctor
963+
&& did == repl_def_id
964+
{
965+
return true;
966+
} else if block_data.statements.is_empty()
967+
&& let Some(term) = &block_data.terminator
968+
{
969+
match &term.kind {
970+
TerminatorKind::Call {
971+
func: Operand::Constant(c),
972+
..
973+
} if let rustc_ty::FnDef(did, _args) = c.ty().kind()
974+
&& *did == repl_def_id =>
975+
{
976+
return true;
977+
},
978+
TerminatorKind::TailCall {
979+
func: Operand::Constant(c),
980+
..
981+
} if let rustc_ty::FnDef(did, _args) = c.ty().kind()
982+
&& *did == repl_def_id =>
983+
{
984+
return true;
985+
},
986+
_ => {},
987+
}
988+
}
909989
}
990+
false
910991
}
911992

912-
/// Returns true if the expr is equal to `Default::default()` of it's type when evaluated.
993+
/// Returns true if the expr is equal to `Default::default()` of its type when evaluated.
913994
///
914-
/// It doesn't cover all cases, for example indirect function calls (some of std
915-
/// functions are supported) but it is the best we have.
995+
/// It doesn't cover all cases, like struct literals, but it is a close approximation.
916996
pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
917997
match &e.kind {
918998
ExprKind::Lit(lit) => match lit.node {
@@ -933,7 +1013,7 @@ pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
9331013
false
9341014
}
9351015
},
936-
ExprKind::Call(repl_func, []) => is_default_equivalent_call(cx, repl_func),
1016+
ExprKind::Call(repl_func, []) => is_default_equivalent_call(cx, repl_func, Some(e)),
9371017
ExprKind::Call(from_func, [arg]) => is_default_equivalent_from(cx, from_func, arg),
9381018
ExprKind::Path(qpath) => is_res_lang_ctor(cx, cx.qpath_res(qpath, e.hir_id), OptionNone),
9391019
ExprKind::AddrOf(rustc_hir::BorrowKind::Ref, _, expr) => matches!(expr.kind, ExprKind::Array([])),

tests/ui/derivable_impls.fixed

+34
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,40 @@ impl Default for SpecializedImpl2<String> {
144144
}
145145
}
146146

147+
#[derive(Default)]
148+
pub struct DirectDefaultDefaultCall {
149+
v: Vec<i32>,
150+
}
151+
152+
153+
#[derive(Default)]
154+
pub struct EquivalentToDefaultDefaultCallVec {
155+
v: Vec<i32>,
156+
}
157+
158+
159+
pub struct S {
160+
x: i32,
161+
}
162+
163+
impl S {
164+
fn new() -> S {
165+
S { x: 42 }
166+
}
167+
}
168+
169+
impl Default for S {
170+
fn default() -> Self {
171+
Self::new()
172+
}
173+
}
174+
175+
#[derive(Default)]
176+
pub struct EquivalentToDefaultDefaultCallLocal {
177+
v: S,
178+
}
179+
180+
147181
// https://github.com/rust-lang/rust-clippy/issues/7654
148182

149183
pub struct Color {

tests/ui/derivable_impls.rs

+49
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,55 @@ impl Default for SpecializedImpl2<String> {
181181
}
182182
}
183183

184+
pub struct DirectDefaultDefaultCall {
185+
v: Vec<i32>,
186+
}
187+
188+
impl Default for DirectDefaultDefaultCall {
189+
fn default() -> Self {
190+
// When calling `Default::default()` in all fields, we know it is the same as deriving.
191+
Self { v: Default::default() }
192+
}
193+
}
194+
195+
pub struct EquivalentToDefaultDefaultCallVec {
196+
v: Vec<i32>,
197+
}
198+
199+
impl Default for EquivalentToDefaultDefaultCallVec {
200+
fn default() -> Self {
201+
// The body of `<Vec as Default>::default()` is `Vec::new()`, so they are equivalent.
202+
Self { v: Vec::new() }
203+
}
204+
}
205+
206+
pub struct S {
207+
x: i32,
208+
}
209+
210+
impl S {
211+
fn new() -> S {
212+
S { x: 42 }
213+
}
214+
}
215+
216+
impl Default for S {
217+
fn default() -> Self {
218+
Self::new()
219+
}
220+
}
221+
222+
pub struct EquivalentToDefaultDefaultCallLocal {
223+
v: S,
224+
}
225+
226+
impl Default for EquivalentToDefaultDefaultCallLocal {
227+
fn default() -> Self {
228+
// The body of `<S as Default>::default()` is `S::new()`, so they are equivalent.
229+
Self { v: S::new() }
230+
}
231+
}
232+
184233
// https://github.com/rust-lang/rust-clippy/issues/7654
185234

186235
pub struct Color {

tests/ui/derivable_impls.stderr

+54-3
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,58 @@ LL ~ struct WithoutSelfParan(bool);
9898
|
9999

100100
error: this `impl` can be derived
101-
--> tests/ui/derivable_impls.rs:216:1
101+
--> tests/ui/derivable_impls.rs:188:1
102+
|
103+
LL | / impl Default for DirectDefaultDefaultCall {
104+
LL | | fn default() -> Self {
105+
LL | | // When calling `Default::default()` in all fields, we know it is the same as deriving.
106+
LL | | Self { v: Default::default() }
107+
LL | | }
108+
LL | | }
109+
| |_^
110+
|
111+
help: replace the manual implementation with a derive attribute
112+
|
113+
LL + #[derive(Default)]
114+
LL ~ pub struct DirectDefaultDefaultCall {
115+
|
116+
117+
error: this `impl` can be derived
118+
--> tests/ui/derivable_impls.rs:199:1
119+
|
120+
LL | / impl Default for EquivalentToDefaultDefaultCallVec {
121+
LL | | fn default() -> Self {
122+
LL | | // The body of `<Vec as Default>::default()` is `Vec::new()`, so they are equivalent.
123+
LL | | Self { v: Vec::new() }
124+
LL | | }
125+
LL | | }
126+
| |_^
127+
|
128+
help: replace the manual implementation with a derive attribute
129+
|
130+
LL + #[derive(Default)]
131+
LL ~ pub struct EquivalentToDefaultDefaultCallVec {
132+
|
133+
134+
error: this `impl` can be derived
135+
--> tests/ui/derivable_impls.rs:226:1
136+
|
137+
LL | / impl Default for EquivalentToDefaultDefaultCallLocal {
138+
LL | | fn default() -> Self {
139+
LL | | // The body of `<S as Default>::default()` is `S::new()`, so they are equivalent.
140+
LL | | Self { v: S::new() }
141+
LL | | }
142+
LL | | }
143+
| |_^
144+
|
145+
help: replace the manual implementation with a derive attribute
146+
|
147+
LL + #[derive(Default)]
148+
LL ~ pub struct EquivalentToDefaultDefaultCallLocal {
149+
|
150+
151+
error: this `impl` can be derived
152+
--> tests/ui/derivable_impls.rs:265:1
102153
|
103154
LL | / impl Default for RepeatDefault1 {
104155
LL | | fn default() -> Self {
@@ -114,7 +165,7 @@ LL ~ pub struct RepeatDefault1 {
114165
|
115166

116167
error: this `impl` can be derived
117-
--> tests/ui/derivable_impls.rs:250:1
168+
--> tests/ui/derivable_impls.rs:299:1
118169
|
119170
LL | / impl Default for SimpleEnum {
120171
LL | | fn default() -> Self {
@@ -132,5 +183,5 @@ LL ~ #[default]
132183
LL ~ Bar,
133184
|
134185

135-
error: aborting due to 8 previous errors
186+
error: aborting due to 11 previous errors
136187

tests/ui/mem_replace.fixed

+2
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,12 @@ fn replace_option_with_none() {
1818
fn replace_with_default() {
1919
let mut s = String::from("foo");
2020
let _ = std::mem::take(&mut s);
21+
let _ = std::mem::take(&mut s);
2122

2223
let s = &mut String::from("foo");
2324
let _ = std::mem::take(s);
2425
let _ = std::mem::take(s);
26+
let _ = std::mem::take(s);
2527

2628
let mut v = vec![123];
2729
let _ = std::mem::take(&mut v);

tests/ui/mem_replace.rs

+2
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,11 @@ fn replace_option_with_none() {
1818
fn replace_with_default() {
1919
let mut s = String::from("foo");
2020
let _ = std::mem::replace(&mut s, String::default());
21+
let _ = std::mem::replace(&mut s, String::new());
2122

2223
let s = &mut String::from("foo");
2324
let _ = std::mem::replace(s, String::default());
25+
let _ = std::mem::replace(s, String::new());
2426
let _ = std::mem::replace(s, Default::default());
2527

2628
let mut v = vec![123];

0 commit comments

Comments
 (0)