New lint: `struct_fields_rest_default` #14412

WeiTheShinobi · 2025-03-15T14:08:33Z

changelog: [struct_fields_rest_default]: add new lint

rustbot · 2025-03-15T14:08:38Z

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

The current code would have a false positive for the extra test case to add in your main function:

    {
        struct Default;
        impl Default {
            fn default() -> Foo {
                Foo { a: 0, b: 0, c: 0 }
            }
        }
        // should not lint
        let _ = Foo { a: 0, ..Default::default() };
    }

Hence the proposal to switch to a late lint.

But also, why restrict this check to default()? Wouldn't any base value have the same issue if the goal is to force the developer to initialize all the fields?

samueltardieu · 2025-03-15T19:33:38Z

clippy_lints/src/struct_fields_rest_default.rs

+        if let ExprKind::Struct(struct_expr) = &expr.kind
+            && let StructRest::Base(base) = &struct_expr.rest
+            && !base.span.in_external_macro(cx.sess().source_map())
+            && let ExprKind::Call(call, _) = &base.kind
+            && let ExprKind::Path(_, path) = &call.kind
+            && let [part1, part2] = path.segments.as_slice()
+            && part1.ident.name == rustc_span::sym::Default
+            && part2.ident.name == rustc_span::kw::Default


This will match even if the Default that is in scope is not the one from the standard library. If you had used a late lint, you would be able to match against the real Default::default() diagnostic item as paths would be resolved. Also, you would be able to match against Foo::default() in your tests, which is the same thing as Default::default().

With a late lint, you could use something like the shorter:

Suggested change

if let ExprKind::Struct(struct_expr) = &expr.kind

&& let StructRest::Base(base) = &struct_expr.rest

&& !base.span.in_external_macro(cx.sess().source_map())

&& let ExprKind::Call(call, _) = &base.kind

&& let ExprKind::Path(_, path) = &call.kind

&& let [part1, part2] = path.segments.as_slice()

&& part1.ident.name == rustc_span::sym::Default

&& part2.ident.name == rustc_span::kw::Default

if let ExprKind::Struct(_, _, StructTailExpr::Base(base)) = &expr.kind

&& !base.span.in_external_macro(cx.sess().source_map())

&& let ExprKind::Call(func, _) = base.kind

&& let Some(did) = path_def_id(cx, func)

&& cx.tcx.is_diagnostic_item(sym::default_fn, did)

samueltardieu · 2025-03-15T19:36:10Z

tests/ui/auxiliary/external_macro.rs

+#[macro_export]
+macro_rules! external_struct_rest_default {
+    () => {
+        #[derive(Default)]
+        struct ExternalDefault {
+            a: i32,
+            b: i32,
+        }
+
+        let _ = ExternalDefault {
+            a: 10,
+            ..Default::default()
+        };
+    };
+}


Can't you use proc_macros::external!{ … } in your tests, without defining a dedicated external macro?

samueltardieu · 2025-03-15T19:37:26Z

clippy_lints/src/struct_fields_rest_default.rs

+    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
+        if let ExprKind::Struct(struct_expr) = &expr.kind
+            && let StructRest::Base(base) = &struct_expr.rest
+            && !base.span.in_external_macro(cx.sess().source_map())


Any reason to exclude only external macros for the base? Shouldn't also the check be excluded if expr comes from expansion?

Manishearth · 2025-03-17T17:44:36Z

I'll let @samueltardieu review and do the final approval

WeiTheShinobi · 2025-03-18T14:28:34Z

But also, why restrict this check to default()? Wouldn't any base value have the same issue if the goal is to force the developer to initialize all the fields?

That makes a lot of sense! So I modified the lint to reject any base value used, and lint no need to switch to a late lint.
I also fixed the problem related to the macro check.

Thank you!

WeiTheShinobi · 2025-03-19T08:12:03Z

Or we can restrict only Default::default and Foo::default,
providing flexibility for developers to use special cases.
When developers use these cases, they may have a better understanding of what they are doing,
rather than just trying to be lazy.
not lint something like
..Foo::special_case()
Is it make sense?

Or developers can solve this problem using a function or macro, it is fine?

// only restrict `default()`
let foo = Foo {
    a: 1,
    b: 2,
    ..Foo::spec()
}

// if restrict all base value.
let mut foo = Foo::spec();
foo.a = 1;
foo.b = 2;

// or
let foo = Foo::spec(a, b);

let foo = Foo::New(/* args.. */);

samueltardieu · 2025-03-20T22:13:45Z

Indeed, right now it looks like the lint would catch many legitimate cases: https://github.com/rust-lang/rust-clippy/actions/runs/13925788373?pr=14412. Maybe try restricting this to Default::default() or Type::default() and see how it behaves.

Out of curiosity, do you know of any project which forbids this, or would want to? Or is it a "just in case" lint?

WeiTheShinobi · 2025-03-21T13:47:03Z

Indeed, right now it looks like the lint would catch many legitimate cases: https://github.com/rust-lang/rust-clippy/actions/runs/13925788373?pr=14412. Maybe try restricting this to Default::default() or Type::default() and see how it behaves.

Ok.

Out of curiosity, do you know of any project which forbids this, or would want to? Or is it a "just in case" lint?

