Stream: t-compiler/wg-nll

Topic: #52708-new-place


csmoe (Jul 30 2018 at 13:35, on Zulip):

@nikomatsakis
as you suggested in https://github.com/rust-lang/rust/issues/52708#issue-344543133 , since the real def of ProjectionElem is ProjectionElem<'tcx, V, T>, so repr of Place will be:

struct Place<'tcx, V, T> {
    base: PlaceBase<'tcx>,
    elem: &'tcx &Slice<ProjectionElem<'tcx, V, T>>,
}

but Place<'tcx, V, T> is a bit untidy when the other objects wrap it like:

enum Abc<'tcx, V, T> {
    Foo(Place<'tcx, V, T>)
}

the new-intro'd V, T type parameter will break tons of code, I think it's unnecessary(wrong?).

(maybe lucky?) I find

/// Alias for projections as they appear in places, where the base is a place
/// and the index is a local.
pub type PlaceElem<'tcx> = ProjectionElem<'tcx, Local, Ty<'tcx>>;

already lives there, so

struct Place<'tcx> {
    ...
    elem: &'tcx Slice<PlaceElem<'tcx>>,
}

can hidden V, T. can PlaceElem be intro'd here?

nikomatsakis (Jul 30 2018 at 14:42, on Zulip):

why does Place need the V/T parameters?

nikomatsakis (Jul 30 2018 at 14:42, on Zulip):

in any case, I think a type alias is ok

csmoe (Jul 31 2018 at 00:07, on Zulip):

V, T come from ProjectionElem<'tcx, V, T>

nikomatsakis (Jul 31 2018 at 04:33, on Zulip):

right, but in the existing Place struct they are defined to e Place and Ty respectively, right?

nikomatsakis (Jul 31 2018 at 04:34, on Zulip):

are you saying that some places which used to use ProjectionElem with other values of V and T now want to use Place?

nikomatsakis (Jul 31 2018 at 04:35, on Zulip):

in other words, the existing Place uses this alias:

pub type PlaceProjection<'tcx> = Projection<'tcx, Place<'tcx>, Local, Ty<'tcx>>;

(as I think you already observed)

nikomatsakis (Aug 06 2018 at 14:39, on Zulip):

@csmoe what ever happened here? (My apologies if there is an open PR, I haven't swept those yet)

csmoe (Aug 06 2018 at 14:41, on Zulip):

@nikomatsakis I'm working on this right now, I asked help from eddyb in niko-missing time.

csmoe (Aug 07 2018 at 14:03, on Zulip):

@nikomatsakis thanks, your instructions will be addressed soon

csmoe (Aug 07 2018 at 14:05, on Zulip):

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

Option<(Place<'tcx>, &PlaceProjection<'tcx>)> should be Option<(Place, Slice<PlaceProjection>)>?

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

no

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

I meant that given e.g. (B, [P1, P2]) it might return ((B, [P1]), &P2)

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

i.e., it "peels off" the last projection

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

and returns it to you, along with the "base" place

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

i.e., the place without that projection

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

I suspect this is something we commonly want to do

csmoe (Aug 07 2018 at 14:15, on Zulip):

ooh, get it

csmoe (Aug 09 2018 at 10:09, on Zulip):

@nikomatsakis may I ask a bit stupid question(unrelated to this topic)? for single let x = 3, does x has a region(lifetime) in NLL? I get used to thinki that region only involves in borrow like let x = &3.

nikomatsakis (Aug 09 2018 at 10:11, on Zulip):

x does not have a region

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

you are correct that lifetimes are only for borrows

csmoe (Aug 10 2018 at 13:16, on Zulip):

PR https://github.com/rust-lang/rust/pull/53247
@nikomatsakis some lifetime conflicts caused by introduced TyCtxtfor interning will be addressed soon . and the place_conflicthttps://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/librustc_mir/borrow_check/places_conflict.rs#L32-L37 is simply translated, maybe I need to rewrite it with slice disjoint detection?

nikomatsakis (Aug 10 2018 at 13:17, on Zulip):

oh, nice :)

nikomatsakis (Aug 10 2018 at 13:17, on Zulip):

I'll take a look I guess

nikomatsakis (Aug 10 2018 at 13:17, on Zulip):

places_conflict ought to be something we can implement much more simply...

csmoe (Aug 10 2018 at 13:19, on Zulip):

btw, it's the first time I make such a big modification sine learning programming, could you give me some tips about the conflict detection since I'm a algorithm newbie?

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

like, how it works in general?

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

the idea is that we start with the base of the path and go forward, iirc

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

so e.g. if you have a.b.c and a.b.d.e

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

and we want to see if a borrow of a.b.c affects a write to a.b.d.e

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

we start with the local -- a in each case -- and we see that so far they appear to be overlapping paths

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

then we go to the next step: a.b in both cases

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

still overlapping

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

finally we get to a.b.c vs a.b.d

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

and there we see they are disjoint

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

(sometimes, though, the same field name may still be overlapping -- e.g., in a union)

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

er, sorry, different field names

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

e.g., if c and d are declared in the same union

csmoe (Aug 10 2018 at 13:22, on Zulip):

the lifetime conflict isn't solved now, so the ./x.py check cannot pass, but it's just complains lifetime conflicts, I have clean up all the other errors

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

ok

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

if you want

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

I can pull the branch and take a look

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

sometimes these are obvious when you've been hacking on the compiler daily for years ;)

csmoe (Aug 10 2018 at 13:26, on Zulip):

thanks, I will try to address conflict-detection.
yep, I wanna your pulling.

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

ok, will do

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

@csmoe ping — I haven't had any time to look at your PR, but I would likely be able to do so next week

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

have you had any time to look at it any more?

csmoe (Aug 16 2018 at 22:09, on Zulip):

@nikomatsakis I'm working on this daily, rewriting the old-recursive place check stuffs to iter as eddyb reviewed(it's a bit slow since I am not proficient in algorithms and the place check, but I made some progress). If I am stuck or it's done, i'll ping you/eddyb

nikomatsakis (Aug 21 2018 at 18:55, on Zulip):

ok, well, RustConf is over and I'm more available, so let me know if you want me to take another look

csmoe (Aug 22 2018 at 03:07, on Zulip):

@nikomatsakis updating: ./py check succeeded locally, taking care of ./x.py test --stage 1

csmoe (Aug 22 2018 at 07:16, on Zulip):

@nikomatsakis ./x.py test emitted abort panic stack overflow when compiling core :
pasted image
If you have time, may I have your review? here is my latest code https://github.com/rust-lang/rust/pull/53247/files
anyway, I'm self-reviewing to find the bug.

Matthew Jasper (Aug 22 2018 at 12:01, on Zulip):

@csmoe you could try compiling a minimal #![no_core]library to see if you can minimise the problem.

csmoe (Aug 22 2018 at 12:02, on Zulip):

@Matthew Jasper thanks

nikomatsakis (Aug 22 2018 at 17:41, on Zulip):

@csmoe ok, doing a local build now

nikomatsakis (Aug 24 2018 at 18:42, on Zulip):

@csmoe so I fixed one problem -- but there are more. See my comments here -- let me know if that makes any sense.

csmoe (Aug 24 2018 at 20:31, on Zulip):

@nikomatsakis thank you, I'll fix the rest and recheck the other potential bug with the new place workflow.
btw, would you mind sharing your stack overflow debugging approach generally?(or maybe this bug is just discovered by reviewing)

nikomatsakis (Aug 24 2018 at 20:31, on Zulip):

in this case, I did this:

nikomatsakis (Aug 24 2018 at 20:31, on Zulip):

first, ./x.py build ... -vv

nikomatsakis (Aug 24 2018 at 20:31, on Zulip):

this will cause it to print out the command it was using

nikomatsakis (Aug 24 2018 at 20:32, on Zulip):

something like this:

rustc command: "LD_LIBRARY_PATH"="/home/nmatsakis/versioned/rust-3/build/x86_64-unknown-linux-gnu/stage1/lib:/home/nmatsakis/versioned/rust-3/build/x86_64-unknown-linux-gnu/stage1-std/release/deps:/home/\
nmatsakis/versioned/rust-3/build/x86_64-unknown-linux-gnu/stage0/lib" "/home/nmatsakis/versioned/rust-3/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "--crate-name" "core" "libcore/lib.rs" "--crate-ty\
pe" "lib" "--emit=dep-info,link" "-C" "opt-level=2" "-C" "metadata=b33d847693b19528-rustc" "-C" "extra-filename=-b33d847693b19528" "--out-dir" "/home/nmatsakis/versioned/rust-3/build/x86_64-unknown-linux\
-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps" "--target" "x86_64-unknown-linux-gnu" "-C" "incremental=/home/nmatsakis/versioned/rust-3/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-li\
nux-gnu/release/incremental" "-L" "dependency=/home/nmatsakis/versioned/rust-3/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps" "-L" "dependency=/home/nmatsakis/versioned/\
rust-3/build/x86_64-unknown-linux-gnu/stage1-std/release/deps" "--cfg" "stage1" "--sysroot" "/home/nmatsakis/versioned/rust-3/build/x86_64-unknown-linux-gnu/stage1" "-Cprefer-dynamic" "-Cdebuginfo=1" "-C\
" "debug-assertions=y" "-C" "codegen-units=28" "-C" "link-args=-Wl,-rpath,$ORIGIN/../lib" "-Z" "force-unstable-if-unmarked" "-Dwarnings" "-Dbare_trait_objects"
nikomatsakis (Aug 24 2018 at 20:32, on Zulip):

kind of a pain to parse visually :)

nikomatsakis (Aug 24 2018 at 20:32, on Zulip):

in any case, then you find the place in there where the rustc executable is

nikomatsakis (Aug 24 2018 at 20:32, on Zulip):

and you put gdb --args in front of it (if using linux)

nikomatsakis (Aug 24 2018 at 20:32, on Zulip):

type r

nikomatsakis (Aug 24 2018 at 20:32, on Zulip):

when it crashes, you can run the up command various times to see the points in the cycle

nikomatsakis (Aug 24 2018 at 20:33, on Zulip):

you can also press bt but for overflow there tend to be a ton of frames :)

csmoe (Aug 24 2018 at 20:34, on Zulip):

really helps, thank you :)

nikomatsakis (Aug 24 2018 at 20:37, on Zulip):

if you want me to help with the next cycle you encounter, I can do so =)

csmoe (Aug 24 2018 at 20:39, on Zulip):

I'll try to resolve it, if stuck, will ping you here. really thanks :)

nikomatsakis (Aug 27 2018 at 19:39, on Zulip):

@csmoe how goes it?

csmoe (Aug 28 2018 at 14:29, on Zulip):

stack overflow cleared, trying to fix the broken MIr err

nikomatsakis (Aug 28 2018 at 14:35, on Zulip):

ok, I can try a local build and see what I see

nikomatsakis (Aug 28 2018 at 14:42, on Zulip):

well I can definitely reproduce the broken MIR =)

nikomatsakis (Aug 29 2018 at 21:47, on Zulip):

@csmoe I've been looking at your PR some

nikomatsakis (Aug 29 2018 at 21:48, on Zulip):

I am a bit worried about perf implications of some aspects of it (in particular, the modifications to MIR builder may be problematic since they seem to do a lot of interning)

nikomatsakis (Aug 29 2018 at 21:48, on Zulip):

but I guess let's get it working first :)

csmoe (Aug 30 2018 at 10:44, on Zulip):

(interning stuff like Place.field(tcx, ...))?

my self-review almost done, but still cannot capture the logic bug

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

Re: efficiency, my thought is that we may want to rework the MIR builder so it doesn't return a fully-formed Place but rather a kind of "place builder" that (using a Vec) that we can intern once we are done.

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

but let's find the logic bug first I think

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

I'm doing a bit of investigating on the side

nikomatsakis (Aug 30 2018 at 12:19, on Zulip):

this is where things go wrong:

DEBUG 2018-08-30T12:15:51Z: rustc_mir::borrow_check::nll::type_check: sanitize_place: Place { base: _1, elems: [Deref, Index(_17)] }
DEBUG 2018-08-30T12:15:51Z: rustc_mir::borrow_check::nll::type_check: prove_predicates(predicate=Binder(TraitPredicate(<&mut [u8] as marker::Copy>)), locations=Interesting(bb16[0]))
DEBUG 2018-08-30T12:15:51Z: rustc_mir::borrow_check::nll::type_check: prove_predicate(predicate=Binder(TraitPredicate(<&mut [u8] as marker::Copy>)), location=Interesting(bb16[0]))
nikomatsakis (Aug 30 2018 at 12:19, on Zulip):

if you look at the MIR, you will see that _1 has type &mut [u8]

nikomatsakis (Aug 30 2018 at 12:19, on Zulip):

the MIR in question is:

nikomatsakis (Aug 30 2018 at 12:19, on Zulip):
CheckedAdd(Place { base: _1, elems: [Deref, Index(_17)] }, const 1u8)
nikomatsakis (Aug 30 2018 at 12:19, on Zulip):

in particular, Place { base: _1, elems: [Deref, Index(_17)] } should not have the type &mut [u8] but rather [u8]

nikomatsakis (Aug 30 2018 at 12:20, on Zulip):

ah, I think the bug is in the changes to sanitize_place

nikomatsakis (Aug 30 2018 at 12:21, on Zulip):

it seems to just ignore the projections altogether now

nikomatsakis (Aug 30 2018 at 12:21, on Zulip):

hmm not quite

nikomatsakis (Aug 30 2018 at 12:21, on Zulip):

ps I see a lot of code like this:

        if !place.has_no_projection() {
            for elem in place.elems.iter(){ /* side effect */ }
        }
nikomatsakis (Aug 30 2018 at 12:21, on Zulip):

afaict it would be simpler to just do:

            for elem in place.elems.iter(){
nikomatsakis (Aug 30 2018 at 12:22, on Zulip):

since, if the vector is empty, that has no effect

nikomatsakis (Aug 30 2018 at 12:23, on Zulip):

anyway, I'm trying this diff and we'll see what effect it has

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

@csmoe ok I pushed a commit that seems to solve a lot of problems. I'll leave a few tips on the next problem in the PR I think

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

@nikomatsakis thanks, your tips will be addressed soon

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

did you ever add a split function to Place as I suggested?

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

I don't see one, so maybe not

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

I think it is what we need for the next problem :)

csmoe (Aug 30 2018 at 13:37, on Zulip):

I have added it, early named split_projection, now elem_base? let me have a look.

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

I have to look at the details of how you defined Place.

csmoe (Aug 30 2018 at 13:40, on Zulip):

oops, I removed split_projection in my latest commits, since eddyb said it is not needed from irc

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

I see

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

I disagree :)

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

but maybe

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

in many cases it is not

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

in this particular case, maybe

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

let me leave my comment anyway

csmoe (Aug 30 2018 at 13:40, on Zulip):

okay

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

I think ti'd be useful to be able to treat a Place as a tree or a slice

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

which seems like it should be possible if we set things up right

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

comment: https://github.com/rust-lang/rust/pull/53247/files#r214034747

csmoe (Aug 30 2018 at 13:54, on Zulip):

@nikomatsakis btw, If you have extra time, could you take a look at the place_conflict diffs https://github.com/rust-lang/rust/pull/53247/files#diff-97da28e7b634e3c63bd18a6dc618d962 ?(maybe you have already reviewed that).

csmoe (Aug 30 2018 at 13:57, on Zulip):

my new conflict checking workflow:
1. checking base
2. if base EqOrDisjoint, then process to elems
3. if base Disjoint, return true immediately

csmoe (Aug 30 2018 at 13:58, on Zulip):

I wanna your review since I feel a bit mess when rewrite that

nikomatsakis (Aug 30 2018 at 14:09, on Zulip):

ok, will review

csmoe (Sep 06 2018 at 08:37, on Zulip):

update: stuck by const_err
pasted image

nikomatsakis (Sep 06 2018 at 09:50, on Zulip):

@csmoe I've been meaning to come back to this branch. I'm mildly worried that we should see if we can find a way to take another, less disruptive stab at this refactoring... do you have new commits I should pull, in any case?

csmoe (Sep 06 2018 at 09:51, on Zulip):

@nikomatsakis yes, latest commits had been pushed

csmoe (Sep 06 2018 at 09:58, on Zulip):

@nikomatsakis

I'm mildly worried that we should see if we can find a way to take another

yep, I think so, the master has merged several Place related PRs and my commits are very hybrid.
(really regretted that i hadn't followed the change-check-pass-change commit workflow your told me in the DebrjinIndex PR)

nikomatsakis (Sep 06 2018 at 09:59, on Zulip):

I have to think about what those gradual steps are perhaps

nikomatsakis (Sep 06 2018 at 09:59, on Zulip):

one thing I could imagine: not changing anything about the actual Place structure, just adding methods that convert a Place into the new format, and then converting the algorithms one by one

nikomatsakis (Sep 06 2018 at 10:00, on Zulip):

the truth is, these sorts of refactorings are hard to pull off :)

nikomatsakis (Sep 06 2018 at 10:00, on Zulip):

I don't see any new commits btw...? the last I see is 552a935f26, is that right?

nikomatsakis (Sep 06 2018 at 10:00, on Zulip):

In any case, I got to go, bbl

csmoe (Sep 06 2018 at 10:02, on Zulip):

btw, as eddyb mentioned in IRC, he wanna refactor Derefout of Place

csmoe (Sep 06 2018 at 10:02, on Zulip):

yes 552a935

nikomatsakis (Sep 06 2018 at 10:28, on Zulip):

btw, as eddyb mentioned in IRC, he wanna refactor Derefout of Place

yes, I know. That's.. a much bigger job.

nikomatsakis (Sep 11 2018 at 19:49, on Zulip):

@csmoe you around btw? we should maybe reserve a time to dig into this together and decide what next steps are

csmoe (Sep 12 2018 at 12:22, on Zulip):

@nikomatsakis i'll be here about 1 day later. will ping you then

csmoe (Sep 13 2018 at 13:01, on Zulip):

@nikomatsakis ping, I'm back

nikomatsakis (Sep 13 2018 at 13:07, on Zulip):

hey @csmoe — I'll be back in a few minutes :)

nikomatsakis (Sep 13 2018 at 13:39, on Zulip):

ok, back now @csmoe, let's talk briefly...

nikomatsakis (Sep 13 2018 at 13:40, on Zulip):

so I think we both agree that the existing PR is sort of doing too much in one bite, right?

nikomatsakis (Sep 13 2018 at 13:40, on Zulip):

still, it's quite useful as it contains some version of all the things we're going to have to do (well, most)

nikomatsakis (Sep 13 2018 at 13:41, on Zulip):

so we can kind of skim over it and see what the steps are and if there is a way to order them so that we can test them more piece-by-piece

nikomatsakis (Sep 13 2018 at 13:42, on Zulip):

there is one thing that I think we are missing still, which is that I think we may want to modify MIR construction so that we don't intern every little "in between place", but rather accumulate a full place and then intern the end result

nikomatsakis (Sep 13 2018 at 13:43, on Zulip):

so we can kind of skim over it and see what the steps are and if there is a way to order them so that we can test them more piece-by-piece

oh, and I think it's useful for another reason: I strongly suspect we'll be kind of copying over these hunks as we go

nikomatsakis (Sep 13 2018 at 13:45, on Zulip):

so what are the pieces?

nikomatsakis (Sep 13 2018 at 13:45, on Zulip):

I think that's it?

nikomatsakis (Sep 13 2018 at 13:46, on Zulip):

I think where I would start is by creating an alternative place, let's call it PlaceSlice<'tcx> or Place2 or something; this defines the new representation

nikomatsakis (Sep 13 2018 at 13:46, on Zulip):

then I would have a way to convert from the existing Place into that form

csmoe (Sep 13 2018 at 13:46, on Zulip):

impl From<Place> for NewPlace like?

nikomatsakis (Sep 13 2018 at 13:46, on Zulip):

then I would go through and find the algorithms you had to rewrite — starting with the ones (like places_conflict) that work better using iteration

nikomatsakis (Sep 13 2018 at 13:47, on Zulip):

impl From<Place> for NewPlace like?

well, I was thinking more like place.as_new_place(tcx)

nikomatsakis (Sep 13 2018 at 13:47, on Zulip):

or tcx.as_new_place(place)

nikomatsakis (Sep 13 2018 at 13:47, on Zulip):

the reason being you will want the tcx to do interning

nikomatsakis (Sep 13 2018 at 13:47, on Zulip):

this basically lets us add the "new repr + intern" code without breaking anything yet :)

nikomatsakis (Sep 13 2018 at 13:47, on Zulip):

then we can go to each algorithm, 1 by 1, and have them convert from "old place" to "new place"

nikomatsakis (Sep 13 2018 at 13:47, on Zulip):

(it's slow, but so what)

nikomatsakis (Sep 13 2018 at 13:48, on Zulip):

once you've converted an algorithm, you can then run tests, everything should still work

nikomatsakis (Sep 13 2018 at 13:48, on Zulip):

eventually we should be able to remove all uses of the original place

nikomatsakis (Sep 13 2018 at 13:49, on Zulip):

I would probably do this by making the details of the struct private or something, running x.py check, find a place that breaks. Make it public again, but change that place. Then repeat.

nikomatsakis (Sep 13 2018 at 13:49, on Zulip):

at that point we'll have a slow compiler but a working on that basically uses the new places only

csmoe (Sep 13 2018 at 13:49, on Zulip):

okay, get it.
I'll start working on that with change-test-change flow tomorrow(midnight now in utc+8)

nikomatsakis (Sep 13 2018 at 13:49, on Zulip):

finally then we can figure out the MIR construction piece and just make the new places from the start

nikomatsakis (Sep 13 2018 at 13:49, on Zulip):

ok, let me know how it goes

csmoe (Sep 13 2018 at 13:50, on Zulip):

thank you :)

nikomatsakis (Sep 13 2018 at 13:50, on Zulip):

also, once you've got the first commit (the one that just adds a new kind of place but doesn't really use it yet), ping me so I can take a look :)

csmoe (Sep 13 2018 at 13:51, on Zulip):

okay.

nikomatsakis (Sep 13 2018 at 13:51, on Zulip):

cool!

nikomatsakis (Sep 13 2018 at 13:51, on Zulip):

PS I will just share with you one concern. I like 99.5% sure that — once we're done — this will be a better way to represent places. but there is the chance we'll decide otherwise. ;)

nikomatsakis (Sep 13 2018 at 13:51, on Zulip):

(I am particularly confident having seen your PR, a lot of the algorithms and code felt more natural)

nikomatsakis (Sep 13 2018 at 13:52, on Zulip):

(and I think we can trivially recover the "recursive" style of Place as well, so we've lost nothing)

nikomatsakis (Sep 13 2018 at 13:52, on Zulip):

anyway, that's always the risk with refactorings I guess though

csmoe (Sep 13 2018 at 13:56, on Zulip):

a bit sorry that I can't give you some more constructive reply as a newbie, but I'll try my best to land your instructions, at least.

nikomatsakis (Sep 13 2018 at 13:59, on Zulip):

I'm mostly just prep'ing you :)

nikomatsakis (Sep 13 2018 at 13:59, on Zulip):

that is, warning you

nikomatsakis (Sep 13 2018 at 13:59, on Zulip):

but I figure, either way, you got a tour of the codebase

nikomatsakis (Sep 13 2018 at 13:59, on Zulip):

:P

nikomatsakis (Sep 13 2018 at 14:00, on Zulip):

as I said, i'm pretty sure we want to go in this direction

csmoe (Sep 13 2018 at 14:01, on Zulip):

yep :)

csmoe (Sep 14 2018 at 07:40, on Zulip):

@nikomatsakis
update: introduce new place definition
https://github.com/csmoe/rust/commit/fde10d88700968e35af4cac0ab9ad2c724dc9635

nikomatsakis (Sep 14 2018 at 14:40, on Zulip):

@csmoe looking good =) we probably want to write the code to convert from Place to NeoPlace I guess :)

csmoe (Sep 15 2018 at 07:36, on Zulip):

@nikomatsakis
update: implement .as_new_place for TyCtxt
https://github.com/csmoe/rust/commit/ea0d974ce01abc8f2c09b8f9516f9e8fc109b1e9

nikomatsakis (Sep 15 2018 at 10:16, on Zulip):

@csmoe I noticed one bug, left a comment

csmoe (Sep 15 2018 at 10:42, on Zulip):

@nikomatsakis okay fixed, then I'll "copy" the methods of old_place to NeoPlace.

nikomatsakis (Sep 18 2018 at 18:57, on Zulip):

@csmoe is the comment on https://github.com/rust-lang/rust/pull/53247 just antiquated?

csmoe (Sep 19 2018 at 12:19, on Zulip):

@nikomatsakis yes

csmoe (Sep 24 2018 at 09:06, on Zulip):

@nikomatsakis
for some data structures wrap Place inside them, like T(Place), should I replace inner Place with NeoPlace or just leave the def unchanged and convert it to NeoPlace later in every method acted on it?

   fn foo() {
        if let T(place) = t {
            tcx.as_new_place(place);
            ...
         }
    }

EDIT: i'd take the later approach since early changes on def is kind of breaking change, it's hard to maintain.

nikomatsakis (Sep 24 2018 at 15:55, on Zulip):

@csmoe sounds wise... how goes?

csmoe (Sep 24 2018 at 15:56, on Zulip):

goes well locally

csmoe (Sep 26 2018 at 06:13, on Zulip):

@nikomatsakis for the inelegant .has_no_projection() checking you said when we wanna match with place without projection, how about add a helper as:

impl NeoPlace {
   // Base.[]
  //       ^^ no projection
   fn bare_place(&self) -> Option<PlaceBase> {
        if self.elems.is_empty() {
             Some(self.base)
        } else {
             None
        }
   }
}

then the old matching
if let Some(Place::Local(...)) = old_place
=>
if let Some(PlaceBase::Local) = neo_place.bare_place()

csmoe (Sep 26 2018 at 10:41, on Zulip):

ok, CI said yes about this, I'll move forward, if you have any suggestions, i'll be back to fixup

nikomatsakis (Sep 26 2018 at 16:16, on Zulip):

how about add a helper as:

@csmoe sounds reasonable :)

nikomatsakis (Sep 28 2018 at 19:37, on Zulip):

@csmoe left a comment on the PR

csmoe (Sep 29 2018 at 12:45, on Zulip):

@nikomatsakis an unrelated question:
why this snippet failed as Eq seems already implemented for fn? cc https://doc.rust-lang.org/std/cmp/trait.Eq.html#implementors

fn foo(i: i32) -> i32 {
    1
}
fn bar(i: i32) -> i32 {
    1
}

fn main() {
    let x = foo;
    let y = bar;
    if x == y {}
}
error[E0369]: binary operation `==` cannot be applied to type `fn(i32) -> i32 {foo}`
  --> src/main.rs:12:8
   |
12 |     if x == y {}
   |        ^^^^^^
   |
   = note: an implementation of `std::cmp::PartialEq` might be missing for `fn(i32) -> i32 {foo}`

https://play.rust-lang.org/?gist=5b2ba6a3308e2ff369e67348c8645023&version=nightly&mode=debug&edition=2018

Gabriel Majeri (Sep 29 2018 at 14:12, on Zulip):

Hmm, this seems dubious. AFAIK Rust does not guarantee that different functions won't have the same address. So maybe this is intended, to avoid bugs?

memoryruins (Sep 29 2018 at 16:41, on Zulip):

it compiles if we annotate x and y (and correctly panics on the assert_eq) https://play.rust-lang.org/?gist=466217d00a9cb33e9f5156046c85603a&version=nightly&mode=debug&edition=2018

memoryruins (Sep 29 2018 at 17:38, on Zulip):

compiling on release causes them to have the same address, but the assert will still panic

memoryruins (Sep 30 2018 at 03:52, on Zulip):

wait, adding a println doesnt make the assert panic though? :thinking: https://play.rust-lang.org/?gist=ba8b4da114bac52f4e94eaaa0d8f1e98&version=nightly&mode=release&edition=2018

memoryruins (Sep 30 2018 at 04:13, on Zulip):

an issue was opened after tossing into another chat https://github.com/rust-lang/rust/issues/54685

csmoe (Sep 30 2018 at 05:05, on Zulip):

@Gabriel Majeri @memoryruins thanks, it makes no sense to compare two pointers(raw address), as ishitatsuyuki said:

We run mergefunc pass on release builds. Anyway, can you explain why this is a problem? Rust does not make any guarantee on pointer values.

I messed up the fn type and function pointer in the comparison

memoryruins (Oct 01 2018 at 16:41, on Zulip):

@csmoe fn pointers implement PartialEq so it's understandable to expect some consistency. the conversation on the thread has led to proposing a patch of llvm (it affects constant globals too)

csmoe (Oct 01 2018 at 16:43, on Zulip):

that's great:)

nikomatsakis (Oct 01 2018 at 17:49, on Zulip):

@csmoe the problem in your original example is that the values are not actually of fn type, but perhaps that was clarified for you already?

csmoe (Oct 02 2018 at 10:25, on Zulip):

yes

nikomatsakis (Oct 04 2018 at 20:15, on Zulip):

@csmoe how's it going?

csmoe (Oct 07 2018 at 12:03, on Zulip):

sorry for postponing, I have been preparing for an exam recently. will ping for review with commits.

csmoe (Oct 16 2018 at 16:45, on Zulip):

@nikomatsakis saw your comment there, I'm trapped in a China M.S. entrance exam(Dec. 22), so I cannot allocate enough time to the complicated parts but cleaning up small stuffs about neo_place when taking a break from the books.
I feel good with current incremental refactoring so far and beg your patience, this issue can be fixed within 1 week once I'm freed from the exam.
but if you wanna the place be replaced as soon as possible, splitting is fine.

nikomatsakis (Oct 16 2018 at 16:51, on Zulip):

nah it's fine, I was just worried you were not having time and I didn't want to see the PR flounder after you've put so much good work into it

nikomatsakis (Oct 16 2018 at 16:51, on Zulip):

I don't think it's urgent per se

csmoe (Oct 16 2018 at 16:52, on Zulip):

thank you :)

pnkfelix (Oct 16 2018 at 19:03, on Zulip):

(Probably better to wait until after October 25th to land the refactoring, IMO ... to avoid losing time rebasing last minute PRs for the edition ....)

nikomatsakis (Oct 16 2018 at 19:13, on Zulip):

it won't be ready by then anyway :)

Santiago Pastorino (Oct 16 2018 at 22:14, on Zulip):

@csmoe @nikomatsakis I'm interested in getting involved in this part of the code, if @csmoe is ok I can take a look and start to figure out what's going on and then if @csmoe wants I can try to help them, pair or something like that

csmoe (Oct 17 2018 at 04:21, on Zulip):

@Santiago Pastorino thank you
so let me take a summary here:
1. niko mentored me to refactor Place Take 2: step-by-step instead of replacing all its appearances Take 1: at once(this approach is really hard to maintain)
2. the Place itself and needed methods were already refactored for NeoPlace(it will be rename to Place finally), as you can check them in mir/mod.rs mir/tcx.rs, so you can pull my branch(I rebased it onto the latest master just now) and continue the following work.
3. the tasks left are just rewriting the original recursive operations on Place into iterative, as diff showed in the img pasted image
- btw, a common workflow is:
recur_logic(old_place)
=> let neo_place = tcx.as_new_place(old_place)
=> recur_logic_to_iter(neo_place)
4. when all the recursive stuffs are cleared, just delete old Place and rename NeoPlace to Place, finally clean up all the conversions by as_new_place,.

csmoe (Oct 17 2018 at 05:41, on Zulip):

for anyone interested in this, the remaining tasks are not hard based on my experience in take1, the most complicated parts are places_conflict and describe_place, but they're kind of easy with the existing comments/niko's reviews.

nikomatsakis (Oct 17 2018 at 13:20, on Zulip):

I think it's a great idea to collaborate here

nikomatsakis (Oct 17 2018 at 13:21, on Zulip):

there are a large-ish number of small changes to make, after all

Santiago Pastorino (Oct 17 2018 at 18:58, on Zulip):

:+1:

Santiago Pastorino (Oct 17 2018 at 18:58, on Zulip):

thanks for the info @csmoe

csmoe (Oct 20 2018 at 14:37, on Zulip):

@nikomatsakis I wanna your help in this diff
I've tried 4 times as the CI complained, but still failed at codegen testcases

nikomatsakis (Oct 22 2018 at 15:43, on Zulip):

@csmoe ok

nikomatsakis (Oct 23 2018 at 17:47, on Zulip):

looking now

nikomatsakis (Oct 23 2018 at 17:54, on Zulip):

left a comment with a possible source of the problem

nikomatsakis (Oct 24 2018 at 20:06, on Zulip):

left a new comment @csmoe

csmoe (Oct 25 2018 at 10:21, on Zulip):

@nikomatsakis it still breaks.

csmoe (Oct 25 2018 at 10:22, on Zulip):

the logic for scalar is wrong as the codegen/scalar-pair-bool.rs failed?

nikomatsakis (Oct 25 2018 at 12:32, on Zulip):

@csmoe did you push your latest?

csmoe (Oct 25 2018 at 12:55, on Zulip):

@nikomatsakis yes, it's latest

csmoe (Nov 18 2018 at 05:34, on Zulip):

@nikomatsakis just inform you, I'm back on this. but still cannot figure out what's wrong with codegen_llvm, so I'm gonna revert the diffs in src/codegen_llvm and move the refactoring direction to the other parts.

csmoe (Nov 19 2018 at 10:33, on Zulip):

update: rewrite places_confilct done

nikomatsakis (Nov 19 2018 at 17:32, on Zulip):

@csmoe ok -- as some point you and @Santiago Pastorino were talking about collaborating

nikomatsakis (Nov 19 2018 at 17:32, on Zulip):

I'm presuming that's not happened, but maybe it'd be useful?

nikomatsakis (Nov 19 2018 at 17:32, on Zulip):

sorry i've not had time to help out with debugging

nikomatsakis (Nov 19 2018 at 17:32, on Zulip):

but that could change

nikomatsakis (Nov 19 2018 at 17:32, on Zulip):

on wed is the code deadline for Rust 2018

nikomatsakis (Nov 19 2018 at 17:32, on Zulip):

I could prob spare a few hours after that

Santiago Pastorino (Nov 19 2018 at 17:41, on Zulip):

yes

Santiago Pastorino (Nov 19 2018 at 17:41, on Zulip):

I can collaborate

Santiago Pastorino (Nov 19 2018 at 17:41, on Zulip):

after I finish my assigned task I guess I can start helping

csmoe (Dec 05 2018 at 13:16, on Zulip):

@nikomatsakis
does upvardecl.by_ref mean?:

let closure = |_| {...};
let x = 1;
closure(&1);
        ^--by_ref?
nikomatsakis (Dec 05 2018 at 13:17, on Zulip):

nope :) that is not an upvar

nikomatsakis (Dec 05 2018 at 13:17, on Zulip):

a by-ref upvar might be foo in

let foo = 1;
let x = || println("{}", foo);
x();
csmoe (Dec 05 2018 at 13:18, on Zulip):

thanks, gocha :slight_smile:

blitzerr (Dec 05 2018 at 20:41, on Zulip):

Upvar is a variable declared before the closure definition. Hence, the name up-var, so you stand at the closure definition and look up, to find your up var :grinning:

nikomatsakis (Dec 06 2018 at 13:57, on Zulip):

@csmoe sorry been unresponsive this week -- how is the PR going? I'll try to check in and do some reviews this morning

csmoe (Dec 06 2018 at 13:57, on Zulip):

@nikomatsakis I have been working on this these days these days

nikomatsakis (Dec 06 2018 at 13:58, on Zulip):

great!

nikomatsakis (Dec 06 2018 at 13:58, on Zulip):

I remember @Santiago Pastorino was maybe going to poke at it too? Not sure if you ever found any way to divide work

csmoe (Dec 06 2018 at 13:58, on Zulip):

new place_conflict was impl'd

csmoe (Dec 06 2018 at 14:00, on Zulip):

but still stuck by describe_neo_place, it's a bit complicated. I am trying to impl the tree structure you mentioned

csmoe (Dec 06 2018 at 14:01, on Zulip):

so maybe the describe_place can be divided into sub-task

csmoe (Dec 06 2018 at 14:09, on Zulip):
base.[a, Deref, b, c]
=>
   Deref
    / \
   /   \
  a     b
         \
          \
           c

@nikomatsakis is this tree you expected?

nikomatsakis (Dec 06 2018 at 14:35, on Zulip):

I'm not sure @csmoe what this notation means :)

nikomatsakis (Dec 06 2018 at 14:35, on Zulip):

what is "base" here?

csmoe (Dec 06 2018 at 14:36, on Zulip):

PlaceBase::Local etc

csmoe (Dec 06 2018 at 14:36, on Zulip):

https://github.com/rust-lang/rust/pull/54426#discussion_r220255568
here is your review comment.

csmoe (Dec 06 2018 at 14:38, on Zulip):

same like comment for describe_place https://github.com/rust-lang/rust/pull/54426#discussion_r221360415

nikomatsakis (Dec 06 2018 at 14:44, on Zulip):

so assuming base is (e.g.) a local variable like x, I expect base.[a, Deref, b, c] to mean:

base
|
field: a
|
deref
|
b
|
c
nikomatsakis (Dec 06 2018 at 14:45, on Zulip):

or put in rust struct notation:

PlaceProjection {
  field: c,
  base: PlaceProjection {
    field: b,
    base: PlaceProjection {
      deref,
      base: PlaceProjection {
        field: a,
        base: PlaceBase {
            local: x
        }
      }
    }
  }
}
nikomatsakis (Dec 06 2018 at 14:45, on Zulip):

@csmoe :point_up:

csmoe (Dec 06 2018 at 14:50, on Zulip):

thanks, get the direction.

Santiago Pastorino (Dec 06 2018 at 15:00, on Zulip):

hey, yeah, we could see a way in which I can help

Santiago Pastorino (Dec 06 2018 at 15:01, on Zulip):

I guess, I'd need to catch up with what @csmoe is already doing

Santiago Pastorino (Dec 06 2018 at 15:01, on Zulip):

@nikomatsakis any ideas for that?

csmoe (Dec 06 2018 at 15:04, on Zulip):

@Santiago Pastorino I'll update the status tomorrow(it's local midnight)

Santiago Pastorino (Dec 06 2018 at 15:14, on Zulip):

:+1:

csmoe (Dec 19 2018 at 10:19, on Zulip):
pub fn capture_assign_part(x: (i32,)) {
        || { x.0 = 1; };
}

@davidtwco is x.0 a upvar_field_projection? is_upvar_field_projection

csmoe (Dec 19 2018 at 10:20, on Zulip):
NeoPlace { base: _1, elems: [Deref, Field(field[0], &mut (i32,)), Deref, Field(field[0], i32)] }

this is x.0 written in on-working new_place.

csmoe (Dec 19 2018 at 10:24, on Zulip):

CI was happy with my new is_upvar_field_projection, but while checking x.0, it failed.

davidtwco (Dec 19 2018 at 10:31, on Zulip):

I think so..

csmoe (Dec 19 2018 at 10:35, on Zulip):

okay, I'd recheck my refactoring of is_upvar_field_projection.

csmoe (Dec 19 2018 at 11:24, on Zulip):

@davidtwco here is your is_upvar_field_projection

pub fn is_upvar_field_projection<'cx, 'gcx>(
    &self,
    mir: &'cx Mir<'tcx>,
    tcx: &TyCtxt<'cx, 'gcx, 'tcx>
) -> Option<Field> {
    // 1st
    let (place, by_ref) = if let Place::Projection(ref proj) = self {
        if let ProjectionElem::Deref = proj.elem {
            (&proj.base, true)
        } else {
            (self, false)
        }
    } else {
        (self, false)
    };

    // 2nd
    match place {
        Place::Projection(ref proj) => match proj.elem {
            ProjectionElem::Field(field, _ty) => {
                let base_ty = proj.base.ty(mir, *tcx).to_ty(*tcx);

                if (base_ty.is_closure() || base_ty.is_generator()) &&
                    (!by_ref || mir.upvar_decls[field.index()].by_ref)
                {
                    Some(field)
                } else {
                    None
                }
            },
            _ => None,
        }
        _ => None,
    }
}

given x.0written as the old recursive Place:

Place {
    elem: Field2,
    base: Place {
        elem: Deref,  // ERROR: base_ty is_not_closure
        base: Place {
            elem: Field1,
            base: Place {
                elem: deref,
                base: Place {
                    base: Local(1)
                }
            }
        }
    }
}

while following your checking-flow, x.0 cannot be upvar_field_projection.

davidtwco (Dec 19 2018 at 11:27, on Zulip):

I was probably wrong then, it's been a while since I messed with is_upvar_field_projection.

csmoe (Dec 19 2018 at 12:10, on Zulip):

@nikomatsakis could you give me some tips about validating a placeis_upvar_field_projection with >=2 field projection?

csmoe (Dec 19 2018 at 12:10, on Zulip):

this current version seems only works for place with only ONE field

csmoe (Dec 19 2018 at 12:12, on Zulip):

as above, the base_ty of field2 is by-passed by field1, field1_ty shadows the inner most ty_of_Local(closure ty info lives here)

nikomatsakis (Dec 19 2018 at 20:09, on Zulip):

@csmoe hmm

nikomatsakis (Dec 19 2018 at 20:09, on Zulip):

are you saying you have a version that works but only for one field?

nikomatsakis (Dec 19 2018 at 20:09, on Zulip):

I should refresh my build of your branch I guess

csmoe (Dec 20 2018 at 15:23, on Zulip):

@nikomatsakis I have translated the original method with neo_place and replace all its usages with new one, CI was happy with the new one.
but while trying to check base.deref.field1.deref.field2, it failed.
so I suspect that there is something wrong with the original checking logic if we declare x.0 is upvar_field_projection.

fn is_upvar_field_projection() {
    let (place, by_ref) = if let Place::Projection(ref proj) = self {
           if let ProjectionElem::Deref = proj.elem {
            (&proj.base, true)
       }
    }
    Place::Projection(ref proj) => match proj.elem {
            ProjectionElem::Field(field, _ty) => {
                let base_ty = proj.base.ty(mir, *tcx).to_ty(*tcx);
   }
}

this flow works with Base.deref. field1.deref: unwarps deref and field1; base = Base.deref; base_ty = base.ty(...); the base_ty derive from Base holds the is_closure_generator info.

for Base.deref.field1.deref.field2: after unwrapping field2 and deref, base = base.deref.field1, base_ty = base.ty(..); but this case the base_ty from field1 lost the is_closure_generator info. so is_upvar_field_projection checking failed.

csmoe (Dec 20 2018 at 15:31, on Zulip):

anyway, the tldr above can be ignored. since CI was happy with is_upvar_field_projection and I found some trick to avoid is_upvar_field_projection checking with x.0.

nikomatsakis (Dec 20 2018 at 16:06, on Zulip):

@csmoe what is current status of this branch anyway? I'm sorry this turned into such a monster project :) should I be starting up doing reviews again?

csmoe (Dec 20 2018 at 16:13, on Zulip):

@nikomatsakis describe-place is finally done, but I'm sorry that I didn't follow the structure you suggested since that was kind new recursion while implementing it. my solution is a bit tricky. may i have your review on the latest 4 describe-place commits?

csmoe (Dec 20 2018 at 16:15, on Zulip):

and especially for the diffs in ui stderr, I made some changes on them.

csmoe (Dec 20 2018 at 16:18, on Zulip):

as for the status, I think the hardest time will be gone if you are happy with redescribe-place. the remaining work is easy.

nikomatsakis (Jan 02 2019 at 22:09, on Zulip):

@csmoe sorry for not doing those reviews yet, will try to do so asap

nikomatsakis (Jan 02 2019 at 22:09, on Zulip):

holidays intervened

nikomatsakis (Jan 02 2019 at 22:09, on Zulip):

@Santiago Pastorino and I were just discussing if collaboration would be helpful

Santiago Pastorino (Jan 02 2019 at 22:10, on Zulip):

sorry that I've promised to do so in the past and didn't have time

Santiago Pastorino (Jan 02 2019 at 22:10, on Zulip):

can be around now

Santiago Pastorino (Jan 02 2019 at 22:10, on Zulip):

need to catch up

csmoe (Jan 02 2019 at 22:15, on Zulip):

@nikomatsakis if possible, would you mind taking a look at the stderr diffs(made by redescribe-place) first?

csmoe (Jan 02 2019 at 22:17, on Zulip):

I changed some deref descriptions like:
assignment to *foo.bar => assignment to foo.bar

nikomatsakis (Jan 02 2019 at 22:17, on Zulip):

ok

nikomatsakis (Jan 03 2019 at 19:30, on Zulip):

@csmoe which commit is that in particular?

nikomatsakis (Jan 03 2019 at 19:30, on Zulip):

I am looking now

nikomatsakis (Jan 03 2019 at 19:36, on Zulip):

oh I guess " re-describe place "

nikomatsakis (Jan 03 2019 at 19:51, on Zulip):

left some comments

csmoe (Jan 04 2019 at 08:04, on Zulip):

@nikomatsakis amended some changes, reduced the stderr diffs to just 3 ref foocases.

csmoe (Jan 04 2019 at 08:10, on Zulip):

https://rust-lang.zulipchat.com/#narrow/stream/122657-wg-nll/subject/.2352708-new-place/near/151011215
here is your suggestions to me, while tried to implement that, I felt like introducing another recursive datastructure.

csmoe (Jan 04 2019 at 08:12, on Zulip):

(really sorry for "betray"ing)

csmoe (Jan 04 2019 at 08:41, on Zulip):

the key for describing Deref is detecting autoderef, which controls whether * should be print.

csmoe (Jan 04 2019 at 08:44, on Zulip):

my approach is for each Derefelem, it is autoderef if the future contains autoderef_elem:

let autoderef_elem = |&e| match e {
    ProjectionElem::Field(..)
    | ProjectionElem::Subslice { .. }
    | ProjectionElem::ConstantIndex { .. }
    | ProjectionElem::Index { .. } => true,
    _ => false,
};

let autoderef = place.elems[i+1..].iter().any(autoderef_elem);
csmoe (Jan 04 2019 at 08:49, on Zulip):

for example: base.[Deref1, Field, Deref2]:
1. base: place = base;
2. deref1: its next elem is Field, and autoderf = true, thus no *, place = base
3. field: place = base.field
4. deref2: autoderef = false, then * should be there, place = *base.field

csmoe (Jan 04 2019 at 08:53, on Zulip):

trick explanation:

csmoe (Jan 04 2019 at 08:58, on Zulip):
let no_local_name = if self.mir.local_decls.is_empty() {
    true
} else {
    if self.mir.local_decls.len() > 1 {
        self.mir.local_decls[Local::new(1)].name.is_none()
    } else {
        false
    }
};
if PlaceBase::Local(Local::new(1)) == place.base && no_local_name {
     // see [check-explanation] below.
} else {
    match place.base {
       Local(local) => local_str(local)? // Blame
   }
}

to be honest, this snippet is ugly.

[check-explanation]
all those check is for avoiding unindented panic when we cannot get the name from local_str()?
as you can see, I describe place from base to elems. when local_str()? panic,
describe_place will return None directly, but it should not in some cases.
in the original recursive flow, place is described from outer elems to base, although it cannot get a name from base, it still got a name from elem like Field.
so my check will skip Local base in those cases and just forward to elems description.

nikomatsakis (Jan 04 2019 at 19:04, on Zulip):

hmm @csmoe, I'll take another look. Would you object if I do a bit of experimentation and maybe push something?

csmoe (Jan 04 2019 at 19:27, on Zulip):

@nikomatsakis yes, whatever.

csmoe (Jan 04 2019 at 19:33, on Zulip):

btw, all the commits before the latest rec_lookup rewriting works fine(include re-describing).

nikomatsakis (Jan 10 2019 at 19:26, on Zulip):

@csmoe would you want to try and schedule a time to chat synchronously about this? Maybe with @Santiago Pastorino? could be a video call or else maybe chat on zulip

nikomatsakis (Jan 10 2019 at 19:27, on Zulip):

it's been really hard for me to carve out time to follow up and I'm sorry, which is partly why I'm thinking scheduling time might help :)

Santiago Pastorino (Jan 10 2019 at 19:31, on Zulip):

:+1:

Santiago Pastorino (Jan 10 2019 at 19:32, on Zulip):

it would be great so I can catch up

csmoe (Jan 11 2019 at 08:58, on Zulip):

@Santiago Pastorino it's 17:00pm now in my local time, I'll be available in next 5 hours. if you are here, just ping me.

csmoe (Jan 11 2019 at 08:58, on Zulip):

we'll chat here.

Santiago Pastorino (Jan 11 2019 at 13:09, on Zulip):

@csmoe hey, I’m not around, will be in 2hs but let’s coordinate a day and time

csmoe (Jan 11 2019 at 13:20, on Zulip):

@Santiago Pastorino okay, I see, I'll be available again 9 hours later, it'll be 19:20pm at your timezone.

Santiago Pastorino (Jan 11 2019 at 13:53, on Zulip):

during january I’m around between 12pm and 4pm GMT-3

Santiago Pastorino (Jan 11 2019 at 13:53, on Zulip):

:disappointed:

csmoe (Jan 11 2019 at 14:12, on Zulip):

@Santiago Pastorino so how about writing down what you wanna know here? I'll try my best to clearfy. if really needed, I'll align your 4 hours.

Santiago Pastorino (Jan 11 2019 at 14:19, on Zulip):

@csmoe just in case, right now I'm available but the idea was to make a call or chat with @nikomatsakis

Santiago Pastorino (Jan 11 2019 at 14:19, on Zulip):

@nikomatsakis should we coordinate?

Santiago Pastorino (Jan 11 2019 at 14:20, on Zulip):

sorry about my tight schedule during january but I'm on vacations with my family and doing some work on hours when the sun exposure would be bad :)

nikomatsakis (Jan 11 2019 at 20:19, on Zulip):

ok, I was thinking more like "some time next week" vs right now

nikomatsakis (Jan 11 2019 at 20:19, on Zulip):

in the limit, we could do a doodle

Santiago Pastorino (Jan 11 2019 at 20:19, on Zulip):

:+1:

nikomatsakis (Jan 11 2019 at 20:19, on Zulip):

maybe we should just do that

Santiago Pastorino (Jan 11 2019 at 20:20, on Zulip):

yes

Santiago Pastorino (Jan 11 2019 at 20:20, on Zulip):

I can build it if you want

nikomatsakis (Jan 11 2019 at 20:25, on Zulip):

@Santiago Pastorino I'd appreciate it =)

Santiago Pastorino (Jan 11 2019 at 20:25, on Zulip):

:+1:

Santiago Pastorino (Jan 11 2019 at 20:33, on Zulip):

https://doodle.com/poll/7mwdnxdpbtrv8f9n

Santiago Pastorino (Jan 11 2019 at 20:34, on Zulip):

@csmoe @nikomatsakis :point_up:

nikomatsakis (Jan 11 2019 at 21:38, on Zulip):

I filled it out

Santiago Pastorino (Jan 14 2019 at 15:24, on Zulip):

@csmoe remember to complete the doodle so we can see when to meet https://doodle.com/poll/7mwdnxdpbtrv8f9n

Santiago Pastorino (Jan 14 2019 at 15:32, on Zulip):

@nikomatsakis @csmoe there are 2 possibilities, today 1pm GMT-3, which is in half an hour or wednesday 1pm GMT-3

Santiago Pastorino (Jan 14 2019 at 15:32, on Zulip):

I'm fine with both, but unsure if doing a meeting in 30 minutes is a wise thing to do :P

Santiago Pastorino (Jan 14 2019 at 15:34, on Zulip):

let me know what do you think

csmoe (Jan 14 2019 at 15:39, on Zulip):

@Santiago Pastorino I'm fine with both :slight_smile:

csmoe (Jan 14 2019 at 15:42, on Zulip):

no matter how long the meeting will take, I'll be there.
a bit sorry for the time mismatching in the past days.

Santiago Pastorino (Jan 14 2019 at 15:42, on Zulip):

no worries

Santiago Pastorino (Jan 14 2019 at 15:43, on Zulip):

it's probably too late to let @nikomatsakis know about the meeting for today

Santiago Pastorino (Jan 14 2019 at 15:43, on Zulip):

I'd aim for wednesday 1pm GMT-3

Santiago Pastorino (Jan 14 2019 at 15:43, on Zulip):

but let's wait for @nikomatsakis

Santiago Pastorino (Jan 14 2019 at 16:13, on Zulip):

ok, definitely wednesday 1pm GMT-3

Santiago Pastorino (Jan 14 2019 at 16:13, on Zulip):

@csmoe @nikomatsakis :point_up:

csmoe (Jan 14 2019 at 16:14, on Zulip):

@Santiago Pastorino :time_ticking:

Santiago Pastorino (Jan 14 2019 at 16:15, on Zulip):

@csmoe I can send a calendar invite if you want

csmoe (Jan 14 2019 at 16:16, on Zulip):

@Santiago Pastorino yes, I need your ping.

Santiago Pastorino (Jan 14 2019 at 16:16, on Zulip):

give me your email address if you want that :)

csmoe (Jan 14 2019 at 16:16, on Zulip):

csmoe@msn.com

Santiago Pastorino (Jan 14 2019 at 16:17, on Zulip):

done @nikomatsakis @csmoe both invited to the calendar event

csmoe (Jan 14 2019 at 16:18, on Zulip):

:calendar:

Santiago Pastorino (Jan 15 2019 at 15:44, on Zulip):

@csmoe just in case, the meeting is confirmed for tomorrow (wednesday) 1pm GMT-3

Santiago Pastorino (Jan 15 2019 at 15:44, on Zulip):

Niko just confirmed that he is able

csmoe (Jan 15 2019 at 15:47, on Zulip):

@Santiago Pastorino gotcha

nikomatsakis (Jan 15 2019 at 16:06, on Zulip):

just to be clear, the meeting event is updated?

nikomatsakis (Jan 15 2019 at 16:06, on Zulip):

but yeah that time works for me

Santiago Pastorino (Jan 15 2019 at 16:07, on Zulip):

no, it's not updated just wednesday 1pm GMT-3

nikomatsakis (Jan 15 2019 at 16:07, on Zulip):

wait, it's not updated?

nikomatsakis (Jan 15 2019 at 16:08, on Zulip):

can we update it? :)

nikomatsakis (Jan 15 2019 at 16:08, on Zulip):

/me does some time zone arithmetic

nikomatsakis (Jan 15 2019 at 16:08, on Zulip):

my calendar shows 11 UTC-5, which -- I think -- is the same as 13 UTC-5 :P

nikomatsakis (Jan 15 2019 at 16:09, on Zulip):

but maybe I'm doing my math wrong or something

Jake Goulding (Jan 15 2019 at 18:13, on Zulip):

my calendar shows 11 UTC-5, which -- I think -- is the same as 13 UTC-5 :P

This is that new math I keep hearing about. (11, -5) =?= (13, -5)

Santiago Pastorino (Jan 15 2019 at 18:18, on Zulip):

11 UTC-5 = 13 UTC-3 (UTC-3 is my time zone)

Santiago Pastorino (Jan 15 2019 at 18:19, on Zulip):

11+5=13+3 :)

Jake Goulding (Jan 15 2019 at 18:33, on Zulip):

sure, but those aren't the numbers that were typed

Jake Goulding (Jan 15 2019 at 18:33, on Zulip):

pasted image

nikomatsakis (Jan 15 2019 at 19:25, on Zulip):

This is that new math I keep hearing about. (11, -5) =?= (13, -5)

die

nikomatsakis (Jan 15 2019 at 19:25, on Zulip):

you know what I meant :P

Santiago Pastorino (Jan 16 2019 at 15:59, on Zulip):

ping @csmoe @nikomatsakis meeting in 2 mins :)

csmoe (Jan 16 2019 at 15:59, on Zulip):

pong:)

Santiago Pastorino (Jan 16 2019 at 16:00, on Zulip):

@nikomatsakis are we having the meeting here or a video call?

csmoe (Jan 16 2019 at 16:02, on Zulip):

@Santiago Pastorino I'd like to chat here, it's midnight here, my little brother sleeping aside, sorry

Santiago Pastorino (Jan 16 2019 at 16:06, on Zulip):

:+1:, let's wait for @nikomatsakis then

Santiago Pastorino (Jan 16 2019 at 16:06, on Zulip):

meanwhile, can you explain more or less what the task was about and what have you done?

Santiago Pastorino (Jan 16 2019 at 16:06, on Zulip):

so meanwhile Niko is coming I can more or less catch up

csmoe (Jan 16 2019 at 16:08, on Zulip):

okay

csmoe (Jan 16 2019 at 16:10, on Zulip):

this PR is intended to change the representation of Place from recursive into iteration as the issue wrote https://github.com/rust-lang/rust/issues/52708

csmoe (Jan 16 2019 at 16:12, on Zulip):

niko mentored me to make it in an "incremental" way as:
1. introduce a temp place named NeoPlace

csmoe (Jan 16 2019 at 16:13, on Zulip):

2. then "mirror" all the methods associated to Place for NeoPlace(impl NeoPlace { ... })

csmoe (Jan 16 2019 at 16:14, on Zulip):

(this part was done)

Santiago Pastorino (Jan 16 2019 at 16:14, on Zulip):

:+1:

Santiago Pastorino (Jan 16 2019 at 16:14, on Zulip):

first thing I wonder is what comprehends a Place thing

Santiago Pastorino (Jan 16 2019 at 16:14, on Zulip):

from the docs it says

Santiago Pastorino (Jan 16 2019 at 16:14, on Zulip):
 /// A path to a value; something that can be evaluated without
/// changing or disturbing program state.
Santiago Pastorino (Jan 16 2019 at 16:15, on Zulip):

I wonder specifically about changing or disturbing program state

Santiago Pastorino (Jan 16 2019 at 16:15, on Zulip):

what's an example of a path that disturbs the program state?

Santiago Pastorino (Jan 16 2019 at 16:15, on Zulip):

I guess assigning a thing is not considered a Place?

Santiago Pastorino (Jan 16 2019 at 16:16, on Zulip):

I want to be 100% clear of what a Place is because I'm not 100% sure

csmoe (Jan 16 2019 at 16:16, on Zulip):
struct Foo {
    a: i32,
}
let foo = Foo { a: 1};
foo.a // place
csmoe (Jan 16 2019 at 16:16, on Zulip):

and things like array[2]

Santiago Pastorino (Jan 16 2019 at 16:16, on Zulip):

yeah

Santiago Pastorino (Jan 16 2019 at 16:17, on Zulip):

I mean

Santiago Pastorino (Jan 16 2019 at 16:17, on Zulip):

from

