Suggest using a temporary variable to fix borrowck errors by camelid · Pull Request #83174 · rust-lang/rust

camelid · 2021-03-15T22:20:14Z

In Rust, nesting method calls with both require &mut access to self
produces a borrow-check error:

error[E0499]: cannot borrow `*self` as mutable more than once at a time
 --> src/lib.rs:7:14
  |
7 |     self.foo(self.bar());
  |     ---------^^^^^^^^^^-
  |     |    |   |
  |     |    |   second mutable borrow occurs here
  |     |    first borrow later used by call
  |     first mutable borrow occurs here

That's because Rust has a left-to-right evaluation order, and the method
receiver is passed first. Thus, the argument to the method cannot then
mutate self.

There's an easy solution to this error: just extract a local variable
for the inner argument:

let tmp = self.bar();
self.foo(tmp);

However, the error doesn't give any suggestion of how to solve the
problem. As a result, new users may assume that it's impossible to
express their code correctly and get stuck.

This commit adds a (non-structured) suggestion to extract a local
variable for the inner argument to solve the error. The suggestion uses
heuristics that eliminate most false positives, though there are a few
false negatives (cases where the suggestion should be emitted but is
not). Those other cases can be implemented in a future change.

rust-highfive · 2021-03-15T22:20:16Z

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs

src/test/ui/borrowck/borrowck-assign-comp-idx.stderr

bors · 2021-05-02T17:34:42Z

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

camelid · 2021-06-12T23:45:44Z

Starting with a rebase so this doesn't get too stale.

camelid · 2021-10-20T05:00:55Z

Status update: I've figured out, and implemented, a way to find the locations of
the outer and inner calls.

camelid · 2021-10-20T19:25:39Z

FYI, I just noticed this (pre-existing) test has a typo in it, but it probably shouldn't matter now that MIR borrowck is the default, right?

https://github.com/rust-lang/rust/blob/e6875793dd149345fca224f84d71814a0daa1be7/src/test/ui/borrowck/two-phase-sneaky.rs#L1

camelid

@oli-obk I've made some progress on this. When you get a chance, could you look at the code and the errors? There are a few issues where the wrong span is used, but the false positive rate is much lower 🎉

I left comments on all the test outputs that have issues. I would appreciate suggestions on how to improve the accuracy :)

src/test/ui/borrowck/suggest-local-var-double-mut.stderr

src/test/ui/borrowck/suggest-local-var-imm-and-mut.stderr

camelid · 2021-10-20T19:23:43Z

src/test/ui/borrowck/two-phase-nonrecv-autoref.nll.stderr

This span's not great, but not terrible either.

I like it more than the one that goes to the end of the call as spans won't overlap this way

This regressed with #83174 (comment); the suggestion is no longer reported. I think let's just leave it for now.

src/test/ui/borrowck/two-phase-sneaky.stderr

src/test/ui/codemap_tests/one_line.stderr

src/test/ui/coercion/coerce-overloaded-autoderef-fail.stderr

src/test/ui/issues/issue-18566.stderr

src/test/ui/issues/issue-61623.stderr

bors · 2021-12-10T19:49:42Z

📌 Commit 381b275 has been approved by oli-obk

bors · 2021-12-10T19:49:43Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

camelid · 2021-12-10T19:54:58Z

@oli-obk thank you so much for helping me with this PR over the past 9 months! ❤️

Suggest using a temporary variable to fix borrowck errors Fixes rust-lang#77834. In Rust, nesting method calls with both require `&mut` access to `self` produces a borrow-check error: error[E0499]: cannot borrow `*self` as mutable more than once at a time --> src/lib.rs:7:14 | 7 | self.foo(self.bar()); | ---------^^^^^^^^^^- | | | | | | | second mutable borrow occurs here | | first borrow later used by call | first mutable borrow occurs here That's because Rust has a left-to-right evaluation order, and the method receiver is passed first. Thus, the argument to the method cannot then mutate `self`. There's an easy solution to this error: just extract a local variable for the inner argument: let tmp = self.bar(); self.foo(tmp); However, the error doesn't give any suggestion of how to solve the problem. As a result, new users may assume that it's impossible to express their code correctly and get stuck. This commit adds a (non-structured) suggestion to extract a local variable for the inner argument to solve the error. The suggestion uses heuristics that eliminate most false positives, though there are a few false negatives (cases where the suggestion should be emitted but is not). Those other cases can be implemented in a future change.

matthiaskrgr · 2021-12-10T21:39:16Z

@bors r- rollup-
failed in a rollup:
#91756 (comment)
Can you please double check that there is no test failure? Thanks!

In Rust, nesting method calls with both require `&mut` access to `self` produces a borrow-check error: error[E0499]: cannot borrow `*self` as mutable more than once at a time --> src/lib.rs:7:14 | 7 | self.foo(self.bar()); | ---------^^^^^^^^^^- | | | | | | | second mutable borrow occurs here | | first borrow later used by call | first mutable borrow occurs here That's because Rust has a left-to-right evaluation order, and the method receiver is passed first. Thus, the argument to the method cannot then mutate `self`. There's an easy solution to this error: just extract a local variable for the inner argument: let tmp = self.bar(); self.foo(tmp); However, the error doesn't give any suggestion of how to solve the problem. As a result, new users may assume that it's impossible to express their code correctly and get stuck. This commit adds a (non-structured) suggestion to extract a local variable for the inner argument to solve the error. The suggestion uses heuristics that eliminate most false positives, though there are a few false negatives (cases where the suggestion should be emitted but is not). Those other cases can be implemented in a future change.

