Stream: t-compiler/wg-nll

Topic: #52663-span-decl-captured-variables


davidtwco (Aug 07 2018 at 12:09, on Zulip):

Looking at this diagnostic improvement. This is the code that produces the error. I figured that I'd look up the upvar_decls to find the upvar and then get the span. However, normally I'd use place.is_upvar_field_projection(..) or look for the upvar index manually but I can't quite work it out. Here's the gist - I'd normally expect one of the regions to be external but I might be getting mixed up. Thoughts @nikomatsakis?

nikomatsakis (Aug 07 2018 at 12:10, on Zulip):

/me looks

nikomatsakis (Aug 07 2018 at 12:11, on Zulip):

ok so let's look at this code...

nikomatsakis (Aug 07 2018 at 12:12, on Zulip):

this part is just checking that we have self.foo for some field

nikomatsakis (Aug 07 2018 at 12:12, on Zulip):

that .. seems a bit bogus to me, I think it should be using is_upvar_field_projection, as you said

davidtwco (Aug 07 2018 at 13:11, on Zulip):

Sorry, had to run for a moment there, back now.

davidtwco (Aug 07 2018 at 13:14, on Zulip):

What I was finding is that inside that match arm, is_upvar_field_projection was returning None and therefore I wasn't able to get the definition span for the upvar.

nikomatsakis (Aug 07 2018 at 13:14, on Zulip):

er sorry got distracted myself

nikomatsakis (Aug 07 2018 at 13:14, on Zulip):

hmm

davidtwco (Aug 07 2018 at 13:15, on Zulip):

Am I right in thinking that any upvars should have external regions in the MIR? This isn't related to how I want to tackle this issue, I just noticed that in the MIR, there wasn't a external region and that seemed strange.

nikomatsakis (Aug 07 2018 at 13:15, on Zulip):

not all upvars will have external regions

nikomatsakis (Aug 07 2018 at 13:16, on Zulip):

e.g., when capturing "by value"

davidtwco (Aug 07 2018 at 13:16, on Zulip):

Ah, I see.

davidtwco (Aug 07 2018 at 13:16, on Zulip):

That makes sense.

nikomatsakis (Aug 07 2018 at 13:16, on Zulip):

I'm confused why is_upvar_field_projection would not return true

davidtwco (Aug 07 2018 at 13:17, on Zulip):

I also found this function which seems to be equivalent to is_upvar_field_projection(..).is_some()?

nikomatsakis (Aug 07 2018 at 13:18, on Zulip):

yes

nikomatsakis (Aug 07 2018 at 13:18, on Zulip):

seems like there is some refactoring need here

davidtwco (Aug 07 2018 at 13:18, on Zulip):

(Since I ended up digging around related code figuring out what it used when looking up upvar_decls given that is_upvar_field_projection was returning None)

nikomatsakis (Aug 07 2018 at 13:18, on Zulip):

it seems like it'd be useful to dump out the place etc

nikomatsakis (Aug 07 2018 at 13:18, on Zulip):

maybe we are confused about something

davidtwco (Aug 07 2018 at 13:18, on Zulip):

Give me a second, I've got a build with some logging here.

nikomatsakis (Aug 07 2018 at 13:18, on Zulip):

ok

davidtwco (Aug 07 2018 at 13:20, on Zulip):

Here we go - that's the arguments and some of the intermediate variables of the report function I initially linked.

davidtwco (Aug 07 2018 at 13:20, on Zulip):

In particular, the (*_1) is the place.

davidtwco (Aug 07 2018 at 13:21, on Zulip):

Which makes sense, given that _1 is the captured y variable.

davidtwco (Aug 07 2018 at 13:21, on Zulip):

It's from bb0[1].

nikomatsakis (Aug 07 2018 at 13:21, on Zulip):

huh

nikomatsakis (Aug 07 2018 at 13:21, on Zulip):

let me check that MIR you sent

davidtwco (Aug 07 2018 at 13:22, on Zulip):

I'm probably missing something silly, but it made sense to me looking at it that is_upvar_field_projection should be a Some(..) with _1.

nikomatsakis (Aug 07 2018 at 13:22, on Zulip):

well is_upvar_field_projection wants something like (*_1).0

nikomatsakis (Aug 07 2018 at 13:22, on Zulip):

maybe we are feeding it just *_1?

davidtwco (Aug 07 2018 at 13:22, on Zulip):

Ah, that could be it.

nikomatsakis (Aug 07 2018 at 13:22, on Zulip):

in that case, there is no speific field

nikomatsakis (Aug 07 2018 at 13:22, on Zulip):

that is, the field index comes from the .0 part

nikomatsakis (Aug 07 2018 at 13:23, on Zulip):

maybe we have to modify the move code to give more information?

davidtwco (Aug 07 2018 at 13:23, on Zulip):

In this case there isn't a field I don't think.

nikomatsakis (Aug 07 2018 at 13:24, on Zulip):

well there must be a field

nikomatsakis (Aug 07 2018 at 13:24, on Zulip):

but it's not included in the information at hand

nikomatsakis (Aug 07 2018 at 13:25, on Zulip):

I'm looking a bit to see what would be the easiest way to fix that

davidtwco (Aug 07 2018 at 13:27, on Zulip):

Backtracking slightly, in this is_upvar function, on this line - there's an extra condition, that the upvar decl is by_ref - will that make it return differently from is_upvar_field_projection(..).is_some()?

nikomatsakis (Aug 07 2018 at 13:27, on Zulip):

I think that this is where the move errors are collected:

nikomatsakis (Aug 07 2018 at 13:27, on Zulip):

https://github.com/rust-lang/rust/blob/18925dee25ce649562d203e72068e3a57b60b153/src/librustc_mir/dataflow/move_paths/builder.rs#L409-L412

nikomatsakis (Aug 07 2018 at 13:28, on Zulip):

in particular, if we modified errors from Vec<MoveError<'tcx>> to Vec<(Place<'tcx>, MoveError<'tcx>)>

nikomatsakis (Aug 07 2018 at 13:28, on Zulip):

then we can keep along the original path

nikomatsakis (Aug 07 2018 at 13:28, on Zulip):

which is what we are missing here

davidtwco (Aug 07 2018 at 13:28, on Zulip):

Sounds good.

nikomatsakis (Aug 07 2018 at 13:28, on Zulip):

what winds up happening is that we convert a Place like (*_1).0 into a "move path"

nikomatsakis (Aug 07 2018 at 13:28, on Zulip):

this works recursively

nikomatsakis (Aug 07 2018 at 13:29, on Zulip):

so it will try to convert *_1

nikomatsakis (Aug 07 2018 at 13:29, on Zulip):

when that fails, we don't have enough context anymore

nikomatsakis (Aug 07 2018 at 13:29, on Zulip):

but if we keep the top-level path, that would be good

nikomatsakis (Aug 07 2018 at 13:29, on Zulip):

of course, then we might wind up with something like (*_1).0.1

nikomatsakis (Aug 07 2018 at 13:29, on Zulip):

but what we could do is to "peel off" projections from the original path

nikomatsakis (Aug 07 2018 at 13:29, on Zulip):

until we encounter something for which is_upvar_field_projection returns true

nikomatsakis (Aug 07 2018 at 13:30, on Zulip):

Backtracking slightly, in this is_upvar function, on this line - there's an extra condition, that the upvar decl is by_ref - will that make it return differently from is_upvar_field_projection(..).is_some()?

looking

davidtwco (Aug 07 2018 at 13:30, on Zulip):

I just spotted the .0 part of (*_1).0 in the MIR, I never read that properly before. :face_palm:

nikomatsakis (Aug 07 2018 at 13:30, on Zulip):

I think @davidtwco that the two functions are "morally" equivalent

nikomatsakis (Aug 07 2018 at 13:30, on Zulip):

you are correct that is_upvar is a bit ... more careful

nikomatsakis (Aug 07 2018 at 13:30, on Zulip):

however

nikomatsakis (Aug 07 2018 at 13:31, on Zulip):

in practice we will never generate an access to a by-ref upvar

nikomatsakis (Aug 07 2018 at 13:31, on Zulip):

that does not have a deref

davidtwco (Aug 07 2018 at 13:31, on Zulip):

Just checking before refactoring slightly to use is_upvar_field_projection throughout.

nikomatsakis (Aug 07 2018 at 13:31, on Zulip):

but really being careful is good

nikomatsakis (Aug 07 2018 at 13:31, on Zulip):

e.g., if we have a not by-ref upvar

nikomatsakis (Aug 07 2018 at 13:31, on Zulip):

then the MIR might be just (*_1).0

nikomatsakis (Aug 07 2018 at 13:31, on Zulip):

whereas for a by-ref upvar we would have *(*_1).0

nikomatsakis (Aug 07 2018 at 13:31, on Zulip):

(e.g. if the upvar has type i32 or something)

nikomatsakis (Aug 07 2018 at 13:32, on Zulip):

it might make sense to merge the two

davidtwco (Aug 07 2018 at 13:32, on Zulip):

Add in an assert to the main is_upvar_field_projection since it shouldn't ever happen?

nikomatsakis (Aug 07 2018 at 13:32, on Zulip):

that is, if you have some code like move || *x -- that will generate MIR like *(*_1).0

nikomatsakis (Aug 07 2018 at 13:32, on Zulip):

no, I think it can happen

nikomatsakis (Aug 07 2018 at 13:33, on Zulip):

is_upvar_field_projection will "incorrectly" categorize *(*_1).0 as a field projection

nikomatsakis (Aug 07 2018 at 13:33, on Zulip):

I say "incorrectly" because it may never matter ..

nikomatsakis (Aug 07 2018 at 13:33, on Zulip):

but it could lead us to print x somewhere where we should print *x for example

nikomatsakis (Aug 07 2018 at 13:33, on Zulip):

I think what i'm saying is that we should maybe modify is_upvar_field_projection to be the same as the other one

nikomatsakis (Aug 07 2018 at 13:34, on Zulip):

but then merge everything to call just is_upvar_field_projection

csmoe (Aug 07 2018 at 13:35, on Zulip):

@nikomatsakis sorry for cutting in, this is the "new" def of is_upvar_field_projection based on new Place

    pub fn is_upvar_field_projection<'cx, 'gcx>(
        &self,
        mir: &'cx Mir<'tcx>,
        tcx: &TyCtxt<'cx, 'gcx, 'tcx>,
    ) -> Option<Field> {
        match self.projection() {
            Some(ProjectionElem::Field(field, _ty)) => {
                let base_ty = self.ty(mir, *tcx).to_ty(*tcx);

                if  base_ty.is_closure() || base_ty.is_generator() {
                    Some(*field)
                } else {
                    None
                }
            },
            _ => None,
        }
    }
}
nikomatsakis (Aug 07 2018 at 13:36, on Zulip):

hmm what does self.projection() do?

csmoe (Aug 07 2018 at 13:36, on Zulip):

place.elems.last()

davidtwco (Aug 07 2018 at 13:36, on Zulip):

Does this mean we're in a race to see who will need to deal with the merge conflict of this change?

nikomatsakis (Aug 07 2018 at 13:36, on Zulip):

ok, that doesn't seem quite right then

nikomatsakis (Aug 07 2018 at 13:36, on Zulip):

in particular, the current one includes some logic to skip derefs:

       let place = if let Place::Projection(ref proj) = self {
            if let ProjectionElem::Deref = proj.elem {
                &proj.base
            } else {
                self
            }
        } else {
            self
        };
csmoe (Aug 07 2018 at 13:36, on Zulip):

eddyb told me that in new Place, the projection lives in the last elems of the placeelem slice

nikomatsakis (Aug 07 2018 at 13:37, on Zulip):

as I was just saying to @davidtwco, that logic is a bit aggressive

nikomatsakis (Aug 07 2018 at 13:37, on Zulip):

yes, so, I think that your version would work for e.g. a place like (*_1).0

nikomatsakis (Aug 07 2018 at 13:37, on Zulip):

but not *(*_1).0

nikomatsakis (Aug 07 2018 at 13:38, on Zulip):

I think it wants a kind of recursive call

nikomatsakis (Aug 07 2018 at 13:39, on Zulip):

well I'm not sure exactly how to write it but basically

nikomatsakis (Aug 07 2018 at 13:39, on Zulip):

if the final projection is a Deref

nikomatsakis (Aug 07 2018 at 13:40, on Zulip):

then we want to look at the penultimate projection and check if it is a field

nikomatsakis (Aug 07 2018 at 13:40, on Zulip):

@csmoe I forget, is it easy to make a Place for the "base" path?

nikomatsakis (Aug 07 2018 at 13:40, on Zulip):

in particular, we can "subslice" I guess, right?

nikomatsakis (Aug 07 2018 at 13:40, on Zulip):

i.e., we have some kind of (Base, &[Projections]) representation?

nikomatsakis (Aug 07 2018 at 13:40, on Zulip):

that would be useful here :)

csmoe (Aug 07 2018 at 13:41, on Zulip):

the tuple repr is easy to generate

csmoe (Aug 07 2018 at 13:43, on Zulip):

MirBorrowckCtxt has a base_path method

nikomatsakis (Aug 07 2018 at 13:43, on Zulip):

ok; it doesn't matter if it's a tuple or not per se

csmoe (Aug 07 2018 at 13:44, on Zulip):
impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
    // FIXME (#16118): function intended to allow the borrow checker
    // to be less precise in its handling of Box while still allowing
    // moves out of a Box. They should be removed when/if we stop
    // treating Box specially (e.g. when/if DerefMove is added...)

    fn base_path<'d>(&self, place: &'d Place<'tcx>) -> &'d Place<'tcx> {
        //! Returns the base of the leftmost (deepest) dereference of an
        //! Box in `place`. If there is no dereference of an Box
        //! in `place`, then it just returns `place` itself.

        let mut cursor = place;
        let mut deepest = place;
        loop {
            let proj = match *cursor {
                Place::Promo
nikomatsakis (Aug 07 2018 at 13:44, on Zulip):

my main point was that if you have e.g. (B, [P1, P2]) -- i.e., a base with two projections

nikomatsakis (Aug 07 2018 at 13:44, on Zulip):

we want to cheaply be able to make a Place like (B, [P1])

nikomatsakis (Aug 07 2018 at 13:45, on Zulip):

or (B, [])

nikomatsakis (Aug 07 2018 at 13:45, on Zulip):

i.e., the prefix paths

nikomatsakis (Aug 07 2018 at 13:45, on Zulip):

anyway, did you understand my concern about your "translation" of is_upvar_field_projection?

csmoe (Aug 07 2018 at 13:48, on Zulip):

my is_upvar_field_projection just checks the "final" projection(c), it should cover like b in base.[a, b, c]?

nikomatsakis (Aug 07 2018 at 13:49, on Zulip):

yes, in particular when c is a deref

csmoe (Aug 07 2018 at 13:49, on Zulip):

for Place: B.[P1, P2] produce: B.[], B.[P1,], B.[P1, P2]?

nikomatsakis (Aug 07 2018 at 13:50, on Zulip):

when we capture upvars, if we capture them "by reference", then something like x translates to (e.g.) *self.x

nikomatsakis (Aug 07 2018 at 13:50, on Zulip):

for Place: B.[P1, P2] produce: B.[], B.[P1,], B.[P1, P2]?

well, that is just a generally useful thing to be able to do

nikomatsakis (Aug 07 2018 at 13:51, on Zulip):

in this case we want logic that's like, at a high-level

is_upvar_field_projection() {
    let mut place = self;
    if place is a deref projection {
        place = place.base_place();
    }

    if place is a field projection with index X {
        if place.base_place() has closure type {
            return Some(X)
       }
    }

    None
}
nikomatsakis (Aug 07 2018 at 13:51, on Zulip):

at least, that is the current fn

nikomatsakis (Aug 07 2018 at 13:51, on Zulip):

@davidtwco pointed out that there is another version that is a bit more careful around the deref

nikomatsakis (Aug 07 2018 at 13:52, on Zulip):

but it would look like

is_upvar_field_projection() {
    let mut place = self;
    let mut by_ref = false;

    if place is a deref projection {
        place = place.base_place();
        by_ref = true;
    }

    if place is a field projection with index X {
        if place.base_place() has closure type {
            if (upvar X is b reference) == by_ref {
                return Some(X)
             }
       }
    }

    None
}
nikomatsakis (Aug 07 2018 at 13:52, on Zulip):

in any case, if I understood your code, you were missing the "if place is a deref projection" part

csmoe (Aug 07 2018 at 13:53, on Zulip):

well, that is just a generally useful thing to be able to do

so what about create a shrink method for Place?

okay, that's clear. will fix it soon.

csmoe (Aug 07 2018 at 13:53, on Zulip):

then base_place() is B.[]?

nikomatsakis (Aug 07 2018 at 14:01, on Zulip):

@csmoe I wonder if we should make a split_projection method that returns Option<(Place<'tcx>, &PlaceProjection<'tcx>)>

nikomatsakis (Aug 07 2018 at 14:01, on Zulip):

analogous to https://doc.rust-lang.org/std/primitive.slice.html#method.split_last

nikomatsakis (Aug 07 2018 at 14:01, on Zulip):

anyway, this probably belongs in a different topic :)

nikomatsakis (Aug 07 2018 at 14:01, on Zulip):

at this point, anyway...

davidtwco (Aug 07 2018 at 16:49, on Zulip):

@nikomatsakis Submitted #53164.

nikomatsakis (Aug 07 2018 at 17:18, on Zulip):

@davidtwco can you easily test this?

nikomatsakis (Aug 07 2018 at 17:18, on Zulip):

(is the test case I want clear?)

davidtwco (Aug 07 2018 at 17:20, on Zulip):

Yeah, it doesn't work for that.

davidtwco (Aug 07 2018 at 17:24, on Zulip):

Perhaps it is better to do something along the lines of original_path.is_upvar_field_projection(self.mir, &self.tcx).or_else(|| { place.is_upvar_field_projection(self.mir, &self.tcx) }) rather than try modify original_path. Of course, the place that the function previously had wouldn't work as it only has (*_1) not (*_1).0.

nikomatsakis (Aug 07 2018 at 17:26, on Zulip):

well

nikomatsakis (Aug 07 2018 at 17:26, on Zulip):

what I had in mind was something like this:

nikomatsakis (Aug 07 2018 at 17:27, on Zulip):
place.prefixes().any(|p| is_upvar_field_projection(p))
nikomatsakis (Aug 07 2018 at 17:27, on Zulip):

though I don't know if the method prefixes exists :)

nikomatsakis (Aug 07 2018 at 17:27, on Zulip):

there is something more cumbersome:

davidtwco (Aug 07 2018 at 17:27, on Zulip):

Sadly, it does not.

nikomatsakis (Aug 07 2018 at 17:27, on Zulip):

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_mir/borrow_check/struct.MirBorrowckCtxt.html#method.prefixes

nikomatsakis (Aug 07 2018 at 17:27, on Zulip):

we could write prefixes of course :)

