Stream: t-compiler/wg-rfc-2229

Topic: upvar path


ange (Mar 11 2019 at 17:14, on Zulip):

@blitzerr @nikomatsakis would it be ok if I started taking a look at Step 3 of the Roadmap since it's kinda orthogonal? lots to learn there for me, I guess

blitzerr (Mar 11 2019 at 17:31, on Zulip):

Is there a sub section (A, B, ..) that particularly interests you in step 3 ?

ange (Mar 11 2019 at 17:44, on Zulip):

B and C I guess?

ange (Mar 11 2019 at 17:44, on Zulip):

B if I had to choose

blitzerr (Mar 11 2019 at 17:47, on Zulip):

:+1:

ange (Mar 16 2019 at 14:59, on Zulip):

OK, so, this is positively infested with FIXMEs, but, for this source:

#![feature(rustc_attrs)]

struct Quux {
    z: usize,
    w: usize,
}
struct Bar {
    quux: Quux,
}

fn foo<F>(mut cl: F) -> usize where F: FnMut() -> usize {
    cl()
}

#[rustc_dump_closure_captures]
fn main() {
    let x = 5;
    let mut y = 6;
    let bar = Bar {
        quux: Quux {
            z: 9,
            w: 10,
        },
    };
    foo(|| {
        y += 1;
        x + y + bar.quux.w + bar.quux.z
    });
}

I now get

error: closure capture paths: Upvar local mut y (id=53) ["Deref Deref"]
  --> example-annotated.rs:25:9
   |
25 |       foo(|| {
   |  _________^
26 | |         y += 1;
27 | |         x + y + bar.quux.w + bar.quux.z
28 | |     });
   | |_____^

error: closure capture paths: Upvar local bar (id=56) ["Deref Deref Field(quux) Field(w)", "Deref Deref Field(quux) Field(z)"]
  --> example-annotated.rs:25:9
   |
25 |       foo(|| {
   |  _________^
26 | |         y += 1;
27 | |         x + y + bar.quux.w + bar.quux.z
28 | |     });
   | |_____^

error: closure capture paths: Upvar local x (id=50) ["Deref Deref"]
  --> example-annotated.rs:25:9
   |
25 |       foo(|| {
   |  _________^
26 | |         y += 1;
27 | |         x + y + bar.quux.w + bar.quux.z
28 | |     });
   | |_____^

error: aborting due to 3 previous errors
ange (Mar 16 2019 at 15:00, on Zulip):

(this is by walking the cmt_ linked-list in adjust_upvar_borrow_captures_for_consume FWIW)

ange (Mar 16 2019 at 15:02, on Zulip):

Will need to take a close look at the usage of NoteClosureEnv and NoteUpvarRef to get rid of the synthetic Derefs

ange (Mar 16 2019 at 15:05, on Zulip):

@blitzerr @nikomatsakis FWIW, it doesn't seem viable to keep the paths in UpvarId (needs to be Copy). Currently, I just added a separate map from UpvarId -> paths

nikomatsakis (Mar 18 2019 at 20:13, on Zulip):

Oh, nice

ange (Mar 22 2019 at 16:09, on Zulip):