Santiago Pastorino (Jan 16 2019 at 16:17, on Zulip):
 pub enum Place<'tcx> {
     /// local variable
     Local(Local),

     /// static or static mut variable
     Static(Box<Static<'tcx>>),

     /// Constant code promoted to an injected static
     Promoted(Box<(Promoted, Ty<'tcx>)>),

     /// projection out of a place (access a field, deref a pointer, etc)
     Projection(Box<PlaceProjection<'tcx>>),
}
Santiago Pastorino (Jan 16 2019 at 16:17, on Zulip):

there are a lot of things that are clearly a Place

csmoe (Jan 16 2019 at 16:17, on Zulip):

I wonder specifically about changing or disturbing program state

I'm not sure about what this means, left to @nikomatsakis

Santiago Pastorino (Jan 16 2019 at 16:17, on Zulip):

I wonder where is exactly the line

nikomatsakis (Jan 16 2019 at 16:17, on Zulip):

argh argh

Santiago Pastorino (Jan 16 2019 at 16:17, on Zulip):

of things that seems like a Place but are not :)

nikomatsakis (Jan 16 2019 at 16:18, on Zulip):

I was in another meeting and it ran over

Santiago Pastorino (Jan 16 2019 at 16:18, on Zulip):

hey @nikomatsakis !!!

nikomatsakis (Jan 16 2019 at 16:18, on Zulip):

let me read the backlog

Santiago Pastorino (Jan 16 2019 at 16:18, on Zulip):

no worries

Santiago Pastorino (Jan 16 2019 at 16:18, on Zulip):

I was just asking silly questions ;)

nikomatsakis (Jan 16 2019 at 16:19, on Zulip):

ok

Santiago Pastorino (Jan 16 2019 at 16:19, on Zulip):

from the code I get that

nikomatsakis (Jan 16 2019 at 16:19, on Zulip):

regarding your question about "disturbing program state"

Santiago Pastorino (Jan 16 2019 at 16:19, on Zulip):
let a = 1;
nikomatsakis (Jan 16 2019 at 16:19, on Zulip):

I thnk the way to think of it is:

nikomatsakis (Jan 16 2019 at 16:19, on Zulip):

"places" are expressions

Santiago Pastorino (Jan 16 2019 at 16:19, on Zulip):

ok go ahead better :)

nikomatsakis (Jan 16 2019 at 16:19, on Zulip):

but not all expressions

nikomatsakis (Jan 16 2019 at 16:19, on Zulip):

so e.g. *x in rust surface syntax might or might not be a place, depending on whether * is overloaded

nikomatsakis (Jan 16 2019 at 16:19, on Zulip):

foo() is not a place expression

Santiago Pastorino (Jan 16 2019 at 16:20, on Zulip):

yep

nikomatsakis (Jan 16 2019 at 16:20, on Zulip):

another way to think about it is:

nikomatsakis (Jan 16 2019 at 16:20, on Zulip):

place expressions can be evaluated to a location in memory

Santiago Pastorino (Jan 16 2019 at 16:20, on Zulip):

it's an expression that leads to an area in the memory I guess?

nikomatsakis (Jan 16 2019 at 16:20, on Zulip):

something with an address

Santiago Pastorino (Jan 16 2019 at 16:20, on Zulip):

exactly :)

Santiago Pastorino (Jan 16 2019 at 16:20, on Zulip):

so

nikomatsakis (Jan 16 2019 at 16:20, on Zulip):

right, but foo().f in some sense leads to memory, but is not a place

Santiago Pastorino (Jan 16 2019 at 16:20, on Zulip):

a.b.c = "hi"

nikomatsakis (Jan 16 2019 at 16:20, on Zulip):

(As an aside, I actually have come to prefer the term path)

Santiago Pastorino (Jan 16 2019 at 16:20, on Zulip):

this is a place or not?

nikomatsakis (Jan 16 2019 at 16:21, on Zulip):

and to use place to refer to the memory location at runtime (which a path refers to)

nikomatsakis (Jan 16 2019 at 16:21, on Zulip):

bu aynway

Santiago Pastorino (Jan 16 2019 at 16:21, on Zulip):

right, but foo().f in some sense leads to memory, but is not a place

hmm I'd have said that it was a Place

Santiago Pastorino (Jan 16 2019 at 16:21, on Zulip):

why not ?

nikomatsakis (Jan 16 2019 at 16:21, on Zulip):

because to figure out where f is, you have to call foo()

nikomatsakis (Jan 16 2019 at 16:21, on Zulip):

in MIR terms, it is not a place

Santiago Pastorino (Jan 16 2019 at 16:21, on Zulip):

I see

Santiago Pastorino (Jan 16 2019 at 16:21, on Zulip):

and to use place to refer to the memory location at runtime (which a path refers to)

what's the difference you're seeing between a path and a place?

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

if you have e.g. &foo().f, we will compile that down to:

tmp0 = foo();
&tmp0.f
nikomatsakis (Jan 16 2019 at 16:22, on Zulip):

here, tmp0.f is a place

Santiago Pastorino (Jan 16 2019 at 16:22, on Zulip):

yes

Santiago Pastorino (Jan 16 2019 at 16:22, on Zulip):

makes sense

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

what's the difference you're seeing between a path and a place?

I mean the term Place as it is used today, in the MIR anyway, is the same as what I was calling Path

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

anyway ignore the Path thing perhaps, separate question

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

so the refactoring is, as @csmoe said, just about changing the representation of mir::Place

Santiago Pastorino (Jan 16 2019 at 16:22, on Zulip):

ok

Santiago Pastorino (Jan 16 2019 at 16:22, on Zulip):

last thing

Santiago Pastorino (Jan 16 2019 at 16:22, on Zulip):
 /// A path to a value; something that can be evaluated without
/// changing or disturbing program state.
Santiago Pastorino (Jan 16 2019 at 16:22, on Zulip):

what's the meaning of that?

nikomatsakis (Jan 16 2019 at 16:23, on Zulip):

isn't that what we were just talking about? :)

Santiago Pastorino (Jan 16 2019 at 16:23, on Zulip):

I mean, what's the changing and disturbing program state part?

nikomatsakis (Jan 16 2019 at 16:23, on Zulip):

evaluating a place means finding the location in memory it refers to

nikomatsakis (Jan 16 2019 at 16:23, on Zulip):

the reason that foo().f is not a place in this sense,

nikomatsakis (Jan 16 2019 at 16:23, on Zulip):

is that evaluating it would require invoking foo(),

nikomatsakis (Jan 16 2019 at 16:23, on Zulip):

which might mutate stuff

nikomatsakis (Jan 16 2019 at 16:23, on Zulip):

and "change or disturb program state"

Santiago Pastorino (Jan 16 2019 at 16:24, on Zulip):

I see :)

nikomatsakis (Jan 16 2019 at 16:24, on Zulip):

(in contrast, evaluating a.b.c. has no side-effects)

Santiago Pastorino (Jan 16 2019 at 16:24, on Zulip):

makes a lot of sense and now it's 100% clear what a Place is to me :)

nikomatsakis (Jan 16 2019 at 16:24, on Zulip):

OK so the goal was to change from the recursive representation that we use today

Santiago Pastorino (Jan 16 2019 at 16:24, on Zulip):

I've read the issue and got what you want to do

nikomatsakis (Jan 16 2019 at 16:24, on Zulip):

to a "flat" one

Santiago Pastorino (Jan 16 2019 at 16:24, on Zulip):

yeah :)

nikomatsakis (Jan 16 2019 at 16:24, on Zulip):

ok

Santiago Pastorino (Jan 16 2019 at 16:25, on Zulip):

what I'm not sure is what's the current state of that

nikomatsakis (Jan 16 2019 at 16:25, on Zulip):

this does however require rewriting the algorithms that use the representation today

Santiago Pastorino (Jan 16 2019 at 16:25, on Zulip):

and if you have any tip on how can I help

nikomatsakis (Jan 16 2019 at 16:25, on Zulip):

well, that is where @csmoe comes in, I am also a bit unclear on the current status

nikomatsakis (Jan 16 2019 at 16:25, on Zulip):

I'd also like us to discuss the plan to land this PR, which has been open a long time and is definitely a "merge conflict magnet"

Santiago Pastorino (Jan 16 2019 at 16:25, on Zulip):

this does however require rewriting the algorithms that use the representation today

yep, basically change the representations and see compilation and tests errors :')

nikomatsakis (Jan 16 2019 at 16:26, on Zulip):

maybe best to start with this question: @csmoe can you update us on sort of "where things are" -- how many bits of code do you think you have left to convert? are there particular bits of code that are still causing you trouble? etc

Santiago Pastorino (Jan 16 2019 at 16:27, on Zulip):

and I'd also like to know how can I help you @csmoe :)

csmoe (Jan 16 2019 at 16:27, on Zulip):

I have "mirror"ed the methods of Place, and rewrote the places_conflict detecting successfully.
so the next task is: describe the place in error reporting

Santiago Pastorino (Jan 16 2019 at 16:27, on Zulip):

if you want me to help thinking

Santiago Pastorino (Jan 16 2019 at 16:27, on Zulip):

if you want me to help coding :)

Santiago Pastorino (Jan 16 2019 at 16:27, on Zulip):

etc :)

nikomatsakis (Jan 16 2019 at 16:27, on Zulip):

I have "mirror"ed the methods of Place, and rewrote the places_conflict detecting successfully.
so the next task is: describe the place in error reporting

what do you mean by "mirror"?

csmoe (Jan 16 2019 at 16:28, on Zulip):

old_place.xxx() => neo_palce.xxx()

csmoe (Jan 16 2019 at 16:28, on Zulip):

I mean. :up:

Santiago Pastorino (Jan 16 2019 at 16:28, on Zulip):

I was also wondering about the old_place + neo_place approach

Santiago Pastorino (Jan 16 2019 at 16:28, on Zulip):

why not just changing the thing

Santiago Pastorino (Jan 16 2019 at 16:28, on Zulip):

@nikomatsakis I guess you had reasons that I don't see :)

csmoe (Jan 16 2019 at 16:29, on Zulip):

why not just changing the thing

nikomatsakis (Jan 16 2019 at 16:29, on Zulip):

because changing everything at once is untenable

nikomatsakis (Jan 16 2019 at 16:29, on Zulip):

you want to be able to change one piece of code without changing all the pieces of code

Santiago Pastorino (Jan 16 2019 at 16:29, on Zulip):

yeah, makes sense

Santiago Pastorino (Jan 16 2019 at 16:29, on Zulip):

at the end can just replace things and done

nikomatsakis (Jan 16 2019 at 16:29, on Zulip):

however, I'm not sure the neo-place approach itself was best. Or at least I was thinking of a different tactic.

nikomatsakis (Jan 16 2019 at 16:30, on Zulip):

I would really like it if we can get something that we can land

nikomatsakis (Jan 16 2019 at 16:30, on Zulip):

and have a "to do" list of other code to convert

Santiago Pastorino (Jan 16 2019 at 16:30, on Zulip):

and meanwhile having tiny parts which you can compile, test, etc

nikomatsakis (Jan 16 2019 at 16:30, on Zulip):

but not have to do everything in one PR

nikomatsakis (Jan 16 2019 at 16:30, on Zulip):

on the other hand, if @csmoe is already close enough, maybe it's not worth it, that's part of what I'm trying to judge

nikomatsakis (Jan 16 2019 at 16:30, on Zulip):

One question: @csmoe, at times we have discussed my idea to add some methods that you operate on the new "flat places" in a "tree-like way"

nikomatsakis (Jan 16 2019 at 16:31, on Zulip):

( I can try to make that more concrete for @Santiago Pastorino perhaps )

nikomatsakis (Jan 16 2019 at 16:31, on Zulip):

so basically the new representation would be:

struct Place<'tcx> {
  base: PlaceBase,
  projections: &'tcx [PlaceProjection<'tcx>],
}
csmoe (Jan 16 2019 at 16:32, on Zulip):

@nikomatsakis describe_place is the last biggest refactoring part.

csmoe (Jan 16 2019 at 16:32, on Zulip):

just go ahead :slight_smile:

nikomatsakis (Jan 16 2019 at 16:32, on Zulip):

and I was saying that you could imagine a method as_tree that returned a PlaceTree:

enum PlaceTree<'me, 'tcx> {
    Base(&'me PlaceBase),
    Proj(Place<'tcx>, &'tcx Projection),
}

This structure is basically the same as today's Place

nikomatsakis (Jan 16 2019 at 16:32, on Zulip):

as_tree might look like:

nikomatsakis (Jan 16 2019 at 16:34, on Zulip):
impl<'tcx> Place<'tcx> {
  pub fn as_tree<'me>(&'me self) -> PlaceTree<'me, 'tcx> {
    match self.projections.split_last() {
       None => PlaceTree::Base(&self.base),
       Some((last, rest)) => PlaceTree::Proj(Place { ...}, last),
  }
}
nikomatsakis (Jan 16 2019 at 16:34, on Zulip):

so basically if you had a place like a.b.c, which is now represented as (a, [b, c]), then as_tree would change that to PlaceTree::Proj((a, [b]), c)

nikomatsakis (Jan 16 2019 at 16:35, on Zulip):

the reason for this is:

Santiago Pastorino (Jan 16 2019 at 16:35, on Zulip):

yep impl PlaceTree :)

nikomatsakis (Jan 16 2019 at 16:35, on Zulip):

you can then convert old in a very mechanical way

nikomatsakis (Jan 16 2019 at 16:35, on Zulip):

i.e., where it used to descend down a mir::Place, it now just converts to a tree at each step

Santiago Pastorino (Jan 16 2019 at 16:35, on Zulip):

yep impl PlaceTree :)

ahh I what you meant

Santiago Pastorino (Jan 16 2019 at 16:36, on Zulip):

so Place is the new Place, PlaceTree is the old one

nikomatsakis (Jan 16 2019 at 16:36, on Zulip):

I imagine we could land a PR that (a) introduced the PlaceTree abstraction but leaves Place the same (in this case, PlaceTree is basically the same as place), then (b) change how places are build to build new-style places, but the analyses still work, then (c) convert the analyses one by one to not use as_tree where appropriate

Santiago Pastorino (Jan 16 2019 at 16:36, on Zulip):

and basically all over the code we call as_tree in places where we haven't converted the algorithms to use the new data type

nikomatsakis (Jan 16 2019 at 16:36, on Zulip):

right

nikomatsakis (Jan 16 2019 at 16:36, on Zulip):

part of the context here is that many analyses are easier with the new place

nikomatsakis (Jan 16 2019 at 16:36, on Zulip):

than the old place

nikomatsakis (Jan 16 2019 at 16:36, on Zulip):

but .. not necessarily all

nikomatsakis (Jan 16 2019 at 16:36, on Zulip):

e.g., describe-place might not fit this description

nikomatsakis (Jan 16 2019 at 16:37, on Zulip):

anyway, @csmoe, what do you think? I kind of think this approach would've been better in some sense, but maybe it's too late now? or still worth it?

nikomatsakis (Jan 16 2019 at 16:37, on Zulip):

(I imagine even if we did this, we can keep your existing branch and refer to it as we convert analyses, also)

nikomatsakis (Jan 16 2019 at 16:38, on Zulip):

and basically all over the code we call as_tree in places where we haven't converted the algorithms to use the new data type

well, as I said, in some cases, I think that the code might be nicer with as_tree anyway -- like, it's not bad to view places as a "tree"

csmoe (Jan 16 2019 at 16:38, on Zulip):

@nikomatsakis I think as_tree should be a helper for describing_place.

csmoe (Jan 16 2019 at 16:38, on Zulip):

based on my previous experience, the other parts works well with the new place

nikomatsakis (Jan 16 2019 at 16:38, on Zulip):

so one concernI have is:

nikomatsakis (Jan 16 2019 at 16:39, on Zulip):

with the PR as it is, I don't think we can land it until we convert place construction

nikomatsakis (Jan 16 2019 at 16:39, on Zulip):

because converting to a "neo-place" is expensive

csmoe (Jan 16 2019 at 16:39, on Zulip):

exactly

nikomatsakis (Jan 16 2019 at 16:39, on Zulip):

I think this alternate path I described could be landed as we go

nikomatsakis (Jan 16 2019 at 16:39, on Zulip):

and then we could port over the code from your branch once we have place construction in place

nikomatsakis (Jan 16 2019 at 16:39, on Zulip):

or we can keep rebasing your branch, and just get it done

nikomatsakis (Jan 16 2019 at 16:40, on Zulip):

I feel like either is ok, but if past experience is any guide, it's better to go with the incremental approach :)

Santiago Pastorino (Jan 16 2019 at 16:40, on Zulip):

with the PR as it is, I don't think we can land it until we convert place construction

I guess I can help shaping the PR

Santiago Pastorino (Jan 16 2019 at 16:40, on Zulip):

if that's desirable

Santiago Pastorino (Jan 16 2019 at 16:40, on Zulip):

like picking and tidying commits from @csmoe and adapting to this idea

nikomatsakis (Jan 16 2019 at 16:40, on Zulip):

I feel like either is ok, but if past experience is any guide, it's better to go with the incremental approach :)

by this I mean: it's always more work left than you think :)

csmoe (Jan 16 2019 at 16:41, on Zulip):

I kept rebasing my branch these days, there has no conflict now.

csmoe (Jan 16 2019 at 16:42, on Zulip):

so I prefer to follow my PR, if we clean up describe_place, it can be landed very soon.

nikomatsakis (Jan 16 2019 at 16:42, on Zulip):

ok

nikomatsakis (Jan 16 2019 at 16:42, on Zulip):

that seems reasonable too :)

Santiago Pastorino (Jan 16 2019 at 16:42, on Zulip):

:+1:

nikomatsakis (Jan 16 2019 at 16:42, on Zulip):

do you think that @Santiago Pastorino can help in any way?

Santiago Pastorino (Jan 16 2019 at 16:42, on Zulip):

exactly what I was going to ask :)

Santiago Pastorino (Jan 16 2019 at 16:43, on Zulip):

if I've helped by asking questions and having this meeting I'm perfectly fine :)

csmoe (Jan 16 2019 at 16:43, on Zulip):

so @Santiago Pastorino, I wanna your help for as_tree and describe_place

Santiago Pastorino (Jan 16 2019 at 16:43, on Zulip):

ok

Santiago Pastorino (Jan 16 2019 at 16:44, on Zulip):

@csmoe do you want me to try to give you some tips and guide if I can? or just coding something and ammending to your commits?

nikomatsakis (Jan 16 2019 at 16:44, on Zulip):

(I guess the other question is whether I can help at any place =)

Santiago Pastorino (Jan 16 2019 at 16:45, on Zulip):

:D

Santiago Pastorino (Jan 16 2019 at 16:45, on Zulip):

last question I have to be 100% sure what I need to do

Santiago Pastorino (Jan 16 2019 at 16:45, on Zulip):

I can pull @csmoe PR

nikomatsakis (Jan 16 2019 at 16:45, on Zulip):

I don't have much coding time, so it'd be more like "dear god Niko why have you not reviewed commit X yet"

nikomatsakis (Jan 16 2019 at 16:45, on Zulip):

an alternative would be

nikomatsakis (Jan 16 2019 at 16:46, on Zulip):

that I could do a pairing session with @Santiago Pastorino to kind of get them off int he right direction

Santiago Pastorino (Jan 16 2019 at 16:46, on Zulip):

and on top of that convert to use the Place and PlaceTree idea and provide as_tree fn

Santiago Pastorino (Jan 16 2019 at 16:46, on Zulip):

and then go and see the describe thing?

nikomatsakis (Jan 16 2019 at 16:46, on Zulip):

is there some amount of work on your branch, @csmoe, that we would want to revert? i.e., existing edits to describe_place?

csmoe (Jan 16 2019 at 16:46, on Zulip):

@Santiago Pastorino i prefer former. so if possible, may I have your mentorship?

Santiago Pastorino (Jan 16 2019 at 16:46, on Zulip):

that I could do a pairing session with @Santiago Pastorino to kind of get them off int he right direction

that would be nice

csmoe (Jan 16 2019 at 16:46, on Zulip):

@nikomatsakis the last one commit

Santiago Pastorino (Jan 16 2019 at 16:47, on Zulip):

@Santiago Pastorino i prefer former. so if possible, may I have your mentorship?

I don't have a solid experience on the compiler to mentor

Santiago Pastorino (Jan 16 2019 at 16:47, on Zulip):

but I can try to think and help you of course

csmoe (Jan 16 2019 at 16:47, on Zulip):

ci was happy with everything before re-describe-place

Santiago Pastorino (Jan 16 2019 at 16:47, on Zulip):

would be great if I can guide you :)

csmoe (Jan 16 2019 at 16:47, on Zulip):

@Santiago Pastorino thank you :heart:

nikomatsakis (Jan 16 2019 at 16:47, on Zulip):

ok, let's do this

Santiago Pastorino (Jan 16 2019 at 16:48, on Zulip):

mentoring seems like a too strong word for my compiler knowledge :blush:

nikomatsakis (Jan 16 2019 at 16:48, on Zulip):

@Santiago Pastorino you and I will schedule a slot to pair up -- @csmoe if you can come, great, but I'll record it too and put it on youtube -- and we'll try to get started

nikomatsakis (Jan 16 2019 at 16:48, on Zulip):

maybe we can find a time today?

nikomatsakis (Jan 16 2019 at 16:48, on Zulip):

it would have to be soon I realize

nikomatsakis (Jan 16 2019 at 16:48, on Zulip):

since I am a bit busy this afternoon

Santiago Pastorino (Jan 16 2019 at 16:48, on Zulip):

yes

Santiago Pastorino (Jan 16 2019 at 16:48, on Zulip):

I'm busy too

Santiago Pastorino (Jan 16 2019 at 16:49, on Zulip):

right now is 1.47pm

Santiago Pastorino (Jan 16 2019 at 16:49, on Zulip):

maybe now

csmoe (Jan 16 2019 at 16:49, on Zulip):

@Santiago Pastorino I am glad to have your seniors, you bring me to the programming world.

Santiago Pastorino (Jan 16 2019 at 16:49, on Zulip):

I have a meeting at 3pm

csmoe (Jan 16 2019 at 16:49, on Zulip):

it's midnight here. 00:49

Santiago Pastorino (Jan 16 2019 at 16:49, on Zulip):

and didn't have lunch yet

Santiago Pastorino (Jan 16 2019 at 16:49, on Zulip):

@nikomatsakis if you can now would be great

nikomatsakis (Jan 16 2019 at 16:50, on Zulip):

yeah now is ok

csmoe (Jan 16 2019 at 16:50, on Zulip):

if you can come, great, but I'll record it too and put it on youtube -- and we'll try to get started

csmoe (Jan 16 2019 at 16:50, on Zulip):

I will be there. :slight_smile:

nikomatsakis (Jan 16 2019 at 16:52, on Zulip):

give me five minutes

nikomatsakis (Jan 16 2019 at 16:52, on Zulip):

https://appear.in/i-heart-rust

Santiago Pastorino (Jan 16 2019 at 16:53, on Zulip):

yeah will be here in 2 mins

nikomatsakis (Jan 16 2019 at 17:00, on Zulip):

ssh etEC8yzGxp83SAlxPd9H9SOm7@ny2.tmate.io

nikomatsakis (Jan 16 2019 at 17:03, on Zulip):

whomever just ssh'd in, can you grow your screen? :)

nikomatsakis (Jan 16 2019 at 17:13, on Zulip):

@csmoe are you connected to the ssh? if so, can you make your terminal window bigger and/or font smaller? :)

Santiago Pastorino (Jan 16 2019 at 17:19, on Zulip):

hi?

nikomatsakis (Jan 16 2019 at 17:19, on Zulip):

@Santiago Pastorino lost you :)

csmoe (Jan 16 2019 at 17:31, on Zulip):

@nikomatsakis my ssh window frozen, is it safe to exit and reconnect?

nikomatsakis (Jan 16 2019 at 17:32, on Zulip):

@csmoe yes

csmoe (Jan 16 2019 at 17:39, on Zulip):

@nikomatsakis we have as_place_base in librustc/mir/tcx.rs

nikomatsakis (Jan 16 2019 at 17:40, on Zulip):

@csmoe ah ok =)

nikomatsakis (Jan 16 2019 at 17:45, on Zulip):

OK that work is at https://github.com/nikomatsakis/rust/tree/place2b

nikomatsakis (Jan 16 2019 at 17:46, on Zulip):

I think the next steps are:

One nit about the tree stuff was that, because PlaceBase is not Copy, it might require some cloning here and there to use the tree view, which is a bit less efficient. For describe_place, that doesn't matter, but it might be a problem elsewhere. I think I would prefer to fix this by making PlaceBase be Copy -- basically removing the Box and replacing with interned &'tcx pointers.

nikomatsakis (Jan 16 2019 at 17:46, on Zulip):

@csmoe that sound about right? :point_up:

csmoe (Jan 16 2019 at 17:46, on Zulip):

yep

nikomatsakis (Jan 16 2019 at 17:46, on Zulip):

I also recorded that session

nikomatsakis (Jan 16 2019 at 17:46, on Zulip):

I can post to YouTube

nikomatsakis (Jan 16 2019 at 17:46, on Zulip):

if desired

nikomatsakis (Jan 16 2019 at 17:47, on Zulip):

including my daughter's interruption ;)

csmoe (Jan 16 2019 at 17:47, on Zulip):

yes, youtube

csmoe (Jan 16 2019 at 17:47, on Zulip):

haha :slight_smile:

Santiago Pastorino (Jan 16 2019 at 18:16, on Zulip):

don't worry for me :)

Santiago Pastorino (Jan 16 2019 at 18:17, on Zulip):

maybe @csmoe want to watch, unsure if you were able to follow everything

Santiago Pastorino (Jan 16 2019 at 18:17, on Zulip):

I lost connection twice :P

Santiago Pastorino (Jan 16 2019 at 18:17, on Zulip):

@nikomatsakis @csmoe I won't be able to continue with this today

Santiago Pastorino (Jan 16 2019 at 18:17, on Zulip):

probably tomorrow

Santiago Pastorino (Jan 16 2019 at 18:18, on Zulip):

have a Rust Latam meeting now :)

nikomatsakis (Jan 18 2019 at 19:13, on Zulip):

how goes y'all?

Santiago Pastorino (Jan 18 2019 at 19:19, on Zulip):

hey Niko

Santiago Pastorino (Jan 18 2019 at 19:20, on Zulip):

promised doing something today, couldn't yet

Santiago Pastorino (Jan 18 2019 at 19:20, on Zulip):

gonna spend some time now

Santiago Pastorino (Jan 18 2019 at 19:20, on Zulip):

I have the thing compiling

Santiago Pastorino (Jan 18 2019 at 19:20, on Zulip):

looking for Place usages

Santiago Pastorino (Jan 18 2019 at 19:21, on Zulip):

basically I'm going to remove Place, make the thing compile and then I guess we can rename NeoPlace to Place

Santiago Pastorino (Jan 18 2019 at 19:21, on Zulip):

right?

csmoe (Jan 21 2019 at 11:19, on Zulip):

update: re-describe-place done

Santiago Pastorino (Jan 21 2019 at 12:36, on Zulip):

@csmoe have you pushed the code somewhere?

Santiago Pastorino (Jan 21 2019 at 12:36, on Zulip):

I've done a couple of things too

csmoe (Jan 21 2019 at 12:41, on Zulip):

@Santiago Pastorino the last two commits, I merged niko's work into the PR. and made some modifications.

csmoe (Jan 21 2019 at 12:41, on Zulip):

https://github.com/rust-lang/rust/pull/54426/commits/cba36331ef061a30bfaf8a3d30118ef10a06d4d2

Santiago Pastorino (Jan 21 2019 at 14:38, on Zulip):

@csmoe you around?

Santiago Pastorino (Jan 21 2019 at 14:38, on Zulip):

so ... should I just get what's here https://github.com/rust-lang/rust/pull/54426/ ?

Santiago Pastorino (Jan 21 2019 at 14:38, on Zulip):

and continue from there?

Santiago Pastorino (Jan 21 2019 at 14:38, on Zulip):

that branch is the same as Niko's branch or did you changed something else?

Santiago Pastorino (Jan 21 2019 at 14:39, on Zulip):

maybe could be good idea if you can give me commit access so we can keep stuff in sync

csmoe (Jan 21 2019 at 14:41, on Zulip):

okay, but how can i give the access to you?

Santiago Pastorino (Jan 21 2019 at 14:42, on Zulip):

you need to give me push access to csmoe/rust

Santiago Pastorino (Jan 21 2019 at 14:42, on Zulip):

that branch is the same as Niko's branch or did you changed something else?

:point_up:

Santiago Pastorino (Jan 21 2019 at 14:43, on Zulip):

because last commits I'm seeing there are the ones from Niko

Santiago Pastorino (Jan 21 2019 at 14:43, on Zulip):

do you want to pair with me for a while at some point?

csmoe (Jan 21 2019 at 14:44, on Zulip):

yep. same as niko's

Santiago Pastorino (Jan 21 2019 at 14:45, on Zulip):

ahh ok ok

csmoe (Jan 21 2019 at 14:48, on Zulip):

https://github.com/csmoe/rust/invitations

Santiago Pastorino (Jan 21 2019 at 14:49, on Zulip):

:+1:

csmoe (Jan 21 2019 at 14:50, on Zulip):

invited you as csmoe/rust collaborator, does this guarantee you access?

Santiago Pastorino (Jan 21 2019 at 14:51, on Zulip):

yes

Santiago Pastorino (Jan 21 2019 at 16:21, on Zulip):

@nikomatsakis I'm going over this thing

Santiago Pastorino (Jan 21 2019 at 16:22, on Zulip):

I think the next steps are:

One nit about the tree stuff was that, because PlaceBase is not Copy, it might require some cloning here and there to use the tree view, which is a bit less efficient. For describe_place, that doesn't matter, but it might be a problem elsewhere. I think I would prefer to fix this by making PlaceBase be Copy -- basically removing the Box and replacing with interned &'tcx pointers.

from this suggestion ...

Santiago Pastorino (Jan 21 2019 at 16:22, on Zulip):

convert remaining uses of Place to NeoPlace there are some that are complicated, like hash_stable

Santiago Pastorino (Jan 21 2019 at 16:22, on Zulip):

or things like that that touches a lot of things

Santiago Pastorino (Jan 21 2019 at 16:23, on Zulip):

but anyway, I may follow my own strategy

Santiago Pastorino (Jan 21 2019 at 16:23, on Zulip):

the thing is ... do we want to get rid of old Place at the end?

Santiago Pastorino (Jan 21 2019 at 16:24, on Zulip):

and convert everything to NeoPlace and NeoPlaceTree?

Santiago Pastorino (Jan 21 2019 at 16:26, on Zulip):

I'm working on this thing but I'm after removing old Place

Santiago Pastorino (Jan 21 2019 at 16:51, on Zulip):

I'm trying different approaches but unsure what's the best, maybe I just need now to convert place into neoplace everywhere

Santiago Pastorino (Jan 21 2019 at 16:51, on Zulip):

for instance

