Skip to content
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

Avoid emitting assigning_clones when cloned data borrows from the place to clone into #12756

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

y21
Copy link
Member

@y21 y21 commented May 2, 2024

Fixes #12444
Fixes #12460
Fixes #12749
Fixes #12757

I think the documentation for the function should describe what- and how this is fixing the issues well.
It avoids emitting a warning when the data being cloned borrows from the place to clone into, which is information that we can get from PossibleBorrowerMap. Unfortunately, it is a tiny bit tedious to match on the MIR like that and I'm not sure if this is possibly relying a bit too much on the exact MIR lowering for assignments.

Things left to do:

  • Handle place projections (or verify that they work as expected)
  • Handle non-Drop types

changelog: [assigning_clones]: avoid warning when the suggestion would lead to a borrow-check error

@rustbot
Copy link
Collaborator

rustbot commented May 2, 2024

r? @Alexendoo

rustbot has assigned @Alexendoo.
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 May 2, 2024
@y21 y21 force-pushed the assigning_clones_lifetimes branch from a413cd1 to 0a93106 Compare May 2, 2024 23:51
Comment on lines +289 to +317
let mut s = (NoDrop, NoDrop);
let s2 = &s.1;
s.0 = s2.clone();
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a false negative now but fixing it seems like it'd require large changes to PossibleBorrowerMap to teach it disjoint borrows, but I suppose this FN might be better than a FP

Copy link
Contributor

Choose a reason for hiding this comment

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

In a sense, a performance lint is about reducing time wasted by the computer, but as human time is more expensive, it's surely more costly to waste human time if no computing performance is likely to be gained in the process.

@bors
Copy link
Collaborator

bors commented May 9, 2024

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

@y21 y21 force-pushed the assigning_clones_lifetimes branch from 0a93106 to 60508f5 Compare May 9, 2024 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment