Capture precise paths in THIR and MIR #79553

arora-aman · 2020-11-30T07:08:53Z

This PR allows THIR and MIR to use the result of the new capture analysis to actually capture precise paths

To achieve we:

Writeback min capture results to TypeckResults
Move handling upvars to PlaceBuilder in mir_build
Lower precise paths in THIR build by reading min_captures
Search for ancestors in min_capture when trying to build a MIR place which starts off of an upvar

Closes: rust-lang/project-rfc-2229#10

Partly implements: rust-lang/project-rfc-2229#18

Work that remains (not in this PR):

Known bugs when feature gate is enabled
Use min_capure_map for
- Liveness analysis
- rustc_mir/interpret/validity.rs
- regionck
Update error logging in borrow code project-rfc-2229#8
remove closure_captures and upvar_capture_map

r? @ghost

joshtriplett · 2020-11-30T09:00:46Z

@bors try @rust-timer queue

rust-timer · 2020-11-30T09:00:50Z

Awaiting bors try build completion

bors · 2020-11-30T09:20:41Z

⌛ Trying commit 3ac3ff2 with merge f76f72250e36c9ad4eb3bd3f08770228cc07da6c...

bors · 2020-11-30T10:06:05Z

☀️ Try build successful - checks-actions
Build commit: f76f72250e36c9ad4eb3bd3f08770228cc07da6c (f76f72250e36c9ad4eb3bd3f08770228cc07da6c)

rust-timer · 2020-11-30T10:06:07Z

Queued f76f72250e36c9ad4eb3bd3f08770228cc07da6c with parent 28b86e0, future comparison URL.

rust-timer · 2020-11-30T11:40:50Z

Finished benchmarking try commit (f76f72250e36c9ad4eb3bd3f08770228cc07da6c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

nikomatsakis · 2020-11-30T22:03:08Z

r? @nikomatsakis

nikomatsakis

some initial comments

compiler/rustc_mir_build/src/build/expr/as_place.rs

arora-aman · 2020-12-02T01:02:08Z

compiler/rustc_middle/src/ty/sty.rs

@@ -2179,6 +2179,15 @@ impl<'tcx> TyS<'tcx> {
        }
    }

+    /// Get the `i`-th element of a tuple.


This was discussed here: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Accessing.20a.20field.20in.20a.20tuple

arora-aman · 2020-12-02T01:22:30Z

compiler/rustc_mir_build/src/build/expr/as_place.rs

+/// Computes the index of a capture within the desugared closure provided the closure's
+/// `closure_min_captures` and the capture's index of the capture in the
+/// `ty::MinCaptureList` of the root variable `var_hir_id`.
+fn compute_capture_idx<'tcx>(


This can probably be optimized by moving it into the min_capture map itself, so we move the table from looking like

root variable -> list of captures

to

root variarble -> { start_idx, list of captures }

where the start_idx is essentially the index in min_capture map for the first capture in the list of captures for the root variable.

arora-aman · 2020-12-02T02:20:42Z

@nikomatsakis updated

- Derive TypeFoldable on `hir::place::Place` and associated structs, to them to be written into typeck results. Co-authored-by: Jennifer Wills <[email protected]> Co-authored-by: Logan Mosier <[email protected]>

- final_upvar_tys now reads types from places instead of using `node_ty` Co-authored-by: Roxane Fruytier <[email protected]>

- This allows us to delay figuring out the index of a capture in the closure structure when all projections to atleast form a capture have been applied to the builder Co-authored-by: Roxane Fruytier <[email protected]>

compiler/rustc_middle/src/ty/sty.rs

compiler/rustc_mir_build/src/build/expr/as_place.rs

compiler/rustc_mir_build/src/build/expr/as_rvalue.rs

compiler/rustc_mir_build/src/thir/cx/expr.rs

nikomatsakis · 2020-12-08T11:38:20Z

Hey! Left a bunch of comments. Mostly nits and small suggestions, though there are a few points where I was confused. This is looking really good. :)

arora-aman · 2020-12-09T07:26:10Z

I pushed changed in the same commit because there weren't any logical changes. Just the diff is here: https://github.com/sexxi-goose/rust/compare/ef71437..ad36cd2

arora-aman · 2020-12-09T20:31:19Z

compiler/rustc_mir_build/src/build/expr/as_place.rs

+///           start off of the same root variable.
+///
+/// Eg: 1. `foo.x` which is represented using `projections=[Field(x)]` is an ancestor of
+///     `foo.x.y` which is represented using `prjections=[Field(x), Field(y)]`.


arora-aman · 2020-12-09T20:32:08Z

compiler/rustc_mir_build/src/build/expr/as_place.rs

+    proj_possible_ancestor: &Vec<HirProjectionKind>,
+    proj_capture: &Vec<HirProjectionKind>,
+) -> bool {
+    // We want to make sure `is_ancestor_of_capture("x.0", "x.0.0")` not return true.


is_ancestor_or_same_capture("x.0.0", "x.0")

nikomatsakis

This is looking very good. I am wondering if we want to do more tests.

compiler/rustc_mir_build/src/build/expr/as_place.rs

src/test/ui/closures/2229_closure_analysis/run_pass/multilevel-path-1.rs

src/test/ui/closures/2229_closure_analysis/run_pass/multilevel-path-2.rs

src/test/ui/closures/2229_closure_analysis/run_pass/simple-struct-min-capture.rs

- Use closure_min_capture maps to capture precise paths - PlaceBuilder now searches for ancestors in min_capture list - Add API to `Ty` to allow access to the n-th element in a tuple in O(1) time. Co-authored-by: Roxane Fruytier <[email protected]>

- Closures now use closure_min_captures to figure out captured paths - Build upvar_mutbls using closure_min_captures - Change logic in limit_capture_mutability to differentiate b/w capturing parent's local variable or capturing a variable that is captured by the parent (in case of nested closure) using PlaceBase. Co-authored-by: Roxane Fruytier <[email protected]>

- Use closure_min_captures to generate the Upvar structure that stores information for diagnostics and information about mutability of captures.

arora-aman · 2020-12-11T09:04:42Z

@nikomatsakis updated.

I added some more test cases:

Borrow checker cases so that it print out the precise expression that is captured. (Pointing to expression properly would be part of Update error logging in borrow code project-rfc-2229#8)
Test cases that use Box
Test cases that ensure arrays are fully captured.

nikomatsakis · 2020-12-11T22:26:42Z

@bors r+

bors · 2020-12-11T22:26:44Z

📌 Commit 01df563 has been approved by nikomatsakis

bors · 2020-12-12T00:23:34Z

⌛ Testing commit 01df563 with merge 5bd9b60...

bors · 2020-12-12T02:40:12Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 5bd9b60 to master...

bjorn3 · 2020-12-12T09:11:49Z

compiler/rustc_mir_build/src/build/expr/as_place.rs

+        /// DefId of the closure
+        closure_def_id: DefId,
+        /// The trait closure implements, `Fn`, `FnMut`, `FnOnce`
+        closure_kind: ty::ClosureKind },


Wait, how did rustfmt accept this formatting???

that...is a good question :)

Opened rust-lang/rustfmt#4589.

I've no idea whether this is intentional, but believe this file would be skipped entirely due the ignore settings specified in the rustfmt file since it's under a build directory

Compare the build directory entry in rustfmt.toml vs the gitignore entry where the latter is limited to the root.

As I recall there were also some constraints added around which files get formatted and how due to the nature of some of the content within the repo. It's formatted fine when running rustfmt directly, so may want to check whether x.py fmt is actually running rustfmt on these files

cc @Mark-Simulacrum

Yeah seems like a bug. Would be great to fix in a PR.

rylev · 2020-12-15T17:14:43Z

@arora-aman @nikomatsakis This changes seems to have caused a moderate regression in the clap benchmark. It looks like typck is taking longer than before (37.5% of compilation time vs. 35.5%).

arora-aman · 2020-12-15T17:18:37Z

@rylev I had brought this in t-performance a while back, it's likely that this is because of extra data now being stored in TypeckResults, which adds more data that needs to get hashed in incr-compile mode.

We are working de duplicating some of the information in TypeckResults, to hopefully being this back down :)

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 30, 2020

rust-highfive assigned nikomatsakis Nov 30, 2020

nikomatsakis reviewed Nov 30, 2020

View reviewed changes

camelid added A-closures Area: Closures (`|…| { … }`) A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels Dec 1, 2020

arora-aman force-pushed the mir_min_cap_writeback branch 4 times, most recently from 4c2eabd to 097e490 Compare December 2, 2020 01:11

arora-aman commented Dec 2, 2020

View reviewed changes

arora-aman and others added 2 commits December 6, 2020 15:48

Writeback min_capture map to TypeckResults

76c68aa

- Derive TypeFoldable on `hir::place::Place` and associated structs, to them to be written into typeck results. Co-authored-by: Jennifer Wills <[email protected]> Co-authored-by: Logan Mosier <[email protected]>

Use min_captures for creating UpvarSusbts::tupled_upvar_tys

