Skip to content

Commit

Permalink
Use MIR body to identify more "default equivalent" calls
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
estebank committed Jan 12, 2025
1 parent ab55d3f commit 6fa2199
Show file tree
Hide file tree
Showing 5 changed files with 227 additions and 13 deletions.
2 changes: 1 addition & 1 deletion clippy_lints/src/methods/or_fun_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ pub(super) fn check<'tcx>(
if (is_new(fun) && output_type_implements_default(fun))
|| match call_expr {
Some(call_expr) => is_default_equivalent(cx, call_expr),
None => is_default_equivalent_call(cx, fun) || closure_body_returns_empty_to_string(cx, fun),
None => is_default_equivalent_call(cx, fun, None) || closure_body_returns_empty_to_string(cx, fun),
}
{
span_lint_and_sugg(
Expand Down
98 changes: 89 additions & 9 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ use rustc_hir::{
use rustc_lexer::{TokenKind, tokenize};
use rustc_lint::{LateContext, Level, Lint, LintContext};
use rustc_middle::hir::place::PlaceBase;
use rustc_middle::mir::Const;
use rustc_middle::mir::{AggregateKind, Const, Operand, RETURN_PLACE, Rvalue, StatementKind, TerminatorKind};
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow};
use rustc_middle::ty::fast_reject::SimplifiedType;
use rustc_middle::ty::layout::IntegerExt;
Expand Down Expand Up @@ -897,22 +897,102 @@ fn is_default_equivalent_ctor(cx: &LateContext<'_>, def_id: DefId, path: &QPath<
}

/// Returns true if the expr is equal to `Default::default` when evaluated.
pub fn is_default_equivalent_call(cx: &LateContext<'_>, repl_func: &Expr<'_>) -> bool {
pub fn is_default_equivalent_call(
cx: &LateContext<'_>,
repl_func: &Expr<'_>,
whole_call_expr: Option<&Expr<'_>>,
) -> bool {
if let ExprKind::Path(ref repl_func_qpath) = repl_func.kind
&& let Some(repl_def_id) = cx.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id()
&& (is_diag_trait_item(cx, repl_def_id, sym::Default)
|| is_default_equivalent_ctor(cx, repl_def_id, repl_func_qpath))
{
true
} else {
false
return true;
}

// Get the type of the whole method call expression, find the exact method definition, look at
// its body and check if it is similar to the corresponding `Default::default()` body.
let Some(e) = whole_call_expr else { return false };
let Some(default_fn_def_id) = cx.tcx.get_diagnostic_item(sym::default_fn) else {
return false;
};
let Some(ty) = cx.tcx.typeck(e.hir_id.owner.def_id).expr_ty_adjusted_opt(e) else {
return false;
};
let args = rustc_ty::GenericArgs::for_item(cx.tcx, default_fn_def_id, |param, _| {
if let rustc_ty::GenericParamDefKind::Lifetime = param.kind {
cx.tcx.lifetimes.re_erased.into()
} else if param.index == 0 && param.name == kw::SelfUpper {
ty.into()
} else {
param.to_error(cx.tcx)
}
});
let instance = rustc_ty::Instance::try_resolve(cx.tcx, cx.typing_env(), default_fn_def_id, args);

let Ok(Some(instance)) = instance else { return false };
if let rustc_ty::InstanceKind::Item(def) = instance.def
&& !cx.tcx.is_mir_available(def)
{
// Avoid ICE while running rustdoc for not providing `optimized_mir` query.
return false;
}
let ExprKind::Path(ref repl_func_qpath) = repl_func.kind else {
return false;
};
let Some(repl_def_id) = cx.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id() else {
return false;
};

// Get the MIR Body for the `<Ty as Default>::default()` function.
// If it is a value or call (either fn or ctor), we compare its `DefId` against the one for the
// resolution of the expression we had in the path. This lets us identify, for example, that
// the body of `<Vec<T> as Default>::default()` is a `Vec::new()`, and the field was being
// initialized to `Vec::new()` as well.
let body = cx.tcx.instance_mir(instance.def);
for block_data in body.basic_blocks.iter() {
if block_data.statements.len() == 1
&& let StatementKind::Assign(assign) = &block_data.statements[0].kind
&& assign.0.local == RETURN_PLACE
&& let Rvalue::Aggregate(kind, _places) = &assign.1
&& let AggregateKind::Adt(did, variant_index, _, _, _) = &**kind
&& let def = cx.tcx.adt_def(did)
&& let variant = &def.variant(*variant_index)
&& variant.fields.is_empty()
&& let Some((_, did)) = variant.ctor
&& did == repl_def_id
{
return true;
} else if block_data.statements.is_empty()
&& let Some(term) = &block_data.terminator
{
match &term.kind {
TerminatorKind::Call {
func: Operand::Constant(c),
..
} if let rustc_ty::FnDef(did, _args) = c.ty().kind()
&& *did == repl_def_id =>
{
return true;
},
TerminatorKind::TailCall {
func: Operand::Constant(c),
..
} if let rustc_ty::FnDef(did, _args) = c.ty().kind()
&& *did == repl_def_id =>
{
return true;
},
_ => {},
}
}
}
false
}

/// Returns true if the expr is equal to `Default::default()` of it's type when evaluated.
/// Returns true if the expr is equal to `Default::default()` of its 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.
/// It doesn't cover all cases, like struct literals, but it is a close approximation.
pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
match &e.kind {
ExprKind::Lit(lit) => match lit.node {
Expand All @@ -933,7 +1013,7 @@ pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
false
}
},
ExprKind::Call(repl_func, []) => is_default_equivalent_call(cx, repl_func),
ExprKind::Call(repl_func, []) => is_default_equivalent_call(cx, repl_func, Some(e)),
ExprKind::Call(from_func, [arg]) => is_default_equivalent_from(cx, from_func, arg),
ExprKind::Path(qpath) => is_res_lang_ctor(cx, cx.qpath_res(qpath, e.hir_id), OptionNone),
ExprKind::AddrOf(rustc_hir::BorrowKind::Ref, _, expr) => matches!(expr.kind, ExprKind::Array([])),
Expand Down
34 changes: 34 additions & 0 deletions tests/ui/derivable_impls.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,40 @@ impl Default for SpecializedImpl2<String> {
}
}

#[derive(Default)]
pub struct DirectDefaultDefaultCall {
v: Vec<i32>,
}


#[derive(Default)]
pub struct EquivalentToDefaultDefaultCallVec {
v: Vec<i32>,
}


pub struct S {
x: i32,
}

impl S {
fn new() -> S {
S { x: 42 }
}
}

impl Default for S {
fn default() -> Self {
Self::new()
}
}

#[derive(Default)]
pub struct EquivalentToDefaultDefaultCallLocal {
v: S,
}


// https://github.com/rust-lang/rust-clippy/issues/7654

pub struct Color {
Expand Down
49 changes: 49 additions & 0 deletions tests/ui/derivable_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,55 @@ impl Default for SpecializedImpl2<String> {
}
}

pub struct DirectDefaultDefaultCall {
v: Vec<i32>,
}

impl Default for DirectDefaultDefaultCall {
fn default() -> Self {
// When calling `Default::default()` in all fields, we know it is the same as deriving.
Self { v: Default::default() }
}
}

pub struct EquivalentToDefaultDefaultCallVec {
v: Vec<i32>,
}

impl Default for EquivalentToDefaultDefaultCallVec {
fn default() -> Self {
// The body of `<Vec as Default>::default()` is `Vec::new()`, so they are equivalent.
Self { v: Vec::new() }
}
}

pub struct S {
x: i32,
}

impl S {
fn new() -> S {
S { x: 42 }
}
}

impl Default for S {
fn default() -> Self {
Self::new()
}
}

pub struct EquivalentToDefaultDefaultCallLocal {
v: S,
}

impl Default for EquivalentToDefaultDefaultCallLocal {
fn default() -> Self {
// The body of `<S as Default>::default()` is `S::new()`, so they are equivalent.
Self { v: S::new() }
}
}

// https://github.com/rust-lang/rust-clippy/issues/7654

pub struct Color {
Expand Down
57 changes: 54 additions & 3 deletions tests/ui/derivable_impls.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,58 @@ LL ~ struct WithoutSelfParan(bool);
|

error: this `impl` can be derived
--> tests/ui/derivable_impls.rs:216:1
--> tests/ui/derivable_impls.rs:188:1
|
LL | / impl Default for DirectDefaultDefaultCall {
LL | | fn default() -> Self {
LL | | // When calling `Default::default()` in all fields, we know it is the same as deriving.
LL | | Self { v: Default::default() }
LL | | }
LL | | }
| |_^
|
help: replace the manual implementation with a derive attribute
|
LL + #[derive(Default)]
LL ~ pub struct DirectDefaultDefaultCall {
|

error: this `impl` can be derived
--> tests/ui/derivable_impls.rs:199:1
|
LL | / impl Default for EquivalentToDefaultDefaultCallVec {
LL | | fn default() -> Self {
LL | | // The body of `<Vec as Default>::default()` is `Vec::new()`, so they are equivalent.
LL | | Self { v: Vec::new() }
LL | | }
LL | | }
| |_^
|
help: replace the manual implementation with a derive attribute
|
LL + #[derive(Default)]
LL ~ pub struct EquivalentToDefaultDefaultCallVec {
|

error: this `impl` can be derived
--> tests/ui/derivable_impls.rs:226:1
|
LL | / impl Default for EquivalentToDefaultDefaultCallLocal {
LL | | fn default() -> Self {
LL | | // The body of `<S as Default>::default()` is `S::new()`, so they are equivalent.
LL | | Self { v: S::new() }
LL | | }
LL | | }
| |_^
|
help: replace the manual implementation with a derive attribute
|
LL + #[derive(Default)]
LL ~ pub struct EquivalentToDefaultDefaultCallLocal {
|

error: this `impl` can be derived
--> tests/ui/derivable_impls.rs:265:1
|
LL | / impl Default for RepeatDefault1 {
LL | | fn default() -> Self {
Expand All @@ -114,7 +165,7 @@ LL ~ pub struct RepeatDefault1 {
|

error: this `impl` can be derived
--> tests/ui/derivable_impls.rs:250:1
--> tests/ui/derivable_impls.rs:299:1
|
LL | / impl Default for SimpleEnum {
LL | | fn default() -> Self {
Expand All @@ -132,5 +183,5 @@ LL ~ #[default]
LL ~ Bar,
|

error: aborting due to 8 previous errors
error: aborting due to 11 previous errors

0 comments on commit 6fa2199

Please sign in to comment.