arbitrary_source_item_ordering: add configurable trait impl item ordering modes

[wasd243]

Add configuration options for trait impl item ordering

Implements three-mode configuration for arbitrary_source_item_ordering:

• alphabetical (default, preserves current behavior)
• trait_item_ordering (follow trait definition order)
• alphabetical_or_trait_item_ordering (accept either ordering)

This allows users to choose the ordering style that best fits their codebase.
Traits like IntoIterator with non-alphabetical definition orders can now be
handled gracefully without false positives.

The default behavior is unchanged, maintaining backward compatibility.

changelog: [arbitrary_source_item_ordering]: add configurable trait impl item ordering modes

Closes #17241

[rustbot]

Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @samueltardieu (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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 8 candidates
• 8 candidates expanded to 8 candidates
• Random selection from Jarcho, llogiq, samueltardieu

[github-actions]

No changes for 04dafef

[samueltardieu]

This PR changes the behavior of the lint, which is rarely a good idea.

I think this should be controlled by an option. People might not want to depend on the order defined by the trait author, especially when it can change between versions without even changing the API. There is no guarantee that, even in the standard library, traits will internally keep the existing ordering of items.

This option could have three possible values: alphabetical (current behavior), trait_item_ordering (the behavior implemented by this PR), or alphabetical_or_trait_item_ordering to accept both the current behavior and the ordering of the trait items (the names may not be the best ones, they are examples).

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[wasd243]

Thanks for the detailed feedback. I've implemented the three-option config system as suggested.

Changes

1. New configuration option

Added trait-impl-item-order with three values: alphabetical (default, current behavior), trait_item_ordering, and alphabetical_or_trait_item_ordering.

In types.rs:

/// Represents the ordering requirement for associated items in trait impls.
#[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, Serialize)]
#[serde(rename_all = "snake_case")]
pub enum TraitImplItemOrder {
    Alphabetical,
    TraitItemOrdering,
    AlphabeticalOrTraitItemOrdering,
}

In conf.rs, with Alphabetical as the default to keep the current behavior unchanged:

#[lints(arbitrary_source_item_ordering)]
trait_impl_item_order: TraitImplItemOrder = TraitImplItemOrder::Alphabetical,

Usage in clippy.toml:

trait-impl-item-order = "alphabetical_or_trait_item_ordering"

2. Lint logic

Added three helper functions in arbitrary_source_item_ordering.rs, one per mode:

fn check_impl_alphabetical(&self, cx: &LateContext<'_>, trait_impl: &rustc_hir::Impl<'_>) { ... }

fn check_impl_trait_def_order(&self, cx: &LateContext<'_>, trait_impl: &rustc_hir::Impl<'_>) { ... }

fn check_impl_alphabetical_or_trait_def_order(&self, cx: &LateContext<'_>, trait_impl: &rustc_hir::Impl<'_>) { ... }

The Impl arm in check_item dispatches on the configured value:

ItemKind::Impl(trait_impl) if self.enable_ordering_for_impl => match self.trait_impl_item_order {
    TraitImplItemOrder::Alphabetical => {
        self.check_impl_alphabetical(cx, trait_impl);
    },
    TraitImplItemOrder::TraitItemOrdering => {
        self.check_impl_trait_def_order(cx, trait_impl);
    },
    TraitImplItemOrder::AlphabeticalOrTraitItemOrdering => {
        self.check_impl_alphabetical_or_trait_def_order(cx, trait_impl);
    },
},

For impl blocks without a trait, the two new modes fall back to the alphabetical check.

3. Tests & documentation

• Added test cases under tests/ui-toml/arbitrary_source_item_ordering/ in the trait_definition_order/ and alphabetical_or_trait_item_order/ directories, covering both accepted and linted orderings per mode.
• Restored the pre-existing tests to their upstream state, since the default behavior is unchanged.
• Regenerated the configuration docs with cargo bless --test config-metadata.
• CI is green.

Happy to rename the option or its values if you prefer different naming.

@rustbot ready

[wasd243]

@rustbot review

Squash and rebased commits.
All requested changes implemented. Ready for review.

[rustbot]

This lint has been nominated for inclusion.

A FCP topic has been created on Zulip.

[samueltardieu]

Could you please add more tests with the trait_item_ordering and alphabetical_or_trait_item_ordering options in case of a trait with default function implementations? I want to make sure that the user can skip some of the functions in the middle and not be warned about that, as it keeps the order consistent.

View changes since this review

[samueltardieu]

And please, update the PR title and description to acknowledge that this is no longer a fix but a new feature, as pointed out in the Zulip thread (as pointed out by @kpreid).

[wasd243]

Thank you for reviewing. I've added test cases to make sure that trait_item_ordering
and alphabetical_or_trait_item_ordering don't warn users when they skip trait items.

Implemented test cases

In both mode test folders (trait_definition_order/ and alphabetical_or_trait_item_order/),
added trait-skipping tests such as:

// Default trait impl ordering.
trait WithDefault {
    fn a() {}
    fn b() {}
    fn c() {}
}

struct SkipMiddle;

impl WithDefault for SkipMiddle {
    fn a() {}
    fn c() {}
}

Also added coverage for const items in traits:

trait ConstTrait2 {
    const A: bool = true;
    const B: bool = false;
    const C: bool = true;
    fn method() {}
}

struct SkipImpl;

// Tests skipped `const` in trait.
impl ConstTrait2 for SkipImpl {
    const A: bool = true;
    const C: bool = true;
    fn method() {}
}

Both passing (@check-pass) and failing cases are covered for each scenario.

@rustbot ready

[wasd243]

@rustbot review

Squash and rebased commits, renamed the title and PR description, ready for review.