6e5cca7

- final_upvar_tys now reads types from places instead of using `node_ty` Co-authored-by: Roxane Fruytier <[email protected]>

arora-aman force-pushed the mir_min_cap_writeback branch from 097e490 to 487352b Compare December 6, 2020 22:25

Move handling UpvarRef to PlaceBuilder

d952362

- This allows us to delay figuring out the index of a capture in the closure structure when all projections to atleast form a capture have been applied to the builder Co-authored-by: Roxane Fruytier <[email protected]>

arora-aman force-pushed the mir_min_cap_writeback branch from 487352b to ef71437 Compare December 6, 2020 23:59

nikomatsakis reviewed Dec 8, 2020

View reviewed changes

arora-aman force-pushed the mir_min_cap_writeback branch from ef71437 to ad36cd2 Compare December 9, 2020 07:23

arora-aman commented Dec 9, 2020

View reviewed changes

nikomatsakis approved these changes Dec 9, 2020

View reviewed changes

arora-aman and others added 3 commits December 9, 2020 22:33

Use Places for captures in MIR

6a1d069

- Use closure_min_capture maps to capture precise paths - PlaceBuilder now searches for ancestors in min_capture list - Add API to `Ty` to allow access to the n-th element in a tuple in O(1) time. Co-authored-by: Roxane Fruytier <[email protected]>

Use closure_min_captures in borrow checker

237ad12

- Use closure_min_captures to generate the Upvar structure that stores information for diagnostics and information about mutability of captures.

arora-aman force-pushed the mir_min_cap_writeback branch from ad36cd2 to 0f60e68 Compare December 11, 2020 09:00

Test cases for RFC 2229

01df563

arora-aman force-pushed the mir_min_cap_writeback branch from 0f60e68 to 01df563 Compare December 11, 2020 10:19

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 11, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 12, 2020

bors merged commit 5bd9b60 into rust-lang:master Dec 12, 2020

rustbot added this to the 1.50.0 milestone Dec 12, 2020

arora-aman deleted the mir_min_cap_writeback branch December 12, 2020 04:49

bjorn3 reviewed Dec 12, 2020

View reviewed changes

bjorn3 mentioned this pull request Dec 13, 2020

Not formatting when it should rust-lang/rustfmt#4589

Closed

lqd mentioned this pull request Apr 28, 2022

Internal Compiler Error on stable (Broken MIR) #96512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capture precise paths in THIR and MIR #79553

Capture precise paths in THIR and MIR #79553

arora-aman commented Nov 30, 2020 •

edited

Loading

joshtriplett commented Nov 30, 2020

rust-timer commented Nov 30, 2020

bors commented Nov 30, 2020

bors commented Nov 30, 2020

rust-timer commented Nov 30, 2020

rust-timer commented Nov 30, 2020

nikomatsakis commented Nov 30, 2020

nikomatsakis left a comment

arora-aman Dec 2, 2020

arora-aman Dec 2, 2020

arora-aman commented Dec 2, 2020

nikomatsakis commented Dec 8, 2020

arora-aman commented Dec 9, 2020

arora-aman Dec 9, 2020

arora-aman Dec 9, 2020

nikomatsakis left a comment

arora-aman commented Dec 11, 2020

nikomatsakis commented Dec 11, 2020

bors commented Dec 11, 2020

bors commented Dec 12, 2020

bors commented Dec 12, 2020

bjorn3 Dec 12, 2020

nikomatsakis Dec 12, 2020

bjorn3 Dec 13, 2020

calebcartwright Dec 13, 2020

Mark-Simulacrum Dec 14, 2020

rylev commented Dec 15, 2020

arora-aman commented Dec 15, 2020

Capture precise paths in THIR and MIR #79553

Capture precise paths in THIR and MIR #79553

Conversation

arora-aman commented Nov 30, 2020 • edited Loading

joshtriplett commented Nov 30, 2020

rust-timer commented Nov 30, 2020

bors commented Nov 30, 2020

bors commented Nov 30, 2020

rust-timer commented Nov 30, 2020

rust-timer commented Nov 30, 2020

nikomatsakis commented Nov 30, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arora-aman commented Dec 2, 2020

nikomatsakis commented Dec 8, 2020

arora-aman commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

arora-aman commented Dec 11, 2020

nikomatsakis commented Dec 11, 2020

bors commented Dec 11, 2020

bors commented Dec 12, 2020

bors commented Dec 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylev commented Dec 15, 2020

arora-aman commented Dec 15, 2020

arora-aman commented Nov 30, 2020 •

edited

Loading