feat(multiple_inherent_impl): Add config option to target specific scope #15843

paulmialane · 2025-10-08T09:22:32Z

Add a config option inherent-impl-lint-scope to the lint multiple_inherent_impl to target a different scope according to people's needs. It can take three values: module, file, and crate (default).

module is the weakest option. It lints if there are two or more impls in the same module.
file is a bit stronger, since it lints if there are two or more impls in the same file. So, this triggers the lint (where it did not with module):
crate is the strongest of them; it triggers as soon as there are two or more impls anywhere in the crate. It is the current behaviour of the lint, so it's the default option.

changelog: [multiple_inherent_impl] : Add config option (module, file or crate) to target specific scope

fixes #14867

rustbot · 2025-10-08T09:22:38Z

r? @samueltardieu

rustbot has assigned @samueltardieu.
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

samueltardieu

Thanks for your contribution. There are some things that need to be changed in the way the configuration option is parsed (and output in messages) so that lower-case versions "crate", "file", and "module" can be used instead of a mixed-case one.

View changes since this review

clippy_config/src/types.rs

samueltardieu · 2025-10-08T10:21:13Z

clippy_lints/src/inherent_impl.rs

    ///
+    /// The config option controls the scope at which multiple inherent impl blocks for the same
+    /// struct are linted, allowing values of "module" (only within the same module), "file"
+    /// (within the same file), or "crate" (anywhere in the crate, default).


Those lowercase values are not currently recognized (yet), see other comment.

samueltardieu · 2025-10-08T10:47:01Z

tests/ui-cargo/multiple_inherent_impl/crate_fail/src/main.rs

+struct T;
+
+impl S {
+    fn fisrt() {}


Typo? (several occurrences)

samueltardieu

Thanks for the update. Here are a new round of comments, the PR is starting to look good.

View changes since this review

samueltardieu · 2025-10-08T15:23:39Z

clippy_config/src/conf.rs

    /// A list of paths to types that should be treated as if they do not contain interior mutability
    #[lints(borrow_interior_mutable_const, declare_interior_mutable_const, ifs_same_cond, mutable_key_type)]
    ignore_interior_mutability: Vec<String> = Vec::from(["bytes::Bytes".into()]),
+    /// Sets the scope at which duplicate impl blocks for the same type are linted.


Nit

Suggested change

/// Sets the scope at which duplicate impl blocks for the same type are linted.

/// Sets the scope ("crate", "file", or "module") in which duplicate inherent `impl` blocks for the same type are linted.

samueltardieu · 2025-10-08T15:42:38Z

clippy_lints/src/inherent_impl.rs

                    Entry::Vacant(e) => {
                        // Store the id for the first impl block of this type. The span is retrieved lazily.
-                        e.insert(IdOrSpan::Id(impl_id));
+                        e.insert((IdOrSpan::Id(impl_id), criterion.clone()));


There seems to be no need to insert the criterion as part of the value (while it is useful as part of the key). Can't you just keep inserting the IdOrSpan as was done before? Also, this would let you remove the .clone() and write match type_map.entry((impl_ty, criterion)) { above.

samueltardieu · 2025-10-08T15:43:24Z

clippy_lints/src/inherent_impl.rs

                    Entry::Occupied(mut e) => {
                        if let Some(span) = get_impl_span(cx, impl_id) {
-                            let first_span = match *e.get() {
+                            let first_span = match e.get().0 {


You can also restore the dereference once you remove the criterion.

samueltardieu

Could you also please squash the commits?

View changes since this review

samueltardieu · 2025-10-09T08:14:25Z

clippy_lints/src/inherent_impl.rs

+                        if let Node::Item(&Item {
+                            kind: ItemKind::Impl(_impl_id),
+                            span,
+                            ..
+                        }) = cx.tcx.hir_node(hir_id)
+                        {
+                            Criterion::File(cx.tcx.sess.source_map().lookup_source_file(span.lo()).name.clone())
+                        } else {
+                            // We know we are working on an impl, so the pattern matching can
+                            // not fail
+                            unreachable!()
+                        }


Suggested change

if let Node::Item(&Item {

kind: ItemKind::Impl(_impl_id),

span,

..

}) = cx.tcx.hir_node(hir_id)

{

Criterion::File(cx.tcx.sess.source_map().lookup_source_file(span.lo()).name.clone())

} else {

// We know we are working on an impl, so the pattern matching can

// not fail

unreachable!()

}

let span = cx.tcx.hir_span(hir_id);

Criterion::File(cx.tcx.sess.source_map().lookup_source_file(span.lo()).name.clone())

samueltardieu · 2025-10-09T08:20:38Z

clippy_lints/src/inherent_impl.rs

            || is_lint_allowed(
                cx,
                MULTIPLE_INHERENT_IMPL,
                cx.tcx.local_def_id_to_hir_id(id),


I think this will not work if #[expect] is used on the type definition, as the expectation will not be fulfilled. It would probably be better to call clippy_utils::fulfill_or_allowed(…). This must be done only when you know for sure that the lint would be emitted if not allowed or expected.

Could you add some tests with #[expect] to check that this works as expected (ah ah)?

paulmialane · 2025-10-09T11:45:28Z

I need help to understand what's wrong with my test file.

I looked at how #[expect(...)] works, and as suggested I tried using fulfill_or_allowed(). I think I have the desired behaviour, as when I test on my own crate using cargo dev lint ... I have the expected errors.

But, when I use cargo uibless on my new test file:

error: diagnostic code `clippy::unfulfilled_lint_expectations` not found on line 80
  --> tests/ui/impl.rs:81:6
   |
81 | //~^ unfulfilled_lint_expectations
   |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected because of this pattern
   |

error: there were 1 unmatched diagnostics
  --> tests/ui/impl.rs:80:10
   |
80 | #[expect(clippy::multiple_inherent_impl)]
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Error[unfulfilled_lint_expectations]: this lint expectation is unfulfilled

I don't understand why I get this... The way I wrote it is probably not the intended way, but then how?

Thank you!

rustbot assigned samueltardieu Oct 8, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 8, 2025

feat(multiple_inherent_impl): Add config option to target specific scope

b2b0a44

samueltardieu requested changes Oct 8, 2025

View reviewed changes

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 Oct 8, 2025

fix(multiple_inherent_impl): add lowercase config option + fix typo

fa88945

paulmialane requested a review from samueltardieu October 8, 2025 12:58

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 8, 2025

samueltardieu requested changes Oct 8, 2025

View reviewed changes

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 Oct 8, 2025

fix(multiple_inherent_impl): Criterion not needed in the value

f287b42

paulmialane requested a review from samueltardieu October 9, 2025 07:34

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 9, 2025

samueltardieu requested changes Oct 9, 2025

View reviewed changes

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 Oct 9, 2025

WIP: Error using cargo uibless

21e0f33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(multiple_inherent_impl): Add config option to target specific scope #15843

feat(multiple_inherent_impl): Add config option to target specific scope #15843

Uh oh!

paulmialane commented Oct 8, 2025 •

edited by samueltardieu

Loading

Uh oh!

rustbot commented Oct 8, 2025

Uh oh!

samueltardieu left a comment •

edited by rustbot

Loading

Uh oh!

Uh oh!

samueltardieu Oct 8, 2025

Uh oh!

samueltardieu Oct 8, 2025

Uh oh!

samueltardieu left a comment •

edited by rustbot

Loading

Uh oh!

samueltardieu Oct 8, 2025

Uh oh!

samueltardieu Oct 8, 2025

Uh oh!

samueltardieu Oct 8, 2025

Uh oh!

samueltardieu left a comment •

edited by rustbot

Loading

Uh oh!

samueltardieu Oct 9, 2025

Uh oh!

samueltardieu Oct 9, 2025

Uh oh!

paulmialane commented Oct 9, 2025

Uh oh!

Uh oh!

	/// Sets the scope at which duplicate impl blocks for the same type are linted.
	/// Sets the scope ("crate", "file", or "module") in which duplicate inherent `impl` blocks for the same type are linted.

feat(multiple_inherent_impl): Add config option to target specific scope #15843

Are you sure you want to change the base?

feat(multiple_inherent_impl): Add config option to target specific scope #15843

Uh oh!

Conversation

paulmialane commented Oct 8, 2025 • edited by samueltardieu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 8, 2025

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samueltardieu Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

paulmialane commented Oct 9, 2025

Uh oh!

Uh oh!

paulmialane commented Oct 8, 2025 •

edited by samueltardieu

Loading

samueltardieu left a comment •

edited by rustbot

Loading

samueltardieu left a comment •

edited by rustbot

Loading

samueltardieu left a comment •

edited by rustbot

Loading