Fix missing explanation of where the borrowed reference is used when the same borrow occurs multiple times due to loop iterations #102080

yanchen4791 · 2022-09-21T00:12:09Z

Problem of the issue:
If a borrow occurs in a loop, the borrowed reference could be invalidated at the same place at next iteration of the loop. When this happens, the point where the borrow occurs is the same as the intervening point that might invalidate the reference in the loop. This causes a problem for the current code finding the point where the resulting reference is used, so that the explanation of the cause will be missing. As the second point of "explain all errors in terms of three points" (see leveraging intuition framing errors in terms of points", this explanation is very helpful for user to understand the error.

In the current implementation, the searching region for finding the location where the borrowed reference is used is limited to between the place where the borrow occurs and the place where the reference is invalidated. If those two places happen to be the same, which indicates that the borrow and invalidation occur at the same place in a loop, the search will fail.

One solution to the problem is when these two places are the same, find the terminator of the loop, and then use the location of the loop terminator instead of the location of the borrow for the region to find the place where the borrowed reference is used.

rust-highfive · 2022-09-21T00:12:13Z

r? @davidtwco

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

yanchen4791 · 2022-09-21T04:12:43Z

@rustbot label +A-diagnostics

compiler/rustc_borrowck/src/region_infer/mod.rs

src/test/ui/borrowck/two-phase-across-loop.stderr

compiler/rustc_borrowck/src/region_infer/mod.rs

src/test/ui/nll/closures-in-loops.stderr

src/test/ui/borrowck/two-phase-across-loop.stderr

davidtwco

A few more comments..

davidtwco

Some more changes, going to re-assign this to double-check it because it's been a while since I've worked on the borrow checker.

compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs

compiler/rustc_borrowck/src/region_infer/mod.rs

src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.stderr

davidtwco · 2022-09-23T08:17:46Z

r? @cjgillot

cjgillot

Thanks @yanchen4791. The simplification looks good. I have a few questions.

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs

compiler/rustc_borrowck/src/region_infer/mod.rs

compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

compiler/rustc_borrowck/src/region_infer/mod.rs

cjgillot · 2022-09-28T19:49:29Z

src/test/ui/nll/closures-in-loops.stderr

-   |                ^^ - borrows occur due to use of `x` in closure
-   |                |
-   |                `x` was mutably borrowed here in the previous iteration of the loop
+   |         -------^^-------------------


The spans are a bit unfortunate. Is there anything to do here?

You mean the format of using "-------^^-------------------" is not clear?

I just realised: the longer --- is meant to point to loop_terminator_location. It should point to some } below, shouldn't it?

src/test/ui/nll/closures-in-loops.stderr

compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs

…borrow occurs in loop iteration

yanchen4791 · 2022-10-13T22:56:19Z

@cjgillot Any other comments?

cjgillot · 2022-10-15T13:04:34Z

Sorry for the wait @yanchen4791. It took me a bit of time to understand why this PR was doing the right thing.
@bors r+

bors · 2022-10-15T13:04:36Z

📌 Commit b3bf931 has been approved by cjgillot

It is now in the queue for this repository.

…illot Fix missing explanation of where the borrowed reference is used when the same borrow occurs multiple times due to loop iterations Fix rust-lang#99824. Problem of the issue: If a borrow occurs in a loop, the borrowed reference could be invalidated at the same place at next iteration of the loop. When this happens, the point where the borrow occurs is the same as the intervening point that might invalidate the reference in the loop. This causes a problem for the current code finding the point where the resulting reference is used, so that the explanation of the cause will be missing. As the second point of "explain all errors in terms of three points" (see [leveraging intuition framing errors in terms of points"](https://rust-lang.github.io/rfcs/2094-nll.html#leveraging-intuition-framing-errors-in-terms-of-points), this explanation is very helpful for user to understand the error. In the current implementation, the searching region for finding the location where the borrowed reference is used is limited to between the place where the borrow occurs and the place where the reference is invalidated. If those two places happen to be the same, which indicates that the borrow and invalidation occur at the same place in a loop, the search will fail. One solution to the problem is when these two places are the same, find the terminator of the loop, and then use the location of the loop terminator instead of the location of the borrow for the region to find the place where the borrowed reference is used.

bors · 2022-10-16T07:41:00Z

⌛ Testing commit b3bf931 with merge 11432fe...

bors · 2022-10-16T10:10:35Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 11432fe to master...

bors · 2022-10-16T10:10:36Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 11432fe to master...

rust-timer · 2022-10-16T11:29:20Z

