Stream: t-compiler/wg-nll

Topic: #54499 Invalid "variable does not need to be mutable"


Santiago Pastorino (Sep 27 2018 at 16:49, on Zulip):

opening this for discussion

Santiago Pastorino (Sep 27 2018 at 16:49, on Zulip):

@nikomatsakis don't remember if you've said something but it's all the needed information on the issue?

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

need to check a bit the issues you mentioned, #21232 and #53258

Santiago Pastorino (Sep 27 2018 at 16:52, on Zulip):

I see that the code in AST borrowck fails

Santiago Pastorino (Sep 27 2018 at 16:52, on Zulip):
[santiago@archlinux tmp]$ rustc test.rs
error[E0381]: use of possibly uninitialized variable: `s.0`
 --> test.rs:5:23
  |
5 |     println!("{} {}", s.0, s.1);
  |                       ^^^ use of possibly uninitialized `s.0`

error[E0381]: use of possibly uninitialized variable: `s.1`
 --> test.rs:5:28
  |
5 |     println!("{} {}", s.0, s.1);
  |                            ^^^ use of possibly uninitialized `s.1`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0381`.
Santiago Pastorino (Sep 27 2018 at 16:52, on Zulip):

and it gives that warning in NLL

Santiago Pastorino (Sep 27 2018 at 16:52, on Zulip):
[santiago@archlinux tmp]$ rustc test.rs
warning: variable does not need to be mutable
 --> test.rs:4:9
  |
4 |     let mut s: (i32, i32);
  |         ----^
  |         |
  |         help: remove this `mut`
  |
  = note: #[warn(unused_mut)] on by default
Santiago Pastorino (Sep 27 2018 at 16:53, on Zulip):

you also mention that it would be a good idea to fix #21232

Santiago Pastorino (Sep 27 2018 at 16:53, on Zulip):

so what should I do?

Santiago Pastorino (Sep 27 2018 at 16:54, on Zulip):

is the idea to fix #21232?

Santiago Pastorino (Sep 27 2018 at 16:54, on Zulip):

or should we first remove that warning?

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

@Santiago Pastorino let me get back up to speed

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

ok I see

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

well

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

what I meant was "remove the warning"

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

I'm not 100% sure how to do that :)

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

but I can get you in the right direction

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

@Santiago Pastorino left some preliminary notes here, but you'll have to do a bit of digging

Santiago Pastorino (Sep 27 2018 at 18:16, on Zulip):

yeah, no worries

Santiago Pastorino (Sep 27 2018 at 18:17, on Zulip):

I mainly wanted to know what to do

Santiago Pastorino (Sep 27 2018 at 18:17, on Zulip):

but thanks for the tips :)

Santiago Pastorino (Sep 27 2018 at 18:54, on Zulip):

So we probably need to add something to that set when we see a.b = c. That is a MIR assignment, so it winds up invoking mutate_place:

Santiago Pastorino (Sep 27 2018 at 18:54, on Zulip):

I'm not sure exactly what you meant there

Santiago Pastorino (Sep 27 2018 at 18:55, on Zulip):

you meant that I need to fall in that part of the code, line 493?

Santiago Pastorino (Sep 27 2018 at 18:55, on Zulip):

you meant just that mutate_place needs to be called like in 493?

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

I am just tracing through what happens

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

when we borrow check the example

Santiago Pastorino (Sep 27 2018 at 18:56, on Zulip):

you meant that StatementKind::Assign ...

Santiago Pastorino (Sep 27 2018 at 18:56, on Zulip):

no worries I can do it

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

I didn't trace that far, as you can see :)

Santiago Pastorino (Sep 27 2018 at 18:56, on Zulip):

I didn't want to dismiss your message

Santiago Pastorino (Sep 27 2018 at 18:56, on Zulip):

yeah, don't worry

Santiago Pastorino (Sep 27 2018 at 18:57, on Zulip):

was trying to understand that just the used_mut bit is already an important tip

Santiago Pastorino (Sep 27 2018 at 18:57, on Zulip):

I just wasn't sure what was mutate_place for and why did you mentioned it

Santiago Pastorino (Sep 27 2018 at 18:59, on Zulip):

from what I've seen it seems like a.b = c is not an StatementKind::Assign ?

nikomatsakis (Sep 27 2018 at 19:04, on Zulip):

it is

Santiago Pastorino (Sep 27 2018 at 19:04, on Zulip):

was just trying that :)

nikomatsakis (Sep 27 2018 at 19:04, on Zulip):

but yeah used_mut was the key bit

Santiago Pastorino (Sep 27 2018 at 19:06, on Zulip):

so if it is, weren't you saying that it needs to call mutate_place?

Santiago Pastorino (Sep 27 2018 at 19:06, on Zulip):

if it is Assign that should be called

nikomatsakis (Sep 27 2018 at 19:07, on Zulip):

so if it is, weren't you saying that it needs to call mutate_place?

I'm just saying it already will be called

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

at least, I think so

Santiago Pastorino (Sep 27 2018 at 19:08, on Zulip):

:+1:

Santiago Pastorino (Sep 27 2018 at 19:54, on Zulip):

so, in this case, is calling https://github.com/rust-lang/rust/blob/c9865b1c37f8cb8a257591e6ea0b32a5df1f3d41/src/librustc_mir/borrow_check/mod.rs#L1721

Santiago Pastorino (Sep 27 2018 at 19:54, on Zulip):

the root_place being for s in the example

Santiago Pastorino (Sep 27 2018 at 19:55, on Zulip):

the thing is is_local_mutation_allowed is No

Santiago Pastorino (Sep 27 2018 at 19:55, on Zulip):

I guess it should be Yes for this to work?

Santiago Pastorino (Sep 27 2018 at 19:55, on Zulip):

taking a look at what the code does

nikomatsakis (Sep 27 2018 at 20:27, on Zulip):

oh dear I hate that flag

nikomatsakis (Sep 27 2018 at 20:28, on Zulip):

so the annoying thing here

nikomatsakis (Sep 27 2018 at 20:28, on Zulip):

is that the logic of the code as is is correct

nikomatsakis (Sep 27 2018 at 20:28, on Zulip):

but we are a bit inconsistent in some sense

nikomatsakis (Sep 27 2018 at 20:28, on Zulip):

is that the logic of the code as is is correct

that is, the logic of the used_mut code

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

inconsistent how?

Santiago Pastorino (Sep 27 2018 at 20:30, on Zulip):

I was guessing that for some reason in this case that flag was set to No when it should be Yes

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

well, you are not supposed to need mut unless a given path is assigned more than once

Santiago Pastorino (Sep 27 2018 at 20:30, on Zulip):

but unsure if that's right

Santiago Pastorino (Sep 27 2018 at 20:30, on Zulip):

yeah, but in this particular case you want to allow mut regardless of that

nikomatsakis (Sep 27 2018 at 20:31, on Zulip):

hmm

nikomatsakis (Sep 27 2018 at 20:32, on Zulip):

I am looking a bit

nikomatsakis (Sep 27 2018 at 20:32, on Zulip):

I'm actually a bit confused

Santiago Pastorino (Sep 27 2018 at 20:32, on Zulip):

:+1:

Santiago Pastorino (Sep 27 2018 at 20:33, on Zulip):

so if I understood correct from the issue we have two alternatives

nikomatsakis (Sep 27 2018 at 20:33, on Zulip):

well

Santiago Pastorino (Sep 27 2018 at 20:33, on Zulip):

or we do the right thing and implement https://github.com/rust-lang/rust/issues/21232

nikomatsakis (Sep 27 2018 at 20:33, on Zulip):

what debug statements did you add :)

Santiago Pastorino (Sep 27 2018 at 20:33, on Zulip):

or we just remove the warning for that mut

nikomatsakis (Sep 27 2018 at 20:33, on Zulip):

it looks check_access_permissions

nikomatsakis (Sep 27 2018 at 20:34, on Zulip):

is what modifies the set of "used mut" variables

Santiago Pastorino (Sep 27 2018 at 20:34, on Zulip):
[santiago@archlinux rust1 (invalid-no-need-mut)]$ git diff
diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs
index 6ecbc5ee72..2bd4b2d69f 100644
--- a/src/librustc_mir/borrow_check/mod.rs
+++ b/src/librustc_mir/borrow_check/mod.rs
@@ -481,6 +481,10 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx

         self.check_activations(location, span, flow_state);

+        debug!(
+            "MirBorrowckCtxt::process_statement: stmt.kind= {:?}",
+            stmt.kind
+        );
         match stmt.kind {
             StatementKind::Assign(ref lhs, ref rhs) => {
                 self.consume_rvalue(
@@ -850,6 +854,7 @@ enum InitializationRequiringAction {
     Assignment,
 }

+#[derive(Debug)]
 struct RootPlace<'d, 'tcx: 'd> {
     place: &'d Place<'tcx>,
     is_local_mutation_allowed: LocalMutationIsAllowed,
@@ -1716,8 +1721,16 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 }
             }
             Reservation(WriteKind::Mutate) | Write(WriteKind::Mutate) => {
+                debug!(
+                    "check_access_permissions: {:?}, {:?}",
+                    place, is_local_mutation_allowed
+                );
                 match self.is_mutable(place, is_local_mutation_allowed) {
                     Ok(root_place) => {
+                        debug!(
+                            "check_access_permissions: {:?}",
+                            root_place
+                        );
                         self.add_used_mut(root_place, flow_state);
                         return false;
                     }
nikomatsakis (Sep 27 2018 at 20:34, on Zulip):

ok, what was the output from those? :)

nikomatsakis (Sep 27 2018 at 20:34, on Zulip):

(and on what test?)

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

the test is the one on the issue

Santiago Pastorino (Sep 27 2018 at 20:35, on Zulip):
#![feature(nll)]

fn main() {
    let mut s: (i32, i32);
    s.0 = 3;
    s.1 = 4;
    println!("{} {}", s.0, s.1);
}
nikomatsakis (Sep 27 2018 at 20:35, on Zulip):

ok

Santiago Pastorino (Sep 27 2018 at 20:35, on Zulip):
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::universal_regions: build: global regions = 0..1
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::universal_regions: build: extern regions = 1..1
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::universal_regions: build: local regions  = 1..2
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_mir()
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_mir: mir.arg_count=0
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=i32, ty_context=Location(bb0[1]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=i32)
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=i32
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=i32, ty_context=Location(bb0[1]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=i32)
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=i32
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=Const { ty: i32, val: Scalar(Bits { size: 4, bits: 3 }) })
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=i32, ty_context=Location(bb0[2]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=i32)
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=i32
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=i32, ty_context=Location(bb0[2]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=i32)
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=i32
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=Const { ty: i32, val: Scalar(Bits { size: 4, bits: 4 }) })
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region(region=ReScope(Node(ItemLocalId(88))), location=bb0[7])
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=ReScope(Node(ItemLocalId(88))))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region: region='_#2r
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=[&str; 3], ty_context=Location(bb0[7]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=[&str; 3])
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=[&str; 3]
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region(region=ReScope(Node(ItemLocalId(88))), location=bb0[8])
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=ReScope(Node(ItemLocalId(88))))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region: region='_#4r
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=&[&str], ty_context=Location(bb0[9]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=&[&str])
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=&[&str]
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region(region=ReScope(Node(ItemLocalId(88))), location=bb0[17])
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=ReScope(Node(ItemLocalId(88))))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region: region='_#7r
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=i32, ty_context=Location(bb0[17]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=i32)
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=i32
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region(region=ReScope(Node(ItemLocalId(88))), location=bb0[19])
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=ReScope(Node(ItemLocalId(88))))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region: region='_#8r
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=i32, ty_context=Location(bb0[19]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=i32)
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=i32
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=&i32, ty_context=Location(bb3[1]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=&i32)
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=&i32
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=&i32, ty_context=Location(bb3[3]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=&i32)
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=&i32
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region(region=ReScope(Node(ItemLocalId(88))), location=bb3[6])
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=ReScope(Node(ItemLocalId(88))))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region: region='_#11r
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error> {<i32 as std::fmt::Display>::fmt}, ty_context=Location(bb3[8]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error> {<i32 as std::fmt::Display>::fmt})
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error> {<i32 as std::fmt::Display>::fmt}
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=Const { ty: for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error> {<i32 as std::fmt::Display>::fmt}, val: Scalar(Bits { size: 0, bits: 0 }) })
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>, ty_context=Location(bb3[8]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>)
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=for<'b> fn(&'b i32, for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>) -> std::fmt::ArgumentV1<'b> {std::fmt::ArgumentV1<'_>::new::<i32>}, ty_context=Location(bb3[9]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=for<'b> fn(&'b i32, for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>) -> std::fmt::ArgumentV1<'b> {std::fmt::ArgumentV1<'_>::new::<i32>})
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=for<'b> fn(&'b i32, for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>) -> std::fmt::ArgumentV1<'b> {std::fmt::ArgumentV1<'_>::new::<i32>}
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=Const { ty: for<'b> fn(&'b i32, for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>) -> std::fmt::ArgumentV1<'b> {std::fmt::ArgumentV1<'_>::new::<i32>}, val: Scalar(Bits { size: 0, bits: 0 }) })
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region(region=ReScope(Node(ItemLocalId(88))), location=bb4[4])
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=ReScope(Node(ItemLocalId(88))))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_region: region='_#14r
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error> {<i32 as std::fmt::Display>::fmt}, ty_context=Location(bb4[6]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error> {<i32 as std::fmt::Display>::fmt})
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty: ty=for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error> {<i32 as std::fmt::Display>::fmt}
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: renumber_regions(value=Const { ty: for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error> {<i32 as std::fmt::Display>::fmt}, val: Scalar(Bits { size: 0, bits: 0 }) })
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::renumber: visit_ty(ty=for<'r, 's, 't0> fn(&'r i32, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>, ty_context=Location(bb4[6]))
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check::nll::r
nikomatsakis (Sep 27 2018 at 20:35, on Zulip):

maybe make a gist?

nikomatsakis (Sep 27 2018 at 20:35, on Zulip):

I don't think Zulip likes so much text :)

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

https://gist.github.com/spastorino/07a71bb46c425fd3e0d0c3bfc4b8edd0

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

I was exactly looking at those add_used_mut calls

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

my guess anyway was that root_access contains an is_local_mutation_allowed property with value No

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

and I guess it should be Yes

nikomatsakis (Sep 27 2018 at 20:37, on Zulip):

I don't think it should be YEs

nikomatsakis (Sep 27 2018 at 20:37, on Zulip):

that would allow you to mutate things that are not declared mut

nikomatsakis (Sep 27 2018 at 20:37, on Zulip):

for .. various annoying reasons .. we sometimes want to permit that

nikomatsakis (Sep 27 2018 at 20:37, on Zulip):

e.g., if you are executing the Drop at the end of scope

nikomatsakis (Sep 27 2018 at 20:37, on Zulip):

that is technically mutation

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

but not in this case

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

but don't you want to allow mutating s?

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

so... here is the thing that is confusing me:

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

if I udnerstand this log output

nikomatsakis (Sep 27 2018 at 20:39, on Zulip):
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check: check_access_permissions((_1.0: i32), Write(Mutate), No)
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check: check_access_permissions: (_1.0: i32), No
DEBUG 2018-09-27T19:48:01Z: rustc_mir::borrow_check: check_access_permissions: RootPlace { place: _1, is_local_mutation_allowed: No }
nikomatsakis (Sep 27 2018 at 20:39, on Zulip):

we are adding RootPlace { place: _1, is_local_mutation_allowed: No } into the used_mut set

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

so I think that means that somewhere — even though it's in the set — we are issuing the warning anyway

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

yep

Santiago Pastorino (Sep 27 2018 at 20:40, on Zulip):

I guessed it was because of that flag

Santiago Pastorino (Sep 27 2018 at 20:40, on Zulip):

that was all my guessing

Santiago Pastorino (Sep 27 2018 at 20:40, on Zulip):

but I'm not sure

Santiago Pastorino (Sep 27 2018 at 20:40, on Zulip):

because of https://github.com/rust-lang/rust/blob/c9865b1c37f8cb8a257591e6ea0b32a5df1f3d41/src/librustc_mir/borrow_check/mod.rs#L1810

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

sorry, changed the line number

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

if is_local_mutation_allowed != LocalMutationIsAllowed::Yes {

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

ah wait

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

ok, yes, that's the code I was confused about

nikomatsakis (Sep 27 2018 at 20:42, on Zulip):

I ... suspect we can just remove that logic

nikomatsakis (Sep 27 2018 at 20:42, on Zulip):

and just always invoke self.used_muts.insert(..)

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

I was trying to change that and make this code run to see what happens

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

exactly

nikomatsakis (Sep 27 2018 at 20:42, on Zulip):

that logic used to be needed

nikomatsakis (Sep 27 2018 at 20:43, on Zulip):

and will be needed again if we properly fix #21232 :)

Santiago Pastorino (Sep 27 2018 at 20:43, on Zulip):

by remove that logic you meant remove the if?

nikomatsakis (Sep 27 2018 at 20:43, on Zulip):

yeah, I mean replace this:

                if is_local_mutation_allowed != LocalMutationIsAllowed::Yes {
                    // If the local may be initialized, and it is now currently being
                    // mutated, then it is justified to be annotated with the `mut`
                    // keyword, since the mutation may be a possible reassignment.
                    let mpi = self.move_data.rev_lookup.find_local(*local);
                    let ii = &self.move_data.init_path_map[mpi];
                    for &index in ii {
                        if flow_state.ever_inits.contains(index) {
                            self.used_mut.insert(*local);
                            break;
                        }
                    }
                }

with

                            self.used_mut.insert(*local);
nikomatsakis (Sep 27 2018 at 20:44, on Zulip):

basically I think the case this thing is looking for — the first initialization of non-mut thing — is handled separately now

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

ok

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

(do you see what I mean?)

Santiago Pastorino (Sep 27 2018 at 20:46, on Zulip):

yes

Santiago Pastorino (Sep 27 2018 at 20:46, on Zulip):

I mean, no idea where is handled now but yes

Santiago Pastorino (Sep 27 2018 at 20:46, on Zulip):

I see

nikomatsakis (Sep 27 2018 at 20:50, on Zulip):

basically we special case that code out:

        // Special case: you can assign a immutable local variable
        // (e.g., `x = ...`) so long as it has never been initialized
        // before (at this point in the flow).
        if let &Place::Local(local) = place_span.0 {
            if let Mutability::Not = self.mir.local_decls[local].mutability {
                // check for reassignments to immutable local variables
                self.check_if_reassignment_to_immutable_state(
                    context,
                    local,
                    place_span,
                    flow_state,
                );
                return;
            }
        }

        // Otherwise, use the normal access permission rules.
        self.access_place(
            context,
            place_span,
            (kind, Write(WriteKind::Mutate)),
            LocalMutationIsAllowed::No,
            flow_state,
        );
nikomatsakis (Sep 27 2018 at 20:50, on Zulip):

we never even invoke check_access_permission in that case

Santiago Pastorino (Sep 27 2018 at 21:03, on Zulip):

:+1:

Santiago Pastorino (Sep 27 2018 at 21:03, on Zulip):

makes sense

Santiago Pastorino (Sep 27 2018 at 21:04, on Zulip):

it works

Santiago Pastorino (Sep 27 2018 at 21:11, on Zulip):

@nikomatsakis https://github.com/rust-lang/rust/pull/54621

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

after that should we continue with https://github.com/rust-lang/rust/issues/21232 ?

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

unsure how hard it is :)

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

seems reasonable to continue with

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

not the highest priority thing ever but it'd be a nice bit of Rust to cleanup

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

left a few nits

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

the field was only useful for that?

nikomatsakis (Sep 27 2018 at 21:24, on Zulip):

I think so

nikomatsakis (Sep 27 2018 at 21:24, on Zulip):

but you can check :)

Santiago Pastorino (Sep 27 2018 at 21:24, on Zulip):

will do

Santiago Pastorino (Sep 27 2018 at 21:24, on Zulip):

need to leave now

Santiago Pastorino (Sep 27 2018 at 21:25, on Zulip):

anyway I want to see ci running :)

Santiago Pastorino (Sep 28 2018 at 13:16, on Zulip):

@nikomatsakis have you seen ci errors?

nikomatsakis (Sep 28 2018 at 14:32, on Zulip):

@Santiago Pastorino I have not

nikomatsakis (Sep 28 2018 at 14:33, on Zulip):

though it looks like this is affecting mo..oh

nikomatsakis (Sep 28 2018 at 14:33, on Zulip):

I guess I was a bit aggressive

nikomatsakis (Sep 28 2018 at 14:46, on Zulip):

I guess we need some logic like that

nikomatsakis (Sep 28 2018 at 14:46, on Zulip):

just not that logic :P

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

@Santiago Pastorino so do you have some idea what is needed to fix or do you need more tips?

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

left a review on the PR with some tips

Santiago Pastorino (Sep 28 2018 at 21:09, on Zulip):

@nikomatsakis sorry, couldn’t answer before

Santiago Pastorino (Sep 28 2018 at 21:09, on Zulip):

I guess I can figure out

Santiago Pastorino (Sep 28 2018 at 21:09, on Zulip):

it’s just that I’m on vacations :grinning_face_with_smiling_eyes:

Santiago Pastorino (Sep 28 2018 at 22:07, on Zulip):

@nikomatsakis I see what you mean here https://github.com/rust-lang/rust/pull/54621#discussion_r221356961

Santiago Pastorino (Sep 28 2018 at 22:09, on Zulip):

need to find out how to go from that place to the other one

Santiago Pastorino (Sep 28 2018 at 22:09, on Zulip):

actually in a.0 RootPlace is a, right?

Santiago Pastorino (Sep 28 2018 at 22:37, on Zulip):

I guess I need to find a Projection or something, I don't remember

Santiago Pastorino (Sep 28 2018 at 22:37, on Zulip):

need to find that out

Santiago Pastorino (Sep 28 2018 at 22:37, on Zulip):

but need to leave now :)

Santiago Pastorino (Sep 28 2018 at 23:46, on Zulip):

had some spare minutes again

Santiago Pastorino (Sep 28 2018 at 23:47, on Zulip):

I was wondering if I need to do it here

Santiago Pastorino (Sep 28 2018 at 23:47, on Zulip):
            RootPlace {
                place: place @ Place::Projection(_),
                is_local_mutation_allowed: _,
            } => {
Santiago Pastorino (Sep 28 2018 at 23:47, on Zulip):

match the projection, match the field and insert that into used_mut or something

Santiago Pastorino (Sep 28 2018 at 23:50, on Zulip):

was wondering if this https://github.com/rust-lang/rust/blob/c9865b1c37f8cb8a257591e6ea0b32a5df1f3d41/src/librustc_mir/borrow_check/mod.rs#L1828-L1835 wasn't related

nikomatsakis (Oct 02 2018 at 19:55, on Zulip):

ok raising this topic up again @Santiago Pastorino

nikomatsakis (Oct 02 2018 at 20:00, on Zulip):

was wondering if this https://github.com/rust-lang/rust/blob/c9865b1c37f8cb8a257591e6ea0b32a5df1f3d41/src/librustc_mir/borrow_check/mod.rs#L1828-L1835 wasn't related

I don't think that's terribly related. That occurs when you have an assignment in a closure, and it kind of propagates that assignment back out to the parent

nikomatsakis (Oct 02 2018 at 20:01, on Zulip):

if you look at the callees of add_used_mut though, they do have the original place available (example)

nikomatsakis (Oct 02 2018 at 20:01, on Zulip):

I think the idea would be something like this:

nikomatsakis (Oct 02 2018 at 20:02, on Zulip):
nikomatsakis (Oct 02 2018 at 20:02, on Zulip):
Santiago Pastorino (Oct 03 2018 at 19:02, on Zulip):

@nikomatsakis I have quickly pushed something to https://github.com/rust-lang/rust/pull/54621

Santiago Pastorino (Oct 03 2018 at 19:02, on Zulip):

I'm waiting for local tests but I already see some failures

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

anyway, wanted to check if this was more or less what you wanted

nikomatsakis (Oct 03 2018 at 19:30, on Zulip):

@Santiago Pastorino looks about right, yes, left a request for a comment

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

I saw the failures were related to --keep-stage 1

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

will try again

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

I still need to address 2 comments :)

nikomatsakis (Oct 03 2018 at 19:41, on Zulip):

ok

Santiago Pastorino (Oct 03 2018 at 19:55, on Zulip):
// We need the local be defined as mut until we fix #21232
// FIXME(#21232)
Santiago Pastorino (Oct 03 2018 at 19:55, on Zulip):

@nikomatsakis does this works?

Santiago Pastorino (Oct 03 2018 at 19:55, on Zulip):

or

Santiago Pastorino (Oct 03 2018 at 19:56, on Zulip):
// FIXME(#21232): We need the local be defined as mut until we fix this issue
nikomatsakis (Oct 03 2018 at 20:05, on Zulip):

@Santiago Pastorino I'd probably say:


FIXME(#21232): For the time being, you can only have an assignment to a path like a.b.c = x if a is declared as mut, even when a has not been initialized before. So if we see an assignment to such a path, we can always mark the mut on a as being used.

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

@nikomatsakis https://travis-ci.org/rust-lang/rust/jobs/436807954

nikomatsakis (Oct 03 2018 at 20:56, on Zulip):

hmm.

nikomatsakis (Oct 03 2018 at 20:57, on Zulip):

seems like the test is wrong somehow... oh, I think maybe I know..?

nikomatsakis (Oct 03 2018 at 20:57, on Zulip):

well, not entirely sure, but I guess I would dump out the projections etc involved

Santiago Pastorino (Oct 03 2018 at 20:59, on Zulip):

:+1:

Santiago Pastorino (Oct 03 2018 at 20:59, on Zulip):

as soon as I can compile this :S

Santiago Pastorino (Oct 03 2018 at 20:59, on Zulip):

it's failing to compile rustc

Santiago Pastorino (Oct 03 2018 at 20:59, on Zulip):

I guess are memory issues

Santiago Pastorino (Oct 04 2018 at 16:36, on Zulip):

@nikomatsakis back with this

Santiago Pastorino (Oct 04 2018 at 16:37, on Zulip):

I've built a sample repro example

Santiago Pastorino (Oct 04 2018 at 16:37, on Zulip):

and I'm seeing that add_used_mut is not even called :S

Santiago Pastorino (Oct 04 2018 at 16:37, on Zulip):
#![feature(nll)]

fn main() {
    let mut v : &mut Vec<()> = &mut vec![]; //[lexical]~ ERROR: variable does not need to be mutable
                                            //[nll]~^ ERROR: variable does not need to be mutable
    v.push(());
}
Santiago Pastorino (Oct 04 2018 at 16:38, on Zulip):

investigating

nikomatsakis (Oct 04 2018 at 16:39, on Zulip):

hmm

nikomatsakis (Oct 04 2018 at 16:39, on Zulip):

that seems right for it not to be called :)

nikomatsakis (Oct 04 2018 at 16:39, on Zulip):

I guess the question then is why/how v gets added tho

Santiago Pastorino (Oct 04 2018 at 16:40, on Zulip):

added where?

nikomatsakis (Oct 04 2018 at 16:40, on Zulip):

to the set of muts that are used

nikomatsakis (Oct 04 2018 at 16:40, on Zulip):

i.e., the problem is that we are no longer getting the lint, right?

nikomatsakis (Oct 04 2018 at 16:40, on Zulip):

that means we consider the mut on v to be used

Santiago Pastorino (Oct 04 2018 at 16:41, on Zulip):

yes

Santiago Pastorino (Oct 04 2018 at 16:41, on Zulip):

let me investigate a bit more

Santiago Pastorino (Oct 04 2018 at 16:42, on Zulip):

https://github.com/rust-lang/rust/blob/30101f912b121e5f4185081b616f9b910031fe60/src/librustc_mir/borrow_check/used_muts.rs#L58-L60

Santiago Pastorino (Oct 04 2018 at 16:42, on Zulip):

probably

Santiago Pastorino (Oct 04 2018 at 16:46, on Zulip):

actually, why is it changing used_mut in the place I was modofying the code and also there?

Santiago Pastorino (Oct 04 2018 at 16:47, on Zulip):

I guess it should happen just in one place, right?

nikomatsakis (Oct 04 2018 at 16:47, on Zulip):

that link doesn't work for me

Santiago Pastorino (Oct 04 2018 at 16:47, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/used_muts.rs#L58-L60

Santiago Pastorino (Oct 04 2018 at 16:47, on Zulip):

sorry vim took my local commit :)

nikomatsakis (Oct 04 2018 at 16:48, on Zulip):

does look suspicious

nikomatsakis (Oct 04 2018 at 16:48, on Zulip):

that code...does get called

Santiago Pastorino (Oct 04 2018 at 16:49, on Zulip):

let me remove it and test for this case :)

nikomatsakis (Oct 04 2018 at 16:49, on Zulip):

I was going to say, try removing it and see what happens...

nikomatsakis (Oct 04 2018 at 16:49, on Zulip):

very weird

Santiago Pastorino (Oct 04 2018 at 16:49, on Zulip):

yeah first step test this case

Santiago Pastorino (Oct 04 2018 at 16:49, on Zulip):

second run the whole thing

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

third hopefully see everything working :D

Santiago Pastorino (Oct 04 2018 at 16:55, on Zulip):

@nikomatsakis removing that doesn't make this return any warning either

Santiago Pastorino (Oct 04 2018 at 16:55, on Zulip):

:S

Santiago Pastorino (Oct 04 2018 at 16:56, on Zulip):

seems impossible

Santiago Pastorino (Oct 04 2018 at 16:56, on Zulip):
[santiago@archlinux rust1 (invalid-no-need-mut)]$ rg used_mut | rg insert | rg mir
src/librustc_mir/borrow_check/mod.rs:                                    self.used_mut.insert(local);
src/librustc_mir/borrow_check/mod.rs:                        self.used_mut.insert(*local);
src/librustc_mir/borrow_check/mod.rs:                                self.used_mut.insert(*local);
Santiago Pastorino (Oct 04 2018 at 16:57, on Zulip):

two cases are under a debug flag and is not printed

Santiago Pastorino (Oct 04 2018 at 16:57, on Zulip):

and the other case doesn't have a debug flag but is under Rvalue::Aggregate

Santiago Pastorino (Oct 04 2018 at 16:57, on Zulip):

so shouldn't happen either

nikomatsakis (Oct 04 2018 at 16:58, on Zulip):

:shrug:

nikomatsakis (Oct 04 2018 at 16:58, on Zulip):

add some more debug! I guess :)

Santiago Pastorino (Oct 04 2018 at 16:59, on Zulip):

checking if there are other ways of inserting

Santiago Pastorino (Oct 04 2018 at 18:05, on Zulip):
    /// This field keeps track of all the local variables that are declared mut and are mutated.
    /// Used for the warning issued by an unused mutable local variable.
    used_mut: FxHashSet<Local>,
Santiago Pastorino (Oct 04 2018 at 18:05, on Zulip):

is it the way you say or the other way around?

Santiago Pastorino (Oct 04 2018 at 18:06, on Zulip):

I mean, what's the meaning of used_mut actually?

Santiago Pastorino (Oct 04 2018 at 18:06, on Zulip):

having second thoughts about this

nikomatsakis (Oct 04 2018 at 18:11, on Zulip):

I believe it means:

nikomatsakis (Oct 04 2018 at 18:11, on Zulip):

this local variable must be declared mut or the program would not borrow check

nikomatsakis (Oct 04 2018 at 18:12, on Zulip):

put another way, the mut declaration on this variable was "used"

Santiago Pastorino (Oct 04 2018 at 18:36, on Zulip):

I understand

Santiago Pastorino (Oct 04 2018 at 18:36, on Zulip):

weird

Santiago Pastorino (Oct 04 2018 at 18:37, on Zulip):

I can't find of any insertion into there

Santiago Pastorino (Oct 04 2018 at 18:37, on Zulip):

what I can do is just look for where the error messages are being displayed and output the context that is there

nikomatsakis (Oct 04 2018 at 18:38, on Zulip):

sigh

nikomatsakis (Oct 04 2018 at 18:38, on Zulip):

makes sense

nikomatsakis (Oct 04 2018 at 18:38, on Zulip):

at this point I would probably just add debug! all over the dang compiler :)

Santiago Pastorino (Oct 04 2018 at 18:40, on Zulip):

hehehe

Santiago Pastorino (Oct 04 2018 at 18:41, on Zulip):

couldn't be a part of AST borrowck the one adding to this structure?

nikomatsakis (Oct 04 2018 at 18:48, on Zulip):

not that I know of

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

@Santiago Pastorino so... @pnkfelix and I raised this question with the lang team, and there were a few folks in favor of just ruling out this case altogether as an error (at least until #21232 is properly fixed):

let mut x: (u32, u32);
x.0 = 1;

If we did that, then of course the interaction with the "unused mut" lint becomes a moot point... maybe you want to prep a PR to do that, and we can assess the impact? (I wouldn't expect much, given that you get an error if you try to read x.0 again.)

Santiago Pastorino (Oct 04 2018 at 20:24, on Zulip):

ok

Santiago Pastorino (Oct 04 2018 at 20:24, on Zulip):

so

Santiago Pastorino (Oct 04 2018 at 20:24, on Zulip):

you meant that in this example https://github.com/rust-lang/rust/issues/54499

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

the original issue was that the warning was suggesting the wrong thing

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

you meant to stop what I'm doing and convert the warning in an error?

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

or did I get it wrong?

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

that's what I was suggesting, yes

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

that maybe before we invest too much energy into preserving the strange AST behavior

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

we should consider just making that example an error

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

although the fact that the issue got filed

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

suggests that somebody is relying on the current behavior...

Santiago Pastorino (Oct 04 2018 at 20:47, on Zulip):

actually I didn't tell you because I was doing something else already

Santiago Pastorino (Oct 04 2018 at 20:47, on Zulip):

but I was getting some logs

Santiago Pastorino (Oct 04 2018 at 20:48, on Zulip):

there was a problem in the command I was running and I didn't see

Santiago Pastorino (Oct 04 2018 at 20:48, on Zulip):

but yes there's some output :)

Santiago Pastorino (Oct 04 2018 at 20:48, on Zulip):

so, tell me if you prefer me to stop and forget about this and make an error or just match AST, which I think we should be close to do

Santiago Pastorino (Oct 04 2018 at 20:49, on Zulip):

or wait until we see what the reporter replies to https://github.com/rust-lang/rust/issues/54499#issuecomment-427160299

Santiago Pastorino (Oct 04 2018 at 20:49, on Zulip):

I'm fine with anything :)

Santiago Pastorino (Oct 05 2018 at 15:54, on Zulip):

@nikomatsakis remember to let me know what to do about this :)

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

@Santiago Pastorino well, is @pnkfelix around?

nikomatsakis (Oct 05 2018 at 16:06, on Zulip):

maybe we can all 3 discuss

nikomatsakis (Oct 05 2018 at 16:06, on Zulip):

or did they leave for the day

nikomatsakis (Oct 05 2018 at 16:06, on Zulip):

/me grumbles about lazy europeans and their 9 to 10 hour days

Santiago Pastorino (Oct 05 2018 at 16:09, on Zulip):

:P

nikomatsakis (Oct 05 2018 at 16:09, on Zulip):

OK well I see three options:

I think that option 3 doesn't make sense in the short term; we'd be better off landing one of the first two and then revisiting the semantics at our leisure.

Option 2 is a "breaking change", in the sense that the old AST borrow checker accepted such code. However, it was fairly useless -- even if a is declared mut, you could assign to such a path, but you couldn't read back from it. This is confusing at best. It is unlikely that a significant body of code is relying on this, and we could just call it an NLL bug fix — initially, anyway, you'll just get warnings, since any new errors that result from NLL (but not the AST checker) are always displayed as warnings during the migration period.

Option 1 is what we had been pursuing, but it seems kind of like an annoying amount of work to reach a "not particularly consistent" state.

nikomatsakis (Oct 05 2018 at 16:09, on Zulip):

I am inclined towards Option 2 right now

Santiago Pastorino (Oct 05 2018 at 16:11, on Zulip):

ok

Santiago Pastorino (Oct 05 2018 at 16:11, on Zulip):

let's make that an error

nikomatsakis (Oct 05 2018 at 16:11, on Zulip):

we can also do a crater run to see what happens, though that's a bit .. tricky

nikomatsakis (Oct 05 2018 at 16:12, on Zulip):

because we'd have to do it with NLL enabled, which means we'll see other errors too

nikomatsakis (Oct 05 2018 at 16:12, on Zulip):

so I am kind of inclined not to bother, personally

nikomatsakis (Oct 05 2018 at 16:12, on Zulip):

esp. given the migration mode

Santiago Pastorino (Oct 05 2018 at 16:13, on Zulip):

:+1:

pnkfelix (Oct 05 2018 at 16:17, on Zulip):

Yeah sorry I’m done for the day for now

pnkfelix (Oct 05 2018 at 16:17, on Zulip):

Going to a water park soonish

pnkfelix (Oct 05 2018 at 16:17, on Zulip):

Maybe will work more after I get back from waterskiding

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

@nikomatsakis have you seen https://github.com/rust-lang/rust/issues/54499#issuecomment-427406207 ?

Santiago Pastorino (Oct 08 2018 at 19:03, on Zulip):

there's no real use case

Santiago Pastorino (Oct 08 2018 at 19:03, on Zulip):

anyway, I'm going to make this fail

Santiago Pastorino (Oct 08 2018 at 19:04, on Zulip):

I wonder if we should lay out the code in the way we were doing and handle that case when root_place is local and accessed_place is projection or we want to use the old version of the code?

Santiago Pastorino (Oct 08 2018 at 19:07, on Zulip):

also what's exactly ever_inits in Flows?

Santiago Pastorino (Oct 08 2018 at 19:07, on Zulip):

I guess I need to check for uninits?

Santiago Pastorino (Oct 08 2018 at 19:07, on Zulip):

in order to fail

Santiago Pastorino (Oct 08 2018 at 19:14, on Zulip):

hmmm maybe the best thing is to check here if we are in the case we should warn or fail https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/mod.rs#L324-L337

Santiago Pastorino (Oct 08 2018 at 19:18, on Zulip):

hmm I guess at that point I have no idea what was accessed anymore

Santiago Pastorino (Oct 08 2018 at 19:18, on Zulip):

investigating, @nikomatsakis if you have any pointer would be appreciated :)

Santiago Pastorino (Oct 08 2018 at 19:28, on Zulip):

I'm thinking about something like ...

Santiago Pastorino (Oct 08 2018 at 19:28, on Zulip):
                    Ok(root_place) => {
                        if let Place::Projection(_) = root_place {
                            if let Place::Local(local) = place {
                                let mpi = self.move_data.rev_lookup.find_local(*local);
                                if flow_state.uninits.contains(mpi) {
                                    // report error
                                    return true;
                                }
                            }
                        }

                        self.add_used_mut(place, root_place, flow_state);
                        return false;
                    }
nikomatsakis (Oct 08 2018 at 19:39, on Zulip):

@Santiago Pastorino where would this diff go?

Santiago Pastorino (Oct 08 2018 at 19:44, on Zulip):

where we are calling add_used_mut

nikomatsakis (Oct 08 2018 at 19:45, on Zulip):

I would move this logic earlier

nikomatsakis (Oct 08 2018 at 19:46, on Zulip):

maybe somewhere around here

nikomatsakis (Oct 08 2018 at 19:46, on Zulip):

basically in place of this logic

nikomatsakis (Oct 08 2018 at 19:46, on Zulip):

hmm

nikomatsakis (Oct 08 2018 at 19:46, on Zulip):

not sure if I what I just said makes sense :)

nikomatsakis (Oct 08 2018 at 19:47, on Zulip):

well, maybe it does

nikomatsakis (Oct 08 2018 at 19:47, on Zulip):

we basically want to say that: if you are assigning to some projection rooted in a local variable x

nikomatsakis (Oct 08 2018 at 19:47, on Zulip):

then x must be initialized or else you get an error

nikomatsakis (Oct 08 2018 at 19:47, on Zulip):

though I guess we'll also have to tweak the used-mut set still

Santiago Pastorino (Oct 08 2018 at 19:49, on Zulip):

will be back in a bit

Santiago Pastorino (Oct 08 2018 at 19:49, on Zulip):

needed to close Firefox meanwhile I’m compiling librustc

Santiago Pastorino (Oct 08 2018 at 19:49, on Zulip):

that part uses massive amounts of memory I think and crashes unless I close things

Santiago Pastorino (Oct 08 2018 at 19:49, on Zulip):

on the cellphone meanwhile

nikomatsakis (Oct 08 2018 at 19:50, on Zulip):

oh dear :)

nikomatsakis (Oct 08 2018 at 19:50, on Zulip):

well, anyway, I think what I showed earlier is the right place to issue the error

nikomatsakis (Oct 08 2018 at 19:50, on Zulip):

not sure what we would need around the "used mut" logic

nikomatsakis (Oct 08 2018 at 19:50, on Zulip):

if any

Santiago Pastorino (Oct 08 2018 at 19:55, on Zulip):

back

Santiago Pastorino (Oct 08 2018 at 19:55, on Zulip):

why did you say instead of that logic?

Santiago Pastorino (Oct 08 2018 at 19:55, on Zulip):

why that would need to be removed?

Santiago Pastorino (Oct 08 2018 at 19:56, on Zulip):

and also why the used-mut set needs to be tweaked?

nikomatsakis (Oct 08 2018 at 19:56, on Zulip):

I was wrong to say that actually

Santiago Pastorino (Oct 08 2018 at 19:56, on Zulip):

ahh ok

nikomatsakis (Oct 08 2018 at 19:56, on Zulip):

the existing logic is handling the case a = ...

Santiago Pastorino (Oct 08 2018 at 19:56, on Zulip):

we need to place it there :)

Santiago Pastorino (Oct 08 2018 at 19:56, on Zulip):

yes

nikomatsakis (Oct 08 2018 at 19:56, on Zulip):

but we are interested in the case of a more complex path

Santiago Pastorino (Oct 08 2018 at 19:56, on Zulip):

but next to that

nikomatsakis (Oct 08 2018 at 19:56, on Zulip):

right

nikomatsakis (Oct 08 2018 at 19:56, on Zulip):

it's kind of a "first check"

nikomatsakis (Oct 08 2018 at 19:56, on Zulip):

anyway, re: the used mut thing, I'm not sure if it's relevant,

Santiago Pastorino (Oct 08 2018 at 19:56, on Zulip):

ok

nikomatsakis (Oct 08 2018 at 19:57, on Zulip):

I guess the question is:

let mut x: (u32, u32);
x.0 = 1;

if we do nothing, this will get an error, and then we'll also (I think) warn that the mut flag on x is unused

nikomatsakis (Oct 08 2018 at 19:57, on Zulip):

which seems wrong

Santiago Pastorino (Oct 08 2018 at 19:57, on Zulip):

:+1:

nikomatsakis (Oct 08 2018 at 19:57, on Zulip):

we could probably do just throw the base variable into the set of "used muts"

nikomatsakis (Oct 08 2018 at 19:57, on Zulip):

indiscriminantely

nikomatsakis (Oct 08 2018 at 19:57, on Zulip):

just to suppress the lint

Santiago Pastorino (Oct 08 2018 at 19:58, on Zulip):

let me try all these stuff out

Santiago Pastorino (Oct 08 2018 at 19:58, on Zulip):

btw: is there a report or something about librustc memory usage or something?

Santiago Pastorino (Oct 08 2018 at 19:58, on Zulip):

is it a concern? maybe is not a priority

Santiago Pastorino (Oct 08 2018 at 19:58, on Zulip):

it gives me a lot of headaches :)

nikomatsakis (Oct 08 2018 at 19:59, on Zulip):

btw: is there a report or something about librustc memory usage or something?

like, memory usage when building librustc?

Santiago Pastorino (Oct 08 2018 at 19:59, on Zulip):

I may need to tweak better my swap or something

nikomatsakis (Oct 08 2018 at 19:59, on Zulip):

I'm not sure if there are any bug reports or not

Santiago Pastorino (Oct 08 2018 at 19:59, on Zulip):

btw: is there a report or something about librustc memory usage or something?

like, memory usage when building librustc?

yes

nikomatsakis (Oct 08 2018 at 19:59, on Zulip):

might be good to take a look and see if it can be improved

nikomatsakis (Oct 08 2018 at 19:59, on Zulip):

can't be helping bootstrap times

Santiago Pastorino (Oct 08 2018 at 20:00, on Zulip):

can't be helping bootstrap times

unsure what's the meaning of this phrase

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

I mean "it must be making bootstrap slower than it would otherwise be"

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

in other words, even if it's not a big blocker — e.g., I have a lot of RAM :P — it still makes things slow

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

hehe yeah

Santiago Pastorino (Oct 08 2018 at 20:11, on Zulip):

I need to pass root_place to mutate_place

Santiago Pastorino (Oct 08 2018 at 20:12, on Zulip):

which doesn't seem to be correct

Santiago Pastorino (Oct 08 2018 at 20:12, on Zulip):

can I get it from place or something?

Santiago Pastorino (Oct 08 2018 at 20:14, on Zulip):

I guess I can call the same is_mutable thing

Santiago Pastorino (Oct 08 2018 at 20:14, on Zulip):

unsure if I have the is_local_mutation_allowed thing neither

nikomatsakis (Oct 08 2018 at 20:18, on Zulip):

I need to pass root_place to mutate_place

what is root_place?

Santiago Pastorino (Oct 08 2018 at 20:18, on Zulip):

what type?

nikomatsakis (Oct 08 2018 at 20:18, on Zulip):

no, what .. value is it?

nikomatsakis (Oct 08 2018 at 20:19, on Zulip):

looking at this fn, I dont' see anything called root_place

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

yeah, I mean, I need it for my logic

Santiago Pastorino (Oct 08 2018 at 20:19, on Zulip):
                if let Place::Projection(_) = root_place.place {
                    let mpi = self.move_data.rev_lookup.find_local(*local);
                    if flow_state.uninits.contains(mpi) {
                        self.infcx.tcx.sess.delay_span_bug(
                            place_span.1,
                            &format!(
                                "ERROR"
                            ),
                            );
                    }

                    return;
                }
nikomatsakis (Oct 08 2018 at 20:19, on Zulip):

what is root_place there?

Santiago Pastorino (Oct 08 2018 at 20:20, on Zulip):

so I was guessing, when I have a.b.c

Santiago Pastorino (Oct 08 2018 at 20:20, on Zulip):

place was the place for c

Santiago Pastorino (Oct 08 2018 at 20:20, on Zulip):

and I need also the place for a

nikomatsakis (Oct 08 2018 at 20:20, on Zulip):

I think you just want place_span.0

nikomatsakis (Oct 08 2018 at 20:20, on Zulip):

in the new function

Santiago Pastorino (Oct 08 2018 at 20:20, on Zulip):

I need both

Santiago Pastorino (Oct 08 2018 at 20:20, on Zulip):

I need to know that I'm in a projection

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

I may be understanding this wrong

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

my thought was

nikomatsakis (Oct 08 2018 at 20:21, on Zulip):

you can invoke place_span.0.local() to get the innermost local variable

nikomatsakis (Oct 08 2018 at 20:21, on Zulip):

local()

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

ahh so place_span would be the root_place here?

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

like in a.b.c = 1;

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

place_span would be the place for a ?

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

place_span will be the place for a.b.c

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

place_span.0, that is

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

local() would return Some(a)

Santiago Pastorino (Oct 08 2018 at 21:06, on Zulip):

ok

Santiago Pastorino (Oct 08 2018 at 21:07, on Zulip):

makes sense

Santiago Pastorino (Oct 08 2018 at 21:08, on Zulip):

so place_span.0 will be Projection in this case

nikomatsakis (Oct 08 2018 at 21:10, on Zulip):

right

Santiago Pastorino (Oct 08 2018 at 21:10, on Zulip):

how should the error message look like?

Santiago Pastorino (Oct 08 2018 at 21:11, on Zulip):

btw

Santiago Pastorino (Oct 08 2018 at 21:11, on Zulip):
         match root_place {
             RootPlace {
                 place: Place::Local(local),
-                is_local_mutation_allowed,
+                is_local_mutation_allowed: _,
             } => {
-                if is_local_mutation_allowed != LocalMutationIsAllowed::Yes {
-                    // If the local may be initialized, and it is now currently being
-                    // mutated, then it is justified to be annotated with the `mut`
-                    // keyword, since the mutation may be a possible reassignment.
-                    let mpi = self.move_data.rev_lookup.find_local(*local);
-                    let ii = &self.move_data.init_path_map[mpi];
-                    for &index in ii {
-                        if flow_state.ever_inits.contains(index) {
-                            self.used_mut.insert(*local);
-                            break;
-                        }
-                    }
-                }
+                self.used_mut.insert(*local);
             }
             RootPlace {
                 place: _,
Santiago Pastorino (Oct 08 2018 at 21:11, on Zulip):

this is what you wanted to do with the used_mut set?

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

where is this code living?

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

oh

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

I see

Santiago Pastorino (Oct 08 2018 at 21:14, on Zulip):

https://github.com/rust-lang/rust/blob/423d8109868c1f926f2cfcc3bff980c3daa515fd/src/librustc_mir/borrow_check/mod.rs#L1815-L1827

nikomatsakis (Oct 08 2018 at 21:14, on Zulip):

how should the error message look like?

probably something like


cannot assign to a.b.c when a is not initialized

?

Santiago Pastorino (Oct 08 2018 at 21:14, on Zulip):

ok

nikomatsakis (Oct 08 2018 at 21:15, on Zulip):

re: the used-mut code...

Santiago Pastorino (Oct 08 2018 at 21:15, on Zulip):

though I guess we'll also have to tweak the used-mut set still

was talking about this

nikomatsakis (Oct 08 2018 at 21:15, on Zulip):

what was that code looking like before your PR?

Santiago Pastorino (Oct 08 2018 at 21:15, on Zulip):

anyway I can https://github.com/rust-lang/rust/blob/423d8109868c1f926f2cfcc3bff980c3daa515fd/src/librustc_mir/borrow_check/mod.rs#L1815-L1827

nikomatsakis (Oct 08 2018 at 21:16, on Zulip):

I think it should probably stay that way

nikomatsakis (Oct 08 2018 at 21:16, on Zulip):

but in the code where we are issuing an error

Santiago Pastorino (Oct 08 2018 at 21:17, on Zulip):

just add to the used mut the thing

nikomatsakis (Oct 08 2018 at 21:18, on Zulip):

right, the idea being — "always consider the mut on this variable used" — since we saw an error related to the variable, we know we can't accurately assess whether mut will be imp't once the error is fixed

nikomatsakis (Oct 08 2018 at 21:20, on Zulip):

but in that code you quoted, that code is to handle the non-error case, and I don't think that has changed

nikomatsakis (Oct 08 2018 at 21:20, on Zulip):

(that is, the used_mut adjustments)

nikomatsakis (Oct 08 2018 at 21:20, on Zulip):

make sense?

Santiago Pastorino (Oct 08 2018 at 21:22, on Zulip):

yep

Santiago Pastorino (Oct 08 2018 at 21:22, on Zulip):

makes sense

Santiago Pastorino (Oct 08 2018 at 21:22, on Zulip):

compiling and will run tests to see what happens

Santiago Pastorino (Oct 08 2018 at 21:22, on Zulip):

hmmm last thing

Santiago Pastorino (Oct 08 2018 at 21:22, on Zulip):

how is cannot assign to a.b.c when a is not initialized produced

Santiago Pastorino (Oct 08 2018 at 21:23, on Zulip):

was doing ...

Santiago Pastorino (Oct 08 2018 at 21:23, on Zulip):
                    self.infcx.tcx.sess.delay_span_bug(
                        place_span.1,
                        &format!(
                            "cannot assign to {} when {} is not initialized",
                            place, place
                        ),
                    );
Santiago Pastorino (Oct 08 2018 at 21:23, on Zulip):

of course place, place is wrong :)

nikomatsakis (Oct 08 2018 at 21:24, on Zulip):

well

Santiago Pastorino (Oct 08 2018 at 21:24, on Zulip):

delay_span_bug is also not what we need

nikomatsakis (Oct 08 2018 at 21:25, on Zulip):

you can do a

                            struct_span_err!(self.sess, e.span, E0571,
                                             "`break` with value from a `{}` loop",
                                             kind.name())
nikomatsakis (Oct 08 2018 at 21:25, on Zulip):

something like that

pnkfelix (Oct 08 2018 at 21:25, on Zulip):

just to be clear, @Santiago Pastorino resolving the thing from #21232 ? Should I officially assign @Santiago Pastorino on that issue?

nikomatsakis (Oct 08 2018 at 21:25, on Zulip):

to make a custom error

nikomatsakis (Oct 08 2018 at 21:25, on Zulip):

right now @Santiago Pastorino is working on making it a "clean error"

nikomatsakis (Oct 08 2018 at 21:25, on Zulip):

but I think it might be a logical follow-up to tackle the "correct semantics", if they wanted, though @Keith Yeung had also expressed some interest I think

pnkfelix (Oct 08 2018 at 21:26, on Zulip):

let me try again. Is @Santiago Pastorino doing this: "we have tentatively decided to attempt, for the short-term, to adopt the semantics that rejects all three cases."

Keith Yeung (Oct 08 2018 at 21:26, on Zulip):

hello

nikomatsakis (Oct 08 2018 at 21:26, on Zulip):

let me try again. Is @Santiago Pastorino doing this: "we have tentatively decided to attempt, for the short-term, to adopt the semantics that rejects all three cases."

confirm

Santiago Pastorino (Oct 08 2018 at 21:26, on Zulip):

I could work on that thing also if needed, unsure if there are other priorities

pnkfelix (Oct 08 2018 at 21:26, on Zulip):

okay

pnkfelix (Oct 08 2018 at 21:27, on Zulip):

I ask because I had a branch in another directory that I was playing with as a background task, where I was trying to add the check to fn check_if_assigned_path_is_moved

Keith Yeung (Oct 08 2018 at 21:28, on Zulip):

oh, i just said that #21232 is a pretty annoying error and it's a real footgun for beginners

Keith Yeung (Oct 08 2018 at 21:28, on Zulip):

that said, i don't exactly know what the correct semantics would be for that particular issue

pnkfelix (Oct 08 2018 at 21:28, on Zulip):

since we already had this code there: https://github.com/rust-lang/rust/blob/423d8109868c1f926f2cfcc3bff980c3daa515fd/src/librustc_mir/borrow_check/mod.rs#L1661

Keith Yeung (Oct 08 2018 at 21:29, on Zulip):

my understanding is that we can sort of keep track of the fields that are initialized after it's been moved out, and if any of them are undefined during a use, throw a compile time error

pnkfelix (Oct 08 2018 at 21:29, on Zulip):

And it seemed "obvious" to me that we might add the "clean error" by using the same logic regardless of whether it was a type with a dtor or not. Of course, the "obvious" thing had totally non-obvious consequences...

Keith Yeung (Oct 08 2018 at 21:30, on Zulip):

but i'm unsure if it's a sound solution

Keith Yeung (Oct 08 2018 at 21:31, on Zulip):

more concretely, i'm not sure if we do anything else special during the construction of an ADT

pnkfelix (Oct 08 2018 at 21:33, on Zulip):

Anyway, I'm headed to bed now, but @Santiago Pastorino , if you want to compare notes on your approach versus the one I was trying out, we can try to chat sometime tomorrow.

Santiago Pastorino (Oct 09 2018 at 04:34, on Zulip):

@pnkfelix :+1: to everything

Santiago Pastorino (Oct 09 2018 at 04:34, on Zulip):

@nikomatsakis the stuff we've talked about is not working .local() returns None

Santiago Pastorino (Oct 09 2018 at 04:34, on Zulip):

need to find out

Santiago Pastorino (Oct 09 2018 at 04:38, on Zulip):

in this case place is a Promoted thing

Santiago Pastorino (Oct 09 2018 at 12:45, on Zulip):

@nikomatsakis ping

Santiago Pastorino (Oct 09 2018 at 12:46, on Zulip):

I'm getting a Promoted thing instead of Projection

Santiago Pastorino (Oct 09 2018 at 12:46, on Zulip):

for this example ...

Santiago Pastorino (Oct 09 2018 at 12:46, on Zulip):
#![feature(nll)]

fn main() {
    let mut s: (i32, i32);
    s.0 = 3;
    s.1 = 4;
    println!("{} {}", s.0, s.1);
}
Santiago Pastorino (Oct 09 2018 at 12:47, on Zulip):

not even sure what Promoted is about

Santiago Pastorino (Oct 09 2018 at 12:48, on Zulip):

so when I call .local() it returns None

Santiago Pastorino (Oct 09 2018 at 12:48, on Zulip):

https://github.com/rust-lang/rust/blob/425a1debb0de638560d277066231eedfa5c5fbd6/src/librustc/mir/mod.rs#L1978

Santiago Pastorino (Oct 09 2018 at 12:49, on Zulip):

First, why s.0 = 3 generates a Promoted thing?

pnkfelix (Oct 09 2018 at 12:49, on Zulip):

https://github.com/rust-lang/rust/blob/425a1debb0de638560d277066231eedfa5c5fbd6/src/librustc/mir/mod.rs#L1867

pnkfelix (Oct 09 2018 at 12:50, on Zulip):

promoted usually means its some expression that got turned into something that we could evaluate at compile-time and store as static data

pnkfelix (Oct 09 2018 at 12:50, on Zulip):

I cannot tell from context here whether it is the 3 that has been promoted, or the whole s

Santiago Pastorino (Oct 09 2018 at 12:50, on Zulip):

yeah that's the 3 part I guess, what about s.0?

Santiago Pastorino (Oct 09 2018 at 12:51, on Zulip):

I cannot tell from context here whether it is the 3 that has been promoted, or the whole s

ahh ok, that makes sense

pnkfelix (Oct 09 2018 at 12:51, on Zulip):

well don't take my word for it 100%

Santiago Pastorino (Oct 09 2018 at 12:51, on Zulip):

but it's s.0

Santiago Pastorino (Oct 09 2018 at 12:51, on Zulip):
DEBUG 2018-10-09T04:59:17Z: rustc_mir::borrow_check: mutate_place: place = (_1.0: i32)
DEBUG 2018-10-09T04:59:17Z: rustc_mir::borrow_check: mutate_place: place.local() = None
Santiago Pastorino (Oct 09 2018 at 12:52, on Zulip):

printed the Debug version of the Place and it gives (_1.0: i32) which is the s.0 part

pnkfelix (Oct 09 2018 at 12:52, on Zulip):

hmm. but it doesn't say _1 as a local?

Santiago Pastorino (Oct 09 2018 at 12:53, on Zulip):

unsure I got what you meant?

pnkfelix (Oct 09 2018 at 12:53, on Zulip):

or I guess that's the point, the promoted thing is (_1.0: i32)

Santiago Pastorino (Oct 09 2018 at 12:53, on Zulip):

yes

Santiago Pastorino (Oct 09 2018 at 12:54, on Zulip):

I was expecting a Projection and apply .local() to get the innermost Local part

pnkfelix (Oct 09 2018 at 12:54, on Zulip):

it seems weird to me

pnkfelix (Oct 09 2018 at 12:54, on Zulip):

I just wasn't expecting a Promoted to print as _1.0

Santiago Pastorino (Oct 09 2018 at 12:55, on Zulip):

https://github.com/rust-lang/rust/blob/425a1debb0de638560d277066231eedfa5c5fbd6/src/librustc/mir/mod.rs#L1995

pnkfelix (Oct 09 2018 at 12:55, on Zulip):

right, and look what promoted.0 is

pnkfelix (Oct 09 2018 at 12:55, on Zulip):

since that would explain the _1.0

pnkfelix (Oct 09 2018 at 12:55, on Zulip):

promoted.0 should be an instance of a struct Promoted

Santiago Pastorino (Oct 09 2018 at 12:56, on Zulip):

ohh

Santiago Pastorino (Oct 09 2018 at 12:56, on Zulip):

I think I did this yesterday a bit sleepy

pnkfelix (Oct 09 2018 at 12:56, on Zulip):

but I think those would print as promoted[INDEX]

Santiago Pastorino (Oct 09 2018 at 12:56, on Zulip):

I think it's a Field

pnkfelix (Oct 09 2018 at 12:56, on Zulip):

based on https://github.com/rust-lang/rust/blob/425a1debb0de638560d277066231eedfa5c5fbd6/src/librustc/mir/mod.rs#L2412

Santiago Pastorino (Oct 09 2018 at 12:57, on Zulip):

well I don't know what it is but I can check out

pnkfelix (Oct 09 2018 at 12:57, on Zulip):

I think it's a Field

so you are now doubting whether its an instance of Place::Promoted ?

Santiago Pastorino (Oct 09 2018 at 12:57, on Zulip):

yes

pnkfelix (Oct 09 2018 at 12:57, on Zulip):

okay

Santiago Pastorino (Oct 09 2018 at 12:57, on Zulip):

the problem is that if Local is None I guessed it was because it wasn't a Local nor a Projection

Santiago Pastorino (Oct 09 2018 at 12:58, on Zulip):

and I jumped quickly to that conclusion based on the Debug prints of it

Santiago Pastorino (Oct 09 2018 at 12:59, on Zulip):

@pnkfelix is there an API to print the variant of an enum in Rust?

Santiago Pastorino (Oct 09 2018 at 12:59, on Zulip):

I mean, some way of introspect

pnkfelix (Oct 09 2018 at 12:59, on Zulip):

there's a discriminant_value method

Santiago Pastorino (Oct 09 2018 at 12:59, on Zulip):

:+1:

pnkfelix (Oct 09 2018 at 13:00, on Zulip):

or rather, that's an intrinsic

nikomatsakis (Oct 09 2018 at 13:00, on Zulip):

@Santiago Pastorino sorry, was afk for a sec, did you resolve your questions?

pnkfelix (Oct 09 2018 at 13:00, on Zulip):

but you probably want std::mem::discriminant

pnkfelix (Oct 09 2018 at 13:00, on Zulip):

https://doc.rust-lang.org/std/mem/fn.discriminant.html

nikomatsakis (Oct 09 2018 at 13:00, on Zulip):

fwiw I sort of hate how we implement Debug for most things

nikomatsakis (Oct 09 2018 at 13:00, on Zulip):

though I think also having the full Debug output would be pretty tedious most of the time

pnkfelix (Oct 09 2018 at 13:00, on Zulip):

Didn't at some point we have some way to select

pnkfelix (Oct 09 2018 at 13:00, on Zulip):

verbose vs non-verbose

nikomatsakis (Oct 09 2018 at 13:01, on Zulip):

we still do

Santiago Pastorino (Oct 09 2018 at 13:01, on Zulip):

@nikomatsakis not yet but I need to find out first what is the variant of this Place that I was expecting to be a Projection

pnkfelix (Oct 09 2018 at 13:01, on Zulip):

in the format string?

nikomatsakis (Oct 09 2018 at 13:01, on Zulip):

but it's not uniform

nikomatsakis (Oct 09 2018 at 13:01, on Zulip):

ah, yes, we have that too

nikomatsakis (Oct 09 2018 at 13:01, on Zulip):

{:#?}

nikomatsakis (Oct 09 2018 at 13:01, on Zulip):

but I don't think our formatters pay any attention

pnkfelix (Oct 09 2018 at 13:01, on Zulip):

I didn't mean -Z verbose, though that might be useful too

nikomatsakis (Oct 09 2018 at 13:01, on Zulip):

might be nice if they did, actually

nikomatsakis (Oct 09 2018 at 13:01, on Zulip):

@Santiago Pastorino what is the output string?

pnkfelix (Oct 09 2018 at 13:01, on Zulip):

ah. We should totally leverage {:#?} in our impl Debug for MIR, or at least for Place...

nikomatsakis (Oct 09 2018 at 13:02, on Zulip):

I absolutely HATE how regions print with {:?} by default

nikomatsakis (Oct 09 2018 at 13:02, on Zulip):

that is part of what made it so !@!$! hard to debug that other problem

pnkfelix (Oct 09 2018 at 13:02, on Zulip):

I have -Z identify-regions

pnkfelix (Oct 09 2018 at 13:02, on Zulip):

it used to help me with this

pnkfelix (Oct 09 2018 at 13:02, on Zulip):

but I dont' know if it continues to work well with NLL-renumbered regions

pnkfelix (Oct 09 2018 at 13:03, on Zulip):

it might

Santiago Pastorino (Oct 09 2018 at 13:03, on Zulip):

@Santiago Pastorino what is the output string?

DEBUG 2018-10-09T04:59:17Z: rustc_mir::borrow_check: mutate_place: place = (_1.0: i32)

nikomatsakis (Oct 09 2018 at 13:04, on Zulip):

that is (I suspect) a field projection, yes

nikomatsakis (Oct 09 2018 at 13:04, on Zulip):
                ProjectionElem::Field(field, ty) => {
                    write!(fmt, "({:?}.{:?}: {:?})", data.base, field.index(), ty)
                }
pnkfelix (Oct 09 2018 at 13:04, on Zulip):

I think local() isn't as general as @Santiago Pastorino thought it was

nikomatsakis (Oct 09 2018 at 13:04, on Zulip):

I think local should be fine

pnkfelix (Oct 09 2018 at 13:04, on Zulip):

since it doesn't recur

pnkfelix (Oct 09 2018 at 13:05, on Zulip):

and it only handles Deref projections

nikomatsakis (Oct 09 2018 at 13:05, on Zulip):

wait, or maybe I am wong

nikomatsakis (Oct 09 2018 at 13:05, on Zulip):

I thought it recurred

nikomatsakis (Oct 09 2018 at 13:05, on Zulip):

what on earth

pnkfelix (Oct 09 2018 at 13:05, on Zulip):

https://github.com/rust-lang/rust/blob/425a1debb0de638560d277066231eedfa5c5fbd6/src/librustc/mir/mod.rs#L1971

nikomatsakis (Oct 09 2018 at 13:05, on Zulip):

what is the point of that

pnkfelix (Oct 09 2018 at 13:05, on Zulip):

Don't ask me

nikomatsakis (Oct 09 2018 at 13:05, on Zulip):

ok, so @Santiago Pastorino I was wrong to suggest local

nikomatsakis (Oct 09 2018 at 13:05, on Zulip):

which apparently does something a bit strange

pnkfelix (Oct 09 2018 at 13:05, on Zulip):

/me goes to use git blame to find out if in fact @pnkfelix is to blame for this

nikomatsakis (Oct 09 2018 at 13:06, on Zulip):

and not what it's doc string suggests (to me)

nikomatsakis (Oct 09 2018 at 13:06, on Zulip):

I shouldn't be so harsh :) I'm sure it made sense at the time

nikomatsakis (Oct 09 2018 at 13:06, on Zulip):

it's just not what I anticipated

pnkfelix (Oct 09 2018 at 13:06, on Zulip):

/me wasn't in the habit of checking @davidtwco 's code for patterns of the form base: Place::Local back at that time. :)

nikomatsakis (Oct 09 2018 at 13:06, on Zulip):

the question is, is it what we actually want for the other callers

nikomatsakis (Oct 09 2018 at 13:07, on Zulip):

looks like it is only used in some debug output

pnkfelix (Oct 09 2018 at 13:07, on Zulip):

I'm betting that it will be fine to generalize it

nikomatsakis (Oct 09 2018 at 13:07, on Zulip):

I would expect so

Santiago Pastorino (Oct 09 2018 at 17:06, on Zulip):

hey I'm back

Santiago Pastorino (Oct 09 2018 at 17:06, on Zulip):

@nikomatsakis when you suggested me to use local() I trusted you that was gonna work regardless I saw it wasn't making the recursion

Santiago Pastorino (Oct 09 2018 at 17:06, on Zulip):

given that this case was just one level deep

Santiago Pastorino (Oct 09 2018 at 17:07, on Zulip):

but anyway I saved a question which was ... what's the point for RootPlace then? :)

Santiago Pastorino (Oct 09 2018 at 17:07, on Zulip):

now I see

Santiago Pastorino (Oct 09 2018 at 17:07, on Zulip):

what if we properly implement local() and maybe even remove RootPlace?

Santiago Pastorino (Oct 09 2018 at 17:08, on Zulip):

I guess we may need to be careful about performance but I guess it worth checking how RootPlace is generated and if we can generate something like that with same perf

Santiago Pastorino (Oct 09 2018 at 17:08, on Zulip):

or maybe just implement local() properly?

nikomatsakis (Oct 09 2018 at 17:12, on Zulip):

I believe that RootPlace plays a different role

nikomatsakis (Oct 09 2018 at 17:12, on Zulip):

so I dont' think we can just remove it

nikomatsakis (Oct 09 2018 at 17:12, on Zulip):

it indicates the place from which the mutability comes

nikomatsakis (Oct 09 2018 at 17:12, on Zulip):

which isn't always the local variable

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

e.g., if you have a x: &mut Foo, and you have a place (*x).bar

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

then the mutability of that place is not dependent on whether the local variable x is mutable

Santiago Pastorino (Oct 09 2018 at 17:13, on Zulip):

I see

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

however

Santiago Pastorino (Oct 09 2018 at 17:13, on Zulip):

the name made me think of a different meaning

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

for our purposes, we can just look at x

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

because -- in the case above -- x would have to be initialized

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

or else you would have an error

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

same as for x.bar = ... (with no deref)

nikomatsakis (Oct 09 2018 at 17:14, on Zulip):

basically, we don't care about derefs, because to make the reference in the first place, the referent must be initialized

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

yep to derefs :)

Santiago Pastorino (Oct 09 2018 at 17:15, on Zulip):

so unsure if I follow we need to navigate Projection until we find what?

nikomatsakis (Oct 09 2018 at 17:16, on Zulip):

something that is not a projection :)

nikomatsakis (Oct 09 2018 at 17:17, on Zulip):

my expectation was that local would be something like:

fn base_local(&self) -> Option<Local> {
    match self {
        Projection(base, _) => base.base_local(),
        Local(l) => Some(l),
        Static(..) | Promoted(..) => None,
    }
}

(Note: I would not use _ here)

Santiago Pastorino (Oct 09 2018 at 17:22, on Zulip):

hmm I'm not understanding properly

Santiago Pastorino (Oct 09 2018 at 17:22, on Zulip):

why just that?

Santiago Pastorino (Oct 09 2018 at 17:22, on Zulip):

why local() is different?

Santiago Pastorino (Oct 09 2018 at 17:22, on Zulip):

_ would be just Static and Promoted

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

why wouldn't it be different?

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

I'm confused what you mean, I guess

nikomatsakis (Oct 09 2018 at 17:24, on Zulip):

the point of this is to return the "root local variable" (if any)

nikomatsakis (Oct 09 2018 at 17:24, on Zulip):

are you asking, why avoid _?

nikomatsakis (Oct 09 2018 at 17:24, on Zulip):

the answer is: in general I disapprove of _

Santiago Pastorino (Oct 09 2018 at 17:24, on Zulip):

I'm asking 3 things

nikomatsakis (Oct 09 2018 at 17:24, on Zulip):

because it is not "future proof" when the enum changes

Santiago Pastorino (Oct 09 2018 at 17:24, on Zulip):

the answer is: in general I disapprove of _

agree with this, just wanted to point out that it was equivalent to what was going on in the local() case

nikomatsakis (Oct 09 2018 at 17:24, on Zulip):

equivalent how?

nikomatsakis (Oct 09 2018 at 17:25, on Zulip):

in the function as it is, it only recurses for one specific kind of projection

nikomatsakis (Oct 09 2018 at 17:25, on Zulip):

a deref

Santiago Pastorino (Oct 09 2018 at 17:25, on Zulip):

because the rest of the 2 variants are Static and Promoted

nikomatsakis (Oct 09 2018 at 17:25, on Zulip):

note that statics / promoted paths are also "root" paths that have no base path, so you can't recurse there

Santiago Pastorino (Oct 09 2018 at 17:25, on Zulip):

yes, I'm talking about the other part of the match

nikomatsakis (Oct 09 2018 at 17:25, on Zulip):

I see

Santiago Pastorino (Oct 09 2018 at 17:25, on Zulip):

I was wondering why do not change local()

nikomatsakis (Oct 09 2018 at 17:25, on Zulip):

well

nikomatsakis (Oct 09 2018 at 17:25, on Zulip):

so in general I think the setup of Place is ungreat

Jake Goulding (Oct 09 2018 at 17:26, on Zulip):

I disapprove of _

... instead preferring .., right?

nikomatsakis (Oct 09 2018 at 17:26, on Zulip):

but refactoring it has proven slow going (@csmoe has been wokring on it)

Santiago Pastorino (Oct 09 2018 at 17:26, on Zulip):

I disapprove of _

... instead preferring .., right?

lol

nikomatsakis (Oct 09 2018 at 17:26, on Zulip):

in particular, in this case, I think it'd be nice to have a distinction between "base" and "projection", where base = (Local | Static | Promoted), and to return a Option<Base>

nikomatsakis (Oct 09 2018 at 17:27, on Zulip):

but we don't have such a type right now...

Santiago Pastorino (Oct 09 2018 at 17:27, on Zulip):

and we are returning None for Static and Promoted

Santiago Pastorino (Oct 09 2018 at 17:27, on Zulip):

so is not really base

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

right, it's just the base local

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

but for our purposes, that suffices

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

:+1:

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

in particular, FOO.bar = ... doesn't have to care whether FOO is initialized

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

since all statics are always initialized

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

same for constants

nikomatsakis (Oct 09 2018 at 17:29, on Zulip):

(that will be an error anyway, but for other reasons)

Santiago Pastorino (Oct 09 2018 at 17:29, on Zulip):

yes

Santiago Pastorino (Oct 09 2018 at 17:54, on Zulip):
[santiago@archlinux rust1 (invalid-no-need-mut)]$ RUST_BACKTRACE=1 rustc +rust-dev test.rs
error: internal compiler error: Error constructed but not emitted

thread 'main' panicked at 'explicit panic', librustc_errors/diagnostic_builder.rs:340:13
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:227
   4: rustc::util::common::panic_hook
             at librustc/util/common.rs:51
   5: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:480
   6: std::panicking::begin_panic
             at libstd/panicking.rs:410
   7: <rustc_errors::diagnostic_builder::DiagnosticBuilder<'a> as core::ops::drop::Drop>::drop
             at librustc_errors/diagnostic_builder.rs:340
   8: rustc_mir::borrow_check::MirBorrowckCtxt::mutate_place
             at ./src/libcore/ptr.rs:194
             at librustc_mir/borrow_check/mod.rs:1132
   9: <rustc_mir::borrow_check::MirBorrowckCtxt<'cx, 'gcx, 'tcx> as rustc_mir::dataflow::DataflowResultsConsumer<'cx, 'tcx>>::visit_statement_entry
             at librustc_mir/borrow_check/mod.rs:0
  10: rustc_mir::dataflow::DataflowResultsConsumer::process_basic_block
             at librustc_mir/dataflow/mod.rs:328
  11: rustc_mir::dataflow::DataflowResultsConsumer::analyze_results
             at librustc_mir/dataflow/mod.rs:318
  12: rustc_mir::borrow_check::do_mir_borrowck
             at librustc_mir/borrow_check/mod.rs:278
  13: rustc::ty::context::tls::set_tlv
             at librustc_mir/borrow_check/mod.rs:124
             at ./src/librustc/infer/mod.rs:508
             at ./src/librustc/ty/context.rs:1676
             at ./src/librustc/ty/context.rs:2022
             at ./src/librustc/ty/context.rs:1961
  14: rustc::ty::context::tls::with_context_opt
             at ./src/librustc/ty/context.rs:2021
             at ./src/librustc/ty/context.rs:1675
             at ./src/librustc/ty/context.rs:2122
             at ./src/librustc/ty/context.rs:2106
             at ./src/librustc/ty/context.rs:2097
  15: rustc::infer::InferCtxtBuilder::enter
             at ./src/librustc/ty/context.rs:2106
             at ./src/librustc/ty/context.rs:2117
             at ./src/librustc/ty/context.rs:1668
             at ./src/librustc/infer/mod.rs:507
  16: rustc_mir::borrow_check::mir_borrowck
             at librustc_mir/borrow_check/mod.rs:122
  17: rustc::ty::query::__query_compute::mir_borrowck
             at librustc/ty/query/plumbing.rs:834
             at librustc/ty/query/plumbing.rs:796
  18: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors<'tcx> for rustc::ty::query::queries::mir_borrowck<'tcx>>::compute
             at librustc/ty/query/plumbing.rs:826
  19: rustc::dep_graph::graph::DepGraph::with_task_impl
             at librustc/dep_graph/graph.rs:342
  20: rustc::ty::context::tls::set_tlv
             at librustc/dep_graph/graph.rs:208
             at librustc/ty/query/plumbing.rs:550
             at librustc/ty/query/plumbing.rs:208
             at librustc/ty/context.rs:2022
             at librustc/ty/context.rs:1961
  21: rustc::ty::context::tls::with_context_opt
             at librustc/ty/context.rs:2021
             at librustc/ty/query/plumbing.rs:207
             at librustc/ty/context.rs:2122
             at librustc/ty/context.rs:2106
             at librustc/ty/context.rs:2097
  22: <rustc::ty::query::plumbing::JobOwner<'a, 'tcx, Q>>::start
             at librustc/ty/context.rs:2106
             at librustc/ty/context.rs:2117
             at librustc/ty/query/plumbing.rs:197
  23: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::force_query_with_job
             at librustc/ty/query/plumbing.rs:543
  24: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
             at librustc/ty/query/plumbing.rs:383
             at librustc/ty/query/plumbing.rs:629
             at librustc/ty/query/plumbing.rs:640
  25: rustc::ty::query::<impl rustc::ty::context::TyCtxt<'a, 'tcx, 'lcx>>::mir_borrowck
             at librustc/ty/query/plumbing.rs:888
             at librustc/ty/query/plumbing.rs:881
  26: rustc::ty::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::par_body_owners
             at librustc_driver/driver.rs:1322
             at ./src/librustc/ty/mod.rs:2576
             at ./src/libcore/iter/iterator.rs:553
             at ./src/libcore/slice/mod.rs:2737
             at ./src/libcore/iter/iterator.rs:553
             at ./src/librustc/ty/mod.rs:2575
  27: rustc::util::common::time_ext
             at librustc_driver/driver.rs:1322
             at ./src/librustc/util/common.rs:163
  28: rustc::ty::context::tls::set_tlv
             at ./src/librustc/util/common.rs:157
             at librustc_driver/driver.rs:1320
             at ./src/librustc/ty/context.rs:2054
             at ./src/librustc/ty/context.rs:2022
             at ./src/librustc/ty/context.rs:1961
  29: <std::thread::local::LocalKey<T>>::try_with
             at ./src/librustc/ty/context.rs:2021
             at ./src/librustc/ty/context.rs:2053
             at ./src/librustc/ty/context.rs:2011
             at ./src/libstd/thread/local.rs:294
  30: <std::thread::local::LocalKey<T>>::try_with
             at ./src/libstd/thread/local.rs:248
             at ./src/librustc/ty/context.rs:2003
             at ./src/libstd/thread/local.rs:294
  31: rustc::ty::context::TyCtxt::create_and_enter
             at ./src/libstd/thread/local.rs:248
             at ./src/librustc/ty/context.rs:1995
             at ./src/librustc/ty/context.rs:2033
             at ./src/librustc/ty/context.rs:1249
  32: rustc_driver::driver::phase_3_run_analysis_passes
             at librustc_driver/driver.rs:1259
  33: rustc_driver::driver::compile_input
             at librustc_driver/driver.rs:287
  34: rustc_driver::run_compiler_with_pool
             at librustc_driver/lib.rs:563
  35: <scoped_tls::ScopedKey<T>>::set
             at librustc_driver/lib.rs:485
             at librustc_driver/driver.rs:76
             at /home/santiago/.cargo/registry/src/github.com-1ecc6299db9ec823/scoped-tls-0.1.2/src/lib.rs:155
  36: rustc_driver::run_compiler
             at librustc_driver/driver.rs:75
             at librustc_driver/lib.rs:484
  37: <scoped_tls::ScopedKey<T>>::set
             at librustc_driver/lib.rs:1746
             at librustc_driver/lib.rs:190
             at /home/santiago/.cargo/registry/src/github.com-1ecc6299db9ec823/scoped-tls-0.1.2/src/lib.rs:155
  38: <scoped_tls::ScopedKey<T>>::set
             at ./src/libsyntax/lib.rs:106
             at /home/santiago/.cargo/registry/src/github.com-1ecc6299db9ec823/scoped-tls-0.1.2/src/lib.rs:155
  39: std::panicking::try::do_call
             at ./src/libsyntax/lib.rs:105
             at librustc_driver/lib.rs:189
             at librustc_driver/lib.rs:1661
             at ./src/libstd/panic.rs:313
             at ./src/libstd/panicking.rs:310
  40: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:102
  41: std::panicking::try
             at ./src/libstd/panicking.rs:289
  42: rustc_driver::in_named_rustc_thread
             at ./src/libstd/panic.rs:392
             at librustc_driver/lib.rs:1575
  43: rustc_driver::monitor
             at librustc_driver/lib.rs:1586
             at librustc_driver/lib.rs:1660
  44: rustc_driver::run
             at librustc_driver/lib.rs:188
  45: rustc_driver::main
             at librustc_driver/lib.rs:1739
  46: std::rt::lang_start::{{closure}}
             at ./src/libstd/rt.rs:74
  47: std::panicking::try::do_call
             at libstd/rt.rs:59
             at libstd/panicking.rs:310
  48: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:102
  49: std::panicking::try
             at libstd/panicking.rs:289
  50: std::rt::lang_start_internal
             at libstd/panic.rs:392
             at libstd/rt.rs:58
  51: main
  52: __libc_start_main
  53: _start
query stack during panic:
#0 [mir_borrowck] processing `main`
end of query stack
error: aborting due to previous error


error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.31.0-dev running on x86_64-unknown-linux-gnu
Santiago Pastorino (Oct 09 2018 at 17:54, on Zulip):

@nikomatsakis I was expecting the error to be emitted somewhere

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

you have to invoke .emit() yourself

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

only we actually want to invoke .buffer I think

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

because of the whole migration business

Santiago Pastorino (Oct 09 2018 at 17:55, on Zulip):

I was just doing ...

Santiago Pastorino (Oct 09 2018 at 17:55, on Zulip):
                    struct_span_err!(self.infcx.tcx.sess, place_span.1, E0718,
                                     "cannot assign to `{:?}` when `{:?}` is not initialized",
                                     place, Place::Local(local)
                    );
                    return;
nikomatsakis (Oct 09 2018 at 17:56, on Zulip):

yeah, that doesn't work

nikomatsakis (Oct 09 2018 at 17:56, on Zulip):

that just constructs a diagnostic

Santiago Pastorino (Oct 09 2018 at 17:56, on Zulip):

you meant to call buffer after that?

nikomatsakis (Oct 09 2018 at 17:56, on Zulip):

and drops it on the floor

Santiago Pastorino (Oct 09 2018 at 17:56, on Zulip):

yes

nikomatsakis (Oct 09 2018 at 17:56, on Zulip):

right

Santiago Pastorino (Oct 09 2018 at 17:56, on Zulip):

ok

Santiago Pastorino (Oct 09 2018 at 17:56, on Zulip):

makes sense

nikomatsakis (Oct 09 2018 at 17:56, on Zulip):

you can look around for an example

Santiago Pastorino (Oct 09 2018 at 17:56, on Zulip):

yep

Santiago Pastorino (Oct 09 2018 at 17:56, on Zulip):

I remember now

Santiago Pastorino (Oct 09 2018 at 17:56, on Zulip):

forgot the way of how this did work

Santiago Pastorino (Oct 09 2018 at 17:56, on Zulip):

for some minute thought you kept calling this and emit was called at the end

Santiago Pastorino (Oct 09 2018 at 18:27, on Zulip):

@nikomatsakis in order to get the name from a place I need to go to the hir level?

Santiago Pastorino (Oct 09 2018 at 18:27, on Zulip):

&self.tcx.hir.name(self.tcx.hir.hir_to_node_id(id)).as_str()

Santiago Pastorino (Oct 09 2018 at 18:27, on Zulip):

something like that?

Santiago Pastorino (Oct 09 2018 at 18:27, on Zulip):

I guess I need some kind of id first?

nikomatsakis (Oct 09 2018 at 18:28, on Zulip):

you don't want that

nikomatsakis (Oct 09 2018 at 18:28, on Zulip):

you want a user-printable string from a place?

Santiago Pastorino (Oct 09 2018 at 18:28, on Zulip):

yeah

Santiago Pastorino (Oct 09 2018 at 18:29, on Zulip):

checked impl Place in several places and there's no such a thing or couldn't find that

nikomatsakis (Oct 09 2018 at 18:29, on Zulip):

there is a method describe_place

nikomatsakis (Oct 09 2018 at 18:29, on Zulip):
    // End-user visible description of `place` if one can be found. If the
    // place is a temporary for instance, None will be returned.
    pub(super) fn describe_place(&self, place: &Place<'tcx>) -> Option<String> {
        self.describe_place_with_options(place, IncludingDowncast(false))
    }
nikomatsakis (Oct 09 2018 at 18:29, on Zulip):

it sometimes returns None because some places cannot be described

nikomatsakis (Oct 09 2018 at 18:29, on Zulip):

I don't think that should apply in this case though

Santiago Pastorino (Oct 09 2018 at 18:32, on Zulip):

you meant that I should just call unwrap() there?

nikomatsakis (Oct 09 2018 at 18:33, on Zulip):

usually the best is to have some alternate wording

nikomatsakis (Oct 09 2018 at 18:33, on Zulip):

in this particular case though

nikomatsakis (Oct 09 2018 at 18:33, on Zulip):

I am not sure if we have to describe the place at all

nikomatsakis (Oct 09 2018 at 18:34, on Zulip):

or, at least, if describe_place returns None

nikomatsakis (Oct 09 2018 at 18:34, on Zulip):

you could say a message like "use of uninitialized variable x"

nikomatsakis (Oct 09 2018 at 18:34, on Zulip):

after all, we always have a local variable here

nikomatsakis (Oct 09 2018 at 18:34, on Zulip):

and we can get its name from mir.local_decls[local].name — and that I think we can unwrap

nikomatsakis (Oct 09 2018 at 18:34, on Zulip):

because the compiler will never generate a use of an uninitialized local

Santiago Pastorino (Oct 09 2018 at 18:35, on Zulip):
error[E0718]: cannot assign to `s.0` when `s` is not initialized
 --> test.rs:5:5
  |
5 |     s.0 = 3;
  |     ^^^^^^^

error[E0718]: cannot assign to `s.1` when `s` is not initialized
 --> test.rs:6:5
  |
6 |     s.1 = 4;
  |     ^^^^^^^

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0718`.
Santiago Pastorino (Oct 09 2018 at 18:35, on Zulip):

:)

Santiago Pastorino (Oct 09 2018 at 18:36, on Zulip):

gonna tweak it a bit with your last tips and push again

Santiago Pastorino (Oct 09 2018 at 18:49, on Zulip):

@nikomatsakis done https://github.com/rust-lang/rust/pull/54621

Santiago Pastorino (Oct 09 2018 at 18:49, on Zulip):

I may end pushing a rustfmt ran or something and would like to see tests running

Santiago Pastorino (Oct 09 2018 at 18:54, on Zulip):

but anyway it's more or less finished I think :)

Santiago Pastorino (Oct 09 2018 at 19:14, on Zulip):

I've made a lot of local fixes

Santiago Pastorino (Oct 09 2018 at 19:14, on Zulip):

pushing a last version

Santiago Pastorino (Oct 09 2018 at 19:14, on Zulip):

after compiling process finishes

nikomatsakis (Oct 09 2018 at 19:18, on Zulip):

looking good

nikomatsakis (Oct 09 2018 at 19:18, on Zulip):

I left some nits

nikomatsakis (Oct 09 2018 at 19:18, on Zulip):

mosly just requests for more comments

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

I still didn’t push the fixed version but will check your nits

Santiago Pastorino (Oct 09 2018 at 21:00, on Zulip):

@nikomatsakis your comments were address but I've also made some other tweaks

