Skip to content

New lint: borrow_mutable_copy #13984

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

New lint: borrow_mutable_copy #13984

wants to merge 1 commit into from
+521 −30

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Jan 11, 2025 •
edited
Loading

This detects when the result of a block, with a Copy type, is borrowed mutably. The user might not be aware that using a block will borrow a copy instead of the value expression itself.

changelog: [borrow_mutable_copy]: new lint

Close #13967

@rustbot
Copy link
Collaborator

rustbot commented Jan 11, 2025

r? @blyxyas

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 11, 2025
@samueltardieu samueltardieu force-pushed the push-kpvspntvlrtr branch 2 times, most recently from a881f75 to 1ac5de7 Compare January 12, 2025 08:37
@samueltardieu
Copy link
Contributor Author

Rebased

@samueltardieu
Copy link
Contributor Author

Rebased

@samueltardieu
Copy link
Contributor Author

r? @Clippy

@rustbot rustbot assigned flip1995 and unassigned blyxyas Jan 31, 2025
@samueltardieu
Copy link
Contributor Author

Rebased

@samueltardieu
Copy link
Contributor Author

Rebased

flip1995
flip1995 reviewed Feb 10, 2025
View reviewed changes
@samueltardieu
Copy link
Contributor Author

@flip1995 Anything else needs to be done?

@flip1995
Copy link
Member

I guess the FCP on Zulip. Feel free to start that :)

@samueltardieu
Copy link
Contributor Author

FCP opened on https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/FCP.3A.20copy_then_borrow_mut

@samueltardieu samueltardieu force-pushed the push-kpvspntvlrtr branch 2 times, most recently from 410fe93 to b0e27cc Compare February 28, 2025 14:15
Alexendoo
Alexendoo reviewed Mar 22, 2025
View reviewed changes
@samueltardieu samueltardieu force-pushed the push-kpvspntvlrtr branch 4 times, most recently from eac9c71 to 5309578 Compare March 25, 2025 21:58
@rustbot

This comment has been minimized.

Sign in to view
@samueltardieu
Copy link
Contributor Author

Rebased

@samueltardieu
Copy link
Contributor Author

Any blocker for this one?

@samueltardieu
Copy link
Contributor Author

Rebased and updated the Clippy version to 1.88.

@samueltardieu
Copy link
Contributor Author

Rebased and updated the Clippy version to 1.89.

@rustbot

This comment has been minimized.

Sign in to view
Centri3
Centri3 reviewed May 20, 2025
View reviewed changes
clippy_config/src/conf.rs Outdated
Comment on lines 540 to 545
/// Whether to search for mutable borrows of freshly copied data in tests.
#[lints(copy_then_borrow_mut)]
check_copy_then_borrow_mut_in_tests: bool = true,
Copy link
Member

Choose a reason for hiding this comment

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

Not quite sure how much I agree with a config option on the premise of it being used without examples to show for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have the impression that we don't have any example showing how to set a boolean configuration option to true or false. Or is that not what you mean?

Copy link
Member

Choose a reason for hiding this comment

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

I'm talking about llogiq's reasoning for this config option.

clippy_lints/src/copy_then_borrow_mut.rs Outdated
/// };
/// ```
#[clippy::version = "1.89.0"]
pub COPY_THEN_BORROW_MUT,
Copy link
Member

Choose a reason for hiding this comment

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

I find the name to be very general despite only operating on a very specific case. I don't know what to do here, but it just feels off to me. WDYT? Leave it as-is or nah? Are there any cases this could be expanded to lint in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a better idea, and no better name has been suggested during the FCP. Do you have an idea for a better name?

I'm not sure this lint can be expanded, it is quite specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe mutable_borrow_of_copy? This flows well with #[allow(mutable_borrow_of_copy)].

Copy link
Member

Choose a reason for hiding this comment

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

That sounds pretty good imo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

clippy_lints/src/copy_then_borrow_mut.rs Outdated
expr.span,
"mutable borrow of a value which was just copied",
|diag| {
diag.multipart_suggestion(
Copy link
Member

Choose a reason for hiding this comment

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

I feel like if we're making a suggestion with the idea this is never what the user intended, then this should be deny-by-default, no? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, let's aim for correctness then. We'll see if questions arise during the next beta cycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After thinking more about it, I'd tend to make it suspicious. What if the copied value is used as a template?

I'll have to think more about it.

@rustbot author

Centri3
Centri3 reviewed May 20, 2025
View reviewed changes
@samueltardieu
Copy link
Contributor Author

I'll do two successive pushes: one to rebase on master, with no change, and one with the changes, so that it is easier to check what has been modified.

@samueltardieu samueltardieu force-pushed the push-kpvspntvlrtr branch 3 times, most recently from 03e6eac to ec135bc Compare May 21, 2025 00:45
@samueltardieu samueltardieu changed the title New lint: copy_then_borrow_mut New lint: borrow_mutable_copy May 21, 2025
@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 May 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 21, 2025

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

@samueltardieu
Copy link
Contributor Author

samueltardieu commented May 21, 2025 •
edited
Loading

I've put it back into suspicious, with MaybeIncorrect applicability.

@samueltardieu
Copy link
Contributor Author

@rustbot ready

@rustbot 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 May 21, 2025
@rustbot

This comment has been minimized.

Sign in to view
@samueltardieu
Copy link
Contributor Author

Rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centri3 Centri3 Centri3 left review comments

@flip1995 flip1995 Awaiting requested review from flip1995

@Alexendoo Alexendoo Awaiting requested review from Alexendoo

Assignees

@flip1995 flip1995

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

reborrow or copy-then-borrow
6 participants
@samueltardieu @rustbot @flip1995 @Alexendoo @Centri3 @blyxyas