Auto merge of #6769 - Y-Nak:inconsistent-struct-constructor, r=matthi…

…askrgr

Inconsistent struct constructor

fixes: #6352
r? `@matthiaskrgr`

I added the lint that checks for the struct constructors where the order of the field init shorthands is inconsistent with that in the struct definition.

changelog: Add style lint: `inconsistent_struct_constructor`

CHANGELOG.md

@@ -2109,6 +2109,7 @@ Released 2018-09-13
 [`implicit_saturating_sub`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_sub
 [`imprecise_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#imprecise_flops
 [`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping
+[`inconsistent_struct_constructor`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor
 [`indexing_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing
 [`ineffective_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#ineffective_bit_mask
 [`inefficient_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#inefficient_to_string

clippy_lints/src/inconsistent_struct_constructor.rs

@@ -0,0 +1,134 @@
+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, declare_tool_lint};
+use rustc_span::symbol::Symbol;
+
+use if_chain::if_chain;
+
+use crate::utils::{snippet, span_lint_and_sugg};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for struct constructors where the order of the field init
+    /// shorthand 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
+    /// resulted instance as the below example indicates,
+    ///
+    /// ```rust
+    /// #[derive(Debug, PartialEq, Eq)]
+    /// struct Foo {
+    ///     x: i32,
+    ///     y: i32,
+    /// }
+    /// let x = 1;
+    /// let y = 2;
+    ///
+    /// // This assertion never fails.
+    /// assert_eq!(Foo { x, y }, Foo { y, x });
+    /// ```
+    ///
+    /// inconsistent order means nothing and just decreases readability and consistency.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// struct Foo {
+    ///     x: i32,
+    ///     y: i32,
+    /// }
+    /// let x = 1;
+    /// let y = 2;
+    /// Foo { y, x };
+    /// ```
+    ///
+    /// Use instead:
+    /// ```rust
+    /// # struct Foo {
+    /// #     x: i32,
+    /// #     y: i32,
+    /// # }
+    /// # let x = 1;
+    /// # let y = 2;
+    /// Foo { x, y };
+    /// ```
+    pub INCONSISTENT_STRUCT_CONSTRUCTOR,
+    style,
+    "the order of the field init shorthand is inconsistent with the order in the struct definition"
+}
+
+declare_lint_pass!(InconsistentStructConstructor => [INCONSISTENT_STRUCT_CONSTRUCTOR]);
+
+impl LateLintPass<'_> for InconsistentStructConstructor {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
+        if_chain! {
+            if let ExprKind::Struct(qpath, fields, base) = expr.kind;
+            if let Some(def_id)  = cx.qpath_res(qpath, expr.hir_id).opt_def_id();
+            let ty = cx.tcx.type_of(def_id);
+            if let Some(adt_def) = ty.ty_adt_def();
+            if adt_def.is_struct();
+            if let Some(variant) = adt_def.variants.iter().next();
+            if fields.iter().all(|f| f.is_shorthand);
+            then {
+                let mut def_order_map = FxHashMap::default();
+                for (idx, field) in variant.fields.iter().enumerate() {
+                    def_order_map.insert(field.ident.name, idx);
+                }
+
+                if is_consistent_order(fields, &def_order_map) {
+                    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 {
+                    fields_snippet.push_str(&format!("{}, ", 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!("{} {{ {}{} }}",
+                    snippet(cx, qpath.span(), ".."),
+                    fields_snippet,
+                    base_snippet,
+                    );
+
+                span_lint_and_sugg(
+                    cx,
+                    INCONSISTENT_STRUCT_CONSTRUCTOR,
+                    expr.span,
+                    "inconsistent struct constructor",
+                    "try",
+                    sugg,
+                    Applicability::MachineApplicable,
+                )
+            }
+        }
+    }
+}
+
+// Check whether the order of the fields in the constructor is consistent with the order in the
+// definition.
+fn is_consistent_order<'tcx>(fields: &'tcx [hir::Field<'tcx>], def_order_map: &FxHashMap<Symbol, usize>) -> bool {
+    let mut cur_idx = usize::MIN;
+    for f in fields {
+        let next_idx = def_order_map[&f.ident.name];
+        if cur_idx > next_idx {
+            return false;
+        }
+        cur_idx = next_idx;
+    }
+
+    true
+}

clippy_lints/src/lib.rs

@@ -221,6 +221,7 @@ mod if_let_some_result;
 mod if_not_else;
 mod implicit_return;
 mod implicit_saturating_sub;
+mod inconsistent_struct_constructor;
 mod indexing_slicing;
 mod infinite_iter;
 mod inherent_impl;
@@ -656,6 +657,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &if_not_else::IF_NOT_ELSE,
         &implicit_return::IMPLICIT_RETURN,
         &implicit_saturating_sub::IMPLICIT_SATURATING_SUB,
+        &inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR,
         &indexing_slicing::INDEXING_SLICING,
         &indexing_slicing::OUT_OF_BOUNDS_INDEXING,
         &infinite_iter::INFINITE_ITER,
@@ -1036,6 +1038,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box implicit_return::ImplicitReturn);
     store.register_late_pass(|| box implicit_saturating_sub::ImplicitSaturatingSub);
     store.register_late_pass(|| box default_numeric_fallback::DefaultNumericFallback);
+    store.register_late_pass(|| box inconsistent_struct_constructor::InconsistentStructConstructor);
 
     let msrv = conf.msrv.as_ref().and_then(|s| {
         parse_msrv(s, None, None).or_else(|| {
@@ -1486,6 +1489,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&identity_op::IDENTITY_OP),
         LintId::of(&if_let_mutex::IF_LET_MUTEX),
         LintId::of(&if_let_some_result::IF_LET_SOME_RESULT),
+        LintId::of(&inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR),
         LintId::of(&indexing_slicing::OUT_OF_BOUNDS_INDEXING),
         LintId::of(&infinite_iter::INFINITE_ITER),
         LintId::of(&inherent_to_string::INHERENT_TO_STRING),
@@ -1737,6 +1741,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&functions::MUST_USE_UNIT),
         LintId::of(&functions::RESULT_UNIT_ERR),
         LintId::of(&if_let_some_result::IF_LET_SOME_RESULT),
+        LintId::of(&inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR),
         LintId::of(&inherent_to_string::INHERENT_TO_STRING),
         LintId::of(&len_zero::COMPARISON_TO_EMPTY),
         LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY),

clippy_lints/src/unnecessary_sort_by.rs

@@ -212,10 +212,10 @@ fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> {
             if !expr_borrows(cx, left_expr) {
                 return Some(LintTrigger::SortByKey(SortByKeyDetection {
                     vec_name,
-                    unstable,
                     closure_arg,
                     closure_body,
-                    reverse
+                    reverse,
+                    unstable,
                 }));
             }
         }

tests/ui/inconsistent_struct_constructor.fixed

@@ -0,0 +1,61 @@
+// run-rustfix
+// edition:2018
+#![warn(clippy::inconsistent_struct_constructor)]
+#![allow(clippy::redundant_field_names)]
+#![allow(clippy::unnecessary_operation)]
+#![allow(clippy::no_effect)]
+#![allow(dead_code)]
+
+#[derive(Default)]
+struct Foo {
+    x: i32,
+    y: i32,
+    z: i32,
+}
+
+mod without_base {
+    use super::Foo;
+
+    fn test() {
+        let x = 1;
+        let y = 1;
+        let z = 1;
+
+        // Should lint.
+        Foo { x, y, z };
+
+        // Shoule NOT lint because the order is the same as in the definition.
+        Foo { x, y, z };
+
+        // Should NOT lint because z is not a shorthand init.
+        Foo { y, x, z: z };
+    }
+}
+
+mod with_base {
+    use super::Foo;
+
+    fn test() {
+        let x = 1;
+        let z = 1;
+
+        // Should lint.
+        Foo { x, z, ..Default::default() };
+
+        // Should NOT lint because the order is consistent with the definition.
+        Foo {
+            x,
+            z,
+            ..Default::default()
+        };
+
+        // Should NOT lint because z is not a shorthand init.
+        Foo {
+            z: z,
+            x,
+            ..Default::default()
+        };
+    }
+}
+
+fn main() {}

tests/ui/inconsistent_struct_constructor.rs

@@ -0,0 +1,65 @@
+// run-rustfix
+// edition:2018
+#![warn(clippy::inconsistent_struct_constructor)]
+#![allow(clippy::redundant_field_names)]
+#![allow(clippy::unnecessary_operation)]
+#![allow(clippy::no_effect)]
+#![allow(dead_code)]
+
+#[derive(Default)]
+struct Foo {
+    x: i32,
+    y: i32,
+    z: i32,
+}
+
+mod without_base {
+    use super::Foo;
+
+    fn test() {
+        let x = 1;
+        let y = 1;
+        let z = 1;
+
+        // Should lint.
+        Foo { y, x, z };
+
+        // Shoule NOT lint because the order is the same as in the definition.
+        Foo { x, y, z };
+
+        // Should NOT lint because z is not a shorthand init.
+        Foo { y, x, z: z };
+    }
+}
+
+mod with_base {
+    use super::Foo;
+
+    fn test() {
+        let x = 1;
+        let z = 1;
+
+        // Should lint.
+        Foo {
+            z,
+            x,
+            ..Default::default()
+        };
+
+        // Should NOT lint because the order is consistent with the definition.
+        Foo {
+            x,
+            z,
+            ..Default::default()
+        };
+
+        // Should NOT lint because z is not a shorthand init.
+        Foo {
+            z: z,
+            x,
+            ..Default::default()
+        };
+    }
+}
+
+fn main() {}

tests/ui/inconsistent_struct_constructor.stderr

@@ -0,0 +1,20 @@
+error: inconsistent struct constructor
+  --> $DIR/inconsistent_struct_constructor.rs:25:9
+   |
+LL |         Foo { y, x, z };
+   |         ^^^^^^^^^^^^^^^ help: try: `Foo { x, y, z }`
+   |
+   = note: `-D clippy::inconsistent-struct-constructor` implied by `-D warnings`
+
+error: inconsistent struct constructor
+  --> $DIR/inconsistent_struct_constructor.rs:43:9
+   |
+LL | /         Foo {
+LL | |             z,
+LL | |             x,
+LL | |             ..Default::default()
+LL | |         };
+   | |_________^ help: try: `Foo { x, z, ..Default::default() }`
+
+error: aborting due to 2 previous errors
+