Handle field attributes in suggestions

smoelius · smoelius · commit cfc6c4b30d73 · 2024-12-15T13:14:33.000-05:00
diff --git a/clippy_lints/src/inconsistent_struct_constructor.rs b/clippy_lints/src/inconsistent_struct_constructor.rs
@@ -2,14 +2,15 @@ use clippy_config::Conf;
 use clippy_config::types::InitializerSuggestionApplicability;
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::fulfill_or_allowed;
-use clippy_utils::source::{snippet, snippet_opt};
+use clippy_utils::source::snippet_opt;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::Applicability;
-use rustc_hir::{self as hir, ExprKind, StructTailExpr};
+use rustc_hir::{self as hir, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::TyCtxt;
 use rustc_session::impl_lint_pass;
+use rustc_span::Span;
 use rustc_span::symbol::Symbol;
-use std::fmt::{self, Write as _};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -80,7 +81,7 @@ impl_lint_pass!(InconsistentStructConstructor => [INCONSISTENT_STRUCT_CONSTRUCTO
 
 impl<'tcx> LateLintPass<'tcx> for InconsistentStructConstructor {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
-        let ExprKind::Struct(qpath, fields, base) = expr.kind else {
+        let ExprKind::Struct(_, fields, _) = expr.kind else {
             return;
         };
         let applicability = if fields.iter().all(|f| f.is_shorthand) {
@@ -107,32 +108,15 @@ impl<'tcx> LateLintPass<'tcx> for InconsistentStructConstructor {
                 return;
             }
 
-            let mut ordered_fields: Vec<_> = fields.to_vec();
-            ordered_fields.sort_unstable_by_key(|id| def_order_map[&id.ident.name]);
-
-            let mut fields_snippet = String::new();
-            let (last_field, fields) = ordered_fields.split_last().unwrap();
-            for field in fields {
-                let _: fmt::Result = write!(fields_snippet, "{}, ", snippet_opt(cx, field.span).unwrap());
-            }
-            fields_snippet.push_str(&snippet_opt(cx, last_field.span).unwrap());
-
-            let base_snippet = if let StructTailExpr::Base(base) = base {
-                format!(", ..{}", snippet(cx, base.span, ".."))
-            } else {
-                String::new()
-            };
-
-            let sugg = format!(
-                "{} {{ {fields_snippet}{base_snippet} }}",
-                snippet(cx, qpath.span(), ".."),
-            );
+            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(
                     cx,
                     INCONSISTENT_STRUCT_CONSTRUCTOR,
-                    expr.span,
+                    span,
                     "struct constructor field order is inconsistent with struct definition field order",
                     "try",
                     sugg,
@@ -157,3 +141,45 @@ fn is_consistent_order<'tcx>(fields: &'tcx [hir::ExprField<'tcx>], def_order_map
 
     true
 }
+
+fn suggestion<'tcx>(
+    cx: &LateContext<'_>,
+    fields: &'tcx [hir::ExprField<'tcx>],
+    def_order_map: &FxHashMap<Symbol, usize>,
+) -> String {
+    let ws = fields
+        .windows(2)
+        .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()
+        })
+        .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())
+        .collect::<Vec<_>>();
+
+    assert_eq!(field_snippets.len(), ws.len() + 1);
+
+    let mut sugg = String::new();
+    for i in 0..field_snippets.len() {
+        sugg += &field_snippets[i];
+        if i < ws.len() {
+            sugg += &ws[i];
+        }
+    }
+    sugg
+}
+
+fn field_with_attrs_span(tcx: TyCtxt<'_>, field: &hir::ExprField<'_>) -> Span {
+    if let Some(attr) = tcx.hir().attrs(field.hir_id).first() {
+        field.span.with_lo(attr.span.lo())
+    } else {
+        field.span
+    }
+}
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
@@ -17,5 +17,25 @@ fn main() {
 
     Foo { x, y, z: z };
 
-    Foo { x, z: z, ..Default::default() };
+    Foo {
+        x,
+        z: z,
+        ..Default::default()
+    };
+}
+
+// https://github.com/rust-lang/rust-clippy/pull/13737#discussion_r1859261645
+mod field_attributes {
+    struct HirId;
+    struct BodyVisitor {
+        macro_unsafe_blocks: Vec<HirId>,
+        expn_depth: u32,
+    }
+    fn check_body(condition: bool) {
+        BodyVisitor {
+            macro_unsafe_blocks: Vec::new(),
+            #[expect(clippy::bool_to_int_with_if)] // obfuscates the meaning
+            expn_depth: if condition { 1 } else { 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
@@ -23,3 +23,19 @@ fn main() {
         ..Default::default()
     };
 }
+
+// https://github.com/rust-lang/rust-clippy/pull/13737#discussion_r1859261645
+mod field_attributes {
+    struct HirId;
+    struct BodyVisitor {
+        macro_unsafe_blocks: Vec<HirId>,
+        expn_depth: u32,
+    }
+    fn check_body(condition: bool) {
+        BodyVisitor {
+            #[expect(clippy::bool_to_int_with_if)] // obfuscates the meaning
+            expn_depth: if condition { 1 } else { 0 },
+            macro_unsafe_blocks: Vec::new(),
+        };
+    }
+}
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
@@ -1,21 +1,39 @@
 error: struct constructor field order is inconsistent with struct definition field order
-  --> tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs:18:5
+  --> tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs:18:11
    |
 LL |     Foo { y, x, z: z };
-   |     ^^^^^^^^^^^^^^^^^^ help: try: `Foo { x, y, z: z }`
+   |           ^^^^^^^^^^ help: 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)]`
 
 error: struct constructor field order is inconsistent with struct definition field order
-  --> tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs:20:5
+  --> tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs:21:9
    |
-LL | /     Foo {
-LL | |         z: z,
+LL | /         z: z,
 LL | |         x,
-LL | |         ..Default::default()
-LL | |     };
-   | |_____^ help: try: `Foo { x, z: z, ..Default::default() }`
+   | |_________^
+   |
+help: try
+   |
+LL ~         x,
+LL ~         z: z,
+   |
+
+error: struct constructor field order is inconsistent with struct definition field order
+  --> tests/ui-toml/toml_inconsistent_struct_constructor/conf_inconsistent_struct_constructor.rs:36:13
+   |
+LL | /             #[expect(clippy::bool_to_int_with_if)] // obfuscates the meaning
+LL | |             expn_depth: if condition { 1 } else { 0 },
+LL | |             macro_unsafe_blocks: Vec::new(),
+   | |___________________________________________^
+   |
+help: 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 2 previous errors
+error: aborting due to 3 previous errors
 
diff --git a/tests/ui/inconsistent_struct_constructor.fixed b/tests/ui/inconsistent_struct_constructor.fixed
@@ -60,7 +60,11 @@ mod with_base {
         let z = 1;
 
         // Should lint.
-        Foo { x, z, ..Default::default() };
+        Foo {
+            x,
+            z,
+            ..Default::default()
+        };
 
         // Should NOT lint because the order is consistent with the definition.
         Foo {
diff --git a/tests/ui/inconsistent_struct_constructor.stderr b/tests/ui/inconsistent_struct_constructor.stderr
@@ -1,21 +1,24 @@
 error: struct constructor field order is inconsistent with struct definition field order
-  --> tests/ui/inconsistent_struct_constructor.rs:36:9
+  --> tests/ui/inconsistent_struct_constructor.rs:36:15
    |
 LL |         Foo { y, x, z };
-   |         ^^^^^^^^^^^^^^^ help: try: `Foo { x, y, z }`
+   |               ^^^^^^^ help: try: `x, y, z`
    |
    = note: `-D clippy::inconsistent-struct-constructor` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::inconsistent_struct_constructor)]`
 
 error: struct constructor field order is inconsistent with struct definition field order
-  --> tests/ui/inconsistent_struct_constructor.rs:63:9
+  --> tests/ui/inconsistent_struct_constructor.rs:64:13
    |
-LL | /         Foo {
-LL | |             z,
+LL | /             z,
 LL | |             x,
-LL | |             ..Default::default()
-LL | |         };
-   | |_________^ help: try: `Foo { x, z, ..Default::default() }`
+   | |_____________^
+   |
+help: try
+   |
+LL ~             x,
+LL ~             z,
+   |
 
 error: aborting due to 2 previous errors
 
