Make alphabetic ordering in module item groups configurable (new defa…

…ult: off) From feedback to this lint after its inclusion in clippy 1.82, this has turned out to be the most requested improvement. With this improvement, it is possible to make the lint check certain top-level structural checks on modules (e.g. use statements and module inclusions at the top), but still leaving everything else up to the developer.
rust-lang · Jan 16, 2025 · 9f4ba3c · 9f4ba3c
1 parent 8d0c0eb
commit 9f4ba3c
Show file tree

Hide file tree

Showing 20 changed files with 750 additions and 113 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6308,6 +6308,7 @@ Released 2018-09-13
 [`min-ident-chars-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#min-ident-chars-threshold
 [`missing-docs-in-crate-items`]: https://doc.rust-lang.org/clippy/lint_configuration.html#missing-docs-in-crate-items
 [`module-item-order-groupings`]: https://doc.rust-lang.org/clippy/lint_configuration.html#module-item-order-groupings
+[`module-items-ordered-within-groups`]: https://doc.rust-lang.org/clippy/lint_configuration.html#module-items-ordered-within-groups
 [`msrv`]: https://doc.rust-lang.org/clippy/lint_configuration.html#msrv
 [`pass-by-value-size-limit`]: https://doc.rust-lang.org/clippy/lint_configuration.html#pass-by-value-size-limit
 [`pub-underscore-fields-behavior`]: https://doc.rust-lang.org/clippy/lint_configuration.html#pub-underscore-fields-behavior

diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md
@@ -713,6 +713,21 @@ The named groupings of different source item kinds within modules.
 * [`arbitrary_source_item_ordering`](https://rust-lang.github.io/rust-clippy/master/index.html#arbitrary_source_item_ordering)
 
 
+## `module-items-ordered-within-groups`
+Whether the items within module groups should be ordered alphabetically or not.
+
+This option can be configured to "all", "none", or a list of specific grouping names that should be checked
+(e.g. only "enums").
+
+Unknown grouping names will simply be ignored.
+
+**Default Value:** `"none"`
+
+---
+**Affected lints:**
+* [`arbitrary_source_item_ordering`](https://rust-lang.github.io/rust-clippy/master/index.html#arbitrary_source_item_ordering)
+
+
 ## `msrv`
 The minimum rust version that the project supports. Defaults to the `rust-version` field in `Cargo.toml`
 

diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs
@@ -3,6 +3,7 @@ use crate::types::{
     DisallowedPath, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour, Rename, SourceItemOrdering,
     SourceItemOrderingCategory, SourceItemOrderingModuleItemGroupings, SourceItemOrderingModuleItemKind,
     SourceItemOrderingTraitAssocItemKind, SourceItemOrderingTraitAssocItemKinds,
+    SourceItemOrderingWithinModuleItemGroupings,
 };
 use clippy_utils::msrvs::Msrv;
 use rustc_errors::Applicability;
@@ -586,6 +587,15 @@ define_Conf! {
     /// The named groupings of different source item kinds within modules.
     #[lints(arbitrary_source_item_ordering)]
     module_item_order_groupings: SourceItemOrderingModuleItemGroupings = DEFAULT_MODULE_ITEM_ORDERING_GROUPS.into(),
+    /// Whether the items within module groups should be ordered alphabetically or not.
+    ///
+    /// This option can be configured to "all", "none", or a list of specific grouping names that should be checked
+    /// (e.g. only "enums").
+    ///
+    /// Unknown grouping names will simply be ignored.
+    #[lints(arbitrary_source_item_ordering)]
+    module_items_ordered_within_groups: SourceItemOrderingWithinModuleItemGroupings =
+        SourceItemOrderingWithinModuleItemGroupings::None,
     /// The minimum rust version that the project supports. Defaults to the `rust-version` field in `Cargo.toml`
     #[default_text = "current version"]
     #[lints(

diff --git a/clippy_config/src/types.rs b/clippy_config/src/types.rs
@@ -241,6 +241,7 @@ impl SourceItemOrderingModuleItemKind {
 pub struct SourceItemOrderingModuleItemGroupings {
     groups: Vec<(String, Vec<SourceItemOrderingModuleItemKind>)>,
     lut: HashMap<SourceItemOrderingModuleItemKind, usize>,
+    back_lut: HashMap<SourceItemOrderingModuleItemKind, String>,
 }
 
 impl SourceItemOrderingModuleItemGroupings {
@@ -256,6 +257,22 @@ impl SourceItemOrderingModuleItemGroupings {
         lut
     }
 
+    fn build_back_lut(
+        groups: &[(String, Vec<SourceItemOrderingModuleItemKind>)],
+    ) -> HashMap<SourceItemOrderingModuleItemKind, String> {
+        let mut lut = HashMap::new();
+        for (group_name, items) in groups {
+            for item in items {
+                lut.insert(item.clone(), group_name.clone());
+            }
+        }
+        lut
+    }
+
+    pub fn grouping_name_of(&self, item: &SourceItemOrderingModuleItemKind) -> Option<String> {
+        self.back_lut.get(item).cloned()
+    }
+
     pub fn module_level_order_of(&self, item: &SourceItemOrderingModuleItemKind) -> Option<usize> {
         self.lut.get(item).copied()
     }
@@ -266,7 +283,8 @@ impl From<&[(&str, &[SourceItemOrderingModuleItemKind])]> for SourceItemOrdering
         let groups: Vec<(String, Vec<SourceItemOrderingModuleItemKind>)> =
             value.iter().map(|item| (item.0.to_string(), item.1.to_vec())).collect();
         let lut = Self::build_lut(&groups);
-        Self { groups, lut }
+        let back_lut = Self::build_back_lut(&groups);
+        Self { groups, lut, back_lut }
     }
 }
 
@@ -284,6 +302,7 @@ impl<'de> Deserialize<'de> for SourceItemOrderingModuleItemGroupings {
         let groups = Vec::<(String, Vec<SourceItemOrderingModuleItemKind>)>::deserialize(deserializer)?;
         let items_total: usize = groups.iter().map(|(_, v)| v.len()).sum();
         let lut = Self::build_lut(&groups);
+        let back_lut = Self::build_back_lut(&groups);
 
         let mut expected_items = SourceItemOrderingModuleItemKind::all_variants();
         for item in lut.keys() {
@@ -306,7 +325,7 @@ impl<'de> Deserialize<'de> for SourceItemOrderingModuleItemGroupings {
                 ));
             }
 
-            Ok(Self { groups, lut })
+            Ok(Self { groups, lut, back_lut })
         } else if items_total != all_items.len() {
             Err(de::Error::custom(format!(
                 "Some module item kinds were configured more than once, or were missing, in the source ordering configuration. \
@@ -418,6 +437,83 @@ impl Serialize for SourceItemOrderingTraitAssocItemKinds {
     }
 }
 
+/// Describes which specific groupings should have their items ordered
+/// alphabetically.
+///
+/// This is separate from defining and enforcing groupings. For example,
+/// defining enums are grouped before structs still allows for an enum B to be
+/// placed before an enum A. Only when enforcing ordering within the grouping,
+/// will it be checked if A is placed before B.
+#[derive(Clone, Debug)]
+pub enum SourceItemOrderingWithinModuleItemGroupings {
+    /// All groupings should have their items ordered.
+    All,
+
+    /// None of the groupings should have their order checked.
+    None,
+
+    /// Only the specified groupings should have their order checked.
+    Custom(Vec<String>),
+}
+
+impl SourceItemOrderingWithinModuleItemGroupings {
+    pub fn ordered_within(&self, grouping_name: &String) -> bool {
+        match self {
+            SourceItemOrderingWithinModuleItemGroupings::All => true,
+            SourceItemOrderingWithinModuleItemGroupings::None => false,
+            SourceItemOrderingWithinModuleItemGroupings::Custom(groups) => groups.contains(grouping_name),
+        }
+    }
+}
+
+/// Helper struct for deserializing the [`SourceItemOrderingWithinModuleItemGroupings`].
+#[derive(Deserialize)]
+#[serde(untagged)]
+enum StringOrVecOfString {
+    String(String),
+    Vec(Vec<String>),
+}
+
+impl<'de> Deserialize<'de> for SourceItemOrderingWithinModuleItemGroupings {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        let description = "The available options for configuring an ordering within module item groups are: \
+                    \"all\", \"none\", or a list of module item group names \
+                    (as configured with the `module_item_order_groupings` configuration option).";
+
+        match StringOrVecOfString::deserialize(deserializer) {
+            Ok(StringOrVecOfString::String(preset)) if preset == "all" => {
+                Ok(SourceItemOrderingWithinModuleItemGroupings::All)
+            },
+            Ok(StringOrVecOfString::String(preset)) if preset == "none" => {
+                Ok(SourceItemOrderingWithinModuleItemGroupings::None)
+            },
+            Ok(StringOrVecOfString::String(preset)) => Err(de::Error::custom(format!(
+                "Unknown configuration option: {preset}.\n{description}"
+            ))),
+            Ok(StringOrVecOfString::Vec(groupings)) => {
+                Ok(SourceItemOrderingWithinModuleItemGroupings::Custom(groupings))
+            },
+            Err(e) => Err(de::Error::custom(format!("{e}\n{description}"))),
+        }
+    }
+}
+
+impl Serialize for SourceItemOrderingWithinModuleItemGroupings {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: ser::Serializer,
+    {
+        match self {
+            SourceItemOrderingWithinModuleItemGroupings::All => serializer.serialize_str("all"),
+            SourceItemOrderingWithinModuleItemGroupings::None => serializer.serialize_str("none"),
+            SourceItemOrderingWithinModuleItemGroupings::Custom(vec) => vec.serialize(serializer),
+        }
+    }
+}
+
 // these impls are never actually called but are used by the various config options that default to
 // empty lists
 macro_rules! unimplemented_serialize {

diff --git a/clippy_lints/src/arbitrary_source_item_ordering.rs b/clippy_lints/src/arbitrary_source_item_ordering.rs
@@ -2,6 +2,7 @@ use clippy_config::Conf;
 use clippy_config::types::{
     SourceItemOrderingCategory, SourceItemOrderingModuleItemGroupings, SourceItemOrderingModuleItemKind,
     SourceItemOrderingTraitAssocItemKind, SourceItemOrderingTraitAssocItemKinds,
+    SourceItemOrderingWithinModuleItemGroupings,
 };
 use clippy_utils::diagnostics::span_lint_and_note;
 use rustc_hir::{
@@ -58,8 +59,21 @@ declare_clippy_lint! {
     /// | `PascalCase`       | "ty_alias", "opaque_ty", "enum", "struct", "union", "trait", "trait_alias", "impl" |
     /// | `lower_snake_case` | "fn"                 |
     ///
+    /// The groups' names are arbitrary and can be changed to suit the
+    /// conventions that should be enforced for a specific project.
+    ///
     /// All item kinds must be accounted for to create an enforceable linting
-    /// rule set.
+    /// rule set. Following are some example configurations that may be useful.
+    ///
+    /// Example: *module inclusions and use statements to be at the top*
+    ///
+    /// ```toml
+    /// module-item-order-groupings = [
+    ///     [ "modules", [ "extern_crate", "mod", "foreign_mod" ], ],
+    ///     [ "use", [ "use", ], ],
+    ///     [ "everything_else", [ "macro", "global_asm", "static", "const", "ty_alias", "enum", "struct", "union", "trait", "trait_alias", "impl", "fn", ], ],
+    /// ]
+    /// ```
     ///
     /// ### Known Problems
     ///
@@ -144,6 +158,7 @@ pub struct ArbitrarySourceItemOrdering {
     enable_ordering_for_struct: bool,
     enable_ordering_for_trait: bool,
     module_item_order_groupings: SourceItemOrderingModuleItemGroupings,
+    module_items_ordered_within_groups: SourceItemOrderingWithinModuleItemGroupings,
 }
 
 impl ArbitrarySourceItemOrdering {
@@ -158,6 +173,7 @@ impl ArbitrarySourceItemOrdering {
             enable_ordering_for_struct: conf.source_item_ordering.contains(&Struct),
             enable_ordering_for_trait: conf.source_item_ordering.contains(&Trait),
             module_item_order_groupings: conf.module_item_order_groupings.clone(),
+            module_items_ordered_within_groups: conf.module_items_ordered_within_groups.clone(),
         }
     }
 
@@ -192,7 +208,7 @@ impl ArbitrarySourceItemOrdering {
         );
     }
 