Santiago Pastorino (Oct 09 2018 at 21:01, on Zulip):

like changing the two if let's to a match and stuff like that

Santiago Pastorino (Oct 09 2018 at 21:01, on Zulip):

now if ci says so it's ready to merge

Santiago Pastorino (Oct 09 2018 at 21:03, on Zulip):

actually having second thoughts about this https://github.com/rust-lang/rust/pull/54621/commits/b8dbca11db58d62dfbf748377277d74669002fe5#diff-5b4d01d26caf43976125ba0f877e78c0R1123

Santiago Pastorino (Oct 09 2018 at 21:03, on Zulip):

should that wrap everything I guess

Santiago Pastorino (Oct 09 2018 at 21:04, on Zulip):

if it's not mutable this error https://github.com/rust-lang/rust/pull/54621/commits/b8dbca11db58d62dfbf748377277d74669002fe5#diff-5b4d01d26caf43976125ba0f877e78c0R1132 doesn't make any sense

Santiago Pastorino (Oct 09 2018 at 21:05, on Zulip):

and if that's the case why all the tests pass?

Santiago Pastorino (Oct 09 2018 at 21:05, on Zulip):

is it possible that that local has Mutability other than Mut?

nikomatsakis (Oct 09 2018 at 21:05, on Zulip):

if it's not mutable this error https://github.com/rust-lang/rust/pull/54621/commits/b8dbca11db58d62dfbf748377277d74669002fe5#diff-5b4d01d26caf43976125ba0f877e78c0R1132 doesn't make any sense

why does that error not make sense?

nikomatsakis (Oct 09 2018 at 21:06, on Zulip):

I think it makes sense

nikomatsakis (Oct 09 2018 at 21:06, on Zulip):

but all tests passing is a bit suspicious, maybe we just weren't testing this scenario?

Santiago Pastorino (Oct 09 2018 at 21:06, on Zulip):

ok, you're right

Santiago Pastorino (Oct 09 2018 at 21:06, on Zulip):

makes sense

Santiago Pastorino (Oct 09 2018 at 21:07, on Zulip):

we are talking about this example ...

Santiago Pastorino (Oct 09 2018 at 21:07, on Zulip):
fn main() {
    let s: (i32, i32);
    s.0 = 3;
    s.1 = 4;
    println!("{} {}", s.0, s.1);
}
Santiago Pastorino (Oct 09 2018 at 21:07, on Zulip):

right?

nikomatsakis (Oct 09 2018 at 21:07, on Zulip):

yes

Santiago Pastorino (Oct 09 2018 at 21:07, on Zulip):

