Skip to content

Commit 8a38bcc

Browse files
committed
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`
1 parent b8e569e commit 8a38bcc

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+450
-97
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -6250,6 +6250,7 @@ Released 2018-09-13
62506250
[`future-size-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#future-size-threshold
62516251
[`ignore-interior-mutability`]: https://doc.rust-lang.org/clippy/lint_configuration.html#ignore-interior-mutability
62526252
[`large-error-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#large-error-threshold
6253+
[`lint-inconsistent-struct-field-initializers`]: https://doc.rust-lang.org/clippy/lint_configuration.html#lint-inconsistent-struct-field-initializers
62536254
[`literal-representation-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#literal-representation-threshold
62546255
[`matches-for-let-else`]: https://doc.rust-lang.org/clippy/lint_configuration.html#matches-for-let-else
62556256
[`max-fn-params-bools`]: https://doc.rust-lang.org/clippy/lint_configuration.html#max-fn-params-bools

book/src/lint_configuration.md

+27
Original file line numberDiff line numberDiff line change
@@ -572,6 +572,33 @@ The maximum size of the `Err`-variant in a `Result` returned from a function
572572
* [`result_large_err`](https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err)
573573

574574

575+
## `lint-inconsistent-struct-field-initializers`
576+
Whether to suggest reordering constructor fields when initializers are present.
577+
578+
Warnings produced by this configuration aren't necessarily fixed by just reordering the fields. Even if the
579+
suggested code would compile, it can change semantics if the initializer expressions have side effects. The
580+
following example [from rust-clippy#11846] shows how the suggestion can run into borrow check errors:
581+
582+
```rust
583+
struct MyStruct {
584+
vector: Vec<u32>,
585+
length: usize
586+
}
587+
fn main() {
588+
let vector = vec![1,2,3];
589+
MyStruct { length: vector.len(), vector};
590+
}
591+
```
592+
593+
[from rust-clippy#11846]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924
594+
595+
**Default Value:** `false`
596+
597+
---
598+
**Affected lints:**
599+
* [`inconsistent_struct_constructor`](https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor)
600+
601+
575602
## `literal-representation-threshold`
576603
The lower bound for linting decimal literals
577604

clippy.toml

+2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
avoid-breaking-exported-api = false
22

3+
lint-inconsistent-struct-field-initializers = true
4+
35
[[disallowed-methods]]
46
path = "rustc_lint::context::LintContext::lint"
57
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

+20
Original file line numberDiff line numberDiff line change
@@ -529,6 +529,26 @@ define_Conf! {
529529
/// The maximum size of the `Err`-variant in a `Result` returned from a function
530530
#[lints(result_large_err)]
531531
large_error_threshold: u64 = 128,
532+
/// Whether to suggest reordering constructor fields when initializers are present.
533+
///
534+
/// Warnings produced by this configuration aren't necessarily fixed by just reordering the fields. Even if the
535+
/// suggested code would compile, it can change semantics if the initializer expressions have side effects. The
536+
/// following example [from rust-clippy#11846] shows how the suggestion can run into borrow check errors:
537+
///
538+
/// ```rust
539+
/// struct MyStruct {
540+
/// vector: Vec<u32>,
541+
/// length: usize
542+
/// }
543+
/// fn main() {
544+
/// let vector = vec![1,2,3];
545+
/// MyStruct { length: vector.len(), vector};
546+
/// }
547+
/// ```
548+
///
549+
/// [from rust-clippy#11846]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924
550+
#[lints(inconsistent_struct_constructor)]
551+
lint_inconsistent_struct_field_initializers: bool = false,
532552
/// The lower bound for linting decimal literals
533553
#[lints(decimal_literal_representation)]
534554
literal_representation_threshold: u64 = 16384,

clippy_dev/src/fmt.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -179,8 +179,8 @@ fn fmt_conf(check: bool) -> Result<(), Error> {
179179
#[expect(clippy::drain_collect)]
180180
fields.push(ClippyConf {
181181
name,
182-
lints: lints.drain(..).collect(),
183182
attrs: &conf[attrs_start..attrs_end],
183+
lints: lints.drain(..).collect(),
184184
field: conf[field_start..i].trim_end(),
185185
});
186186
attrs_start = i;
@@ -191,8 +191,8 @@ fn fmt_conf(check: bool) -> Result<(), Error> {
191191
#[expect(clippy::drain_collect)]
192192
fields.push(ClippyConf {
193193
name,
194-
lints: lints.drain(..).collect(),
195194
attrs: &conf[attrs_start..attrs_end],
195+
lints: lints.drain(..).collect(),
196196
field: conf[field_start..i].trim_end(),
197197
});
198198
attrs_start = i;
@@ -220,8 +220,8 @@ fn fmt_conf(check: bool) -> Result<(), Error> {
220220
}
221221
fields.push(ClippyConf {
222222
name,
223-
lints,
224223
attrs: &conf[attrs_start..attrs_end],
224+
lints,
225225
field: conf[field_start..].trim_end(),
226226
});
227227

clippy_lints/src/arbitrary_source_item_ordering.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -428,8 +428,8 @@ impl<'tcx> LateLintPass<'tcx> for ArbitrarySourceItemOrdering {
428428

429429
// Makes a note of the current item for comparison with the next.
430430
cur_t = Some(CurItem {
431-
order: module_level_order,
432431
item,
432+
order: module_level_order,
433433
name: get_item_name(item),
434434
});
435435
}

clippy_lints/src/doc/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -798,8 +798,8 @@ fn check_attrs(cx: &LateContext<'_>, valid_idents: &FxHashSet<String>, attrs: &[
798798
parser.into_offset_iter(),
799799
&doc,
800800
Fragments {
801-
fragments: &fragments,
802801
doc: &doc,
802+
fragments: &fragments,
803803
},
804804
))
805805
}

clippy_lints/src/entry.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -678,12 +678,12 @@ fn find_insert_calls<'tcx>(
678678
map: contains_expr.map,
679679
key: contains_expr.key,
680680
ctxt: expr.span.ctxt(),
681-
edits: Vec::new(),
682-
is_map_used: false,
683681
allow_insert_closure: true,
684682
can_use_entry: true,
685683
in_tail_pos: true,
686684
is_single_insert: true,
685+
is_map_used: false,
686+
edits: Vec::new(),
687687
loops: Vec::new(),
688688
locals: HirIdSet::default(),
689689
};

clippy_lints/src/implied_bounds_in_impls.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -243,11 +243,11 @@ fn collect_supertrait_bounds<'tcx>(cx: &LateContext<'tcx>, bounds: GenericBounds
243243
&& !predicates.is_empty()
244244
{
245245
Some(ImplTraitBound {
246+
span: bound.span(),
246247
predicates,
248+
trait_def_id,
247249
args: path.args.map_or([].as_slice(), |p| p.args),
248250
constraints: path.args.map_or([].as_slice(), |p| p.constraints),
249-
trait_def_id,
250-
span: bound.span(),
251251
})
252252
} else {
253253
None

clippy_lints/src/inconsistent_struct_constructor.rs

+90-37
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,21 @@
1-
use clippy_utils::diagnostics::span_lint_and_sugg;
1+
use clippy_config::Conf;
2+
use clippy_utils::diagnostics::span_lint_and_then;
23
use clippy_utils::fulfill_or_allowed;
34
use clippy_utils::source::snippet;
45
use rustc_data_structures::fx::FxHashMap;
56
use rustc_errors::Applicability;
6-
use rustc_hir::{self as hir, ExprKind, StructTailExpr};
7+
use rustc_hir::{self as hir, ExprKind};
78
use rustc_lint::{LateContext, LateLintPass};
8-
use rustc_session::declare_lint_pass;
9+
use rustc_middle::ty::TyCtxt;
10+
use rustc_session::impl_lint_pass;
11+
use rustc_span::Span;
912
use rustc_span::symbol::Symbol;
10-
use std::fmt::{self, Write as _};
1113

1214
declare_clippy_lint! {
1315
/// ### What it does
14-
/// Checks for struct constructors where all fields are shorthand and
15-
/// the order of the field init shorthand in the constructor is inconsistent
16-
/// with the order in the struct definition.
16+
/// Checks for struct constructors where the order of the field
17+
/// init in the constructor is inconsistent with the order in the
18+
/// struct definition.
1719
///
1820
/// ### Why is this bad?
1921
/// Since the order of fields in a constructor doesn't affect the
@@ -59,16 +61,37 @@ declare_clippy_lint! {
5961
#[clippy::version = "1.52.0"]
6062
pub INCONSISTENT_STRUCT_CONSTRUCTOR,
6163
pedantic,
62-
"the order of the field init shorthand is inconsistent with the order in the struct definition"
64+
"the order of the field init is inconsistent with the order in the struct definition"
6365
}
6466

65-
declare_lint_pass!(InconsistentStructConstructor => [INCONSISTENT_STRUCT_CONSTRUCTOR]);
67+
pub struct InconsistentStructConstructor {
68+
lint_inconsistent_struct_field_initializers: bool,
69+
}
70+
71+
impl InconsistentStructConstructor {
72+
pub fn new(conf: &'static Conf) -> Self {
73+
Self {
74+
lint_inconsistent_struct_field_initializers: conf.lint_inconsistent_struct_field_initializers,
75+
}
76+
}
77+
}
78+
79+
impl_lint_pass!(InconsistentStructConstructor => [INCONSISTENT_STRUCT_CONSTRUCTOR]);
6680

6781
impl<'tcx> LateLintPass<'tcx> for InconsistentStructConstructor {
6882
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
69-
if let ExprKind::Struct(qpath, fields, base) = expr.kind
70-
&& fields.iter().all(|f| f.is_shorthand)
71-
&& !expr.span.from_expansion()
83+
let ExprKind::Struct(_, fields, _) = expr.kind else {
84+
return;
85+
};
86+
let all_fields_are_shorthand = fields.iter().all(|f| f.is_shorthand);
87+
let applicability = if all_fields_are_shorthand {
88+
Applicability::MachineApplicable
89+
} else if self.lint_inconsistent_struct_field_initializers {
90+
Applicability::MaybeIncorrect
91+
} else {
92+
return;
93+
};
94+
if !expr.span.from_expansion()
7295
&& let ty = cx.typeck_results().expr_ty(expr)
7396
&& let Some(adt_def) = ty.ty_adt_def()
7497
&& adt_def.is_struct()
@@ -85,36 +108,24 @@ impl<'tcx> LateLintPass<'tcx> for InconsistentStructConstructor {
85108
return;
86109
}
87110

88-
let mut ordered_fields: Vec<_> = fields.iter().map(|f| f.ident.name).collect();
89-
ordered_fields.sort_unstable_by_key(|id| def_order_map[id]);
90-
91-
let mut fields_snippet = String::new();
92-
let (last_ident, idents) = ordered_fields.split_last().unwrap();
93-
for ident in idents {
94-
let _: fmt::Result = write!(fields_snippet, "{ident}, ");
95-
}
96-
fields_snippet.push_str(&last_ident.to_string());
97-
98-
let base_snippet = if let StructTailExpr::Base(base) = base {
99-
format!(", ..{}", snippet(cx, base.span, ".."))
100-
} else {
101-
String::new()
102-
};
103-
104-
let sugg = format!(
105-
"{} {{ {fields_snippet}{base_snippet} }}",
106-
snippet(cx, qpath.span(), ".."),
107-
);
111+
let span = field_with_attrs_span(cx.tcx, fields.first().unwrap())
112+
.with_hi(field_with_attrs_span(cx.tcx, fields.last().unwrap()).hi());
108113

109114
if !fulfill_or_allowed(cx, INCONSISTENT_STRUCT_CONSTRUCTOR, Some(ty_hir_id)) {
110-
span_lint_and_sugg(
115+
span_lint_and_then(
111116
cx,
112117
INCONSISTENT_STRUCT_CONSTRUCTOR,
113-
expr.span,
118+
span,
114119
"struct constructor field order is inconsistent with struct definition field order",
115-
"try",
116-
sugg,
117-
Applicability::MachineApplicable,
120+
|diag| {
121+
let msg = if all_fields_are_shorthand {
122+
"try"
123+
} else {
124+
"if the field evaluation order doesn't matter, try"
125+
};
126+
let sugg = suggestion(cx, fields, &def_order_map);
127+
diag.span_suggestion(span, msg, sugg, applicability);
128+
},
118129
);
119130
}
120131
}
@@ -135,3 +146,45 @@ fn is_consistent_order<'tcx>(fields: &'tcx [hir::ExprField<'tcx>], def_order_map
135146

136147
true
137148
}
149+
150+
fn suggestion<'tcx>(
151+
cx: &LateContext<'_>,
152+
fields: &'tcx [hir::ExprField<'tcx>],
153+
def_order_map: &FxHashMap<Symbol, usize>,
154+
) -> String {
155+
let ws = fields
156+
.windows(2)
157+
.map(|w| {
158+
let w0_span = field_with_attrs_span(cx.tcx, &w[0]);
159+
let w1_span = field_with_attrs_span(cx.tcx, &w[1]);
160+
let span = w0_span.between(w1_span);
161+
snippet(cx, span, " ")
162+
})
163+
.collect::<Vec<_>>();
164+
165+
let mut fields = fields.to_vec();
166+
fields.sort_unstable_by_key(|field| def_order_map[&field.ident.name]);
167+
let field_snippets = fields
168+
.iter()
169+
.map(|field| snippet(cx, field_with_attrs_span(cx.tcx, field), ".."))
170+
.collect::<Vec<_>>();
171+
172+
assert_eq!(field_snippets.len(), ws.len() + 1);
173+
174+
let mut sugg = String::new();
175+
for i in 0..field_snippets.len() {
176+
sugg += &field_snippets[i];
177+
if i < ws.len() {
178+
sugg += &ws[i];
179+
}
180+
}
181+
sugg
182+
}
183+
184+
fn field_with_attrs_span(tcx: TyCtxt<'_>, field: &hir::ExprField<'_>) -> Span {
185+
if let Some(attr) = tcx.hir().attrs(field.hir_id).first() {
186+
field.span.with_lo(attr.span.lo())
187+
} else {
188+
field.span
189+
}
190+
}

clippy_lints/src/lib.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -648,7 +648,11 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
648648
store.register_late_pass(|_| Box::new(implicit_return::ImplicitReturn));
649649
store.register_late_pass(move |_| Box::new(implicit_saturating_sub::ImplicitSaturatingSub::new(conf)));
650650
store.register_late_pass(|_| Box::new(default_numeric_fallback::DefaultNumericFallback));
651-
store.register_late_pass(|_| Box::new(inconsistent_struct_constructor::InconsistentStructConstructor));
651+
store.register_late_pass(move |_| {
652+
Box::new(inconsistent_struct_constructor::InconsistentStructConstructor::new(
653+
conf,
654+
))
655+
});
652656
store.register_late_pass(|_| Box::new(non_octal_unix_permissions::NonOctalUnixPermissions));
653657
store.register_early_pass(|| Box::new(unnecessary_self_imports::UnnecessarySelfImports));
654658
store.register_late_pass(move |_| Box::new(approx_const::ApproxConstant::new(conf)));

clippy_lints/src/loops/infinite_loop.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,8 @@ pub(super) fn check<'tcx>(
3838
cx,
3939
label,
4040
inner_labels: label.into_iter().collect(),
41-
is_finite: false,
4241
loop_depth: 0,
42+
is_finite: false,
4343
};
4444
loop_visitor.visit_block(loop_block);
4545

clippy_lints/src/macro_metavars_in_unsafe.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -220,11 +220,11 @@ impl<'tcx> LateLintPass<'tcx> for ExprMetavarsInUnsafe {
220220
// `check_stmt_post` on `(Late)LintPass`, which we'd need to detect when we're leaving a macro span
221221

222222
let mut vis = BodyVisitor {
223+
macro_unsafe_blocks: Vec::new(),
223224
#[expect(clippy::bool_to_int_with_if)] // obfuscates the meaning
224225
expn_depth: if body.value.span.from_expansion() { 1 } else { 0 },
225-
macro_unsafe_blocks: Vec::new(),
226-
lint: self,
227-
cx
226+
cx,
227+
lint: self
228228
};
229229
vis.visit_body(body);
230230
}

clippy_lints/src/methods/needless_collect.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -470,14 +470,14 @@ fn detect_iter_and_into_iters<'tcx: 'a, 'a>(
470470
captured_ids: HirIdSet,
471471
) -> Option<Vec<IterFunction>> {
472472
let mut visitor = IterFunctionVisitor {
473-
uses: Vec::new(),
474-
target: id,
475-
seen_other: false,
476-
cx,
477-
current_mutably_captured_ids: HirIdSet::default(),
478473
illegal_mutable_capture_ids: captured_ids,
474+
current_mutably_captured_ids: HirIdSet::default(),
475+
cx,
476+
uses: Vec::new(),
479477
hir_id_uses_map: FxHashMap::default(),
480478
current_statement_hir_id: None,
479+
seen_other: false,
480+
target: id,
481481
};
482482
visitor.visit_block(block);
483483
if visitor.seen_other {

clippy_lints/src/methods/str_splitn.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -297,8 +297,8 @@ fn parse_iter_usage<'tcx>(
297297
{
298298
Some(IterUsage {
299299
kind: IterUsageKind::NextTuple,
300-
span: e.span,
301300
unwrap_kind: None,
301+
span: e.span,
302302
})
303303
} else {
304304
None

0 commit comments

Comments
 (0)