Skip to content

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

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

Open
mj10021 opened this issue Jul 31, 2024 · 6 comments
Open

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

mj10021 opened this issue Jul 31, 2024 · 6 comments
Assignees
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-discussion Category: Discussion or questions that doesn't represent real issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mj10021
Copy link
Contributor

mj10021 commented Jul 31, 2024

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.

@mj10021 mj10021 added the C-bug Category: This is a bug. label Jul 31, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 31, 2024
@compiler-errors
Copy link
Member

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?

@compiler-errors compiler-errors added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed C-bug Category: This is a bug. labels Jul 31, 2024
@mj10021
Copy link
Contributor Author

mj10021 commented Jul 31, 2024

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

@saethlin saethlin added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jul 31, 2024
@LunarLambda
Copy link

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
Copy link
Contributor Author

mj10021 commented Aug 4, 2024

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
Copy link

LunarLambda commented Aug 4, 2024

..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
Copy link
Contributor Author

mj10021 commented Aug 4, 2024

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-discussion Category: Discussion or questions that doesn't represent real issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants