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

new lint: source_item_ordering #13376

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

Conversation

decryphe
Copy link

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!

@rustbot
Copy link
Collaborator

rustbot commented Sep 10, 2024

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 rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 10, 2024
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

The big open question is around macro-generated code. Otherwise, apart from a few small doc improvements, this looks good.

clippy_lints/src/arbitrary_source_item_ordering.rs Outdated Show resolved Hide resolved
/// implemented in the code. Sometimes this will be referred to as
/// "bike-shedding".
///
/// Keep in mind, that ordering source code alphabetically can lead to
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be under a # Known problems header.

We also might want to add that the lint doesn't supply suggestions because it's quite hard to produce them in a correct way.

Copy link
Author

Choose a reason for hiding this comment

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

Moved it down to the (now renamed) chapter "Known Problems". Is there a standard way to write this?

Ctrl+F "# Known" yields different ways to write this: "Known problems", "Known Problems", "Known issues"

const B: i8 = 1;

const A: i8 = 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see some macro-generated items. I'm open to debate whether the lint should apply to them.

Copy link
Author

Choose a reason for hiding this comment

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

I've updated the uitests to contain some derive macros. What other things should be tested?
I also added a uitest that passes the ordering lint and produces no output.

Copy link
Author

Choose a reason for hiding this comment

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

Also, note that I've added ordering for trait impls on the module level.
I will add at least a test where I reconfigure item placement of trait impls into its own section, such that it's certainly possible to split trait defs and trait impls. The default groups all CamelCase items, including impls, but I can see why somebody would like to split this category up.

Copy link
Author

Choose a reason for hiding this comment

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

Note: I've added a bunch of test cases, including modified configurations.

Copy link
Author

Choose a reason for hiding this comment

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

Turns out that it can't reasonably be applied to macro-generated items, there's a ton of things in a real codebase that generates items. Using the in_external_macro() function from clippy_utils to filter items yields a decent behaviour in a real code-base.
Also, turns out we do adhere surprisingly well to our own convention, even though we do so manually. Some typos and forgotten things, but nothing majorly wrong in our source file ordering. :-)

I believe this is now in a reviewable state and should work as advertised.

@llogiq
Copy link
Contributor

llogiq commented Sep 21, 2024

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 21, 2024
@bors
Copy link
Collaborator

bors commented Sep 22, 2024

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

@decryphe decryphe force-pushed the source-ordering branch 2 times, most recently from 3953233 to b46da06 Compare September 27, 2024 14:42
@decryphe
Copy link
Author

decryphe commented Oct 1, 2024

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 decryphe force-pushed the source-ordering branch 3 times, most recently from 1aa0157 to 8af8a22 Compare October 11, 2024 17:08
@bors
Copy link
Collaborator

bors commented Oct 13, 2024

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

@bors
Copy link
Collaborator

bors commented Oct 15, 2024

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

@llogiq
Copy link
Contributor

llogiq commented Oct 27, 2024

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.

///
/// [cargo-pgo]: https://github.com/Kobzol/cargo-pgo/blob/main/README.md
///
#[clippy::version = "1.81.0"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[clippy::version = "1.81.0"]
#[clippy::version = "1.82.0"]

Copy link
Author

Choose a reason for hiding this comment

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

Updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants