Implement default_mismatches_new lint

[nyurik]

Re-opening #14234 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

r? @samueltardieu

See also: non_canonical_partial_ord_impl lint

[rustbot]

r? @Jarcho

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 7 candidates
• 7 candidates expanded to 7 candidates
• Random selection from Jarcho, dswij, llogiq, samueltardieu

[github-actions]

Lintcheck changes for cd78473

Lint	Added	Removed	Changed
clippy::default_mismatches_new	12	0	0

This comment will be updated if you push new changes

[ada4a]

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

Some of the examples from Lintcheck seem to match this pattern where new() apparently repeats default() just to be able to be const. Maybe in this case the suggestion should need to be changed to, or extended to include, "in order to keep new() as a const fn, implement a custom Default::default() that calls Self::new()"

[samueltardieu]

Some of the examples from Lintcheck seem to match this pattern where new() apparently repeats default() just to be able to be const. Maybe in this case the suggestion should need to be changed to, or extended to include, "in order to keep new() as a const fn, implement a custom Default::default() that calls Self::new()"

Yes, I think we should not lint in this case. There will be a time where it will be possible to have T::default() be const (this is already possible using the const_default and const_trait_impl features), but until then we can't tell people not to auto-derive Default.

[samueltardieu]

First pass at the review.

The part about default_constructed_unit_structs seems independent. If this is the case, can you isolate it and make a new PR explaining what it fixes? (you can assign it to me)

Also, it would be beneficial to cleanup the content of new_without_default.rs in a first commit. It has been written a long time ago, and, for example, we now import struct names from rustc_hir directly instead of using hir::StructName (for example for hir::ExprKind::Ret, we now use ExprKind::Ret).

Keep in mind that those lints, including the new proposed one, are warned by default, so we have to be extra careful in what we put in there.

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.