Add the derivable_impls lint

[HKalbasi]

Fix #7550

changelog: Add new derivable_impls lint. mem_replace_with_default now covers non constructor cases.

[rust-highfive]

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

[camsteffen]

You can use is_default_equivalent_ctor. Otherwise I think you got it covered. Careful not to lint macros where a dynamically generated impl just happens to be Default-equivalent.

[HKalbasi]

I added is_default_equivalent_ctor and some more cases to is_default.

Careful not to lint macros where a dynamically generated impl just happens to be Default-equivalent.

I think it should be ok, do you have special test in mind that I can add it?

r? @camsteffen

[camsteffen]

The lint is technically correct because the result of mem::replace is the return value of the function. Although I don't really like the suggestion in that case. I think we should allow mem::replace for primitive types.

[HKalbasi]

Or maybe just for boolean? For integers take isn't better?

[camsteffen]

mem::take makes the most sense when you have something with heap allocation like a Vec where you are literally taking the heap-allocated data from the Vec. mem::take with integers is okay but (at least personally) I don't think it is better than mem::replace(n, 0). Allowing primitives is the simplest way to draw the line here.

[camsteffen]

Careful not to lint macros where a dynamically generated impl just happens to be Default-equivalent.

I think it should be ok, do you have special test in mind that I can add it?

Here's a couple tests - playground

[HKalbasi]

I disabled that lint for primitives. But it is still enable for stack allocated things like (0, false).

@rustbot label +S-waiting-on-review

[bors]

☔ The latest upstream changes (presumably #7595) made this pull request unmergeable. Please resolve the merge conflicts.

[flip1995]

r? @camsteffen reassigning, since you already did most of the reviewing work.

[camsteffen]

Whoops I somehow assumed this was already assigned to me.

[camsteffen]

We should watch out for cases where a manual impl is needed because a derive adds different type bounds (rust-lang/rust#26925). For example, a struct with Option<T> does not require T: Default, but a derive adds that type bound anyways. A simple approach would be to just ignore impls with any type variables. Or, unless I'm missing something, I think it would be correct to lint if all type variables have the Default constraint. In any case, we should probably mention this issue in the lint docs.

[HKalbasi]

I now disabled lint for non-lifetime generics, because in addition to that issue, specialized impls can create false positives, and detection of them is not always easy:

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(),
        }
    }
}

Not sure if there is real use case for specialized impls of default trait, but #[derive(Default)] behave differently in this case.

[camsteffen]

Wouldn't that example be the same as derive?

[HKalbasi]

No? I think derive will generate this:

impl<A: Default, B: Default> Default for SpecializedImpl<A, B> {
    fn default() -> Self {
        Self {
            a: A::default(),
            b: B::default(),
        }
    }
}

[camsteffen]

A couple more things and this is almost ready! Can you be a little more specific for the mem_replace_with_default changelog?

[camsteffen]

Sorry, found a couple more things.

[bors]

☔ The latest upstream changes (presumably #7604) made this pull request unmergeable. Please resolve the merge conflicts.

[camsteffen]

Thanks!

@bors r+

[bors]

📌 Commit 8221f9e has been approved by camsteffen

[bors]

⌛ Testing commit 8221f9e with merge 6541662...

[bors]

💔 Test failed - checks-action_test

[HKalbasi]

changelog: is there, what is wrong?

[camsteffen]

Oh it has to be on the same line like changelog: <stuff>

[camsteffen]

@bors retry

[HKalbasi]

Like this? Please edit it in the desired way if it is wrong.

[bors]

⌛ Testing commit 8221f9e with merge a8c269a...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing a8c269a to master...