diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 6902e212685c..eefb07f2c31e 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -565,8 +565,9 @@ A list of paths to types that should be treated as if they do not contain interi ## `initializer-suggestions` Whether to suggest reordering constructor fields when initializers are present. -Note that such suggestions are not applied automatically with `--fix`. The following example -[due to @ronnodas] shows why. Swapping the fields in the constructor produces incompilable code: +Warnings produced by this configuration aren't necessarily fixed by just reordering the fields. Even if the +suggested code would compile, it can change semantics if the initializer expressions have side effects. The +following example [from rust-clippy#11846] shows how the suggestion can run into borrow check errors: ```rust struct MyStruct { @@ -579,7 +580,7 @@ fn main() { } ``` -[due to @ronnodas]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924 +[from rust-clippy#11846]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924 **Default Value:** `false` diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index be84ff152ba5..03fa8ee54ad0 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -528,8 +528,9 @@ define_Conf! { ignore_interior_mutability: Vec = Vec::from(["bytes::Bytes".into()]), /// Whether to suggest reordering constructor fields when initializers are present. /// - /// Note that such suggestions are not applied automatically with `--fix`. The following example - /// [due to @ronnodas] shows why. Swapping the fields in the constructor produces incompilable code: + /// Warnings produced by this configuration aren't necessarily fixed by just reordering the fields. Even if the + /// suggested code would compile, it can change semantics if the initializer expressions have side effects. The + /// following example [from rust-clippy#11846] shows how the suggestion can run into borrow check errors: /// /// ```rust /// struct MyStruct { @@ -542,7 +543,7 @@ define_Conf! { /// } /// ``` /// - /// [due to @ronnodas]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924 + /// [from rust-clippy#11846]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924 #[lints(inconsistent_struct_constructor)] initializer_suggestions: bool = false, /// The maximum size of the `Err`-variant in a `Result` returned from a function diff --git a/clippy_lints/src/inconsistent_struct_constructor.rs b/clippy_lints/src/inconsistent_struct_constructor.rs index 0b23362117e9..9b7985967e53 100644 --- a/clippy_lints/src/inconsistent_struct_constructor.rs +++ b/clippy_lints/src/inconsistent_struct_constructor.rs @@ -1,7 +1,7 @@ use clippy_config::Conf; -use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::fulfill_or_allowed; -use clippy_utils::source::snippet_opt; +use clippy_utils::source::snippet; use rustc_data_structures::fx::FxHashMap; use rustc_errors::Applicability; use rustc_hir::{self as hir, ExprKind}; @@ -83,7 +83,8 @@ impl<'tcx> LateLintPass<'tcx> for InconsistentStructConstructor { let ExprKind::Struct(_, fields, _) = expr.kind else { return; }; - let applicability = if fields.iter().all(|f| f.is_shorthand) { + let all_fields_are_shorthand = fields.iter().all(|f| f.is_shorthand); + let applicability = if all_fields_are_shorthand { Applicability::MachineApplicable } else if self.initializer_suggestions { Applicability::MaybeIncorrect @@ -109,17 +110,22 @@ impl<'tcx> LateLintPass<'tcx> for InconsistentStructConstructor { let span = field_with_attrs_span(cx.tcx, fields.first().unwrap()) .with_hi(field_with_attrs_span(cx.tcx, fields.last().unwrap()).hi()); - let sugg = suggestion(cx, fields, &def_order_map); if !fulfill_or_allowed(cx, INCONSISTENT_STRUCT_CONSTRUCTOR, Some(ty_hir_id)) { - span_lint_and_sugg( + span_lint_and_then( cx, INCONSISTENT_STRUCT_CONSTRUCTOR, span, "struct constructor field order is inconsistent with struct definition field order", - "try", - sugg, - applicability, + |diag| { + let msg = if all_fields_are_shorthand { + "try" + } else { + "if the field evaluation order doesn't matter, try" + }; + let sugg = suggestion(cx, fields, &def_order_map); + diag.span_suggestion(span, msg, sugg, applicability); + }, ); } } @@ -151,8 +157,8 @@ fn suggestion<'tcx>( .map(|w| { let w0_span = field_with_attrs_span(cx.tcx, &w[0]); let w1_span = field_with_attrs_span(cx.tcx, &w[1]); - let span = w0_span.with_hi(w1_span.lo()).with_lo(w0_span.hi()); - snippet_opt(cx, span).unwrap() + let span = w0_span.between(w1_span); + snippet(cx, span, " ") }) .collect::>(); @@ -160,7 +166,7 @@ fn suggestion<'tcx>( fields.sort_unstable_by_key(|field| def_order_map[&field.ident.name]); let field_snippets = fields .iter() - .map(|field| snippet_opt(cx, field_with_attrs_span(cx.tcx, field)).unwrap()) + .map(|field| snippet(cx, field_with_attrs_span(cx.tcx, field), "..")) .collect::>(); assert_eq!(field_snippets.len(), ws.len() + 1); diff --git a/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.fixed b/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.fixed index 5d21b5052e52..8092e40ff9f1 100644 --- a/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.fixed +++ b/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.fixed @@ -39,3 +39,41 @@ mod field_attributes { }; } } + +// https://github.com/rust-lang/rust-clippy/pull/13737#discussion_r1874539800 +mod cfgs_between_fields { + #[allow(clippy::non_minimal_cfg)] + fn cfg_all() { + struct S { + a: i32, + b: i32, + #[cfg(all())] + c: i32, + d: i32, + } + let s = S { + a: 3, + b: 2, + #[cfg(all())] + c: 1, + d: 0, + }; + } + + fn cfg_any() { + struct S { + a: i32, + b: i32, + #[cfg(any())] + c: i32, + d: i32, + } + let s = S { + a: 3, + #[cfg(any())] + c: 1, + b: 2, + d: 0, + }; + } +} diff --git a/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs b/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs index 47ae74d2445f..cd1aff966528 100644 --- a/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs +++ b/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs @@ -39,3 +39,41 @@ mod field_attributes { }; } } + +// https://github.com/rust-lang/rust-clippy/pull/13737#discussion_r1874539800 +mod cfgs_between_fields { + #[allow(clippy::non_minimal_cfg)] + fn cfg_all() { + struct S { + a: i32, + b: i32, + #[cfg(all())] + c: i32, + d: i32, + } + let s = S { + d: 0, + #[cfg(all())] + c: 1, + b: 2, + a: 3, + }; + } + + fn cfg_any() { + struct S { + a: i32, + b: i32, + #[cfg(any())] + c: i32, + d: i32, + } + let s = S { + d: 0, + #[cfg(any())] + c: 1, + b: 2, + a: 3, + }; + } +} diff --git a/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.stderr b/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.stderr index da7a78c52483..d2533960b84c 100644 --- a/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.stderr +++ b/tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.stderr @@ -2,7 +2,7 @@ error: struct constructor field order is inconsistent with struct definition fie --> tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs:18:11 | LL | Foo { y, x, z: z }; - | ^^^^^^^^^^ help: try: `x, y, z: z` + | ^^^^^^^^^^ help: if the field evaluation order doesn't matter, try: `x, y, z: z` | = note: `-D clippy::inconsistent-struct-constructor` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::inconsistent_struct_constructor)]` @@ -14,7 +14,7 @@ LL | / z: z, LL | | x, | |_________^ | -help: try +help: if the field evaluation order doesn't matter, try | LL ~ x, LL ~ z: z, @@ -28,12 +28,50 @@ LL | | expn_depth: if condition { 1 } else { 0 }, LL | | macro_unsafe_blocks: Vec::new(), | |___________________________________________^ | -help: try +help: if the field evaluation order doesn't matter, try | LL ~ macro_unsafe_blocks: Vec::new(), LL + #[expect(clippy::bool_to_int_with_if)] // obfuscates the meaning LL ~ expn_depth: if condition { 1 } else { 0 }, | -error: aborting due to 3 previous errors +error: struct constructor field order is inconsistent with struct definition field order + --> tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs:55:13 + | +LL | / d: 0, +LL | | #[cfg(all())] +LL | | c: 1, +LL | | b: 2, +LL | | a: 3, + | |________________^ + | +help: if the field evaluation order doesn't matter, try + | +LL ~ a: 3, +LL + b: 2, +LL + #[cfg(all())] +LL + c: 1, +LL ~ d: 0, + | + +error: struct constructor field order is inconsistent with struct definition field order + --> tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs:72:13 + | +LL | / d: 0, +LL | | #[cfg(any())] +LL | | c: 1, +LL | | b: 2, +LL | | a: 3, + | |________________^ + | +help: if the field evaluation order doesn't matter, try + | +LL ~ a: 3, +LL + #[cfg(any())] +LL + c: 1, +LL + b: 2, +LL ~ d: 0, + | + +error: aborting due to 5 previous errors