Skip to content

New lint to ensure Default::default calls T::new, rather than vice versa. #12662

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
solarretrace opened this issue Apr 10, 2024 · 7 comments
Open
Labels
A-lint Area: New lints

Comments

@solarretrace
Copy link

What it does

Detects cases where a parameterless new simply calls Default::default, and indicates that default should call new, rather than the opposite.

Advantage

new functions can potentially be const, while default cannot, and calling them in the opposite order can prohibit this.

Drawbacks

The lint is completely irrelevant if the new function is never intended to be const.

Example

pub struct Config;

impl Default for Config {
    fn default() -> Self { Self }
}

impl Config {
    pub fn new() -> Self {
        Self::default()
    }
}

Could be written as:

pub struct Config;

impl Default for Config {
    fn default() -> Self { Self::new() }
}

impl Config {
    pub fn new() -> Self {
        Self
    }
}
@solarretrace solarretrace added the A-lint Area: New lints label Apr 10, 2024
@Jarcho
Copy link
Contributor

Jarcho commented Apr 12, 2024

This is going to end up with way too many false-positives and be more work to write and maintain if Default is derived. This would have to be limited to Default impls that could be const if they weren't a trait function. Probably only for exported types as well.

@nyurik
Copy link
Contributor

nyurik commented Jan 24, 2025

I would actually suggest the opposite IF default was derived. Case to the point is a bug I just caught in the core lib - rust-lang/rust#135977 -- where fn new would instantiate struct with non-zero values, while default() was auto-derived.

@nyurik
Copy link
Contributor

nyurik commented Jan 25, 2025

I created a new lint suggestion that detects non-matching new and default - #14075

@nyurik
Copy link
Contributor

nyurik commented Jan 31, 2025

After some more thinking, I think this would actually be a bad pattern, at least for the cases when the Default impl is auto derived. If Default is not autoderived, this same pattern should still be followed for consistency.

// User code should follow this pattern when possible:

#[derive(Default)]
pub struct Config;

impl Config {
    pub fn new() -> Self {
        Self::default()
    }
}

@nyurik
Copy link
Contributor

nyurik commented Jan 31, 2025

@Skgland I see you disagreed - could you clarify why? Do you think new() -> Self should have a different implementation than the auto-generated Default trait?

@Skgland
Copy link
Contributor

Skgland commented Jan 31, 2025

If Default is not autoderived, this same pattern should still be followed for consistency.

Was the part I disagree with.

In case default is derived I would agree that it's reasonable to have new call default, but in the case where default is manually implemented having default call new so the later can be made const is in my opinion preferable (at least until const Traits are available).

@nyurik
Copy link
Contributor

nyurik commented Jan 31, 2025

ah, yes, that does make sense, thx. Hopefully rust-lang/rust#67792 will get stabilized at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

4 participants