Detect when a field default is not using that field's type's default values

[estebank]

Given

pub struct Foo {
    pub something: i32 = 2,
}

pub struct Bar {
    pub foo: Foo = Foo { something: 10 },
}

The value for Bar { .. }.foo.something is different to the value in Foo { .. }.something. This can cause confusion so we lint against it. We don't check that the values actually differ. At just the mere presence of the default value we suggest .. to avoid the possibility of divergence. This is just a lint to allow for APIs where that divergence is intentional.

[rustbot]

r? @chenyukang

rustbot has assigned @chenyukang.
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

[nicoburns]

Perhaps I'm missing something, but I don't think this needs to be linted against? I would not expect the default value of a struct field to match the default value of the type of that field. And I think this is going to be common enough that it's going to be extremely annoying if it is linted against (although I guess it's easy enough to turn off a lint globally). For example, wouldn't this lint against any Option<T> field with a non-None default value (because the default for Option is None?

[estebank]

Given

#![feature(default_field_values)]
enum Option<T> {
    Some(T),
    None,
}

use Option::None;

struct Foo {
    a: Option<()> = None,
}

struct Bar {
    foo: Foo = Foo { a: None },
}

you get

error: default field overrides that field's type's default
  --> bat.rs:14:25
   |
14 |     foo: Foo = Foo { a: None },
   |                ---      ^^^^ this overrides the default of field `a` in `Foo`
   |                |
   |                when constructing this value
   |
note: this field's default value in `Foo` is overriden
  --> bat.rs:10:21
   |
9  | struct Foo {
   | ----------
10 |     a: Option<()> = None,
   |                     ^^^^
   = note: `#[deny(default_field_overrides_default_field)]` on by default

What the lint is asking is that you write

struct Bar {
    foo: Foo = Foo { .. },
}

If you were to write

struct Foo {
    a: Option<()> = Some(()),
}

struct Bar {
    foo: Foo = Foo { a: None },
}

it would complain, as well as if you were to write

struct Foo {
    a: Option<()> = Some(()),
}

struct Bar {
    foo: Foo = Foo { a: Some(()) },
}

The intention of the lint is to detect that you are writing a default which is a literal for a struct that has a default field value instead of using that default. I do think that this lint is likely better as a clippy pedantic lint, but would be concerned if it wasn't warn by default, so that it could catch mistakes.

Note that enums don't have a concept of "default variant", beyond the #[derive(Default)]/#[default] attributes, which are orthogonal to "default field values", which can be used in enum variants, so this doesn't affect the case of "I'm setting this field to a default value that doesn't match Default::default() for that enum".

(The lint should have a structured suggestion, which it doesn't now.)

[Nadrieril]

I'm confused, if I wanted to use Foo's default value, I would have simply written

#[derive(Default)]
struct Foo {
    a: Option<()> = Some(()),
}

#[derive(Default)]
struct Bar {
    foo: Foo = Foo::default(),
}

If I'm going out of my way to write an explicit default value, it's presumably intentional. Could you spell out the kind of mistake you want to catch, ideally with a realer example instead of Foo/Bar structs?

EDIT: oops, forgot about the const restriction.

[Nadrieril]

Here's one case that this arguably would catch:

#[derive(Default)]
struct Pet {
    name: String,
    age: i128 = 42, // impl Default for Pet will use the literal 42 for age
    foo: Foo = Foo { something: 10 },
}

#[derive(Default)]
struct Foo {
    something: i32 = 2,
    otherthing: i32 = 4,
}

fn main() {
    let my_pet = Pet {
        name: "ferris".into(),
        foo: Foo { otherthing = 12, .. },
    };
}

I can imagine the user expecting my_pet.foo.something to be 10 and being surprised here. If that's the thing we want to catch though, maybe the place to lint is at construction site, not in the definition of Pet.

[estebank]

If that's the thing we want to catch though, maybe the place to lint is at construction site, not in the definition of Pet.

That is indeed the case I want to catch. I would like to hear your reasoning there because I feel like I'm missing something? In my eyes this is a warning to the API author for "you might be causing confusion to your users" instead of a warning to users for "you might be getting confused by the API".

(Note that on Pet::foo you're missing a , .. for it to be valid.)

[Nadrieril]

In my eyes this is a warning to the API author for "you might be causing confusion to your users"

This is what I'm not seeing. The author believes that value to be the default value that's most relevant for that struct, why would the global default be relevant? To me that feels like if you linted against field: usize = 1 because 0 is the default value for usize, or field: Option<usize> = Some(0) because None is the default value for Option.

I guess that's my main stance, I'm not sure how I can explain that more. Maybe we have wildly different usages in mind?

I do see that a user writing Pet { foo: Foo { otherthing: 12, .. }, .. } may fail to notice that this is also updating pet.foo.something. This very much feels like a user confusion to me: the intent of the crate author is clear, the intent of the user isn't. Hence a lint, with a simple always-correct resolution that involves being explicit about the ambiguous field value: Pet { foo: Foo { otherthing: 12, something: 10 }, .. }.

On top of that, with your lint the only option to keep the desired behavior is to #[allow] the lint. I'm not a fan of a lint that suggests changing behavior on the premise that the written behavior would be confusing; is there precedent for a lint like this?

[nicoburns]

Hmm... I can see that this might potentially be confusing. On the other hand, I would expect the majority of the examples of this to be valid uses cases that would then need to ignore the lint. I feel like this might be something that users just need to learn.

In general, it's already the case that:

TypeA::default().b won't always be equal to TypeB::default().

[estebank]

@nicoburns you're absolutely right that this is exactly the same thing that can be done with Default... And we don't have a lint either in rustc or clippy for that. I suspect the reason we don't is because the analysis is more complex than anyone wanted to take on (default fields are easier to wrangle than looking at multiple Default::default() bodies), combined with a lower "interest" in doing that.

To all, I still think having a lint doing this analysis would be useful, but I am convinced that it doesn't need to live in rustc itself and be more suitable for clippy, likely allow-by-default under the pedantic family, given all the feedback.