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

new_without_default_derive triggers on types which cannot #[derive(Default)] #1890

Closed
sgrif opened this issue Jul 13, 2017 · 12 comments
Closed
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience. T-middle Type: Probably requires verifiying types

Comments

@sgrif
Copy link

sgrif commented Jul 13, 2017

The lint seems to trigger on any function with the signature fn new() -> Self. #[derive(Default)] requires a default bound on all type parameters, even if those types do not appear in the actual type body.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 14, 2017

The idea of that lint is not to necessarily use derive, but to implement Default by forwarding to new

@oli-obk oli-obk added the S-needs-discussion Status: Needs further discussion before merging or work can be started label Jul 14, 2017
@sgrif
Copy link
Author

sgrif commented Jul 14, 2017

If that's the case, it should probably not include derive in the name or the help text

@oli-obk
Copy link
Contributor

oli-obk commented Jul 14, 2017

Oh sorry. I confused the two new_without_default* lints. That makes total sense.

@oli-obk oli-obk added E-medium Call for participation: Medium difficulty level problem and requires some initial experience. C-bug Category: Clippy is not doing the correct thing T-middle Type: Probably requires verifiying types and removed S-needs-discussion Status: Needs further discussion before merging or work can be started labels Jul 14, 2017
@oli-obk
Copy link
Contributor

oli-obk commented Jul 14, 2017

Can you give an example struct where the lint triggers but shouldn't?

I tried

struct Foo<T>(std::marker::PhantomData<T>);

struct Bar<T>(Foo<T>);

impl<T> Bar<T> {
    fn new() -> Self { unimplemented!() }
}

but clippy doesn't trigger on it.

@sgrif
Copy link
Author

sgrif commented Jul 14, 2017

use std::collections::HashMap;

trait SomeTrait {
    type SomeType;
}

#[derive(Hash, PartialEq, Eq)]
struct Key<T: SomeTrait> {
    foo: T::SomeType,
}

struct Cache<T: SomeTrait> {
    foo: HashMap<Key<T>, ()>,
}

impl<T> Cache<T>
where
    T: SomeTrait,
    T::SomeType: Hash + Eq,
{
    pub fn new() -> Self {
        Cache {
            foo: HashMap::new(),
        }
    }
}

@oli-obk
Copy link
Contributor

oli-obk commented Jul 14, 2017

Unfortunately I cannot reproduce: http://play.integer32.com/?gist=56b78c856f90a1f8e0bb9e8ee50a392a&version=stable or http://play.integer32.com/?gist=8179a9caaadbd71712a54d804fea13e5&version=stable (your code as it is doesn't compile, so I had to guess)

@sgrif
Copy link
Author

sgrif commented Jul 14, 2017

Sorry about that, this is the code which compiles:

use std::collections::HashMap;
use std::hash::Hash;

trait SomeTrait {
    type SomeType;
}

#[derive(Hash, PartialEq, Eq)]
struct Key<T: SomeTrait> {
    foo: T::SomeType,
}

struct Cache<T: SomeTrait> {
    foo: HashMap<Key<T>, ()>,
}

impl<T> Cache<T>
where
    T: SomeTrait,
    Key<T>: Hash + Eq,
{
    pub fn new() -> Self {
        Cache {
            foo: HashMap::new(),
        }
    }
}

fn main() {
}

@oli-obk
Copy link
Contributor

oli-obk commented Jul 14, 2017

But it doesn't trigger the lint. At least not on play.integer32.com

@sgrif
Copy link
Author

sgrif commented Jul 14, 2017

The actual type which 100% triggers the lint and cannot derive Default which I was attempting to simplify is here

@oli-obk
Copy link
Contributor

oli-obk commented Jul 17, 2017

I consider this a bug in derives, not in clippy: rust-lang/rust#9607

minimal repro:

use std::hash::Hash;
use std::collections::HashMap;

pub struct Foo<T> {
    bar: HashMap<T, usize>,
}

impl<T: Hash + Eq> Foo<T> {
    pub fn new() -> Self {
        Foo {
            bar: HashMap::default(),
        }
    }
}

@radix
Copy link

radix commented Sep 1, 2017

I'm not sure if this should be a separate issue or not, but I feel like this code should not trigger this lint:

#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize)]
pub struct MapID(Uuid);
impl MapID {
  pub fn new() -> MapID {
    MapID(Uuid::new_v4())
  }
  pub fn to_string(&self) -> String {
    self.0.hyphenated().to_string()
  }
}

IMO it should only trigger it when the contents of the new method would be equivalent to using a derived Default.

@rail-rain
Copy link
Contributor

For anyone who happened to be here: new_without_dafault_derive was merged into new_without_default in #3558, and then it stopped suggesting deriving Default in #5616. Therefore, unless someone re-implemented the feature with the enhancement suggested by flip1995, there's nothing to be done for this issue.

By the way, the repro by oli-obk can derive Default now due to rust-lang/rust#67642; which is unrelated to the problem.

@oli-obk oli-obk closed this as completed Dec 11, 2020
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 E-medium Call for participation: Medium difficulty level problem and requires some initial experience. T-middle Type: Probably requires verifiying types
Projects
None yet
Development

No branches or pull requests

4 participants