Stream: t-compiler/wg-nll

Topic: #53773 erroneous loop diagnostic


nikomatsakis (Nov 13 2018 at 20:56, on Zulip):

Hi @Santiago Pastorino

Santiago Pastorino (Nov 13 2018 at 20:56, on Zulip):

hi

nikomatsakis (Nov 13 2018 at 20:58, on Zulip):

you may want to read this comment that I wrote on #54015

nikomatsakis (Nov 13 2018 at 20:58, on Zulip):

it explains how that code is working now

nikomatsakis (Nov 13 2018 at 20:58, on Zulip):

I'm not sure why that did not fix #53773

Santiago Pastorino (Nov 13 2018 at 20:59, on Zulip):

:+1:

Santiago Pastorino (Nov 13 2018 at 20:59, on Zulip):

I will start with this probably tomorrow

Santiago Pastorino (Nov 14 2018 at 15:31, on Zulip):

hey @nikomatsakis

Santiago Pastorino (Nov 14 2018 at 15:31, on Zulip):

checking this out

Santiago Pastorino (Nov 14 2018 at 15:31, on Zulip):

I see that the code compile without nll and doesn't compile with nll

Santiago Pastorino (Nov 14 2018 at 15:32, on Zulip):

to start with, is that correct?

nikomatsakis (Nov 14 2018 at 15:36, on Zulip):

I didn't notice that

nikomatsakis (Nov 14 2018 at 15:36, on Zulip):

good question :)

nikomatsakis (Nov 14 2018 at 15:37, on Zulip):

ah, yes, I think that's a case of NLL-fixed-by-NLL

Santiago Pastorino (Nov 14 2018 at 15:37, on Zulip):

so, it shouldn't compile?

Santiago Pastorino (Nov 14 2018 at 15:38, on Zulip):

I don't remember what's NLL-fixed-by-NLL :)

Santiago Pastorino (Nov 14 2018 at 15:38, on Zulip):

thinking about the example :)

nikomatsakis (Nov 14 2018 at 15:40, on Zulip):

point is, this is supposed to error

Santiago Pastorino (Nov 14 2018 at 15:40, on Zulip):

yeah, I'm checking to figure out why

Santiago Pastorino (Nov 14 2018 at 15:41, on Zulip):

I see this impl<'a> Drop for C<'a> { fn drop(&mut self) { } } and I guess that's why the stuff is dropped

Santiago Pastorino (Nov 14 2018 at 15:41, on Zulip):

but doesn't at that point you have references?

Santiago Pastorino (Nov 14 2018 at 15:41, on Zulip):

and C is created before that function

Santiago Pastorino (Nov 14 2018 at 15:41, on Zulip):

trying to see why a C is created inside fn error

nikomatsakis (Nov 14 2018 at 15:42, on Zulip):

I see this impl<'a> Drop for C<'a> { fn drop(&mut self) { } } and I guess that's why the stuff is dropped

point is that we assume Drop might touch the stuff in C

Santiago Pastorino (Nov 14 2018 at 15:42, on Zulip):

ahhh

Santiago Pastorino (Nov 14 2018 at 15:42, on Zulip):

fn next(&mut self) -> Option<C<'a>> { panic!() }

Santiago Pastorino (Nov 14 2018 at 15:42, on Zulip):

it's C there not &C

Santiago Pastorino (Nov 14 2018 at 15:43, on Zulip):

the iterator returns an owned thing

Santiago Pastorino (Nov 14 2018 at 15:52, on Zulip):

from the error ...

Santiago Pastorino (Nov 14 2018 at 15:52, on Zulip):
   Compiling playground v0.0.1 (/playground)
error[E0713]: borrow may still be in use when destructor runs
  --> src/main.rs:24:22
   |
24 |         members.push(child.raw);
   |                      ^^^^^^^^^
25 |     }
   |     - here, drop of `child` needs exclusive access to `*child.raw`, because the type `C<'_>` implements the `Drop` trait
26 |     members.len();
   |     ------- borrow used here, in later iteration of loop

error: aborting due to previous error

For more information about this error, try `rustc --explain E0713`.
error: Could not compile `playground`.

To learn more, run the command again with --verbose.
Santiago Pastorino (Nov 14 2018 at 15:52, on Zulip):

all we want is to remove , in later iteration of loop, right?

Santiago Pastorino (Nov 14 2018 at 15:53, on Zulip):

and I also wonder if the error when you remove member.len() is right, which is ...

Santiago Pastorino (Nov 14 2018 at 15:54, on Zulip):
   Compiling playground v0.0.1 (/playground)
error[E0713]: borrow may still be in use when destructor runs
  --> src/main.rs:24:22
   |
24 |         members.push(child.raw);
   |         -------      ^^^^^^^^^
   |         |
   |         borrow used here, in later iteration of loop
25 |     }
   |     - here, drop of `child` needs exclusive access to `*child.raw`, because the type `C<'_>` implements the `Drop` trait

error: aborting due to previous error

For more information about this error, try `rustc --explain E0713`.
error: Could not compile `playground`.

To learn more, run the command again with --verbose.
Santiago Pastorino (Nov 14 2018 at 15:54, on Zulip):

I'm unsure what "in later iteration of the loop" means

nikomatsakis (Nov 14 2018 at 16:49, on Zulip):

ah, so, I think I see the problem

nikomatsakis (Nov 14 2018 at 16:50, on Zulip):

the existing code prints that message if the given point was reached via a loop backedge

nikomatsakis (Nov 14 2018 at 16:50, on Zulip):

however

nikomatsakis (Nov 14 2018 at 16:50, on Zulip):

what we want is to print the message if the given point is ONLY reachable via a loop backedge

nikomatsakis (Nov 14 2018 at 16:50, on Zulip):

i.e., here, you could reach the members.len(); call after traversing the for loop

nikomatsakis (Nov 14 2018 at 16:50, on Zulip):

but you might also ... not

nikomatsakis (Nov 14 2018 at 16:50, on Zulip):

actually I wonder if.. hmm

nikomatsakis (Nov 14 2018 at 16:51, on Zulip):

so ok maybe my formulation wasn't right

nikomatsakis (Nov 14 2018 at 16:51, on Zulip):

but regardless the point is:

nikomatsakis (Nov 14 2018 at 16:52, on Zulip):

the code is working "correctly" in some sense -- in order to reach the members.len() call, you have to pass through the loop backedge; the problem here is that the code in question is not "in the loop"

nikomatsakis (Nov 14 2018 at 16:52, on Zulip):

this is actually a bit tricky to tell

nikomatsakis (Nov 14 2018 at 16:52, on Zulip):

in a simple way

nikomatsakis (Nov 14 2018 at 16:52, on Zulip):

@Santiago Pastorino are you following me at all?

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

hey

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

back

Santiago Pastorino (Nov 14 2018 at 17:15, on Zulip):

more or less yeah

Santiago Pastorino (Nov 14 2018 at 17:16, on Zulip):

what is "that message", the "in later iteration of the loop" part or all the message?

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

what do you mean by loop backedge? backedge unsure what's the meaning of that in english but I have a guess :)

nikomatsakis (Nov 14 2018 at 17:18, on Zulip):

what is "that message", the "in later iteration of the loop" part or all the message?

the "later iteration of loop" part specifically

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

a backedge is a control-flow graph is an edge A -> B where B dominates A (i.e., you can't reach A without going through B)

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

so in this case the final thing in the loop (A) branches back to the head of the loop (B)

Santiago Pastorino (Nov 14 2018 at 17:22, on Zulip):

what you meant is that you may skip the end of the loop and reach the len() call

Santiago Pastorino (Nov 14 2018 at 17:22, on Zulip):

in case for instance the iterator has no elements

nikomatsakis (Nov 14 2018 at 17:38, on Zulip):

yeah

nikomatsakis (Nov 14 2018 at 17:38, on Zulip):

but I think that was the wrong analysis

nikomatsakis (Nov 14 2018 at 17:38, on Zulip):

that is, in this example, the borrow occurs in the loop

nikomatsakis (Nov 14 2018 at 17:39, on Zulip):

it IS TRUE that we will always "go back around the loop" to reach the error

nikomatsakis (Nov 14 2018 at 17:39, on Zulip):

but the error was in the assumption that this meant we should say "in a later iteration of the loop"

nikomatsakis (Nov 14 2018 at 17:39, on Zulip):

I'm not 100% sure when we should print that :)

Santiago Pastorino (Nov 14 2018 at 17:45, on Zulip):

yeah, I don't understand either

Santiago Pastorino (Nov 14 2018 at 17:46, on Zulip):

but besides from that, the text in this particular case shouldn't have that part?

Santiago Pastorino (Nov 14 2018 at 17:46, on Zulip):

is that all I need to do?

Santiago Pastorino (Nov 14 2018 at 17:46, on Zulip):

I was kind of still trying to understand what needs to be done

nikomatsakis (Nov 14 2018 at 19:18, on Zulip):

well we need to decide when to print that text:)

nikomatsakis (Nov 14 2018 at 19:18, on Zulip):

I'd like to come up with a relatively simple rule

nikomatsakis (Nov 14 2018 at 19:18, on Zulip):

one rule that would work but is perhaps a bit too conservative

nikomatsakis (Nov 14 2018 at 19:19, on Zulip):

ah well

nikomatsakis (Nov 14 2018 at 19:19, on Zulip):

ok so one thing you could do:

nikomatsakis (Nov 14 2018 at 19:19, on Zulip):

if the "use" dominates the "borrow" -- that means it must come "before" the borrow

nikomatsakis (Nov 14 2018 at 19:19, on Zulip):

in that case, printing the label is fine

nikomatsakis (Nov 14 2018 at 19:19, on Zulip):

and indeed I think we could even .. get rid of the backedge search altogether?

Santiago Pastorino (Nov 14 2018 at 19:21, on Zulip):

@nikomatsakis ok, can try to get rid of that

Santiago Pastorino (Nov 14 2018 at 19:21, on Zulip):

if the "use" dominates the "borrow" -- that means it must come "before" the borrow

what do you mean by this?

nikomatsakis (Nov 14 2018 at 19:30, on Zulip):

example:

let mut x = 1;
let mut y = &22;
loop {
    *x += 1; // use
    y = &x; // borrow
}
nikomatsakis (Nov 14 2018 at 19:30, on Zulip):

however, that analysis is limited

nikomatsakis (Nov 14 2018 at 19:30, on Zulip):

I got distracted before completing

nikomatsakis (Nov 14 2018 at 19:30, on Zulip):

e.g., this would not trigger the "next iteration of loop" message, but arguably should

let mut x = 1;
let mut y = &22;
loop {
    if true {
       *x += 1; // use
     } else { }
    y = &x; // borrow
}
nikomatsakis (Nov 14 2018 at 19:31, on Zulip):

nor would this

let mut x = 1;
let mut y = &22;
loop {
    if true {
       *x += 1; // use
     } else {
        y = &x; // borrow
    }
}
nikomatsakis (Nov 14 2018 at 19:31, on Zulip):

I think the real test we want is something like this:

nikomatsakis (Nov 14 2018 at 19:31, on Zulip):

if so, they are part of the same loop