-    fn lint_member_item<T: LintContext>(cx: &T, item: &Item<'_>, before_item: &Item<'_>) {
+    fn lint_member_item<T: LintContext>(cx: &T, item: &Item<'_>, before_item: &Item<'_>, msg: &'static str) {
         let span = if item.ident.as_str().is_empty() {
             &item.span
         } else {
@@ -216,14 +232,7 @@ impl ArbitrarySourceItemOrdering {
             return;
         }
 
-        span_lint_and_note(
-            cx,
-            ARBITRARY_SOURCE_ITEM_ORDERING,
-            *span,
-            "incorrect ordering of items (must be alphabetically ordered)",
-            Some(*before_span),
-            note,
-        );
+        span_lint_and_note(cx, ARBITRARY_SOURCE_ITEM_ORDERING, *span, msg, Some(*before_span), note);
     }
 
     /// Produces a linting warning for incorrectly ordered trait items.
@@ -403,6 +412,7 @@ impl<'tcx> LateLintPass<'tcx> for ArbitrarySourceItemOrdering {
             }
 
             let item_kind = convert_module_item_kind(&item.kind);
+            let grouping_name = self.module_item_order_groupings.grouping_name_of(&item_kind);
             let module_level_order = self
                 .module_item_order_groupings
                 .module_level_order_of(&item_kind)
@@ -412,13 +422,27 @@ impl<'tcx> LateLintPass<'tcx> for ArbitrarySourceItemOrdering {
                 use std::cmp::Ordering; // Better legibility.
                 match module_level_order.cmp(&cur_t.order) {
                     Ordering::Less => {
-                        Self::lint_member_item(cx, item, cur_t.item);
+                        Self::lint_member_item(
+                            cx,
+                            item,
+                            cur_t.item,
+                            "incorrect ordering of items (module item groupings specify another order)",
+                        );
                     },
                     Ordering::Equal if item_kind == SourceItemOrderingModuleItemKind::Use => {
                         // Skip ordering use statements, as these should be ordered by rustfmt.
                     },
-                    Ordering::Equal if cur_t.name > get_item_name(item) => {
-                        Self::lint_member_item(cx, item, cur_t.item);
+                    Ordering::Equal
+                        if (grouping_name.is_some_and(|grouping_name| {
+                            self.module_items_ordered_within_groups.ordered_within(&grouping_name)
+                        }) && cur_t.name > get_item_name(item)) =>
+                    {
+                        Self::lint_member_item(
+                            cx,
+                            item,
+                            cur_t.item,
+                            "incorrect ordering of items (must be alphabetically ordered)",
+                        );
                     },
                     Ordering::Equal | Ordering::Greater => {
                         // Nothing to do in this case, they're already in the right order.

diff --git a/tests/ui-toml/arbitrary_source_item_ordering/bad_conf_4/clippy.toml b/tests/ui-toml/arbitrary_source_item_ordering/bad_conf_4/clippy.toml
@@ -0,0 +1 @@
+module-items-ordered-within-groups = true
diff --git a/tests/ui-toml/arbitrary_source_item_ordering/default_exp/clippy.toml b/tests/ui-toml/arbitrary_source_item_ordering/default_exp/clippy.toml
@@ -9,4 +9,4 @@ module-item-order-groupings = [
     ["PascalCase", ["ty_alias", "enum", "struct", "union", "trait", "trait_alias", "impl"]],
     ["lower_snake_case", ["fn"]]
 ]
-
+module-items-ordered-within-groups = "none"
diff --git a/tests/ui-toml/arbitrary_source_item_ordering/ord_in_2/clippy.toml b/tests/ui-toml/arbitrary_source_item_ordering/ord_in_2/clippy.toml
@@ -0,0 +1 @@
+module-items-ordered-within-groups = ["PascalCase", "ignored unknown grouping"]
diff --git a/tests/ui-toml/arbitrary_source_item_ordering/ord_within/clippy.toml b/tests/ui-toml/arbitrary_source_item_ordering/ord_within/clippy.toml
@@ -0,0 +1 @@
+module-items-ordered-within-groups = "all"
diff --git a/tests/ui-toml/arbitrary_source_item_ordering/ordering_good.bad_conf_4.stderr b/tests/ui-toml/arbitrary_source_item_ordering/ordering_good.bad_conf_4.stderr
@@ -0,0 +1,8 @@
+error: error reading Clippy's configuration file: data did not match any variant of untagged enum StringOrVecOfString  The available options for configuring an ordering within module item groups are: "all", "none", or a list of module item group names (as configured with the `module_item_order_groupings` configuration option).
+  --> $DIR/tests/ui-toml/arbitrary_source_item_ordering/bad_conf_4/clippy.toml:1:38
+   |
+LL | module-items-ordered-within-groups = true
+   |                                      ^^^^
+
+error: aborting due to 1 previous error
+
diff --git a/tests/ui-toml/arbitrary_source_item_ordering/ordering_good.rs b/tests/ui-toml/arbitrary_source_item_ordering/ordering_good.rs
@@ -1,10 +1,11 @@
 //@aux-build:../../ui/auxiliary/proc_macros.rs
-//@revisions: default default_exp bad_conf_1 bad_conf_2 bad_conf_3
+//@revisions: default default_exp bad_conf_1 bad_conf_2 bad_conf_3 bad_conf_4
 //@[default] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/arbitrary_source_item_ordering/default
 //@[default_exp] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/arbitrary_source_item_ordering/default_exp
 //@[bad_conf_1] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/arbitrary_source_item_ordering/bad_conf_1
 //@[bad_conf_2] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/arbitrary_source_item_ordering/bad_conf_2
 //@[bad_conf_3] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/arbitrary_source_item_ordering/bad_conf_3
+//@[bad_conf_4] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/arbitrary_source_item_ordering/bad_conf_4
 
 #![allow(dead_code)]
 #![warn(clippy::arbitrary_source_item_ordering)]
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		module-items-ordered-within-groups = ["PascalCase", "ignored unknown grouping"]