Finished benchmarking commit (11432fe): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.4%	[-3.4%, -3.4%]	1
Improvements ✅ (secondary)	-3.6%	[-10.6%, -2.0%]	11
All ❌✅ (primary)	-3.4%	[-3.4%, -3.4%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.7%	[3.7%, 3.7%]	1
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.0%	[-2.0%, -2.0%]	1

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

rust-highfive assigned davidtwco Sep 21, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2022

yanchen4791 mentioned this pull request Sep 21, 2022

Borrow checker gives E0499 in loops with mutable borrows, but it is misleading #99824

Closed

rustbot added the A-diagnostics Area: Messages for errors, warnings, and lints label Sep 21, 2022

chenyukang reviewed Sep 21, 2022

View reviewed changes

compiler/rustc_borrowck/src/region_infer/mod.rs Outdated Show resolved Hide resolved

chenyukang reviewed Sep 21, 2022

View reviewed changes

src/test/ui/borrowck/two-phase-across-loop.stderr Outdated Show resolved Hide resolved

davidtwco reviewed Sep 21, 2022

View reviewed changes

src/test/ui/borrowck/two-phase-across-loop.stderr Outdated Show resolved Hide resolved

yanchen4791 force-pushed the issue-99824-fix branch from 88bb1ba to 5601f53 Compare September 21, 2022 21:26

davidtwco reviewed Sep 22, 2022

View reviewed changes

compiler/rustc_borrowck/src/region_infer/mod.rs Outdated Show resolved Hide resolved

src/test/ui/nll/closures-in-loops.stderr Outdated Show resolved Hide resolved

src/test/ui/borrowck/two-phase-across-loop.stderr Outdated Show resolved Hide resolved

davidtwco reviewed Sep 22, 2022

View reviewed changes

yanchen4791 force-pushed the issue-99824-fix branch from 5601f53 to 9452116 Compare September 23, 2022 00:47

davidtwco requested changes Sep 23, 2022

View reviewed changes

rust-highfive assigned cjgillot and unassigned davidtwco Sep 23, 2022

yanchen4791 force-pushed the issue-99824-fix branch from 9452116 to db81002 Compare September 23, 2022 17:32

cjgillot reviewed Sep 24, 2022

View reviewed changes

yanchen4791 force-pushed the issue-99824-fix branch from db81002 to 89a3f91 Compare September 26, 2022 23:40

cjgillot reviewed Sep 28, 2022

View reviewed changes

Fix missing explanation of where borrowed reference is used when the …

b3bf931

…borrow occurs in loop iteration

yanchen4791 force-pushed the issue-99824-fix branch from 89a3f91 to b3bf931 Compare September 28, 2022 22:35

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 Oct 15, 2022

matthiaskrgr mentioned this pull request Oct 15, 2022

Rollup of 5 pull requests #103090

Closed

matthiaskrgr mentioned this pull request Oct 15, 2022

Rollup of 6 pull requests #103097

Closed

matthiaskrgr mentioned this pull request Oct 15, 2022

Rollup of 6 pull requests #103099

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 16, 2022

bors merged commit 11432fe into rust-lang:master Oct 16, 2022

rustbot added this to the 1.66.0 milestone Oct 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing explanation of where the borrowed reference is used when the same borrow occurs multiple times due to loop iterations #102080

Fix missing explanation of where the borrowed reference is used when the same borrow occurs multiple times due to loop iterations #102080

yanchen4791 commented Sep 21, 2022

rust-highfive commented Sep 21, 2022

yanchen4791 commented Sep 21, 2022

davidtwco left a comment

davidtwco left a comment

davidtwco commented Sep 23, 2022

cjgillot left a comment

cjgillot Sep 28, 2022

yanchen4791 Sep 28, 2022

cjgillot Oct 1, 2022

yanchen4791 commented Oct 13, 2022

cjgillot commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 16, 2022

bors commented Oct 16, 2022

bors commented Oct 16, 2022

rust-timer commented Oct 16, 2022

Fix missing explanation of where the borrowed reference is used when the same borrow occurs multiple times due to loop iterations #102080

Fix missing explanation of where the borrowed reference is used when the same borrow occurs multiple times due to loop iterations #102080

Conversation

yanchen4791 commented Sep 21, 2022

rust-highfive commented Sep 21, 2022

yanchen4791 commented Sep 21, 2022

davidtwco left a comment

Choose a reason for hiding this comment

davidtwco left a comment

Choose a reason for hiding this comment

davidtwco commented Sep 23, 2022

cjgillot left a comment

Choose a reason for hiding this comment

cjgillot Sep 28, 2022

Choose a reason for hiding this comment

yanchen4791 Sep 28, 2022

Choose a reason for hiding this comment

cjgillot Oct 1, 2022

Choose a reason for hiding this comment

yanchen4791 commented Oct 13, 2022

cjgillot commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 16, 2022

bors commented Oct 16, 2022

bors commented Oct 16, 2022

rust-timer commented Oct 16, 2022

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes