Skip to content

Commit 39d73d5

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 b7ec4c1 commit 39d73d5

File tree

8 files changed

+264
-34
lines changed

8 files changed

+264
-34
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

+88-8
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ use rustc_hir::{
113113
use rustc_lexer::{TokenKind, tokenize};
114114
use rustc_lint::{LateContext, Level, Lint, LintContext};
115115
use rustc_middle::hir::place::PlaceBase;
116+
use rustc_middle::mir::{AggregateKind, Operand, RETURN_PLACE, Rvalue, StatementKind, TerminatorKind};
116117
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow};
117118
use rustc_middle::ty::fast_reject::SimplifiedType;
118119
use rustc_middle::ty::layout::IntegerExt;
@@ -919,22 +920,101 @@ fn is_default_equivalent_ctor(cx: &LateContext<'_>, def_id: DefId, path: &QPath<
919920
}
920921

921922
/// Returns true if the expr is equal to `Default::default` when evaluated.
922-
pub fn is_default_equivalent_call(cx: &LateContext<'_>, repl_func: &Expr<'_>) -> bool {
923+
pub fn is_default_equivalent_call(
924+
cx: &LateContext<'_>,
925+
repl_func: &Expr<'_>,
926+
whole_call_expr: Option<&Expr<'_>>,
927+
) -> bool {
923928
if let ExprKind::Path(ref repl_func_qpath) = repl_func.kind
924929
&& let Some(repl_def_id) = cx.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id()
925930
&& (is_diag_trait_item(cx, repl_def_id, sym::Default)
926931
|| is_default_equivalent_ctor(cx, repl_def_id, repl_func_qpath))
927932
{
928-
true
929-
} else {
930-
false
933+
return true;
934+
}
935+
936+
// Get the type of the whole method call expression, find the exact method definition, look at
937+
// its body and check if it is similar to the corresponding `Default::default()` body.
938+
let Some(e) = whole_call_expr else { return false };
939+
let Some(default_fn_def_id) = cx.tcx.get_diagnostic_item(sym::default_fn) else {
940+
return false;
941+
};
942+
let Some(ty) = cx.tcx.typeck(e.hir_id.owner.def_id).expr_ty_adjusted_opt(e) else {
943+
return false;
944+
};
945+
let args = rustc_ty::GenericArgs::for_item(cx.tcx, default_fn_def_id, |param, _| {
946+
if let rustc_ty::GenericParamDefKind::Lifetime = param.kind {
947+
cx.tcx.lifetimes.re_erased.into()
948+
} else if param.index == 0 && param.name == kw::SelfUpper {
949+
ty.into()
950+
} else {
951+
param.to_error(cx.tcx)
952+
}
953+
});
954+
let instance = rustc_ty::Instance::try_resolve(cx.tcx, cx.typing_env(), default_fn_def_id, args);
955+
956+
let Ok(Some(instance)) = instance else { return false };
957+
if let rustc_ty::InstanceKind::Item(def) = instance.def
958+
&& !cx.tcx.is_mir_available(def)
959+
{
960+
return false;
961+
}
962+
let ExprKind::Path(ref repl_func_qpath) = repl_func.kind else {
963+
return false;
964+
};
965+
let Some(repl_def_id) = cx.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id() else {
966+
return false;
967+
};
968+
969+
// Get the MIR Body for the `<Ty as Default>::default()` function.
970+
// If it is a value or call (either fn or ctor), we compare its `DefId` against the one for the
971+
// resolution of the expression we had in the path. This lets us identify, for example, that
972+
// the body of `<Vec<T> as Default>::default()` is a `Vec::new()`, and the field was being
973+
// initialized to `Vec::new()` as well.
974+
let body = cx.tcx.instance_mir(instance.def);
975+
for block_data in body.basic_blocks.iter() {
976+
if block_data.statements.len() == 1
977+
&& let StatementKind::Assign(assign) = &block_data.statements[0].kind
978+
&& assign.0.local == RETURN_PLACE
979+
&& let Rvalue::Aggregate(kind, _places) = &assign.1
980+
&& let AggregateKind::Adt(did, variant_index, _, _, _) = &**kind
981+
&& let def = cx.tcx.adt_def(did)
982+
&& let variant = &def.variant(*variant_index)
983+
&& variant.fields.is_empty()
984+
&& let Some((_, did)) = variant.ctor
985+
&& did == repl_def_id
986+
{
987+
return true;
988+
} else if block_data.statements.is_empty()
989+
&& let Some(term) = &block_data.terminator
990+
{
991+
match &term.kind {
992+
TerminatorKind::Call {
993+
func: Operand::Constant(c),
994+
..
995+
} if let rustc_ty::FnDef(did, _args) = c.ty().kind()
996+
&& *did == repl_def_id =>
997+
{
998+
return true;
999+
},
1000+
TerminatorKind::TailCall {
1001+
func: Operand::Constant(c),
1002+
..
1003+
} if let rustc_ty::FnDef(did, _args) = c.ty().kind()
1004+
&& *did == repl_def_id =>
1005+
{
1006+
return true;
1007+
},
1008+
_ => {},
1009+
}
1010+
}
9311011
}
1012+
false
9321013
}
9331014

934-
/// Returns true if the expr is equal to `Default::default()` of it's type when evaluated.
1015+
/// Returns true if the expr is equal to `Default::default()` of its type when evaluated.
9351016
///
936-
/// It doesn't cover all cases, for example indirect function calls (some of std
937-
/// functions are supported) but it is the best we have.
1017+
/// It doesn't cover all cases, like struct literals, but it is a close approximation.
9381018
pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
9391019
match &e.kind {
9401020
ExprKind::Lit(lit) => match lit.node {
@@ -955,7 +1035,7 @@ pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
9551035
false
9561036
}
9571037
},
958-
ExprKind::Call(repl_func, []) => is_default_equivalent_call(cx, repl_func),
1038+
ExprKind::Call(repl_func, []) => is_default_equivalent_call(cx, repl_func, Some(e)),
9591039
ExprKind::Call(from_func, [arg]) => is_default_equivalent_from(cx, from_func, arg),
9601040
ExprKind::Path(qpath) => is_res_lang_ctor(cx, cx.qpath_res(qpath, e.hir_id), OptionNone),
9611041
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)