Stream: t-compiler/wg-nll

Topic: #52669 borrow of possibly uninitialized variable


Santiago Pastorino (Sep 12 2018 at 20:22, on Zulip):

Opening this topic to start discussing this https://github.com/rust-lang/rust/issues/52669 issue :)

Santiago Pastorino (Sep 12 2018 at 20:41, on Zulip):

@nikomatsakis to continue talking about this here

Santiago Pastorino (Sep 12 2018 at 20:41, on Zulip):

if I change the code to foo(a.b), I get ...

Santiago Pastorino (Sep 12 2018 at 20:42, on Zulip):
error[E0382]: borrow of moved value: `a.b`
  --> test.rs:17:5
   |
16 |     foo(a.b);
   |         --- value moved here
17 |     a.b.clone()
   |     ^^^ value borrowed here after move
   |
   = note: move occurs because `a.b` has type `B`, which does not implement the `Copy` trait

error: aborting due to previous error
Santiago Pastorino (Sep 12 2018 at 20:42, on Zulip):

so, it seems that we are failing to check for moves from parent paths as you guessed :)

Santiago Pastorino (Sep 12 2018 at 20:44, on Zulip):

and if I change the code to use a, by doing a.clone(), I get ...

Santiago Pastorino (Sep 12 2018 at 20:45, on Zulip):
error[E0382]: borrow of moved value: `a`
  --> test.rs:18:5
   |
17 |     foo(a);
   |         - value moved here
18 |     a.clone();
   |     ^ value borrowed here after move
   |
   = note: move occurs because `a` has type `A`, which does not implement the `Copy` trait

error: aborting due to previous error
Santiago Pastorino (Sep 12 2018 at 20:45, on Zulip):

so definitely seems to be that we are failing to check for moves from parent paths

nikomatsakis (Sep 12 2018 at 20:54, on Zulip):

ok

nikomatsakis (Sep 12 2018 at 20:54, on Zulip):

interesting

nikomatsakis (Sep 12 2018 at 20:55, on Zulip):

I guess we can just look for moves of any parent path by "iterating more"

nikomatsakis (Sep 12 2018 at 20:55, on Zulip):

in the backwards DFS

nikomatsakis (Sep 12 2018 at 20:55, on Zulip):

interestingly, this is not something that is .. obviously correct if we were to try and write that fn more generically

nikomatsakis (Sep 12 2018 at 20:55, on Zulip):

so I'm glad you didn't go ahead and do that :)

Santiago Pastorino (Sep 12 2018 at 21:18, on Zulip):

:)

Santiago Pastorino (Sep 12 2018 at 21:18, on Zulip):

need to go, will try to code this later

Santiago Pastorino (Sep 12 2018 at 21:18, on Zulip):

or tomorrow

Santiago Pastorino (Sep 14 2018 at 20:23, on Zulip):

@nikomatsakis you around?

nikomatsakis (Sep 14 2018 at 20:30, on Zulip):

@Santiago Pastorino am now

Santiago Pastorino (Sep 14 2018 at 20:37, on Zulip):

have some minutes to dig into this

Santiago Pastorino (Sep 14 2018 at 20:39, on Zulip):

I was guessing that I need to do something like ... move_data.move_paths[mpi].place

Santiago Pastorino (Sep 14 2018 at 20:39, on Zulip):

to access place and from there I guess I have access to the whole chain of calls, right?

Santiago Pastorino (Sep 14 2018 at 20:39, on Zulip):

so I can iterate over a.b.c.d calls

Santiago Pastorino (Sep 14 2018 at 20:45, on Zulip):

I see there's has_any_child_of to go to childs

Santiago Pastorino (Sep 14 2018 at 20:45, on Zulip):

but I want to go up

nikomatsakis (Sep 14 2018 at 20:49, on Zulip):

that sounds right

nikomatsakis (Sep 14 2018 at 20:49, on Zulip):

there is a parent field I think

nikomatsakis (Sep 14 2018 at 20:49, on Zulip):

so you can go up

nikomatsakis (Sep 14 2018 at 20:49, on Zulip):

probably want to do that once in the beginning

nikomatsakis (Sep 14 2018 at 20:49, on Zulip):

and collect to a vector

nikomatsakis (Sep 14 2018 at 20:49, on Zulip):

not sure if there is a helper; I think I looked for one once and didn't find one

Santiago Pastorino (Sep 14 2018 at 20:50, on Zulip):

unsure I follow what you want to do

Santiago Pastorino (Sep 14 2018 at 20:50, on Zulip):

I mean, when and why collecting into a vector at the beginning?

Santiago Pastorino (Sep 14 2018 at 20:51, on Zulip):

ahh

Santiago Pastorino (Sep 14 2018 at 20:51, on Zulip):

you meant ...

Santiago Pastorino (Sep 14 2018 at 20:51, on Zulip):

collect &self.move_data.loc_map[l] + parents?

Santiago Pastorino (Sep 14 2018 at 20:52, on Zulip):

and then iterate

Santiago Pastorino (Sep 14 2018 at 20:52, on Zulip):

is that what you meant?

nikomatsakis (Sep 14 2018 at 20:58, on Zulip):

yep

nikomatsakis (Sep 14 2018 at 20:59, on Zulip):

collect that stuff before you enter the reverse DFS basically

nikomatsakis (Sep 14 2018 at 20:59, on Zulip):

so you have it available

Santiago Pastorino (Sep 14 2018 at 21:50, on Zulip):

:+1:

Santiago Pastorino (Sep 14 2018 at 21:56, on Zulip):

@nikomatsakis ...

Santiago Pastorino (Sep 14 2018 at 21:56, on Zulip):
error[E0599]: no method named `map` found for type `std::vec::Vec<dataflow::move_paths::indexes::MoveOutIndex>` in the current scope
   --> librustc_mir/borrow_check/error_reporting.rs:555:54
    |
555 |                 let mois = self.move_data.loc_map[l].map(|moi| {
    |                                                      ^^^
    |
    = note: the method `map` exists but the following trait bounds were not satisfied:
            `&mut std::vec::Vec<dataflow::move_paths::indexes::MoveOutIndex> : std::iter::Iterator`
            `&mut [dataflow::move_paths::indexes::MoveOutIndex] : std::iter::Iterator`
Santiago Pastorino (Sep 14 2018 at 21:57, on Zulip):

not sure I understand correctly the error

Jake Goulding (Sep 14 2018 at 21:57, on Zulip):

.iter().map() ?

Santiago Pastorino (Sep 14 2018 at 21:57, on Zulip):

yes

Santiago Pastorino (Sep 14 2018 at 21:57, on Zulip):

I just did that

Santiago Pastorino (Sep 14 2018 at 21:58, on Zulip):

but still, don't understand that much the error

Jake Goulding (Sep 14 2018 at 21:58, on Zulip):

Oh, is that error message from some other run?

Jake Goulding (Sep 14 2018 at 21:58, on Zulip):

Cause it's true; Vec does not have a map method.

Santiago Pastorino (Sep 14 2018 at 21:58, on Zulip):

no no, right after commenting I added an iter() call meanwhile you've said the same

Santiago Pastorino (Sep 14 2018 at 21:59, on Zulip):

Cause it's true; Vec does not have a map method.

but it says ...

Santiago Pastorino (Sep 14 2018 at 21:59, on Zulip):

= note: the method map exists but the following trait bounds were not satisfied:

Santiago Pastorino (Sep 14 2018 at 21:59, on Zulip):

it's kind of misleading

Santiago Pastorino (Sep 14 2018 at 21:59, on Zulip):

it's saying that the method exists

Jake Goulding (Sep 14 2018 at 21:59, on Zulip):

It does (as Iterator::map)

Santiago Pastorino (Sep 14 2018 at 22:00, on Zulip):

I see what you mean

Santiago Pastorino (Sep 14 2018 at 22:00, on Zulip):

when I read the method exist I thought it was referring that it exists on the type of the thing not somewhere

Jake Goulding (Sep 14 2018 at 22:01, on Zulip):

I don't know why it picks only &mut here. Maybe something to do with the index?

Santiago Pastorino (Sep 14 2018 at 22:01, on Zulip):

and that's also another thing that have confused me

Santiago Pastorino (Sep 14 2018 at 22:01, on Zulip):

but first the thing I read was ...

Santiago Pastorino (Sep 14 2018 at 22:02, on Zulip):

no method named map found for type Vec but note that the method map exists

Santiago Pastorino (Sep 14 2018 at 22:03, on Zulip):

I guessed in Vec, why would this be talking about some other type

Santiago Pastorino (Sep 14 2018 at 22:03, on Zulip):

but in some way you're not satisfying the traits bounds mentioned

Santiago Pastorino (Sep 14 2018 at 22:03, on Zulip):

that's what I've read from the error

Santiago Pastorino (Sep 14 2018 at 22:03, on Zulip):

hehe

Jake Goulding (Sep 14 2018 at 22:04, on Zulip):

I could see it being reworded...

note: the method map exists on the traits (Foo, Bar, Baz, ...) but the following trait bounds were not satisfied:

Jake Goulding (Sep 14 2018 at 22:05, on Zulip):

rewording the next part is tougher

Jake Goulding (Sep 14 2018 at 22:05, on Zulip):

cause how do you succinctly explain auto-deref

Santiago Pastorino (Sep 14 2018 at 22:06, on Zulip):

yep

Santiago Pastorino (Sep 15 2018 at 01:27, on Zulip):

@nikomatsakis got a chance to come back to the computer and see the output of this thing and is giving ...

Santiago Pastorino (Sep 15 2018 at 01:27, on Zulip):
-error[E0382]: use of moved value: `a.b`
-  --> $DIR/issue-52669.rs:23:5
+error[E0382]: borrow of moved value: `a.b`
+  --> $DIR/issue-52669.rs:25:5
    |
 LL |     foo(a);
    |         - value moved here
 LL |     a.b.clone()
-   |     ^^^ value used here after move
+   |     ^^^ value borrowed here after move
Santiago Pastorino (Sep 15 2018 at 01:27, on Zulip):

is using the word borrowed instead of use

Santiago Pastorino (Sep 15 2018 at 01:27, on Zulip):

I'd need to investigate a bit more

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

@Santiago Pastorino seems... not wrong. That is, a.b.clone() does borrow a.b

Santiago Pastorino (Sep 15 2018 at 10:43, on Zulip):

yes, I wasn’t sure if we wanted to match AST

nikomatsakis (Sep 15 2018 at 11:30, on Zulip):

I don't think we have to match 100% — although, separately, I think we should revisit this sort of terminology and decide how confusing it is, or if we can make it clearer.

Santiago Pastorino (Sep 15 2018 at 12:05, on Zulip):

:+1:

Santiago Pastorino (Sep 15 2018 at 15:35, on Zulip):

have opened https://github.com/rust-lang/rust/pull/54255

Santiago Pastorino (Sep 15 2018 at 15:35, on Zulip):

pretty sure that this part https://github.com/rust-lang/rust/pull/54255/files#diff-eebdb0687a3fb91fde3e02235e377676R558 can be made better

Santiago Pastorino (Sep 15 2018 at 15:35, on Zulip):

need to check a bit better

Santiago Pastorino (Sep 16 2018 at 14:03, on Zulip):

@nikomatsakis have fixed the comments you've made

nikomatsakis (Sep 17 2018 at 15:04, on Zulip):

@Santiago Pastorino one thing I was wondering is whether we want a test (like the one I added in a comment somewhere) just showing that the opposite works

nikomatsakis (Sep 17 2018 at 15:04, on Zulip):

we may have one

nikomatsakis (Sep 17 2018 at 15:04, on Zulip):

but maybe we should have a dedicated test

Santiago Pastorino (Sep 17 2018 at 15:05, on Zulip):

can do that

Santiago Pastorino (Sep 17 2018 at 15:05, on Zulip):

the rest looks good?

nikomatsakis (Sep 17 2018 at 15:06, on Zulip):

yep

Santiago Pastorino (Sep 17 2018 at 15:08, on Zulip):

btw

Santiago Pastorino (Sep 17 2018 at 15:08, on Zulip):
Note that the moves data already includes "parent" paths, so we don't have to worry about the other case: that is, if there is a move of a.b.c, it is already marked as a move of a.b and a as well, so we will generate the correct errors there.
Santiago Pastorino (Sep 17 2018 at 15:09, on Zulip):

where do you want to add that?

Santiago Pastorino (Sep 17 2018 at 15:10, on Zulip):

doing this ...

Santiago Pastorino (Sep 17 2018 at 15:10, on Zulip):
                // If we are found a use of a.b.c which was in error, then we want to look for
                // moves not only of a.b.c but also a.b and a.
                //
                // Note that the moves data already includes "parent" paths, so we don't have to
                // worry about the other case: that is, if there is a move of a.b.c, it is already
                // marked as a move of a.b and a as well, so we will generate the correct errors
                // there.
                let mut mpis = vec![mpi];
                let move_paths = &self.move_data.move_paths;
                mpis.extend(move_paths[mpi].parents(move_paths));
Santiago Pastorino (Sep 17 2018 at 15:10, on Zulip):

not sure if that's what you wanted

nikomatsakis (Sep 17 2018 at 15:12, on Zulip):

yes

Santiago Pastorino (Sep 17 2018 at 15:13, on Zulip):

done, running tests and all that and will push

Santiago Pastorino (Sep 17 2018 at 16:39, on Zulip):

@nikomatsakis pushed again, with that comment included and the test

Last update: Nov 22 2019 at 00:25UTC