add default_mismatches_new lint

[nyurik]

Implement #14075. Prevents rust-lang/rust#135977.

What it does

If a type has an auto-derived Default trait and a fn new() -> Self, this lint checks if the new() method performs custom logic rather than calling Self::default()

Why is this bad?

Users expect the new() method to be equivalent to default(), so if the Default trait is auto-derived, the new() method should not perform custom logic. Otherwise, there is a risk of different behavior between the two instantiation methods.

Example

#[derive(Default)]
struct MyStruct(i32);
impl MyStruct {
  fn new() -> Self {
    Self(42)
  }
}

Users are unlikely to notice that MyStruct::new() and MyStruct::default() would produce different results. The new() method should use auto-derived default() instead to be consistent:

#[derive(Default)]
struct MyStruct(i32);
impl MyStruct {
  fn new() -> Self {
    Self::default()
  }
}

Alternatively, if the new() method requires non-default initialization, implement a custom Default. This also allows you to mark the new() implementation as const:

struct MyStruct(i32);
impl MyStruct {
  const fn new() -> Self {
    Self(42)
  }
}
impl Default for MyStruct {
  fn default() -> Self {
    Self::new()
  }
}

• Followed lint naming conventions
• Added passing UI tests (including committed .stderr file)
• cargo test passes locally
• Executed cargo dev update_lints
• Added lint documentation
• Run cargo dev fmt

changelog: [default_mismatches_new]: new lint to catch fn new() -> Self method not matching the Default implementation

Notable cases in the wild

• glob - default() and new() are actually different - likely was a bug that glob team wants to fix (per comments)
• backtrace - new() creates cache with Vec::with_capacity(), whereas default() uses Vec::default(). Possibly a bug.
• tracing_core uses Self(()) -- should this be ignored?
• tracing_subscriber similarly uses Self { _p: () } for struct Identity { _p: () } -- probably better to use ::default()

TODO/TBD

• Which cases of manual fn new() -> Self implementations are OK? (rather than delegating to Self::default())
  • In case Self is a unit type
• given an rustc_hir::hir::ImplItem of a fn new() -> Self, get the body of that function
• verify that the fn body simply does return Self::default(); and no other logic (in all variants like Default::default(), etc.
• I did a minor modification to clippy_lints/src/default_constructed_unit_structs.rs while trying to understand its logic - I think we may want to have a is_unit_type(ty) utility function?
• Handle generic types. This should be done in another PR due to complexity. See comment

[rustbot]

r? @llogiq

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

[samueltardieu]

There will be a false positive on this one and the suggestion won't apply:

#[derive(Default)]
struct S<T> {
    o: Option<T>,
}

impl<T> S<T> {
    fn new() -> Self {
        S { o: None }
    }
}

[nyurik]

@samueltardieu good catch! Auto-derive implements it with a where T: Default:

impl<T: Default> Default for S<T> {
    fn default() -> S<T> {
        S { o: Default::default() }
    }
}

What logic does the Default derive uses? Does it simply Default constraint to all generic parameters? The actual Option::<T>::default() exists for all T, not just those with Default.

Possible solutions

• do not warn for generics (meh)
• in case of generics, offer to implement Default manually and redirect to new() (also somewhat meh)
• analyse code to check if T used in the new() already has Default constraint. Offer Self::default() solution only if T is constraint to Default, otherwise suggest to implement Default manually
• ...?

[nyurik]

For now, I restricted this lint to non-generic types. Lets do generics separately.

[samueltardieu]

* analyse code to check if `T` used in the `new()` already has `Default` constraint

Constraints are additive, you would have also to consider the constraints on the enclosing impl as well.

[nyurik]

@samueltardieu thx, so lets skip types with generics for now - even without generics this catches all the same cases as part of CI lint check. We can track it in a separate issue/PR.

[nyurik]

@samueltardieu should @llogiq or someone else be assigned as the reviewer? (per your priv comment)

[samueltardieu]

@samueltardieu should @llogiq or someone else be assigned as the reviewer? (per your priv comment)

This should not be necessary as the issue is assigned to him already, but just in case I've requested his review.

[llogiq]

This would unify the new and default implementation, losing either one or the other. I think the docs should suggest renaming new to clarify the difference.

[nyurik]

@llogiq thx for review! Could you clarify how you think the docs should change? The rustc issue that actually caused me to write this lint was that new() would set the separator to ' ', whereas the default() set it to '\0'. So unifying them should loose one or the other, and consolidate the code, but keep the original function as new()

[nyurik]

P.S. are there any blockers to start the FCP?

[samueltardieu]

@llogiq thx for review! Could you clarify how you think the docs should change? The rustc issue that actually caused me to write this lint was that new() would set the separator to ' ', whereas the default() set it to '\0'. So unifying them should loose one or the other, and consolidate the code, but keep the original function as new()

I understand it as "Another less standard method name would be better than new() if it is different from default(), you could suggest renaming the new implementation".

[nyurik]

thx @samueltardieu , fixed

[samueltardieu]

r? @samueltardieu

I've started the FCP on Zulip.

Could you please rebase?

[samueltardieu]

You are supposed to explain the problem here, not give the solution. Right now, there seems to be no indication of what is wrong in the stderr file, just a suggestion to delegate. Nowhere does the notion of "mismatch" appear in the output. This text should probably appear as a diag.help() message.

Also, is the suggestion accurate when new() and default() don't match? Shouldn't it suggest to either be consistent and delegate, or (as discussed in the PR comments) rename the new() associated function to something more specific?

Should the case where new() and default() return the same value be considered a mismatch? Or just a redundancy, in which case the lint may be misnamed.

[samueltardieu]

From a recent discussion with @Alexendoo, it looks like we should avoid adding more Unspecified suggestions; MaybeIncorrect would probably be more appropriate.

[samueltardieu]

From what I get from the code, if you pass Some(expr), it should look inside callee and look if its content is similar to the default implementation.

In your case, since you only lint on #[derive(Default)], I'm not sure it would make a difference, but it probably wouldn't hurt.

[samueltardieu]

Suggested change

	#[clippy::version = "1.86.0"]
	#[clippy::version = "1.88.0"]

[samueltardieu]

Here is an example of an unhelpful error message: it doesn't tell what the problem is, it only gives a suggestion to "fix" it.