Lint suggestion: `duplicate_impl_blocks`

[amab8901]

What it does

It suggests you to refactor the code so you don't have redundant impl <Struct> blocks, and so that you have all the methods in one place (for that scope).

I would prefer to have the lint to apply to only impl blocks that are in the same scope, rather than globally across the program (because I like to have different impl blocks of the same struct in different files so that the method defined in the impl block is closest to the place where it's used).

If the file is so large that one would consider multiple impl blocks just to keep each method closer to where it's used, then it's probably a sign that the file is too large and needs to be split into multiple smaller files rather than having multiple impl blocks in the same file for the same struct.

Advantage

• reduce redundant occurrences of impl <Struct> to improve signal/noise ratio
• keep all the functions in one place (among those impl blocks defined in the same scope), so you can more easily/quickly/confidently know that you have found all the methods defined in this scope (usually the main scope of the file) and that there are no more methods to search for elsewhere in that scope (usually the main scope of the file)

Drawbacks

idk what drawbacks there would be besides the trivial ones like "it would increase the amount of code in clippy and it would take time/energy to implement this lint"

Example

fn main() {
    let structy = Structy::default();

    dbg!(&structy.field1());
    dbg!(&structy.field2());
}

#[derive(Default)]
pub(crate) struct Structy {
    field1: String,
    field2: String,
}

impl Structy {
    pub(crate) fn field1(&self) -> &String {
        &self.field1
    }
}

impl Structy {
    pub(crate) fn field2(&self) -> &String {
        &self.field2
    }
}

Could be written as:

fn main() {
    let structy = Structy::default();

    dbg!(&structy.field1());
    dbg!(&structy.field2());
}

#[derive(Default)]
pub(crate) struct Structy {
    field1: String,
    field2: String,
}

impl Structy {
    pub(crate) fn field1(&self) -> &String {
        &self.field1
    }

    pub(crate) fn field2(&self) -> &String {
        &self.field2
    }
}

[Jarcho]

This already exists as multiple_inherent_impl. I think that will lint across modules, but a config option would work to solve that.

[amab8901]

Even though multiple_inherent_impl exists (which I recently learned about), this lint doesn't get applied, despite me having pedantic lint level. Why isn't this lint being applied?

Here is my full lint config:

[lints.clippy]
let_and_return = "allow"
module_name_repetitions = "allow"
similar_names = "allow"
must_use_candidate = "allow"
missing_inline_in_public_items = "allow"
std_instead_of_core = "allow"
mod_module_files = "allow"
self_named_module_files = "allow"
too_many_arguments = "allow"
missing_errors_doc = "allow"
missing_panics_doc = "allow"

needless_pass_by_ref_mut = "warn"
pedantic = { level = "warn", priority = -1 }

[Jarcho]

It's in the restriction category, not pedantic.

[ismailarilik]

A related closed issue from 2022: #7131

[ismailarilik]

Allowing multiple impls across files and also modules might be a good idea (Is it?). Let me check if this is something I will be (capable of) working on.

Anyone who enabled this lint might want multiple impls in different files and also different modules. A rationale for this may look like this:

Assume that there is a struct named Book. We can just define it inside book.rs and leave impls to other files in the same module. For example; we may have these files:

• book.rs (For the Book struct)
• impl_book_to_read.rs (For the methods for reading one page, reading sections, reading whole book, etc.)
• impl_book_to_share.rs (For the methods for getting a share link, reading together, etc.)
• impl_book_to_review.rs (For the methods for reviewers to review content, add comments, maybe fix, etc.)

These files which have impl blocks may have a generic module. We may also have a module named (maybe) voice and those files may employ this module:

• impl_book_to_vocalize.rs (For the methods to vocalize, listen together, etc.)
• impl_book_to_pronounce.rs (For the methods to differentiate different pronounces, vocalize them, etc.)

So there are some use cases of having multiple impls in different files or modules, which make sense in the context of single responsibility.

The first thing that comes to my mind is having a config named allow_for. The default value should be []. Possible values may be files and modules. What do you think?

[ismailarilik]

I have found the implementation of this lint in inherent_impl.rs#L47. As I understood, this method is getting all impls for all types and marks the duplicates. To constrain it, I need file and module info for these impls but I couldn't find when I searched for it so I decided to ask here.

Is there anyone who could guide me?

Note: Asked also in #clippy > How to get file and module info of an impl? @ 💬.

[amab8901]

I have found the implementation of this lint in inherent_impl.rs#L47. As I understood, this method is getting all impls for all types and marks the duplicates. To constrain it, I need file and module info for these impls but I couldn't find when I searched for it so I decided to ask here.

Is there anyone who could guide me?

Note: Asked also in #clippy > How to get file and module info of an impl? @ 💬.

if u don't find anyone to guide you, one approach you could use is to just create a unit test and then tinker around with the code until the unit test passes. Then you refactor your code to be the most readable/efficient/optimized/etc without breaking cargo t. You can also run it through copilot to see if it gives you any ideas for how to make it better or if there's something u missed (but don't take its words at face value. Copilot is just throwing things on the wall to see what sticks). Then you send PR and people will prob point out if you missed something or if there's a better approach

[samueltardieu]

The first thing that comes to my mind is having a config named allow_for. The default value should be []. Possible values may be files and modules. What do you think?

I'm not convinced that distinguishing between files is useful. A module may be extracted into its own file, or grouped with others in a file, it still is the same module.

[amab8901]

I'm not convinced that distinguishing between files is useful. A module may be extracted into its own file, or grouped with others in a file, it still is the same module.

I agree. Every .rs file (except perhaps main.rs and lib.rs) are going to be modules automatically if they are part of the program. So by distinguishing between modules (and treating main.rs and lib.rs as if they were modules) would automatically result in files being distinguished