I'm not sure, but I've encountered bugs caused by this kind of situation before. I'll use it in our team's project.

Thank you!

samueltardieu · 2025-03-21T15:38:47Z

clippy_lints/src/struct_fields_rest_default.rs

+            && let Some(did) = path_def_id(cx, func)
+            && cx.tcx.is_diagnostic_item(sym::default_fn, did)


For a more compact test, you can use clippy_utils::is_trait_item():

Suggested change

&& let Some(did) = path_def_id(cx, func)

&& cx.tcx.is_diagnostic_item(sym::default_fn, did)

&& is_trait_item(cx, func, sym::Default)

samueltardieu · 2025-03-21T15:39:18Z

clippy_lints/src/struct_fields_rest_default.rs

+                base.span,
+                format!(
+                    "should not use `..{}` to omit rest of struct field initialization",
+                    snippet(cx, base.span, "..")


If the snippet cannot be obtained, "" would probably be clearer than ".." in an error message.

samueltardieu · 2025-03-21T15:39:50Z

clippy_lints/src/struct_fields_rest_default.rs

+                    snippet(cx, base.span, "..")
+                ),
+                Some(expr.span),
+                "each field's initial value should be explicitly specified",


Doesn't that no longer fit when another default expression is given?

flip1995

Some additional comments.

flip1995 · 2025-03-22T13:27:01Z

clippy_lints/src/struct_fields_rest_default.rs

+                    snippet(cx, base.span, "")
+                ),
+                Some(expr.span),
+                "explicitly specify all fields or use other base value instead of `..*::default()`",


I think this lint should only trigger, if the Default implementation is derived, but not when it is manually implemented. Manually implemented it isn't different from using something like Foo::get_foo().

flip1995 · 2025-03-22T13:28:39Z

clippy_lints/src/struct_fields_rest_default.rs

+                STRUCT_FIELDS_REST_DEFAULT,
+                base.span,
+                format!(
+                    "should not use `..{}` to omit rest of struct field initialization",


Suggested change

"should not use `..{}` to omit rest of struct field initialization",

"usage of `..{}` to initialize struct fields",

The error message should state what the code does, not what should (not) be done. This belongs into the help/note/sugg message, as you done below.

rustbot · 2025-04-16T06:27:04Z

☔ The latest upstream changes (possibly 8eed350) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned Manishearth Mar 15, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 15, 2025

WeiTheShinobi force-pushed the struct_fields_default branch 2 times, most recently from 2a1f974 to 1a98f1f Compare March 15, 2025 14:22

New lint: struct_fields_rest_default

b11506a

WeiTheShinobi force-pushed the struct_fields_default branch from 1a98f1f to b11506a Compare March 15, 2025 14:35

samueltardieu reviewed Mar 15, 2025

View reviewed changes

struct_fields_rest_default will reject any base value used.

85c6663

WeiTheShinobi force-pushed the struct_fields_default branch from ab8de84 to 85c6663 Compare March 18, 2025 14:30

Restrict only *::default

554f903

WeiTheShinobi force-pushed the struct_fields_default branch from a7bf8c7 to 554f903 Compare March 21, 2025 15:15

samueltardieu reviewed Mar 21, 2025

View reviewed changes

WeiTheShinobi force-pushed the struct_fields_default branch 2 times, most recently from 7d7365d to 7b8912c Compare March 21, 2025 16:44

Fix struct_fields_rest_default helping message

0dab1e3

WeiTheShinobi force-pushed the struct_fields_default branch from 7b8912c to 0dab1e3 Compare March 21, 2025 16:52

flip1995 reviewed Mar 22, 2025

View reviewed changes

WeiTheShinobi marked this pull request as draft March 23, 2025 07:06

lint struct_fields_rest_default only detect derive Default now

7f63851

WeiTheShinobi force-pushed the struct_fields_default branch from a8fd2c1 to 7f63851 Compare March 30, 2025 13:21

WeiTheShinobi marked this pull request as ready for review March 30, 2025 13:32

		&& let Some(did) = path_def_id(cx, func)
		&& cx.tcx.is_diagnostic_item(sym::default_fn, did)

	&& let Some(did) = path_def_id(cx, func)
	&& cx.tcx.is_diagnostic_item(sym::default_fn, did)
	&& is_trait_item(cx, func, sym::Default)

	"should not use `..{}` to omit rest of struct field initialization",
	"usage of `..{}` to initialize struct fields",

New lint: struct_fields_rest_default #14412

Are you sure you want to change the base?

New lint: struct_fields_rest_default #14412

Uh oh!

Conversation

WeiTheShinobi commented Mar 15, 2025

Uh oh!

rustbot commented Mar 15, 2025

Uh oh!

samueltardieu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Mar 17, 2025

Uh oh!

WeiTheShinobi commented Mar 18, 2025

Uh oh!

WeiTheShinobi commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samueltardieu commented Mar 20, 2025

Uh oh!

WeiTheShinobi commented Mar 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flip1995 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Apr 16, 2025

Uh oh!

Uh oh!

New lint: `struct_fields_rest_default` #14412

New lint: `struct_fields_rest_default` #14412

WeiTheShinobi commented Mar 19, 2025 •

edited

Loading