Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use MIR body to identify more "default equivalent" calls for derivable_impls #13988

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jan 12, 2025

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

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

struct R {
    s: S,
}

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

the derivable_impls lint will now trigger.

changelog: [derivable_impls]: detect when a Default impl is using the same fn call that that type's Default::default calls
changelog: [mem_replace_with_default]: detect when std::mem::replace is being called with the same fn that that type's Default::default calls, without the need of a manually maintained list

@rustbot
Copy link
Collaborator

rustbot commented Jan 12, 2025

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 12, 2025
@estebank estebank changed the title Use MIR body to identify more "default equivalent" calls Use MIR body to identify more "default equivalent" calls for derivable_impls Jan 12, 2025
@estebank estebank force-pushed the equivalence-to-default-default branch from 01edde1 to 6fa2199 Compare January 12, 2025 00:13
@samueltardieu
Copy link
Contributor

samueltardieu commented Jan 12, 2025

Very interesting.

In particular, your change also affects mem_replace_with_default, see the lintcheck run. If those suggestions are valid (and it looks like they are), you should add tests to tests/ui/mem_replace.rs as well to reflect this and make sure we don't regress later.

Copy link
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay. Just one question, otherwise this looks good.

Comment on lines 934 to 961
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know there are cases where the MIR isn't available, but what does rustdoc have to do with clippy here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this might have been a comment from when I implemented this in rustc itself, which triggered an ICE when invoked by rustdoc. The check I think is still good to keep, but maybe not the comment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use impossible_predicates in other places to cover when optimized_mir fails, but is_mir_available seems like it's the better thing to use.

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.
@estebank estebank force-pushed the equivalence-to-default-default branch from e932dd3 to 39d73d5 Compare February 11, 2025 02:59
@Jarcho Jarcho added this pull request to the merge queue Feb 11, 2025
Merged via the queue into rust-lang:master with commit ff87bea Feb 11, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants