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

Peel off explicit (or implicit) deref before suggesting clone on move error in borrowck, remove some hacks #128244

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jul 26, 2024

Also remove a heck of a lot of weird hacks in suggest_cloning that I don't think we should have around.

I know this regresses tests, but I don't believe most of these suggestions were accurate, b/c:

  1. They either produced type errors (e.g. turning &x into x.clone())
  2. They don't fix the issue
  3. They fix the issue ostensibly, but introduce logic errors (e.g. cloning a &mut Option<T> to then Option::take out...)

Most of the suggestions are still wrong, but they're not particularly less wrong IMO.

Stacked on top of #128241, which is an "obviously worth landing" subset of this PR.

r? estebank

@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 26, 2024
&& let Some(ty) = typeck_results.expr_ty_opt(borrowed_expr)
{
self.suggest_cloning(&mut err, ty, borrowed_expr, Some(move_spans));
} else if typeck_results.expr_adjustments(expr).first().is_some_and(|adj| {
Copy link
Member Author

Choose a reason for hiding this comment

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

Implicit borrow, like in f() when f: impl FnMut.

@estebank
Copy link
Contributor

They either produced type errors (e.g. turning &x into x.clone())

I was kind of relying on that to avoid introducing logic errors with the suggestion, but I'm ok with ripping these off, specially if we have an idea to produce better suggestions here.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 31, 2024

📌 Commit 91acacf has been approved by estebank

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 31, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 31, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#123813 (Add `REDUNDANT_IMPORTS` lint for new redundant import detection)
 - rust-lang#126697 ([RFC] mbe: consider the `_` in 2024 an expression)
 - rust-lang#127159 (match lowering: Hide `Candidate` from outside the lowering algorithm)
 - rust-lang#128244 (Peel off explicit (or implicit) deref before suggesting clone on move error in borrowck, remove some hacks)
 - rust-lang#128431 (Add myself as VxWorks target maintainer for reference)
 - rust-lang#128438 (Add special-case for [T, 0] in dropck_outlives)
 - rust-lang#128457 (Fix docs for OnceLock::get_mut_or_init)

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

Rollup of 7 pull requests

Successful merges:

 - rust-lang#123813 (Add `REDUNDANT_IMPORTS` lint for new redundant import detection)
 - rust-lang#126697 ([RFC] mbe: consider the `_` in 2024 an expression)
 - rust-lang#127159 (match lowering: Hide `Candidate` from outside the lowering algorithm)
 - rust-lang#128244 (Peel off explicit (or implicit) deref before suggesting clone on move error in borrowck, remove some hacks)
 - rust-lang#128431 (Add myself as VxWorks target maintainer for reference)
 - rust-lang#128438 (Add special-case for [T, 0] in dropck_outlives)
 - rust-lang#128457 (Fix docs for OnceLock::get_mut_or_init)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 52f7d33 into rust-lang:master Aug 1, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 1, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 1, 2024
Rollup merge of rust-lang#128244 - compiler-errors:move-clone-sugg, r=estebank

Peel off explicit (or implicit) deref before suggesting clone on move error in borrowck, remove some hacks

Also remove a heck of a lot of weird hacks in `suggest_cloning` that I don't think we should have around.

I know this regresses tests, but I don't believe most of these suggestions were accurate, b/c:
1. They either produced type errors (e.g. turning `&x` into `x.clone()`)
2. They don't fix the issue
3. They fix the issue ostensibly, but introduce logic errors (e.g. cloning a `&mut Option<T>` to then `Option::take` out...)

Most of the suggestions are still wrong, but they're not particularly *less* wrong IMO.

Stacked on top of rust-lang#128241, which is an "obviously worth landing" subset of this PR.

r? estebank
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.

4 participants