Implement `default_could_be_derived` and `default_overrides_default_fields` lints #134441

estebank · 2024-12-17T21:49:54Z

Detect when a manual Default implementation isn't using the existing default field values and suggest using .. instead:

error: `Default` impl doesn't use the declared default field values
  --> $DIR/manual-default-impl-could-be-derived.rs:13:1
   |
LL | / impl Default for A {
LL | |     fn default() -> Self {
LL | |         A {
LL | |             x: S,
LL | |             y: 0,
   | |                - this field has a default value
...  |
LL | | }
   | |_^
   |
note: the lint level is defined here
  --> $DIR/manual-default-impl-could-be-derived.rs:4:35
   |
LL | #![deny(default_could_be_derived, default_overrides_default_fields)]
   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: use the default values in the `impl` to avoid them diverging over time
   |
LL -             x: S,
LL -             y: 0,
LL +             x: S, ..
   |

Detect when a manual Default implementation for a type containing at least one default field value has all fields that could be derived and suggest #[derive(Default)]:

error: `Default` impl that could be derived
  --> $DIR/manual-default-impl-could-be-derived.rs:27:1
   |
LL |   struct B {
   |   -------- all the fields in this struct have default values
LL |       x: S = S,
   |              - default value
LL |       y: i32 = 1,
   |                - default value
...
LL | / impl Default for B {
LL | |     fn default() -> Self {
LL | |         B {
LL | |             x: S,
...  |
LL | | }
   | |_^
   |
note: the lint level is defined here
  --> $DIR/manual-default-impl-could-be-derived.rs:4:9
   |
LL | #![deny(default_could_be_derived, default_overrides_default_fields)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^
help: to avoid divergence in behavior between `Struct { .. }` and `<Struct as Default>::default()`, derive the `Default`
   |
LL ~ #[derive(Default)] struct B {
   |

Store a mapping between the DefId for an impl Default for Ty {} and the DefId of either a Unit variant/struct or an fn with no arguments that is called within <Ty as Default>::default().

When linting impls, if it is for Default, we evaluate the contents of their fn default(). If it is only an ADT literal for Self and every field is either a "known to be defaulted" value (0 or false), an explicit Default::default() call or a call or path to the same "equivalent" DefId from that field's type's Default::default() implementation.

rustbot · 2024-12-17T21:50:02Z

r? @Noratrieb

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

estebank · 2024-12-17T21:55:16Z

CC @rust-lang/lang: introduction of lint on manual Default impls for types involving default_field_values.

I have a draft text where I make the case that we should also expand this behavior to all Default impls

Manual implementation of traits runs the risk of diverging on behavior over
time from what the implementor expects. If a field is meant to use that
field's default type always, but a literal is written and the type's default
is changed afterwards, the user's manual implementation will be incorrect.
If they manually write Default::default() instead, they protect themselves
against that issue, but because it is often more verbose than writing the
expected value, like vec![] or 0, people are pushed towards not using
Default::default(), let alone <Type as Default>::default().

Furthermore, with the introduction of default_field_values, there are more
avenues for confusion or divergence, like when a default field value doesn't
match with the value that the Default implementation uses. As part of that
feature, new lints to detect those cases are being introduced. The
difference between those and the ones being propsed here are small enough
that it makes sense to generalize and include the non-default_field_values
case as well, both conceptually (the cases are similar in their problems and
solutions) as well as implementation-wise (the checks can be shared).

Clippy already includes a lint clippy::default-impl which looks for a
subset of these cases, but with compiler support we can perform more
advanced analysis, like evaluating "equivalences": looking at the Default
implementation of a type to see if they are an enum variant with no values
(X::Unit, X::Struct {} or X::Tuple()), a struct with no values
(Unit, Struct {} or Tuple()) a const or an fn const call with no
arguments; if so, register this (even in the cross-crate metadata) to
compare these against the literal that a user wrote. This would allow not
only to look for literals, but for the "equivalent" code, where the user is
writing the same code that the Default implementation has.

compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs

compiler/rustc_middle/src/query/mod.rs

compiler-errors

This definitely needs to be T-lang nominated before it lands since it's warn by default. I also am frankly somewhat confused about the motivation for adding this directly as a rustc lint; this seems like something squarely in the territory of clippy.

As for the impl, it seems a bit complicated (like adding a new query doesn't seem totally obvious to me) but I tried to ask questions just to get my bearings first.

compiler/rustc_lint/src/default_could_be_derived.rs

compiler-errors · 2024-12-17T22:36:06Z

compiler/rustc_lint/src/default_could_be_derived.rs

+        // We have a manual `impl Default for Ty {}` item, where `Ty` has no type parameters or is
+        // an enum.
+
+        for assoc in data.items {


everything nested here should be pulled into a function. it's indentation runaway like crazy lol.

compiler/rustc_lint/src/default_could_be_derived.rs

compiler/rustc_middle/src/ty/context.rs

estebank · 2024-12-17T22:59:24Z

The reason for the query is not necessarily self-evident. I'll try to explain the "equivalent" difficult to follow situation.

Given an impl:

impl<T> Default for Vec<T> {
    fn default() -> Vec<T> {
        Vec::new()
    }
}

we keep track of the DefId of Vec::new. We do the same for the DefId of Option::None in:

impl<T> Default for Option<T> {
    fn default() -> Option<T> {
        Option::None
    }
}

Once we have that, we can check in the lint a situation like the following:

struct Struct {
    a: Option<i32>,
    b: Vec<i32>,
}

impl Default for Struct {
    fn default() -> Struct {
        a: None,
        b: Vec::new(),
    }
}

Because we know that None and Vec::new() are effectively the same as calling Default::default(), we can (correctly) identify that those fields could be derived.

The above should have instead have been written as

#[derive(Default)]
struct Struct {
    a: Option<i32>,
    b: Vec<i32>,
}

A query is needed to record this because we cannot inspect foreign crate HIR to find those "equivalences". The clippy lint that performs a check for all types today doesn't do that extra analysis (which would detect more uselessly manual Default impls). Even if we don't land the query for ourselves, we would want to add that logic for clippy.

This definitely needs to be T-lang nominated before it lands since it's warn by default. I also am frankly somewhat confused about the motivation for adding this directly as a rustc lint; this seems like something squarely in the territory of clippy.

These are all gated behind default_field_values and only trigger if the type being defaulted has at least one default field value. It was one of the conditions t-lang mentioned for the feature to be accepted in the first place. There were concerns about people writing code with diverging behavior between ..Default::default() and ...

compiler-errors · 2024-12-17T23:01:57Z

These are all gated behind default_field_values and only trigger if the type being defaulted has at least one default field value.

You should actually gate the lints correctly in their lint definitions then. I think you can look at other feature gated lints (I think we still have some?). It was not obvious (or not true? idk) that this logic is gated behind the feature gate from first impression.

compiler/rustc_lint/src/default_could_be_derived.rs

rustbot · 2024-12-19T23:06:01Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

estebank · 2024-12-20T01:21:33Z

compiler/rustc_lint/src/default_could_be_derived.rs

+        let Some(Data { type_def_id, parent }) = self.data else {
+            return;
+        };
+        // FIXME: evaluate bodies with statements and evaluate bindings to see if they would be
+        // derivable.
+        let hir::ExprKind::Block(hir::Block { stmts: _, expr: Some(expr), .. }, None) =
+            body.value.kind
+        else {
+            return;
+        };


I can remove all of these checks to a single method, but then I would end up returning a big tuple of unrelated fields. I leave it to you whether it is preferable to have shorter methods with descriptive names for these steps, or more linear logic with comments.

estebank · 2024-12-20T01:23:12Z

compiler/rustc_lint/src/default_could_be_derived.rs

+                    "`Default` impl that could be derived"
+                });
+
+                let mut removals = vec![];


The whole "field removal" span wrangling is not really great :-/

rustbot · 2024-12-20T17:01:34Z

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

estebank · 2024-12-20T17:01:41Z

compiler/rustc_ast/src/ast.rs

+    /// No trailing `..` or expression. The `Span` points at the trailing `,` or spot before `}`.
+    None(Span),


We can get away with not adding this span if we take the span between the end of the last field and the end of the struct expression (}), but that makes things unnecessarily complicated.

tests/ui/structs/manual-default-impl-could-be-derived.fixed

estebank · 2024-12-20T17:06:16Z

compiler/rustc_lint/src/default_could_be_derived.rs

+    for block_data in body.basic_blocks.iter() {
+        // FIXME: make another pass on this logic. For example we should be confirming that the
+        // place we assign to is `_0`.
+        if block_data.statements.len() == 1
+            && let mir::StatementKind::Assign(assign) = &block_data.statements[0].kind
+            && let mir::Rvalue::Aggregate(kind, _places) = &assign.1
+            && let mir::AggregateKind::Adt(did, variant_index, _, _, _) = &**kind
+            && let def = cx.tcx.adt_def(did)
+            && let variant = &def.variant(*variant_index)
+            && variant.fields.is_empty()
+            && let Some((_, did)) = variant.ctor
+            && did == def_id
+        {
+            return true;
+        } else if block_data.statements.len() == 0
+            && let Some(term) = &block_data.terminator
+        {
+            match &term.kind {
+                mir::TerminatorKind::Call { func: mir::Operand::Constant(c), .. }
+                    if let ty::FnDef(did, _args) = c.ty().kind()
+                        && *did == def_id =>
+                {
+                    return true;
+                }
+                mir::TerminatorKind::TailCall { func: mir::Operand::Constant(c), .. }
+                    if let ty::FnDef(did, _args) = c.ty().kind()
+                        && *did == def_id =>
+                {
+                    return true;


This is the iffiest part of this PR, IMO.

estebank · 2024-12-20T17:07:58Z

The commits are not logical units, only temporal work units to be squashed before merge, so it might be easier to review the full diff, but left unsquashed just in case it is useful to you.

Edit: went ahead and squashed... I can reflog and publish the previous state if it would aid in the review.

…ields` lints Detect when a manual `Default` implementation isn't using the existing default field values and suggest using `..` instead: ``` error: `Default` impl doesn't use the declared default field values --> $DIR/manual-default-impl-could-be-derived.rs:13:1 | LL | / impl Default for A { LL | | fn default() -> Self { LL | | A { LL | | x: S, LL | | y: 0, | | - this field has a default value ... | LL | | } | |_^ | note: the lint level is defined here --> $DIR/manual-default-impl-could-be-derived.rs:4:35 | LL | #![deny(default_could_be_derived, default_overrides_default_fields)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the default values in the `impl` to avoid them diverging over time | LL - x: S, LL - y: 0, LL + x: S, .. | ``` Detect when a manual `Default` implementation for a type containing at least one default field value has *all* fields that could be derived and suggest `#[derive(Default)]`: ``` error: `Default` impl that could be derived --> $DIR/manual-default-impl-could-be-derived.rs:27:1 | LL | struct B { | -------- all the fields in this struct have default values LL | x: S = S, | - default value LL | y: i32 = 1, | - default value ... LL | / impl Default for B { LL | | fn default() -> Self { LL | | B { LL | | x: S, ... | LL | | } | |_^ | note: the lint level is defined here --> $DIR/manual-default-impl-could-be-derived.rs:4:9 | LL | #![deny(default_could_be_derived, default_overrides_default_fields)] | ^^^^^^^^^^^^^^^^^^^^^^^^ help: to avoid divergence in behavior between `Struct { .. }` and `<Struct as Default>::default()`, derive the `Default` | LL ~ #[derive(Default)] struct B { | ``` Store a mapping between the `DefId` for an `impl Default for Ty {}` and the `DefId` of either a Unit variant/struct or an fn with no arguments that is called within `<Ty as Default>::default()`. When linting `impl`s, if it is for `Default`, we evaluate the contents of their `fn default()`. If it is *only* an ADT literal for `Self` and every field is either a "known to be defaulted" value (`0` or `false`), an explicit `Default::default()` call or a call or path to the same "equivalent" `DefId` from that field's type's `Default::default()` implementation.

compiler-errors · 2024-12-23T03:06:24Z

Since this logic is still pretty involved, plz give me a bit of time to understand it. Ping me if I've gone to long without a review.

I'd appreciate if you give this a very thorough self review in the mean time, and make sure this logic is documented sufficiently.

If you want to split this up to land the trivial parts first, you could split out just the part where you add the span to the non FRU variant to struct constructors in the AST. Up to you.

llogiq · 2024-12-23T13:21:52Z

Perhaps I've missed it, but I don't see an MSRV check in there, which would have this lint apply only to code that declares a current or newer minimum supported Rust version, thereby avoiding a lot of false positives.

compiler-errors · 2024-12-23T13:30:27Z

@llogiq: MSRV checks are a clippy-only thing afaict.

compiler-errors · 2024-12-23T13:31:10Z

We can probavly sort that out by the time stabilization of this feature rolls around, tbough, but I'd rather this PR not get more complicated.

llogiq · 2024-12-23T16:01:00Z

With the implementation as it is, library authors that want to preserve backwards compatibility and implement Default for their types where possible to do the right thing get rewarded by a barrage of useless warnings.

If we cannot avoid that, I suggest making the lint allow-by-default for the time being.

estebank · 2024-12-24T23:49:42Z

@compiler-errors opened #134737 that keeps only the most fundamental lint ("is there a Default impl that overrides a default field value?") and removes all of the suggestion logic that is hairy on its own and the checks for "is this expression suitable for a const block" and "is this expression equivalent to the field's type's Default::default() body?", making it quite shorter.

@llogiq note that these lints, as written, only affect structs that have default field values and a manual Default impl. I fail to see how there is a back-compat issue here.

rustbot assigned Noratrieb Dec 17, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 17, 2024

estebank added T-lang Relevant to the language team, which will review and decide on the PR/issue. F-default_field_values `#![feature(default_field_values)]` labels Dec 17, 2024

estebank mentioned this pull request Dec 17, 2024

Tracking issue for RFC 3681: Default field values #132162

Open

12 tasks

This comment has been minimized.

Sign in to view

compiler-errors reviewed Dec 17, 2024

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/query/mod.rs Outdated Show resolved Hide resolved

compiler-errors requested changes Dec 17, 2024

View reviewed changes

estebank commented Dec 17, 2024

View reviewed changes

compiler/rustc_lint/src/default_could_be_derived.rs Outdated Show resolved Hide resolved

estebank commented Dec 17, 2024

View reviewed changes

compiler/rustc_lint/src/default_could_be_derived.rs Outdated Show resolved Hide resolved

compiler-errors assigned compiler-errors and unassigned Noratrieb Dec 18, 2024

This comment was marked as resolved.

Sign in to view

estebank force-pushed the derive-lint-default-fields branch from 1b107d7 to 143570b Compare December 19, 2024 23:09

This comment has been minimized.

Sign in to view

estebank commented Dec 20, 2024

View reviewed changes

estebank force-pushed the derive-lint-default-fields branch from 06d5f30 to aab7aef Compare December 20, 2024 01:24

This comment has been minimized.

Sign in to view

estebank commented Dec 20, 2024

View reviewed changes

tests/ui/structs/manual-default-impl-could-be-derived.fixed Outdated Show resolved Hide resolved

estebank commented Dec 20, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

estebank force-pushed the derive-lint-default-fields branch from 4563d15 to be8b02f Compare December 23, 2024 02:55

estebank mentioned this pull request Dec 24, 2024

Implement default_overrides_default_fields lint #134737

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `default_could_be_derived` and `default_overrides_default_fields` lints #134441

Implement `default_could_be_derived` and `default_overrides_default_fields` lints #134441

estebank commented Dec 17, 2024

rustbot commented Dec 17, 2024

estebank commented Dec 17, 2024

This comment has been minimized.

compiler-errors left a comment

compiler-errors Dec 17, 2024

estebank commented Dec 17, 2024 •

edited

Loading

compiler-errors commented Dec 17, 2024

This comment was marked as resolved.

rustbot commented Dec 19, 2024

This comment has been minimized.

estebank Dec 20, 2024

estebank Dec 20, 2024

This comment has been minimized.

rustbot commented Dec 20, 2024

estebank Dec 20, 2024

estebank Dec 20, 2024

estebank commented Dec 20, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

compiler-errors commented Dec 23, 2024

llogiq commented Dec 23, 2024

compiler-errors commented Dec 23, 2024

compiler-errors commented Dec 23, 2024

llogiq commented Dec 23, 2024

estebank commented Dec 24, 2024

		/// No trailing `..` or expression. The `Span` points at the trailing `,` or spot before `}`.
		None(Span),

Implement default_could_be_derived and default_overrides_default_fields lints #134441

Are you sure you want to change the base?

Implement default_could_be_derived and default_overrides_default_fields lints #134441

Conversation

estebank commented Dec 17, 2024

rustbot commented Dec 17, 2024

estebank commented Dec 17, 2024

This comment has been minimized.

compiler-errors left a comment

Choose a reason for hiding this comment

compiler-errors Dec 17, 2024

Choose a reason for hiding this comment

estebank commented Dec 17, 2024 • edited Loading

compiler-errors commented Dec 17, 2024

This comment was marked as resolved.

rustbot commented Dec 19, 2024

This comment has been minimized.

estebank Dec 20, 2024

Choose a reason for hiding this comment

estebank Dec 20, 2024

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Dec 20, 2024

estebank Dec 20, 2024

Choose a reason for hiding this comment

estebank Dec 20, 2024

Choose a reason for hiding this comment

estebank commented Dec 20, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

compiler-errors commented Dec 23, 2024

llogiq commented Dec 23, 2024

compiler-errors commented Dec 23, 2024

compiler-errors commented Dec 23, 2024

llogiq commented Dec 23, 2024

estebank commented Dec 24, 2024

Implement `default_could_be_derived` and `default_overrides_default_fields` lints #134441

Implement `default_could_be_derived` and `default_overrides_default_fields` lints #134441

estebank commented Dec 17, 2024 •

edited

Loading

estebank commented Dec 20, 2024 •

edited

Loading