Stream: t-compiler/wg-nll

Topic: #54015 Improve move error loop detection


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

cc @blitzerr let's chat here if needed...

blitzerr (Sep 12 2018 at 11:51, on Zulip):

Got it:+1:

blitzerr (Sep 13 2018 at 17:05, on Zulip):

@nikomatsakis
I have a question related to the mentoring notes
"This only works when you have something like (1) loop { drop(x); } -- here, the drop(x) from the previous loop iteration is conflicting with the drop(x) with the next iteration. But it doesn't work for a case like this:(2) loop { use(x); drop(x); }, which is what we have in this example."
#1 is obvious but why is #2 not the same? I think in #2 as well in the second iteration of the loop, it is being used as well. But also in #2, drop(x) is trying to borrow what use(x) already borrowed ? Is my understanding correct ?

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

@blitzerr

So, in the second example:

loop {
  drop(x); // L1
  drop(x); // L2
}

when we get to L1, we see that x is uninitialized on entry. So we walk backwards to find where the move came from. We go backwards around the loop and stop when we reach L2.

Now, the problem is that our "in previous iteration of the loop" message prints only if L1 == L2 (not true here)

blitzerr (Sep 13 2018 at 17:18, on Zulip):

I didn't think x was uninitialized. I just thought this is a code snippet and x can be initialized earlier. Also to be clear, drop is a normal function call and not the destructor ?

nikomatsakis (Sep 13 2018 at 17:18, on Zulip):

drop is just a normal function, yes

nikomatsakis (Sep 13 2018 at 17:18, on Zulip):

presumably it was initialized somewhere outside the loop

nikomatsakis (Sep 13 2018 at 17:19, on Zulip):

by drop I meant std::mem::drop

nikomatsakis (Sep 13 2018 at 17:19, on Zulip):

which is just fn drop<T>(x: T) { } :)

nikomatsakis (Sep 13 2018 at 17:19, on Zulip):

so e.g.

let x = vec![];
loop {
  drop(x); // L1
  drop(x); // L2
}
nikomatsakis (Sep 13 2018 at 17:19, on Zulip):

now L1 is reachable via two paths

blitzerr (Sep 13 2018 at 17:19, on Zulip):

So in the new example that you just pasted because L1 and L2 are same, here "in the previous iteration of the loop" will be printed just fine as we want it to, right ?

nikomatsakis (Sep 13 2018 at 17:19, on Zulip):

on one path (the first loop iteration) it is initialized

nikomatsakis (Sep 13 2018 at 17:19, on Zulip):

but on the second one, it is not

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

So in the new example that you just pasted because L1 and L2 are same, here "in the previous iteration of the loop" will be printed just fine as we want it to, right ?

no, it will not

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

because L1 and L2 are not the same point in the program :)

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

if you had

loop {
  drop(x); // L1
}
nikomatsakis (Sep 13 2018 at 17:20, on Zulip):

then it would walk backwards from L1 and find L1 again

blitzerr (Sep 13 2018 at 17:21, on Zulip):

makes sense. Thank you so much.

blitzerr (Sep 13 2018 at 17:23, on Zulip):