nikomatsakis (Aug 07 2018 at 17:28, on Zulip):

or just open code it...

davidtwco (Aug 07 2018 at 17:28, on Zulip):

So, it does exist?

davidtwco (Aug 07 2018 at 17:28, on Zulip):

I only looked on Place itself before saying it didn't.

nikomatsakis (Aug 07 2018 at 17:28, on Zulip):
let mut p = place;
loop {
    if p.is_upvar_field_project() {
        return true;
    }

    if let Projection(proj) = p {
       p = &proj.base;
    } else {
        return false;
    }
}
nikomatsakis (Aug 07 2018 at 17:29, on Zulip):

the prefixes method that exists is something like self.prefixes(place, PrefixSet::All)

nikomatsakis (Aug 07 2018 at 17:29, on Zulip):

where self is the MirBorrowckCtxt

davidtwco (Aug 07 2018 at 17:30, on Zulip):

Yeah, I'm trying that just now.

davidtwco (Aug 07 2018 at 17:42, on Zulip):

Seems to have done the job, adding that as a test case and updating the PR momentarily.

davidtwco (Aug 07 2018 at 17:50, on Zulip):

@nikomatsakis Fixed on PR.

davidtwco (Aug 07 2018 at 18:56, on Zulip):

@nikomatsakis Fixed PR again.

nikomatsakis (Aug 07 2018 at 18:57, on Zulip):

one annoying thing about the blue link

nikomatsakis (Aug 07 2018 at 18:57, on Zulip):

we need a link to the PR :)

davidtwco (Aug 07 2018 at 18:59, on Zulip):

I wonder if it can do two links?

davidtwco (Aug 07 2018 at 18:59, on Zulip):

It can.

nikomatsakis (Aug 07 2018 at 19:00, on Zulip):

I don't see any blue arrows in your zulip test...?

davidtwco (Aug 07 2018 at 19:22, on Zulip):

Fixed again - I've been awful for lots of minor travis failures of late.

davidtwco (Aug 07 2018 at 19:22, on Zulip):

Looks like simulacrum already got it.

davidtwco (Aug 13 2018 at 21:21, on Zulip):

This PR needs a retry @nikomatsakis.

davidtwco (Aug 13 2018 at 21:21, on Zulip):

#53164

davidtwco (Aug 13 2018 at 21:28, on Zulip):

Thanks.

davidtwco (Aug 14 2018 at 16:39, on Zulip):

Rebased this after the big PR landed.

davidtwco (Aug 15 2018 at 16:17, on Zulip):

@nikomatsakis can you quickly take a look at this again? It's just a rebase so nothing's really changed, just test output.

nikomatsakis (Aug 15 2018 at 16:31, on Zulip):

ok

Last update: Nov 22 2019 at 00:00UTC