rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.77k stars 12.5k forks source link

Recursive ..Default::default() overflows #128421

Open mj10021 opened 1 month ago

mj10021 commented 1 month ago

I tried this code:

#[derive(Debug)]
struct Foo {
    bar: u32
}

impl Default for Foo {
    fn default()-> Self {
        Self {
            ..Default::default()
        }
    }
}
fn main() {
    println!("{:?}",Foo::default());
}

I expected to see this happen: Default values for each field

Instead, this happened: Infinite recursion and stack overflow

Discussion

I know that what I was trying to do is not a supported syntax, but the overflow can be hard to debug and the warning is not super clear.

compiler-errors commented 1 month ago

We already emit a warning explaining that it will recurse:

warning: function cannot return without recursing
 --> src/main.rs:7:5
  |
7 |     fn default()-> Self {
  |     ^^^^^^^^^^^^^^^^^^^ cannot return without recursing
8 |         Self {
9 |             ..Default::default()
  |               ------------------ recursive call site
  |
  = help: a `loop` may express intention better if this is on purpose
  = note: `#[warn(unconditional_recursion)]` on by default

What do you expect to see changed?

mj10021 commented 1 month ago

Would it make any sense to have deny rather than a warning by default because it is guaranteed to fail if called?

LunarLambda commented 1 month ago

I think that'd be a good idea.

But also, maybe we could improve the lint in the 'recursive default impl' case specifically? Lots of people assume it will call Default for each field, instead of recursively calling Default for struct itself.

mj10021 commented 1 month ago

Glad it's not just me haha... would it make sense if

struct Foo {
    bar: u32,
}
impl Default for Foo {
    fn default() -> Self {
        Self {..Default::default()}
    }
}

actually tried to use a default for each field, like u32::default() in this example? I think that is what the syntax kind of suggests should happen

LunarLambda commented 1 month ago

..value is functional record update syntax, and by definition takes a single value of the type of the struct you're using it on.

Nothing about it suggests it should use Default for each field. It's identical to writing ..Self::default(). And we cannot make it behave differently for default specifically, because you can use it with any value of matching type.

e.g. you can do

let a = Self { bar: 123 };

Self { ..a }
mj10021 commented 1 month ago

@rustbot claim