Santiago Pastorino (Jan 21 2019 at 16:51, on Zulip):
diff --git a/src/librustc/mir/tcx.rs b/src/librustc/mir/tcx.rs
index 408fb076442..5af92ed8f28 100644
--- a/src/librustc/mir/tcx.rs
+++ b/src/librustc/mir/tcx.rs
@@ -157,20 +157,6 @@ EnumTypeFoldableImpl! {
 }

 impl<'tcx> Place<'tcx> {
-    pub fn ty<'a, 'gcx, D>(&self, local_decls: &D, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> PlaceTy<'tcx>
-        where D: HasLocalDecls<'tcx>
-    {
-        match *self {
-            Place::Local(index) =>
-                PlaceTy::Ty { ty: local_decls.local_decls()[index].ty },
-            Place::Promoted(ref data) => PlaceTy::Ty { ty: data.1 },
-            Place::Static(ref data) =>
-                PlaceTy::Ty { ty: data.ty },
-            Place::Projection(ref proj) =>
-                proj.base.ty(local_decls, tcx).projection_ty(tcx, &proj.elem),
-        }
-    }
-
     // If this is a field projection, and the field is being projected from a closure type,
     // then returns the index of the field being projected. Note that this closure will always
     // be `self` in the current MIR, because that is the only time we directly access the fields
@@ -377,7 +363,9 @@ impl<'tcx> Rvalue<'tcx> {
                 tcx.mk_array(operand.ty(local_decls, tcx), count)
             }
             Rvalue::Ref(reg, bk, ref place) => {
-                let place_ty = place.ty(local_decls, tcx).to_ty(tcx);
+                let neo_place = tcx.as_new_place(place);
+                let place_ty = neo_place.ty(local_decls, tcx).to_ty(tcx);
+
                 tcx.mk_ref(reg,
                     ty::TypeAndMut {
                         ty: place_ty,
@@ -403,7 +391,8 @@ impl<'tcx> Rvalue<'tcx> {
                 operand.ty(local_decls, tcx)
             }
             Rvalue::Discriminant(ref place) => {
-                let ty = place.ty(local_decls, tcx).to_ty(tcx);
+                let neo_place = tcx.as_new_place(place);
+                let ty = neo_place.ty(local_decls, tcx).to_ty(tcx);
                 if let ty::Adt(adt_def, _) = ty.sty {
                     adt_def.repr.discr_type().to_ty(tcx)
                 } else {
@@ -452,7 +441,10 @@ impl<'tcx> Operand<'tcx> {
     {
         match self {
             &Operand::Copy(ref l) |
-            &Operand::Move(ref l) => l.ty(local_decls, tcx).to_ty(tcx),
+            &Operand::Move(ref l) => {
+                let neo_place = tcx.as_new_place(l);
+                neo_place.ty(local_decls, tcx).to_ty(tcx)
+            }
             &Operand::Constant(ref c) => c.ty,
         }
     }
Santiago Pastorino (Jan 21 2019 at 16:52, on Zulip):

and after that are 53 errors because I need to apply ty over neoplace

Santiago Pastorino (Jan 21 2019 at 16:52, on Zulip):

and convert place into neoplace

Santiago Pastorino (Jan 21 2019 at 16:52, on Zulip):

but then when I convert to create neoplace I'd need to change the stuff again

Santiago Pastorino (Jan 21 2019 at 16:53, on Zulip):

I guess it's better to just create neoplace instead of place

Santiago Pastorino (Jan 21 2019 at 16:53, on Zulip):

looking for where in code that happens ...

Santiago Pastorino (Jan 21 2019 at 17:14, on Zulip):

@nikomatsakis I'm not sure what did you exactly mean by Convert place construction to build a NeoPlace directly

Santiago Pastorino (Jan 21 2019 at 17:15, on Zulip):

if you meant that in the entire code and that basically means replacing the whole place thing with neoplace or if you were talking about some specific cases

nikomatsakis (Jan 22 2019 at 16:30, on Zulip):

ok @Santiago Pastorino so what I mean is

nikomatsakis (Jan 22 2019 at 16:30, on Zulip):

there is various bits of code in MIR construction that build up a "place"

nikomatsakis (Jan 22 2019 at 16:33, on Zulip):

usually converting from a HAIR expression

nikomatsakis (Jan 22 2019 at 17:00, on Zulip):

the code tends to work in a 'recursive' way right now:

so e.g. to translate a.b.c ,we might translate a to a Place, and then wrap that in another place to build a.b etc

nikomatsakis (Jan 22 2019 at 17:00, on Zulip):

however, for building a neo-place, that approach would lead to terrible performance

nikomatsakis (Jan 22 2019 at 17:00, on Zulip):

what we need is some sort of PlaceBuilder

nikomatsakis (Jan 22 2019 at 17:01, on Zulip):

that replaces the &[Projection] array with a Vec<Projection>

Santiago Pastorino (Jan 22 2019 at 17:02, on Zulip):

not sure about what case are you talking about

Santiago Pastorino (Jan 22 2019 at 17:02, on Zulip):

my approach was just to remove Place entirely

nikomatsakis (Jan 22 2019 at 18:30, on Zulip):

@Santiago Pastorino I'm talking about this method, basically

Santiago Pastorino (Jan 22 2019 at 19:01, on Zulip):

ok, what I don't see is what process are you suggesting to follow

Santiago Pastorino (Jan 22 2019 at 19:01, on Zulip):

I was more or less trying to remove Place and make the thing compile

Santiago Pastorino (Jan 22 2019 at 19:01, on Zulip):

regardless of performance

Santiago Pastorino (Jan 22 2019 at 19:02, on Zulip):

then as a second step I can transform things to make them better I guess

Santiago Pastorino (Jan 22 2019 at 19:02, on Zulip):

or are you suggesting this for some reason that I don't see

nikomatsakis (Jan 22 2019 at 20:27, on Zulip):

I was more or less trying to remove Place and make the thing compile

this seems ok, modulo one thing

nikomatsakis (Jan 22 2019 at 20:27, on Zulip):

I would definitely to keep each "major refactoring" within a single commit

nikomatsakis (Jan 22 2019 at 20:27, on Zulip):

i.e., to refactor one bit of code at a time so that you can test it in isolation

nikomatsakis (Jan 22 2019 at 20:28, on Zulip):

I guess that for the librustc_mir, I personally wouldn't refactor it first to build Place slowly and then PlaceBuilder but rather just direct to PlaceBuilder, but I also have no objection to doing it in 2 steps

nikomatsakis (Jan 22 2019 at 20:28, on Zulip):

maybe it's just because I see more clearly where I think it should go and it seems like a similar-ish amount of work

Santiago Pastorino (Jan 22 2019 at 20:28, on Zulip):

I would definitely to keep each "major refactoring" within a single commit

the problem is that I'm not sure that's always possible

nikomatsakis (Jan 22 2019 at 20:28, on Zulip):

maybe not, we should be trying to find ways to make it possible though :)

nikomatsakis (Jan 22 2019 at 20:28, on Zulip):

up till now we've been doing by interconverting between the two at "boundaries"

Santiago Pastorino (Jan 22 2019 at 20:29, on Zulip):

cases like implementing the mentioned traits could be isolated

nikomatsakis (Jan 22 2019 at 20:29, on Zulip):

which seems "ok" -- it's slow obviously, but presuably the final commit will be to remove all of that

Santiago Pastorino (Jan 22 2019 at 20:29, on Zulip):

yes, agree on that

nikomatsakis (Jan 22 2019 at 20:29, on Zulip):

I think if we don't manage to confine the rewrite to narrow pieces of code, though, the PR will never land =) (I still sort of worry that may be true)

Santiago Pastorino (Jan 22 2019 at 20:29, on Zulip):

but for instance ...

Santiago Pastorino (Jan 22 2019 at 20:30, on Zulip):

it's not always easy to convert from Place to NeoPlace

Santiago Pastorino (Jan 22 2019 at 20:31, on Zulip):

because we need a context which we don't always have

Santiago Pastorino (Jan 22 2019 at 20:31, on Zulip):

as_new_place is in TyCtxt

Santiago Pastorino (Jan 22 2019 at 20:31, on Zulip):

and there are places where I do not have the context

nikomatsakis (Jan 22 2019 at 20:33, on Zulip):

ok

nikomatsakis (Jan 22 2019 at 20:33, on Zulip):

apart from MIR construction, how many places are left that consume raw places and not neo-places, roughly? do we have an idea of that?

nikomatsakis (Jan 22 2019 at 20:34, on Zulip):

and, what's an example of a such a place where you encountered trouble?

Santiago Pastorino (Jan 22 2019 at 20:34, on Zulip):

I don't think there's that much left other than MIR construction

Santiago Pastorino (Jan 22 2019 at 20:35, on Zulip):

let me check a bit again

Santiago Pastorino (Jan 22 2019 at 20:37, on Zulip):

@nikomatsakis for instance https://github.com/rust-lang/rust/blob/master/src/librustc/ich/impls_mir.rs#L201

Santiago Pastorino (Jan 22 2019 at 20:37, on Zulip):

and a lot of stuff that's there

Santiago Pastorino (Jan 22 2019 at 20:39, on Zulip):

I mean, in order to implement hash_stable for some types you need Place implementing that thing

Santiago Pastorino (Jan 22 2019 at 20:39, on Zulip):

so if you want to convert that to NeoPlace you need to convert a lot of things

Santiago Pastorino (Jan 22 2019 at 20:39, on Zulip):

same for mir construction

Santiago Pastorino (Jan 22 2019 at 20:39, on Zulip):

anyway, I guess I can continue working on this and maybe tomorrow we can sync

Santiago Pastorino (Jan 22 2019 at 20:39, on Zulip):

let me figure out some more things

Santiago Pastorino (Jan 22 2019 at 20:40, on Zulip):

@nikomatsakis the doubt I have now is how to implement is_prefix_of for NeoPlace

Santiago Pastorino (Jan 22 2019 at 20:40, on Zulip):

given that PlaceBase and PlaceElem are different types

Santiago Pastorino (Jan 22 2019 at 20:41, on Zulip):

I can't just try to see if self's &[PlaceElem] includes other's &[PlaceElem] because we would be missing the base

Santiago Pastorino (Jan 22 2019 at 20:42, on Zulip):

basically unsure how to compare PlaceBase to PlaceElem

nikomatsakis (Jan 22 2019 at 20:47, on Zulip):

and a lot of stuff that's there

well, yeah, I would not modify the hashing impls

Santiago Pastorino (Jan 22 2019 at 20:48, on Zulip):

yeah, agreed :)

nikomatsakis (Jan 22 2019 at 20:48, on Zulip):

as for is_prefix_of

nikomatsakis (Jan 22 2019 at 20:49, on Zulip):

if you have two neo-places...

nikomatsakis (Jan 22 2019 at 20:49, on Zulip):

I guess that is_prefix_of would iterate in "lock-step" down self/other

nikomatsakis (Jan 22 2019 at 20:49, on Zulip):

so first the base of both must be equal

nikomatsakis (Jan 22 2019 at 20:49, on Zulip):

then each elem in self must be equal to an elem in other

nikomatsakis (Jan 22 2019 at 20:49, on Zulip):

plus other must have longer length

nikomatsakis (Jan 22 2019 at 20:49, on Zulip):

so something like

Santiago Pastorino (Jan 22 2019 at 20:50, on Zulip):

we are missing the base in the projection now, that's why I can't compare things

Santiago Pastorino (Jan 22 2019 at 20:50, on Zulip):

I guess I'm missing something ... :)

nikomatsakis (Jan 22 2019 at 20:50, on Zulip):
fn is_prefix_of(&self, other: &NeoPlace<'tcx>) -> bool {
    if self.len() > other.len() { return false; }
    if self.base != other.base { return false; }
    self.elems.iter().zip(other.elems).all(|(self_elem, other_elem)| self_elem == other_elem)
}
nikomatsakis (Jan 22 2019 at 20:51, on Zulip):

or

fn is_prefix_of(&self, other: &NeoPlace<'tcx>) -> bool {
    self.len() <= other.len() &&
    self.base == other.base &&
    self.elems.iter().zip(other.elems).all(|(self_elem, other_elem)| self_elem == other_elem)
}
nikomatsakis (Jan 22 2019 at 20:51, on Zulip):

you'd have to modify the trait though, maybe to something like this

pub trait IsPrefixOf<'tcx> {
    fn is_prefix_of(&self, other: &Self) -> bool;
}
nikomatsakis (Jan 22 2019 at 20:52, on Zulip):

the idea is that e.g. a.b is a prefix of a.b.c, right?

Santiago Pastorino (Jan 22 2019 at 20:52, on Zulip):

if that's the implementation I was misunderstanding what was is_prefix_of exactly

nikomatsakis (Jan 22 2019 at 20:52, on Zulip):

I believe that is the idea

Santiago Pastorino (Jan 22 2019 at 20:53, on Zulip):

ahh ya

Santiago Pastorino (Jan 22 2019 at 20:53, on Zulip):

yes

Santiago Pastorino (Jan 22 2019 at 20:53, on Zulip):

so yes

Santiago Pastorino (Jan 22 2019 at 20:53, on Zulip):

I was confused

Santiago Pastorino (Jan 22 2019 at 20:53, on Zulip):

the thing I was trying to implement is way more complicated :)

nikomatsakis (Jan 22 2019 at 20:53, on Zulip):

it's actually a good example for the kind of 'inversion' you have to do

nikomatsakis (Jan 22 2019 at 20:53, on Zulip):

to the existing logic

nikomatsakis (Jan 22 2019 at 20:53, on Zulip):

i.e., the existing code compares from the "end" backwards sort of, but this new code compares forward

Santiago Pastorino (Jan 23 2019 at 14:28, on Zulip):

@nikomatsakis I've pushed a couple of commits

Santiago Pastorino (Jan 23 2019 at 14:29, on Zulip):

in a while I'm going to check out what happens if I remove Place definition, in order to see if some other usage shows up and share with you to see what strategy may be better to follow

nikomatsakis (Jan 23 2019 at 17:06, on Zulip):

@Santiago Pastorino ok. I'll try to be available to answer questions

nikomatsakis (Jan 23 2019 at 17:07, on Zulip):

we could maybe do a brief video chat if we hit some thorny parts

Santiago Pastorino (Jan 23 2019 at 17:07, on Zulip):

that would be great

Santiago Pastorino (Jan 23 2019 at 17:07, on Zulip):

first I've pushed some stuff to the PR, csmoe gave me access

Santiago Pastorino (Jan 23 2019 at 17:08, on Zulip):

then I've removed the old Place and have some errors

nikomatsakis (Jan 23 2019 at 17:09, on Zulip):

I expect quite a few :)

Santiago Pastorino (Jan 23 2019 at 17:10, on Zulip):

yes, I was surprised because I was expecting more

Santiago Pastorino (Jan 23 2019 at 17:10, on Zulip):

error: aborting due to 83 previous errors

Santiago Pastorino (Jan 23 2019 at 17:10, on Zulip):

let me make a gist with them

Santiago Pastorino (Jan 23 2019 at 17:11, on Zulip):

https://gist.github.com/spastorino/31239f650e52047e9f436c97fa377ebd

Santiago Pastorino (Jan 23 2019 at 17:11, on Zulip):

for instance

Santiago Pastorino (Jan 23 2019 at 17:12, on Zulip):

the ones coming from src/librustc/ich/impls_mir.rs are related to hash stable

nikomatsakis (Jan 23 2019 at 17:12, on Zulip):

k

nikomatsakis (Jan 23 2019 at 17:12, on Zulip):

I have to run out for a bit

Santiago Pastorino (Jan 23 2019 at 17:12, on Zulip):

no worries

Santiago Pastorino (Jan 23 2019 at 17:13, on Zulip):

I will keep this going

Santiago Pastorino (Jan 23 2019 at 17:13, on Zulip):

there's for instance

Santiago Pastorino (Jan 23 2019 at 17:13, on Zulip):
impl<'tcx> Place<'tcx> {
    pub fn ty<'a, 'gcx, D>(&self, local_decls: &D, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> PlaceTy<'tcx>
Santiago Pastorino (Jan 23 2019 at 17:13, on Zulip):

I guess I can just remove that and change to use NeoPlace one and keep that idea going

Matthew Jasper (Jan 23 2019 at 18:13, on Zulip):

yes, I was surprised because I was expecting more

That's only the errors in librustc, there will also be errors in librustc_mir and librustc_codegen_ssa once librustc compiles.

Matthew Jasper (Jan 23 2019 at 18:14, on Zulip):

I guess I can just remove that and change to use NeoPlace one and keep that idea going

Yes

Santiago Pastorino (Jan 23 2019 at 19:04, on Zulip):

yes, right

csmoe (Jan 24 2019 at 12:33, on Zulip):

you can check the my previous work as kind of "diff map" https://github.com/rust-lang/rust/pull/53247/files
I replaced the Place with new Place in the whole codebase at once in that PR, and x.py check passed but x.py test failed. that was hard to maintain, so niko suggested current neo_place approach.

Santiago Pastorino (Jan 25 2019 at 15:23, on Zulip):

@nikomatsakis just rebased because there were merging conflicts, you can check some of my latest commits in the branch to see if everything is going as you more or less expected

csmoe (Jan 25 2019 at 16:14, on Zulip):

@Santiago Pastorino oops, I had rebased hours ago and made some commits, your force push bypassed the building.(anyway, never mind, I'd recommit later)

Santiago Pastorino (Jan 25 2019 at 16:22, on Zulip):

@csmoe it must have been almost simultaneosly

Santiago Pastorino (Jan 25 2019 at 16:22, on Zulip):

because I checked

Santiago Pastorino (Jan 25 2019 at 16:22, on Zulip):

anyway, great that you have your part

Santiago Pastorino (Jan 25 2019 at 16:23, on Zulip):

do you want to pair at some point or something?

Santiago Pastorino (Jan 25 2019 at 16:23, on Zulip):

what are you exactly working on?

csmoe (Jan 25 2019 at 16:52, on Zulip):

the commits cleaned up all the place in rustc_mir/borrow_check

Santiago Pastorino (Jan 25 2019 at 16:54, on Zulip):

can you rebase now those now and push to place2?

Santiago Pastorino (Jan 25 2019 at 16:55, on Zulip):

or push to a different branch? or even do a push -f with your stuff and I will fix stuff

Santiago Pastorino (Jan 25 2019 at 16:55, on Zulip):

@csmoe because I guess we are more or less doing the same stuff :)

csmoe (Jan 25 2019 at 17:15, on Zulip):

@Santiago Pastorino sorry, cannot play with my machine right now, you can just move forward, I'll make the commit track your path.

Santiago Pastorino (Jan 25 2019 at 17:20, on Zulip):

ok :)

nikomatsakis (Jan 25 2019 at 19:07, on Zulip):

@csmoe I was tlaking to @Santiago Pastorino about doing a quick meeting sometime next week. If we arrange it, we can either record or perhaps you'll be able to attend

Santiago Pastorino (Jan 25 2019 at 19:09, on Zulip):

@csmoe yeah, I've also mentioned that I'd be ok to pair if you want or doing a coding session where you can connect and we can chat or whatever works for you

csmoe (Jan 25 2019 at 22:33, on Zulip):

@nikomatsakis okay, I'll be there.
btw, I wanna express my sincere gratitude to you. I just received an offer as a junior Rust developer. I don't know if you still remember, your tweet and the comments in my first pull request gave me a lot of encouragement and confidence. You are my gateway to the Rust and open source world. Thank you

Santiago Pastorino (Jan 25 2019 at 22:43, on Zulip):

:clap::clap::clap::clap::clap:

Santiago Pastorino (Jan 25 2019 at 22:44, on Zulip):

@csmoe this is amazing, congrats

csmoe (Jan 25 2019 at 22:46, on Zulip):

@Santiago Pastorino thanks :slight_smile:

lqd (Jan 25 2019 at 23:37, on Zulip):

@csmoe oooh awesome, congratulations !

nikomatsakis (Jan 28 2019 at 19:59, on Zulip):

@csmoe oh, awesome! congratulations!

Santiago Pastorino (Jan 28 2019 at 21:16, on Zulip):

@nikomatsakis you around?

Santiago Pastorino (Jan 28 2019 at 21:16, on Zulip):

unsure if you saw my commits to the PR

nikomatsakis (Jan 28 2019 at 21:16, on Zulip):

I did not see them

Santiago Pastorino (Jan 28 2019 at 21:17, on Zulip):

but I'm now in the point where I need to start changing the visitor and hash_stable

Santiago Pastorino (Jan 28 2019 at 21:17, on Zulip):

the thing is if you start changing things randomly in that area you end changing all the stuff at once

Santiago Pastorino (Jan 28 2019 at 21:18, on Zulip):

I guess I can check if there's some leaf, some place where Place can be replaced with NeoPlace but the rest of the data structures can keep working with Place

Santiago Pastorino (Jan 28 2019 at 21:18, on Zulip):

so maybe I'd even need some convertions

Santiago Pastorino (Jan 28 2019 at 21:19, on Zulip):

my guess is that first of all I can move from Place to use NeoPlaceTree

Santiago Pastorino (Jan 28 2019 at 21:19, on Zulip):

and in some parts from NeoPlaceTree to use NeoPlace directly

Santiago Pastorino (Jan 28 2019 at 21:20, on Zulip):

and then if someone needs that NeoPlace but accepts a Place which would now be a NeoPlaceTree I can convert between NeoPlace to NeoPlaceTree using the as_tree function

Santiago Pastorino (Jan 28 2019 at 21:21, on Zulip):

@nikomatsakis unsure if you understood what I've tried to say because I know that my english was a bit entangled

nikomatsakis (Jan 28 2019 at 21:21, on Zulip):

=)

Santiago Pastorino (Jan 28 2019 at 21:21, on Zulip):

hehe

Santiago Pastorino (Jan 28 2019 at 21:21, on Zulip):

so

Santiago Pastorino (Jan 28 2019 at 21:22, on Zulip):

there's a graph of type dependencies

nikomatsakis (Jan 28 2019 at 21:22, on Zulip):

I confess i'm a bit lost though I think I sort of get the idea :P

Santiago Pastorino (Jan 28 2019 at 21:22, on Zulip):

some needs Place

Santiago Pastorino (Jan 28 2019 at 21:22, on Zulip):

and stuff like that

Santiago Pastorino (Jan 28 2019 at 21:22, on Zulip):

my idea is first of all I guess we can remove Place and make that be a NeoPlaceTree which should be a direct thing to do

Santiago Pastorino (Jan 28 2019 at 21:23, on Zulip):

then, slowly replace NeoPlaceTree with NeoPlace in some areas where it's easier, let's say in a leaf of that graph (in a place where nothing else depends on that type)

Santiago Pastorino (Jan 28 2019 at 21:24, on Zulip):

there's a possibility that some other type use that type I've just converted

Santiago Pastorino (Jan 28 2019 at 21:24, on Zulip):

then it will be expecting a NeoPlaceTree but I have a NeoPlace

Santiago Pastorino (Jan 28 2019 at 21:24, on Zulip):

then I can keep this idea going by just ... first using as_tree

nikomatsakis (Jan 28 2019 at 21:24, on Zulip):

I see

Santiago Pastorino (Jan 28 2019 at 21:24, on Zulip):

and then just making the thing using NeoPlace

nikomatsakis (Jan 28 2019 at 21:24, on Zulip):

interesting

Santiago Pastorino (Jan 28 2019 at 21:24, on Zulip):

I guess that idea may work

nikomatsakis (Jan 28 2019 at 21:24, on Zulip):

I'm trying to decide if convering to a NeoPlaceTree is a side-step or worthwhile

nikomatsakis (Jan 28 2019 at 21:25, on Zulip):

i.e., in a way they are equivalent

nikomatsakis (Jan 28 2019 at 21:25, on Zulip):

but I guess yu're right that at least we're down to "two types" somehow :)

Santiago Pastorino (Jan 28 2019 at 21:25, on Zulip):

well, maybe NeoPlaceTree should just be Place :)

nikomatsakis (Jan 28 2019 at 21:25, on Zulip):

and because convering from NeoPlace to NeoPlaceTree sort of exists

nikomatsakis (Jan 28 2019 at 21:25, on Zulip):

yeah, that's something I was debating :)

nikomatsakis (Jan 28 2019 at 21:26, on Zulip):

I mean, we could certainly write the code to conver from NeoPlace to Place

Santiago Pastorino (Jan 28 2019 at 21:26, on Zulip):

we can remove NeoPlaceTree and make the function return Place

nikomatsakis (Jan 28 2019 at 21:26, on Zulip):

merge neo-place-tree and place, somehow

Santiago Pastorino (Jan 28 2019 at 21:26, on Zulip):

yeah

nikomatsakis (Jan 28 2019 at 21:26, on Zulip):

and then try to switch the code that creates places

nikomatsakis (Jan 28 2019 at 21:26, on Zulip):

to create neo-places

Santiago Pastorino (Jan 28 2019 at 21:26, on Zulip):

isn't exactly the same type with a different name?

nikomatsakis (Jan 28 2019 at 21:26, on Zulip):

and convert back to place at each consumer point

nikomatsakis (Jan 28 2019 at 21:26, on Zulip):

I think it's more-or-less the same type, yes

nikomatsakis (Jan 28 2019 at 21:26, on Zulip):

there may be some minor difference

Santiago Pastorino (Jan 28 2019 at 21:26, on Zulip):

ok

nikomatsakis (Jan 28 2019 at 21:26, on Zulip):

I forget exactly how Place is formulated

Santiago Pastorino (Jan 28 2019 at 21:26, on Zulip):

going to try something like that out

Santiago Pastorino (Jan 28 2019 at 21:27, on Zulip):

when you can check the PR

Santiago Pastorino (Jan 28 2019 at 21:27, on Zulip):

it's probably a very quick thing to do because most of the commits are self explanatory

Santiago Pastorino (Jan 28 2019 at 21:27, on Zulip):

and very mechanical code

Santiago Pastorino (Jan 28 2019 at 21:27, on Zulip):

would be a good way to validate if what I did is more or less what you were expecting

nikomatsakis (Jan 28 2019 at 21:29, on Zulip):

ok

nikomatsakis (Jan 29 2019 at 16:18, on Zulip):

@Santiago Pastorino https://zoom.us/j/432921624

nikomatsakis (Jan 29 2019 at 17:13, on Zulip):

So @csmoe, @Santiago Pastorino and I were chatting a bit

nikomatsakis (Jan 29 2019 at 17:13, on Zulip):

I think in the end we decided a few things

nikomatsakis (Jan 29 2019 at 17:13, on Zulip):

as an immediate way forward, we can create a function that builds a Place from a NeoPlace

nikomatsakis (Jan 29 2019 at 17:13, on Zulip):

we can then convert the MIR IR to store NeoPlace

nikomatsakis (Jan 29 2019 at 17:13, on Zulip):

we do NOT modify the builder OR the (remaining) analyses at this time

nikomatsakis (Jan 29 2019 at 17:14, on Zulip):

instead, we take the Place that the builder makes now and convert it to neo-place

nikomatsakis (Jan 29 2019 at 17:14, on Zulip):

then convert back at the consumer

nikomatsakis (Jan 29 2019 at 17:14, on Zulip):

this is kind of a "no-op"

nikomatsakis (Jan 29 2019 at 17:14, on Zulip):

but once it is done, we can convert the builder and analyses independently

nikomatsakis (Jan 29 2019 at 17:14, on Zulip):

and everything should keep working

nikomatsakis (Jan 29 2019 at 17:14, on Zulip):

however, I did want to raise a concern :)

nikomatsakis (Jan 29 2019 at 17:14, on Zulip):

notably, I expect us to talk over changes to MIR at the all hands

nikomatsakis (Jan 29 2019 at 17:14, on Zulip):

I think this change will be a part of it

nikomatsakis (Jan 29 2019 at 17:14, on Zulip):

but it may be that the branch winds up kind of outdated

nikomatsakis (Jan 29 2019 at 17:15, on Zulip):