that should also fail

nikomatsakis (Oct 09 2018 at 21:07, on Zulip):

I just added a review to request you add a test like that

nikomatsakis (Oct 09 2018 at 21:07, on Zulip):

right, it should error in the same way for now

Santiago Pastorino (Oct 09 2018 at 21:08, on Zulip):

let me check what happens

nikomatsakis (Oct 09 2018 at 21:08, on Zulip):

(eventually, we would like to accept both)

nikomatsakis (Oct 09 2018 at 21:08, on Zulip):

(but that's not our immediate goal)

Santiago Pastorino (Oct 09 2018 at 21:09, on Zulip):

yep

Santiago Pastorino (Oct 09 2018 at 21:09, on Zulip):

right now

Santiago Pastorino (Oct 09 2018 at 21:09, on Zulip):
[santiago@archlinux tmp]$ rustc test.rs
error[E0594]: cannot assign to `s.0`, as `s` is not declared as mutable
 --> test.rs:5:5
  |
4 |     let s: (i32, i32);
  |         - help: consider changing this to be mutable: `mut s`
5 |     s.0 = 3;
  |     ^^^^^^^ cannot assign

error[E0594]: cannot assign to `s.1`, as `s` is not declared as mutable
 --> test.rs:6:5
  |
4 |     let s: (i32, i32);
  |         - help: consider changing this to be mutable: `mut s`
5 |     s.0 = 3;
6 |     s.1 = 4;
  |     ^^^^^^^ cannot assign

error: aborting due to 2 previous errors
Santiago Pastorino (Oct 09 2018 at 21:09, on Zulip):

with nightly

Santiago Pastorino (Oct 09 2018 at 21:09, on Zulip):

I'm compiling my thing again

Santiago Pastorino (Oct 09 2018 at 21:10, on Zulip):

in our case is going to say something like

Santiago Pastorino (Oct 09 2018 at 21:10, on Zulip):
cannot assign to `s.0` when `s` is not initialized
nikomatsakis (Oct 09 2018 at 21:10, on Zulip):

seems better

Santiago Pastorino (Oct 09 2018 at 21:10, on Zulip):

:+1:

nikomatsakis (Oct 09 2018 at 21:11, on Zulip):

particularly since we are currently advising people to add mut — but even if they add mut, it's not going to work well

Santiago Pastorino (Oct 09 2018 at 21:11, on Zulip):

is not initialized but at the same time is not mutable

Santiago Pastorino (Oct 09 2018 at 21:11, on Zulip):

what about for this second case, something like ...

Santiago Pastorino (Oct 09 2018 at 21:12, on Zulip):
cannot assign to `s.0` when `s` is not initialized and not mutable either
Santiago Pastorino (Oct 09 2018 at 21:12, on Zulip):

or something written better :)

