Stream: t-compiler/wg-nll

Topic: #53394 do `move_outs` computation lazilly


nikomatsakis (Aug 15 2018 at 22:27, on Zulip):

So @Santiago Pastorino, this PR does a reverse DFS https://github.com/rust-lang/rust/pull/53314/ -- but I realize it is perhaps a bit different than what you would do, since it does it based on PointIndex values (which map to Location)

nikomatsakis (Aug 15 2018 at 22:27, on Zulip):

the heart of the search though is this:

    fn compute_use_live_points_for(&mut self, live_local: LocalWithRegion) {
        self.stack.extend(self.cx.local_use_map.uses(live_local));
        while let Some(p) = self.stack.pop() {
            if self.defs.contains(p) {
                continue;
            }
            if self.use_live_at.insert(p) {
                self.cx
                    .point_index_map
                    .push_predecessors(p, &mut self.stack)
            }
        }
    }
nikomatsakis (Aug 15 2018 at 22:28, on Zulip):

from here

nikomatsakis (Aug 15 2018 at 22:28, on Zulip):

but I think to rewrite in terms of locations it would look something like

nikomatsakis (Aug 15 2018 at 22:29, on Zulip):
let mut stack = vec![initial_location];

while let Some(l) = stack.pop() {
  if already_visited(l) { continue; }
  if l.statement_index == 0 {
    // start of basic block
    for predecessor_block in mir.predecessors_for(l.block) {
       let predecessor_end_location = mir.terminator_loc(predecessor_block);
        stack.push(predecessor_end_location);
    } else {
        stack.push(Location { block: l.block, statement_index: l.statement_index - 1 });
    }
  }
}
nikomatsakis (Aug 15 2018 at 22:30, on Zulip):

where terminator_loc is the helper I added here and which you probably want to just add in your PR as well:

pub fn terminator_loc(&self, bb: BasicBlock) -> Location {
        Location {
            block: bb,
            statement_index: self[bb].statements.len(),
        }
    }
nikomatsakis (Aug 15 2018 at 22:30, on Zulip):

does that make any sense?

Santiago Pastorino (Aug 15 2018 at 22:32, on Zulip):

yeap

Santiago Pastorino (Aug 15 2018 at 22:32, on Zulip):

makes sense

Santiago Pastorino (Aug 15 2018 at 22:33, on Zulip):

@nikomatsakis I was wondering about some types, in particular MoveData

Santiago Pastorino (Aug 15 2018 at 22:33, on Zulip):

for instance, what does this https://github.com/rust-lang/rust/blob/0f4b4987cd6dea5406dec0634770839fb31ce72c/src/librustc_mir/dataflow/move_paths/mod.rs#L131 represents?

Santiago Pastorino (Aug 15 2018 at 22:33, on Zulip):

mpi is the actual location where the move happened?

Santiago Pastorino (Aug 15 2018 at 22:33, on Zulip):

or what?

Santiago Pastorino (Aug 15 2018 at 22:34, on Zulip):

well the actual location I guess it's content.loc

Santiago Pastorino (Aug 15 2018 at 22:34, on Zulip):

I'm not sure what mpi is and what is that mapping

nikomatsakis (Aug 15 2018 at 22:34, on Zulip):

so

nikomatsakis (Aug 15 2018 at 22:35, on Zulip):

a MovePathIndex is the index of a particular place

nikomatsakis (Aug 15 2018 at 22:35, on Zulip):

in particular, we map each place that is moved from (e.g., a.b.c) to an MPI

nikomatsakis (Aug 15 2018 at 22:35, on Zulip):

I believe then that this map you pointed me at

nikomatsakis (Aug 15 2018 at 22:36, on Zulip):

tells us each of the MoveOutIndex values that move a particular MPI

nikomatsakis (Aug 15 2018 at 22:36, on Zulip):

a MoveOutIndex is the unique index given to each move

nikomatsakis (Aug 15 2018 at 22:36, on Zulip):

you can look up more information about a move here:

nikomatsakis (Aug 15 2018 at 22:36, on Zulip):

https://github.com/rust-lang/rust/blob/0f4b4987cd6dea5406dec0634770839fb31ce72c/src/librustc_mir/dataflow/move_paths/mod.rs#L125

nikomatsakis (Aug 15 2018 at 22:36, on Zulip):

see also the MoveOut structure

nikomatsakis (Aug 15 2018 at 22:36, on Zulip):

make sense @Santiago Pastorino ?

Santiago Pastorino (Aug 15 2018 at 22:37, on Zulip):

got distracted reading the code :)

Santiago Pastorino (Aug 15 2018 at 22:37, on Zulip):

let me read what you've said

Santiago Pastorino (Aug 15 2018 at 22:38, on Zulip):

ahh I see

Santiago Pastorino (Aug 15 2018 at 22:39, on Zulip):

let me see if I got it right

Santiago Pastorino (Aug 15 2018 at 22:39, on Zulip):

so when you have a.b.c

Santiago Pastorino (Aug 15 2018 at 22:40, on Zulip):

you have a map from and index of that path the mpi, which I guess it containts the location where that shows up? to the places a, a.b and a.b.c ?

Santiago Pastorino (Aug 15 2018 at 22:40, on Zulip):

is more or less that what you meant?

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

so yes there is an MPI for a.b.c., but also for a.b and a

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

and they are linked

nikomatsakis (Aug 15 2018 at 22:44, on Zulip):

i.e., I can go to the "parents" of the MPI for a.b.c and it will give me a.b and a

nikomatsakis (Aug 15 2018 at 22:44, on Zulip):

the MPI itself just maps to a place

Santiago Pastorino (Aug 15 2018 at 22:44, on Zulip):

ok

nikomatsakis (Aug 15 2018 at 22:44, on Zulip):

then the MoveOut structure indicates a particular move of some place at some location

nikomatsakis (Aug 15 2018 at 22:44, on Zulip):

and so we have a map from the MPI To the move-outs that involve that MPI

nikomatsakis (Aug 15 2018 at 22:44, on Zulip):

so that gives you -- basically -- all the places that an MPI is moved

Santiago Pastorino (Aug 15 2018 at 22:45, on Zulip):

:+1:

Santiago Pastorino (Aug 15 2018 at 23:00, on Zulip):

so in that search if I need to search for an assign to that place

Santiago Pastorino (Aug 15 2018 at 23:01, on Zulip):

@nikomatsakis I remember you showed me some code this morning about this

Santiago Pastorino (Aug 15 2018 at 23:01, on Zulip):

I need to search for the kills

nikomatsakis (Aug 15 2018 at 23:02, on Zulip):

mmm

nikomatsakis (Aug 15 2018 at 23:02, on Zulip):

why do you want to search for that? :)

Santiago Pastorino (Aug 15 2018 at 23:02, on Zulip):

don't we need to explain why the thing was moved?

nikomatsakis (Aug 15 2018 at 23:02, on Zulip):

I think you want to check for the moves that occur at a particular location, right?

nikomatsakis (Aug 15 2018 at 23:03, on Zulip):

(vs assignments)

nikomatsakis (Aug 15 2018 at 23:03, on Zulip):

and I think that this field is what you want

Santiago Pastorino (Aug 15 2018 at 23:04, on Zulip):

yeah, I'm probably not understanding something correctly

Santiago Pastorino (Aug 15 2018 at 23:04, on Zulip):

so

Santiago Pastorino (Aug 15 2018 at 23:04, on Zulip):
    pub(super) fn report_use_of_moved_or_uninitialized(
        &mut self,
        context: Context,
        desired_action: InitializationRequiringAction,
        (place, span): (&Place<'tcx>, Span),
        mpi: MovePathIndex,
Santiago Pastorino (Aug 15 2018 at 23:05, on Zulip):

I was guessing that mpi it's the index for the place that's being used after a move

Santiago Pastorino (Aug 15 2018 at 23:05, on Zulip):

and given that we don't have the information why that thing was moved

Santiago Pastorino (Aug 15 2018 at 23:05, on Zulip):

we were going to search for that

Santiago Pastorino (Aug 15 2018 at 23:07, on Zulip):

so I guess you're saying that we do that search looking for the moves that are happening and see if one of those moves is the one I'm about to report?

Santiago Pastorino (Aug 15 2018 at 23:07, on Zulip):

is that what you meant?

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

that sounds right

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

I think we want to search backwards from context.loc

Santiago Pastorino (Aug 15 2018 at 23:11, on Zulip):

yes

nikomatsakis (Aug 15 2018 at 23:11, on Zulip):

at each place, we can iterate over the moves that occur there

nikomatsakis (Aug 15 2018 at 23:11, on Zulip):

and check which paths they affect

nikomatsakis (Aug 15 2018 at 23:11, on Zulip):

in particular look for a MoveOut that moves mpi

nikomatsakis (Aug 15 2018 at 23:11, on Zulip):

when we find that, we can stop the search

Santiago Pastorino (Aug 15 2018 at 23:11, on Zulip):

so for the sake of understanding a bit better, why does this don't we need to explain why the thing was moved? would be wrong?

nikomatsakis (Aug 15 2018 at 23:11, on Zulip):

maybe I'm not sure what you mean by that

nikomatsakis (Aug 15 2018 at 23:11, on Zulip):

it seems like finding the moves

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

is explaining the thing that was moved

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

that is, we want to find the moves, so we can show them

Santiago Pastorino (Aug 15 2018 at 23:12, on Zulip):

ahh the thing is the moves where already calculated, right?

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

it occurs to me that we will also need to search each place for the initializations, which is maybe what you meant initially

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

ah, right, yes. the MoveData already has indexed the moves that occur at any given location

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

so we are just searching backwards through that index

Santiago Pastorino (Aug 15 2018 at 23:12, on Zulip):

what I've said "calculates" that again

Santiago Pastorino (Aug 15 2018 at 23:12, on Zulip):

ok

Santiago Pastorino (Aug 15 2018 at 23:13, on Zulip):

makes a lot of sense

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

but we also need to look for initialization, because of examples like this:

let mut x = vec![];
drop(x);
x = vec![];
if true { drop(x); }
println!("{:?}", x); // <-- error here
nikomatsakis (Aug 15 2018 at 23:13, on Zulip):

in particular, we would walk backwards and find the second drop(x)

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

and when we encoutner the x = vec![] we want to stop the search there too

Santiago Pastorino (Aug 15 2018 at 23:13, on Zulip):

that's more or less what I mean when I ask you about data structures and the information that you have around, it's easy to see what to do, but harder to see what stuff you have around and all that :)

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

and not keep going (else we wil also find the first x = vec![])

Santiago Pastorino (Aug 15 2018 at 23:14, on Zulip):

the code was going ...

Santiago Pastorino (Aug 15 2018 at 23:14, on Zulip):
 let mois = self.move_data.path_map[mpi]
            .iter()
            .filter(|moi| curr_move_out.contains(moi)))
            .collect::<Vec<_>>();
Santiago Pastorino (Aug 15 2018 at 23:14, on Zulip):

I guess I should keep returning mois

Santiago Pastorino (Aug 15 2018 at 23:14, on Zulip):

?

nikomatsakis (Aug 15 2018 at 23:15, on Zulip):

yes probably

Santiago Pastorino (Aug 15 2018 at 23:15, on Zulip):

which I don't remember what I was :P

nikomatsakis (Aug 15 2018 at 23:15, on Zulip):

that'd be easy enough to do anyway

Santiago Pastorino (Aug 15 2018 at 23:15, on Zulip):

ok

nikomatsakis (Aug 15 2018 at 23:15, on Zulip):

that is just a unique index

nikomatsakis (Aug 15 2018 at 23:15, on Zulip):

assigned to each MoveOut

Santiago Pastorino (Aug 15 2018 at 23:15, on Zulip):

ok

nikomatsakis (Aug 15 2018 at 23:15, on Zulip):

(which are collected into a vector)

Santiago Pastorino (Aug 15 2018 at 23:15, on Zulip):

so last thing I'm not sure about

Santiago Pastorino (Aug 15 2018 at 23:16, on Zulip):

you said and when we encoutner the x = vec![] we want to stop the search there too

Santiago Pastorino (Aug 15 2018 at 23:16, on Zulip):

well we stop when the thing was first initialized I guess

Santiago Pastorino (Aug 15 2018 at 23:16, on Zulip):

but if it was moved we keep searching ?

Santiago Pastorino (Aug 15 2018 at 23:16, on Zulip):

don't remember what's actually being reported but if it's moved in more than one place I guess we want all of them

Santiago Pastorino (Aug 15 2018 at 23:17, on Zulip):

and that's I guess why is mois and not moi

nikomatsakis (Aug 15 2018 at 23:17, on Zulip):

but if it was moved we keep searching ?

no, I think we stop when we encounter a move

nikomatsakis (Aug 15 2018 at 23:17, on Zulip):

and that's I guess why is mois and not moi

yes

nikomatsakis (Aug 15 2018 at 23:17, on Zulip):

the MoveData should also track which was initialized at each spot

nikomatsakis (Aug 15 2018 at 23:17, on Zulip):

in terms of the dataflow...

nikomatsakis (Aug 15 2018 at 23:17, on Zulip):

the moves are a gen (and we can stop there, because if there was a preceding move, it'd be an error to move twice)

nikomatsakis (Aug 15 2018 at 23:18, on Zulip):

and the initializations are a kill

nikomatsakis (Aug 15 2018 at 23:18, on Zulip):

(and we can stop there, because if there was a preceding move, it would have been killed)

nikomatsakis (Aug 15 2018 at 23:18, on Zulip):

so actually looking at the impl and seeing how it defines gen and kill

nikomatsakis (Aug 15 2018 at 23:18, on Zulip):

can help you to navigate the data structures

nikomatsakis (Aug 15 2018 at 23:18, on Zulip):

one sec I will give you the link

Santiago Pastorino (Aug 15 2018 at 23:19, on Zulip):

I still not see where the plural of mois come from

nikomatsakis (Aug 15 2018 at 23:19, on Zulip):

that: https://github.com/rust-lang/rust/blob/0f4b4987cd6dea5406dec0634770839fb31ce72c/src/librustc_mir/dataflow/impls/mod.rs#L491-L566

Santiago Pastorino (Aug 15 2018 at 23:19, on Zulip):

if you stop in the first move

nikomatsakis (Aug 15 2018 at 23:19, on Zulip):

well there can be multiple paths

nikomatsakis (Aug 15 2018 at 23:19, on Zulip):

e.g.

nikomatsakis (Aug 15 2018 at 23:19, on Zulip):
if true {
  drop(x);
} else {
  drop(x);
}
use(x);
Santiago Pastorino (Aug 15 2018 at 23:19, on Zulip):

ahh ya

Santiago Pastorino (Aug 15 2018 at 23:19, on Zulip):

right

Santiago Pastorino (Aug 15 2018 at 23:19, on Zulip):

that's tracked by kills and gens

Santiago Pastorino (Aug 15 2018 at 23:19, on Zulip):

ok

nikomatsakis (Aug 15 2018 at 23:21, on Zulip):

@Santiago Pastorino here is the code that generates the *kills* at a given location -- in other words, to walk the set of paths initialized

nikomatsakis (Aug 15 2018 at 23:21, on Zulip):

this I think generates the gens

Santiago Pastorino (Aug 15 2018 at 23:31, on Zulip):

I just need one of those, right?

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

one of whats? :)

Santiago Pastorino (Aug 15 2018 at 23:32, on Zulip):

kills or gens

Santiago Pastorino (Aug 15 2018 at 23:32, on Zulip):

so I keep iterating meanwhile I see the thing in the kills set

nikomatsakis (Aug 15 2018 at 23:32, on Zulip):

well I think we prob want to stop the search at either an initialization (kill) or a move (gen)

nikomatsakis (Aug 15 2018 at 23:32, on Zulip):

so we kind of have to look for both I guess?

Santiago Pastorino (Aug 15 2018 at 23:32, on Zulip):

ahh right

Santiago Pastorino (Aug 15 2018 at 23:32, on Zulip):

ok

Santiago Pastorino (Aug 16 2018 at 20:29, on Zulip):

@nikomatsakis just had a chance to leave this thing running

Santiago Pastorino (Aug 16 2018 at 20:30, on Zulip):

there's a test failing

Santiago Pastorino (Aug 16 2018 at 20:30, on Zulip):

still running

Santiago Pastorino (Aug 16 2018 at 20:32, on Zulip):

I mean, so we are covered :)

Santiago Pastorino (Aug 16 2018 at 20:32, on Zulip):

I need to add the "initialized" code

Santiago Pastorino (Aug 16 2018 at 20:36, on Zulip):
failures:

---- [ui] ui/borrowck/borrowck-storage-dead.rs stdout ----

The actual stderr differed from the expected stderr.
Actual stderr saved to /home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/test/ui/borrowck/borrowck-storage-dead/borrowck-storage-dead.stderr
Actual stderr saved to /home/santiago/src/oss/rust1/src/test/ui/borrowck/borrowck-storage-dead.stderr

error: /home/santiago/src/oss/rust1/src/test/ui/borrowck/borrowck-storage-dead.rs:28: unexpected error: '28:17: 28:18: use of moved value: `x` (Mir) [E0382]'

error: /home/santiago/src/oss/rust1/src/test/ui/borrowck/borrowck-storage-dead.rs:28: expected error not found: (Mir) [E0381]

error: 1 unexpected errors found, 1 expected errors not found
status: exit code: 1
command: "/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "/home/santiago/src/oss/rust1/src/test/ui/borrowck/borrowck-storage-dead.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/test/ui/borrowck/borrowck-storage-dead/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "borrowck=compare" "-L" "/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/test/ui/borrowck/borrowck-storage-dead/auxiliary" "-A" "unused"
unexpected errors (from JSON output): [
    Error {
        line_num: 28,
        kind: Some(
            Error
        ),
        msg: "28:17: 28:18: use of moved value: `x` (Mir) [E0382]"
    }
]

not found errors (from test file): [
    Error {
        line_num: 28,
        kind: Some(
            Error
        ),
        msg: "(Mir) [E0381]"
    }
]

thread '[ui] ui/borrowck/borrowck-storage-dead.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:1283:13
stack backtrace:
   0:     0x556149dabcee - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::hab08c28baa7df427
                               at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1:     0x556149d9c3d6 - std::sys_common::backtrace::print::hb73053b1bfdbfebe
                               at libstd/sys_common/backtrace.rs:71
                               at libstd/sys_common/backtrace.rs:59
   2:     0x556149d98c1d - std::panicking::default_hook::{{closure}}::h3f1dc75ac5c59882
                               at libstd/panicking.rs:211
   3:     0x556149d9892b - std::panicking::default_hook::h6e2cc1a5e193171c
                               at libstd/panicking.rs:221
   4:     0x556149d9928c - std::panicking::rust_panic_with_hook::h75e1df07c2b91379
                               at libstd/panicking.rs:475
   5:     0x556149c6b1c6 - std::panicking::begin_panic::he049bbdeb48f06a3
   6:     0x556149c86bc9 - compiletest::runtest::TestCx::check_expected_errors::h1c23466ab236c27d
   7:     0x556149c7bc29 - compiletest::runtest::TestCx::run_revision::heaee5cdb04caf6d8
   8:     0x556149c743eb - compiletest::runtest::run::h70337afe44c21fe2
   9:     0x556149c99ca2 - <F as alloc::boxed::FnBox<A>>::call_box::h827c94cb4a225c9d
  10:     0x556149cdff42 - <F as alloc::boxed::FnBox<A>>::call_box::h389fb44a69e56229
                               at libtest/lib.rs:1444
                               at /checkout/src/liballoc/boxed.rs:642
  11:     0x556149dbe949 - __rust_maybe_catch_panic
                               at libpanic_unwind/lib.rs:105
  12:     0x556149cfcfef - std::sys_common::backtrace::__rust_begin_short_backtrace::h82f6704267b276a2
                               at /checkout/src/libstd/panicking.rs:289
                               at /checkout/src/libstd/panic.rs:392
                               at libtest/lib.rs:1406
                               at /checkout/src/libstd/sys_common/backtrace.rs:136
  13:     0x556149cfd9a4 - std::panicking::try::do_call::h14201cddbab609e9
                               at /checkout/src/libstd/thread/mod.rs:409
                               at /checkout/src/libstd/panic.rs:313
                               at /checkout/src/libstd/panicking.rs:310
  14:     0x556149dbe949 - __rust_maybe_catch_panic
                               at libpanic_unwind/lib.rs:105
  15:     0x556149cf33a6 - <F as alloc::boxed::FnBox<A>>::call_box::h22e4c29e48b9d747
                               at /checkout/src/libstd/panicking.rs:289
                               at /checkout/src/libstd/panic.rs:392
                               at /checkout/src/libstd/thread/mod.rs:408
                               at /checkout/src/liballoc/boxed.rs:642
  16:     0x556149d9b0ca - std::sys_common::thread::start_thread::h54ff7746c22c98d3
                               at /checkout/src/liballoc/boxed.rs:652
                               at libstd/sys_common/thread.rs:24
  17:     0x556149d997f5 - std::sys::unix::thread::Thread::new::thread_start::h817575057e7b413b
                               at libstd/sys/unix/thread.rs:90
  18:     0x7fbfa5206a8c - start_thread
  19:     0x7fbfa511c822 - clone
  20:                0x0 - <unknown>

---- [ui] ui/issues/issue-15919.rs stdout ----

error: Error: expected failure status (Some(1)) but received status None.
status: signal: 6
command: "/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "/home/santiago/src/oss/rust1/src/test/ui/issues/issue-15919.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-15919/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-15919/auxiliary" "-A" "unused"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
{"message":"the type `[usize; 18446744073709551615]` is too big for the current architecture","code":null,"level":"error","spans":[],"children":[],"rendered":"error: the type `[usize; 18446744073709551615]` is too big for the current architecture\n\n"}
{"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to previous error\n\n"}
rustc: /home/santiago/src/oss/rust1/src/llvm/lib/CodeGen/TargetPassConfig.cpp:812: virtual void llvm::TargetPassConfig::addMachinePasses(): Assertion `TPI && IPI && "Pass ID not registered!"' failed.

------------------------------------------

thread '[ui] ui/issues/issue-15919.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3166:9
stack backtrace:
   0:     0x556149dabcee - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::hab08c28baa7df427
                               at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1:     0x556149d9c3d6 - std::sys_common::backtrace::print::hb73053b1bfdbfebe
                               at libstd/sys_common/backtrace.rs:71
                               at libstd/sys_common/backtrace.rs:59
   2:     0x556149d98c1d - std::panicking::default_hook::{{closure}}::h3f1dc75ac5c59882
                               at libstd/panicking.rs:211
   3:     0x556149d9892b - std::panicking::default_hook::h6e2cc1a5e193171c
                               at libstd/panicking.rs:221
   4:     0x556149d9928c - std::panicking::rust_panic_with_hook::h75e1df07c2b91379
                               at libstd/panicking.rs:475
   5:     0x556149c6b1c6 - std::panicking::begin_panic::he049bbdeb48f06a3
   6:     0x556149c8fc3f - compiletest::runtest::ProcRes::fatal::hc58d063b69dc5824
   7:     0x556149c8b7f2 - compiletest::runtest::TestCx::fatal_proc_rec::h748991f6d3edcb2a
   8:     0x556149c7fa4f - compiletest::runtest::TestCx::check_correct_failure_status::hd887d1deee64ae79
   9:     0x556149c7b073 - compiletest::runtest::TestCx::run_revision::heaee5cdb04caf6d8
  10:     0x556149c743eb - compiletest::runtest::run::h70337afe44c21fe2
  11:     0x556149c99ca2 - <F as alloc::boxed::FnBox<A>>::call_box::h827c94cb4a225c9d
  12:     0x556149cdff42 - <F as alloc::boxed::FnBox<A>>::call_box::h389fb44a69e56229
                               at libtest/lib.rs:1444
                               at /checkout/src/liballoc/boxed.rs:642
  13:     0x556149dbe949 - __rust_maybe_catch_panic
                               at libpanic_unwind/lib.rs:105
  14:     0x556149cfcfef - std::sys_common::backtrace::__rust_begin_short_backtrace::h82f6704267b276a2
                               at /checkout/src/libstd/panicking.rs:289
                               at /checkout/src/libstd/panic.rs:392
                               at libtest/lib.rs:1406
                               at /checkout/src/libstd/sys_common/backtrace.rs:136
  15:     0x556149cfd9a4 - std::panicking::try::do_call::h14201cddbab609e9
                               at /checkout/src/libstd/thread/mod.rs:409
                               at /checkout/src/libstd/panic.rs:313
                               at /checkout/src/libstd/panicking.rs:310
  16:     0x556149dbe949 - __rust_maybe_catch_panic
                               at libpanic_unwind/lib.rs:105
  17:     0x556149cf33a6 - <F as alloc::boxed::FnBox<A>>::call_box::h22e4c29e48b9d747
                               at /checkout/src/libstd/panicking.rs:289
                               at /checkout/src/libstd/panic.rs:392
                               at /checkout/src/libstd/thread/mod.rs:408
                               at /checkout/src/liballoc/boxed.rs:642
  18:     0x556149d9b0ca - std::sys_common::thread::start_thread::h54ff7746c22c98d3
                               at /checkout/src/liballoc/boxed.rs:652
                               at libstd/sys_common/thread.rs:24
  19:     0x556149d997f5 - std::sys::unix::thread::Thread::new::thread_start::h817575057e7b
Santiago Pastorino (Aug 16 2018 at 21:53, on Zulip):

@nikomatsakis pushed something https://github.com/rust-lang/rust/pull/53403, it's failing need to figure out what's going on after this thing runs

Santiago Pastorino (Aug 16 2018 at 21:53, on Zulip):

but if you want to check how it goes you can see the commit there

nikomatsakis (Aug 16 2018 at 21:53, on Zulip):

I mean, so we are covered :)

nice :)

Santiago Pastorino (Aug 18 2018 at 20:00, on Zulip):

debugging this issue

Santiago Pastorino (Aug 18 2018 at 20:00, on Zulip):

seeing this in the mir dump

Santiago Pastorino (Aug 18 2018 at 20:01, on Zulip):
// MIR for `main`
// source = MirSource { def_id: DefId(0/0:3 ~ test[317d]::main[0]), promoted: None }
// pass_name = nll
// disambiguator = 0

| Free Region Mapping
| '_#0r    | Global   | ['_#0r, '_#1r]
| '_#1r    | Local    | ['_#1r]
|
| Inferred Region Values
| '_#0r    | U0 | {bb0[0..=127], '_#0r}
| '_#1r    | U0 | {bb0[0..=127], '_#1r}
|
| Inference Constraints
| '_#0r live at {bb0[0..=127]}
| '_#1r live at {bb0[0..=127]}
fn main() -> (){
    let mut _0: ();                      // return place
    scope 1 {
    }
    scope 2 {
        let _1: i32;                     // "x" in scope 2 at test.rs:2:13: 2:14
    }
    let mut _2: i32;
    let mut _3: i32;

    bb0: {
                                         | Live variables on entry to bb0[0]: []
        UserAssertTy(Canonical { variables: [], value: i32 }, _1); // bb0[0]: scope 0 at test.rs:1:11: 5:2
                                         | Live variables on entry to bb0[1]: []
        StorageLive(_1);                 // bb0[1]: scope 0 at test.rs:2:13: 2:14
                                         | Live variables on entry to bb0[2]: []
        StorageLive(_2);                 // bb0[2]: scope 1 at test.rs:3:17: 3:22
                                         | Live variables on entry to bb0[3]: []
        StorageLive(_3);                 // bb0[3]: scope 1 at test.rs:3:17: 3:18
                                         | Live variables on entry to bb0[4]: []
        _3 = _1;                         // bb0[4]: scope 1 at test.rs:3:17: 3:18
                                         | Live variables on entry to bb0[5]: []
        _2 = Add(move _3, const 1i32);   // bb0[5]: scope 1 at test.rs:3:17: 3:22
                                         // ty::Const
                                         // + ty: i32
                                         // + val: Scalar(Bits { size: 4, bits: 1 })
                                         // mir::Constant
                                         // + span: test.rs:3:21: 3:22
                                         // + ty: i32
                                         // + literal: Const { ty: i32, val: Scalar(Bits { size: 4, bits: 1 }) }
                                         | Live variables on entry to bb0[6]: []
        StorageDead(_3);                 // bb0[6]: scope 1 at test.rs:3:21: 3:22
                                         | Live variables on entry to bb0[7]: []
        StorageDead(_2);                 // bb0[7]: scope 1 at test.rs:3:22: 3:23
                                         | Live variables on entry to bb0[8]: []
        _0 = ();                         // bb0[8]: scope 0 at test.rs:1:11: 5:2
                                         | Live variables on entry to bb0[9]: []
        StorageDead(_1);                 // bb0[9]: scope 0 at test.rs:5:1: 5:2
                                         | Live variables on entry to bb0[10]: []
        return;                          // bb0[10]: scope 0 at test.rs:5:2: 5:2
    | Live variables on exit from bb0: []
    }
}
Santiago Pastorino (Aug 18 2018 at 20:01, on Zulip):

in particular I don't understand _2 = Add(move _3, const 1i32); // bb0[5]: scope 1 at test.rs:3:17: 3:22

Santiago Pastorino (Aug 18 2018 at 20:01, on Zulip):

why it says move there?

Santiago Pastorino (Aug 18 2018 at 20:02, on Zulip):

_3 is x and it's an i32

Matthew Jasper (Aug 18 2018 at 20:02, on Zulip):

Temporaries are always moved

Matthew Jasper (Aug 18 2018 at 20:02, on Zulip):

Well, operands at least

Santiago Pastorino (Aug 18 2018 at 20:02, on Zulip):

:+1:

Santiago Pastorino (Aug 19 2018 at 00:03, on Zulip):

gonna leave a couple of comments about what I've found on this issue, for @nikomatsakis to check out and give some pointers :)

Santiago Pastorino (Aug 19 2018 at 00:03, on Zulip):

so ...

Santiago Pastorino (Aug 19 2018 at 00:03, on Zulip):

this code ...

Santiago Pastorino (Aug 19 2018 at 00:03, on Zulip):
fn main() {
    loop {
        let x: i32;
        let _ = x + 1; //~ERROR (Ast) [E0381]
                       //~^ ERROR (Mir) [E0381]
    }
}
Santiago Pastorino (Aug 19 2018 at 00:04, on Zulip):

this is the code that make the current PR fail

Santiago Pastorino (Aug 19 2018 at 00:05, on Zulip):

the thing is that the algorithm is returning a collection with one moi when it should be empty

Santiago Pastorino (Aug 19 2018 at 00:05, on Zulip):

this is the mir dump ...

Santiago Pastorino (Aug 19 2018 at 00:05, on Zulip):
// MIR for `main`
// source = MirSource { def_id: DefId(0/0:3 ~ test[317d]::main[0]), promoted: None }
// pass_name = nll
// disambiguator = 0

| Free Region Mapping
| '_#0r    | Global   | ['_#0r, '_#1r]
| '_#1r    | Local    | ['_#1r]
|
| Inferred Region Values
| '_#0r    | U0 | {bb0[0], bb1[0], bb2[0..=10], bb3[0..=114], '_#0r}
| '_#1r    | U0 | {bb0[0], bb1[0], bb2[0..=10], bb3[0..=114], '_#1r}
|
| Inference Constraints
| '_#0r live at {bb0[0], bb1[0], bb2[0..=10], bb3[0..=114]}
| '_#1r live at {bb0[0], bb1[0], bb2[0..=10], bb3[0..=114]}
fn main() -> (){
    let mut _0: ();                      // return place
    scope 1 {
    }
    scope 2 {
        let _3: i32;                     // "x" in scope 2 at test.rs:3:13: 3:14
    }
    let mut _1: !;
    let mut _2: ();
    let mut _4: i32;
    let mut _5: i32;

    bb0: {
                                         | Live variables on entry to bb0[0]: []
        goto -> bb1;                     // bb0[0]: scope 0 at test.rs:2:5: 6:6
    | Live variables on exit from bb0: []
    }

    bb1: {
                                         | Live variables on entry to bb1[0]: []
        falseUnwind -> [real: bb2, cleanup: bb3]; // bb1[0]: scope 0 at test.rs:2:5: 6:6
    | Live variables on exit from bb1: []
    }

    bb2: {
                                         | Live variables on entry to bb2[0]: []
        UserAssertTy(Canonical { variables: [], value: i32 }, _3); // bb2[0]: scope 0 at test.rs:2:10: 6:6
                                         | Live variables on entry to bb2[1]: []
        StorageLive(_3);                 // bb2[1]: scope 0 at test.rs:3:13: 3:14
                                         | Live variables on entry to bb2[2]: []
        StorageLive(_4);                 // bb2[2]: scope 1 at test.rs:4:17: 4:22
                                         | Live variables on entry to bb2[3]: []
        StorageLive(_5);                 // bb2[3]: scope 1 at test.rs:4:17: 4:18
                                         | Live variables on entry to bb2[4]: []
        _5 = _3;                         // bb2[4]: scope 1 at test.rs:4:17: 4:18
                                         | Live variables on entry to bb2[5]: []
        _4 = Add(move _5, const 1i32);   // bb2[5]: scope 1 at test.rs:4:17: 4:22
                                         // ty::Const
                                         // + ty: i32
                                         // + val: Scalar(Bits { size: 4, bits: 1 })
                                         // mir::Constant
                                         // + span: test.rs:4:21: 4:22
                                         // + ty: i32
                                         // + literal: Const { ty: i32, val: Scalar(Bits { size: 4, bits: 1 }) }
                                         | Live variables on entry to bb2[6]: []
        StorageDead(_5);                 // bb2[6]: scope 1 at test.rs:4:21: 4:22
                                         | Live variables on entry to bb2[7]: []
        StorageDead(_4);                 // bb2[7]: scope 1 at test.rs:4:22: 4:23
                                         | Live variables on entry to bb2[8]: []
        _2 = ();                         // bb2[8]: scope 0 at test.rs:2:10: 6:6
                                         | Live variables on entry to bb2[9]: []
        StorageDead(_3);                 // bb2[9]: scope 0 at test.rs:6:5: 6:6
                                         | Live variables on entry to bb2[10]: []
        goto -> bb1;                     // bb2[10]: scope 0 at test.rs:2:5: 6:6
    | Live variables on exit from bb2: []
    }

    bb3: {                               // cleanup
                                         | Live variables on entry to bb3[0]: []
        resume;                          // bb3[0]: scope 0 at test.rs:1:1: 7:2
    | Live variables on exit from bb3: []
    }
}
Santiago Pastorino (Aug 19 2018 at 00:06, on Zulip):

what I'm seeing when I run with logs enabled is this ...

Santiago Pastorino (Aug 19 2018 at 00:07, on Zulip):
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: MirBorrowckCtxt::process_statement(bb2[0], UserAssertTy(Canonical { variables: [], value: i32 }, _3)): borrows in effect: [] borrows generated: [] uninits: [_0, _1, _2, _3, _4, _5] ever_init: [mp2@test.rs:2:10: 6:6 (Deep)]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: MirBorrowckCtxt::process_statement(bb2[1], StorageLive(_3)): borrows in effect: [] borrows generated: [] uninits: [_0, _1, _2, _3, _4, _5] ever_init: [mp2@test.rs:2:10: 6:6 (Deep)]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: MirBorrowckCtxt::process_statement(bb2[2], StorageLive(_4)): borrows in effect: [] borrows generated: [] uninits: [_0, _1, _2, _3, _4, _5] ever_init: [mp2@test.rs:2:10: 6:6 (Deep)]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: MirBorrowckCtxt::process_statement(bb2[3], StorageLive(_5)): borrows in effect: [] borrows generated: [] uninits: [_0, _1, _2, _3, _4, _5] ever_init: [mp2@test.rs:2:10: 6:6 (Deep)]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: MirBorrowckCtxt::process_statement(bb2[4], _5 = _3): borrows in effect: [] borrows generated: [] uninits: [_0, _1, _2, _3, _4, _5] ever_init: [mp2@test.rs:2:10: 6:6 (Deep)]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: check_access_permissions(_3, Read(Copy), No)
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: check_access_for_conflict(context=Context { kind: AssignRhs, loc: bb2[4] }, place_span=(_3, test.rs:4:17: 4:18), sd=Deep, rw=Read(Copy))
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: check_if_full_path_is_moved place: _3
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[3]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[2]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[1]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[0]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb1[0]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[10]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[9]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: moi=mo3
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: found
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb0[0]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: mois=[mo3]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: check_if_path_or_subpath_is_moved place: _3
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[3]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[2]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[1]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[0]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb1[0]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[10]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb2[9]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: moi=mo3
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: found
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: current_location=bb0[0]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check::error_reporting: report_use_of_moved_or_uninitialized: mois=[mo3]
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: check_if_assigned_path_is_moved place: _5
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: check_access_permissions(_5, Write(Mutate), ExceptUpvars)
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: check_access_for_conflict(context=Context { kind: AssignLhs, loc: bb2[4] }, place_span=(_5, test.rs:4:17: 4:18), sd=Shallow(None), rw=Write(Mutate))
DEBUG 2018-08-18T21:10:04Z: rustc_mir::borrow_check: check_if_reassignment_to_immutable_state(_5)
Santiago Pastorino (Aug 19 2018 at 00:09, on Zulip):

there you can see that it start going backwards and from the loop statement it ends restarting from the end and finding the x in x + 1 and it thinks its moved

Santiago Pastorino (Aug 19 2018 at 00:09, on Zulip):

I guess we should stop when a StorageLive for the thing that we are looking for is found

Santiago Pastorino (Aug 19 2018 at 00:09, on Zulip):

@nikomatsakis those are more or less my thoughts, will wait for your comments to see what are the options :)

nikomatsakis (Aug 21 2018 at 16:25, on Zulip):

@Santiago Pastorino :wave: ok I'll try to catch up shortly

Santiago Pastorino (Aug 21 2018 at 19:32, on Zulip):

@nikomatsakis :hi:

Santiago Pastorino (Aug 21 2018 at 19:32, on Zulip):

sure

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

ping @Santiago Pastorino

Santiago Pastorino (Aug 22 2018 at 17:42, on Zulip):

hey

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

want to chat about this at some point? :)

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

not sure what is the current status

Santiago Pastorino (Aug 22 2018 at 17:44, on Zulip):

sure

Santiago Pastorino (Aug 22 2018 at 17:44, on Zulip):

current status is the last stuff I've written :)

Santiago Pastorino (Aug 22 2018 at 17:44, on Zulip):

haven't touched again

Santiago Pastorino (Aug 22 2018 at 17:44, on Zulip):

my way back home was a pain

Santiago Pastorino (Aug 22 2018 at 17:44, on Zulip):

:(

Santiago Pastorino (Aug 22 2018 at 17:44, on Zulip):

had a 12hs delay in the last flight

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

oh wow

Santiago Pastorino (Aug 22 2018 at 17:45, on Zulip):

I didn't even had to complain that the airline already gave me a lot of miles

Santiago Pastorino (Aug 22 2018 at 17:45, on Zulip):

without complaining

Santiago Pastorino (Aug 22 2018 at 17:45, on Zulip):

anyway, I've sent a huge complain

Santiago Pastorino (Aug 22 2018 at 17:45, on Zulip):

because we all think they have lied in a lot of things

Santiago Pastorino (Aug 22 2018 at 17:46, on Zulip):

and basically were avoiding saying that the flight was cancelled to avoid paying a hotel for each of us

Santiago Pastorino (Aug 22 2018 at 17:46, on Zulip):

they kept delaying 2hs

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

yeah it seems like some number of hours is effectively a cancellation

Santiago Pastorino (Aug 22 2018 at 17:46, on Zulip):

then 3 more

Santiago Pastorino (Aug 22 2018 at 17:46, on Zulip):

etc

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

in the EU, at least, I know there's a "bill of rights" about this sort of thing. Not sure about the US, probably not.

Santiago Pastorino (Aug 22 2018 at 17:46, on Zulip):

it was at the end not cancelled

Santiago Pastorino (Aug 22 2018 at 17:46, on Zulip):

it was moved

Santiago Pastorino (Aug 22 2018 at 17:46, on Zulip):

but that happens because this is the aircraft they use to go to MVD

Santiago Pastorino (Aug 22 2018 at 17:47, on Zulip):

they needed to have it in MVD to do the MVD->MIA flight back

Santiago Pastorino (Aug 22 2018 at 17:47, on Zulip):

anyway, back to lazily computation ...

Santiago Pastorino (Aug 22 2018 at 17:47, on Zulip):

if you check the log, the last stuff I've shared are my last thoughts

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

let me step one step further back

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

the problem was that your PR was getting the wrong results?

Santiago Pastorino (Aug 22 2018 at 18:00, on Zulip):

yes

nikomatsakis (Aug 24 2018 at 09:53, on Zulip):

@Santiago Pastorino so looking at the code I do see that the original ignored StorageDead for some reason

nikomatsakis (Aug 24 2018 at 09:53, on Zulip):

I think you also mentioned this

nikomatsakis (Aug 24 2018 at 09:53, on Zulip):

that seems like the most likely error here?

nikomatsakis (Aug 24 2018 at 09:53, on Zulip):

I've been pondering if we are "hard-coding" this too much

nikomatsakis (Aug 24 2018 at 09:54, on Zulip):

er, sorry, that's sort of an independent thought

nikomatsakis (Aug 24 2018 at 09:54, on Zulip):

in particular, I was wondering if we should write a sort of "reverse depth-first search" that works for any BitDenotation

nikomatsakis (Aug 24 2018 at 09:54, on Zulip):

then we could invoke it with the existing MovingOutStatements (the one I asked you to remove from the PR)

nikomatsakis (Aug 24 2018 at 09:54, on Zulip):

that would let us write the code once

nikomatsakis (Aug 24 2018 at 09:55, on Zulip):

and let us sort of re-use exactly what MovingOutStatements does today

nikomatsakis (Aug 24 2018 at 09:55, on Zulip):

otoh it's probably a good idea to just make a targeted edit for now to make sure this StorageDead thing is the problem

nikomatsakis (Aug 24 2018 at 09:56, on Zulip):

to that end, I applied this diff

nikomatsakis (Aug 24 2018 at 09:56, on Zulip):
modified   src/librustc_mir/borrow_check/error_reporting.rs
@@ -544,10 +544,18 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             }

             // check for moves
-            for moi in &self.move_data.loc_map[l] {
-                if self.move_data.moves[*moi].path == mpi {
-                    result.push(*moi);
-                    continue 'dfs;
+            let stmt = &mir[l.block].statements[l.statement_index];
+            if let StatementKind::StorageDead(..) = stmt.kind {
+                // this analysis only tries to find moves explicitly
+                // written by the user, so we ignore the move-outs
+                // created by `StorageDead` and at the beginning
+                // of a function.
+            } else {
+                for moi in &self.move_data.loc_map[l] {
+                    if self.move_data.moves[*moi].path == mpi {
+                        result.push(*moi);
+                        continue 'dfs;
+                    }
                 }
             }
nikomatsakis (Aug 24 2018 at 09:57, on Zulip):

annoyingly I forgot --keep-stage 1, so it'll be a few minutes before I have the answer :P

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

ok, a (slightly tweaked) version of that diff fixes on problem, but issue-29723.rs still fails

nikomatsakis (Aug 24 2018 at 10:14, on Zulip):

I'm not quite sure why

nikomatsakis (Aug 24 2018 at 10:14, on Zulip):
1       error[E0382]: use of moved value: `s`
2         --> $DIR/issue-29723.rs:22:13
3          |
-       LL |         0 if { drop(s); false } => String::from("oops"),
-          |                     - value moved here
-       ...
7       LL |             s
-          |             ^ value used here after move
+          |             ^ value moved here in previous iteration of loop
9          |
10         = note: move occurs because `s` has type `std::string::String`, which does not implement the `Copy` trait
nikomatsakis (Aug 24 2018 at 10:15, on Zulip):

have to look at the MIR I guess

nikomatsakis (Aug 24 2018 at 10:15, on Zulip):

oh, this looks wrong (in your code):

        let l = context.loc;
        let mut stack = Vec::new();
        if context.loc.statement_index == 0 {
            stack.push(l);
        } else {
            stack.push(Location { block: l.block, statement_index: l.statement_index - 1 });
        }
nikomatsakis (Aug 24 2018 at 10:16, on Zulip):

I think what you really want is to add a fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = Location> function to the Mir type

nikomatsakis (Aug 24 2018 at 10:16, on Zulip):

and then do

nikomatsakis (Aug 24 2018 at 10:16, on Zulip):

stack.extend(mir.predecessor_locations(l));

nikomatsakis (Aug 24 2018 at 10:16, on Zulip):

you can then replace the existing code you have down below

            if l.statement_index == 0 {
                for predecessor_block in mir.predecessors_for(l.block).iter() {
                    let predecessor_end_location = mir.terminator_loc(*predecessor_block);
                    stack.push(predecessor_end_location);
                }
            } else {
                stack.push(Location { block: l.block, statement_index: l.statement_index - 1 });
            }

with a call to that same function.

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

writing these sorts of functions, where you want to return one of two iterators, is a bi tricky -- I usually find it works best to use Option etc

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

ok, left a review to that effect @Santiago Pastorino — I'll stop here :)

Santiago Pastorino (Aug 24 2018 at 12:18, on Zulip):

cool, checked your thoughts really quick

Santiago Pastorino (Aug 24 2018 at 12:19, on Zulip):

using the cellphone

Santiago Pastorino (Aug 24 2018 at 12:19, on Zulip):

will properly read this later in the computer

Santiago Pastorino (Aug 24 2018 at 12:19, on Zulip):

but thanks for the feedback

nikomatsakis (Aug 24 2018 at 12:29, on Zulip):

@Santiago Pastorino great!

nikomatsakis (Aug 26 2018 at 15:18, on Zulip):

@Santiago Pastorino did you ever get a chance to look into this? (no worries if not, just checking in)

Santiago Pastorino (Aug 26 2018 at 17:20, on Zulip):

@nikomatsakis not yet, tomorrow will do

Santiago Pastorino (Aug 26 2018 at 17:21, on Zulip):

first weekend back was complicated :blush:

Santiago Pastorino (Aug 27 2018 at 20:36, on Zulip):

@Santiago Pastorino so looking at the code I do see that the original ignored StorageDead for some reason

what do you mean here?

Santiago Pastorino (Aug 27 2018 at 20:37, on Zulip):

I've been pondering if we are "hard-coding" this too much

yeah, it may be too hard coded

Santiago Pastorino (Aug 27 2018 at 20:39, on Zulip):

in particular, I was wondering if we should write a sort of "reverse depth-first search" that works for any BitDenotation

yeah, I'm all for refactoring and making things that make more sense rather than adding snippets of things that make the thing work :P

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

I'm torn tbh

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

but it does seem like we ought to be able to factor this "reverse DFS" out

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

regarding StorageDead

nikomatsakis (Aug 27 2018 at 20:40, on Zulip):

I was referring to the definition of MovingOutStatements

nikomatsakis (Aug 27 2018 at 20:41, on Zulip):

and specifically this section

Santiago Pastorino (Aug 27 2018 at 20:42, on Zulip):

ok, a (slightly tweaked) version of that diff fixes on problem, but issue-29723.rs still fails

you mean that the diff you've pasted is not proper? do you have the new diff?

Santiago Pastorino (Aug 27 2018 at 20:45, on Zulip):

and specifically this section

yeah, but what do you mean, should we consider StorageDead or not?

nikomatsakis (Aug 27 2018 at 20:54, on Zulip):

Yes, we should — that is, if we want to match the behavior of the existing dataflow analysis

nikomatsakis (Aug 27 2018 at 20:54, on Zulip):

(which we do)

nikomatsakis (Aug 27 2018 at 20:58, on Zulip):

you mean that the diff you've pasted is not proper? do you have the new diff?

I think I pushed it to your branch

nikomatsakis (Aug 27 2018 at 20:59, on Zulip):

yeah, I'm all for refactoring and making things that make more sense rather than adding snippets of things that make the thing work :P

what I had in mind here is that we could write a generic "reverse depth-first search" that is customized by the BitDenotation

nikomatsakis (Aug 27 2018 at 20:59, on Zulip):

e.g., fn find_live_bits<BD>(bd: &BD, mir: &Mir, point: Location) -> Vec<BD::Idx> where BD: BitDenotation

nikomatsakis (Aug 27 2018 at 21:15, on Zulip):

(does that make sense @Santiago Pastorino ?)

nikomatsakis (Aug 27 2018 at 21:16, on Zulip):

this also might be the sort of code that is almost easier to write in the abstract than in the specific, but anyway

Santiago Pastorino (Aug 27 2018 at 21:30, on Zulip):

will check later, with the phone now

Santiago Pastorino (Aug 28 2018 at 02:54, on Zulip):

makes sense

Santiago Pastorino (Aug 28 2018 at 02:55, on Zulip):

anyway, will check and ping you tomorrow, it's late here :)

Santiago Pastorino (Aug 28 2018 at 16:41, on Zulip):

@nikomatsakis back, so ... the branch with my code and your code only lacks handling the location properly and refactoring this find_live_bits thing, right?

Santiago Pastorino (Aug 28 2018 at 16:41, on Zulip):

also I was wondering what is exactly the error in the predecessor thing in the current code

Santiago Pastorino (Aug 28 2018 at 16:42, on Zulip):

because I don't know what to do exactly

Santiago Pastorino (Aug 28 2018 at 16:42, on Zulip):

I guess you want me to do something like ...

Santiago Pastorino (Aug 28 2018 at 16:42, on Zulip):
    pub fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = Location> {
        if loc.statement_index == 0 {
            self.predecessors_for(loc.block).iter()
        } else {
            vec![Location { block: loc.block, statement_index: loc.statement_index - 1 }].iter()
        }
    }
Santiago Pastorino (Aug 28 2018 at 16:42, on Zulip):

take that as pseudo code

Santiago Pastorino (Aug 28 2018 at 16:43, on Zulip):

pretty sure that I can do better on the else branch

Santiago Pastorino (Aug 28 2018 at 16:44, on Zulip):

hmm now I have doubts ... I guess I should just call predecessors_for without that if

Santiago Pastorino (Aug 28 2018 at 16:44, on Zulip):

going to see what predecessors_for really do

nikomatsakis (Aug 28 2018 at 16:57, on Zulip):

predecessors_for does not exist

nikomatsakis (Aug 28 2018 at 16:57, on Zulip):

or, sorry

nikomatsakis (Aug 28 2018 at 16:57, on Zulip):

there is a method that yields up a sequence of basic blocks

nikomatsakis (Aug 28 2018 at 16:57, on Zulip):

that code looks roughly right, at least as pseudo-code

nikomatsakis (Aug 28 2018 at 16:58, on Zulip):

the point that we distinguish between the predecessors for the location where statement_index is 0

nikomatsakis (Aug 28 2018 at 16:58, on Zulip):

(in which case we have to go across basic blocks)

nikomatsakis (Aug 28 2018 at 16:58, on Zulip):

from the other cases

nikomatsakis (Aug 28 2018 at 16:58, on Zulip):

(in which case we are going within a basic block)

Santiago Pastorino (Aug 28 2018 at 17:00, on Zulip):

the point that we distinguish between the predecessors for the location where statement_index is 0

didn't understand since this phrase on ...

Santiago Pastorino (Aug 28 2018 at 17:00, on Zulip):

you mean that is right to do that?

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

yes

Santiago Pastorino (Aug 28 2018 at 17:00, on Zulip):

also ... predecesor_for returns an iterator of basic blocks

Santiago Pastorino (Aug 28 2018 at 17:01, on Zulip):

I guess we need to do a flat map or something like that and turn that into locations

Santiago Pastorino (Aug 28 2018 at 17:01, on Zulip):

but anyway, don't see what was wrong in the old code

Santiago Pastorino (Aug 28 2018 at 17:01, on Zulip):

or is it that you just wanted to move code to mir?

Santiago Pastorino (Aug 28 2018 at 17:01, on Zulip):

I'm asking because otherwise I'm going to add the same code but on mir :)

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

the old code didn't handle the case where statement index is 0

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

correctly

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

here is the code

nikomatsakis (Aug 28 2018 at 17:04, on Zulip):
if context.loc.statement_index == 0 {
            stack.push(l);
        } else {
            stack.push(Location { block: l.block, statement_index: l.statement_index - 1 });
        }
nikomatsakis (Aug 28 2018 at 17:04, on Zulip):

that is what is in the PR now

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

the intention is to push the predecessors onto the stack

Santiago Pastorino (Aug 28 2018 at 17:05, on Zulip):

yeah I see now

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

here, l == context.loc

Santiago Pastorino (Aug 28 2018 at 17:05, on Zulip):

it was only the terminator being pushed

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

so long as context.loc fell in the middle of a basic block, it was correct

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

but if it feel at the start of a block

Santiago Pastorino (Aug 28 2018 at 17:05, on Zulip):

:+1:

Santiago Pastorino (Aug 28 2018 at 17:05, on Zulip):

ok, going to see how to get locations from a basic_block

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

@Santiago Pastorino there is a method terminator_loc

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

actually, there are a few (sigh)

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

but I literally just added one

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

that gives you the Location of the terminator of a basic block

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

this is the method

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

ok. let me fetch

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

what I did not notice is that there were two other such methods -- probably those should be removed but oh well

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

the one I just added is so new it's not in the published rustdocs yet :)

Santiago Pastorino (Aug 28 2018 at 17:08, on Zulip):

:)

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

it's a method on the mir, so you would do mir.terminator_loc(bb)

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

(this is because it has to access the data from the basic block that is stored in the MIR to know the index of the terminator)

Santiago Pastorino (Aug 28 2018 at 17:09, on Zulip):

you're talking about statement_index == 0 case, right?

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

yes

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

well, sort of :)

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

that is, when we are finding the pred's of a location whose statement_index is 0

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

in that case, we want to push the terminator location from each predecessor block

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

the predecessors_for(BasicBlock) method gives the pred basic blocks

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

and the terminator_loc converts that to a Location

Santiago Pastorino (Aug 28 2018 at 17:13, on Zulip):

ok, it's pretty clear to me now what I don't follow :)

Santiago Pastorino (Aug 28 2018 at 17:14, on Zulip):

why do we want from a regular basic block to ger the terminator loc and not all the locations?

Santiago Pastorino (Aug 28 2018 at 17:14, on Zulip):

I see that in the case statement_index == 0

Santiago Pastorino (Aug 28 2018 at 17:14, on Zulip):

don't follow why we want to do that in the rest of the cases

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

don't follow why we want to do that in the rest of the cases

I don't understand this

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

we're not doing that in the other cases?

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

to expand on your pseudo-code:

Santiago Pastorino (Aug 28 2018 at 17:16, on Zulip):

this is what I'm doing ...

Santiago Pastorino (Aug 28 2018 at 17:16, on Zulip):
    #[inline]
    pub fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = &Location> {
        if loc.statement_index == 0 {
            vec![self.terminator_loc(loc.block)].iter()
        } else {
            self.predecessors_for(loc.block).flat_map(|bb| bb.locations).iter()
        }
    }
nikomatsakis (Aug 28 2018 at 17:16, on Zulip):
 pub fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = Location> {
        if loc.statement_index == 0 {
            self.predecessors_for(loc.block).into().map(move |basic_block| self.terminator_loc(basic_block))
        } else {
            vec![Location { block: loc.block, statement_index: loc.statement_index - 1 }].iter()
        }
}
nikomatsakis (Aug 28 2018 at 17:16, on Zulip):

yes so that is exactly backwards the way you have it :)

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

think of the graph

Santiago Pastorino (Aug 28 2018 at 17:17, on Zulip):

yes

Santiago Pastorino (Aug 28 2018 at 17:17, on Zulip):

makes sense

Santiago Pastorino (Aug 28 2018 at 17:17, on Zulip):

hehe

Santiago Pastorino (Aug 28 2018 at 17:17, on Zulip):

I understand why

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

you have e.g.

BB0 {
    statement 0
    statement 1
    statement 2
    terminator: goto BB1, ...
}

BB1 {
  statement 0
  statement 1
  ...
}
nikomatsakis (Aug 28 2018 at 17:18, on Zulip):

here, the predecessor of BB1/1 is BB1/0

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

the predecessor of BB1/0 is BB0/3 (terminator of BB0)

Santiago Pastorino (Aug 28 2018 at 17:18, on Zulip):

there's only one parent in statements that are not 0

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

predecessor of BB0/3 is BB0/2 etc

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

there's only one parent in statements that are not 0

yes

Santiago Pastorino (Aug 28 2018 at 17:20, on Zulip):
 pub fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = Location> {
        if loc.statement_index == 0 {
            self.predecessors_for(loc.block).into().map(move |basic_block| self.terminator_loc(basic_block))
        } else {
            vec![Location { block: loc.block, statement_index: loc.statement_index - 1 }].iter()
        }
}

what do you want to do there with the into and map?

Santiago Pastorino (Aug 28 2018 at 17:20, on Zulip):

that's kind of a flat map or what's the intention?

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

er sorry I meant to do iter() -- or maybe into_iter

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

it is not a flat map

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

it is a map

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

for each predecessor block, there is exactly 1 terminator location

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

converting this into an impl Iterator might be annoying because of lifetime issues

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

I hate the way we use a ref-cell here

Santiago Pastorino (Aug 28 2018 at 17:23, on Zulip):

ya

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

though the obvious alternatives aren't much better

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

so one thing that is easy to do is to convert this to a closure callback setup

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

sorry, to be clear, what I am concerned about is the return type of the existing predecessors_for method:

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

https://doc.rust-lang.org/nightly/nightly-rustc/rustc/mir/struct.Mir.html#method.predecessors_for

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

which is actually a Ref<'_, Vec<BasicBlock>> (the rustdoc is a bit confusing on this point)

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

this means that this Ref value is owned by the caller -- so you can't invoke iter() and return that to the caller

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

since iter will borrow the Ref, and then you are trying to return that borrow longer than the Ref exists

Santiago Pastorino (Aug 28 2018 at 17:26, on Zulip):

hmmm

Santiago Pastorino (Aug 28 2018 at 17:26, on Zulip):

was trying stuff and seeing errors already :)

Santiago Pastorino (Aug 28 2018 at 17:26, on Zulip):

error[E0308]: if and else have incompatible types

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

well, that's a separate problem

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

and an easier one to overcome

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

the problem there is that you are returning an impl Iterator -- that must be a single iterator type

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

so the if/else can't have two options

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

the usual way to fix that is to "concatenate" the two iterators

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

I should write something about this

Santiago Pastorino (Aug 28 2018 at 17:28, on Zulip):

I guess I haven't seen that trick that often

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

/me is finding lots of good Intermediate Rust blog topics

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

so I can explain it,

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

but the problem is that the first half

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

isn't going to work

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

in this particular case

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

unless you clone the vector :)

Santiago Pastorino (Aug 28 2018 at 17:28, on Zulip):

I understand that I have two different iterators and that's wrong

Santiago Pastorino (Aug 28 2018 at 17:28, on Zulip):

unless you clone the vector :)

yeah, hehe

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

if you want I can show you how to do it that way

Santiago Pastorino (Aug 28 2018 at 17:29, on Zulip):

I don't understand what you mean by concatenating iterators

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

we can validate that it fixes the problem anyway

Santiago Pastorino (Aug 28 2018 at 17:29, on Zulip):

yes

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

ok so

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

the idea would be something like this

nikomatsakis (Aug 28 2018 at 17:31, on Zulip):
 pub fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = Location> + '_ {
        let if_zero_locations = if loc.statement_index == 0 {
            let predecessor_blocks: Vec<BasicBlock> = self.predecessors_for(loc.block).clone();
            Some(predecessor_blocks.into_iter().map(move |basic_block| self.terminator_loc(basic_block)))
        } else {
            None
        };

        let if_not_zero_locations = if loc.statement_index == 0 {
            None
        } else {
            Some(Location { block: loc.block, statement_index: loc.statement_index - 1 })
        };

        if_zero_locations.into_iter().flatten().chain(if_not_zero_locations)
}
nikomatsakis (Aug 28 2018 at 17:32, on Zulip):

note the final return value:

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

it is always the same, no matter what

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

it's the combination of two iterators

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

but in practice one or the other will be empty

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

in the case of statement_index == 0, we return a Some(I) where I is some iterator of locations

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

in the case of statement_index > 0, we return a Some(L) for the pred loc (and None otherwise)

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

another simpler way to do it might be

Santiago Pastorino (Aug 28 2018 at 17:33, on Zulip):

I see

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

well anyway

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

that's the idea :)

Santiago Pastorino (Aug 28 2018 at 17:33, on Zulip):

:)

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

(you can also use the Either crate)

Santiago Pastorino (Aug 28 2018 at 17:33, on Zulip):

yep

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

which would look like

fn fo() -> impl Iterator {
    if something { Either::Left(...) } else { Either::Right(...) }
}
Santiago Pastorino (Aug 28 2018 at 17:33, on Zulip):

do you want me to try this or just switch directly to the callbacks idea

Santiago Pastorino (Aug 28 2018 at 17:34, on Zulip):

with this I mean, the code, the Either crate, which I've already used in rustc

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

I'd probably do the callabcks

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

cloning the vector seems silly

Santiago Pastorino (Aug 28 2018 at 17:34, on Zulip):

ok

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

but it's annoying

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

I'm pondering if there is anyway to fix that :)

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

besides refactoring how predecessors_for works

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

which — although I've wanted to do it for a while — seems out of scope =)

Santiago Pastorino (Aug 28 2018 at 17:35, on Zulip):

I'm ok to do whatever you prefer :)

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

part of the problem is I don't know what I would refactor to =)

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

I think you should do the callback variant

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

seems easiest for now

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

er well ok so

Santiago Pastorino (Aug 28 2018 at 17:37, on Zulip):

so ... you want the thing to be ...

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

I think I know how you could do it

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

took me a second to think of it :)

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

that is, with impl Iterator

Santiago Pastorino (Aug 28 2018 at 17:38, on Zulip):

ok

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

something like this would work for the first case (where statement_index == 0)

Santiago Pastorino (Aug 28 2018 at 17:38, on Zulip):

:)

nikomatsakis (Aug 28 2018 at 17:39, on Zulip):
let predecessor_blocks = self.predecessors_for(loc.block); // returns a `Ref<'_, Vec<BasicBlock>>`
let num_predecessor_blocks = predecessor_blocks.len();
(0 .. num_predecessor_blocks)
    .map(move |i| predecessor_blocks[i])
    .map(move |bb| self.terminator_loc(bb))
nikomatsakis (Aug 28 2018 at 17:40, on Zulip):

the key point here is that the closure will capture (and take ownership of) the predecessor_blocks value

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

so it lives as long as that closure does

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

I think you should use this version :)

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

does it make sense? :)

Santiago Pastorino (Aug 28 2018 at 17:43, on Zulip):

I see

Santiago Pastorino (Aug 28 2018 at 17:44, on Zulip):

and then I have this issue

Santiago Pastorino (Aug 28 2018 at 17:44, on Zulip):
    = note: expected type `std::iter::Map<std::iter::Map<std::ops::Range<usize>, [closure@librustc/mir/mod.rs:212:22: 212:52 predecessor_blocks:_]>, [closure@librustc/mir/mod.rs:213:22: 213:55 self:_]>`
               found type `std::slice::Iter<'_, mir::Location>`
Santiago Pastorino (Aug 28 2018 at 17:44, on Zulip):

should I combine iterators?

Santiago Pastorino (Aug 28 2018 at 17:44, on Zulip):

use Either or is there a better idea?

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

what is the code you have exactly?

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

I would probably still use the Option approach

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

I find it more elegant

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

but it's a matter of taste :P

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

the option variant is (I think) potentially more efficient, as well, but I doubt it matters much

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

(the difference is that Either will kind of "re-check" for each call to next() which branch you took)

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

but there are many other variables

Santiago Pastorino (Aug 28 2018 at 17:47, on Zulip):
        if loc.statement_index == 0 {
            let predecessor_blocks = self.predecessors_for(loc.block); // returns a `Ref<'_, Vec<BasicBlock>>`
            let num_predecessor_blocks = predecessor_blocks.len();
            (0 .. num_predecessor_blocks)
                .map(move |i| predecessor_blocks[i])
                .map(move |bb| self.terminator_loc(bb))
        } else {
            vec![Location { block: loc.block, statement_index: loc.statement_index - 1 }].iter()
        }
Santiago Pastorino (Aug 28 2018 at 17:47, on Zulip):

ok, let me go with the option thing because I'm also adding a vec there that doesn't make a lot of sense

Santiago Pastorino (Aug 28 2018 at 17:50, on Zulip):

wow, got some lifetimes issues

Santiago Pastorino (Aug 28 2018 at 17:51, on Zulip):
error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
   --> librustc/mir/mod.rs:209:43
    |
209 |             let predecessor_blocks = self.predecessors_for(loc.block); // returns a `Ref<'_, Vec<BasicBlock>>`
    |                                           ^^^^^^^^^^^^^^^^
    |
note: first, the lifetime cannot outlive the lifetime 'tcx as defined on the impl at 137:6...
   --> librustc/mir/mod.rs:137:6
    |
