Rust-clippy: clippy::new_without_default_derive triggers and shouldn't

[vitorenesduarte]

$ cargo -V
cargo 1.33.0-nightly (907c0febe 2019-01-20)
$ rustc -V
rustc 1.33.0-nightly (01f8e25b1 2019-01-24)
$ cargo clippy -V
clippy 0.0.212 (c5325063 2019-01-25)

Hi. After going through most of chapter 17.3, we end up with something like this:

fn main() {
    let mut post = Post::new();

    post.add_text("Hello");
    assert_eq!("", post.content());

    post.request_review();
    assert_eq!("", post.content());

    post.approve();
    assert_eq!("Hello", post.content());
}

pub struct Post {
    state: Option<Box<dyn State>>,
    content: String,
}

impl Post {
    pub fn new() -> Self {
        Post {
            state: Some(Box::new(Draft {})),
            content: String::new(),
        }
    }

    fn content(&self) -> &str {
        self.state.as_ref().unwrap().content(&self)
    }

    fn add_text(&mut self, text: &str) {
        self.content.push_str(text)
    }

    fn request_review(&mut self) {
        if let Some(s) = self.state.take() {
            self.state = Some(s.request_review())
        }
    }

    fn approve(&mut self) {
        if let Some(s) = self.state.take() {
            self.state = Some(s.approve())
        }
    }
}

trait State {
    fn request_review(self: Box<Self>) -> Box<dyn State>;

    fn approve(self: Box<Self>) -> Box<dyn State>;

    fn content<'a>(&self, _post: &'a Post) -> &'a str {
        ""
    }
}

struct Draft {}

impl State for Draft {
    fn request_review(self: Box<Self>) -> Box<dyn State> {
        Box::new(PendingReview {})
    }

    fn approve(self: Box<Self>) -> Box<dyn State> {
        self
    }
}

struct PendingReview {}

impl State for PendingReview {
    fn request_review(self: Box<Self>) -> Box<dyn State> {
        self
    }

    fn approve(self: Box<Self>) -> Box<dyn State> {
        Box::new(Published {})
    }
}

struct Published {}

impl State for Published {
    fn request_review(self: Box<Self>) -> Box<dyn State> {
        self
    }

    fn approve(self: Box<Self>) -> Box<dyn State> {
        self
    }

    fn content<'a>(&self, post: &'a Post) -> &'a str {
        &post.content
    }
}

and if we run cargo clippy, we get a warning that I believe we shouldn't get:

warning: you should consider deriving a `Default` implementation for `Post`
  --> src/main.rs:20:5
   |
20 | /     pub fn new() -> Self {
21 | |         Post {
22 | |             state: Some(Box::new(Draft {})),
23 | |             content: String::new(),
24 | |         }
25 | |     }
   | |_____^
   |
   = note: #[warn(clippy::new_without_default)] on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
help: try this
   |
14 | #[derive(Default)]
   |

Adding #[derive(Default)] to struct Pub without further changes is enough for the warning to disappear (which looks like a bug by itself); and if we replace the body of fn new with Default::default(), as suggested here, we get a different behavior.

(BTW, this warning only triggers if the declarations of both struct Pub and fn new are prefixed with pub, as in the example above).

This might be related with #1579.

[vitorenesduarte]

A simpler example would be:

fn main() {
    let post = Post::new();
    assert_eq!(0, post.content());
}

pub struct Post {
    state: Option<i32>,
}

impl Post {
    pub fn new() -> Self {
        Post {
            state: Some(0),
        }
    }

    fn content(&self) -> i32 {
        self.state.unwrap()
    }
}

The assert fails since the derived default implementation initializes Post.state with None.

[flip1995]

A few notes if someone wants to pick this up:

• new_without_default_derive got merged into new_without_default in #3558
• Linting this is not wrong, the suggestion is. It should suggest to explicitly implement Default instead of deriving it
• In a more general way: This lint should check if the new function assigns something different than the Default value to the struct fields. If that's the case an explicit implementation of Default is required

[rail-rain]

Note that the entire function to suggest deriving is removed in #5616 now. I'm not sure whether the flip1995's suggestion should be preserved as a future enhancement for the lint.