Santiago Pastorino (Nov 14 2018 at 21:27, on Zulip):

back

Santiago Pastorino (Nov 14 2018 at 21:27, on Zulip):

let me check again all this stuff

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

I still don't see some stuff

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

I'm more thinking as a user perspective

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

haven't yet thought from rustc perspective

Santiago Pastorino (Nov 14 2018 at 21:32, on Zulip):

I guess, we want "borrow used here in later iteration of loop" when a value was dropped and used later in the loop

Santiago Pastorino (Nov 14 2018 at 21:32, on Zulip):

@nikomatsakis is that right?

Santiago Pastorino (Nov 14 2018 at 21:33, on Zulip):

I wasn't sure I was understanding the phrase "later iteration of loop"

Santiago Pastorino (Nov 14 2018 at 21:34, on Zulip):

I guess it means that when you start iterating you have a value, you drop it and when you loop again you try to use that dropped thing

Santiago Pastorino (Nov 14 2018 at 21:34, on Zulip):

?

nikomatsakis (Nov 14 2018 at 21:37, on Zulip):

@Santiago Pastorino yeah it's basically meant to capture cases where

nikomatsakis (Nov 14 2018 at 21:37, on Zulip):

the execution order is not "obvious"

nikomatsakis (Nov 14 2018 at 21:38, on Zulip):

particularly since the snippet doesn't show the loop

nikomatsakis (Nov 14 2018 at 21:38, on Zulip):

actually, it might be nice to highlight the loop but anyway

nikomatsakis (Nov 14 2018 at 21:39, on Zulip):

so e.g. something like this (not the actual labels, but same idea) would be kind of confusing

2  | do_foo();
   | ------ then `foo` is called
3  | do_bar();
   | ^^^^^^ first `bar` is called

but maybe this is clearer:

2  | do_foo();
   | ------ then `foo` is called, in a later iteration of the loop
3  | do_bar();
   | ^^^^^^ first `bar` is called
Santiago Pastorino (Nov 14 2018 at 21:42, on Zulip):

ok

Santiago Pastorino (Nov 14 2018 at 21:42, on Zulip):

now let me switch to rustc mode and read what you've said :')

Santiago Pastorino (Nov 15 2018 at 21:17, on Zulip):

hey @nikomatsakis back

Santiago Pastorino (Nov 15 2018 at 21:17, on Zulip):

sorry that I ended not having time yesterday after our conversation

Santiago Pastorino (Nov 15 2018 at 21:17, on Zulip):

I've just read everything and makes sense

Santiago Pastorino (Nov 15 2018 at 21:17, on Zulip):

what I wonder is ...

Santiago Pastorino (Nov 15 2018 at 21:19, on Zulip):

I think the real test we want is something like this:

I wonder you're describing here the way to figure out if we are in a loop or not

Santiago Pastorino (Nov 15 2018 at 21:19, on Zulip):

don't we want something like ...

Santiago Pastorino (Nov 15 2018 at 21:22, on Zulip):

all the paths from borrow to the use pass through a backedge that is later than the use in the cfg and that back edge goes to a position that is before the use in the cfg

Santiago Pastorino (Nov 15 2018 at 21:22, on Zulip):

isn't that what we want?

nikomatsakis (Nov 15 2018 at 22:09, on Zulip):

I think that sounds roughly equivalent, @Santiago Pastorino

nikomatsakis (Nov 15 2018 at 22:09, on Zulip):

maybe even entirely equivalent :)

nikomatsakis (Nov 15 2018 at 22:09, on Zulip):

so in other words, "yes"

Santiago Pastorino (Nov 15 2018 at 22:47, on Zulip):

hehe, that's what I wanted to hear :)

Santiago Pastorino (Nov 15 2018 at 22:47, on Zulip):

now I see

Santiago Pastorino (Nov 15 2018 at 22:47, on Zulip):

makes sense

Santiago Pastorino (Nov 15 2018 at 22:47, on Zulip):

path from the use to the borrow is equivalent and more oriented to the code that I need to write than what I've said

Santiago Pastorino (Nov 15 2018 at 22:48, on Zulip):

cool

Santiago Pastorino (Nov 15 2018 at 22:48, on Zulip):

tomorrow I may implement this otherwise on monday

Santiago Pastorino (Nov 15 2018 at 22:48, on Zulip):

but seems easy :)

Santiago Pastorino (Nov 16 2018 at 16:50, on Zulip):

@nikomatsakis why does this https://github.com/rust-lang/rust/blob/6b9b97bd9b704f85f0184f7a213cc4d62bd9654c/src/librustc_mir/borrow_check/error_reporting.rs#L161 if branch fill is_loop_move and the next one doesn't?

Santiago Pastorino (Nov 16 2018 at 16:50, on Zulip):

by next one I meant, https://github.com/rust-lang/rust/blob/6b9b97bd9b704f85f0184f7a213cc4d62bd9654c/src/librustc_mir/borrow_check/error_reporting.rs#L162

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

@nikomatsakis hey

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

first check the questions I've left you

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

but then

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

I saw the traversed_back_edge thing you did

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

well unsure if it was you :)

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

I have a couple of questions about what do I have around

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

first, I guess from the MoveData structure I can get the borrow

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

I think the real test we want is something like this:

and then I was wondering, I guess I need to do something similar to what was done for the backedge, I mean, go to the get_moved_indexes fn and search the use

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

or is it the use already around?

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

I'm searching for that info in the code

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

I see, I have the used_place

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

anyway, I'm not sure where the information related to the borrow comes from

nikomatsakis (Nov 19 2018 at 20:00, on Zulip):

first check the questions I've left you

er, @Santiago Pastorino sorry, I missed those I guess :)

nikomatsakis (Nov 19 2018 at 20:01, on Zulip):

I saw the traversed_back_edge thing you did

well, it was @blitzerr, but I was mentoring

Santiago Pastorino (Nov 19 2018 at 20:35, on Zulip):

:+1:

Santiago Pastorino (Nov 19 2018 at 20:36, on Zulip):

@nikomatsakis when you have some time and can answer the rest would be appreciated :)

Santiago Pastorino (Nov 19 2018 at 20:36, on Zulip):

don't want to bother but I guess you may have missed some questions

Santiago Pastorino (Nov 19 2018 at 20:37, on Zulip):

but the main sure is where do I get the borrow from

nikomatsakis (Nov 19 2018 at 20:39, on Zulip):

why does this https://github.com/rust-lang/rust/blob/6b9b97bd9b704f85f0184f7a213cc4d62bd9654c/src/librustc_mir/borrow_check/error_reporting.rs#L161 if branch fill is_loop_move and the next one doesn't?

I think what is_loop_move is really asking is "did we already put a label on span?

i.e., if the use and move are not the same spot, we want to say "value used here after move". Otherwise, we label it differently.

nikomatsakis (Nov 19 2018 at 20:39, on Zulip):

is there another question I missed, @Santiago Pastorino ?

nikomatsakis (Nov 19 2018 at 20:40, on Zulip):

first, I guess from the MoveData structure I can get the borrow

oh, this

nikomatsakis (Nov 19 2018 at 20:40, on Zulip):

let me look at the code

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

yep that thing

Santiago Pastorino (Nov 19 2018 at 20:42, on Zulip):

why does this https://github.com/rust-lang/rust/blob/6b9b97bd9b704f85f0184f7a213cc4d62bd9654c/src/librustc_mir/borrow_check/error_reporting.rs#L161 if branch fill is_loop_move and the next one doesn't?

I think what is_loop_move is really asking is "did we already put a label on span?

i.e., if the use and move are not the same spot, we want to say "value used here after move". Otherwise, we label it differently.

it is weird to me because both branches are about loops

Santiago Pastorino (Nov 19 2018 at 20:42, on Zulip):

but one gets the is_loop_move = true and the other one not

Santiago Pastorino (Nov 19 2018 at 20:42, on Zulip):

check https://github.com/rust-lang/rust/blob/6b9b97bd9b704f85f0184f7a213cc4d62bd9654c/src/librustc_mir/borrow_check/error_reporting.rs#L156-L169

nikomatsakis (Nov 19 2018 at 20:43, on Zulip):

btw

nikomatsakis (Nov 19 2018 at 20:43, on Zulip):

looking at the actual diagnostic in question

nikomatsakis (Nov 19 2018 at 20:43, on Zulip):

but one gets the is_loop_move = true and the other one not

the name of the variable is not good

nikomatsakis (Nov 19 2018 at 20:43, on Zulip):

but also, this part of code is for a sligthly different error

nikomatsakis (Nov 19 2018 at 20:43, on Zulip):

that is, this part of the code is for "use after move"

nikomatsakis (Nov 19 2018 at 20:43, on Zulip):

but #53773 is actually " borrow may still be in use when destructor runs "

nikomatsakis (Nov 19 2018 at 20:44, on Zulip):

that message appears to be created here,

nikomatsakis (Nov 19 2018 at 20:44, on Zulip):

based on the UsedLaterInLoop variant, which is created here

nikomatsakis (Nov 19 2018 at 20:45, on Zulip):

so maybe the real question is why is_borrow_location_in_loop returns true

nikomatsakis (Nov 19 2018 at 20:46, on Zulip):

(I suspect, also, that maybe this code could be "brought together")

nikomatsakis (Nov 19 2018 at 20:46, on Zulip):

i.e., there is some kind of duplication here it feels like

nikomatsakis (Nov 19 2018 at 20:46, on Zulip):

in other words, @Santiago Pastorino, I may have sent you on a bit of a wild goose chase

Santiago Pastorino (Nov 19 2018 at 20:47, on Zulip):

hehe, what does that mean?

nikomatsakis (Nov 19 2018 at 20:49, on Zulip):

just saying that the code I was showing you before

nikomatsakis (Nov 19 2018 at 20:49, on Zulip):

isn't actually responsible for the bug I dont' think

nikomatsakis (Nov 19 2018 at 20:49, on Zulip):

rather, this other logic is

nikomatsakis (Nov 19 2018 at 20:49, on Zulip):

logic which is .. kind of similar though

Santiago Pastorino (Nov 19 2018 at 20:49, on Zulip):

ahh ok ok

Santiago Pastorino (Nov 19 2018 at 21:01, on Zulip):

@nikomatsakis weird

Santiago Pastorino (Nov 19 2018 at 21:01, on Zulip):

https://github.com/rust-lang/rust/blob/6b9b97bd9b704f85f0184f7a213cc4d62bd9654c/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs#L289

Santiago Pastorino (Nov 19 2018 at 21:02, on Zulip):

I don't see how this code would return true for our example

Santiago Pastorino (Nov 19 2018 at 21:02, on Zulip):

gonna investigate

Santiago Pastorino (Nov 19 2018 at 21:04, on Zulip):

maybe is called with the wrong location?

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

not sure

Santiago Pastorino (Nov 19 2018 at 21:18, on Zulip):

isn't it location instead of context.loc?

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

I was just trying with it and running but I'm not 100% will need to think and investigate a bit better

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

using location works but it may be for another reason

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

ok, I see what's the issue

Santiago Pastorino (Nov 19 2018 at 21:31, on Zulip):

it's what we talked about

Santiago Pastorino (Nov 19 2018 at 21:31, on Zulip):

we need to pass both locations

Santiago Pastorino (Nov 19 2018 at 21:31, on Zulip):

the one of the borrow and the one of the use

Santiago Pastorino (Nov 19 2018 at 21:31, on Zulip):

and check the things you mentioned

Santiago Pastorino (Nov 19 2018 at 21:31, on Zulip):

I think the real test we want is something like this:

this basically

nikomatsakis (Nov 19 2018 at 21:56, on Zulip):

yes, but applied to a different set of code I guess :)

Santiago Pastorino (Nov 19 2018 at 22:01, on Zulip):

@nikomatsakis what do you mean?

nikomatsakis (Nov 19 2018 at 22:01, on Zulip):

I just mean that the code we were talking about earlier had to do with move errors

nikomatsakis (Nov 19 2018 at 22:01, on Zulip):

and is not executing here from what I can tell

nikomatsakis (Nov 19 2018 at 22:01, on Zulip):

unless I'm missing something

Santiago Pastorino (Nov 19 2018 at 22:02, on Zulip):

@nikomatsakis I was talking about this https://github.com/rust-lang/rust/blob/6b9b97bd9b704f85f0184f7a213cc4d62bd9654c/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs#L289

Santiago Pastorino (Nov 19 2018 at 22:03, on Zulip):

seems that the important part is https://github.com/rust-lang/rust/blob/6b9b97bd9b704f85f0184f7a213cc4d62bd9654c/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs#L227-L230

Santiago Pastorino (Nov 19 2018 at 22:03, on Zulip):

I guess that function needs to take both locations

Santiago Pastorino (Nov 19 2018 at 22:03, on Zulip):

location of use and location of move

Santiago Pastorino (Nov 19 2018 at 22:03, on Zulip):

and do the logic you mentioned

nikomatsakis (Nov 20 2018 at 14:29, on Zulip):

@Santiago Pastorino yeah, the function currently seems to just compute whether the borrow location can reach itself

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

which isn't really an interesting question

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

thinking more about this

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

maybe we want a function like this:

nikomatsakis (Nov 20 2018 at 14:31, on Zulip):

can_reach_ignoring_backedges(P, Q) -- is there a path from P to Q that doesn't use any backedges?

nikomatsakis (Nov 20 2018 at 14:32, on Zulip):

in that case, you could say something like this:

nikomatsakis (Nov 20 2018 at 14:32, on Zulip):

is it as simple as that? feels like it is

Santiago Pastorino (Nov 20 2018 at 14:33, on Zulip):

yeah, exactly

Santiago Pastorino (Nov 20 2018 at 14:34, on Zulip):

the search needs to go in forward direction and stop as soon as we reach the end and ignoring the already visited locations

nikomatsakis (Nov 20 2018 at 14:39, on Zulip):

and ignoring backedges

nikomatsakis (Nov 20 2018 at 14:40, on Zulip):

(any edge P -> Q where Q dominates P)

Santiago Pastorino (Nov 20 2018 at 14:46, on Zulip):

yep

Santiago Pastorino (Nov 20 2018 at 14:46, on Zulip):

but wouldn't just moving forward without visiting already visited locations do that already?

nikomatsakis (Nov 20 2018 at 14:48, on Zulip):

already visited when?

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

you're right ;)

Santiago Pastorino (Nov 20 2018 at 16:17, on Zulip):

@nikomatsakis was starting to build this thing

Santiago Pastorino (Nov 20 2018 at 16:17, on Zulip):

quick question

Santiago Pastorino (Nov 20 2018 at 16:17, on Zulip):
        stack.extend(mir.predecessor_locations(context.loc).map(|predecessor| {
            let is_back_edge = context.loc.dominates(predecessor, &self.dominators);
            (predecessor, is_back_edge)
        }));
Santiago Pastorino (Nov 20 2018 at 16:18, on Zulip):

this is the code of the is_back_edge stuff

Santiago Pastorino (Nov 20 2018 at 16:18, on Zulip):

in the code I'm writing the search is forward

Santiago Pastorino (Nov 20 2018 at 16:18, on Zulip):

so I'm looking at one location

Santiago Pastorino (Nov 20 2018 at 16:18, on Zulip):

I need to get the successors

Santiago Pastorino (Nov 20 2018 at 16:19, on Zulip):

and check if successors dominates location

Santiago Pastorino (Nov 20 2018 at 16:19, on Zulip):

right?

Santiago Pastorino (Nov 20 2018 at 16:21, on Zulip):

and the other question is ... that happens when the TerminatorKind is Goto?

nikomatsakis (Nov 20 2018 at 17:57, on Zulip):

right?

right @Santiago Pastorino

nikomatsakis (Nov 20 2018 at 17:57, on Zulip):

and the other question is ... that happens when the TerminatorKind is Goto?

that should be like any other terminator, I think

nikomatsakis (Nov 20 2018 at 17:57, on Zulip):

there is a method to get the successors I think

Santiago Pastorino (Nov 20 2018 at 17:58, on Zulip):

I don't know why this particular method is making a difference between different terminators

Santiago Pastorino (Nov 20 2018 at 17:58, on Zulip):

I guess I just should use a successors method and that's it?

nikomatsakis (Nov 20 2018 at 17:58, on Zulip):

which method

nikomatsakis (Nov 20 2018 at 17:58, on Zulip):

oh

nikomatsakis (Nov 20 2018 at 17:58, on Zulip):

I think it's just written in ignorance of the successors() helper

Santiago Pastorino (Nov 20 2018 at 17:58, on Zulip):

https://github.com/rust-lang/rust/blob/6b9b97bd9b704f85f0184f7a213cc4d62bd9654c/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs#L317

Santiago Pastorino (Nov 20 2018 at 17:58, on Zulip):

ok

nikomatsakis (Nov 20 2018 at 17:59, on Zulip):

yeah, I think that all that code can be replaced with one line :)

Santiago Pastorino (Nov 20 2018 at 18:05, on Zulip):

hehe

Santiago Pastorino (Nov 20 2018 at 18:05, on Zulip):

yeah

Santiago Pastorino (Nov 20 2018 at 18:06, on Zulip):

ok, that thing confused me a lot

Santiago Pastorino (Nov 20 2018 at 18:06, on Zulip):

and also, is the if part needed?

Santiago Pastorino (Nov 20 2018 at 18:06, on Zulip):

need to check successors method it may already handle that

Santiago Pastorino (Nov 20 2018 at 18:11, on Zulip):

ok, I need more or less this https://github.com/rust-lang/rust/blob/6b9b97bd9b704f85f0184f7a213cc4d62bd9654c/src/librustc_mir/borrow_check/nll/explain_borrow/find_use.rs#L77-L93

nikomatsakis (Nov 20 2018 at 18:16, on Zulip):

sounds right @Santiago Pastorino, I've wanted to make a helper for that exact bit of logic before

Santiago Pastorino (Nov 20 2018 at 18:19, on Zulip):

:+1:

Santiago Pastorino (Nov 20 2018 at 19:12, on Zulip):

thing is ready, I'm testing this

Santiago Pastorino (Nov 20 2018 at 19:18, on Zulip):

@nikomatsakis in case you want to check it out https://github.com/rust-lang/rust/compare/master...spastorino:erroneous-loop-diagnostic-in-nll

Santiago Pastorino (Nov 20 2018 at 19:18, on Zulip):

if tests pass gonna open a PR

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

I'm not sure if I really need the visited_locations, since that the algorithm is already discarding back edges

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

I guess we don't and I can remove that

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

well actually we need that despite of back edges :)

nikomatsakis (Nov 20 2018 at 19:27, on Zulip):

well actually we need that despite of back edges :)

yeah, could be a DAG

nikomatsakis (Nov 20 2018 at 19:27, on Zulip):

I mean you don't need it

nikomatsakis (Nov 20 2018 at 19:27, on Zulip):

but it's wasteful

Santiago Pastorino (Nov 20 2018 at 19:31, on Zulip):

yep

Santiago Pastorino (Nov 20 2018 at 19:35, on Zulip):
diff --git a/src/test/ui/borrowck/borrowck-lend-flow-loop.nll.stderr b/src/test/ui/borrowck/borrowck-lend-flow-loop.nll.stderr
index 19de3582c88..396fd6ffa0c 100644
--- a/src/test/ui/borrowck/borrowck-lend-flow-loop.nll.stderr
+++ b/src/test/ui/borrowck/borrowck-lend-flow-loop.nll.stderr
@@ -8,13 +8,13 @@ LL |         borrow(&*v); //[ast]~ ERROR cannot borrow
    |                ^^^ immutable borrow occurs here
 ...
 LL |     *x = box 5;
-   |     -- mutable borrow used here, in later iteration of loop
+   |     -- mutable borrow later used here

 error[E0502]: cannot borrow `*v` as immutable because it is also borrowed as mutable
   --> $DIR/borrowck-lend-flow-loop.rs:109:16
    |
 LL |         **x += 1;
-   |         -------- mutable borrow used here, in later iteration of loop
+   |         -------- mutable borrow later used here
 LL |         borrow(&*v); //[ast]~ ERROR cannot borrow
    |                ^^^ immutable borrow occurs here
 ...
diff --git a/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr b/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr
index 5301ee7a403..3b56c00145c 100644
--- a/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr
+++ b/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr
@@ -4,7 +4,7 @@ error[E0499]: cannot borrow `x` as mutable more than once at a time
 LL |             1 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
    |                    ----      ^^^^^^ second mutable borrow occurs here
    |                    |
-   |                    first borrow used here, in later iteration of loop
+   |                    first borrow later used here
 ...
 LL |             _ => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
    |                              ------ first mutable borrow occurs here
@@ -13,7 +13,7 @@ error[E0499]: cannot borrow `x` as mutable more than once at a time
   --> $DIR/borrowck-mut-borrow-linear-errors.rs:25:30
    |
 LL |             1 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
-   |                    ---- first borrow used here, in later iteration of loop
+   |                    ---- first borrow later used here
 LL |             //[mir]~^ ERROR [E0499]
 LL |             2 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
    |                              ^^^^^^ second mutable borrow occurs here
Santiago Pastorino (Nov 20 2018 at 19:35, on Zulip):

@nikomatsakis it's changing some stuff

Santiago Pastorino (Nov 20 2018 at 19:36, on Zulip):

in particular

Santiago Pastorino (Nov 20 2018 at 19:36, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.rs

Santiago Pastorino (Nov 20 2018 at 19:36, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr

Santiago Pastorino (Nov 20 2018 at 19:38, on Zulip):

in particular this line https://github.com/rust-lang/rust/blob/master/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr#L7

Santiago Pastorino (Nov 20 2018 at 19:38, on Zulip):

now says

Santiago Pastorino (Nov 20 2018 at 19:38, on Zulip):

first borrow later used here

Santiago Pastorino (Nov 20 2018 at 19:39, on Zulip):

and same here https://github.com/rust-lang/rust/blob/master/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr#L16

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

I don't understand that example to be honest

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

what's wrong with it in nll mode?

Santiago Pastorino (Nov 20 2018 at 19:43, on Zulip):

also, the borrow and the use are siblings in the graph so I'm never going to reach one from the other one

Santiago Pastorino (Nov 20 2018 at 19:51, on Zulip):

opened PR meanwhile we discuss https://github.com/rust-lang/rust/pull/56113

nikomatsakis (Nov 20 2018 at 19:59, on Zulip):

first borrow later used here

i.e., it no longer says "in later iteration of loop"?

nikomatsakis (Nov 20 2018 at 19:59, on Zulip):

seems like it still should say that

nikomatsakis (Nov 20 2018 at 19:59, on Zulip):

let me check the PR

Santiago Pastorino (Nov 20 2018 at 20:06, on Zulip):

@nikomatsakis isn't it because borrow and use are siblings?

Santiago Pastorino (Nov 20 2018 at 20:06, on Zulip):

never going to reach the arm of the match

nikomatsakis (Nov 20 2018 at 20:11, on Zulip):

yes, so it should print the message

nikomatsakis (Nov 20 2018 at 20:11, on Zulip):

i.e., if one can't reach the other without going through the loop

nikomatsakis (Nov 20 2018 at 20:12, on Zulip):

then you say "in later iteration of loop

nikomatsakis (Nov 20 2018 at 20:19, on Zulip):

left a first round of nits
https://github.com/rust-lang/rust/pull/56113#pullrequestreview-176955464

Santiago Pastorino (Nov 20 2018 at 20:20, on Zulip):

i.e., if one can't reach the other without going through the loop

didn't understand what you mean here

Santiago Pastorino (Nov 20 2018 at 20:21, on Zulip):

shouldn't we also print if the borrow itself is inside the loop? as it was done before

nikomatsakis (Nov 20 2018 at 20:22, on Zulip):

nope

nikomatsakis (Nov 20 2018 at 20:22, on Zulip):

that's not really interesting

nikomatsakis (Nov 20 2018 at 20:23, on Zulip):

I mean the idea is to capture sort of surprising flow

nikomatsakis (Nov 20 2018 at 20:23, on Zulip):

if you see

nikomatsakis (Nov 20 2018 at 20:24, on Zulip):
  | read(p);
  |      - `x` later used here
  | p = &x;
  |      ^ `x` borrowed here
nikomatsakis (Nov 20 2018 at 20:24, on Zulip):

it's a bit "surprising" because the use appears to come first

nikomatsakis (Nov 20 2018 at 20:24, on Zulip):

that is why we want to note "because of a loop"

nikomatsakis (Nov 20 2018 at 20:25, on Zulip):

but if you see p = &x; .. read(p);, you don't really care if that's within a loop or not

Santiago Pastorino (Nov 20 2018 at 20:25, on Zulip):

yeah, that makes sense

Santiago Pastorino (Nov 20 2018 at 20:25, on Zulip):

what I don't see then is what's going on in those examples where changed the output

nikomatsakis (Nov 20 2018 at 20:28, on Zulip):

as I commented on your PR, I think you just need to invert the sense of the if

nikomatsakis (Nov 20 2018 at 20:28, on Zulip):

that is, you are saying "print the loop message if there IS a path from borrow to use"

nikomatsakis (Nov 20 2018 at 20:28, on Zulip):

but I think we want to print it there is NOT a path

nikomatsakis (Nov 20 2018 at 20:29, on Zulip):

or at least, no path that doesn't require backedges

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

ok, let me check all that

Santiago Pastorino (Nov 20 2018 at 20:38, on Zulip):

@nikomatsakis I didn't understand your feedback about visited_locations

Santiago Pastorino (Nov 20 2018 at 20:38, on Zulip):

https://github.com/rust-lang/rust/pull/56113#discussion_r235154167

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

should I change to a FxHashSet or remove it?

nikomatsakis (Nov 20 2018 at 20:39, on Zulip):

keep it, make it a set

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

if I should remove it, why?

nikomatsakis (Nov 20 2018 at 20:39, on Zulip):

a vector is just a poor choice of data structure for this

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

yes

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

agree

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

but what about this https://github.com/rust-lang/rust/pull/56113#discussion_r235154222 ?

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

@nikomatsakis I guess you regretted of that comment?

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

I guess you did the if hash_set.insert thing :)

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

:+1:

nikomatsakis (Nov 20 2018 at 20:47, on Zulip):

no I mean you don't need that

nikomatsakis (Nov 20 2018 at 20:47, on Zulip):

if you check instead later on

nikomatsakis (Nov 20 2018 at 20:47, on Zulip):

when you do the insert

nikomatsakis (Nov 20 2018 at 20:47, on Zulip):

i.e., this code checks when you remove things from the stack

nikomatsakis (Nov 20 2018 at 20:47, on Zulip):

but it's better to check before you put them on

nikomatsakis (Nov 20 2018 at 20:47, on Zulip):

otherwise, you can grow the stack with a lot of duplicates

nikomatsakis (Nov 20 2018 at 20:47, on Zulip):

not super important but...

nikomatsakis (Nov 20 2018 at 20:47, on Zulip):

..it irks me:P

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

yes yes

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

figured out after asking

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

makes sense

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

that happens to me for copying code without paying a lot of attention ;)

nikomatsakis (Nov 20 2018 at 20:56, on Zulip):

=)

Santiago Pastorino (Nov 20 2018 at 21:11, on Zulip):

made all the changes the issue still adds in later iteration of loop :(

nikomatsakis (Nov 20 2018 at 21:13, on Zulip):

let me take a look

nikomatsakis (Nov 20 2018 at 21:14, on Zulip):

can you push @Santiago Pastorino to #56113 ?

Santiago Pastorino (Nov 20 2018 at 21:14, on Zulip):

done

Santiago Pastorino (Nov 20 2018 at 21:16, on Zulip):

forgot to remove the skip locations if

Santiago Pastorino (Nov 20 2018 at 21:16, on Zulip):

but still that shouldn't make any difference

Santiago Pastorino (Nov 20 2018 at 21:18, on Zulip):

!location.dominates shouldn't be just location.dominates?

Santiago Pastorino (Nov 20 2018 at 21:18, on Zulip):

because you're filtering those

Santiago Pastorino (Nov 20 2018 at 21:21, on Zulip):

(deleted)

Santiago Pastorino (Nov 20 2018 at 21:57, on Zulip):

@nikomatsakis I was wrong in what I've said, still doesn't work

Santiago Pastorino (Nov 20 2018 at 22:05, on Zulip):

I meant, anyway, the posted code still doesn't work

Santiago Pastorino (Nov 20 2018 at 22:33, on Zulip):

@nikomatsakis and also there are a lot of failures with that code :( https://github.com/rust-lang/rust/pull/56113#issuecomment-440452364

nikomatsakis (Nov 20 2018 at 22:41, on Zulip):

@Santiago Pastorino see comments here

Santiago Pastorino (Nov 20 2018 at 23:03, on Zulip):

still same issue

Santiago Pastorino (Nov 20 2018 at 23:04, on Zulip):

pushed

Santiago Pastorino (Nov 21 2018 at 12:34, on Zulip):

@nikomatsakis 2 existing examples fail and the new test still says in later iteration of loop

Santiago Pastorino (Nov 21 2018 at 12:34, on Zulip):

https://travis-ci.org/rust-lang/rust/jobs/457696480

Santiago Pastorino (Nov 21 2018 at 12:34, on Zulip):

if you have any other tip let me know, I'm going to debug this later

nikomatsakis (Nov 21 2018 at 18:59, on Zulip):

@Santiago Pastorino yeah I'm not quite sure

nikomatsakis (Nov 21 2018 at 18:59, on Zulip):

I'll have to do a local build I guess

Santiago Pastorino (Nov 21 2018 at 19:58, on Zulip):

@nikomatsakis :+1:

Santiago Pastorino (Nov 21 2018 at 19:58, on Zulip):

I guess maybe tomorrow I can start debugging this

Santiago Pastorino (Nov 27 2018 at 11:25, on Zulip):

@nikomatsakis we need to discuss about this issue :)

Santiago Pastorino (Nov 27 2018 at 11:25, on Zulip):

not sure what exactly should I do now

Santiago Pastorino (Nov 27 2018 at 11:30, on Zulip):

saw the discussion on Discord but I saw that @eddyb was not ok with adding info to the CFG

Santiago Pastorino (Nov 27 2018 at 11:31, on Zulip):

dunno what to do to fix that then :)

eddyb (Nov 27 2018 at 11:34, on Zulip):

I'd defer to @nikomatsakis - it might not be that bad, we already have "false edges" and whatnot

Ariel Ben-Yehuda (Nov 27 2018 at 11:35, on Zulip):

false edges are normative, while this is only for diagnostics

nikomatsakis (Nov 27 2018 at 15:25, on Zulip):

Yeah, we should discuss. I personally lean against modifying the MIR, but I guess we should decide how good a job we want to do.

One option might be to modify the way that we print the diagnostic, such that it makes sense in either scenario

nikomatsakis (Nov 27 2018 at 15:25, on Zulip):

Another option might be to be more conservative

nikomatsakis (Nov 27 2018 at 15:25, on Zulip):

Another option is to consider the "lexical scoping" information

nikomatsakis (Nov 27 2018 at 15:27, on Zulip):

each statement has a source_info, we could check whether the scope of the backedge target includes the scope of the backedge source (in which case, they are part of the same loop from the user's POV)

nikomatsakis (Nov 27 2018 at 15:27, on Zulip):

not sure if scopes are that complete, maybe @eddyb or @Ariel Ben-Yehuda remembers

nikomatsakis (Nov 27 2018 at 15:28, on Zulip):

One option might be to modify the way that we print the diagnostic, such that it makes sense in either scenario

For example:

error[E0713]: borrow may still be in use when destructor runs
  --> src/main.rs:24:22
   |
23 |    for child in archive.iter() {
   |    --- after the borrow, code flows around this loop to reach the use
24 |         members.push(child.raw);
   |                      ^^^^^^^^^
25 |     }
   |     - here, drop of `child` needs exclusive access to `*child.raw`, because the type `C<'_>` implements the `Drop` trait
26 |     members.len();
   |     ------- borrow used here

error: aborting due to previous error
eddyb (Nov 27 2018 at 15:29, on Zulip):

we usually emit scopes after let/inside match

nikomatsakis (Nov 27 2018 at 15:31, on Zulip):

I guess we could easily add scopes for loops

eddyb (Nov 27 2018 at 15:31, on Zulip):

I think it's possible the scope check might make sense, although @Ariel Ben-Yehuda and @Matthew Jasper found that the only use of visibility_scope in MIR borrowck was redundant (on top of being confusing)

nikomatsakis (Nov 27 2018 at 15:32, on Zulip):

otherwise, I guess we would add some sort of "no-op" statement that marks exiting a loop?

nikomatsakis (Nov 27 2018 at 15:32, on Zulip):

I think that's what @Ariel Ben-Yehuda proposed

nikomatsakis (Nov 27 2018 at 15:33, on Zulip):

I'm wondering about cases like this:

loop {
    if something {
        while something_else {
            p = &foo;
        }
    } else {
        foo += 1;
        use(p);
    }
}
nikomatsakis (Nov 27 2018 at 15:33, on Zulip):

here, my scope proposal would not print any message, but arguably you should

nikomatsakis (Nov 27 2018 at 15:34, on Zulip):

(otoh I think people can sometimes figure this stuff out, it doens't have to be perfect)

nikomatsakis (Nov 27 2018 at 15:35, on Zulip):

well, the scope proposal could figure it out

nikomatsakis (Nov 27 2018 at 15:35, on Zulip):

you'd have to track the 'outermost scope' of the backedges you took

Santiago Pastorino (Nov 27 2018 at 17:13, on Zulip):

Yeah, we should discuss. I personally lean against modifying the MIR, but I guess we should decide how good a job we want to do.

One option might be to modify the way that we print the diagnostic, such that it makes sense in either scenario

I may be completely wrong but can't we just use a separate structure for this? like a bitset where cfg nodes are labelled as this node is the head of the loop and then in diagnostics you look for it, so is separated from the mir?

Santiago Pastorino (Nov 27 2018 at 17:14, on Zulip):

maybe I got incorrectly what you mean by modifying the mir :)

eddyb (Nov 27 2018 at 18:24, on Zulip):

the real questions are "why do you want that kind of precise AST shape information?" and "can you talk about the code without referring to specific syntax?"

nikomatsakis (Nov 27 2018 at 18:25, on Zulip):

maybe I got incorrectly what you mean by modifying the mir :)

