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

False positive derivable_impls for type parameters with defaults #10396

Closed
willburden opened this issue Feb 24, 2023 · 0 comments · Fixed by #10399
Closed

False positive derivable_impls for type parameters with defaults #10396

willburden opened this issue Feb 24, 2023 · 0 comments · Fixed by #10399
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@willburden
Copy link

willburden commented Feb 24, 2023

Summary

In certain cases involving type parameters with defaults, it is possible for clippy to suggest re-writing an impl block using a derive, when in fact a derive would be more generic and may even cause a compiler error.

Lint Name

derivable_impls

Reproducer

I tried this code:

#![allow(dead_code)]

#[derive(Default)]
struct DefaultType;

struct GenericType<T = DefaultType> {
    t: T,
}

impl Default for GenericType {
    fn default() -> Self {
        Self { t: Default::default() }
    }
}

This triggers the lint saying the impl can instead be derived:

$ cargo clippy
warning: this `impl` can be derived
  --> src/lib.rs:10:1
   |
10 | / impl Default for GenericType {
11 | |     fn default() -> Self {
12 | |         Self { t: Default::default() }
13 | |     }
14 | | }
   | |_^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derivable_impls
   = note: `#[warn(clippy::derivable_impls)]` on by default
   = help: remove the manual implementation...
help: ...and instead derive it
   |
6  | #[derive(Default)]
   |

This is a false positive as a derive here would be semantically different. The impl block as written implements Default specifically for GenericType, which is equivalent to GenericType<DefaultType>. However a derive would implement Default for any GenericType<T: Default>.

A similar case is the following, with an additional layer of indirection:

#![allow(dead_code)]

#[derive(Default)]
struct DefaultType;

struct InnerGenericType<T> {
    t: T,
}

impl Default for InnerGenericType<DefaultType> {
    fn default() -> Self {
        Self { t: Default::default() }
    }
}

struct GenericType<T = DefaultType> {
    inner: InnerGenericType<T>,
}

impl Default for GenericType {
    fn default() -> Self {
        Self { inner: Default::default() }
    }
}

This triggers the lint:

$ cargo clippy
warning: this `impl` can be derived
  --> src/lib.rs:20:1
   |
20 | / impl Default for GenericType {
21 | |     fn default() -> Self {
22 | |         Self { inner: Default::default() }
23 | |     }
24 | | }
   | |_^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derivable_impls
   = note: `#[warn(clippy::derivable_impls)]` on by default
   = help: remove the manual implementation...
help: ...and instead derive it
   |
16 | #[derive(Default)]
   |

But following the advice would create a compiler error:

...

#[derive(Default)]
struct GenericType<T = DefaultType> {
    inner: InnerGenericType<T>,
}
$ cargo clippy
error[E0277]: the trait bound `InnerGenericType<T>: std::default::Default` is not satisfied
  --> src/lib.rs:18:5
   |
16 | #[derive(Default)]
   |          ------- in this derive macro expansion
17 | struct GenericType<T = DefaultType> {
18 |     inner: InnerGenericType<T>,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::default::Default` is not implemented for `InnerGenericType<T>`
   |
   = note: this error originates in the derive macro `Default` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider annotating `InnerGenericType<T>` with `#[derive(Default)]`
   |
6  | #[derive(Default)]
   |
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
   |
17 | struct GenericType<T = DefaultType> where InnerGenericType<T>: std::default::Default {
   |                                     ++++++++++++++++++++++++++++++++++++++++++++++++

For more information about this error, try `rustc --explain E0277`.

The difference here is that the impl block implements Default for GenericType<DefaultType>, whereas the derive implements Default for GenericType<T: Default>. The derive therefore has the trait bound that InnerGenericType<T> implements Default for all T: Default, which isn't satisfied: only InnerGenericType<DefaultType> implements Default.

(And breathe.)

This is similar to #7655 which was fixed – this one still occurs likely because of the special case with the type parameter having a default (<T = DefaultType>). The lint isn't triggered if the type parameter is explicitly specified, i.e.:

impl Default for GenericType<DefaultType>

instead of

impl Default for GenericType

Version

rustc 1.67.1 (d5a82bbd2 2023-02-07)
binary: rustc
commit-hash: d5a82bbd26e1ad8b7401f6a718a9c57c96905483
commit-date: 2023-02-07
host: aarch64-apple-darwin
release: 1.67.1
LLVM version: 15.0.6

Additional Labels

No response

@willburden willburden added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Feb 24, 2023
@Alexendoo Alexendoo added the I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied label Feb 24, 2023
@bors bors closed this as completed in 2742ac0 Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants