Skip to content

New lint: struct_fields_rest_default #14412

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6139,6 +6139,7 @@ Released 2018-09-13
[`strlen_on_c_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#strlen_on_c_strings
[`struct_excessive_bools`]: https://rust-lang.github.io/rust-clippy/master/index.html#struct_excessive_bools
[`struct_field_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#struct_field_names
[`struct_fields_rest_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#struct_fields_rest_default
[`stutter`]: https://rust-lang.github.io/rust-clippy/master/index.html#stutter
[`suboptimal_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#suboptimal_flops
[`suspicious_arithmetic_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_arithmetic_impl
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
crate::strings::STR_TO_STRING_INFO,
crate::strings::TRIM_SPLIT_WHITESPACE_INFO,
crate::strlen_on_c_strings::STRLEN_ON_C_STRINGS_INFO,
crate::struct_fields_rest_default::STRUCT_FIELDS_REST_DEFAULT_INFO,
crate::suspicious_operation_groupings::SUSPICIOUS_OPERATION_GROUPINGS_INFO,
crate::suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL_INFO,
crate::suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL_INFO,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ mod std_instead_of_core;
mod string_patterns;
mod strings;
mod strlen_on_c_strings;
mod struct_fields_rest_default;
mod suspicious_operation_groupings;
mod suspicious_trait_impl;
mod suspicious_xor_used_as_pow;
Expand Down Expand Up @@ -984,5 +985,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_late_pass(move |_| Box::new(non_std_lazy_statics::NonStdLazyStatic::new(conf)));
store.register_late_pass(|_| Box::new(manual_option_as_slice::ManualOptionAsSlice::new(conf)));
store.register_late_pass(|_| Box::new(single_option_map::SingleOptionMap));
store.register_late_pass(|_| Box::new(struct_fields_rest_default::StructFieldsDefault));
// add lints here, do not remove this comment, it's used in `new_lint`
}
79 changes: 79 additions & 0 deletions clippy_lints/src/struct_fields_rest_default.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::path_def_id;
use clippy_utils::source::snippet;
use rustc_hir::{ExprKind, StructTailExpr};
use rustc_lint::{LateLintPass, LintContext};
use rustc_session::declare_lint_pass;
use rustc_span::sym;

declare_clippy_lint! {
/// ### What it does
/// Check struct initialization uses `..*::default()` pattern to skip rest of struct field initialization.
///
/// ### Why restrict this?
/// Using `..*::default()` can hide field initialization when new fields are added to structs,
/// potentially leading to bugs where developers forget to explicitly set values for new fields.
///
/// ### Example
/// ```no_run
/// #[derive(Default)]
/// struct Foo {
/// a: i32,
/// b: i32,
/// // when add new filed `c`
/// c: i32,
/// }
///
/// let _ = Foo {
/// a: Default::default(),
/// ..Default::default()
/// // developer may forget to explicitly set field `c` and cause bug
/// };
/// ```
/// Use instead:
/// ```no_run
/// #[derive(Default)]
/// struct Foo {
/// a: i32,
/// b: i32,
/// // when add new filed `c`
/// c: i32,
/// }
///
/// // make the compiler check for new fields to avoid bug.
/// let _ = Foo {
/// a: Default::default(),
/// b: Default::default(),
/// c: Default::default(),
/// };
/// ```
#[clippy::version = "1.87.0"]
pub STRUCT_FIELDS_REST_DEFAULT,
restriction,
"should not use `..Default::default()` to omit rest of struct field initialization"
}

declare_lint_pass!(StructFieldsDefault => [STRUCT_FIELDS_REST_DEFAULT]);

impl<'tcx> LateLintPass<'tcx> for StructFieldsDefault {
fn check_expr(&mut self, cx: &rustc_lint::LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx>) {
if !expr.span.in_external_macro(cx.sess().source_map())
&& let ExprKind::Struct(_, _, StructTailExpr::Base(base)) = &expr.kind
&& let ExprKind::Call(func, _) = base.kind
&& let Some(did) = path_def_id(cx, func)
&& cx.tcx.is_diagnostic_item(sym::default_fn, did)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

{
span_lint_and_help(
cx,
STRUCT_FIELDS_REST_DEFAULT,
base.span,
format!(
"should not use `..{}` to omit rest of struct field initialization",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

snippet(cx, base.span, "..")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

),
Some(expr.span),
"each field's initial value should be explicitly specified",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
}
}
}
60 changes: 60 additions & 0 deletions tests/ui/struct_fields_rest_default.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
//@aux-build:proc_macros.rs
#![warn(clippy::struct_fields_rest_default)]
extern crate proc_macros;

#[derive(Default)]
struct Foo {
a: i32,
b: i32,
c: i32,
}

impl Foo {
fn get_foo() -> Self {
Foo { a: 0, b: 0, c: 0 }
}
}

fn main() {
#[rustfmt::skip]
let _ = Foo {
a: 10,
..Default::default()
//~^ struct_fields_rest_default
};

#[rustfmt::skip]
let _ = Foo {
a: 10,
..Foo::default()
//~^ struct_fields_rest_default
};

// should not lint
#[rustfmt::skip]
let _ = Foo {
a: 10,
..Foo::get_foo()
};

// should not lint in external macro
proc_macros::external! {
#[derive(Default)]
struct ExternalDefault {
a: i32,
b: i32,
}

let _ = ExternalDefault {
a: 10,
..Default::default()
};
}

// should not lint
let _ = Foo {
a: Default::default(),
b: Default::default(),
c: Default::default(),
};
}
38 changes: 38 additions & 0 deletions tests/ui/struct_fields_rest_default.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
error: should not use `..Default::default()` to omit rest of struct field initialization
--> tests/ui/struct_fields_rest_default.rs:22:11
|
LL | ..Default::default()
| ^^^^^^^^^^^^^^^^^^
|
help: each field's initial value should be explicitly specified
--> tests/ui/struct_fields_rest_default.rs:20:13
|
LL | let _ = Foo {
| _____________^
LL | | a: 10,
LL | | ..Default::default()
LL | |
LL | | };
| |_____^
= note: `-D clippy::struct-fields-rest-default` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::struct_fields_rest_default)]`

error: should not use `..Foo::default()` to omit rest of struct field initialization
--> tests/ui/struct_fields_rest_default.rs:29:11
|
LL | ..Foo::default()
| ^^^^^^^^^^^^^^
|
help: each field's initial value should be explicitly specified
--> tests/ui/struct_fields_rest_default.rs:27:13
|
LL | let _ = Foo {
| _____________^
LL | | a: 10,
LL | | ..Foo::default()
LL | |
LL | | };
| |_____^

error: aborting due to 2 previous errors