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.

Sign up for GitHub

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

arbitrary_source_item_ordering: Make alphabetic ordering in module item groups optional #13718

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

arbitrary_source_item_ordering: Make alphabetic ordering in module item groups optional #13718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6211,6 +6211,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
Expand Down
10 changes: 10 additions & 0 deletions book/src/lint_configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,16 @@ 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`
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense for this configuration to accept an array of group names similar to how source-item-ordering lets you configure it for specific kinds of items and default to an empty array? Or do you think that's not needed and a simple boolean is enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was mostly thinking that the config is getting convoluted in either case, but I fully agree that it would make sense to go for higher granularity here. I would like to have an option for "all", but would need to investigate how to best express that in TOML.

Preferrably I'd have the options of:

module-items-ordered-within-groups = "none" (being the default)
module-items-ordered-within-groups = ["mod", "use", "my_custom_group", ...]
module-items-ordered-within-groups = "all"

I'll put some thought into this in the coming days.

Whether the items within module groups should be ordered alphabetically or not.

**Default Value:** `false`

---
**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`

Expand Down
3 changes: 3 additions & 0 deletions clippy_config/src/conf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,9 @@ 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.
#[lints(arbitrary_source_item_ordering)]
module_items_ordered_within_groups: bool = false,
/// The minimum rust version that the project supports. Defaults to the `rust-version` field in `Cargo.toml`
#[default_text = "current version"]
#[lints(
Expand Down
21 changes: 19 additions & 2 deletions clippy_lints/src/arbitrary_source_item_ordering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,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
///
Expand Down Expand Up @@ -144,6 +157,7 @@ pub struct ArbitrarySourceItemOrdering {
enable_ordering_for_struct: bool,
enable_ordering_for_trait: bool,
module_item_order_groupings: SourceItemOrderingModuleItemGroupings,
module_items_ordered_within_groups: bool,
}

impl ArbitrarySourceItemOrdering {
Expand All @@ -158,6 +172,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,
}
}

Expand Down Expand Up @@ -417,7 +432,9 @@ impl<'tcx> LateLintPass<'tcx> for ArbitrarySourceItemOrdering {
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) => {
Ordering::Equal
if (self.module_items_ordered_within_groups && cur_t.name > get_item_name(item)) =>
{
Self::lint_member_item(cx, item, cur_t.item);
},
Ordering::Equal | Ordering::Greater => {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui-toml/arbitrary_source_item_ordering/default_exp/clippy.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = false
1 change: 1 addition & 0 deletions tests/ui-toml/arbitrary_source_item_ordering/ord_within/clippy.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module-items-ordered-within-groups = true
120 changes: 27 additions & 93 deletions tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.default.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,226 +1,160 @@
error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:21:14
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:22:14
|
LL | use std::rc::Weak;
| ^^^^
|
note: should be placed before `SNAKE_CASE`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:19:7
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:20:7
|
LL | const SNAKE_CASE: &str = "zzzzzzzz";
| ^^^^^^^^^^
= note: `-D clippy::arbitrary-source-item-ordering` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::arbitrary_source_item_ordering)]`

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:64:1
|
LL | / impl CloneSelf for StructOrdered {
LL | | fn clone_self(&self) -> Self {
LL | | Self {
LL | | a: true,
... |
LL | | }
LL | | }
| |_^
|
note: should be placed before the following item
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:54:1
|
LL | / impl Default for StructOrdered {
LL | | fn default() -> Self {
LL | | Self {
LL | | a: true,
... |
LL | | }
LL | | }
| |_^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:136:7
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:137:7
|
LL | const ZIS_SHOULD_BE_REALLY_EARLY: () = ();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: should be placed before `TraitUnorderedItemKinds`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:124:7
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:125:7
|
LL | trait TraitUnorderedItemKinds {
| ^^^^^^^^^^^^^^^^^^^^^^^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:151:1
|
LL | impl BasicEmptyTrait for StructOrdered {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: should be placed before the following item
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:138:1
|
LL | / impl TraitUnordered for StructUnordered {
LL | | const A: bool = false;
LL | | const C: bool = false;
LL | | const B: bool = false;
... |
LL | | fn b() {}
LL | | }
| |_^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:170:5
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:171:5
|
LL | mod this_is_in_the_wrong_position {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: should be placed before `main`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:165:4
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:166:4
|
LL | fn main() {
| ^^^^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:178:7
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:179:7
|
LL | const ZIS_SHOULD_BE_EVEN_EARLIER: () = ();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: should be placed before `ZisShouldBeBeforeZeMainFn`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:176:8
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:177:8
|
LL | struct ZisShouldBeBeforeZeMainFn;
| ^^^^^^^^^^^^^^^^^^^^^^^^^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:12:11
|
LL | const AFTER: i8 = 0;
| ^^^^^
|
note: should be placed before `BEFORE`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:10:11
|
LL | const BEFORE: i8 = 0;
| ^^^^^^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:38:5
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:39:5
|
LL | B,
| ^
|
note: should be placed before `C`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:37:5
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:38:5
|
LL | C,
| ^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:88:5
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:89:5
|
LL | b: bool,
| ^
|
note: should be placed before `c`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:87:5
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:88:5
|
LL | c: bool,
| ^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:96:5
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:97:5
|
LL | b: bool,
| ^
|
note: should be placed before `c`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:95:5
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:96:5
|
LL | c: bool,
| ^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:115:11
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:116:11
|
LL | const B: bool;
| ^
|
note: should be placed before `C`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:114:11
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:115:11
|
LL | const C: bool;
| ^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:121:8
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:122:8
|
LL | fn b();
| ^
|
note: should be placed before `c`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:120:8
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:121:8
|
LL | fn c();
| ^

error: incorrect ordering of trait items (defined order: [Const, Type, Fn])
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:127:5
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:128:5
|
LL | const A: bool;
| ^^^^^^^^^^^^^^
|
note: should be placed before `SomeType`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:125:5
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:126:5
|
LL | type SomeType;
| ^^^^^^^^^^^^^^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:141:11
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:142:11
|
LL | const B: bool = false;
| ^
|
note: should be placed before `C`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:140:11
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:141:11
|
LL | const C: bool = false;
| ^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:147:8
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:148:8
|
LL | fn b() {}
| ^
|
note: should be placed before `c`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:146:8
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:147:8
|
LL | fn c() {}
| ^

error: incorrect ordering of impl items (defined order: [Const, Type, Fn])
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:156:5
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:157:5
|
LL | const A: bool = false;
| ^^^^^^^^^^^^^^^^^^^^^^
|
note: should be placed before `SomeType`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:154:5
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:155:5
|
LL | type SomeType = ();
| ^^^^^^^^^^^^^^^^^^^

error: incorrect ordering of items (must be alphabetically ordered)
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:172:11
|
LL | const A: i8 = 1;
| ^
|
note: should be placed before `C`
--> tests/ui-toml/arbitrary_source_item_ordering/ordering_mixed.rs:171:11
|
LL | const C: i8 = 0;
| ^

error: aborting due to 17 previous errors
error: aborting due to 13 previous errors

Loading