@nikomatsakis since you're answering questions... I'm doing some exploratory coding trying to reuse the cmt_ machinery to track paths to accesses. one issue is that, when there's indexing, you get a Categorization::Rvalue that you can't look past (so that e.g. you can tell that when you have foo.bar[x].quux, the capture path is foo.bar. I'm looking into extending the Rvalue to record the "base" cmt_, not sure if that's the best approach?

nikomatsakis (Mar 22 2019 at 16:10, on Zulip):

hmm

nikomatsakis (Mar 22 2019 at 16:10, on Zulip):

I think you only get cat-rvalue some of the time

nikomatsakis (Mar 22 2019 at 16:10, on Zulip):

specifically, when there is an Index impl

nikomatsakis (Mar 22 2019 at 16:10, on Zulip):

do you also get it for indexing into a slice?

ange (Mar 22 2019 at 16:10, on Zulip):

fair enough, only tried with vecs so far

nikomatsakis (Mar 22 2019 at 16:10, on Zulip):

put another way, we may not want to or be able to do better than that

nikomatsakis (Mar 22 2019 at 16:11, on Zulip):

I actually sort of forget what the RFC said here

nikomatsakis (Mar 22 2019 at 16:11, on Zulip):

but there is this question of when the code in the Deref or Index impl executes

nikomatsakis (Mar 22 2019 at 16:11, on Zulip):

at closure creation time -- or at closure execution time

nikomatsakis (Mar 22 2019 at 16:11, on Zulip):

actually, indexing in particular is tricky here

ange (Mar 22 2019 at 16:11, on Zulip):

AFAIU, the RFC defines a minimal capture like I described, i.e. can't look past indexing, but still can determine something more accurate than foo in the above example

nikomatsakis (Mar 22 2019 at 16:11, on Zulip):

like, maybe the index doesn't exist yet :)

nikomatsakis (Mar 22 2019 at 16:12, on Zulip):

I see, I see, I didn't read closely enough

nikomatsakis (Mar 22 2019 at 16:12, on Zulip):

we may want to make a second path, similar to the one that builds cmts

ange (Mar 22 2019 at 16:12, on Zulip):

sure, the issue here isn't about the indexing, I'm asking about the implementation for determining the path up to (but not including) the indexing

nikomatsakis (Mar 22 2019 at 16:12, on Zulip):

but taht does seem mildly unfortunate

nikomatsakis (Mar 22 2019 at 16:13, on Zulip):

yes, I understand better now

nikomatsakis (Mar 22 2019 at 16:13, on Zulip):

the problem is that the cmt is really oriented around the final value

nikomatsakis (Mar 22 2019 at 16:13, on Zulip):

and what we need is more like "start from the beginning and work your way there"

ange (Mar 22 2019 at 16:13, on Zulip):

yah

nikomatsakis (Mar 22 2019 at 16:13, on Zulip):

I'd have to look into the code

nikomatsakis (Mar 22 2019 at 16:13, on Zulip):

a bit more clsoely

ange (Mar 22 2019 at 16:13, on Zulip):

that said, I'm trying to figure out if it would be simple enough to extend the cmt_ to build up a chain in all cases we need

nikomatsakis (Mar 22 2019 at 16:14, on Zulip):

one option is, yes, to extend the cmt

nikomatsakis (Mar 22 2019 at 16:14, on Zulip):

the other would be to include enough info that we can re-create the cmt for the indexed path

ange (Mar 22 2019 at 16:14, on Zulip):

no worries, hope I'll have a better understanding of the issues till tuesday

nikomatsakis (Mar 22 2019 at 16:14, on Zulip):

there is a slightly larger bit of context

ange (Mar 22 2019 at 16:14, on Zulip):

in a side-table?

nikomatsakis (Mar 22 2019 at 16:14, on Zulip):

which is that, as the AST-based borrow check goes away,

nikomatsakis (Mar 22 2019 at 16:14, on Zulip):

we should be able to refactor the mem-categorization code heavily

nikomatsakis (Mar 22 2019 at 16:14, on Zulip):

unfortuantely it still has other uses

nikomatsakis (Mar 22 2019 at 16:14, on Zulip):

but basically I think it's due for a bit of a revamp

nikomatsakis (Mar 22 2019 at 16:15, on Zulip):

it will (afaik) only be used at that point to do upvar-capture-related computation

nikomatsakis (Mar 22 2019 at 16:15, on Zulip):

so we might want to just think about rewriting it

nikomatsakis (Mar 22 2019 at 16:15, on Zulip):

to be cleaner and to have these new goals in mind

nikomatsakis (Mar 22 2019 at 16:15, on Zulip):

and then the old mem-cat code can hang around and get "garbage collected" when the AST-based borrowck goes away

ange (Mar 22 2019 at 16:15, on Zulip):

fair; not sure I grasp enough of the big picture yet to decide on whether this should be done as part of this implementation effort

nikomatsakis (Mar 22 2019 at 16:15, on Zulip):

but I'd want to check on whether there are other uses first, I may be forgetting something

nikomatsakis (Mar 22 2019 at 16:16, on Zulip):

at least one thing I can think of relates to generators

nikomatsakis (Mar 22 2019 at 16:16, on Zulip):

(which is similar-ish to the upvar capture problem)

ange (Mar 24 2019 at 21:58, on Zulip):

Notes on reusing mem_categorization.rs for this:
- recording the "previous" cmt_ in the Rvalue seems to work in principle (when eyeballing the results; I'm sure there would be plenty of bugs to be found on closer examination)
- a rudimentary sanity check of "did we record at least one path for each key in upvar_capture_map" doesn't trigger when building the compiler and going through the ui test suite. unfortunately can't go any further than that, as I based this exploration on the upvar-tuple branch, so there's some undiagnosed failures there

Another implementation issue lies around the corner: AFAIU, we need to track the minimal necessary access type per (upvar, path) (for this exploratory coding side-quest, I just separately stored the observed paths for each UpvarId). This would mean sticking the paths in UpvarId. UpvarId, however, needs to be Copy for a bunch of reasons, and the paths are dynamic both in number and in length.

What's more, if each UpvarId now contains a path (a path is currently a Vec of the provisionally named CapturePathComponents), that'll probably require non-trivial modifications at every use site, roughly: (1) expr_use_visitor..walk_captures (2) mem_categorization...cat_upvar (3) hair conversion

ange (Mar 24 2019 at 22:00, on Zulip):

I should add that I'm going to have effectively zero time after May 1st, so I'm interested in making quick progress in this, if we can decide on the most promising implementation strategy (e.g. I'm open to trying my hand at a rewrite/fork of cmt_ to narrow its scope)

blitzerr (Mar 26 2019 at 03:08, on Zulip):

@ange I am extremely sorry I have been distracted with travel and now something in day to day.

blitzerr (Mar 26 2019 at 03:10, on Zulip):

If it is okay with you, can you please share your branch and then describe your problem/ concerns / approaches or proposals in a paper doc?
Sometimes it better to read through a concise doc rather than chat messages back and forth or I will try to read the chat to understand

ange (Mar 26 2019 at 12:25, on Zulip):

@blitzerr no worries, I'm also on the road till thursday. I hope the comment above will be enough for today's discussion. started porting the exploratory code on top of latest master, but that's WIP that I won't have time to finish today

ange (Mar 29 2019 at 22:08, on Zulip):

started looking at the MIR side, to get some better understanding of how things fit together

ange (Mar 29 2019 at 22:08, on Zulip):

might inform implementation choices for how to record paths, etc

ange (Apr 08 2019 at 20:58, on Zulip):

FWIW, put up some notes on what I've been looking up here. They refer to this exploratory branch cc @blitzerr @csmoe @nikomatsakis, in case anyone wants to take a look

Last update: Nov 17 2019 at 08:25UTC