Skip to content

Add new lint: cloned_ref_to_slice_refs #14284

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

Merged
merged 1 commit into from
May 13, 2025

Conversation

asdfish
Copy link
Contributor

@asdfish asdfish commented Feb 23, 2025

Added lint for catching &[foo.clone()] where foo is a reference and suggests std::slice::from_ref(foo).

changelog: new lint: [cloned_ref_to_slice_refs]

@rustbot
Copy link
Collaborator

rustbot commented Feb 23, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @blyxyas (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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 23, 2025
@asdfish asdfish changed the title Added new lint: cloned_refs_to_slice_refs Add new lint: cloned_refs_to_slice_refs Feb 23, 2025
Copy link
Contributor

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

Thanks for your PR. Here is a first cursory review.

@blyxyas
Copy link
Member

blyxyas commented Mar 14, 2025

I'll have to reroll this one, I'm having capacity problems right now.

r? clippy

@rustbot rustbot assigned llogiq and unassigned blyxyas Mar 14, 2025
@asdfish asdfish changed the title Add new lint: cloned_refs_to_slice_refs Add new lint: cloned_ref_to_slice_refs Mar 16, 2025
Copy link
Contributor

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

The following code will trigger a false positive, as the suggested fix would take a reference on x which would be alive when x is pushed to:

fn main() {
    let mut x = String::from("Hello");
    let r = &[x.clone()];
    x.push('!');
    println!("r = `{}', x = `{x}'", r[0]);
}

The lint should trigger only when the receiver of .clone() is immutable, as it is reasonable to assume that .clone() would build an identical copy from a practical point of view.

Also, the commits should be (squashed and) rebased onto master, as some changes in the MSRV handling have happened since and the code won't compile as-is.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 16, 2025
@rustbot

This comment has been minimized.

@asdfish asdfish marked this pull request as draft March 16, 2025 21:58
@asdfish asdfish force-pushed the cloned_refs_to_slice_refs branch from 8b075f9 to 5f67d09 Compare March 16, 2025 22:13
@asdfish asdfish marked this pull request as ready for review March 17, 2025 11:27
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 17, 2025
@rustbot

This comment has been minimized.

@asdfish asdfish force-pushed the cloned_refs_to_slice_refs branch from 06eeaf5 to e5163a3 Compare April 20, 2025 15:27
@llogiq
Copy link
Contributor

llogiq commented May 12, 2025

Can you please squash your commits? Also I think there are utility functions to detect interior mutability, which would allow us to remove this particular false-positive. But I'm ok with doing that in a followup PR.

remove merge

removed false positive and improved tests

clarify known problems for `cloned_ref_to_slice_refs`
@asdfish asdfish force-pushed the cloned_refs_to_slice_refs branch from e5163a3 to 40e1b0e Compare May 13, 2025 01:13
@asdfish
Copy link
Contributor Author

asdfish commented May 13, 2025

I'm unsure if the lint should even differenciate between clones that have interior mutability.

These two do the same thing.

fn increment(list: &[Arc<Mutex<i32>>]) {
    list.iter().for_each(|i| *i.lock().unwrap() += 1);
}

{
    let x = Arc::new(Mutex::new(1));
    let list = std::slice::from_ref(&x);
    increment(list);
}
{
    let x = Arc::new(Mutex::new(1));
    let list = &[x.clone()]; // same thing but with reference counting overhead
    increment(list);
}

@llogiq
Copy link
Contributor

llogiq commented May 13, 2025

I am completely unconcerned about Rc or Arc here, more about the possibility that a custom type with interior mutability might be used and modified later so that the clone becomes observable because unless cloned, the original value gets modified.

@llogiq
Copy link
Contributor

llogiq commented May 13, 2025

That said, let's merge this, we can do interior mutability detection in a follow-up PR.

@llogiq llogiq added this pull request to the merge queue May 13, 2025
Merged via the queue into rust-lang:master with commit 7bac114 May 13, 2025
11 checks passed
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.

5 participants