Address all review comments but one

smoelius · smoelius · commit dbff5cd3e685 · 2024-12-09T06:24:25.000-05:00
This comment is not yet addressed: #13737 (comment)
diff --git 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
@@ -528,8 +528,9 @@ define_Conf! {
     ignore_interior_mutability: Vec<String> = 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
@@ -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,16 +157,16 @@ 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::<Vec<_>>();
 
     let mut fields = fields.to_vec();
     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::<Vec<_>>();
 
     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
@@ -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
@@ -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
@@ -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
 