Santiago Pastorino (Oct 09 2018 at 21:12, on Zulip):

well unsure if it worth

Santiago Pastorino (Oct 09 2018 at 21:12, on Zulip):

whatever you prefer :)

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

I do not like it because

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

s being mutable should have nothing to do with it

Santiago Pastorino (Oct 09 2018 at 21:14, on Zulip):

I mean

nikomatsakis (Oct 09 2018 at 21:14, on Zulip):

in particular, just because s is not mutable, doesn't mean you can't assign to it — you just can't assign to it more than once along any control-flow path

nikomatsakis (Oct 09 2018 at 21:14, on Zulip):

e.g.

nikomatsakis (Oct 09 2018 at 21:14, on Zulip):
fn main() {
    let x: u32;

    x = 22;
}

is accepted

Santiago Pastorino (Oct 09 2018 at 21:14, on Zulip):

what I meant is ... in order for s.0 = something to work

Santiago Pastorino (Oct 09 2018 at 21:14, on Zulip):

in current code

Santiago Pastorino (Oct 09 2018 at 21:14, on Zulip):

you would need to have s initialized

Santiago Pastorino (Oct 09 2018 at 21:14, on Zulip):

and s be mutable

nikomatsakis (Oct 09 2018 at 21:14, on Zulip):

