Make "all fields are shorthand" requirement configurable

Handle field attributes in suggestions
Fix adjacent code
Address review comments
rust-lang/rust-clippy#13737 (comment)
Address all review comments but one
This comment is not yet addressed: rust-lang/rust-clippy#13737 (comment)
`initializer_suggestions` -> `lint_inconsistent_struct_field_initializers`

Author: smoelius

CHANGELOG.md

@@ -6250,6 +6250,7 @@ Released 2018-09-13
 [`future-size-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#future-size-threshold
 [`ignore-interior-mutability`]: https://doc.rust-lang.org/clippy/lint_configuration.html#ignore-interior-mutability
 [`large-error-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#large-error-threshold
+[`lint-inconsistent-struct-field-initializers`]: https://doc.rust-lang.org/clippy/lint_configuration.html#lint-inconsistent-struct-field-initializers
 [`literal-representation-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#literal-representation-threshold
 [`matches-for-let-else`]: https://doc.rust-lang.org/clippy/lint_configuration.html#matches-for-let-else
 [`max-fn-params-bools`]: https://doc.rust-lang.org/clippy/lint_configuration.html#max-fn-params-bools

book/src/lint_configuration.md

@@ -572,6 +572,33 @@ The maximum size of the `Err`-variant in a `Result` returned from a function
 *[`result_large_err`](https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err)
 
 
+## `lint-inconsistent-struct-field-initializers`
+Whether to suggest reordering constructor fields when initializers are present.
+
+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
+structMyStruct {
+vector:Vec<u32>,
+length:usize
+}
+fnmain() {
+letvector=vec![1,2,3];
+MyStruct { length:vector.len(), vector};
+}
+```
+
+[from rust-clippy#11846]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924
+
+**Default Value:**`false`
+
+---
+**Affected lints:**
+*[`inconsistent_struct_constructor`](https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor)
+
+
 ## `literal-representation-threshold`
 The lower bound for linting decimal literals
 

clippy.toml

@@ -1,5 +1,7 @@
 avoid-breaking-exported-api = false
 
+lint-inconsistent-struct-field-initializers = true
+
 [[disallowed-methods]]
 path = "rustc_lint::context::LintContext::lint"
 reason = "this function does not add a link to our documentation, please use the `clippy_utils::diagnostics::span_lint*` functions instead"

clippy_config/src/conf.rs

@@ -529,6 +529,26 @@ define_Conf! {
 /// The maximum size of the `Err`-variant in a `Result` returned from a function
  #[lints(result_large_err)]
  large_error_threshold:u64 = 128,
+/// Whether to suggest reordering constructor fields when initializers are present.
+///
+/// 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 {
+/// vector: Vec<u32>,
+/// length: usize
+/// }
+/// fn main() {
+/// let vector = vec![1,2,3];
+/// MyStruct { length: vector.len(), vector};
+/// }
+/// ```
+///
+/// [from rust-clippy#11846]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924
+ #[lints(inconsistent_struct_constructor)]
+ lint_inconsistent_struct_field_initializers:bool = false,
 /// The lower bound for linting decimal literals
  #[lints(decimal_literal_representation)]
  literal_representation_threshold:u64 = 16384,

clippy_dev/src/fmt.rs

@@ -179,8 +179,8 @@ fn fmt_conf(check: bool) -> Result<(), Error> {
 #[expect(clippy::drain_collect)]
  fields.push(ClippyConf{
  name,
-lints: lints.drain(..).collect(),
 attrs:&conf[attrs_start..attrs_end],
+lints: lints.drain(..).collect(),
 field: conf[field_start..i].trim_end(),
 });
  attrs_start = i;
@@ -191,8 +191,8 @@ fn fmt_conf(check: bool) -> Result<(), Error> {
 #[expect(clippy::drain_collect)]
  fields.push(ClippyConf{
  name,
-lints: lints.drain(..).collect(),
 attrs:&conf[attrs_start..attrs_end],
+lints: lints.drain(..).collect(),
 field: conf[field_start..i].trim_end(),
 });
  attrs_start = i;
@@ -220,8 +220,8 @@ fn fmt_conf(check: bool) -> Result<(), Error> {
 }
  fields.push(ClippyConf{
  name,
- lints,
 attrs:&conf[attrs_start..attrs_end],
+ lints,
 field: conf[field_start..].trim_end(),
 });
 

clippy_lints/src/arbitrary_source_item_ordering.rs

@@ -428,8 +428,8 @@ impl<'tcx> LateLintPass<'tcx> for ArbitrarySourceItemOrdering {
 
 // Makes a note of the current item for comparison with the next.
  cur_t = Some(CurItem{
-order: module_level_order,
  item,
+order: module_level_order,
 name:get_item_name(item),
 });
 }

clippy_lints/src/doc/mod.rs

@@ -798,8 +798,8 @@ fn check_attrs(cx: &LateContext<'_>, valid_idents: &FxHashSet<String>, attrs: &[
  parser.into_offset_iter(),
 &doc,
 Fragments{
-fragments:&fragments,
 doc:&doc,
+fragments:&fragments,
 },
 ))
 }

clippy_lints/src/entry.rs

@@ -678,12 +678,12 @@ fn find_insert_calls<'tcx>(
 map: contains_expr.map,
 key: contains_expr.key,
 ctxt: expr.span.ctxt(),
-edits:Vec::new(),
-is_map_used:false,
 allow_insert_closure:true,
 can_use_entry:true,
 in_tail_pos:true,
 is_single_insert:true,
+is_map_used:false,
+edits:Vec::new(),
 loops:Vec::new(),
 locals:HirIdSet::default(),
 };

clippy_lints/src/implied_bounds_in_impls.rs

@@ -243,11 +243,11 @@ fn collect_supertrait_bounds<'tcx>(cx: &LateContext<'tcx>, bounds: GenericBounds
  && !predicates.is_empty()
 {
 Some(ImplTraitBound{
+span: bound.span(),
  predicates,
+ trait_def_id,
 args: path.args.map_or([].as_slice(), |p| p.args),
 constraints: path.args.map_or([].as_slice(), |p| p.constraints),
- trait_def_id,
-span: bound.span(),
 })
 }else{
 None

clippy_lints/src/inconsistent_struct_constructor.rs

@@ -1,19 +1,21 @@
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_config::Conf;
+use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::fulfill_or_allowed;
 use clippy_utils::source::snippet;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::Applicability;
-use rustc_hir::{selfas hir,ExprKind,StructTailExpr};
+use rustc_hir::{selfas hir,ExprKind};
 use rustc_lint::{LateContext,LateLintPass};
-use rustc_session::declare_lint_pass;
+use rustc_middle::ty::TyCtxt;
+use rustc_session::impl_lint_pass;
+use rustc_span::Span;
 use rustc_span::symbol::Symbol;
-use std::fmt::{self,Writeas _};
 
 declare_clippy_lint!{
 /// ### What it does
-/// Checks for struct constructors where all fields are shorthand and
-/// the order of the field init shorthand in the constructor is inconsistent
-/// with the order in the struct definition.
+/// Checks for struct constructors where the order of the field
+/// init in the constructor is inconsistent with the order in the
+/// struct definition.
 ///
 /// ### Why is this bad?
 /// Since the order of fields in a constructor doesn't affect the
@@ -59,16 +61,37 @@ declare_clippy_lint! {
  #[clippy::version = "1.52.0"]
 pubINCONSISTENT_STRUCT_CONSTRUCTOR,
  pedantic,
-"the order of the field init shorthand is inconsistent with the order in the struct definition"
+"the order of the field init is inconsistent with the order in the struct definition"
 }
 
-declare_lint_pass!(InconsistentStructConstructor => [INCONSISTENT_STRUCT_CONSTRUCTOR]);
+pubstructInconsistentStructConstructor{
+lint_inconsistent_struct_field_initializers:bool,
+}
+
+implInconsistentStructConstructor{
+pubfnnew(conf:&'staticConf) -> Self{
+Self{
+lint_inconsistent_struct_field_initializers: conf.lint_inconsistent_struct_field_initializers,
+}
+}
+}
+
+impl_lint_pass!(InconsistentStructConstructor => [INCONSISTENT_STRUCT_CONSTRUCTOR]);
 
 impl<'tcx>LateLintPass<'tcx>forInconsistentStructConstructor{
 fncheck_expr(&mutself,cx:&LateContext<'tcx>,expr:&'tcx hir::Expr<'_>){
-ifletExprKind::Struct(qpath, fields, base) = expr.kind
- && fields.iter().all(|f| f.is_shorthand)
- && !expr.span.from_expansion()
+letExprKind::Struct(_, fields, _) = expr.kindelse{
+return;
+};
+let all_fields_are_shorthand = fields.iter().all(|f| f.is_shorthand);
+let applicability = if all_fields_are_shorthand {
+Applicability::MachineApplicable
+}elseifself.lint_inconsistent_struct_field_initializers{
+Applicability::MaybeIncorrect
+}else{
+return;
+};
+if !expr.span.from_expansion()
  && let ty = cx.typeck_results().expr_ty(expr)
  && letSome(adt_def) = ty.ty_adt_def()
  && adt_def.is_struct()
@@ -85,36 +108,24 @@ impl<'tcx> LateLintPass<'tcx> for InconsistentStructConstructor {
 return;
 }
 
-letmut ordered_fields:Vec<_> = fields.iter().map(|f| f.ident.name).collect();
- ordered_fields.sort_unstable_by_key(|id| def_order_map[id]);
-
-letmut fields_snippet = String::new();
-let(last_ident, idents) = ordered_fields.split_last().unwrap();
-for ident in idents {
-let _: fmt::Result = write!(fields_snippet,"{ident}, ");
-}
- fields_snippet.push_str(&last_ident.to_string());
-
-let base_snippet = ifletStructTailExpr::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());
 
 if !fulfill_or_allowed(cx,INCONSISTENT_STRUCT_CONSTRUCTOR,Some(ty_hir_id)){
-span_lint_and_sugg(
+span_lint_and_then(
  cx,
 INCONSISTENT_STRUCT_CONSTRUCTOR,
-expr.span,
+ span,
 "struct constructor field order is inconsistent with struct definition field order",
-"try",
- sugg,
-Applicability::MachineApplicable,
+ |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);
+},
 );
 }
 }
@@ -135,3 +146,45 @@ fn is_consistent_order<'tcx>(fields: &'tcx [hir::ExprField<'tcx>], def_order_map
 
 true
 }
+
+fnsuggestion<'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.between(w1_span);
+snippet(cx, span," ")
+})
+.collect::<Vec<_>>();
+
+letmut fields = fields.to_vec();
+ fields.sort_unstable_by_key(|field| def_order_map[&field.ident.name]);
+let field_snippets = fields
+.iter()
+.map(|field| snippet(cx,field_with_attrs_span(cx.tcx, field),".."))
+.collect::<Vec<_>>();
+
+assert_eq!(field_snippets.len(), ws.len() + 1);
+
+letmut sugg = String::new();
+for i in0..field_snippets.len(){
+ sugg += &field_snippets[i];
+if i < ws.len(){
+ sugg += &ws[i];
+}
+}
+ sugg
+}
+
+fnfield_with_attrs_span(tcx:TyCtxt<'_>,field:&hir::ExprField<'_>) -> Span{
+ifletSome(attr) = tcx.hir().attrs(field.hir_id).first(){
+ field.span.with_lo(attr.span.lo())
+}else{
+ field.span
+}
+}