One more trivial question and mostly because of ignorance.
In the playground (https://play.rust-lang.org/?gist=1d1523cfeed2f9648f3de855eebb1c00&version=nightly&mode=debug&edition=2015)
I find this line:
let y: Box<isize> = box 42;

Box is widely discussed and there are docs on it but what is "box" the small case ?
box 42 (Is that type casting in Rust) ?

nikomatsakis (Sep 13 2018 at 17:26, on Zulip):

ah

nikomatsakis (Sep 13 2018 at 17:26, on Zulip):

that's older syntax

nikomatsakis (Sep 13 2018 at 17:26, on Zulip):

that is now behind a feature gate

nikomatsakis (Sep 13 2018 at 17:26, on Zulip):

it is effectively synonymous with Box::new(42)

nikomatsakis (Sep 13 2018 at 17:27, on Zulip):

certainly for the purposes of this example, they are the same

nikomatsakis (Sep 13 2018 at 17:27, on Zulip):

the test case dates from when Box::new was built into the language and used only via keyword

nikomatsakis (Sep 13 2018 at 17:27, on Zulip):

we've kept the keyword around but never permitted it in Stable Rust

nikomatsakis (Sep 13 2018 at 17:27, on Zulip):

because it can do some things more efficiently

nikomatsakis (Sep 13 2018 at 17:27, on Zulip):

(and we wanted to maybe add it back later, once we settled on various details)

blitzerr (Sep 13 2018 at 17:28, on Zulip):

cool. Thank s a lot for explaining that.

nikomatsakis (Sep 13 2018 at 17:28, on Zulip):

I would rewrite the test to use Box::new

nikomatsakis (Sep 13 2018 at 17:28, on Zulip):

always better to avoid feature gates in tests if we can

nikomatsakis (Sep 13 2018 at 17:28, on Zulip):

so they don't bit rot if the feature is removed or changed

blitzerr (Sep 13 2018 at 17:28, on Zulip):

Benefits of asking the compiler writer directly, you get to the point answer :P

nikomatsakis (Sep 13 2018 at 17:28, on Zulip):

feature gate being the #![feature(xxx)] syntax

blitzerr (Sep 13 2018 at 17:29, on Zulip):

That I guessed :slight_smile:

blitzerr (Sep 13 2018 at 17:30, on Zulip):

NLL is not available in stable release or is it ?

blitzerr (Sep 13 2018 at 17:31, on Zulip):

I think the feature is developed, right ?

nikomatsakis (Sep 13 2018 at 17:33, on Zulip):

not stable yet

nikomatsakis (Sep 13 2018 at 17:33, on Zulip):

you have to add #![feature(nll)]

blitzerr (Sep 13 2018 at 17:38, on Zulip):

Also. When I make a change in the librustc-mir do I run ./x.py build ?

blitzerr (Sep 13 2018 at 17:39, on Zulip):

It takes several minutes even with the -i flag set

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

you can, but it will be slow, yes

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

I suggest the workflow described in rustc-guide

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

specifically, the "Incremental builds with --keep-stage"

blitzerr (Sep 13 2018 at 18:28, on Zulip):

Thanks.

blitzerr (Sep 13 2018 at 18:29, on Zulip):

I am assuming there is no option of attaching a debugger and stepping through the code as the doc does not mention that in the debugging the compiler section ?

nikomatsakis (Sep 13 2018 at 18:31, on Zulip):

sometimes it works

nikomatsakis (Sep 13 2018 at 18:31, on Zulip):

but i've not had much luck with it

nikomatsakis (Sep 13 2018 at 18:31, on Zulip):

debuginfo doesn't survive optimizations very well

nikomatsakis (Sep 13 2018 at 18:31, on Zulip):

and if you don't optimize, rustc is unbearably slow

blitzerr (Sep 13 2018 at 18:42, on Zulip):

Sounds good. Thanks you

pnkfelix (Sep 13 2018 at 20:09, on Zulip):

On the subject of running rustc under a debugger: I used to do it more regularly, and still do it (in the form of an rr run) from time to time. But its mostly a tool for me to get into the middle of the control flow and analyze how we jump between procedures, not really for inspecting the data structures themselves.

pnkfelix (Sep 13 2018 at 20:10, on Zulip):

My advice for anyone who wants to try it is to use rr, and to be prepared to do things like use stepi instead of step in order to deal with the issues of code reorder by optimizations...

blitzerr (Sep 13 2018 at 20:21, on Zulip):

Thanks a lot @pnkfelix . I will try that.

blitzerr (Sep 13 2018 at 22:50, on Zulip):

@pnkfelix rr looks to only work with linux and not macOs.

pnkfelix (Sep 14 2018 at 08:56, on Zulip):

oh, yeah, I forgot to mention that I have a Linux desktop that is basically dedicated to the purpose of running rr. (Well, that and its a faster box in general than my laptop)

blitzerr (Sep 14 2018 at 16:54, on Zulip):

makes sense. I will have to go with the old school print statements. If rr works then gdb would work, no ?

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

@blitzerr you know about the debug! macros?

blitzerr (Sep 14 2018 at 18:09, on Zulip):

Ya, to print logs for debug builds but are stripped off in the release build. Basically you are saying I can use it for print debugging. Is there more to it that I am not aware of ?

nikomatsakis (Sep 14 2018 at 18:16, on Zulip):

I would advise you to create a config.toml like it describes in the rustc-guide

nikomatsakis (Sep 14 2018 at 18:16, on Zulip):

in particular, one that includes debug-assertions = true

nikomatsakis (Sep 14 2018 at 18:16, on Zulip):

then you can use debug! to get logs

nikomatsakis (Sep 14 2018 at 18:16, on Zulip):

as described here

nikomatsakis (Sep 14 2018 at 18:16, on Zulip):

(and/or add your own)

blitzerr (Sep 15 2018 at 03:23, on Zulip):

Sorry I missed this message as Zuip didn't notify.

blitzerr (Sep 15 2018 at 03:24, on Zulip):

I did make the changes as suggested in the rustc development guide.

blitzerr (Sep 15 2018 at 03:25, on Zulip):

Thanks for the RUST_LOG suggestion. I will use that

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

@blitzerr I'll remember to mention you specifically in the future :)

blitzerr (Sep 15 2018 at 23:16, on Zulip):

:slight_smile:

blitzerr (Sep 15 2018 at 23:16, on Zulip):

RUST_LOG=rustc::traits::error_reporting rustc +stage1 ./src/librustc_mir/borrow_check/error_reporting.rs

blitzerr (Sep 15 2018 at 23:18, on Zulip):
RUST_LOG=rustc::traits::error_reporting rustc +stage1 ./src/librustc_mir/borrow_check/error_reporting.rs

I did that to print the debug statements but it errors out

blitzerr (Sep 17 2018 at 14:22, on Zulip):

@nikomatsakis what would be the correct format to enable debug! Output for the error_reporting.rs module only ?

davidtwco (Sep 17 2018 at 14:23, on Zulip):

RUST_LOG=rustc_mir::borrow_check::error_reporting (or similar with the correct path to that module if I've misremembered)

blitzerr (Sep 17 2018 at 14:29, on Zulip):

Thanks @davidtwco

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

how goes @blitzerr ?

blitzerr (Sep 17 2018 at 17:26, on Zulip):

Hi @nikomatsakis Its going well. I read through the code, made the changes suggested by you in the mentoring notes (and the cosmetic changes to go with it) except for the key piece
'''
if span == move_span { //*is_back_edge {
'''
And I expected the test to still fail and it did but not with an error message I expected
'''
F
failures:

---- [ui] ui/liveness/liveness-move-in-while.rs stdout ----
diff of stderr:

1 error[E0382]: use of moved value: y
- --> $DIR/liveness-move-in-while.rs:17:24
+ --> $DIR/liveness-move-in-while.rs:15:24
3 |
4 LL | println!("{}", y); //~ ERROR use of moved value: y
5 | ^ value used here after move

9 = note: move occurs because y has type std::boxed::Box<isize>, which does not implement the Copy trait
10
11 error[E0382]: use of moved value: y
- --> $DIR/liveness-move-in-while.rs:18:52
+ --> $DIR/liveness-move-in-while.rs:16:52
13 |
14 LL | while true { while true { while true { x = y; x.clone(); } } }
15 | ^ value moved here in previous iteration of loop
'''

blitzerr (Sep 17 2018 at 17:28, on Zulip):
F
failures:

---- [ui] ui/liveness/liveness-move-in-while.rs stdout ----
diff of stderr:

1   error[E0382]: use of moved value: `y`
-     --> $DIR/liveness-move-in-while.rs:17:24
+     --> $DIR/liveness-move-in-while.rs:15:24
3      |
4   LL |         println!("{}", y); //~ ERROR use of moved value: `y`
5      |                        ^ value used here after move

9      = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait
10
11  error[E0382]: use of moved value: `y`
-     --> $DIR/liveness-move-in-while.rs:18:52
+     --> $DIR/liveness-move-in-while.rs:16:52
13     |
14  LL |         while true { while true { while true { x = y; x.clone(); } } }
15     |                                                    ^ value moved here in previous iteration of loop
blitzerr (Sep 17 2018 at 17:30, on Zulip):

So from this error, it looks like the test fails because of two line number mismatches (17 vs 15 and 18 vs 16) and not because of the lack of the text "previous iteration of loop"

nikomatsakis (Sep 17 2018 at 17:31, on Zulip):

what is the test again? also, are you fully rebased?

nikomatsakis (Sep 17 2018 at 17:31, on Zulip):

ok, I see...

nikomatsakis (Sep 17 2018 at 17:32, on Zulip):

@blitzerr so I'm not really sure what your diff is, I guess :)

nikomatsakis (Sep 17 2018 at 17:32, on Zulip):

however, the test has a stderr file that encodes our current output as the expected output

nikomatsakis (Sep 17 2018 at 17:32, on Zulip):

so if you've not made the "real fix" yet (which I think you haven't) then I would expect the test to pass

blitzerr (Sep 17 2018 at 17:32, on Zulip):

The test is ui/liveness/liveness-move-in-while.rs

nikomatsakis (Sep 17 2018 at 17:32, on Zulip):

presumably you edited it slightly, which is why it's failing at all

nikomatsakis (Sep 17 2018 at 17:33, on Zulip):

but once you've made the real fix, it will start to fail for real (because the output changed in a more significant way)

nikomatsakis (Sep 17 2018 at 17:33, on Zulip):

you can run ./x.py test with --bless to update the "expected output" file based on the current output (I always do, personally)

nikomatsakis (Sep 17 2018 at 17:33, on Zulip):

then you can just look at git status to see what changed...

nikomatsakis (Sep 17 2018 at 17:33, on Zulip):

(and look for anything unexpected)

blitzerr (Sep 17 2018 at 17:40, on Zulip):

I understand the cause of the line number change / mismatch in the error message but my question is this:
This change is to add this error message :

value moved here in previous iteration of loop

But it looks like that line already exists. So, I am assuming that I need to enable nll and then this test will fail ?

blitzerr (Sep 17 2018 at 17:41, on Zulip):

Because the whole point of the change is that it was not printing
previous iteration of loop, right @nikomatsakis

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

@blitzerr it only exists on the second error, not the first

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

moreover, there is another change from @davidtwco (which I guess hasn't landed yet) which will suppress the second error as a duplicate

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

https://github.com/rust-lang/rust/pull/53995

blitzerr (Sep 17 2018 at 17:43, on Zulip):

2.

RUST_LOG=rustc_mir::borrow_check::error_reporting rustc +stage1 ./src/librustc_mir/borrow_check/error_reporting.rs

gives me errors like this

error[E0433]: failed to resolve. There are too many initial `super`s.
   --> ./src/librustc_mir/borrow_check/error_reporting.rs:716:5
    |
716 | pub(super) struct IncludingDowncast(bool);
    |     ^^^^^ There are too many initial `super`s.

error[E0433]: failed to resolve. There are too many initial `super`s.
   --> ./src/librustc_mir/borrow_check/error_reporting.rs:993:5
    |
993 | pub(super) enum UseSpans {
    |     ^^^^^ There are too many initial `super`s.
blitzerr (Sep 17 2018 at 17:44, on Zulip):

#2 is a different issue, but I wanted to put it there before I go away from my laptop. :slight_smile:

blitzerr (Sep 17 2018 at 17:44, on Zulip):

back to #1, the test.

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

that's a wacky error. I think the syntax is pub(in super)

davidtwco (Sep 17 2018 at 17:48, on Zulip):

(pretty sure pub(super) is valid)

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

I don't think so

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

but maybe

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

can't recall

blitzerr (Sep 17 2018 at 17:49, on Zulip):
error[E0382]: borrow of moved value: `y`
 --> src/main.rs:8:24
  |
8 |         println!("{}", y); //~ ERROR use of moved value: `y`
  |                        ^ value borrowed here after move
9 |         while true { while true { while true { x = y; x.clone(); } } }
  |                                                    - value moved here
  |
  = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

we would rather add "in previous iteration of loop" to the "value moved here" message:

9 |         while true { while true { while true { x = y; x.clone(); } } }
  |                                                    - value moved here, in previous iteration of loop

That's from the mentoring notes. I thought that we wanted the "in previous iteration of loop" only in the second case ? Did I misinterpret it ?

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

I take it back, a quick ripgrep suggests it is :)

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

this is the full error I get:

error[E0382]: use of moved value: `y`
 --> src/main.rs:7:24
  |
7 |         println!("{}", y); //~ ERROR use of moved value: `y`
  |                        ^ value used here after move
8 |         while true { while true { while true { x = y; x.clone(); } } }
  |                                                    - value moved here
  |
  = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

error[E0382]: use of moved value: `y`
 --> src/main.rs:8:52
  |
8 |         while true { while true { while true { x = y; x.clone(); } } }
  |                                                    ^ value moved here in previous iteration of loop
  |
  = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait
nikomatsakis (Sep 17 2018 at 17:49, on Zulip):

the second error looks right

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

but I think the first error is not

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

I would expect:

nikomatsakis (Sep 17 2018 at 17:50, on Zulip):
error[E0382]: use of moved value: `y`
 --> src/main.rs:7:24
  |
7 |         println!("{}", y); //~ ERROR use of moved value: `y`
  |                        ^ value used here after move
8 |         while true { while true { while true { x = y; x.clone(); } } }
  |                                                    - value moved here in previous iteration of loop
  |
  = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

error[E0382]: use of moved value: `y`
 --> src/main.rs:8:52
  |
8 |         while true { while true { while true { x = y; x.clone(); } } }
  |                                                    ^ value moved here in previous iteration of loop
  |
  = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait
nikomatsakis (Sep 17 2018 at 17:50, on Zulip):

though really I want there to be a comma too, but that's a grammatical nit :P

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

"value moved here, in previous iteration of loop"

blitzerr (Sep 17 2018 at 17:51, on Zulip):

A comma, that's too much :P

nikomatsakis (Sep 17 2018 at 17:53, on Zulip):

(I feel like this is a parenthetical clause, hence the comma)

blitzerr (Sep 17 2018 at 17:59, on Zulip):

I was not debating the introduction of a comma. It was meant to be a joke :smiley:

blitzerr (Sep 17 2018 at 18:28, on Zulip):

@nikomatsakis what is the correct format to enable rust_log for the error_reporting.rs

nikomatsakis (Sep 17 2018 at 18:43, on Zulip):

@blitzerr RUST_LOG=crate::module::module is the general format

nikomatsakis (Sep 17 2018 at 18:43, on Zulip):

you can use a comma separated list

blitzerr (Sep 17 2018 at 18:53, on Zulip):

RUST_LOG=rustc_mir::borrow_check::error_reporting rustc +stage1 ./src/librustc_mir/borrow_check/error_reporting.rs
this should be just

RUST_LOG=rustc_mir::borrow_check::error_reporting rustc +stage1 ./src/librustc_mir/borrow_check/error_reporting.rs
``` ?
blitzerr (Sep 17 2018 at 18:55, on Zulip):

I meant should be just:

RUST_LOG=rustc_mir::borrow_check::error_reporting

?

lqd (Sep 17 2018 at 18:55, on Zulip):

the log looks correct, but you'll be seeing what is logged when you compile your test case, that is, probably liveness-move-in-while.rs ? (rather than compiling ./src/librustc_mir/borrow_check/error_reporting.rs)

davidtwco (Sep 17 2018 at 18:56, on Zulip):

Did you compile with the config.toml set to enable debug assertions and logging? If you're not seeing anything then it could be that.

blitzerr (Sep 17 2018 at 18:56, on Zulip):

So those error messages are normal ?

lqd (Sep 17 2018 at 18:57, on Zulip):

which error messages ?

blitzerr (Sep 17 2018 at 18:57, on Zulip):

@davidtwco I have those settings ON in config.toml

davidtwco (Sep 17 2018 at 18:57, on Zulip):

That depends on the error messages.

blitzerr (Sep 17 2018 at 18:58, on Zulip):

@lqd

error[E0433]: failed to resolve. There are too many initial `super`s.
   --> ./src/librustc_mir/borrow_check/error_reporting.rs:716:5
    |
716 | pub(super) struct IncludingDowncast(bool);
    |     ^^^^^ There are too many initial `super`s.

error[E0433]: failed to resolve. There are too many initial `super`s.
   --> ./src/librustc_mir/borrow_check/error_reporting.rs:993:5
    |
993 | pub(super) enum UseSpans {
    |     ^^^^^ There are too many initial `super`s.
lqd (Sep 17 2018 at 18:58, on Zulip):

@blitzerr why are you compiling this file btw ?

blitzerr (Sep 17 2018 at 18:58, on Zulip):

There are many like those, I just copied some

lqd (Sep 17 2018 at 18:59, on Zulip):

try RUST_LOG=rustc_mir::borrow_check::error_reporting rustc +stage1 ./src/test/ui/liveness/liveness-move-in-while.rs

davidtwco (Sep 17 2018 at 18:59, on Zulip):

Ah, yeah, so the issue is that you're compiling one of the compiler source files - not a test.

blitzerr (Sep 17 2018 at 18:59, on Zulip):

@lqd I am making changes to that file and want to understand the code flow

davidtwco (Sep 17 2018 at 19:00, on Zulip):

The path after rustc should be the file that will be compiled (ie. the test case that you're expecting to make a change to). You can then see the logging from RUST_LOG=rustc_mir::borrow_check::error_reporting (which is the file you want to understand the code flow of) as it compiles the test case.

blitzerr (Sep 17 2018 at 19:00, on Zulip):

@davidtwco That might be it. Thanks a lot

lqd (Sep 17 2018 at 19:00, on Zulip):

are you compiling this file to get a working compiler with your changes ? if so, the command you want is probably one of the ./x.py buids

blitzerr (Sep 17 2018 at 19:02, on Zulip):

I have the compiler built just fine with the new changes (using x.py ...). I wanted to get the debug statements when the test is run

blitzerr (Sep 17 2018 at 19:02, on Zulip):

@lqd ^^

lqd (Sep 17 2018 at 19:02, on Zulip):

then try my previous command, compiling the test file :)

lqd (Sep 17 2018 at 19:03, on Zulip):

or using x.py test with the test args for that test would probably also work

blitzerr (Sep 17 2018 at 19:03, on Zulip):

What you suggested should do the trick for me,
rustc +stage1 ./src/test/ui/liveness/liveness-move-in-while.rs

lqd (Sep 17 2018 at 19:04, on Zulip):

yes, with the RUST_LOG set as you mentioned

blitzerr (Sep 17 2018 at 19:05, on Zulip):

Right. Thanks a lot @lqd @davidtwco

lqd (Sep 17 2018 at 19:05, on Zulip):

let us know if that doesn't work and we'll figure it out

blitzerr (Sep 17 2018 at 19:06, on Zulip):

I will. That's very helpful. I will let you guys know later though as I don't have my laptop on which I build rustc

lqd (Sep 17 2018 at 19:06, on Zulip):

alright :)

blitzerr (Sep 18 2018 at 02:48, on Zulip):
RUST_LOG=rustc_mir::borrow_check::move_errors rustc +stage2 ./src/test/ui/liveness/liveness-move-in-while.rs
error[E0382]: use of moved value: `y`
  --> ./src/test/ui/liveness/liveness-move-in-while.rs:17:24
   |
17 |         println!("{}", y); //~ ERROR use of moved value: `y`
   |                        ^ value used here after move
18 |         while true { while true { while true { x = y; x.clone(); } } }
   |                                                    - value moved here
   |
   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

The compilation fails as expected but no debug statements are printed(no errors are generated either, so this command is correct). Your suggestion was the right one. Thanks

blitzerr (Sep 18 2018 at 02:52, on Zulip):
 RUST_LOG=rustc_mir::borrow_check rustc +stage2 ./src/test/ui/liveness/liveness-move-in-while.rs
DEBUG 2018-09-18T02:48:31Z: rustc_mir::borrow_check::nll::type_check: run_pass: DefId(0/0:3 ~ liveness_move_in_while[317d]::main[0])
DEBUG 2018-09-18T02:48:31Z: rustc_mir::borrow_check::nll::type_check: sanitize_place: _2

This prints the debug messages but only from the module rustc_mir::borrow_check::nll::type_check, rightly so because the debug messages I expect like

report_use_of_moved_or_uninitialized: mois=

Don't feature here.

blitzerr (Sep 18 2018 at 03:01, on Zulip):

@lqd

lqd (Sep 18 2018 at 06:43, on Zulip):

oh that's unfortunate, let me try it when I'm in front of a computer

lqd (Sep 18 2018 at 08:35, on Zulip):

@blitzerr so, what you're seeing there are actually old borrowck errors: this test doesn't enable NLL, and niko manually did so here. You could do a similar thing, adding #![feature(nll)]to the file but let's not: you can turn on NLL with rustc flags, as so: RUST_LOG=rustc_mir::borrow_check::error_reporting rustc +stage2 -Zborrowck=mir -Ztwo-phase-borrows ./src/test/ui/liveness/liveness-move-in-while.rs. Then you'll have approximately 400+ lines of report_use_of_moved_or_uninitialized logs you wanted :)

blitzerr (Sep 18 2018 at 15:39, on Zulip):

@lqd Thank you so much for your time in trying this out yourself. It worked like a charm.

blitzerr (Sep 18 2018 at 15:40, on Zulip):

For the next time in other module how do I know if I have to use what in place of
Zborrowck=mir -Ztwo-phase-borrows ?
I have problems with Rust module imports. They are not file paths as it is in cpp

nikomatsakis (Sep 18 2018 at 15:41, on Zulip):

those are flags to rustc

nikomatsakis (Sep 18 2018 at 15:41, on Zulip):

that tell it to enable NLL

nikomatsakis (Sep 18 2018 at 15:41, on Zulip):

I would just put #![feature(nll)] on the top of the test file, though

nikomatsakis (Sep 18 2018 at 15:41, on Zulip):

easier ;)

nikomatsakis (Sep 18 2018 at 15:41, on Zulip):

then you don't need the flags

nikomatsakis (Sep 18 2018 at 15:41, on Zulip):

those flags are independent from logging, though

davidtwco (Sep 18 2018 at 15:42, on Zulip):

(well, they make the code that you're changing run, so without them you won't see the logging from that code, but they in and of themselves, don't "enable" logging or anything like that)

blitzerr (Sep 18 2018 at 15:43, on Zulip):

That I got.

blitzerr (Sep 18 2018 at 15:44, on Zulip):

That they enable the feature via command line args instead of a code change. Thanks everyone.

nikomatsakis (Sep 18 2018 at 16:38, on Zulip):

@blitzerr PR working? if you're having trouble, feel free to open a PR with [WIP] in the title and send me a link. That's the easiest way for me to give feedback.

blitzerr (Sep 18 2018 at 16:42, on Zulip):

Sure @nikomatsakis I will do that tonight, the first version

blitzerr (Sep 19 2018 at 04:56, on Zulip):

@nikomatsakis https://github.com/rust-lang/rust/pull/54343/files - not ready but posted what I had locally

nikomatsakis (Sep 19 2018 at 17:58, on Zulip):

cool; btw, do you have rustfmt installed?

nikomatsakis (Sep 19 2018 at 17:58, on Zulip):

(I'm trying to keep the new code — like this — formatted with rustfmt)

blitzerr (Sep 19 2018 at 19:06, on Zulip):

Sure, I will install rustfmt and format it when I send the next revision

blitzerr (Sep 19 2018 at 19:08, on Zulip):

@nikomatsakis did you get a chance to take a look. I have a few questions from the rust_log output and basic blocks. Maybe that would explain why the current code doesn't work

nikomatsakis (Sep 19 2018 at 19:08, on Zulip):

I didn't look closely — what is the otuput from current version? (no changes?)

nikomatsakis (Sep 19 2018 at 19:08, on Zulip):

I can look now

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

I see the bug, will leave some comments

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

@blitzerr see my review

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

mostly nits, but the final comment identifies the bug I think

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

let me know if that makes sense

blitzerr (Sep 19 2018 at 19:24, on Zulip):

Just now got the GitHub email :slight_smile:

blitzerr (Sep 19 2018 at 19:30, on Zulip):

I wanted to comment on the review but I am not hip enough to comment on a GitHub review from my phone, so I ask here :grinning:

blitzerr (Sep 19 2018 at 19:30, on Zulip):

Why do we prefer a struct instead of a tuple? @nikomatsakis

nikomatsakis (Sep 19 2018 at 19:31, on Zulip):

I prefer a struct for a few reasons;

nikomatsakis (Sep 19 2018 at 19:31, on Zulip):

it gives a place to add comments, first of all, explaining what is going on

nikomatsakis (Sep 19 2018 at 19:31, on Zulip):

it lets us use named fields like is_back_edge instead of foo.1

nikomatsakis (Sep 19 2018 at 19:31, on Zulip):

and finally it lets us define a name ("MoveSite") that we can use for variables and the like :)

nikomatsakis (Sep 19 2018 at 19:31, on Zulip):

we could of course do that with the tuple too...

nikomatsakis (Sep 19 2018 at 19:31, on Zulip):

basically I just find it more self-documenting

blitzerr (Sep 20 2018 at 01:23, on Zulip):

All that makes sense. And it also comes at no performance penalties ? I guess it would depend on how a tuple is implemented, one can more or less expensive than the other but because we will recursively create an object of all basic blocks, it can have a large cost of compilation in big projects ?
These are just questions out of curiosity, I do agree with the self documenting part with well named members argument about structs.

blitzerr (Sep 20 2018 at 01:23, on Zulip):

@nikomatsakis

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

All that makes sense. And it also comes at no performance penalties ?

tuples and structs are identical at runtime, yes

blitzerr (Sep 20 2018 at 19:20, on Zulip):

Awesome thanks @nikomatsakis

blitzerr (Sep 20 2018 at 19:22, on Zulip):

I have another question. The basic blocks's location is the line number of a statement, right? Does it strips the empty lines. I was trying to change the lines numbers in the debug logging for the test for it wouldn't change.

davidtwco (Sep 20 2018 at 19:41, on Zulip):

The Location that you're talking about is the location in the MIR - which is an intermediate representation of the language. It's made up of basic blocks with statements and then a terminator. If you run the compiler with -Z dump-mir=all then you get a mir_dump folder that shows you the MIR. In those files (there's one for each function/closure) you'll see things like bb0[2] which is basic block 0, statement 2. There aren't any empty lines in the MIR.
https://rust-lang-nursery.github.io/rustc-guide/mir/index.html

blitzerr (Sep 20 2018 at 22:16, on Zulip):

thanks @davidtwco
I will do what you suggested. Usually basic block is for a block of code but are you suggesting that Mir treats single statements as the basic block ?

blitzerr (Sep 20 2018 at 22:17, on Zulip):

Thanks for the link:+1:

davidtwco (Sep 20 2018 at 22:17, on Zulip):

A single statement in a test might end up as multiple statements in the Mir with compiler inserted temporaries.

davidtwco (Sep 20 2018 at 22:18, on Zulip):

But within the parts of the code you're working on, you'll always be working within the Mir of one function or closure, which can have many basic blocks, each with statements and a terminator.

blitzerr (Sep 21 2018 at 02:10, on Zulip):

Thanks a lot @davidtwco I will try this out.

blitzerr (Sep 24 2018 at 17:08, on Zulip):

@nikomatsakis
I did get my question answered

blitzerr (Sep 24 2018 at 17:19, on Zulip):

@nikomatsakis I didn't make much progress this weekend. I am sorry that this seemingly an hour worth of work is taking me over a week to get done.

nikomatsakis (Sep 24 2018 at 18:00, on Zulip):

@blitzerr first PR is the hardest :)

blitzerr (Sep 24 2018 at 18:08, on Zulip):

Tell me about it :grinning:

blitzerr (Sep 24 2018 at 20:56, on Zulip):

@nikomatsakis Also I looked at your comments. It looks more than just the error annotations not updated. My pull just eliminated the second comment for the error, which points to the while loop and where the move happened.

blitzerr (Sep 24 2018 at 20:56, on Zulip):

I added a comment on the GitHub earlier.
I will take a look today

blitzerr (Sep 26 2018 at 17:18, on Zulip):

@nikomatsakis
I added a comment on the PR.

blitzerr (Sep 27 2018 at 15:25, on Zulip):
$ rustc +stage1 ./src/test/ui/liveness/liveness-move-in-while.rs
error[E0382]: use of moved value: `y`
  --> ./src/test/ui/liveness/liveness-move-in-while.rs:17:24
   |
17 |         println!("{}", y); //~ ERROR use of moved value: `y`
   |                        ^ value used here after move
18 |         while true { while true { while true { x = y; x.clone(); } } }
   |                                                    - value moved here
   |
   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

error[E0382]: use of moved value: `y`
  --> ./src/test/ui/liveness/liveness-move-in-while.rs:18:52
   |
18 |         while true { while true { while true { x = y; x.clone(); } } }
   |                                                    ^ value moved here in previous iteration of loop
   |
   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0382`.
blitzerr (Sep 27 2018 at 15:26, on Zulip):

After fetching from upstream and doing a clean build, I see these errors while trying to compile the test file. I remember you saying that there is a PR that removes the second error as a duplicate?

blitzerr (Sep 27 2018 at 15:27, on Zulip):

I see the code in my repo, but the output has it, maybe I misunderstood the intended meaning of duplicate ?

blitzerr (Sep 27 2018 at 15:28, on Zulip):

@nikomatsakis

nikomatsakis (Sep 27 2018 at 15:38, on Zulip):

@blitzerr can you test against the output when you use rustc +nightly?

blitzerr (Sep 27 2018 at 15:40, on Zulip):
$ rustc +nightly ./src/test/ui/liveness/liveness-move-in-while.rs
error[E0382]: use of moved value: `y`
  --> ./src/test/ui/liveness/liveness-move-in-while.rs:17:24
   |
17 |         println!("{}", y); //~ ERROR use of moved value: `y`
   |                        ^ value used here after move
18 |         while true { while true { while true { x = y; x.clone(); } } }
   |                                                    - value moved here
   |
   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

error[E0382]: use of moved value: `y`
  --> ./src/test/ui/liveness/liveness-move-in-while.rs:18:52
   |
18 |         while true { while true { while true { x = y; x.clone(); } } }
   |                                                    ^ value moved here in previous iteration of loop
   |
   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0382`.
nikomatsakis (Sep 27 2018 at 15:40, on Zulip):

e.g., I get

athena. rustc +nightly liveness-move-in-while.rs -Zborrowck=mir -Ztwo-phase-borrows
error[E0382]: borrow of moved value: `y`
  --> liveness-move-in-while.rs:17:24
   |
17 |         println!("{}", y); //~ ERROR use of moved value: `y`
   |                        ^ value borrowed here after move
18 |         while true { while true { while true { x = y; x.clone(); } } }
   |                                                    - value moved here
   |
   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait
nikomatsakis (Sep 27 2018 at 15:40, on Zulip):

are you running with -Zborrowck=mir? Otherwise, unless the test has #![feature(nll)], you are running the older, AST-based borrow checker

blitzerr (Sep 27 2018 at 15:41, on Zulip):

Ahh ! This is what I am beating my head against :slight_smile:

nikomatsakis (Sep 27 2018 at 15:41, on Zulip):

d'oh, sorry for the confusion :)

blitzerr (Sep 27 2018 at 15:42, on Zulip):

blitzerr (Sep 27 2018 at 15:42, on Zulip):
$ rustc +nightly ./src/test/ui/liveness/liveness-move-in-while.rs -Zborrowck=mir -Ztwo-phase-borrows
error[E0382]: borrow of moved value: `y`
  --> ./src/test/ui/liveness/liveness-move-in-while.rs:17:24
   |
17 |         println!("{}", y); //~ ERROR use of moved value: `y`
   |                        ^ value borrowed here after move
18 |         while true { while true { while true { x = y; x.clone(); } } }
   |                                                    - value moved here
   |
   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

error[E0382]: use of moved value: `y`
  --> ./src/test/ui/liveness/liveness-move-in-while.rs:18:52
   |
18 |         while true { while true { while true { x = y; x.clone(); } } }
   |                                                    ^ value moved here in previous iteration of loop
   |
   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0382`.
blitzerr (Sep 27 2018 at 15:43, on Zulip):

Still the same, right ?

blitzerr (Sep 27 2018 at 15:44, on Zulip):

You have a different output than I do ! :slight_smile:

blitzerr (Sep 27 2018 at 15:46, on Zulip):

The only change I have in my repo right now, is the RustFmt-ed file
https://github.com/rust-lang/rust/pull/54343/files

blitzerr (Sep 27 2018 at 16:27, on Zulip):

I did a hard reset and this is what my git log shows :

commit c9865b1c37f8cb8a257591e6ea0b32a5df1f3d41 (HEAD -> master, upstream/master)
Merge: c4501a0f1d 3feffd48bd
Author: bors <bors@rust-lang.org>
Date:   Thu Sep 27 12:35:36 2018 +0000

    Auto merge of #54355 - pnkfelix:issue-22323-regression-test, r=davidtwco

    NLL: regression test for "dropck: track order of destruction for r-value temporaries"

    Once this lands, we can remove the E-needstest from #22323.

    (We shouldn't close the bug itself, however, because we are leaving the NLL-fixed-by-NLL bugs open until NLL is turned on by default.)
blitzerr (Sep 27 2018 at 16:28, on Zulip):

@nikomatsakis
This is the output :

$ rustc +nightly ./src/test/ui/liveness/liveness-move-in-while.rs -Zborrowck=mir -Ztwo-phase-borrows
error[E0382]: borrow of moved value: `y`
  --> ./src/test/ui/liveness/liveness-move-in-while.rs:17:24
   |
17 |         println!("{}", y); //~ ERROR use of moved value: `y`
   |                        ^ value borrowed here after move
18 |         while true { while true { while true { x = y; x.clone(); } } }
   |                                                    - value moved here
   |
   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

error[E0382]: use of moved value: `y`
  --> ./src/test/ui/liveness/liveness-move-in-while.rs:18:52
   |
18 |         while true { while true { while true { x = y; x.clone(); } } }
   |                                                    ^ value moved here in previous iteration of loop
   |
   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0382`.
blitzerr (Sep 27 2018 at 16:42, on Zulip):

blitzerr (Sep 27 2018 at 16:43, on Zulip):

blitzerr (Sep 27 2018 at 16:43, on Zulip):
$ rustc +nightly -vV
rustc 1.28.0-nightly (cbc4c8380 2018-06-22)
binary: rustc
commit-hash: cbc4c8380fb92a719ae9be40f9da44ca7e3e2f3f
commit-date: 2018-06-22
host: x86_64-apple-darwin
release: 1.28.0-nightly
LLVM version: 6.0
blitzerr (Sep 27 2018 at 16:43, on Zulip):
 rustc +stage1 -vV
rustc 1.30.0-dev
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-apple-darwin
release: 1.30.0-dev
LLVM version: 8.0
nikomatsakis (Sep 27 2018 at 17:44, on Zulip):

@blitzerr rustup update nightly, perhaps?

nikomatsakis (Sep 27 2018 at 17:44, on Zulip):

in particular your nightly is super old :)

nikomatsakis (Sep 27 2018 at 17:45, on Zulip):

my nightly is:

athena. rustc +nightly -vV
rustc 1.30.0-nightly (ae7fe84e8 2018-09-26)
binary: rustc
commit-hash: ae7fe84e8cf7523f6853d9786e28e9d066d4e5cd
commit-date: 2018-09-26
host: x86_64-unknown-linux-gnu
release: 1.30.0-nightly
LLVM version: 8.0
nikomatsakis (Sep 27 2018 at 17:45, on Zulip):

what does your branch do if you supply -Zborrowck=mir?

nikomatsakis (Sep 27 2018 at 17:45, on Zulip):

I don't see that above

blitzerr (Sep 27 2018 at 17:48, on Zulip):

I see nightly is a separate build, unrelated to the rust forked branch. Sure, I will update that

blitzerr (Sep 27 2018 at 17:50, on Zulip):

I will get back to you on that flag -Zborrowck=mir?
I think it will work like a charm. :slight_smile:
Don't have my laptop with me right now @nikomatsakis

nikomatsakis (Sep 27 2018 at 18:02, on Zulip):

@blitzerr ok

blitzerr (Sep 28 2018 at 05:25, on Zulip):

https://github.com/rust-lang/rust/pull/54343/commits/a4ccbdbb939b7c3d63fadef24ec9fcda5f789347

@nikomatsakis The PR fixes #54015. Please take a look at you convenience and let me know.

$ rustc +stage1 src/test/ui/liveness/liveness-move-in-while.rs -Zborrowck=mir -Ztwo-phase-borrows
error[E0382]: borrow of moved value: `y`
  --> src/test/ui/liveness/liveness-move-in-while.rs:17:24
   |
17 |         println!("{}", y); //~ ERROR use of moved value: `y`
   |                        ^ value borrowed here after move
18 |         while true { while true { while true { x = y; x.clone(); } } }
   |                                                    - value moved here, in previous iteration of loop
   |
   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait

error: aborting due to previous error

Gives the desired error message now and as you can see, it also has the comma. :D

davidtwco (Sep 28 2018 at 09:10, on Zulip):

Looks great @blitzerr, I added a couple small nits when I was taking a peek.

blitzerr (Sep 28 2018 at 11:14, on Zulip):

Thanks @davidtwco for taking a look. I will address them.

blitzerr (Sep 28 2018 at 11:48, on Zulip):

@davidtwco I updated my PR with your suggestions.

davidtwco (Sep 28 2018 at 12:50, on Zulip):

You'll need to --bless some of the UI tests that made fail @blitzerr.

davidtwco (Sep 28 2018 at 12:50, on Zulip):

Otherwise, the nits seem fixed!

davidtwco (Sep 28 2018 at 12:50, on Zulip):

:tada:

blitzerr (Sep 28 2018 at 12:55, on Zulip):

Currently doing that. :slight_smile:Will update the PR once the UI tests runs fine locally

blitzerr (Sep 28 2018 at 12:55, on Zulip):

Thanks for reviewing @davidtwco

blitzerr (Sep 28 2018 at 14:59, on Zulip):

@nikomatsakis
I saw you mention me on GitHub. Are you not able to access my new PR ?

blitzerr (Sep 28 2018 at 14:59, on Zulip):

What can I do to help you (not that you need any:grinning:)?

nikomatsakis (Sep 28 2018 at 15:05, on Zulip):

I am able to

nikomatsakis (Sep 28 2018 at 15:05, on Zulip):

I just wanted to have a link from the main issue

nikomatsakis (Sep 28 2018 at 15:05, on Zulip):

(normally, in your PR description, you would "cite" the issue, and it would get created automatically)

blitzerr (Sep 28 2018 at 16:17, on Zulip):

I did cite it like this:
[54015] NLL:Improve move error loop detection

blitzerr (Sep 28 2018 at 16:18, on Zulip):

Maybe the square brackets messes up the back-linking :slight_smile:

lqd (Sep 28 2018 at 16:21, on Zulip):

you can cite using github's special syntax: in your PR's description (or any comment) using the issue number like "#54015", it will create a backlink from this 54015 issue to the PR/comment, so it's easy to track using links directly in the issue page (rather than search if we don't)

blitzerr (Sep 28 2018 at 16:27, on Zulip):

I just added the test fixes to make Travis happy and I added the "#" before 54015 and things look fine.
Sorry guys

lqd (Sep 28 2018 at 16:28, on Zulip):

@blitzerr there are no worries whatsoever

blitzerr (Sep 28 2018 at 16:29, on Zulip):

:D

lqd (Sep 28 2018 at 16:29, on Zulip):

@blitzerr thanks for the awesome work

blitzerr (Sep 28 2018 at 16:29, on Zulip):

:big_smile:

blitzerr (Sep 28 2018 at 16:30, on Zulip):

Not quite @lqd , it took me a long time to get the first PR out. It should have been trivial piece of work. :slight_smile:

lqd (Sep 28 2018 at 16:30, on Zulip):

(easy access to the PR: #54343)

lqd (Sep 28 2018 at 16:34, on Zulip):

let's just say that rustc and the whole environment wasn't trivial to me for my first PR @blitzerr :) nicely done in any case :thumbs_up:

blitzerr (Sep 28 2018 at 16:39, on Zulip):

Everyone here I have interacted with is very kind and helpful. It makes it a great experience. Thanks all :clap:

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

@blitzerr still seeing some failures in your PR though — do you know what those are about?

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

I didn't look too closely

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

ah, looks like you need to run x.py test with --bless

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

to update the reference files

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

otherwise, looking great!

blitzerr (Sep 28 2018 at 19:35, on Zulip):

I did run bless on the files for which I got error locally. Will do that again. Thanks for taking a look @nikomatsakis

blitzerr (Sep 29 2018 at 14:19, on Zulip):

https://travis-ci.org/rust-lang/rust/builds/434870102?utm_source=github_status&utm_medium=notification

The build passed.

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

@blitzerr great!

blitzerr (Oct 01 2018 at 19:38, on Zulip):

I think this failed again. How to run compile test for UI suite in "compare_mode=nll"?

Matthew Jasper (Oct 01 2018 at 19:41, on Zulip):

./x.py test src/test/ui --bless (--bless will update test output for you), you'll also want to add -i, --stage 1, etc. to match how you're building.

Matthew Jasper (Oct 01 2018 at 19:42, on Zulip):

That will run in non-compare mode first

Last update: Nov 21 2019 at 13:45UTC