137 | impl<'tcx> Mir<'tcx> {
    |      ^^^^
note: ...so that the type `mir::Mir<'tcx>` is not borrowed for too long
   --> librustc/mir/mod.rs:209:38
    |
209 |             let predecessor_blocks = self.predecessors_for(loc.block); // returns a `Ref<'_, Vec<BasicBlock>>`
    |                                      ^^^^
    = note: but, the lifetime must be valid for the static lifetime...
note: ...so that return value is valid for the call
   --> librustc/mir/mod.rs:207:59
    |
207 |     pub fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = Location> {
    |                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: cannot infer an appropriate lifetime
   --> librustc/mir/mod.rs:213:22
    |
207 |     pub fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = Location> {
    |                                                           ------------------------------ this return type evaluates to the `'static` lifetime...
...
213 |                 .map(move |bb| self.terminator_loc(bb))
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ...but this borrow...
    |
note: ...can't outlive the anonymous lifetime #1 defined on the method body at 207:5
   --> librustc/mir/mod.rs:207:5
    |
207 | /     pub fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = Location> {
208 | |         let if_zero_locations = if loc.statement_index == 0 {
209 | |             let predecessor_blocks = self.predecessors_for(loc.block); // returns a `Ref<'_, Vec<BasicBlock>>`
210 | |             let num_predecessor_blocks = predecessor_blocks.len();
...   |
225 | |         if_zero_locations.into_iter().flatten().chain(if_not_zero_locations)
226 | |     }
    | |_____^
help: you can add a constraint to the return type to make it last less than `'static` and match the anonymous lifetime #1 defined on the method body at 207:5
    |
207 |     pub fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = Location> + '_ {
    |                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
   --> librustc/mir/mod.rs:213:22
    |
213 |                 .map(move |bb| self.terminator_loc(bb))
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: first, the lifetime cannot outlive the lifetime 'tcx as defined on the impl at 137:6...
   --> librustc/mir/mod.rs:137:6
    |
137 | impl<'tcx> Mir<'tcx> {
    |      ^^^^
    = note: ...so that the types are compatible:
            expected &mir::Mir<'_>
               found &mir::Mir<'tcx>
    = note: but, the lifetime must be valid for the static lifetime...
note: ...so that return value is valid for the call
   --> librustc/mir/mod.rs:207:59
    |
207 |     pub fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = Location> {
    |                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0495`.
error: Could not compile `rustc`.
nikomatsakis (Aug 28 2018 at 17:51, on Zulip):

so you're gonna have to break it into two ifs, as I wrote, or at least return pairs

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

oh yes so

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

you need to change -> impl Iterator to + '_, as indeed it suggests to you

Santiago Pastorino (Aug 28 2018 at 17:51, on Zulip):

yeah, I did the two ifs approach

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

that is

impl Iterator<Item = Location> + '_ {
nikomatsakis (Aug 28 2018 at 17:52, on Zulip):

the + '_ indicates that this iterator may capture data from self

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

otherwise, we assume it doesn't

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

that error...needs some love

Santiago Pastorino (Aug 28 2018 at 17:52, on Zulip):

hehe :)

Santiago Pastorino (Aug 28 2018 at 17:52, on Zulip):

yeah, didn't even read what it was saying :P

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

I think it'd be mildly better with NLL but anyway

Santiago Pastorino (Aug 28 2018 at 18:19, on Zulip):

this is compiling now

Santiago Pastorino (Aug 28 2018 at 18:19, on Zulip):

running tests

Santiago Pastorino (Aug 28 2018 at 18:19, on Zulip):

anyway I'd need to do the refactor still

Santiago Pastorino (Aug 28 2018 at 18:31, on Zulip):

two Fs

nikomatsakis (Aug 28 2018 at 18:32, on Zulip):

which ones? :)

Santiago Pastorino (Aug 28 2018 at 18:53, on Zulip):
failures:
    [ui] ui/issues/issue-29723.rs
    [ui] ui/macros/macros-nonfatal-errors.rs
nikomatsakis (Aug 28 2018 at 18:53, on Zulip):

I think that the last one is a spurious thing

nikomatsakis (Aug 28 2018 at 18:53, on Zulip):

I've seen it sometimes

nikomatsakis (Aug 28 2018 at 18:54, on Zulip):

the other one is I think the failure I was concerned about?

nikomatsakis (Aug 28 2018 at 18:54, on Zulip):

(what is your full diff?)

nikomatsakis (Aug 28 2018 at 18:54, on Zulip):

it could be I was wrong as to the cause

nikomatsakis (Aug 28 2018 at 18:54, on Zulip):

that would worry me since it would mean we have an untested path, since that code was definitely wrong as is :)

Santiago Pastorino (Aug 28 2018 at 19:05, on Zulip):

so ...

Santiago Pastorino (Aug 28 2018 at 19:05, on Zulip):

let me push again :)

Santiago Pastorino (Aug 28 2018 at 19:14, on Zulip):

https://github.com/spastorino/rust/commit/206088406234f7e27eab9985f6c718a82e868866

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

@Santiago Pastorino https://github.com/spastorino/rust/commit/206088406234f7e27eab9985f6c718a82e868866#r30318086

Santiago Pastorino (Aug 28 2018 at 19:20, on Zulip):

hehehe

Santiago Pastorino (Aug 28 2018 at 19:20, on Zulip):

I did that

Santiago Pastorino (Aug 28 2018 at 19:20, on Zulip):

probably rebasing issues :)

nikomatsakis (Aug 28 2018 at 19:21, on Zulip):

ok, I don't see it in https://github.com/rust-lang/rust/pull/53403/files

Santiago Pastorino (Aug 28 2018 at 19:23, on Zulip):

no I did add that in the last of the method but not there

Santiago Pastorino (Aug 28 2018 at 19:24, on Zulip):

was just distracted

Santiago Pastorino (Aug 28 2018 at 19:24, on Zulip):

anyway ... did it now

Santiago Pastorino (Aug 28 2018 at 19:24, on Zulip):

pushing again

nikomatsakis (Aug 28 2018 at 19:41, on Zulip):

@Santiago Pastorino running locally, the only UI failure I see is

failures:
[ui] ui/macros/macros-nonfatal-errors.rs

Santiago Pastorino (Aug 28 2018 at 19:42, on Zulip):

yes, here it's finishing running

Santiago Pastorino (Aug 28 2018 at 19:42, on Zulip):

for now one F

nikomatsakis (Aug 28 2018 at 19:43, on Zulip):

I believe that this is some kind of stage1 artifact

nikomatsakis (Aug 28 2018 at 19:43, on Zulip):

I haven't looked more closely though

Santiago Pastorino (Aug 28 2018 at 19:43, on Zulip):

you mean because of the keep-stage thing?

Santiago Pastorino (Aug 28 2018 at 19:44, on Zulip):

so yeah, same error

nikomatsakis (Aug 28 2018 at 19:48, on Zulip):

you mean because of the keep-stage thing?

not necessarily...sometimes stage1 tests just don't work for whatever reason

nikomatsakis (Aug 28 2018 at 19:49, on Zulip):

I'm also not using --keep-stage 1

pnkfelix (Aug 28 2018 at 19:49, on Zulip):

not necessarily...sometimes stage1 tests just don't work for whatever reason

(this is very annoying)

Santiago Pastorino (Aug 28 2018 at 19:51, on Zulip):

ok, let's see what the ci says then :)

Santiago Pastorino (Aug 28 2018 at 20:00, on Zulip):

so @nikomatsakis just in case ... about this ... I just need to do the generic reverse depth-first search specialized by BitDenotation and done, right?

nikomatsakis (Aug 28 2018 at 20:01, on Zulip):

well it depends

nikomatsakis (Aug 28 2018 at 20:02, on Zulip):

we could probably land 'as is' and leave that for a follow-up

nikomatsakis (Aug 28 2018 at 20:02, on Zulip):

but I guess it'd be nicer to do it as part of this

nikomatsakis (Aug 28 2018 at 20:02, on Zulip):

since it will need to use the BitDenotation impl that we removed :)

Santiago Pastorino (Aug 28 2018 at 20:17, on Zulip):

yeah

Santiago Pastorino (Aug 28 2018 at 20:17, on Zulip):

will do it before we make it land

Santiago Pastorino (Aug 28 2018 at 20:17, on Zulip):

just wanted to be sure that it was the missing bit

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

yep

nikomatsakis (Aug 28 2018 at 20:51, on Zulip):

@Santiago Pastorino running all the way to stage2, I got this error:

---- [ui (nll)] ui/hygiene/fields-move.rs stdout ----
diff of stderr:

1       error[E0382]: use of moved value: `foo.x`
2         --> $DIR/fields-move.rs:28:9
3          |
-       LL |    $foo.x
-          |    ------ value moved here
-       ...
7       LL |         $foo.x //~ ERROR use of moved value: `foo.x`
8          |         ^^^^^^ value used here after move
9       ...

28      error[E0382]: use of moved value: `foo.x`
29        --> $DIR/fields-move.rs:39:42
30         |
-       LL |    $foo.x
-          |    ------ value moved here
-       ...
34      LL |         $foo.x //~ ERROR use of moved value: `foo.x`
35         |         ------ value moved here
36      ...

-       LL |     assert_two_copies(copy_modern!(foo), foo.x); //~ ERROR use of moved value: `foo.x`
-          |                                          ----- value moved here
39      LL |     assert_two_copies(copy_legacy!(foo), foo.x); //~ ERROR use of moved value: `foo.x`
40         |                                          ^^^^^ value used here after move
41         |
nikomatsakis (Aug 28 2018 at 20:51, on Zulip):

I haven't looked at it at all

Santiago Pastorino (Aug 29 2018 at 00:03, on Zulip):

doh

Santiago Pastorino (Aug 29 2018 at 13:28, on Zulip):

@nikomatsakis seeing the very same thing now

Santiago Pastorino (Aug 29 2018 at 13:29, on Zulip):

the CI have also finished

Santiago Pastorino (Aug 29 2018 at 13:29, on Zulip):

I'd need to investigate a bit

Santiago Pastorino (Aug 29 2018 at 19:20, on Zulip):

@nikomatsakis didn't have time yet but do you have any suggestion on where to look?

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

I haven't really looked at it yet either— I think comparing the before/after like you did before seems good?

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

or, alternatively,

Santiago Pastorino (Aug 29 2018 at 19:21, on Zulip):

yeah, will try to see that

Santiago Pastorino (Aug 29 2018 at 19:21, on Zulip):

didn't want you to jump into the issue

Santiago Pastorino (Aug 29 2018 at 19:22, on Zulip):

but maybe from the top of your head you had some tip or something :)

nikomatsakis (Aug 29 2018 at 19:22, on Zulip):

hmm

nikomatsakis (Aug 29 2018 at 19:22, on Zulip):

the move is occurring in a call

nikomatsakis (Aug 29 2018 at 19:23, on Zulip):

or at least one of them

nikomatsakis (Aug 29 2018 at 19:23, on Zulip):

it's from the parameters of a call

nikomatsakis (Aug 29 2018 at 19:23, on Zulip):

calls are treated a bit funny sometimes, but really just their destination

Santiago Pastorino (Aug 29 2018 at 19:23, on Zulip):

isn't the test about a macro?

nikomatsakis (Aug 29 2018 at 19:24, on Zulip):

the macro is just confusing things

nikomatsakis (Aug 29 2018 at 19:24, on Zulip):

or at least I think so

Santiago Pastorino (Aug 29 2018 at 19:24, on Zulip):

given that it talks about hygiene in the directory

nikomatsakis (Aug 29 2018 at 19:24, on Zulip):

I guess the first thing I might do is to remove the macro from the test and check if you get the same behavior :)

nikomatsakis (Aug 29 2018 at 19:24, on Zulip):

well sure

Santiago Pastorino (Aug 29 2018 at 19:24, on Zulip):

I guess it should be related to some macro thing

Santiago Pastorino (Aug 29 2018 at 19:24, on Zulip):

yep

nikomatsakis (Aug 29 2018 at 19:24, on Zulip):

the purpose of the test has something to do with a macro

nikomatsakis (Aug 29 2018 at 19:24, on Zulip):

but that doens't mean it's affecting you at this moment

Santiago Pastorino (Aug 29 2018 at 19:24, on Zulip):

yep we need to see if this error is because of the macro or not

Santiago Pastorino (Aug 29 2018 at 19:25, on Zulip):

:+1:

Santiago Pastorino (Aug 29 2018 at 19:25, on Zulip):

I guessed it was because it's also failing at stage2, right?

Santiago Pastorino (Aug 29 2018 at 19:25, on Zulip):

not stage1

Santiago Pastorino (Aug 29 2018 at 19:25, on Zulip):

if I remember correctly something more happens with macros between those phases

Santiago Pastorino (Aug 29 2018 at 19:26, on Zulip):

anyway, I guess I should need to compile this thing for stage 2 and start testing and changing the example to get some kind of conclusion

nikomatsakis (Aug 29 2018 at 19:26, on Zulip):

I see

nikomatsakis (Aug 29 2018 at 19:26, on Zulip):

I ran by hand ;)

Santiago Pastorino (Aug 29 2018 at 19:27, on Zulip):

by hand on stage 1 also fails?

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

hmm, sorry

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

yes,

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

this has nothing to do with stage1/stage2

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

it's just that the error was uncovered when we run with compare-mode=nll

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

and stage1 wasn't getting that far

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

this is the test minus macros:

#[derive(Debug)]
struct NonCopy(String);

struct Foo {
    x: NonCopy,
}

fn assert_two_copies(a: NonCopy, b: NonCopy) {
    println!("Got two copies: {:?}, {:?}", a, b);
}

fn main() {
    let foo = Foo { x: NonCopy("foo".into()) };
    assert_two_copies(foo.x, foo.x); //~ ERROR use of moved value: `foo.x`
    assert_two_copies(foo.x, foo.x); //~ ERROR use of moved value: `foo.x`
}
nikomatsakis (Aug 29 2018 at 19:28, on Zulip):

it fails in the same basic way either way

nikomatsakis (Aug 29 2018 at 19:28, on Zulip):

though the output looks different

nikomatsakis (Aug 29 2018 at 19:28, on Zulip):

with nightly, we get:

error[E0382]: use of moved value: `foo.x`
  --> /home/nmatsakis/tmp/spastorino.rs:15:30
   |
14 |     assert_two_copies(foo.x, foo.x); //~ ERROR use of moved value: `foo.x`
   |                       -----  ----- value moved here
   |                       |
   |                       value moved here
15 |     assert_two_copies(foo.x, foo.x); //~ ERROR use of moved value: `foo.x`
   |                       -----  ^^^^^ value used here after move
   |                       |
   |                       value moved here
   |
   = note: move occurs because `foo.x` has type `NonCopy`, which does not implement the `Copy` trait
nikomatsakis (Aug 29 2018 at 19:28, on Zulip):

(actually, we get more errors than that, but this is the last one)

nikomatsakis (Aug 29 2018 at 19:28, on Zulip):

with your code, we get:

error[E0382]: use of moved value: `foo.x`
  --> /home/nmatsakis/tmp/spastorino.rs:15:30
   |
15 |     assert_two_copies(foo.x, foo.x); //~ ERROR use of moved value: `foo.x`
   |                       -----  ^^^^^ value used here after move
   |                       |
   |                       value moved here
   |
   = note: move occurs because `foo.x` has type `NonCopy`, which does not implement the `Copy` trait
nikomatsakis (Aug 29 2018 at 19:28, on Zulip):

interestingly, I would say that your code is more correct

nikomatsakis (Aug 29 2018 at 19:29, on Zulip):

I'm surprised by the nightly output

Santiago Pastorino (Aug 29 2018 at 19:29, on Zulip):

hmmm

nikomatsakis (Aug 29 2018 at 19:29, on Zulip):

in nightly, it highlights three places, but only the last one is relevant

nikomatsakis (Aug 29 2018 at 19:29, on Zulip):

in some sense

nikomatsakis (Aug 29 2018 at 19:29, on Zulip):

but I guess the reason for this is

nikomatsakis (Aug 29 2018 at 19:29, on Zulip):

that you stop walking backwards when you find a move (a GEN)

nikomatsakis (Aug 29 2018 at 19:29, on Zulip):

but in principle you could keep going back further

nikomatsakis (Aug 29 2018 at 19:29, on Zulip):

until you find a kill

Santiago Pastorino (Aug 29 2018 at 19:30, on Zulip):

yes

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

I guess nightly isn't wrong

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

all three of those moves consume the same value

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

that we are trying to later read

Santiago Pastorino (Aug 29 2018 at 19:30, on Zulip):

yeah so I can just update the stderr file and done?

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

debatable

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

either that or update the code :)

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

I can't decide which is more correct :)

Santiago Pastorino (Aug 29 2018 at 19:31, on Zulip):

the code?

Santiago Pastorino (Aug 29 2018 at 19:31, on Zulip):

you mean, the test?

nikomatsakis (Aug 29 2018 at 19:31, on Zulip):

no

Santiago Pastorino (Aug 29 2018 at 19:31, on Zulip):

ahh ok

nikomatsakis (Aug 29 2018 at 19:31, on Zulip):

you could update your reverse DFS to keep going after it finds a move

nikomatsakis (Aug 29 2018 at 19:31, on Zulip):

then I think the result will match

Santiago Pastorino (Aug 29 2018 at 19:31, on Zulip):

thought you were saying that it was better :)

nikomatsakis (Aug 29 2018 at 19:32, on Zulip):

I was but now I'm debating :)

nikomatsakis (Aug 29 2018 at 19:32, on Zulip):

I could go either way, I suppose

nikomatsakis (Aug 29 2018 at 19:32, on Zulip):

but it seems like the new output is "less busy"

nikomatsakis (Aug 29 2018 at 19:32, on Zulip):

so it's probably better

nikomatsakis (Aug 29 2018 at 19:32, on Zulip):

the scenario is sort of artificial

nikomatsakis (Aug 29 2018 at 19:33, on Zulip):

ok well I verified that if you do

athena. git diff
diff --git a/src/libcompiler_builtins b/src/libcompiler_builtins
--- a/src/libcompiler_builtins
+++ b/src/libcompiler_builtins
@@ -1 +1 @@
-Subproject commit d549d85b1735dc5066b2973f8549557a813bb9c8
+Subproject commit d549d85b1735dc5066b2973f8549557a813bb9c8-dirty
diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs
index 818e76957f..28daca5268 100644
--- a/src/librustc_mir/borrow_check/error_reporting.rs
+++ b/src/librustc_mir/borrow_check/error_reporting.rs
@@ -554,7 +554,6 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     if self.move_data.moves[*moi].path == mpi {
                         debug!("report_use_of_moved_or_uninitialized: found");
                         result.push(*moi);
-                        continue 'dfs;
                     }
                 }
             }
nikomatsakis (Aug 29 2018 at 19:33, on Zulip):

then you get the original output :)

nikomatsakis (Aug 29 2018 at 19:33, on Zulip):

but I think I agree that your new output is better

nikomatsakis (Aug 29 2018 at 19:33, on Zulip):

in particular

nikomatsakis (Aug 29 2018 at 19:33, on Zulip):

if there is a scenario where you have two moves A and B

nikomatsakis (Aug 29 2018 at 19:33, on Zulip):

both of which can reach this point

nikomatsakis (Aug 29 2018 at 19:33, on Zulip):

but A also reaches B

nikomatsakis (Aug 29 2018 at 19:33, on Zulip):

(which is what happens here)

nikomatsakis (Aug 29 2018 at 19:33, on Zulip):

then there are really two errors: one is that the move B is illegal

nikomatsakis (Aug 29 2018 at 19:34, on Zulip):

and the current point is also illegal

nikomatsakis (Aug 29 2018 at 19:34, on Zulip):

we are currently showing both A and B for the current move, but that seems kind of redundant

nikomatsakis (Aug 29 2018 at 19:34, on Zulip):

(not sure if that makes sense the way I'm describing it... in this example, my point is that only the most recent self.x is relevant to this error -- the other self.x accesses cause other errors)

nikomatsakis (Aug 29 2018 at 19:35, on Zulip):

so yeah I guess I think you should update the stderr but maybe we should leave a comment or something

nikomatsakis (Aug 29 2018 at 19:36, on Zulip):

saying something like:


// Strictly speaking, we could continue our DFS here. There may be
// other moves that can reach the point of error. But it is kind of confusing
// to highlight them.
//
// Example:
//
// ```
// let a = vec![];
// let b = a;
// let c = a;
// drop(a); // <-- current point of error
// ```
//
// Because we stop the DFS here, we only highlight `let c = a`,
// and not `let b = a`. We will of course also report an error at `let c = a`
// which highlights `let b = a` as the move.
Jake Goulding (Aug 29 2018 at 19:49, on Zulip):

I think reporting just the first "bad move" makes sense from a user POV. I don't think I'd want to see 30x "it also isn't available here". I'm sure some user will disagree, of course :wink:

Santiago Pastorino (Aug 30 2018 at 14:30, on Zulip):

hey

Santiago Pastorino (Aug 30 2018 at 14:30, on Zulip):

cool :+1:

Santiago Pastorino (Aug 30 2018 at 14:30, on Zulip):

gonna make the changes

Santiago Pastorino (Aug 30 2018 at 14:32, on Zulip):

@nikomatsakis do you know how to reproduce the thing locally?

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

reproduce..what?

Santiago Pastorino (Aug 30 2018 at 14:32, on Zulip):

I mean, I 'd run with bless but current test runs in my local machine do not fail

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

I mean just running ./x.py test src/test/ui will work

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

do you want me to run with --bless?

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

(I have it all setup)

Santiago Pastorino (Aug 30 2018 at 14:33, on Zulip):

I mean, I don't know why that test on my machine does not fail

Santiago Pastorino (Aug 30 2018 at 14:33, on Zulip):

unsure what are you doing to make ./x.py test src/test/ui fail

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

hmm the actual command I ran was ./x.py test --stage 2 -i src/test/ui/

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

it failed in the NLL compare-mode part

Santiago Pastorino (Aug 30 2018 at 14:34, on Zulip):

ok, right it was on stage 2

Santiago Pastorino (Aug 30 2018 at 14:34, on Zulip):

well I think you said that wasn't related to stage 2

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

it's not

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

but if you run with stage1, another test fails (the macro thing)

Santiago Pastorino (Aug 30 2018 at 14:34, on Zulip):

I guess it's just that doing that is when the compare-mode part is ran?

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

this prevents us from running in NLL compare-mode

Santiago Pastorino (Aug 30 2018 at 14:34, on Zulip):

ahhhh

Santiago Pastorino (Aug 30 2018 at 14:34, on Zulip):

got it got it

Santiago Pastorino (Aug 30 2018 at 14:35, on Zulip):

that was my missing bit

Santiago Pastorino (Aug 30 2018 at 14:35, on Zulip):

ok

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

so I can do the --bless if you want to add the comment :)

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

I already started it anyway

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

actually though I did have a local diff so it'll have to rebuild

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

but it goes relatively fast locally

Santiago Pastorino (Aug 30 2018 at 14:35, on Zulip):

:+1:

Santiago Pastorino (Aug 30 2018 at 21:39, on Zulip):

@nikomatsakis I've realized that I haven't added the comment you suggested

Santiago Pastorino (Aug 30 2018 at 21:39, on Zulip):

did that

Santiago Pastorino (Aug 30 2018 at 21:40, on Zulip):

should now be ready to +1 I guess

Santiago Pastorino (Aug 31 2018 at 20:46, on Zulip):

@nikomatsakis just ended with this thing compiling

Santiago Pastorino (Aug 31 2018 at 20:46, on Zulip):

I mean, have pasted the old code, the one related to MovingOutStatements back and it's compiling

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

uh :)

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

not sure what you mean :)

Santiago Pastorino (Aug 31 2018 at 20:47, on Zulip):

doesn't matter :)

Santiago Pastorino (Aug 31 2018 at 20:47, on Zulip):

so

Santiago Pastorino (Aug 31 2018 at 20:48, on Zulip):

do you remember that we removed the MovingOutStatements ?

Santiago Pastorino (Aug 31 2018 at 20:48, on Zulip):

was getting the code back

Santiago Pastorino (Aug 31 2018 at 20:48, on Zulip):

was compiling that meanwhile I was doing other stuff

Santiago Pastorino (Aug 31 2018 at 20:48, on Zulip):

now back to the real task

Santiago Pastorino (Aug 31 2018 at 20:48, on Zulip):

I guess we want to provide for BitDenotation a dfs method that receives a fn

Santiago Pastorino (Aug 31 2018 at 20:49, on Zulip):

and then we call that for the MovingOutStatements specific thing we are doing in this issue

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

yes, I imagined writing a function sort of like this (not sure exactly where it would go)

nikomatsakis (Aug 31 2018 at 20:50, on Zulip):
fn find_values_in_scope<BD: BitDenotation>(
    bit: &BD,
    mir: &Mir<'tcx>,
    location: Location,
) -> Vec<BD::Value> { ... }
nikomatsakis (Aug 31 2018 at 20:50, on Zulip):

it would work fairly similarly to what you have

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

but not be hard-coded to any one thing

Santiago Pastorino (Aug 31 2018 at 20:57, on Zulip):

yes

Santiago Pastorino (Aug 31 2018 at 20:57, on Zulip):

but ... wouldn't that be inside of the trait?

Santiago Pastorino (Aug 31 2018 at 20:57, on Zulip):

or I guessed it wrongly?

Santiago Pastorino (Aug 31 2018 at 20:57, on Zulip):

ahh I see what you mean

Santiago Pastorino (Aug 31 2018 at 20:59, on Zulip):

the fn is not part of the trait, it's just a fn that uses this kind of types

Santiago Pastorino (Aug 31 2018 at 20:59, on Zulip):

I probably didn't pay that much attention of what the trait is about

Santiago Pastorino (Aug 31 2018 at 20:59, on Zulip):

but at first I thought you wanted to add the fn as a fn of the trait with a default impl

nikomatsakis (Aug 31 2018 at 21:02, on Zulip):

we could do that

nikomatsakis (Aug 31 2018 at 21:02, on Zulip):

I'd probably make it a standalone thing

nikomatsakis (Aug 31 2018 at 21:02, on Zulip):

probably living in rustc_mir::dataflow though

nikomatsakis (Aug 31 2018 at 21:03, on Zulip):

sort of analogous to do_dataflow

Santiago Pastorino (Aug 31 2018 at 21:04, on Zulip):

:+1:

Last update: Nov 21 2019 at 13:05UTC