Skip to content

add impl-ordering-except-fn-names config field for arbitrary_source_item_ordering #14782

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

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

Conversation

relaxcn
Copy link
Contributor

@relaxcn relaxcn commented May 11, 2025

I think it makes sense to set up a list of function names to exclude certain functions in the impl block from being checked.

Some functions, such as new, are typically used as constructors—either as the first item or in other positions—so we need a configuration to exclude them.

I added a config filed impl-ordering-except-fn-names(Vec<String>) to define the list of function names that should be excluded.

This modification does not change the existing behavior of Clippy; it only takes effect after being explicitly configured.

changelog: [arbitrary_source_item_ordering]: add impl-ordering-except-fn-names config field to exclude some functions in impl blocks.

Close: #14739

@rustbot
Copy link
Collaborator

rustbot commented May 11, 2025

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 11, 2025
@Jarcho
Copy link
Contributor

Jarcho commented May 13, 2025

The config for this lint is currently quite awkward and in need of being redone and I'd rather not add more to it until that's done. I have a few ideas as to how I would change it to make it easier to use and I'll see if I can get to it soon.

If I don't have anything within a week or so can you ping me and I can merge this first instead?

Comment on lines +335 to +343
// If there are exception function names, skip it.
if !self.impl_ordering_except_fn_names.is_empty()
&& convert_assoc_item_kind(item.kind) == SourceItemOrderingTraitAssocItemKind::Fn
&& self
.impl_ordering_except_fn_names
.contains(&item.ident.name.to_string())
{
continue;
}
Copy link
Contributor

@Jarcho Jarcho May 13, 2025

Choose a reason for hiding this comment

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

This happens too early as type sorting hasn't happened yet.

You'll have to move the kind order check on line 353 to happen before the name check at line 351 and then insert this in between.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is only to skip the function names that we don't want to sort.
I don't want to care about if it is one of the two compared objects, I just skip it.
If I change the code to this:

                for item in trait_impl.items {
                    if item.span.in_external_macro(cx.sess().source_map()) {
                        continue;
                    }
                    if let Some(cur_t) = cur_t {
                        let item_kind = convert_assoc_item_kind(item.kind);
                        let item_kind_index = self.assoc_types_order.index_of(&item_kind);

                        let cur_t_kind = convert_assoc_item_kind(cur_t.kind);
                        let cur_t_kind_index = self.assoc_types_order.index_of(&cur_t_kind);

                        // If there are exception function names, skip it.
                        if !self.impl_ordering_except_fn_names.is_empty()
                            && convert_assoc_item_kind(item.kind) == SourceItemOrderingTraitAssocItemKind::Fn
                            && self
                                .impl_ordering_except_fn_names
                                .contains(&item.ident.name.to_string())
                        {
                            continue;
                        }
                        
                        if cur_t_kind == item_kind && cur_t.ident.name.as_str() > item.ident.name.as_str() {
                            Self::lint_member_name(cx, &item.ident, &cur_t.ident);
                        } else if cur_t_kind_index > item_kind_index {
                            self.lint_impl_item(cx, item, cur_t);
                        }
                    }
                    cur_t = Some(item);
                }

Then you will find a uitest error when you run command TESTNAME="ordering_exceptions" cargo uitest.

impl BasicStruct1 {
    fn new() -> Self {
        Self {}
    }
    fn a() {}
    fn b() {}
}
error: there were 1 unmatched diagnostics
  --> tests/ui-toml/arbitrary_source_item_ordering/ordering_exceptions.rs:14:8
   |
14 |     fn a() {}
   |        ^ Error[clippy::arbitrary_source_item_ordering]: incorrect ordering of items (must be alphabetically ordered)
   |

The first function will always participate in the sorting.

@relaxcn
Copy link
Contributor Author

relaxcn commented May 13, 2025

The config for this lint is currently quite awkward and in need of being redone and I'd rather not add more to it until that's done. I have a few ideas as to how I would change it to make it easier to use and I'll see if I can get to it soon.

If I don't have anything within a week or so can you ping me and I can merge this first instead?

Ok, you are right.

@relaxcn
Copy link
Contributor Author

relaxcn commented May 20, 2025

Just a kind ping @Jarcho.

I think you are right. If you are ready to rewrite this lint, I could close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arbitrary_source_item_ordering should allow sorting constructors first
3 participants