well, that's a kind of modification -- adding a "side channel" for more information

Ariel Ben-Yehuda (Nov 27 2018 at 20:42, on Zulip):

The problem with relying on scopes of back-edges is that scopes of Goto instructions can get scrambled pretty well.

Ariel Ben-Yehuda (Nov 27 2018 at 20:43, on Zulip):

in the simplify-cfg pass. We could try maintaining them better, but that feels like more of a hack than having a specific "loop header" statement.

Ariel Ben-Yehuda (Nov 27 2018 at 20:43, on Zulip):

BTW, in that case you can use the source_info.scope and don't need the visibility_scope - loops are lexically scoped.

Ariel Ben-Yehuda (Nov 27 2018 at 20:50, on Zulip):

So the main Q is whether we want to make sure that the source-info of back-edges always represents the source-info of their loop through simplifycfg, or whether we prefer to have loop header statements with the source-info of the loop.

Santiago Pastorino (Nov 28 2018 at 22:10, on Zulip):

so yeah, what would be a nice idea to try then?

Santiago Pastorino (Nov 28 2018 at 22:11, on Zulip):

maybe we can try a first simple approach so we fix this and leave an issue to improve?

Santiago Pastorino (Nov 28 2018 at 22:11, on Zulip):

unsure if that would solve something

nikomatsakis (Nov 28 2018 at 22:28, on Zulip):

so I was thinking if we can find cases that are "unambiguously correct". Basically that would mean finding out that (a) borrow and use are part of the same loop and (b) the borrow cannot reach the use except through a backedge.

I think that would suffice, but it would mean that if you have something that "appears" to be nested in the loop, but breaks out, it doesn't get a message.

But how to decide if they are in the same loop?

I was imagining something like:

nikomatsakis (Nov 28 2018 at 22:28, on Zulip):

that's not quite right around nested loops I don't think

nikomatsakis (Nov 28 2018 at 22:29, on Zulip):

@Santiago Pastorino :point_up:

Ariel Ben-Yehuda (Nov 28 2018 at 23:18, on Zulip):

That would miss the case when there is a return after the use

let x;
let t = 0;
loop {
    if * {
        return t;
    } else {
        x = &t;
    }
}
Ariel Ben-Yehuda (Nov 28 2018 at 23:26, on Zulip):

and, if implemented stupidly, also the case when there are 2 backedges going to the same place (just a loop with an if).

nikomatsakis (Nov 29 2018 at 14:00, on Zulip):

I am aware that it would miss that case, it seems "ok"

nikomatsakis (Nov 29 2018 at 14:00, on Zulip):

basically I was saying "we could do this without any fancy work and it will be helpful sometimes and not un-helpful othertimes"

nikomatsakis (Nov 29 2018 at 14:00, on Zulip):

I think longer term I would like to think a bit more about our presentation anyway

nikomatsakis (Nov 29 2018 at 14:01, on Zulip):

as I don't think this message is especially clear to people

nikomatsakis (Nov 29 2018 at 14:02, on Zulip):

still @Ariel Ben-Yehuda can you expand a bit on your loop header idea? it doesn't seem enough to me to just have a "loop header block" -- or did you mean for that to be the start of the loop body I guess? so the idea would be if we can't reach the use without going through the start of the loop body?

but does that handle a case like this?

loop {
    if condition {
        break;
    }
    *borrow*;
}
*use;*
pnkfelix (Nov 29 2018 at 14:03, on Zulip):

clearly the answer is more dataflow :wink:

nikomatsakis (Nov 29 2018 at 14:03, on Zulip):

I still sort of wonder if we can't just present the control flow in a way that doesn't talk about "loops" per se -- e.g.,

or something.

nikomatsakis (Nov 29 2018 at 14:04, on Zulip):

but it feels like just only saying ", in previous iteration of loop" when we know it is appropriate is "ok"

nikomatsakis (Nov 29 2018 at 14:04, on Zulip):

(if we can indeed find suitably narrow criteria)

nikomatsakis (Nov 29 2018 at 14:12, on Zulip):

I guess at the end of the day I am probably happy to go either way, though I'd like to see a complete proposal. It seems to me that some sort of "loop exit" blocks probably suffice. Maybe @Ariel Ben-Yehuda wrote it in a comment I should go and re-read.

I just feel a bit reluctant to add more statements into the MIR "just" for diagnostics. It feels like something that could have some unexpected complication down the line, though I guess these sort of "no-op" statements are particularly unlikely to cause problems, since we can always strip them out.

It also seems to me that we are encoding lexical information, which is sort of the same as the unsafe check, so maybe it's inconsistent that we do so differently? (Like, why not use a "scope" to encode whether this statement was "part of" a loop or not?)

nikomatsakis (Nov 29 2018 at 14:15, on Zulip):

Regarding my earlier proposal:

nikomatsakis (Nov 29 2018 at 14:15, on Zulip):

it seems I never actually wrote that second part out explicitly :P but it was on my mind

Ariel Ben-Yehuda (Nov 29 2018 at 15:32, on Zulip):

The idea was to also use the source-info of the loop header statement

Ariel Ben-Yehuda (Nov 29 2018 at 15:33, on Zulip):

to check whether a point is within a loop

Ariel Ben-Yehuda (Nov 29 2018 at 15:34, on Zulip):

Y is "across the loop" from X iff there exists a loop header H such that
a) X and Y are both in a subscope of H
2) every path from X to Y goes through H

Ariel Ben-Yehuda (Nov 29 2018 at 15:36, on Zulip):

the loop header is just a place where we can slap the loop scope

nikomatsakis (Nov 29 2018 at 16:07, on Zulip):

oh, ok, that makes sense

nikomatsakis (Nov 29 2018 at 16:08, on Zulip):

so we are ultimately using scope, but we are just making a fixed statement for it

nikomatsakis (Nov 29 2018 at 16:08, on Zulip):

(versus, say, putting some scope on the basic block, which is also possible)

Santiago Pastorino (Nov 30 2018 at 18:36, on Zulip):

@nikomatsakis I've implemented your conservative idea

nikomatsakis (Nov 30 2018 at 18:36, on Zulip):

how's it work :)

Santiago Pastorino (Nov 30 2018 at 18:36, on Zulip):

for now the code is gross

Santiago Pastorino (Nov 30 2018 at 18:36, on Zulip):

have repeated some stuff over

Santiago Pastorino (Nov 30 2018 at 18:37, on Zulip):

I just wanted to check if it worked or not

Santiago Pastorino (Nov 30 2018 at 18:37, on Zulip):

I'm getting a couple of changes

Santiago Pastorino (Nov 30 2018 at 18:37, on Zulip):
diff --git a/src/test/ui/borrowck/borrowck-lend-flow-loop.nll.stderr b/src/test/ui/borrowck/borrowck-lend-flow-loop.nll.stderr
index 19de3582c88..396fd6ffa0c 100644
--- a/src/test/ui/borrowck/borrowck-lend-flow-loop.nll.stderr
+++ b/src/test/ui/borrowck/borrowck-lend-flow-loop.nll.stderr
@@ -8,13 +8,13 @@ LL |         borrow(&*v); //[ast]~ ERROR cannot borrow
    |                ^^^ immutable borrow occurs here
 ...
 LL |     *x = box 5;
-   |     -- mutable borrow used here, in later iteration of loop
+   |     -- mutable borrow later used here

 error[E0502]: cannot borrow `*v` as immutable because it is also borrowed as mutable
   --> $DIR/borrowck-lend-flow-loop.rs:109:16
    |
 LL |         **x += 1;
-   |         -------- mutable borrow used here, in later iteration of loop
+   |         -------- mutable borrow later used here
 LL |         borrow(&*v); //[ast]~ ERROR cannot borrow
    |                ^^^ immutable borrow occurs here
 ...
diff --git a/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr b/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr
index 5301ee7a403..3b56c00145c 100644
--- a/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr
+++ b/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr
@@ -4,7 +4,7 @@ error[E0499]: cannot borrow `x` as mutable more than once at a time
 LL |             1 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
    |                    ----      ^^^^^^ second mutable borrow occurs here
    |                    |
-   |                    first borrow used here, in later iteration of loop
+   |                    first borrow later used here
 ...
 LL |             _ => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
    |                              ------ first mutable borrow occurs here
@@ -13,7 +13,7 @@ error[E0499]: cannot borrow `x` as mutable more than once at a time
   --> $DIR/borrowck-mut-borrow-linear-errors.rs:25:30
    |
 LL |             1 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
-   |                    ---- first borrow used here, in later iteration of loop
+   |                    ---- first borrow later used here
 LL |             //[mir]~^ ERROR [E0499]
 LL |             2 => { addr.push(&mut x); } //[ast]~ ERROR [E0499]
    |                              ^^^^^^ second mutable borrow occurs here
Santiago Pastorino (Nov 30 2018 at 18:39, on Zulip):

actually the first one is right

Santiago Pastorino (Nov 30 2018 at 18:39, on Zulip):
fn block_overarching_alias_mut() {
    // In this instance, the borrow encompasses the entire closure call.

    let mut v: Box<_> = box 3;
    let mut x = &mut v;
    for _ in 0..3 {
        borrow(&*v); //[ast]~ ERROR cannot borrow
        //[nll]~^ ERROR cannot borrow
    }
    *x = box 5;
}
Santiago Pastorino (Nov 30 2018 at 18:39, on Zulip):

it's actually the thing we needed to test

Santiago Pastorino (Nov 30 2018 at 18:40, on Zulip):
 LL |     *x = box 5;
-   |     -- mutable borrow used here, in later iteration of loop
+   |     -- mutable borrow later used here
Santiago Pastorino (Nov 30 2018 at 18:40, on Zulip):

so first it mentioned in later iteration of loop and now it doesn't

