rust-clippy icon indicating copy to clipboard operation
rust-clippy copied to clipboard

new lint: `source_item_ordering`

Open decryphe opened this issue 1 year ago • 6 comments

changelog: [source_item_ordering]: Introduced a new restriction lint that checks the ordering of items in Modules, Enums, Structs, Impls and Traits.

From the written documentation:

Why restrict this? Keeping a consistent ordering throughout the codebase helps with working as a team, and possibly improves maintainability of the codebase. The idea is that by defining a consistent and enforceable rule for how source files are structured, less time will be wasted during reviews on a topic that is (under most circumstances) not relevant to the logic implemented in the code. Sometimes this will be referred to as "bike-shedding".

Keep in mind, that ordering source code alphabetically can lead to reduced performance in cases where the most commonly used enum variant isn't the first entry anymore, and similar optimizations that can reduce branch misses, cache locality and such. Either don't use this lint if that's relevant, or disable the lint in modules or items specifically where it matters. Other solutions can be to use profile guided optimization (PGO), or other advanced optimization methods.

I tried to build it as configurable as possible, as such a highly opinionated lint should be adjustable to personal opinions.

I'm open to any input and will be available both here and on the zulip for communication. In the meantime I'll be testing this lint against my own code-bases, which I've (manually) kept ordered with the default config, to see how well it works in practice.

And lastly, a big thanks to the community for making clippy the best linter there is!

decryphe avatar Sep 10 '24 09:09 decryphe

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot avatar Sep 10 '24 09:09 rustbot

@rustbot author

llogiq avatar Sep 21 '24 07:09 llogiq

:umbrella: The latest upstream changes (presumably #13322) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Sep 22 '24 14:09 bors

I'd like to discuss whether or not I should implement another loop before generating lint output that determines the actual location where a snippet should be placed. Considering the kind-of-weird looking output asking for placement of items before derive macro entries, it's probably worthwhile to do now.

decryphe avatar Oct 01 '24 08:10 decryphe

:umbrella: The latest upstream changes (presumably #13334) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Oct 13 '24 07:10 bors

:umbrella: The latest upstream changes (presumably #13395) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Oct 15 '24 16:10 bors

Sorry you had to wait so long, but personal matters left me little time to review this rather large change.

I'd like to check if the clippy_version is still right and this needs a rebase (and feel free to squash), otherwise r=me.

llogiq avatar Oct 27 '24 10:10 llogiq

Thank you!

@bors r+

llogiq avatar Nov 02 '24 11:11 llogiq

:pushpin: Commit f7ab2c990889b5f0b539cadbb808763d992b790e has been approved by llogiq

It is now in the queue for this repository.

bors avatar Nov 02 '24 11:11 bors

:hourglass: Testing commit f7ab2c990889b5f0b539cadbb808763d992b790e with merge 5c6fe68c006b83eeef0f7f0213cf04c7865f7585...

bors avatar Nov 02 '24 11:11 bors

:sunny: Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test Approved by: llogiq Pushing 5c6fe68c006b83eeef0f7f0213cf04c7865f7585 to master...

bors avatar Nov 02 '24 11:11 bors

I was going to open a new issue, but wanted to confirm once whether or not this is a bug: This lint stops doing anything if I set: source-item-ordering = [] in clippy.toml My use case is that I want to enforce ordering of different item types within a module, but I don't want to enforce any alphabetical ordering within any group of items. In other words, I want to enforce ["const", "type", "fn"] within a trait, but I don't care about alphabetical ordering of consts or types or fns.

utkarshgupta137 avatar Jan 13 '25 13:01 utkarshgupta137

I was going to open a new issue, but wanted to confirm once whether or not this is a bug: This lint stops doing anything if I set: source-item-ordering = [] in clippy.toml My use case is that I want to enforce ordering of different item types within a module, but I don't want to enforce any alphabetical ordering within any group of items. In other words, I want to enforce ["const", "type", "fn"] within a trait, but I don't care about alphabetical ordering of consts or types or fns.

Yes, that disables all checks. I didn't find the time I hoped during Christmas and New Years to work on the features discussed in #13718 - I hope to start working on this soon.

decryphe avatar Jan 13 '25 14:01 decryphe

@utkarshgupta137 I updated the PR mentioned, but re-reading your question, it's a different topic. Your use-case would need yet another configuration option - I'll do some thinking of how to adjust the configurability without making it even more complicated.

decryphe avatar Jan 16 '25 14:01 decryphe