camelid · 2021-12-10T22:35:36Z

src/test/ui/borrowck/suggest-local-var-imm-and-mut.nll.stderr

+help: try adding a local storing this argument...
+  --> $DIR/suggest-local-var-imm-and-mut.rs:12:22
+   |
+LL |             self.foo(self.bar());
+   |                      ^^^^^^^^^^
+help: ...and then using that local as the argument to this call
+  --> $DIR/suggest-local-var-imm-and-mut.rs:12:13
+   |
+LL |             self.foo(self.bar());
+   |             ^^^^^^^^^^^^^^^^^^^^


It's intriguing that the suggestion doesn't show up in normal mode but does (correctly) show up in nll mode.

camelid · 2021-12-10T22:36:11Z

@bors r=oli-obk

bors · 2021-12-10T22:36:13Z

📌 Commit e273152 has been approved by oli-obk

Suggest using a temporary variable to fix borrowck errors Fixes rust-lang#77834. In Rust, nesting method calls with both require `&mut` access to `self` produces a borrow-check error: error[E0499]: cannot borrow `*self` as mutable more than once at a time --> src/lib.rs:7:14 | 7 | self.foo(self.bar()); | ---------^^^^^^^^^^- | | | | | | | second mutable borrow occurs here | | first borrow later used by call | first mutable borrow occurs here That's because Rust has a left-to-right evaluation order, and the method receiver is passed first. Thus, the argument to the method cannot then mutate `self`. There's an easy solution to this error: just extract a local variable for the inner argument: let tmp = self.bar(); self.foo(tmp); However, the error doesn't give any suggestion of how to solve the problem. As a result, new users may assume that it's impossible to express their code correctly and get stuck. This commit adds a (non-structured) suggestion to extract a local variable for the inner argument to solve the error. The suggestion uses heuristics that eliminate most false positives, though there are a few false negatives (cases where the suggestion should be emitted but is not). Those other cases can be implemented in a future change.

…askrgr Rollup of 6 pull requests Successful merges: - rust-lang#83174 (Suggest using a temporary variable to fix borrowck errors) - rust-lang#89734 (Point at capture points for non-`'static` reference crossing a `yield` point) - rust-lang#90270 (Make `Borrow` and `BorrowMut` impls `const`) - rust-lang#90741 (Const `Option::cloned`) - rust-lang#91548 (Add spin_loop hint for RISC-V architecture) - rust-lang#91721 (Minor improvements to `future::join!`'s implementation) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned oli-obk Mar 15, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 15, 2021

camelid commented Mar 15, 2021

View reviewed changes

compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs Outdated Show resolved Hide resolved

src/test/ui/borrowck/borrowck-assign-comp-idx.stderr Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

camelid force-pushed the borrow-help branch from 17996f8 to 51d90d8 Compare March 16, 2021 01:44

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 16, 2021

camelid removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 4, 2021

camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 5, 2021

camelid force-pushed the borrow-help branch from 51d90d8 to 013778d Compare June 12, 2021 23:45

This comment has been minimized.

Sign in to view

camelid force-pushed the borrow-help branch from 013778d to 90d8242 Compare June 13, 2021 00:10

inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021

This comment has been minimized.

Sign in to view

oli-obk removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 6, 2021

camelid force-pushed the borrow-help branch from 90d8242 to 166d1ba Compare October 20, 2021 02:59

This comment has been minimized.

Sign in to view

camelid commented Oct 20, 2021

View reviewed changes

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 Dec 10, 2021

matthiaskrgr mentioned this pull request Dec 10, 2021

Rollup of 22 pull requests #91756

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 10, 2021

camelid force-pushed the borrow-help branch from 381b275 to e273152 Compare December 10, 2021 22:35

camelid commented Dec 10, 2021

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 10, 2021

matthiaskrgr mentioned this pull request Dec 11, 2021

Rollup of 6 pull requests #91775

Closed

matthiaskrgr mentioned this pull request Dec 11, 2021

Rollup of 6 pull requests #91790

Closed

matthiaskrgr mentioned this pull request Dec 11, 2021

Rollup of 6 pull requests #91799

Merged

bors merged commit 433a13b into rust-lang:master Dec 11, 2021

rustbot added this to the 1.59.0 milestone Dec 11, 2021

camelid deleted the borrow-help branch December 11, 2021 21:58

Uh oh!

Conversation

camelid commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Mar 15, 2021

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

bors commented May 2, 2021

Uh oh!

camelid commented Jun 12, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

camelid commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

camelid commented Oct 20, 2021

Uh oh!

camelid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

camelid Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

oli-obk Oct 30, 2021

Choose a reason for hiding this comment

Uh oh!

camelid Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented Dec 10, 2021

Uh oh!

bors commented Dec 10, 2021

Uh oh!

camelid commented Dec 10, 2021

Uh oh!

matthiaskrgr commented Dec 10, 2021

Uh oh!

camelid Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camelid commented Dec 10, 2021

Uh oh!

bors commented Dec 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

camelid commented Mar 15, 2021 •

edited

Loading

camelid commented Oct 20, 2021 •

edited

Loading

camelid Dec 10, 2021 •

edited

Loading