Add fn_param_ref_cloned lint

[medzernik]

View all comments

Co-Authored by: @matej-almasi almasi.mato@gmail.com

changelog: [fn_param_ref_cloned]: added a lint. Fixes #2074

This is a partial WIP PR - We need some feedback (this is also our first contribution).

---

Added a lint to check whether you are not cloning a reference when passed into a function. Per best practices the caller should decide whether to clone data, and not hide the operation within a function.

[rustbot]

Thanks for the pull request, and welcome! The Rust Project 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

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, dswij, llogiq, samueltardieu

[github-actions]

Lintcheck changes for 9193dc8

Lint	Added	Removed	Changed
clippy::fn_param_ref_cloned	133	0	0

This comment will be updated if you push new changes

[llogiq]

Can we have a test with (externally) macro-generated code, too?

View changes since this review

[medzernik]

Can we have a test with (externally) macro-generated code, too?

View changes since this review

I think that for now I do ignore all expansions from macros (though that probably only involves locally expanded span from macros). However, I would be happy to add some testcases for external macros - tho maybe I would need some direction (example) somewhere to see how it's supposed to be done

[medzernik]

After a longer time off, I will try to fixup this PR and see if tests pass properly, then hopefully can finish off remaining suggestions

[medzernik]

I want to rebase all the commits again later just so there are not 13 of them, but I wanted to ask for a re-review now that some time has passed. Would it be OK if we kept the lint smaller-scoped for now, and I could focus on extending it later? Do you think the scope is sufficient for it to be useful? Thanks a bunch!

cc @blyxyas just for the info, I would love as many opinions as I can get :>

[blyxyas]

Great place to start, let's start by some refactoring. Once that everything's a bit cleaner, we can judge the lint better!

View changes since this review

[blyxyas]

Could we use is_diagnostic_item here?

[medzernik]

rustc_hir::ExprKind::MethodCall(name, receiver, args, span)
    if args.is_empty()
        && let rustc_hir::ExprKind::Path(qpath) = receiver.kind
        && let Some(hir_id) = qpath.res_local_id()
        && clippy_utils::is_lang_item_or_ctor(cx, hir_id.owner.to_def_id(), LangItem::Clone) =>

I also tried this:

if args.is_empty()
    && let rustc_hir::ExprKind::Path(qpath) = receiver.kind
    && let Some(hir_id) = qpath.res_local_id()
    && let def_id = cx.tcx.hir_owner_node(cx.tcx.hir_get_parent_item(expr.hir_id))
    && clippy_utils::is_lang_item_or_ctor(cx, def_id.def_id().to_def_id(), LangItem::Clone) =>

I tried something like this (experimented with name.hir_id.owner, and more... however I am not able quite pinpoint the approach, even from examples in other lints. May I ask for some extra direction?

[blyxyas]

What I'm talking about is, this code would turn out true with the following code:

#[derive(Clone)]
struct A;

struct B;

trait MyClone {
    fn clone(&self) -> Self;
}

impl MyClone for B {
// ...
}

pub fn foo(item: &A) { // <- We save &A as candidate
    let x: B = B;
    x.clone(); // <- Technically called "clone"
}

Now, if I'm reading the emit lint function correctly, seems that if the HirId of the clone call does not refer to item, this does not actually lint. We should move that check upwards

[rustbot]

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

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.