Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make inconsistent_struct_constructor "all fields are shorthand" requirement configurable #13737

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6201,6 +6201,7 @@ Released 2018-09-13
[`excessive-nesting-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#excessive-nesting-threshold
[`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
[`initializer-suggestions`]: https://doc.rust-lang.org/clippy/lint_configuration.html#initializer-suggestions
[`large-error-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#large-error-threshold
[`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
Expand Down
30 changes: 30 additions & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,36 @@ A list of paths to types that should be treated as if they do not contain interi
* [`mutable_key_type`](https://rust-lang.github.io/rust-clippy/master/index.html#mutable_key_type)


## `initializer-suggestions`
y21 marked this conversation as resolved.
Show resolved Hide resolved
Suggestion behavior when initializers are present. Options are:

- "none": do not suggest
- "maybe-incorrect": suggest, but do not apply suggestions with `--fix`
- "machine-applicable": suggest and apply suggestions with `--fix`

The following example [due to @ronnodas] shows why "maybe-incorrect" may be the right choice.
Swapping the fields in the constructor produces incompilable code:

```rust
struct MyStruct {
vector: Vec<u32>,
length: usize
}
fn main() {
let vector = vec![1,2,3];
MyStruct { length: vector.len(), vector};
}
```

[due to @ronnodas]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924

**Default Value:** `"none"`

---
**Affected lints:**
* [`inconsistent_struct_constructor`](https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor)


## `large-error-threshold`
The maximum size of the `Err`-variant in a `Result` returned from a function

Expand Down
2 changes: 2 additions & 0 deletions clippy.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
avoid-breaking-exported-api = false

initializer-suggestions = "machine-applicable"

[[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"
Expand Down
29 changes: 26 additions & 3 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use crate::ClippyConfiguration;
use crate::types::{
DisallowedPath, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour, Rename, SourceItemOrdering,
SourceItemOrderingCategory, SourceItemOrderingModuleItemGroupings, SourceItemOrderingModuleItemKind,
SourceItemOrderingTraitAssocItemKind, SourceItemOrderingTraitAssocItemKinds,
DisallowedPath, InitializerSuggestionApplicability, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour,
Rename, SourceItemOrdering, SourceItemOrderingCategory, SourceItemOrderingModuleItemGroupings,
SourceItemOrderingModuleItemKind, SourceItemOrderingTraitAssocItemKind, SourceItemOrderingTraitAssocItemKinds,
};
use clippy_utils::msrvs::Msrv;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -526,6 +526,29 @@ define_Conf! {
/// A list of paths to types that should be treated as if they do not contain interior mutability
#[lints(borrow_interior_mutable_const, declare_interior_mutable_const, ifs_same_cond, mutable_key_type)]
ignore_interior_mutability: Vec<String> = Vec::from(["bytes::Bytes".into()]),
/// Suggestion behavior when initializers are present. Options are:
///
/// - "none": do not suggest
/// - "maybe-incorrect": suggest, but do not apply suggestions with `--fix`
/// - "machine-applicable": suggest and apply suggestions with `--fix`
///
/// The following example [due to @ronnodas] shows why "maybe-incorrect" may be the right choice.
/// Swapping the fields in the constructor produces incompilable code:
///
/// ```rust
/// struct MyStruct {
/// vector: Vec<u32>,
/// length: usize
/// }
/// fn main() {
/// let vector = vec![1,2,3];
/// MyStruct { length: vector.len(), vector};
/// }
/// ```
///
/// [due to @ronnodas]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924
#[lints(inconsistent_struct_constructor)]
initializer_suggestions: InitializerSuggestionApplicability = InitializerSuggestionApplicability::None,
/// The maximum size of the `Err`-variant in a `Result` returned from a function
#[lints(result_large_err)]
large_error_threshold: u64 = 128,
Expand Down
19 changes: 19 additions & 0 deletions clippy_config/src/types.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use clippy_utils::def_path_def_ids;
use rustc_errors::Applicability;
use rustc_hir::def_id::DefIdMap;
use rustc_middle::ty::TyCtxt;
use serde::de::{self, Deserializer, Visitor};
Expand Down Expand Up @@ -46,6 +47,24 @@ pub fn create_disallowed_map(
.collect()
}

#[derive(Clone, Copy, Deserialize, Serialize)]
#[serde(rename_all = "kebab-case")]
pub enum InitializerSuggestionApplicability {
None,
MaybeIncorrect,
MachineApplicable,
}

impl InitializerSuggestionApplicability {
pub fn to_applicability(self) -> Option<Applicability> {
match self {
InitializerSuggestionApplicability::None => None,
InitializerSuggestionApplicability::MaybeIncorrect => Some(Applicability::MaybeIncorrect),
InitializerSuggestionApplicability::MachineApplicable => Some(Applicability::MachineApplicable),
}
}
}

#[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, Serialize)]
pub enum MatchLintBehaviour {
AllTypes,
Expand Down
6 changes: 3 additions & 3 deletions clippy_dev/src/fmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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(),
});

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/arbitrary_source_item_ordering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
});
}
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/doc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -770,8 +770,8 @@ fn check_attrs(cx: &LateContext<'_>, valid_idents: &FxHashSet<String>, attrs: &[
parser.into_offset_iter(),
&doc,
Fragments {
fragments: &fragments,
doc: &doc,
fragments: &fragments,
},
))
}
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
};
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/implied_bounds_in_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
114 changes: 81 additions & 33 deletions clippy_lints/src/inconsistent_struct_constructor.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,22 @@
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;
use clippy_utils::source::snippet_opt;
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::Applicability;
use rustc_hir::{self as 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, Write as _};

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
Expand Down Expand Up @@ -59,16 +62,36 @@ declare_clippy_lint! {
#[clippy::version = "1.52.0"]
pub INCONSISTENT_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]);
pub struct InconsistentStructConstructor {
initializer_suggestions: InitializerSuggestionApplicability,
}