we'll see, but I wanted to raise the possibility that the branch will never land :'(

nikomatsakis (Jan 29 2019 at 17:15, on Zulip):

(though my take is sort of "that happens" -- a lot of experimental branches like this never land, but instead become the foundation for a new approach)

Santiago Pastorino (Jan 29 2019 at 18:02, on Zulip):

as I said on zoom, this is fine for me and a learning experience anyway

Santiago Pastorino (Jan 29 2019 at 18:03, on Zulip):

and also the way to go, if the task is kind of exploratory the result may be worse than the actual code or as you said it could even be that we realize of a better approach

csmoe (Jan 30 2019 at 04:47, on Zulip):

@nikomatsakis @Santiago Pastorino gotcha

Santiago Pastorino (Jan 30 2019 at 13:08, on Zulip):

I'm replacing Place with NeoPlace in some parts of the code

Santiago Pastorino (Jan 30 2019 at 13:08, on Zulip):

in particular, Place derives RustcEncodable

Santiago Pastorino (Jan 30 2019 at 13:08, on Zulip):

I need to make NeoPlace derive it too

pnkfelix (Jan 30 2019 at 13:08, on Zulip):

or at least implement it

Santiago Pastorino (Jan 30 2019 at 13:08, on Zulip):

yep

pnkfelix (Jan 30 2019 at 13:09, on Zulip):

can you just implement it by hand and have it work by converting into a Place and then calling the encode method on that?

Santiago Pastorino (Jan 30 2019 at 13:09, on Zulip):

the problem is that NeoPlace's elem field is &'tcx [PlaceElem<'tcx>]

pnkfelix (Jan 30 2019 at 13:09, on Zulip):

oh

pnkfelix (Jan 30 2019 at 13:09, on Zulip):

well

Santiago Pastorino (Jan 30 2019 at 13:09, on Zulip):

so I'd need ...

Santiago Pastorino (Jan 30 2019 at 13:09, on Zulip):

impl<'tcx> serialize::UseSpecializedDecodable for &'tcx [PlaceElem<'tcx>] {

pnkfelix (Jan 30 2019 at 13:09, on Zulip):

/me goes to double-check API of Encodeable

Santiago Pastorino (Jan 30 2019 at 13:09, on Zulip):

the question is ...

Santiago Pastorino (Jan 30 2019 at 13:09, on Zulip):

exactly

Santiago Pastorino (Jan 30 2019 at 13:10, on Zulip):

where do I find some information about all that

Santiago Pastorino (Jan 30 2019 at 13:10, on Zulip):

trying to search for Encodable right now

pnkfelix (Jan 30 2019 at 13:10, on Zulip):

https://docs.rs/rustc-serialize/0.3.24/rustc_serialize/trait.Encodable.html right?

Santiago Pastorino (Jan 30 2019 at 13:10, on Zulip):

but related to UseSpecializedDecodable doesn't seem to be that much info

Santiago Pastorino (Jan 30 2019 at 13:11, on Zulip):

https://docs.rs/rustc-serialize/0.3.24/rustc_serialize/trait.Encodable.html right?

well I see that, but what about UseSpecializedDecodable

pnkfelix (Jan 30 2019 at 13:11, on Zulip):

I might be looking at the wrong thing anyway

Santiago Pastorino (Jan 30 2019 at 13:11, on Zulip):

and why I see a lot of things implementing that with empty bodies

pnkfelix (Jan 30 2019 at 13:13, on Zulip):

do they have attributes? its possible proc-macros are injecting the actual code

pnkfelix (Jan 30 2019 at 13:13, on Zulip):

/me switches to looking at actual code

Santiago Pastorino (Jan 30 2019 at 13:14, on Zulip):
src/librustc/hir/def_id.rs
98:impl serialize::UseSpecializedDecodable for CrateNum {}
190:impl serialize::UseSpecializedDecodable for DefIndex {}
258:impl serialize::UseSpecializedDecodable for DefId {}
292:impl serialize::UseSpecializedDecodable for LocalDefId {}
pnkfelix (Jan 30 2019 at 13:15, on Zulip):

okay so the relevant code for the API's here is in src/libserialize/

Santiago Pastorino (Jan 30 2019 at 13:15, on Zulip):

I guess I can do something like ...

Santiago Pastorino (Jan 30 2019 at 13:19, on Zulip):
impl serialize::UseSpecializedDecodable for HirId {
    fn default_decode<D: Decoder>(d: &mut D) -> Result<HirId, D::Error> {
        let owner = DefIndex::decode(d)?;
        let local_id = ItemLocalId::decode(d)?;

        Ok(HirId {
            owner,
            local_id
        })
    }
}
Santiago Pastorino (Jan 30 2019 at 13:19, on Zulip):

but for my types

pnkfelix (Jan 30 2019 at 13:19, on Zulip):

By the way, what happens if you just try to make NeoPlace derive RustcEncodeable and RustcDecodable ?

Santiago Pastorino (Jan 30 2019 at 13:20, on Zulip):
error[E0277]: the trait bound `&[mir::ProjectionElem<'_, mir::Local, &ty::TyS<'_>>]: serialize::UseSpecializedDecodable` is not satisfied
    --> src/librustc/mir/mod.rs:1910:61
     |
1910 | #[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
     |                                                             ^^^^^^^^^^^^^^ the trait `serialize::UseSpecializedDecodable` is not implemented for `&[mir::ProjectionElem<'
_, mir::Local, &ty::TyS<'_>>]`
     |
     = note: required because of the requirements on the impl of `serialize::Decodable` for `&[mir::ProjectionElem<'_, mir::Local, &ty::TyS<'_>>]`
     = note: required by `serialize::Decodable::decode`
Santiago Pastorino (Jan 30 2019 at 13:20, on Zulip):

sorry

Santiago Pastorino (Jan 30 2019 at 13:20, on Zulip):

not that error

Santiago Pastorino (Jan 30 2019 at 13:20, on Zulip):

that error :)

pnkfelix (Jan 30 2019 at 13:21, on Zulip):

hmm

pnkfelix (Jan 30 2019 at 13:22, on Zulip):

I suppose if I read sufficient backlog, I'd learn why NeoPlace carries a slice rather than a Vec ?

Santiago Pastorino (Jan 30 2019 at 13:24, on Zulip):

:)

Santiago Pastorino (Jan 30 2019 at 13:25, on Zulip):

good question

Santiago Pastorino (Jan 30 2019 at 13:25, on Zulip):

Niko and csmoe did that

pnkfelix (Jan 30 2019 at 13:27, on Zulip):

(even so, i'd be tempted, based on this problem, to use a cow-slice, i.e. Cow<'tcx, [PlaceElem<'tcx>]>... but I also am now curious how other code is handling decoding of non-slice references. Must be interning or something.)

pnkfelix (Jan 30 2019 at 13:29, on Zulip):

ah okay I'm starting to understand

pnkfelix (Jan 30 2019 at 13:29, on Zulip):

the whole UseSpecialized* stuff is precisely to hook into specialized decoders for reference types

pnkfelix (Jan 30 2019 at 13:31, on Zulip):

start reading from this line: https://github.com/rust-lang/rust/blob/master/src/libserialize/serialize.rs#L846

pnkfelix (Jan 30 2019 at 13:32, on Zulip):

the empty impls you are seeing are markers allowing this impl to trigger: https://github.com/rust-lang/rust/blob/master/src/libserialize/serialize.rs#L898

Santiago Pastorino (Jan 30 2019 at 13:33, on Zulip):

ok

Santiago Pastorino (Jan 30 2019 at 13:33, on Zulip):

will be back in a bit

Santiago Pastorino (Jan 30 2019 at 13:34, on Zulip):

thanks for the info

pnkfelix (Jan 30 2019 at 13:35, on Zulip):

which then allow the specialized decoders to trigger, like this one: https://github.com/rust-lang/rust/blob/master/src/librustc/ty/codec.rs#L369 (with actual code here https://github.com/rust-lang/rust/blob/master/src/librustc/ty/codec.rs#L216 )

Santiago Pastorino (Jan 30 2019 at 14:31, on Zulip):

back

Santiago Pastorino (Jan 30 2019 at 14:32, on Zulip):

I'm getting this

Santiago Pastorino (Jan 30 2019 at 14:32, on Zulip):
error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
    --> src/librustc/mir/mod.rs:3531:1
     |
3531 | impl<'tcx> serialize::UseSpecializedDecodable for &'tcx [PlaceElem<'tcx>] {}
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl doesn't use types inside crate
     |
     = note: the impl does not reference any types defined in this crate
     = note: define and implement a trait or new type instead
Santiago Pastorino (Jan 30 2019 at 14:32, on Zulip):

unsure why because I'm doing so in the place where PlaceElem is defined there

Santiago Pastorino (Jan 30 2019 at 14:51, on Zulip):

and also this ...

Santiago Pastorino (Jan 30 2019 at 14:51, on Zulip):
impl<'tcx> serialize::UseSpecializedEncodable for &'tcx [PlaceElem<'tcx>] {}
impl<'tcx> serialize::UseSpecializedDecodable for &'tcx [PlaceElem<'tcx>] {}
Santiago Pastorino (Jan 30 2019 at 14:51, on Zulip):

exist in the same file

Santiago Pastorino (Jan 30 2019 at 15:20, on Zulip):

the problem is with the slice, I guess, because PlaceElem is defined in the crate but &[PlaceElem] is not

Santiago Pastorino (Jan 30 2019 at 15:20, on Zulip):

unsure how to fix this exactly

Santiago Pastorino (Jan 30 2019 at 15:21, on Zulip):

@pnkfelix ^^^

pnkfelix (Jan 30 2019 at 15:21, on Zulip):

Personally I think your best bet is to not attempt to use a slice at all here

Santiago Pastorino (Jan 30 2019 at 15:21, on Zulip):

I guess I can wrap the type

Santiago Pastorino (Jan 30 2019 at 15:21, on Zulip):

ok

pnkfelix (Jan 30 2019 at 15:22, on Zulip):

Okay well that's another short-term option

Santiago Pastorino (Jan 30 2019 at 15:22, on Zulip):

I guess I can change to Vec and move forward

Santiago Pastorino (Jan 30 2019 at 15:22, on Zulip):

hmm yeah

pnkfelix (Jan 30 2019 at 15:23, on Zulip):

You can either commit to Vec, or if you're nervous about that, you could use Cow<'tcx, [PlaceElem]>. From what I skimmed earlier, it seemed like Cow<[T]> would be supported in serialization contexts where you might have wanted to use &[T].

pnkfelix (Jan 30 2019 at 15:24, on Zulip):

(But if you're already going to go through the effort of changing the type, you probably can just go with Vec and just check with @csmoe and/or @nikomatsakis later to find out whether they had a strong motivation for using a slice here.

csmoe (Jan 30 2019 at 15:28, on Zulip):

I guess I can change to Vec and move forward

@Santiago Pastorino consider List

csmoe (Jan 30 2019 at 15:34, on Zulip):

my original implementation of elems copied the approach from existential code base as https://github.com/rust-lang/rust/blob/43b4c4a36b6c189bf0718a9d77ff1164c3fa7cac/src/librustc/ty/context.rs#L126

pnkfelix (Jan 30 2019 at 15:35, on Zulip):

but we don't currently use that elsewhere in MIR ...

csmoe (Jan 30 2019 at 15:36, on Zulip):

but @nikomatsakis changed to &[...] days ago https://github.com/rust-lang/rust/pull/54426/commits/6eb57e9fc4fcd132450ed8e3603b96ef0fcc4aff

pnkfelix (Jan 30 2019 at 15:37, on Zulip):

hmm. Okay then. That serves as a hint that @nikomatsakis in fact did intend to impl<'tcx> serialize::UseSpecializedDecodable for &'tcx List<PlaceElem<'tcx>>

pnkfelix (Jan 30 2019 at 15:37, on Zulip):

It looks like a rats nest to me, but whatever.

pnkfelix (Jan 30 2019 at 15:38, on Zulip):

( if we expect lots of sharing between Places then a slice makes sense. I just haven't seen what the MIR tree structures look like yet.)

pnkfelix (Jan 30 2019 at 15:38, on Zulip):

hmm. Okay then. That serves as a hint that @nikomatsakis in fact did intend to impl<'tcx> serialize::UseSpecializedDecodable for &'tcx List<PlaceElem<'tcx>>

oh, no, that's me misreading the code/commit

Santiago Pastorino (Jan 30 2019 at 16:53, on Zulip):

actually the wrapper doesn't make any sense :)

Santiago Pastorino (Jan 30 2019 at 16:53, on Zulip):

NeoPlace can work already as a wrapper

Santiago Pastorino (Jan 30 2019 at 16:53, on Zulip):

the thing is that I guess I need to just implement the decode API on NeoPlace

nikomatsakis (Jan 30 2019 at 20:08, on Zulip):

I suppose if I read sufficient backlog, I'd learn why NeoPlace carries a slice rather than a Vec ?

we want a slice -- the decoder can do the interning

nikomatsakis (Jan 30 2019 at 20:08, on Zulip):

the reason we want a slice is so that we can make subslices :)

nikomatsakis (Jan 30 2019 at 20:08, on Zulip):

we used to, however, use the List type, which kind of handled this for us, but that was a bit too restrictive

nikomatsakis (Jan 30 2019 at 20:09, on Zulip):

I have to refresh my memory how this works

Santiago Pastorino (Jan 30 2019 at 20:09, on Zulip):

hmm :)

Santiago Pastorino (Jan 30 2019 at 20:10, on Zulip):

ok

Santiago Pastorino (Jan 30 2019 at 20:10, on Zulip):

I'm not sure how the serializer API works

Santiago Pastorino (Jan 30 2019 at 20:11, on Zulip):

searching ...

nikomatsakis (Jan 30 2019 at 20:14, on Zulip):

ok ok

nikomatsakis (Jan 30 2019 at 20:14, on Zulip):

I remember now

nikomatsakis (Jan 30 2019 at 20:14, on Zulip):

so @Santiago Pastorino this is a bit of a kludge

nikomatsakis (Jan 30 2019 at 20:14, on Zulip):

(it's also one of the places that rustc is relying on an unsoundness in Rust)

nikomatsakis (Jan 30 2019 at 20:14, on Zulip):

(but never mind that now)

nikomatsakis (Jan 30 2019 at 20:14, on Zulip):

I'm pondering whether to leave some of these notes in a form that could become a rustc-guide chapter

Santiago Pastorino (Jan 30 2019 at 20:15, on Zulip):

:)

Santiago Pastorino (Jan 30 2019 at 20:15, on Zulip):

so I'm also a bit confused about libserialize and friends

nikomatsakis (Jan 30 2019 at 20:15, on Zulip):

So, we have this Deserialize trait

nikomatsakis (Jan 30 2019 at 20:16, on Zulip):

er, sorr

nikomatsakis (Jan 30 2019 at 20:16, on Zulip):

Decodable

Santiago Pastorino (Jan 30 2019 at 20:16, on Zulip):

my guess is that I need to implement Decodable for NeoPlace

nikomatsakis (Jan 30 2019 at 20:16, on Zulip):

trying to decide just where to start

nikomatsakis (Jan 30 2019 at 20:16, on Zulip):

no, that's not true

nikomatsakis (Jan 30 2019 at 20:16, on Zulip):

I mean, you do, but not directly

Santiago Pastorino (Jan 30 2019 at 20:16, on Zulip):

but confused, the errors lead me .... ok

nikomatsakis (Jan 30 2019 at 20:17, on Zulip):

so the problem is this:

In order to deseriable a NeoPlace, you need access to a tcx

Santiago Pastorino (Jan 30 2019 at 20:17, on Zulip):

yep to intern

nikomatsakis (Jan 30 2019 at 20:17, on Zulip):

you can get that, in theory, be invoking a method from on the "decoder" that is given to you

Santiago Pastorino (Jan 30 2019 at 20:17, on Zulip):

and also, elems is &[]

nikomatsakis (Jan 30 2019 at 20:18, on Zulip):

because — in practice — this Decoder will be some type that implements TyDecoder:

// src/librustc/ty/codec.rs
pub trait TyDecoder<'a, 'tcx: 'a>: Decoder {
    fn tcx(&self) -> TyCtxt<'a, 'tcx, 'tcx>;
    ...
}
Santiago Pastorino (Jan 30 2019 at 20:18, on Zulip):

so I can't implement impl<'tcx> serialize::UseSpecializedDecodable for &'tcx &[PlaceElem<'tcx>] {}

Santiago Pastorino (Jan 30 2019 at 20:19, on Zulip):

because — in practice — this Decoder will be some type that implements TyDecoder:

// src/librustc/ty/codec.rs
pub trait TyDecoder<'a, 'tcx: 'a>: Decoder {
    fn tcx(&self) -> TyCtxt<'a, 'tcx, 'tcx>;
    ...
}

ok got it

Santiago Pastorino (Jan 30 2019 at 20:20, on Zulip):

so I can't implement impl<'tcx> serialize::UseSpecializedDecodable for &'tcx &[PlaceElem<'tcx>] {}

this :point_up: is one of the main issues I have :)

nikomatsakis (Jan 30 2019 at 20:22, on Zulip):

So

nikomatsakis (Jan 30 2019 at 20:22, on Zulip):

(sorry, got called away for a sec)

nikomatsakis (Jan 30 2019 at 20:22, on Zulip):

one problem is, the Decodable trait doesn't let you specialize on the type of decoder you have

nikomatsakis (Jan 30 2019 at 20:22, on Zulip):

it is defined

nikomatsakis (Jan 30 2019 at 20:22, on Zulip):
trait Decodable {
    fn decode<D: Decoder>(..) // <-- so you have to implement for all `D`
}
nikomatsakis (Jan 30 2019 at 20:23, on Zulip):

this is the UseSpecializedDecodable trait exists

nikomatsakis (Jan 30 2019 at 20:23, on Zulip):

there is an impl:

impl<T: UseSpecializedDecodable> Decodable for T {
    default fn decode<D: Decoder>(d: &mut D) -> Result<T, D::Error> {
        D::specialized_decode(d)
    }
}
nikomatsakis (Jan 30 2019 at 20:23, on Zulip):

so the idea is, if you implement UseSpecializedDecodable, you will be directed to specialized_decode, which is setup in such a way that you can specialize

nikomatsakis (Jan 30 2019 at 20:24, on Zulip):

there is also a blanket impl here for all T:

impl<'a, T: ?Sized + Encodable> UseSpecializedEncodable for &'a T {}
nikomatsakis (Jan 30 2019 at 20:24, on Zulip):

anyway, I guess, hmm, you won't be able to implement for the slice, you are correct, owing to the orphan rules

Santiago Pastorino (Jan 30 2019 at 20:24, on Zulip):

yeah, but still I'm not sure how to implement it for &[PlaceElem]

nikomatsakis (Jan 30 2019 at 20:25, on Zulip):

well, to start, you wouldn't want to

Santiago Pastorino (Jan 30 2019 at 20:25, on Zulip):

I mean, how to avoid the compiler error I've shared above

nikomatsakis (Jan 30 2019 at 20:25, on Zulip):

though you might want to implement for [PlaceElem]

nikomatsakis (Jan 30 2019 at 20:25, on Zulip):

it seems like we might need a newtype wrapper, or else to define that elsewhere

nikomatsakis (Jan 30 2019 at 20:25, on Zulip):

alternatively, we implement for NeoPlace by hand

nikomatsakis (Jan 30 2019 at 20:25, on Zulip):

and side-step the problem

nikomatsakis (Jan 30 2019 at 20:25, on Zulip):

which is perhaps easier

Santiago Pastorino (Jan 30 2019 at 20:26, on Zulip):

yeah that was what I was referring to when I mentioned that I needed to implement the thing for NeoPlace

Santiago Pastorino (Jan 30 2019 at 20:26, on Zulip):

which already acts as a wrapper

Santiago Pastorino (Jan 30 2019 at 20:26, on Zulip):

the other question is ... how does rustc_encodable and rustc_decodable play in this combo?

Santiago Pastorino (Jan 30 2019 at 20:27, on Zulip):

I guess if I implement this trait for NeoPlace I should not derive?

Santiago Pastorino (Jan 30 2019 at 20:27, on Zulip):

I'm not sure deriving that what's the trait that wins a implementation for NeoPlace automatically

nikomatsakis (Jan 30 2019 at 20:27, on Zulip):

I think what you would want to do

nikomatsakis (Jan 30 2019 at 20:27, on Zulip):

is to add an impl into the implement_ty_decoder macro definition

nikomatsakis (Jan 30 2019 at 20:27, on Zulip):

in ty/codec.rs

nikomatsakis (Jan 30 2019 at 20:28, on Zulip):

e.g., in there, you will see impls like this:

            impl<$($typaram),*> SpecializedDecoder<&'tcx ty::AdtDef>
            for $DecoderName<$($typaram),*> {
                fn specialized_decode(&mut self) -> Result<&'tcx ty::AdtDef, Self::Error> {
                    decode_adt_def(self)
                }
            }