Santiago Pastorino (Nov 30 2018 at 18:41, on Zulip):

then

Santiago Pastorino (Nov 30 2018 at 18:41, on Zulip):
fn while_aliased_mut_cond(cond: bool, cond2: bool) {
    let mut v: Box<_> = box 3;
    let mut w: Box<_> = box 4;
    let mut x = &mut w;
    while cond {
        **x += 1;
        borrow(&*v); //[ast]~ ERROR cannot borrow
        //[nll]~^ ERROR cannot borrow
        if cond2 {
            x = &mut v; //[ast]~ ERROR cannot borrow
        }
    }
}
Santiago Pastorino (Nov 30 2018 at 18:41, on Zulip):
 LL |         **x += 1;
-   |         -------- mutable borrow used here, in later iteration of loop
+   |         -------- mutable borrow later used here
Santiago Pastorino (Nov 30 2018 at 18:41, on Zulip):

it's wrong

Santiago Pastorino (Nov 30 2018 at 18:47, on Zulip):

:)

Santiago Pastorino (Nov 30 2018 at 18:47, on Zulip):

I'm filtering backedges but I want to find a path that ends in a backedge, need to avoid filtering that one

nikomatsakis (Nov 30 2018 at 18:49, on Zulip):

hmm

nikomatsakis (Nov 30 2018 at 18:49, on Zulip):

that seems lke "the classic" case

nikomatsakis (Nov 30 2018 at 18:49, on Zulip):

what rule did you implement exactly, @Santiago Pastorino ? :)

Santiago Pastorino (Nov 30 2018 at 18:50, on Zulip):

I was imagining something like:

This one

nikomatsakis (Nov 30 2018 at 18:53, on Zulip):

hmm, it feels like it should work for that example

nikomatsakis (Nov 30 2018 at 18:53, on Zulip):

did you update the PR?

Santiago Pastorino (Nov 30 2018 at 18:53, on Zulip):

no

Santiago Pastorino (Nov 30 2018 at 18:53, on Zulip):

but I found the issue

Santiago Pastorino (Nov 30 2018 at 18:54, on Zulip):

maybe I wrote a bit weird and you didn't figure that :)

Santiago Pastorino (Nov 30 2018 at 18:54, on Zulip):

I'm filtering backedges but I want to find a path that ends in a backedge, need to avoid filtering that one

Santiago Pastorino (Nov 30 2018 at 18:54, on Zulip):

have already added that

Santiago Pastorino (Nov 30 2018 at 18:54, on Zulip):

running tests again

Santiago Pastorino (Nov 30 2018 at 18:57, on Zulip):

@nikomatsakis added the thing but didn't make any difference

nikomatsakis (Nov 30 2018 at 19:01, on Zulip):

what do you mean by didn't make any difference?

Santiago Pastorino (Nov 30 2018 at 19:02, on Zulip):

that it's still wrong

Santiago Pastorino (Nov 30 2018 at 19:02, on Zulip):

the change I added was

Santiago Pastorino (Nov 30 2018 at 19:02, on Zulip):
-                        .filter(|s| !self.is_back_edge(location, *s))
+                        .filter(|s| !self.is_back_edge(location, *s) || to_any.contains(s))
Santiago Pastorino (Nov 30 2018 at 19:03, on Zulip):

with that contains or without it, I get the same results

Santiago Pastorino (Nov 30 2018 at 19:03, on Zulip):

have pushed in case you want to check out

nikomatsakis (Nov 30 2018 at 19:04, on Zulip):

ok

Santiago Pastorino (Nov 30 2018 at 19:07, on Zulip):

I found a thing that is wrong but shouldn't make any difference

nikomatsakis (Nov 30 2018 at 19:08, on Zulip):

@Santiago Pastorino maybe this?

Santiago Pastorino (Nov 30 2018 at 19:09, on Zulip):

right, that's the issue

Santiago Pastorino (Nov 30 2018 at 19:09, on Zulip):

and also

Santiago Pastorino (Nov 30 2018 at 19:09, on Zulip):

that if is wrong, right?

Santiago Pastorino (Nov 30 2018 at 19:09, on Zulip):

this https://github.com/rust-lang/rust/pull/56113/files#diff-fe005d3dcf77fde8f189617b2599e10eR340 is always false

Santiago Pastorino (Nov 30 2018 at 19:13, on Zulip):

@nikomatsakis :wait_one_second:

nikomatsakis (Nov 30 2018 at 20:59, on Zulip):

any more attempts, @Santiago Pastorino ?

Santiago Pastorino (Nov 30 2018 at 23:03, on Zulip):

@nikomatsakis I left since we've talked

Santiago Pastorino (Nov 30 2018 at 23:04, on Zulip):

gonna see if I have a couple of minutes to try again

Santiago Pastorino (Dec 01 2018 at 13:05, on Zulip):

I'm consistently hitting this ...

Santiago Pastorino (Dec 01 2018 at 13:05, on Zulip):
error: Could not compile `rustc`.

To learn more, run the command again with --verbose.
command did not execute successfully: "/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--features" "" "--manifest-path" "/home/santiago/src/oss/rust1/src/rustc/Cargo.toml" "--message-format" "json"
expected success, got: exit code: 101
thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1101:9
stack backtrace:
   0:     0x556b86761a9f - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::h3c55afa80a996a32
                               at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1:     0x556b8676ed47 - std::sys_common::backtrace::print::hcef907ea7a07cd40
                               at libstd/sys_common/backtrace.rs:71
                               at libstd/sys_common/backtrace.rs:59
   2:     0x556b8676784f - std::panicking::default_hook::{{closure}}::hf541876c65f36ea7
                               at libstd/panicking.rs:211
   3:     0x556b867675b4 - std::panicking::default_hook::h4ecc06b30ae4459d
                               at libstd/panicking.rs:227
   4:     0x556b86767f2e - std::panicking::rust_panic_with_hook::h0cc6d9cc9f34a947
                               at libstd/panicking.rs:476
   5:     0x556b86740027 - std::panicking::begin_panic::he4089b50490d89cc
                               at libstd/panicking.rs:410
   6:     0x556b85fb242a - bootstrap::compile::run_cargo::h24b31fcf4cc848dc
                               at bootstrap/compile.rs:1101
   7:     0x556b85faa84a - <bootstrap::compile::Rustc as bootstrap::builder::Step>::run::h5a63412c57dc1205
                               at bootstrap/compile.rs:503
   8:     0x556b8636a544 - bootstrap::builder::Builder::ensure::h059931c28a4fae58
                               at bootstrap/builder.rs:1215
   9:     0x556b85fb03b7 - <bootstrap::compile::Assemble as bootstrap::builder::Step>::run::hb1adf09f74ea3290
                               at bootstrap/compile.rs:959
  10:     0x556b86471d54 - bootstrap::builder::Builder::ensure::hf971c4454e83cc78
                               at bootstrap/builder.rs:1215
  11:     0x556b86496079 - bootstrap::builder::Builder::compiler::h261980de65d32f94
                               at bootstrap/builder.rs:579
  12:     0x556b85fa55f6 - <bootstrap::compile::Std as bootstrap::builder::Step>::make_run::h5b3c0a1480f5473a
                               at bootstrap/compile.rs:55
  13:     0x556b86491400 - bootstrap::builder::StepDescription::maybe_run::h4cbaeb9b41a83a82
                               at bootstrap/builder.rs:191
  14:     0x556b86491fd4 - bootstrap::builder::StepDescription::run::h81efe27184fab9ba
                               at bootstrap/builder.rs:234
  15:     0x556b86496028 - bootstrap::builder::Builder::run_step_descriptions::h15ca74516d14f99b
                               at bootstrap/builder.rs:571
  16:     0x556b86495e6a - bootstrap::builder::Builder::execute_cli::h2b142c7158a4cf31
                               at bootstrap/builder.rs:561
  17:     0x556b85d9ae66 - bootstrap::Build::build::h9b1d15d32ff16426
                               at bootstrap/lib.rs:479
  18:     0x556b85cf7414 - bootstrap::main::h4e2538703a968edf
                               at bootstrap/bin/main.rs:29
  19:     0x556b85cf751f - std::rt::lang_start::{{closure}}::h89db4fb3346f68bc
                               at libstd/rt.rs:74
  20:     0x556b86767952 - std::panicking::try::do_call::h203c4989fde7f201
                               at libstd/rt.rs:59
                               at libstd/panicking.rs:310
  21:     0x556b86779f79 - __rust_maybe_catch_panic
                               at libpanic_unwind/lib.rs:102
  22:     0x556b8674f40a - std::rt::lang_start_internal::h80fd76ad39d9c8d9
                               at libstd/panicking.rs:289
                               at libstd/panic.rs:392
                               at libstd/rt.rs:58
  23:     0x556b85cf74f8 - std::rt::lang_start::he4ebe066ecf58bc3
                               at libstd/rt.rs:74
  24:     0x556b85cf7499 - main
  25:     0x7fc8e7717222 - __libc_start_main
  26:     0x556b85cf725d - _start
  27:                0x0 - <unknown>
failed to run: /home/santiago/src/oss/rust1/build/bootstrap/debug/bootstrap build -i --stage 1 --keep-stage 1 src/libstd
Build completed unsuccessfully in 0:04:41
``
Santiago Pastorino (Dec 01 2018 at 21:28, on Zulip):

@nikomatsakis pushed for you to review, there are some tests that changed the output though

Santiago Pastorino (Dec 01 2018 at 21:28, on Zulip):

the first one I think is more correct

Santiago Pastorino (Dec 01 2018 at 21:28, on Zulip):

from the second one on the changes are also more correct because it's not in later iteration of loop, it's inside the loop but it's just later

Santiago Pastorino (Dec 01 2018 at 21:30, on Zulip):

PR https://github.com/rust-lang/rust/pull/56113

Santiago Pastorino (Dec 01 2018 at 21:42, on Zulip):

@nikomatsakis as I told you there is already three times where we are repeating a bfs search

Santiago Pastorino (Dec 01 2018 at 21:42, on Zulip):

and this PR keeps adding ;)

Santiago Pastorino (Dec 01 2018 at 21:42, on Zulip):

let me know if you want to refactor that as part of this PR or you just want to merge this and we can refactor as a separate thing

nikomatsakis (Dec 04 2018 at 11:51, on Zulip):

@Santiago Pastorino say more about this "could not compile rustc" thing? did it go away?

nikomatsakis (Dec 04 2018 at 11:52, on Zulip):

as it happens, I regularly hit a segfault in llvm that is baffling me (it never happens twice)

nikomatsakis (Dec 04 2018 at 11:52, on Zulip):

I've been tracking backtraces

Santiago Pastorino (Dec 04 2018 at 13:57, on Zulip):

@nikomatsakis it go away yes

Santiago Pastorino (Dec 04 2018 at 13:57, on Zulip):

I think it’s the same issue I always hit

Santiago Pastorino (Dec 04 2018 at 13:57, on Zulip):

related to memory

Santiago Pastorino (Dec 04 2018 at 13:58, on Zulip):

ended shutting down computer and executing without even entering the ui, went straight to the console

Santiago Pastorino (Dec 04 2018 at 13:58, on Zulip):

and removed build just in case

nikomatsakis (Dec 04 2018 at 15:28, on Zulip):

@Santiago Pastorino so I think what @Ariel Ben-Yehuda means here is that the test they describe would be a better way to test "is this use part of the loop X" -- and I think I agree

nikomatsakis (Dec 04 2018 at 15:29, on Zulip):

basically, we first find that the borrow B reaches the use U through some outermost backedge, right? the target L of that backedge is the "head" of the outermost loop

nikomatsakis (Dec 04 2018 at 15:30, on Zulip):

er, that's confusingly written

nikomatsakis (Dec 04 2018 at 15:30, on Zulip):

but basically we find that the borrow B can only reach the use U by going around some loop L

nikomatsakis (Dec 04 2018 at 15:30, on Zulip):

and we want to check if the use U is also in that loop

nikomatsakis (Dec 04 2018 at 15:32, on Zulip):

the check that @Ariel Ben-Yehuda proposed is basically to do a DFS from the use forward, looking for the loop header, but stopping whenever we reach a node that is not dominated by the loop header (which therefore falls outside the loop)

Santiago Pastorino (Dec 05 2018 at 16:49, on Zulip):

@nikomatsakis about this comment https://github.com/rust-lang/rust/pull/56113#discussion_r238454978 from @Ariel Ben-Yehuda, so when he talks about N we are talking about the use but when he talks about L he is talking about the header of the loop and we were using the borrow

Santiago Pastorino (Dec 05 2018 at 16:49, on Zulip):

because if it is the header of the loop we are back to square 1, how can we get it?

Santiago Pastorino (Dec 05 2018 at 16:49, on Zulip):

was reading the stuff on github, now I'm seeing your comments here which seems to clarify a bit :)

Santiago Pastorino (Dec 05 2018 at 17:10, on Zulip):

basically, we first find that the borrow B reaches the use U through some outermost backedge, right? the target L of that backedge is the "head" of the outermost loop

so you meant that L is the thing we found?

Santiago Pastorino (Dec 05 2018 at 17:10, on Zulip):

unsure what the target L of that backedge is the "head" of the outermost loop

Santiago Pastorino (Dec 05 2018 at 17:10, on Zulip):

but basically we find that the borrow B can only reach the use U by going around some loop L

don't understand what some loop L means, I guess you're talking about the target of a backedge?

Santiago Pastorino (Dec 05 2018 at 17:11, on Zulip):

unsure what the target L of that backedge is the "head" of the outermost loop

I think I see what you meant, so basically this is what we already did

Santiago Pastorino (Dec 05 2018 at 17:12, on Zulip):

https://github.com/rust-lang/rust/blob/34059401f4f1cb5ef0a0ec6a4d73503eb9d29eb9/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs#L299

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

ok seems like I need to adapt the rest

Santiago Pastorino (Dec 05 2018 at 17:14, on Zulip):

@nikomatsakis do we have a reusable dfs around?, what I've mainly is bfs used around

Santiago Pastorino (Dec 05 2018 at 17:25, on Zulip):

this https://github.com/rust-lang/rust/blob/34059401f4f1cb5ef0a0ec6a4d73503eb9d29eb9/src/librustc_mir/borrow_check/error_reporting.rs#L1113 suggest a dfs given the label there but it's not really a dfs

Santiago Pastorino (Dec 05 2018 at 17:27, on Zulip):

I mean, I can build one but don't want to keep adding repeated stuff :)

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

generic bfs and dfs over locations seems something useful :)

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

@Santiago Pastorino so -- was it clear what I meant by "Loop L" and all that?

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

I don't think so

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

have pushed something to see what the CI says https://github.com/rust-lang/rust/pull/56113

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

@nikomatsakis ^^^

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

I already see there are some differences on the used in used here, in later iteration of loop that I need to check if are right or not

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

@nikomatsakis checking the diffs now

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

for instance

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

https://github.com/rust-lang/rust/pull/56113/files#diff-49dad6257ff5648bdcbcbe5e1b5445f7

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

the first change is for the better and the second one is for the worse

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

unsure if it's one of the confusible cases we were talking about

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

gonna check that in a while and check the rest of the changes

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

no, the second one is better too

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

because there's no need to later in the loop it's just later but it happens that both statements are in the loop

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

from my point of view this example is better

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

@nikomatsakis this case doesn't seem to be right https://github.com/rust-lang/rust/pull/56113/files#diff-9c381bde352f6799a887ef7dda1c3318L7

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

I mean, after my PR

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

what do you think?

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

well there are more cases like that one

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

maybe if use location = borrow location and that's inside of the loop we should just emit that loop message?

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

actually not 100% sure why the code don't cover that gonna check

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

@nikomatsakis after checking all the examples I think all are better BUT src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.mir.stderr

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

checking that one again

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

I agree that borrowck-mut-borrow-linear-errors.mir.stderr seems less good

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

@nikomatsakis I think I see what's going on

Santiago Pastorino (Jan 08 2019 at 19:22, on Zulip):
        StorageLive(_10);                // bb9[0]: scope 3 at test.rs:8:20: 8:24
        _10 = &'_#7r mut _2;             // bb9[1]: scope 3 at test.rs:8:20: 8:24
        StorageLive(_11);                // bb9[2]: scope 3 at test.rs:8:30: 8:36
        _11 = &'_#8r mut _1;             // bb9[3]: scope 3 at test.rs:8:30: 8:36
        _9 = const <std::vec::Vec<T>>::push(move _10, move _11) -> [return: bb17, unwind: bb8]; // bb9[4]: scope 3 at test.rs:8:20: 8:37
Santiago Pastorino (Jan 08 2019 at 19:22, on Zulip):

the borrow happens before the use

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

so it's not printing the in later iteration of loop

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

but given that high level code is the same line, it seems like it would be better to print that

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

@nikomatsakis thoughts? what should we do?

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

@Santiago Pastorino I left my comments as to which I think are "better" or "less good" — put another way, places where I think printing "in a later iteration of the loop" would make sense

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

the borrow happens before the use

yes, I wondered if this would be it

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

well, actually, that's what is confusing to me

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

that is, the borrow of _10 = &mut _2 occurs before the access to _1

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

but I suspect that the error is occuring at the point of the call

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

because of 2-phase borrows

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

yes, that's what I meant

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

that seems... ok to me

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

if that's ok I guess this is good to go

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

I think the ideal would be

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

to notice that the use is a call

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

or at least good to review :)

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

and say something like ", during the call"

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

ahh ok

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

I think I wrote something about this in the RFC

nikomatsakis (Jan 08 2019 at 19:28, on Zulip):

the reason being that "during the call" is a case where the order in which we write things is sort of reverse

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

yep

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

do you want me to add during the call in this PR?

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

because you mentioned an RFC which I'm not sure which one is

nikomatsakis (Jan 08 2019 at 19:28, on Zulip):

oh I meant the NLL RFC

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

are you planning some future changes?

nikomatsakis (Jan 08 2019 at 19:28, on Zulip):

I think I used that phrasing in some examples

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

ahh ok ok

nikomatsakis (Jan 08 2019 at 19:28, on Zulip):

but this doesn't really require an RFC or anything

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

I thought some major changes were going on for a bit :)

nikomatsakis (Jan 08 2019 at 19:29, on Zulip):

I gotta run -- I guess I don't care too much about whether we put it in the PR, if it's easy maybe why not? it feels a bit different but not unrelated

nikomatsakis (Jan 08 2019 at 19:29, on Zulip):

:)

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

yeah, let me do that in this PR then

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

@nikomatsakis back for a bit with this

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

first of all

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

can you elaborate a bit more on https://github.com/rust-lang/rust/pull/56113#discussion_r246120584 ?

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

I mean, more specifically, do you think that the new output is not wrong? it should look like before? or do you want something different?

nikomatsakis (Jan 09 2019 at 20:21, on Zulip):

well, this is the function in question

fn while_aliased_mut_cond(cond: bool, cond2: bool) {
    let mut v: Box<_> = box 3;
    let mut w: Box<_> = box 4;
    let mut x = &mut w;
    while cond {
        **x += 1;
        borrow(&*v); //[ast]~ ERROR cannot borrow
        //[nll]~^ ERROR cannot borrow
        if cond2 {
            x = &mut v; //[ast]~ ERROR cannot borrow
        }
    }
}
nikomatsakis (Jan 09 2019 at 20:21, on Zulip):

and the error

error[E0502]: cannot borrow `*v` as immutable because it is also borrowed as mutable
  --> $DIR/borrowck-lend-flow-loop.rs:109:16
   |
LL |         **x += 1;
   |         -------- mutable borrow later used here
LL |         borrow(&*v); //[ast]~ ERROR cannot borrow
   |                ^^^ immutable borrow occurs here
...
LL |             x = &mut v; //[ast]~ ERROR cannot borrow
   |                 ------ mutable borrow occurs here
Santiago Pastorino (Jan 09 2019 at 20:22, on Zulip):

yes

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

that's using the output of my PR

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

in this case, the "flow of events" is that

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

so you see that we did indeed have to go around the loop

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

otoh the case is so convoluted...

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

I see

nikomatsakis (Jan 09 2019 at 20:23, on Zulip):

I feel like this is a good example where it'd be nicer to format the error differently

nikomatsakis (Jan 09 2019 at 20:23, on Zulip):

in a more general way

nikomatsakis (Jan 09 2019 at 20:23, on Zulip):

but that's a whole separate matter

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

and that means, leave as is? :)

nikomatsakis (Jan 09 2019 at 20:23, on Zulip):

I'm fine with it as is

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

ok

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

another thing

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

about the during the call thing now

nikomatsakis (Jan 09 2019 at 20:23, on Zulip):

I was just saying that it didn't seem more correct than before kind of "for the record" =)

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

:+1:

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

https://github.com/rust-lang/rust/blob/35d970dc09eead49521bbe905180a44f4639c05a/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs#L66

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

I was expecting that to be a Call

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

but if it were a call it should have printed borrow later used by call

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

which is something similar to what we want

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

the thing is that it's a LaterUseKind::Other thing

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

so ... when you said, look for a call were you referring to that? or to something else?

nikomatsakis (Jan 09 2019 at 20:27, on Zulip):

that does look like the same general idea

nikomatsakis (Jan 09 2019 at 20:28, on Zulip):

I feel like we probably have a lot of "strands" of code that do similar/same things

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

yeah, now I wonder why is that an Other in the moment of printing the message

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

and it's not a call

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

ok, I'd need to check this out properly

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

addr.push(&mut x); addr was the use, so I guess it should be a call, right?

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

I mean, theoretically

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

or am I getting something wrong?

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

something is confusing to me

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

https://github.com/rust-lang/rust/blob/35d970dc09eead49521bbe905180a44f4639c05a/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs#L446-L465

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

@nikomatsakis why the use being a function is tagged as a terminator thing?

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

basically I think I don't understand this https://github.com/rust-lang/rust/blob/35d970dc09eead49521bbe905180a44f4639c05a/src/librustc/mir/mod.rs#L1130-L1146

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

or that's talking about something different to what I'm looking for

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

well maybe I'm not understanding what is /// Block ends with a call of a converging function

nikomatsakis (Jan 09 2019 at 21:56, on Zulip):

addr.push(&mut x); addr was the use, so I guess it should be a call, right?

maybe :) but maybe it's some code elsewhere in the call lowering

nikomatsakis (Jan 09 2019 at 21:57, on Zulip):

re: the Terminator, a "call" in MIr is always the last thing in a basic block (since it can panic)

nikomatsakis (Jan 09 2019 at 21:57, on Zulip):

(given that it can panic, a call has two successors -- the "normal return" and the "unwind" path)

nikomatsakis (Jan 09 2019 at 21:57, on Zulip):

does that answer your question, @Santiago Pastorino ?

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

yes

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

it makes sense and it’s more or less what I thought :blush:

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

if @Santiago Pastorino the "during the call" thing is hard, it seems ok not to do it :)

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

unsure if it’s hard :blush:

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

I already left but tomorrow next thing to do I guess is dumping the mir and debugging this example to see what’s going on

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

@nikomatsakis any other tip?

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

I mean, if I can’t figure out a call there

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

@nikomatsakis

Santiago Pastorino (Jan 10 2019 at 14:57, on Zulip):
#![feature(nll)]

fn main() {
    let mut x = 1;
    let mut addr = vec![];
    loop {
        match 1 {
            _ => addr.push(&mut x);
        }
    }
}
Santiago Pastorino (Jan 10 2019 at 14:57, on Zulip):

vs

Santiago Pastorino (Jan 10 2019 at 14:57, on Zulip):
#![feature(nll)]

fn main() {
    let mut x = 1;
    let mut addr = vec![];
    loop {
        match 1 {
            1 => addr.push(&mut x),
            _ => addr.push(&mut x),
        }
    }
}
Santiago Pastorino (Jan 10 2019 at 14:57, on Zulip):

first example prints

Santiago Pastorino (Jan 10 2019 at 14:57, on Zulip):
error[E0499]: cannot borrow `x` as mutable more than once at a time
  --> test.rs:15:28
   |
15 |             _ => addr.push(&mut x),
   |                            ^^^^^^ mutable borrow starts here in previous iteration of loop

error: aborting due to previous error

For more information about this error, try `rustc --explain E0499`.
Santiago Pastorino (Jan 10 2019 at 14:58, on Zulip):

which seems correct to me

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

second example prints

Santiago Pastorino (Jan 10 2019 at 14:58, on Zulip):
error[E0499]: cannot borrow `x` as mutable more than once at a time
  --> test.rs:15:28
   |
15 |             1 => addr.push(&mut x),
   |                  ----      ^^^^^^ second mutable borrow occurs here
   |                  |
   |                  first borrow later used here
16 |             _ => addr.push(&mut x),
   |                            ------ first mutable borrow occurs here

error[E0499]: cannot borrow `x` as mutable more than once at a time
  --> test.rs:16:28
   |
16 |             _ => addr.push(&mut x),
   |                            ^^^^^^ mutable borrow starts here in previous iteration of loop

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0499`.
Santiago Pastorino (Jan 10 2019 at 14:59, on Zulip):

which is ... weird I guess but not entirely wrong in the sense of mir maybe

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

I guess on mir the _ => part shows up before?

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

can check the mir output

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

but anyway, there's also something weird about the first and second thing noted there

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

well it actually makes sense if in mir _ => happens before

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

the problem is that the 1 => arm is wrong on it's own as the _ =>

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

so saying mutable borrow starts here in previous iteration of loop only for _ => and not for 1 => may be weird

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

I guess what happens before depends on what block.terminator().successors() returns if returns first _ => that would be the first thing, unsure how that logic works

nikomatsakis (Jan 10 2019 at 16:53, on Zulip):

I mean I think in the end there are many possible diagnostics here @Santiago Pastorino

nikomatsakis (Jan 10 2019 at 16:54, on Zulip):

that is, what comes "first" and "second" is sort of arbitrary

nikomatsakis (Jan 10 2019 at 16:54, on Zulip):

since these things are in a loop

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

yeah, I'd expect first as the one that is above in the source code

nikomatsakis (Jan 10 2019 at 16:55, on Zulip):

the problem is that the 1 => arm is wrong on it's own as the _ =>

I don't quite get what you mean by this

nikomatsakis (Jan 10 2019 at 16:55, on Zulip):

yeah, I'd expect first as the one that is above in the source code

it does seem like that would be a better choice; I'm not sure how hard it would be to fix.

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

I mean, both arms are wrong if you put them in the code by themselves

nikomatsakis (Jan 10 2019 at 16:55, on Zulip):

I guess I have to dig a bit in the code

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

I think the diagnostic is also mixing both arms

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

which is another way for that code of being wrong

nikomatsakis (Jan 10 2019 at 16:56, on Zulip):

right, basically there are multiple ways

nikomatsakis (Jan 10 2019 at 16:56, on Zulip):

the code finds "a way"

nikomatsakis (Jan 10 2019 at 16:56, on Zulip):

and reports it to you

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

exactly

nikomatsakis (Jan 10 2019 at 16:56, on Zulip):

I'm not sure how hard it would be for it to find "the best way"

nikomatsakis (Jan 10 2019 at 16:56, on Zulip):

I guess I feel like this is an entirely different problem

nikomatsakis (Jan 10 2019 at 16:56, on Zulip):

from the one you set out to fix

nikomatsakis (Jan 10 2019 at 16:56, on Zulip):

though possibly one worth tackling

nikomatsakis (Jan 10 2019 at 16:57, on Zulip):

basically, looking for cases where -- in a loop -- we can "rejigger" the points we consider to be "first second and third" in order to get a more natural display

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

agreed

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

in that case should we just merge the PR as is?

nikomatsakis (Jan 10 2019 at 17:04, on Zulip):

probably yes

nikomatsakis (Jan 10 2019 at 17:04, on Zulip):

i'll give it a once over later today

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

@nikomatsakis is there a way to kick travis?

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

to run tests again over the PR?

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

I know it should be green, but now after I've pushed a rustfmt commit it failed

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

doh, my bad

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

thought it was giving timeouts but was my bad

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

ci is green https://github.com/rust-lang/rust/pull/56113 :)

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

@nikomatsakis just opened the follow up issue https://github.com/rust-lang/rust/issues/57528 we talked about

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

@nikomatsakis homu is failing for x86_64-gnu-nopt https://github.com/rust-lang/rust/pull/56113#issuecomment-453730748

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

unsure what's going on

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

@Santiago Pastorino hmm there must be some non-determinism here

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

are we using hashmaps to control ordering or something somewhere?

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

often this results in error messages that vary slightly depending on the platform

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

couldn't that be successors?

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

the output of this strongly depends on the successors list

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

can we just get successors sorted by location?

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

that would make things better from my perspective

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

the successors list from the MIR is meaningful and is not in some arbitrary order

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

but you could potentially sort the result if you wanted

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

that is, this particular algorithm doesn't care

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

that is, this particular algorithm doesn't care

what do you mean by this?

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

what particular algorithm?

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

skimming the PR, I don't see any obvious problem

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

what particular algorithm?

the one you wrote :)

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

that decides whether or not to print a label

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

in my case depending on how I get successors it may say in later iteration or not

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

or at least I believe that could happen

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

if I have ...

Santiago Pastorino (Jan 15 2019 at 16:13, on Zulip):
use
something
borrow
nikomatsakis (Jan 15 2019 at 16:13, on Zulip):

does this pass for you locally?

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

and I get

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

maybe it's just an outdated stderr file?

Santiago Pastorino (Jan 15 2019 at 16:13, on Zulip):
borrow
something
use
Santiago Pastorino (Jan 15 2019 at 16:13, on Zulip):

output would be different

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

but it pass on a lot of platforms on travis

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

the question would be ...

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

can I get

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

or

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

depending on how successors are returned?

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

or that's not possible?

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

I guess it could in the match example we talked about

Santiago Pastorino (Jan 15 2019 at 16:53, on Zulip):
fn main() {
    let mut x = 1;
    let mut addr = vec![];
    loop {
        match 1 {
            1 => addr.push(&mut x),
            _ => addr.push(&mut x),
        }
    }
}
Santiago Pastorino (Jan 15 2019 at 16:53, on Zulip):

which one shows up first, 1 => or _ => and in the code it was _ => if I remember correctly

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

@nikomatsakis I run tests locally and they pass

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

have rebased and force pushed

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

maybe we should r+ again and see what happens?

nikomatsakis (Jan 15 2019 at 20:42, on Zulip):

hmm

nikomatsakis (Jan 15 2019 at 20:42, on Zulip):

I feel like the order of successors ought to be consistent across all platforms

nikomatsakis (Jan 15 2019 at 20:42, on Zulip):

so that should not be it

nikomatsakis (Jan 15 2019 at 20:42, on Zulip):

must be something else

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

can we r+ again and see?

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

just in case

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

I've just rebased on top of master and pushed again

nikomatsakis (Jan 15 2019 at 21:34, on Zulip):

@Santiago Pastorino we could, if the PR is green, but it seems strange to be a spurious error

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

hmm yeah, gonna check out tomorrow

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

@nikomatsakis so this is failing according to Travis https://travis-ci.com/rust-lang/rust/jobs/169962373 on x86_64-gnu-nopt is there an easy way to get that image and debug?

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

You may want to ask in the infra channel on Discord -- I think there is some way to use docker images but don't know the details.

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

maybe I can take a look later today or something though

nikomatsakis (Jan 17 2019 at 15:10, on Zulip):

confusing

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

:+1:

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

@nikomatsakis nothing come to my mind other than sorting successors and I was suggested on #infra to run src/ci/docker/run.sh

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

I guess I can pass a flag or something to just use the right platform

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

need to investigate

nikomatsakis (Jan 18 2019 at 19:29, on Zulip):

ok, I'll have another look in a bit

pnkfelix (Jan 28 2019 at 14:45, on Zulip):

@Santiago Pastorino have you actually managed to confirm in some fashion that successors is returning different orderings?

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

@pnkfelix no, I have this issue in my todo list still

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

jumped to #52708

pnkfelix (Jan 29 2019 at 09:37, on Zulip):

@Santiago Pastorino maybe I should work-steal this (#53773) from you in that case?

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

yes, feel free to do so

pnkfelix (Jan 29 2019 at 11:06, on Zulip):

okay. I'm not ready to work-steal anything immediately, but if I manage to finish one of the tasks I'm multi-tasking between right now, I'll consider doing it then.

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

:+1:

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

I was planning to come back to this task at some point also

Santiago Pastorino (Feb 03 2019 at 21:27, on Zulip):

@nikomatsakis @pnkfelix about this issue I've added a commit that fixes the thing https://github.com/rust-lang/rust/pull/56113

Santiago Pastorino (Feb 03 2019 at 21:28, on Zulip):

it was lacking a bless over .nll files

Santiago Pastorino (Feb 03 2019 at 21:28, on Zulip):

and had no idea that when opening a PR this tests didn't run

Santiago Pastorino (Feb 03 2019 at 21:29, on Zulip):

they only run on approval

Last update: Nov 21 2019 at 13:55UTC