Reassign default private

[camsteffen]

changelog: fix field_reassign_with_default false positive

• Fix field_reassign_with_default lint suggest invalid code #6344
• Fix assumption that field: Default::default() is the same as ..Default::default()
• Cleanup some redundant logic

[rust-highfive]

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

[ghost]

Isn't field: Default::default() is the same as ..Default::default() if you're using #[derive(Default)]?

Also, for

    let mut a: A = Default::default();
    a.i = Default::default();

why not suggest

    let mut a: A = A { i: Default::default(), ..Default::default() };

[camsteffen]

Isn't field: Default::default() is the same as ..Default::default() if you're using #[derive(Default)]?

In that case yes. So we could theoretically check for #[derive(Default)]. But I think that is a distinct issue that should not be silently corrected by this lint. It could be a separate lint called redundant_default_field for example. The user may have explicitly set the field for a reason so better safe than sorry.

Also, for

    let mut a: A = Default::default();
    a.i = Default::default();

why not suggest

    let mut a: A = A { i: Default::default(), ..Default::default() };

It should do that.

[flip1995]

So I think checking for derive(Default) in this lint would be a good enhancement.

But I think that is a distinct issue that should not be silently corrected by this lint

I disagree. If we can make sure that we're suggesting semantically equivalent code, that improves readability, why not do so?

It should do that.

Does it? The two removed tests suggest otherwise. If it indeed does do that, not checking for derive(Default) in this lint, but implement a lint that checks for

let mut a: A = A { i: Default::default(), ..Default::default() };

might actually make sense.

Anyway, this can be solved by adding back those tests and make sure that it does suggest

let mut a: A = A { i: Default::default(), ..Default::default() };

[flip1995]

Is there a reason, why span_lint_and_sugg isn't used here? @ebroto I think you reviewed the initial implementation.

[flip1995]

Sorry for taking so long with the review. I was busy with work stuff and the clippy roadmap.

[flip1995]

@bors r+

Thanks!

[bors]

📌 Commit c6450c7 has been approved by flip1995

[bors]

⌛ Testing commit c6450c7 with merge 3661848...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 3661848 to master...

[chrisduerr]

@flip1995 It seems like the previous broken version is part of Rust 1.49.0. Is there a way to bump the clippy version for a release?

This now causes build failures that are impossible to fix without disabling clippy (at least locally) which is quite unfortunate.

[flip1995]

Not really. A stable backport for this is hard to do. We recommend to disable this lint until this fix is deployed. I'll create a beta backport so it will already be in 1.50.

[chrisduerr]

That's very unfortunate and seems quite limiting. Is there any plan on how to change that in the future? It seems quite unfortunate that rustup components would be tied so strictly to the rustc update schedule.

One of the greatest things about clippy is that it's one of the few linters that actually seems usable without having to tweak it 24/7, it's quite unfortunate when that isn't the case and causes trouble like that. Especially for stable releases with obvious bugs.

Now mistakes happen of course, that's no problem, but that's exactly why I feel like it makes sense to have processes in place to deal with these issues. I'd hope that this is the most severe kind of issue clippy would ever ship in a stable release, but it could always get worse. Having to change entire CI setups for stable release cycles because of a clippy bug would be most unfortunate and so would be a bump to the entire rust version just for a component update.

[camsteffen]

Clippy has more than a few bugs and this one just happened to be fixed right after the cutoff. I think the maintainers are doing all that they reasonably should do. Just my two cents.

[flip1995]

Is there any plan on how to change that in the future? It seems quite unfortunate that rustup components would be tied so strictly to the rustc update schedule.

No not the release cycle. This is the best possible way to do Clippy releases.

But like mentioned multiple times in #6462, we want to improve the situation for such bugs getting into stable undetected.

For a bug like this, you don't have to change your CI setup, but just add #![allow(clippy::field_reassign_with_default)] to your crate.

[chrisduerr]

But like mentioned multiple times in #6462, we want to improve the situation for such bugs getting into stable undetected.

That's nice, I suppose the main issue with this was just that you were a bit busy, which is natural of course. Temporarily disabling lints that are broken always is an option, but that also requires work. So being able to fix things retroactively would just allow for a bit more flexibility.

For a bug like this, you don't have to change your CI setup, but just add #![allow(clippy::field_reassign_with_default)] to your crate.

I'm aware, thanks. I just added a local annotation while grinding my teeth. I was talking about some other hypothetical issue that might slip through.

[flip1995]

I suppose the main issue with this was just that you were a bit busy, which is natural of course.

Not really, just that I didn't know that this was a big issue before the 1.49 release. About a year ago I mainly maintained Clippy alone and knew about practically every issue. That isn't possible anymore though.

I just added a local annotation while grinding my teeth

Understandable. We'll try to improve this situation, because we want Clippy to be as easy to use and reliable as possible. This shouldn't happen, but until now we haven't had the resources or a plan how we want to deal with things like this.