and

fn main() {
    let x: (u32, );

    x.0 = 22;
}

seems analogous to me

nikomatsakis (Oct 09 2018 at 21:15, on Zulip):

hmm

nikomatsakis (Oct 09 2018 at 21:15, on Zulip):

that part is true

Santiago Pastorino (Oct 09 2018 at 21:15, on Zulip):

because that s.0 = something would mean that you're reassinging

nikomatsakis (Oct 09 2018 at 21:15, on Zulip):

right, by definition, since it must be initialized

nikomatsakis (Oct 09 2018 at 21:15, on Zulip):

well, that's true

nikomatsakis (Oct 09 2018 at 21:15, on Zulip):

I guess I don't have a strong opinion about it

nikomatsakis (Oct 09 2018 at 21:15, on Zulip):

maybe we could add a label instead?

Santiago Pastorino (Oct 09 2018 at 21:15, on Zulip):

I have less strong opinion :P

nikomatsakis (Oct 09 2018 at 21:15, on Zulip):

e.g., underline the variable with a suggestion like "- will also have to be declared mut"

Santiago Pastorino (Oct 09 2018 at 21:16, on Zulip):

seems good

nikomatsakis (Oct 09 2018 at 21:16, on Zulip):

/me shrugs

pnkfelix (Oct 09 2018 at 21:33, on Zulip):

@Santiago Pastorino if you want to compare notes, this is how I ended up doing it locally: PR #54941

pnkfelix (Oct 09 2018 at 21:34, on Zulip):

