Skip to content

Commit

Permalink
Address all review comments but one
Browse files Browse the repository at this point in the history
This comment is not yet addressed: rust-lang#13737 (comment)
  • Loading branch information
smoelius committed Dec 9, 2024
1 parent 3b2082e commit dbff5cd
Show file tree
Hide file tree
Showing 6 changed files with 143 additions and 21 deletions.
7 changes: 4 additions & 3 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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`

Expand Down
7 changes: 4 additions & 3 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down
28 changes: 17 additions & 11 deletions clippy_lints/src/inconsistent_struct_constructor.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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
Expand All @@ -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);
},
);
}
}
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)]`
Expand All @@ -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,
Expand All @@ -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

0 comments on commit dbff5cd

Please sign in to comment.