impl InconsistentStructConstructor {
pub fn new(conf: &'static Conf) -> Self {
Self {
initializer_suggestions: conf.initializer_suggestions,
}
}
}

impl_lint_pass!(InconsistentStructConstructor => [INCONSISTENT_STRUCT_CONSTRUCTOR]);

impl<'tcx> LateLintPass<'tcx> for InconsistentStructConstructor {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
if let ExprKind::Struct(qpath, fields, base) = expr.kind
&& fields.iter().all(|f| f.is_shorthand)
&& !expr.span.from_expansion()
let ExprKind::Struct(_, fields, _) = expr.kind else {
return;
};
let applicability = if fields.iter().all(|f| f.is_shorthand) {
Applicability::MachineApplicable
} else if let Some(applicability) = self.initializer_suggestions.to_applicability() {
applicability
} else {
return;
};
if !expr.span.from_expansion()
&& let ty = cx.typeck_results().expr_ty(expr)
&& let Some(adt_def) = ty.ty_adt_def()
&& adt_def.is_struct()
Expand All @@ -85,36 +108,19 @@ impl<'tcx> LateLintPass<'tcx> for InconsistentStructConstructor {
return;
}

let mut ordered_fields: Vec<_> = fields.iter().map(|f| f.ident.name).collect();
ordered_fields.sort_unstable_by_key(|id| def_order_map[id]);

let mut 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 = if let Some(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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this use span_lint_and_then and have suggestion() called in the closure? It's a pedantic lint and there's a bit of non trivial work happening in there that could be skipped if the lint isn't enabled

cx,
INCONSISTENT_STRUCT_CONSTRUCTOR,
expr.span,
span,
"struct constructor field order is inconsistent with struct definition field order",
"try",
sugg,
Applicability::MachineApplicable,
applicability,
);
}
}
Expand All @@ -135,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, there could also be comments or cfg'd out fields in ws which could be non-obvious from the binding name, but I don't think it's an issue (in fact, the old code removed them entirely so this is an improvement). But I think we should at least have a test for them

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
};

.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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit: w0_span.between(w1_span)

snippet_opt(cx, span).unwrap()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually use snippet() with some placeholder (" " could work for whitespace), proc macros can give us arbitrarily weird or malformed spans where it could fail that I think it would just be safer to use that here

})
.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
}
}
6 changes: 5 additions & 1 deletion clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,11 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_late_pass(|_| Box::new(implicit_return::ImplicitReturn));
store.register_late_pass(move |_| Box::new(implicit_saturating_sub::ImplicitSaturatingSub::new(conf)));
store.register_late_pass(|_| Box::new(default_numeric_fallback::DefaultNumericFallback));
store.register_late_pass(|_| Box::new(inconsistent_struct_constructor::InconsistentStructConstructor));
store.register_late_pass(move |_| {
Box::new(inconsistent_struct_constructor::InconsistentStructConstructor::new(
conf,
))
});
store.register_late_pass(|_| Box::new(non_octal_unix_permissions::NonOctalUnixPermissions));
store.register_early_pass(|| Box::new(unnecessary_self_imports::UnnecessarySelfImports));
store.register_late_pass(move |_| Box::new(approx_const::ApproxConstant::new(conf)));
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/loops/infinite_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ pub(super) fn check<'tcx>(
cx,
label,
inner_labels: label.into_iter().collect(),
is_finite: false,
loop_depth: 0,
is_finite: false,
};
loop_visitor.visit_block(loop_block);

Expand Down
Loading