(note that this is just addressing the #21232 thing. It doesn't attempt to touch anything about mut tracking.)

pnkfelix (Oct 09 2018 at 21:34, on Zulip):

but I thought it might be nice for you to see what tests I ended up having to touch

pnkfelix (Oct 09 2018 at 21:35, on Zulip):

I don't actually disagree with the strategy that @nikomatsakis has advised, of focusing on the local that underlies the Place. But I didn't take that tack myself.

pnkfelix (Oct 09 2018 at 21:36, on Zulip):

The only place where I could imagine the two strategies diverging would perhaps be in how they would handle a (hypothetical) Box<Struct> where the box itself has been allocated somehow without initializing the struct it carries.

Santiago Pastorino (Oct 09 2018 at 22:50, on Zulip):

@pnkfelix not sure I followed

Santiago Pastorino (Oct 09 2018 at 22:51, on Zulip):

does your PR do the same as mine or address the #21232 thing?

pnkfelix (Oct 09 2018 at 22:51, on Zulip):

My PR was solely focused on the #21232 thing

Santiago Pastorino (Oct 09 2018 at 22:51, on Zulip):

ok

Santiago Pastorino (Oct 09 2018 at 22:51, on Zulip):

but not complete I guess?

pnkfelix (Oct 09 2018 at 22:51, on Zulip):

Yours is much broader

pnkfelix (Oct 09 2018 at 22:51, on Zulip):

I only put mine up in case you wanted to e.g. look at the tests

Santiago Pastorino (Oct 09 2018 at 22:51, on Zulip):

gotcha

pnkfelix (Oct 09 2018 at 22:52, on Zulip):

I'll write a comment making that clear

Santiago Pastorino (Oct 09 2018 at 22:54, on Zulip):

I've just pushed my stuff

Santiago Pastorino (Oct 09 2018 at 22:54, on Zulip):

gonna compare the tests

Santiago Pastorino (Oct 09 2018 at 22:56, on Zulip):

@pnkfelix https://github.com/rust-lang/rust/pull/54621/commits/56011eb2e3bf8450caaf54fb90050a3e0100398f

Santiago Pastorino (Oct 09 2018 at 22:56, on Zulip):

if you take a look at that there are some differences I guess

Santiago Pastorino (Oct 09 2018 at 22:56, on Zulip):

I'm starting to check so unsure yet

pnkfelix (Oct 09 2018 at 22:58, on Zulip):

so

pnkfelix (Oct 09 2018 at 22:58, on Zulip):

you have some extra errors injected

pnkfelix (Oct 09 2018 at 22:58, on Zulip):

that I avoided

Santiago Pastorino (Oct 09 2018 at 22:58, on Zulip):

yep

Santiago Pastorino (Oct 09 2018 at 22:58, on Zulip):

we don't want this https://github.com/rust-lang/rust/pull/54621/commits/56011eb2e3bf8450caaf54fb90050a3e0100398f#diff-27087a37a3701eb55da69c6420fb0fa8R1

Santiago Pastorino (Oct 09 2018 at 22:59, on Zulip):

the moved error is good enough

pnkfelix (Oct 09 2018 at 22:59, on Zulip):

if you look at my patch

pnkfelix (Oct 09 2018 at 22:59, on Zulip):

you can see I deliberately used PrefixSet::Shallow

pnkfelix (Oct 09 2018 at 23:00, on Zulip):

That was because I was hitting the same case on borrowck-issue-48962.stderr

pnkfelix (Oct 09 2018 at 23:00, on Zulip):

until I made that change

pnkfelix (Oct 09 2018 at 23:00, on Zulip):

Its here on my PR: https://github.com/rust-lang/rust/pull/54941/commits/e28b889108285ae5307e09ccb3fbd27febfcd44d#diff-5b4d01d26caf43976125ba0f877e78c0R1718

Santiago Pastorino (Oct 09 2018 at 23:03, on Zulip):

you can see I deliberately used PrefixSet::Shallow

what is that?

pnkfelix (Oct 09 2018 at 23:04, on Zulip):

well

pnkfelix (Oct 09 2018 at 23:04, on Zulip):

we have a helper for iterating over all the prefixes of a path

pnkfelix (Oct 09 2018 at 23:04, on Zulip):

i.e., given a.b.c, it gives you [a.b.c, a.b, a]

pnkfelix (Oct 09 2018 at 23:05, on Zulip):

But for some common situations , you don't want to traverse over all the kinds of projections

pnkfelix (Oct 09 2018 at 23:05, on Zulip):

namely, there are times when you want to stop as soon as you hit a Deref

pnkfelix (Oct 09 2018 at 23:06, on Zulip):

these are documented in rustc_mir::borrow_check::prefixes

Santiago Pastorino (Oct 09 2018 at 23:06, on Zulip):

I see

pnkfelix (Oct 09 2018 at 23:07, on Zulip):

I'm betting you can adapt the code I used

pnkfelix (Oct 09 2018 at 23:07, on Zulip):

but plug it into the context

pnkfelix (Oct 09 2018 at 23:07, on Zulip):

where you have your change

pnkfelix (Oct 09 2018 at 23:07, on Zulip):

if you prefer to keep it there

pnkfelix (Oct 09 2018 at 23:08, on Zulip):

the Only reason I have my code at the spot where I have it

pnkfelix (Oct 09 2018 at 23:09, on Zulip):

is that it happens to fall near where we handle assignments to fields of structs that implement Drop

pnkfelix (Oct 09 2018 at 23:09, on Zulip):

and I thought the two code paths would end up being the same

pnkfelix (Oct 09 2018 at 23:09, on Zulip):

Alas, they didn't end up the same at all.

Santiago Pastorino (Oct 09 2018 at 23:11, on Zulip):

yeah will try that tomorrow

Santiago Pastorino (Oct 09 2018 at 23:11, on Zulip):

thanks for your help

pnkfelix (Oct 09 2018 at 23:11, on Zulip):

no problem

pnkfelix (Oct 09 2018 at 23:11, on Zulip):

I'm just glad

pnkfelix (Oct 09 2018 at 23:11, on Zulip):

I'm not the only one

pnkfelix (Oct 09 2018 at 23:12, on Zulip):

who hit that issue with borrowck-issue-48962.stderr

Santiago Pastorino (Oct 09 2018 at 23:12, on Zulip):

:)

nikomatsakis (Oct 10 2018 at 13:36, on Zulip):

hmm, ok, so yeah I can see that the simplistic approach I advocated would lead to redundant errors

Santiago Pastorino (Oct 10 2018 at 13:37, on Zulip):

I can use the prefixes trick that @pnkfelix used

Santiago Pastorino (Oct 10 2018 at 13:37, on Zulip):

or we should just go the full @pnkfelix path?

nikomatsakis (Oct 10 2018 at 13:37, on Zulip):

prefixes doesn't .. well, that's ok I guess

nikomatsakis (Oct 10 2018 at 13:37, on Zulip):

but we're not really looking to enumerate the set of prefixes per se

Santiago Pastorino (Oct 10 2018 at 13:38, on Zulip):

what do you mean?

nikomatsakis (Oct 10 2018 at 13:38, on Zulip):

but I guess we can just look for a local in that set

pnkfelix (Oct 10 2018 at 13:38, on Zulip):

It’s just a trick to limit oneself to the Shallow set

nikomatsakis (Oct 10 2018 at 13:38, on Zulip):

right, as opposed to rewriting similar-ish logic

nikomatsakis (Oct 10 2018 at 13:38, on Zulip):

I'm just debating

nikomatsakis (Oct 10 2018 at 13:38, on Zulip):

another way to go about it

nikomatsakis (Oct 10 2018 at 13:38, on Zulip):

so basically there are two cases that would get duplicate errors, I think

nikomatsakis (Oct 10 2018 at 13:39, on Zulip):

derefs of references

nikomatsakis (Oct 10 2018 at 13:39, on Zulip):

but also things with drop, maybe?

nikomatsakis (Oct 10 2018 at 13:39, on Zulip):

I think these kind of correspond to cases that have no MovePathIndex?

nikomatsakis (Oct 10 2018 at 13:39, on Zulip):

I imagine indices also count

nikomatsakis (Oct 10 2018 at 13:39, on Zulip):

e.g., foo[3] = 22, will probably never be allowed if foo is not initialized

nikomatsakis (Oct 10 2018 at 13:40, on Zulip):

so maybe we just want to check if the Place has a MovePathIndex?

nikomatsakis (Oct 10 2018 at 13:40, on Zulip):

and, if so, we then look for its base-local?

pnkfelix (Oct 10 2018 at 13:40, on Zulip):

/me points to their PR

nikomatsakis (Oct 10 2018 at 13:40, on Zulip):

sigh, I'll go read it :P

pnkfelix (Oct 10 2018 at 13:41, on Zulip):

I’m more laughing

nikomatsakis (Oct 10 2018 at 13:41, on Zulip):

well

pnkfelix (Oct 10 2018 at 13:41, on Zulip):

Because what you describe overlaps what it does. Except that it doesn’t bother trying to go to the base local

nikomatsakis (Oct 10 2018 at 13:42, on Zulip):

I guess the question is if we hvae to enumerate prefixes, presumably you have examples why we want to :)

Santiago Pastorino (Oct 10 2018 at 13:42, on Zulip):

I'm confused

Santiago Pastorino (Oct 10 2018 at 13:42, on Zulip):

isn't Felix's PR, enough?

Santiago Pastorino (Oct 10 2018 at 13:42, on Zulip):

or a variant of it?

Santiago Pastorino (Oct 10 2018 at 13:42, on Zulip):

I'm ok with just throwing away my PR and using Felix code

nikomatsakis (Oct 10 2018 at 13:43, on Zulip):

/me shrugs

nikomatsakis (Oct 10 2018 at 13:43, on Zulip):

yeah, maybe we should do that?

nikomatsakis (Oct 10 2018 at 13:43, on Zulip):

it seems like they are sort of doing the same thing

Santiago Pastorino (Oct 10 2018 at 13:43, on Zulip):

I remember that @pnkfelix said that it was for #21232

nikomatsakis (Oct 10 2018 at 13:43, on Zulip):

the place where @pnkfelix put their patch also makes sense

nikomatsakis (Oct 10 2018 at 13:43, on Zulip):

maybe more sense

pnkfelix (Oct 10 2018 at 13:44, on Zulip):

I assume that @Santiago Pastorino ‘s other changes are worth keeping

Santiago Pastorino (Oct 10 2018 at 13:44, on Zulip):

what are the other changes you're mentioning?

nikomatsakis (Oct 10 2018 at 13:45, on Zulip):

I think the only thing in @Santiago Pastorino's PR that's not in yours is the used_mut modification?

pnkfelix (Oct 10 2018 at 13:45, on Zulip):

Right

nikomatsakis (Oct 10 2018 at 13:50, on Zulip):

what happens if we have

fn main() {
    let mut a: (u32, u32);
    a.0 = 22;
}

in your PR, @pnkfelix ?

nikomatsakis (Oct 10 2018 at 13:50, on Zulip):

(do you happen to a build available to test?)

nikomatsakis (Oct 10 2018 at 13:50, on Zulip):

seems like it'd be easy to modify your PR to add the local variable to the set

Santiago Pastorino (Oct 10 2018 at 18:35, on Zulip):

@pnkfelix just wanted to be sure here

Santiago Pastorino (Oct 10 2018 at 18:36, on Zulip):

I guess the best thing would be if you continue with your branch and we throw away my PR

Santiago Pastorino (Oct 10 2018 at 18:36, on Zulip):

anyway, let me know if I can help in some way but given you have 99% of the stuff and you've figured most of the stuff I guess it's better if you continue ... ?

pnkfelix (Oct 10 2018 at 18:46, on Zulip):

(Sorry I was afk for a couple of hours; my parents are visiting. I’ll try to answer these Q’s tonight or tomorrow AM at the latest)

pnkfelix (Oct 11 2018 at 09:29, on Zulip):

what happens if we have

fn main() {
    let mut a: (u32, u32);
    a.0 = 22;
}

in your PR, @pnkfelix ?

Egad, my PR doesn't handle that!

pnkfelix (Oct 11 2018 at 09:30, on Zulip):

Will fix

pnkfelix (Oct 11 2018 at 09:46, on Zulip):

Perhaps we should consider unifying Tuple and Adt ...

nikomatsakis (Oct 11 2018 at 09:50, on Zulip):

In general, I think we have WAY too many type variants. They should basically all be ADT...

pnkfelix (Oct 11 2018 at 10:14, on Zulip):

Ah! :scared: https://github.com/rust-lang/rust/blob/master/src/test/run-pass/issues/issue-26996.rs

pnkfelix (Oct 11 2018 at 10:15, on Zulip):

I'd say we're in the right for shifting to rejecting this code

pnkfelix (Oct 11 2018 at 10:17, on Zulip):

Other similar cases: https://github.com/rust-lang/rust/blob/master/src/test/run-pass/issues/issue-27021.rs and https://github.com/rust-lang/rust/blob/master/src/test/run-pass/issues/issue-49298.rs

pnkfelix (Oct 11 2018 at 10:17, on Zulip):

I am however also now wondering whether this is simply too risky a change for us to throw in at this late date

pnkfelix (Oct 11 2018 at 10:18, on Zulip):

The change being "NLL: Reject partial init of uninitialized record (struct or tuple)" #54986 (I've made two distinct issues so we can stop referring ambiguously to #21232)

pnkfelix (Oct 11 2018 at 15:17, on Zulip):

okay, I found a place where I guess we should map to the base local rather than doing just the nearest prefix like I did

pnkfelix (Oct 11 2018 at 15:18, on Zulip):

namely, if you have two levels of struct, like struct R<F> { f: F, ... } and struct S { x: u32, ... }, and you do: let r: R<S>; r.f.x = 10;

pnkfelix (Oct 11 2018 at 15:18, on Zulip):

I think we want the error to point out that r is uninitialized

pnkfelix (Oct 11 2018 at 15:18, on Zulip):

and not just say that r.f is uninitialized.

pnkfelix (Oct 11 2018 at 15:19, on Zulip):

(though I should go back and double check how that interacts with moving in and out of parts of the struct. E.g. if r was initialized and then we moved *out* of r.f, then we *should* report r.f` as the unintialized thing.

nikomatsakis (Oct 11 2018 at 16:40, on Zulip):

@pnkfelix in the case of issue-26996, can you read from those fields again later? (e.g., c.0)

nikomatsakis (Oct 11 2018 at 16:40, on Zulip):

and not just say that r.f is uninitialized.

agreed

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

@pnkfelix in the case of issue-26996, can you read from those fields again later? (e.g., c.0)

I'll find out

pnkfelix (Oct 11 2018 at 21:04, on Zulip):

@pnkfelix in the case of issue-26996, can you read from those fields again later? (e.g., c.0)

I'll find out

Hmm. In AST-borrowck, you get a "use of moved value c.0

pnkfelix (Oct 11 2018 at 21:04, on Zulip):

but in NLL, you can read it. :fear:

pnkfelix (Oct 11 2018 at 21:04, on Zulip):

So, yeah. Lets get this error landed.

pnkfelix (Oct 11 2018 at 21:05, on Zulip):

Well, that, or accept that some code is going to be accepted and we're just going to slowly trudge our way towards implementing #54987

pnkfelix (Oct 11 2018 at 23:36, on Zulip):

Anyway I've posted updates to my PR at #54941

pnkfelix (Oct 11 2018 at 23:36, on Zulip):

I still haven't incorporated the used_mut stuff that would actually match the topic here...

pnkfelix (Oct 11 2018 at 23:36, on Zulip):

(I'll tackle that on Monday)

pnkfelix (Oct 11 2018 at 23:37, on Zulip):

but in the meantime, it might be a good idea for us to double-check whether we are vaguely okay with the changes to the diagnostics that have resulted from my implementation strategy

pnkfelix (Oct 11 2018 at 23:38, on Zulip):

namely, when I report the move error, I now pass along two places: the used_place, and a moved_place. (Where moved_place should always be a prefix of used_place, though of course it may be a trivial prefix where the two are actually the same place)

pnkfelix (Oct 11 2018 at 23:39, on Zulip):

And then the diagnostic distuinguishes in its report when its talking about what was moved vs what was used

pnkfelix (Oct 11 2018 at 23:41, on Zulip):

(I didn't actually update the diagnostic text very well to reflect this change. But you can see an example of the phenomenon by looking at the places reported for a case like this: https://github.com/rust-lang/rust/commit/8843c083f69864b63f0de7fa317ecffa09951797#diff-5deea307db35b1f117045b9ea609beac )

pnkfelix (Oct 11 2018 at 23:42, on Zulip):

having said that, i'm going to bed.

Last update: Nov 21 2019 at 13:15UTC