nikomatsakis (Jan 30 2019 at 20:28, on Zulip):

and you could in theory add a

nikomatsakis (Jan 30 2019 at 20:28, on Zulip):
            impl<$($typaram),*> SpecializedDecoder<NeoPlace<'tcx>>
            for $DecoderName<$($typaram),*> {
                fn specialized_decode(&mut self) -> Result<NeoPlace<'tcx>, Self::Error> {
                    ...
                }
            }
nikomatsakis (Jan 30 2019 at 20:31, on Zulip):

anyway, i'm debating about this

nikomatsakis (Jan 30 2019 at 20:31, on Zulip):

the reason we used the slice was to easily support NeoPlaceTree

nikomatsakis (Jan 30 2019 at 20:31, on Zulip):

which...still seems useful to me

nikomatsakis (Jan 30 2019 at 20:31, on Zulip):

so I guess it's worth doing this

nikomatsakis (Jan 30 2019 at 20:31, on Zulip):

(as opposed to using &'tcx List<ProjectionElem<'tcx>>, which I think is what @csmoe initially had.

Santiago Pastorino (Jan 30 2019 at 20:31, on Zulip):

yeah

nikomatsakis (Jan 30 2019 at 20:31, on Zulip):

If we used that type, a lot of the plumbing is written for us, but we lose the ability to do subslicing

Santiago Pastorino (Jan 30 2019 at 20:32, on Zulip):

yeah

Santiago Pastorino (Jan 30 2019 at 20:32, on Zulip):

couldn't we delay doing this?

Santiago Pastorino (Jan 30 2019 at 20:33, on Zulip):

I mean, maybe just going with List and after revisiting all the Place <-> NeoPlace we can come back

Santiago Pastorino (Jan 30 2019 at 20:33, on Zulip):

or is there something that won't work?

Santiago Pastorino (Jan 30 2019 at 20:33, on Zulip):

mentioning this because it may be faster to experiment with the real migration to NeoPlace

Santiago Pastorino (Jan 30 2019 at 20:33, on Zulip):

and then we can just change the List type to be a Slice

pnkfelix (Jan 30 2019 at 20:46, on Zulip):

/me thinks Cow would be another way to delay doing this

nikomatsakis (Jan 30 2019 at 21:02, on Zulip):

I hate cow :P

nikomatsakis (Jan 30 2019 at 21:02, on Zulip):

but you're not wrong

nikomatsakis (Jan 30 2019 at 21:02, on Zulip):

I mean another way would be to make a newtype like

struct ProjectionList<'tcx> {
    slice: &'tcx [ProjectionElem<'tcx>]
}
nikomatsakis (Jan 30 2019 at 21:02, on Zulip):

I think it'd not be too hard

Santiago Pastorino (Jan 30 2019 at 21:07, on Zulip):

yeah I've already done that

Santiago Pastorino (Jan 30 2019 at 21:07, on Zulip):

but I mean, why is better to make a wrapper around &'tcx [ProjectionElem<'tcx>] rather than just using NeoPlace?

pnkfelix (Jan 30 2019 at 21:08, on Zulip):

The fact that anyone hates Cow represents a design bug somewhere

pnkfelix (Jan 30 2019 at 21:08, on Zulip):

(Not that you’re wrong to hate Cow)

nikomatsakis (Jan 30 2019 at 21:09, on Zulip):

The fact that anyone hates Cow represents a design bug somewhere

in the language :P

nikomatsakis (Jan 30 2019 at 21:09, on Zulip):

I don't even know what I mean by that

pnkfelix (Jan 30 2019 at 21:09, on Zulip):

((But it should be an easier thing to put in API’s))

nikomatsakis (Jan 30 2019 at 21:09, on Zulip):

but I mean, why is better to make a wrapper around &'tcx [ProjectionElem<'tcx>] rather than just using NeoPlace?

no particular reason I suppose, just more narrow

nikomatsakis (Jan 30 2019 at 21:09, on Zulip):

but yeah it's not easier

nikomatsakis (Jan 30 2019 at 21:09, on Zulip):

mosly I think this code is not actually that hard

nikomatsakis (Jan 30 2019 at 21:09, on Zulip):

otoh if NeoPlaceTree is only used rarely

nikomatsakis (Jan 30 2019 at 21:09, on Zulip):

maybe it's fine for it to be inefficient

nikomatsakis (Jan 30 2019 at 21:10, on Zulip):

seems like a shame tho

nikomatsakis (Jan 30 2019 at 21:10, on Zulip):

@Santiago Pastorino is this stuff all on @csmoe 's branch? i.e., can I take a look at it?

Santiago Pastorino (Jan 30 2019 at 21:10, on Zulip):

everything is in there

Santiago Pastorino (Jan 30 2019 at 21:10, on Zulip):

but there's code wip on my machine

Santiago Pastorino (Jan 30 2019 at 21:10, on Zulip):

unsure what do you want to look at

Santiago Pastorino (Jan 30 2019 at 21:11, on Zulip):

I can open tmate and allow you in if you want :)

nikomatsakis (Jan 30 2019 at 21:26, on Zulip):

@Santiago Pastorino we could try that :)

Santiago Pastorino (Jan 30 2019 at 21:27, on Zulip):

:)

nikomatsakis (Jan 30 2019 at 21:57, on Zulip):

@Santiago Pastorino I pushed a commit that implements RustcDecodable for NeoPlace<'tcx>

nikomatsakis (Jan 30 2019 at 21:59, on Zulip):

at least, x.py check builds

Santiago Pastorino (Jan 31 2019 at 01:26, on Zulip):

will check tomorrow

nikomatsakis (Jan 31 2019 at 14:20, on Zulip):

@Santiago Pastorino get a chance to look?

Santiago Pastorino (Jan 31 2019 at 14:21, on Zulip):

@nikomatsakis hey, almost back

Santiago Pastorino (Jan 31 2019 at 14:21, on Zulip):

catching up with Rust Latam stuff :)

Santiago Pastorino (Jan 31 2019 at 14:21, on Zulip):

yesterday I took a look at it

Santiago Pastorino (Jan 31 2019 at 14:21, on Zulip):

actually, let me go over it quickly

Santiago Pastorino (Jan 31 2019 at 14:21, on Zulip):

you don't derive RustcDecodable

Santiago Pastorino (Jan 31 2019 at 14:22, on Zulip):

I guess that interferes with what you're doing, right?

Santiago Pastorino (Jan 31 2019 at 14:22, on Zulip):

I'm not sure what deriving RustcDecodable make the type implement

Santiago Pastorino (Jan 31 2019 at 14:23, on Zulip):

ok, read it

Santiago Pastorino (Jan 31 2019 at 14:23, on Zulip):

there some things that seem magic to me :P

Santiago Pastorino (Jan 31 2019 at 14:54, on Zulip):

@nikomatsakis back to this, yeah the thing makes sense

Santiago Pastorino (Jan 31 2019 at 14:54, on Zulip):

so UseSpecializedDecoder is a marker trait and the decoder API checks for that in order to use a SpecializedDecoder I guess

Santiago Pastorino (Jan 31 2019 at 14:55, on Zulip):

which in this case is the one you defined using macros

Santiago Pastorino (Jan 31 2019 at 14:57, on Zulip):

the only thing I don't understand is what is exactly https://github.com/rust-lang/rust/pull/54426/commits/cf4da24aa9dc93c99d175b9e7a3456b3cec5a166#diff-ba672cead097ccdd2da4d223f02f4735R428 which is the entity that knows how to decode a PlaceBase and stuff like that and used all over the place like in https://github.com/rust-lang/rust/pull/54426/commits/cf4da24aa9dc93c99d175b9e7a3456b3cec5a166#diff-ba672cead097ccdd2da4d223f02f4735R274

Santiago Pastorino (Jan 31 2019 at 14:57, on Zulip):

but I guess that's generic code, it's just decoding a struct :)

nikomatsakis (Jan 31 2019 at 18:49, on Zulip):

decoding a PlaceBase comes from the derive on PlaceBase

Santiago Pastorino (Jan 31 2019 at 18:58, on Zulip):

ahh right

Santiago Pastorino (Jan 31 2019 at 18:59, on Zulip):

what does the decoder do then exactly?

Santiago Pastorino (Jan 31 2019 at 18:59, on Zulip):

or what role it plays?

Santiago Pastorino (Jan 31 2019 at 19:00, on Zulip):

sorry, I'm not asking correctly :)

nikomatsakis (Jan 31 2019 at 19:00, on Zulip):

what does the decoder do then exactly?

e.g., in this line?

let base: mir::PlaceBase<'tcx> = Decodable::decode(decoder)?;
Santiago Pastorino (Jan 31 2019 at 19:00, on Zulip):

yeah, everywhere

Santiago Pastorino (Jan 31 2019 at 19:00, on Zulip):

what's the type?

nikomatsakis (Jan 31 2019 at 19:00, on Zulip):

the decoder basically gives the context needed

Santiago Pastorino (Jan 31 2019 at 19:00, on Zulip):

how does it know how to decode?

Santiago Pastorino (Jan 31 2019 at 19:00, on Zulip):

etc

nikomatsakis (Jan 31 2019 at 19:01, on Zulip):

it's type is impl TyDecode

nikomatsakis (Jan 31 2019 at 19:01, on Zulip):

that is, some type that implements the TyDecoder trait

nikomatsakis (Jan 31 2019 at 19:01, on Zulip):

which extends the Decoder trait

Santiago Pastorino (Jan 31 2019 at 19:01, on Zulip):

ok

nikomatsakis (Jan 31 2019 at 19:01, on Zulip):

the most basic methods it offers are like decode_usize

nikomatsakis (Jan 31 2019 at 19:01, on Zulip):

basically think of it like this:

nikomatsakis (Jan 31 2019 at 19:01, on Zulip):

there is some stream of data being decoded

nikomatsakis (Jan 31 2019 at 19:01, on Zulip):

the decoder knows about that stream

Santiago Pastorino (Jan 31 2019 at 19:01, on Zulip):

yeah basically you need to do what derive RustcDecodable does but you need to unroll the slice

nikomatsakis (Jan 31 2019 at 19:01, on Zulip):

you can tell it to e.g. read the next usize from the stream

nikomatsakis (Jan 31 2019 at 19:01, on Zulip):

and other simple, built-in operations

nikomatsakis (Jan 31 2019 at 19:02, on Zulip):

but then the Decodable trait composes those methods to build bigger types

Santiago Pastorino (Jan 31 2019 at 19:02, on Zulip):

yeah

Santiago Pastorino (Jan 31 2019 at 19:02, on Zulip):

makes a lot of sense

nikomatsakis (Jan 31 2019 at 19:02, on Zulip):

so e.g. the Decodable for (u32, u32) would be like "read two u32s and I will build a tuple"

Santiago Pastorino (Jan 31 2019 at 19:02, on Zulip):

yes

nikomatsakis (Jan 31 2019 at 19:02, on Zulip):

except that really it's built generically, so the Decodable for (A, B) says "read an A and a B"

nikomatsakis (Jan 31 2019 at 19:02, on Zulip):

and the Decodable trait for u32, in turn, says "read a u32"

Santiago Pastorino (Jan 31 2019 at 19:02, on Zulip):

yeah, exactly in the way I was thinking about it :)

nikomatsakis (Jan 31 2019 at 19:02, on Zulip):

the whole UseSpecializedDecoder thing

Santiago Pastorino (Jan 31 2019 at 19:03, on Zulip):

it's just that there are things that seem to appear from nowhere

nikomatsakis (Jan 31 2019 at 19:03, on Zulip):

just lets us take advantage of the fact that we have a TyDecoder, which has a tcx

Santiago Pastorino (Jan 31 2019 at 19:03, on Zulip):

yeah, maybe that part is the part I see the less

Santiago Pastorino (Jan 31 2019 at 19:03, on Zulip):

where that comes from

nikomatsakis (Jan 31 2019 at 19:03, on Zulip):

in a sense it is saying "here is a Decodable that requires not just any Decoder but a TyDecoder" -- there is also a "base implementation" that works for any decoder

Santiago Pastorino (Jan 31 2019 at 19:03, on Zulip):

I guess the macro does something?

nikomatsakis (Jan 31 2019 at 19:03, on Zulip):

but that base implementation just panics

nikomatsakis (Jan 31 2019 at 19:03, on Zulip):

the macro is generating the impl, yeah

nikomatsakis (Jan 31 2019 at 19:03, on Zulip):

I forget why the macro is needed

nikomatsakis (Jan 31 2019 at 19:03, on Zulip):

probably coherence

Santiago Pastorino (Jan 31 2019 at 19:04, on Zulip):

ok

Santiago Pastorino (Jan 31 2019 at 19:04, on Zulip):

:+1:

nikomatsakis (Jan 31 2019 at 19:04, on Zulip):

basically we can't define a rule for "any type T that implements TyDecoder"

nikomatsakis (Jan 31 2019 at 19:04, on Zulip):

because we're in the wrong crate

nikomatsakis (Jan 31 2019 at 19:04, on Zulip):

so instead we define mulitple impls, one for each type that is a TyDecoder

nikomatsakis (Jan 31 2019 at 19:04, on Zulip):

and use the macro to avoid code duplication

Santiago Pastorino (Jan 31 2019 at 19:16, on Zulip):

:+1:

Santiago Pastorino (Feb 02 2019 at 07:05, on Zulip):

@nikomatsakis during non sleep time of MVD -> MAD I've added a commit to the PR

Santiago Pastorino (Feb 02 2019 at 07:05, on Zulip):

https://github.com/rust-lang/rust/pull/54426/commits/fe2fedbd469b3c0a8a1b4543cee21b8ff05c4264

Santiago Pastorino (Feb 02 2019 at 07:05, on Zulip):

it was failing on my machine

Santiago Pastorino (Feb 02 2019 at 07:06, on Zulip):

still unsure why, if you can take a look please do :)

Santiago Pastorino (Feb 02 2019 at 07:06, on Zulip):

it's an ICE

Santiago Pastorino (Feb 02 2019 at 07:13, on Zulip):

here is the error https://gist.github.com/spastorino/fe996acdc5091d8298432485bb5d36dc

csmoe (Feb 02 2019 at 08:01, on Zulip):

@Santiago Pastorino I noticed that you created local place with NeoPlace { base: Local(local), elems: &[]} repeatedly, you can check out the methods impl NeoPlace {} in the PR at src/librustc/mir/mod.rs(line 2119-2211)

Matthew Jasper (Feb 02 2019 at 08:54, on Zulip):

@Santiago Pastorino The issue is that MutVisitor::visit_assign needs to write the new place back to the Assign, if it's been modified.

Santiago Pastorino (Feb 02 2019 at 11:02, on Zulip):

@Santiago Pastorino The issue is that MutVisitor::visit_assign needs to write the new place back to the Assign, if it's been modified.

you're right, anyway this is basically code to be removed

Santiago Pastorino (Feb 02 2019 at 11:02, on Zulip):

but wanna change

Santiago Pastorino (Feb 02 2019 at 11:07, on Zulip):

@Santiago Pastorino The issue is that MutVisitor::visit_assign needs to write the new place back to the Assign, if it's been modified.

hmm, the problem is Assign takes a NeoPlace and visit_assign a Place

Santiago Pastorino (Feb 02 2019 at 11:07, on Zulip):

do you mean that I need both things to be the same thing?

Santiago Pastorino (Feb 02 2019 at 11:07, on Zulip):

@Matthew Jasper why exactly?

Matthew Jasper (Feb 02 2019 at 11:39, on Zulip):

do you mean that I need both things to be the same thing?

I mean that you need to convert the modified place back to a NeoPlace and use it in the Assign.

Santiago Pastorino (Feb 02 2019 at 18:56, on Zulip):

pushed some fixes, still ICEing

Santiago Pastorino (Feb 02 2019 at 18:56, on Zulip):

need to take a closer look

pnkfelix (Feb 06 2019 at 16:32, on Zulip):

@Santiago Pastorino FYI this is a good thing to read regarding 'tcx: https://rust-lang.github.io/rustc-guide/ty.html?highlight=TyCtxt#the-tcx-and-how-it-uses-lifetimes

pnkfelix (Feb 11 2019 at 13:13, on Zulip):

Hi again @Santiago Pastorino ; it was too bad you were not able to stay until Friday. The changes you and @csmoe have been making to Place came up during the MIR 2.0 talks

pnkfelix (Feb 11 2019 at 13:31, on Zulip):

you can see discussion at this paper doc

pnkfelix (Feb 11 2019 at 13:31, on Zulip):

@Wesley Wiser noted that we may want to change our strategy, if possible, to first abstract the API (while leaving the implementation unchanged)

csmoe (Feb 11 2019 at 14:28, on Zulip):

@pnkfelix so is there anything I can help then?

pnkfelix (Feb 11 2019 at 14:31, on Zulip):

I'm not sure. @eddyb seemed very concerned that any attempt to land NeoPlace was going to be very ugly (and/or inefficient) and repeatedly argued that it would be cleaner to just switch over the representation all at once, without trying to convert between the two representations on the fly. In terms of code-cleanliness in short term and ease-of-reviewing, I don't currently agree. But I don't have much of an argument about the inefficiency claim.

pnkfelix (Feb 11 2019 at 14:32, on Zulip):

The proposal to change the API (namely by putting iterator methods in place that work on either the recursive rep or the array-of-projs rep) was made by @Wesley Wiser . I am currently experimenting in a local branch to evaluate whether this would be a realistic avenue.

pnkfelix (Feb 11 2019 at 14:33, on Zulip):

I guess if you (@csmoe ) felt like playing around with that idea, then that would be great. But I also understand that you haven't had much time to work on this task, right?

pnkfelix (Feb 11 2019 at 14:34, on Zulip):

(I had also proposed to @eddyb that we try to set up a video-conf meeting with @Santiago Pastorino and anyone else interested in this issue. I could attempt to include you, @csmoe , in that discussion, if it actually happens. But it might not; nothing has been planned yet.)

Santiago Pastorino (Feb 11 2019 at 14:34, on Zulip):

I'm very interested in continuing with this also given that we have been putting some work on it

Santiago Pastorino (Feb 11 2019 at 14:35, on Zulip):

I've also asked @oli about this because that's what @nikomatsakis suggested me to do

pnkfelix (Feb 11 2019 at 14:36, on Zulip):

@Santiago Pastorino , do you just mean you spoke with @oli about this last week? Or did you make plans to continue talking to @oli in the near future about this topic?

csmoe (Feb 11 2019 at 14:36, on Zulip):

@pnkfelix okay, get it.
I am just back from the Chinese New Year, having plenty of time to spend. and I'm interested in the conf.

pnkfelix (Feb 11 2019 at 14:36, on Zulip):

Ah great thanks for correcting me about that, @csmoe

Santiago Pastorino (Feb 11 2019 at 14:53, on Zulip):

@Santiago Pastorino , do you just mean you spoke with @oli about this last week? Or did you make plans to continue talking to @oli in the near future about this topic?

I've just talked to @oli after the meeting to know how this is going to continue

oli (Feb 11 2019 at 16:14, on Zulip):

can we pull this topic into #t-compiler ? While nll is a user that needs it, it's not nll specific

davidtwco (Feb 11 2019 at 16:22, on Zulip):

it's not possible to move topics between streams, you'd just need to start a new topic.

Santiago Pastorino (Feb 11 2019 at 20:26, on Zulip):

have closed the PR we have been working on, after we figure our exactly what to do we can start working on a new one :)

Last update: Nov 22 2019 at 00:25UTC