Skip to content

Suggestion for "error[E0507]: cannot move out of a shared reference" suggests removing the wrong borrow #132806

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
unflxw opened this issue Nov 9, 2024 · 2 comments · Fixed by #133486
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@unflxw
Copy link

unflxw commented Nov 9, 2024

I tried this code (playground):

use std::collections::HashMap;

fn main() {
    HashMap::<String, i32>::new().iter().filter( |&(&k, &v)| { true });
}

I expected to see this happen:

error[E0507]: cannot move out of a shared reference
[...]
help: consider removing the borrow
  |
4 -     HashMap::<String, i32>::new().iter().filter( |&(&k, &v)| { true });
4 +     HashMap::<String, i32>::new().iter().filter( |&(k, &v)| { true });
  |

Instead, this happened:

error[E0507]: cannot move out of a shared reference
[...]
help: consider removing the borrow
  |
4 -     HashMap::<String, i32>::new().iter().filter( |&(&k, &v)| { true });
4 +     HashMap::<String, i32>::new().iter().filter( |(&k, &v)| { true });
  |

Diff between the two (red is actual, green is expected):

- 4 +     HashMap::<String, i32>::new().iter().filter( |(&k, &v)| { true });
+ 4 +     HashMap::<String, i32>::new().iter().filter( |&(k, &v)| { true });

That is, it is suggesting removing the borrow on the tuple, which is not the correct borrow to remove; instead, it should suggest removing the borrow on k, the element that isn't Copy.

Happens in playground nightly: 1.8.0-nightly (2024-11-08 59cec72)

@unflxw unflxw added the C-bug Category: This is a bug. label Nov 9, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 9, 2024
@jieyouxu jieyouxu added A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. C-bug Category: This is a bug. labels Nov 9, 2024
@dianne
Copy link
Contributor

dianne commented Nov 22, 2024

@rustbot claim

Noratrieb added a commit to Noratrieb/rust that referenced this issue Dec 31, 2024
…r=estebank

borrowck diagnostics: make `add_move_error_suggestions` use the HIR rather than `SourceMap`

This PR aims to fix rust-lang#132806 by rewriting `add_move_error_suggestions`[^1]. Previously, it manually scanned the source text to find a leading `&`, which isn't always going to produce a correct result (see: that issue). Admittedly, the HIR visitor in this PR introduces a lot of boilerplate, but hopefully the logic at its core isn't too complicated (I go over it in the comments). I also tried a simpler version that didn't use a HIR visitor and suggested adding `ref` always, but the `&ref x` suggestions really didn't look good. As a bonus for the added complexity though, it's now able to produce nice `&`-removing suggestions in more cases.

I tried to do this such that it avoids edition-dependent checks and its suggestions can be applied together with those from the match ergonomics 2024 migration lint. I haven't added tests for that since the details of match ergonomics 2024 are still being sorted out, but I can try if desired once that's finalized.

[^1]: In brief, it fires on patterns where users try to bind by-value in such a way that moves out of a reference to a non-Copy type (including slice references with non-copy elements). The suggestions are to change the binding's mode to be by-reference, either by removing[^2] an enclosing `&`/`&mut` or adding `ref` to the binding.

[^2]: Incidentally, I find the terminology of "consider removing the borrow" a bit confusing for a suggestion to remove a `&` pattern in order to make bindings borrow rather than move. I'm not sure what a good, concise way to explain that would be though, and that should go in a separate PR anyway.
@bors bors closed this as completed in 3d3d898 Jan 1, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 1, 2025
Rollup merge of rust-lang#133486 - dianne:fix-move-error-suggestion, r=estebank

borrowck diagnostics: make `add_move_error_suggestions` use the HIR rather than `SourceMap`

This PR aims to fix rust-lang#132806 by rewriting `add_move_error_suggestions`[^1]. Previously, it manually scanned the source text to find a leading `&`, which isn't always going to produce a correct result (see: that issue). Admittedly, the HIR visitor in this PR introduces a lot of boilerplate, but hopefully the logic at its core isn't too complicated (I go over it in the comments). I also tried a simpler version that didn't use a HIR visitor and suggested adding `ref` always, but the `&ref x` suggestions really didn't look good. As a bonus for the added complexity though, it's now able to produce nice `&`-removing suggestions in more cases.

I tried to do this such that it avoids edition-dependent checks and its suggestions can be applied together with those from the match ergonomics 2024 migration lint. I haven't added tests for that since the details of match ergonomics 2024 are still being sorted out, but I can try if desired once that's finalized.

[^1]: In brief, it fires on patterns where users try to bind by-value in such a way that moves out of a reference to a non-Copy type (including slice references with non-copy elements). The suggestions are to change the binding's mode to be by-reference, either by removing[^2] an enclosing `&`/`&mut` or adding `ref` to the binding.

[^2]: Incidentally, I find the terminology of "consider removing the borrow" a bit confusing for a suggestion to remove a `&` pattern in order to make bindings borrow rather than move. I'm not sure what a good, concise way to explain that would be though, and that should go in a separate PR anyway.
@unflxw
Copy link
Author

unflxw commented Jan 1, 2025

Thank you for fixing this @dianne! 🙇

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants