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

Solve a error .clone() suggestion when moving a mutable reference #127579

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

surechen
Copy link
Contributor

If the moved value is a mut reference, it is used in a generic function and it's type is a generic param, suggest it can be reborrowed to avoid moving.

for example:

struct Y(u32);
// x's type is '& mut Y' and it is used in `fn generic<T>(x: T) {}`.
fn generic<T>(x: T) {}

fixes #127285

@rustbot
Copy link
Collaborator

rustbot commented Jul 10, 2024

r? @lcnr

rustbot has assigned @lcnr.
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 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 10, 2024
@rust-log-analyzer

This comment has been minimized.

@surechen surechen changed the title Solve error .clone() suggestion when moving a mutable reference [WIP] Solve error .clone() suggestion when moving a mutable reference Jul 10, 2024
@surechen surechen changed the title [WIP] Solve error .clone() suggestion when moving a mutable reference [WIP] Solve a error .clone() suggestion when moving a mutable reference Jul 11, 2024
@surechen surechen changed the title [WIP] Solve a error .clone() suggestion when moving a mutable reference Solve a error .clone() suggestion when moving a mutable reference Jul 11, 2024
@@ -445,13 +442,36 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
} else {
(None, &[][..], 0)
};
let mut reborrow = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut reborrow = false;
let mut suggest_reborrow = false;

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. Thank you.

Comment on lines 447 to 480
&& let node = self.infcx.tcx.hir_node_by_def_id(def_id)
&& let Some(fn_sig) = node.fn_sig()
&& let Some(ident) = node.ident()
&& let Some(pos) = args.iter().position(|arg| arg.hir_id == expr.hir_id)
&& let Some(arg) = fn_sig.decl.inputs.get(pos + offset)
Copy link
Contributor

Choose a reason for hiding this comment

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

this only works for functions from the current crate, greatly lowering the usefulness of this new diagnostics.

You should instead be able to use tcx.fn_sig(def_id).skip_binder().skip_binder().inputs()[pos + offset].is_param()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this only works for functions from the current crate, greatly lowering the usefulness of this new diagnostics.

You should instead be able to use tcx.fn_sig(def_id).skip_binder().skip_binder().inputs()[pos + offset].is_param()

Done. Thank you.
Because is_param in

pub fn is_param(self, index: u32) -> bool {
needs a arg index which I'm not sure what means, I don't use this function.

false
};
reborrow = is_sugg_reborrow();

let mut span: MultiSpan = arg.span.into();
span.push_span_label(
Copy link
Contributor

Choose a reason for hiding this comment

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

also I feel like we should not emit the "consider changing this parameter type in function generic to borrow instead if owning the value isn't necessary" note if a reborrow would work. so either suggest a reborrow or suggest changing the fn_sig, but not both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also I feel like we should not emit the "consider changing this parameter type in function generic to borrow instead if owning the value isn't necessary" note if a reborrow would work. so either suggest a reborrow or suggest changing the fn_sig, but not both

Done. Thank you.

@surechen surechen force-pushed the fix_127285 branch 2 times, most recently from 2b60569 to 1f78e60 Compare July 11, 2024 15:45
@surechen surechen requested a review from lcnr July 11, 2024 15:49
@rust-log-analyzer

This comment has been minimized.

@surechen surechen changed the title Solve a error .clone() suggestion when moving a mutable reference [WIP] Solve a error .clone() suggestion when moving a mutable reference Jul 12, 2024
@bors
Copy link
Contributor

bors commented Jul 12, 2024

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

@surechen surechen changed the title [WIP] Solve a error .clone() suggestion when moving a mutable reference Solve a error .clone() suggestion when moving a mutable reference Jul 12, 2024
@bors
Copy link
Contributor

bors commented Jul 16, 2024

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

@surechen
Copy link
Contributor Author

Hello, I have made modifications, please review for me again. I received a conflict notification and I would like to resolve the conflict after approved.
Thank you @lcnr

}
false
};
*has_suggest_reborrow = is_sugg_reborrow();
Copy link
Contributor

Choose a reason for hiding this comment

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

why use a closure here instead of inlining the code below into return true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why use a closure here instead of inlining the code below into return true?

Thank you. I just think that because it's in the function suggest_ref_or_clone function, this judgment code checks a special situation and returns early, so maybe this check code could be organized into a closure. Should I abandon the closure here?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, abandon the closure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, abandon the closure

Done. Thank you.

…on and it's type is a generic param, it can be reborrowed to avoid moving.

for example:

```rust
struct Y(u32);
// x's type is '& mut Y' and it is used in `fn generic<T>(x: T) {}`.
fn generic<T>(x: T) {}
```

fixes rust-lang#127285
@lcnr
Copy link
Contributor

lcnr commented Jul 17, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 17, 2024

📌 Commit 4821b84 has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125042 (Use ordinal number in argument error)
 - rust-lang#127229 (rustdoc: click target for sidebar items flush left)
 - rust-lang#127337 (Move a few intrinsics to Rust abi)
 - rust-lang#127472 (MIR building: Stop using `unpack!` for `BlockAnd<()>`)
 - rust-lang#127579 (Solve a error `.clone()` suggestion when moving a mutable reference)
 - rust-lang#127769 (Don't use implicit features in `Cargo.toml` in `compiler/`)
 - rust-lang#127844 (Remove invalid further restricting suggestion for type bound)
 - rust-lang#127855 (Add myself to review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125042 (Use ordinal number in argument error)
 - rust-lang#127229 (rustdoc: click target for sidebar items flush left)
 - rust-lang#127337 (Move a few intrinsics to Rust abi)
 - rust-lang#127472 (MIR building: Stop using `unpack!` for `BlockAnd<()>`)
 - rust-lang#127579 (Solve a error `.clone()` suggestion when moving a mutable reference)
 - rust-lang#127769 (Don't use implicit features in `Cargo.toml` in `compiler/`)
 - rust-lang#127844 (Remove invalid further restricting suggestion for type bound)
 - rust-lang#127855 (Add myself to review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125042 (Use ordinal number in argument error)
 - rust-lang#127229 (rustdoc: click target for sidebar items flush left)
 - rust-lang#127337 (Move a few intrinsics to Rust abi)
 - rust-lang#127472 (MIR building: Stop using `unpack!` for `BlockAnd<()>`)
 - rust-lang#127579 (Solve a error `.clone()` suggestion when moving a mutable reference)
 - rust-lang#127769 (Don't use implicit features in `Cargo.toml` in `compiler/`)
 - rust-lang#127844 (Remove invalid further restricting suggestion for type bound)
 - rust-lang#127855 (Add myself to review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 23dd3a0 into rust-lang:master Jul 18, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 18, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 18, 2024
Rollup merge of rust-lang#127579 - surechen:fix_127285, r=lcnr

Solve a error `.clone()` suggestion when moving a mutable reference

If the moved value is a mut reference, it is used in a generic function and it's type is a generic param, suggest it can be reborrowed to avoid moving.

for example:

```rust
struct Y(u32);
// x's type is '& mut Y' and it is used in `fn generic<T>(x: T) {}`.
fn generic<T>(x: T) {}
```

fixes rust-lang#127285
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unhelpful .clone() suggestion when moving a mutable reference
5 participants