Skip to content

Add fn_param_ref_cloned lint - #17281

Open
medzernik wants to merge 17 commits into
rust-lang:masterfrom
medzernik:2074-clone-on-ref
Open

Add fn_param_ref_cloned lint#17281
medzernik wants to merge 17 commits into
rust-lang:masterfrom
medzernik:2074-clone-on-ref

Conversation

@medzernik

@medzernik medzernik commented Jun 20, 2026 •
edited
Loading

Copy link
Copy Markdown

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 rustbot added needs-fcp PRs that add, remove, or rename lints and need an FCP S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 20, 2026
@rustbot

rustbot commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

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

@medzernik
medzernik force-pushed the 2074-clone-on-ref branch from 3e17354 to fb1530b Compare June 20, 2026 19:07
@github-actions

github-actions Bot commented Jun 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Lintcheck changes for 32e80ba

Lint Added Removed Changed
clippy::fn_param_ref_cloned 133 0 0

This comment will be updated if you push new changes

@medzernik
medzernik force-pushed the 2074-clone-on-ref branch from fb1530b to cf78d0d Compare June 21, 2026 13:48
@rustbot

This comment has been minimized.

Sign in to view
@medzernik
medzernik force-pushed the 2074-clone-on-ref branch from cf78d0d to aa82ebb Compare June 21, 2026 13:49
llogiq
llogiq reviewed Jun 25, 2026
View reviewed changes

@llogiq llogiq left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

View changes since this review

Comment thread tests/ui/fn_param_ref_cloned.rs Outdated
@medzernik

medzernik commented Jun 26, 2026

Copy link
Copy Markdown
Author

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

@rustbot

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@medzernik

medzernik commented Aug 1, 2026

Copy link
Copy Markdown
Author

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

@rustbot

This comment has been minimized.

Sign in to view
@medzernik

medzernik commented Aug 7, 2026

Copy link
Copy Markdown
Author

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 :>

@medzernik
medzernik requested a review from llogiq August 7, 2026 16:27
@rustbot

This comment has been minimized.

Sign in to view
medzernik and others added 9 commits August 9, 2026 18:28
@medzernik
fix unnested_or_patterns according to iss 2074
7082d37 
Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com>
@medzernik
add fn_param_ref_cloned lint
c691d19 
Co-Authored by: Matej Almasi <matej.almasi@protonmail.com>
Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com>

# Conflicts:
#	clippy_lints/src/lib.rs
@medzernik
Update CHANGELOG.md
8a1b10f 
Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com>
@medzernik
Delete tutorial residual .stderr
cca5751 
Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com>
@medzernik
change to pedantic and qualify <>::default()
f3f8146 
Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com>

# Conflicts:
#	clippy_lints/src/lib.rs
@medzernik
Use check_fn instead of check_body
1370a2f 
Signed-off-by: medzernik <medzernik@medzernik.dev>
@medzernik
only lint immutable refs
6ce262b 
Signed-off-by: medzernik <medzernik@medzernik.dev>
@medzernik
skip linting closures
dd42a2b 
Signed-off-by: medzernik <medzernik@medzernik.dev>
@medzernik
skip if and match blocks
77718fd 
Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com>
@rustbot

rustbot commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

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.

@medzernik
revert bad rebase resolution..
80cc7dd 
Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com>
@medzernik
update lint to new for_each_expr
80e2e3a 
Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com>
@medzernik
anonymously use import of trait for sideeffects
32e80ba 
Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com>
blyxyas
blyxyas requested changes Aug 11, 2026
View reviewed changes

@blyxyas blyxyas left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread clippy_lints/src/fn_param_ref_cloned.rs
Comment on lines +37 to +45
type CandidateId = rustc_hir::HirId;
type CandidateSpan = Span;
type Candidate = (CandidateId, CandidateSpan);
type CandidateRebinds = Vec<Candidate>;

#[derive(Default)]
pub struct FnParamRefCloned {
candidates: Vec<(Candidate, CandidateRebinds)>,
}

@blyxyas blyxyas Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that this is adding a lot of cognitive load without much benefit (I get that it's more "readable", but it takes much more time to parse for the first time, and we only use this types here)

Comment thread clippy_lints/src/fn_param_ref_cloned.rs
Comment thread clippy_lints/src/fn_param_ref_cloned.rs

#[derive(Default)]
pub struct FnParamRefCloned {
candidates: Vec<(Candidate, CandidateRebinds)>,

@blyxyas blyxyas Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the median length of this array(s)? Could we use smallvec instead?

Comment thread clippy_lints/src/fn_param_ref_cloned.rs Outdated
Comment thread clippy_lints/src/fn_param_ref_cloned.rs
Comment on lines +85 to +88
let must_impl_trait = [
cx.tcx.lang_items().clone_trait().unwrap(),
cx.tcx.lang_items().drop_trait().unwrap(),
];

@blyxyas blyxyas Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good move here!

Comment thread clippy_lints/src/fn_param_ref_cloned.rs Outdated
Comment thread clippy_lints/src/fn_param_ref_cloned.rs Outdated
Comment thread clippy_lints/src/fn_param_ref_cloned.rs
ControlFlow::<(), Descend>::Continue(Descend::No)
},
rustc_hir::ExprKind::MethodCall(method_name, receiver, args, span)
if method_name.ident.as_str() == "clone"

@blyxyas blyxyas Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we use is_diagnostic_item here?

Comment thread clippy_lints/src/fn_param_ref_cloned.rs
.collect();

// Find all rebinds of param values in the function and add them to the original candidates (tuple)
if let rustc_hir::ExprKind::Block(block, _) = fn_body.value.kind {

@blyxyas blyxyas Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Refactor this from an if let into a Block(block, _) = fn_body.value.kind else { return; }

Comment thread clippy_lints/src/fn_param_ref_cloned.rs
Comment on lines +141 to +149
clippy_utils::diagnostics::span_lint_and_note(
cx,
FN_PARAM_REF_CLONED,
span,
"function gets a parameter by reference, but you later unconditionally clone it",
Some(original_candidate.1),
"consider passing the reference by value instead",
);
}

@blyxyas blyxyas Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please refactor these two calls into another function, called something like emit_lint

@rustbot 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 Aug 11, 2026
@rustbot

rustbot commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

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

@rustbot

rustbot commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (possibly #17538) made this pull request unmergeable. Please resolve the merge conflicts.

medzernik and others added 4 commits August 13, 2026 17:56
@medzernik @blyxyas
Doc comment apply from @blyxyas
99295a7 
Co-authored-by: Alejandra González <blyxyas@goose.love>
@medzernik @blyxyas
Simplify logic by @blyxyas
9ef2905 
Co-authored-by: Alejandra González <blyxyas@goose.love>
@medzernik @blyxyas
eliminate single-letter var names in lambda by @blyxyas
db8d50e 
Co-authored-by: Alejandra González <blyxyas@goose.love>
@medzernik @blyxyas
improve name for lambda param by @blyxyas
74d7fe9 
Co-authored-by: Alejandra González <blyxyas@goose.love>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blyxyas blyxyas blyxyas requested changes
@llogiq llogiq Awaiting requested review from llogiq

Requested changes must be addressed to merge this pull request.

Assignees

@llogiq llogiq

Labels

needs-fcp PRs that add, remove, or rename lints and need an FCP S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New lint: clone on arguments taken by reference

4 participants

@medzernik @rustbot @llogiq @blyxyas