Use EverInit instead of MaybeInit to determine initialization

[KiChjang]

Fixes #50461.
Fixes #50463.

[rust-highfive]

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

[KiChjang]

r? @pnkfelix or @nikomatsakis

[estebank]

lgtm

[rust-highfive]

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[KiChjang]

@TimNN Looks like that if there's a timeout, the logs above say nothing about it.

[pnkfelix]

@KiChjang the change to the compiler functionality looks fine to me

but since it seems like you made what look like two independent changes, I would imagine we need more than the single small unit test that you have here.

In particular, I suspect you need a test to check the change where you added unused mut logic for reservations or writes of WriteKind::MutableBorrow(BorrowKind::Unique))

(Let us know if you want help constructing such a test!)

[jonhoo]

This is currently causing a lot of spurious warnings on nightly. @KiChjang any chance you could take a second look following @pnkfelix's comments?

[KiChjang]

@jonhoo I'm already looking into it; the problem is I don't know how to construct a test for that functionality.

[pnkfelix]

@KiChjang Okay so here's the deal

Unique borrows (sometimes called &uniq) are this funny thing in-between & and &mut.

They largely arise from closures which

1. need unaliased access (&uniq) to some state in a free variable, but
2. are not actually mutating the free variable itself
   • (and thus do not need a &mut borrow nor a mut annotation on the variable).

You can see some high-level discussion of the matter at the following (old!) blog post by @nikomatsakis : http://smallcultfollowing.com/babysteps/blog/2014/05/13/focusing-on-ownership/

• (That blog post and related discussions fired off the so-called "Mutapocalypse")

Here (under the details) are some examples of where the construct arises

mod mutate_deref {
    fn foo(a: &mut i32) { // note: *not* `mut a: &mut i32`
        {
            let _closure = || { *a = 3; }; // mutates deref of `&uniq` borrowed `a`
        }

        *a = 4; // (this proves `a` was reborrowed for limited time, not moved)
    }
}

mod mut_borrow {
    fn inside_closure(_x: &mut i32) { }

    fn foo(a: &mut i32) { // note: *not* `mut a: &mut i32`
        let _closure = || { inside_closure(a) }; // `&uniq` reborrow `a` w/o mutating local
    }
}

mod mutate_field {
    fn foo(a: &mut (i32, i32)) { // note: *not* `mut a`
        {
            let _closure = || { a.0 = 3; }; // mutates deref of `&uniq` borrowed `a`
        }

        a.0 = 4; // (this proves `a` was reborrowed for limited time, not moved)
    }
}

---

For comparison's sake, here (under the details) are some cases that look similar but where the Unique construct does not arise:

mod generic {
    fn inside_closure<X>(_x: X) { }

    fn foo(a: &mut i32) {
        let _closure = || { inside_closure(a) }; // *moves* `a` due to above signature
    }
}

mod shared_borrow {
    fn inside_closure(_x: &i32) { }

    fn foo(a: &mut i32) {
        {
            println!("we can read a: {:?}", a);
            let _closure = || { inside_closure(a) }; // a shared reborrow; not unique.
            println!("we can read a: {:?}", a);
        }

        *a = 4; // this proves we had shared reborrow for more limited time
    }
}

mod mutate_local {
    fn foo<'r>(mut a: &'r mut i32, b: Option<&'r mut i32>) {
        let _closure = || { a = b.unwrap(); }; // mutates local `a` (via implicit `&mut` borrow)
    }
}

---

If my memory is right, the main reason why we distinguish unique borrows from mutable borrows is precisely so that one does not have to mark those locals as mutable. I think the mental model is that you are not mutating the local, but rather than thing it points to, and that does not require a let mut on the local.

Looking at this again, I am now even more curious to understand the actual net-effect of adding the self.add_used_mut(root_place, flow_state) call in the Unique case.

• As you can see from the above examples, mutation of a deref of a local does not itself require mut annotation on the local variable. But when I built your branch locally and tried it out, I did not immediately find any instances where the lint was erroneously suppressed. So your code is not "obviously incorrect".)
• But I still cannot figure out what bug, if any, that part of the code is fixing.
  • In particular, there is already a test, lint-unused-mut-variables.rs that is testing a lot of these cases, both with and without NLL. It even includes cases like the ones I showed above. But your addition self.add_used_mut(root_place, flow_state) call in the Unique case does not seem to have made any change to that test's behavior (unless I am misunderstanding something...).

---

FYI, the quickest way I've found, at least in terms of developer effort, to "discover" how to reconstruct a case like this when you're starting from nothing: Just make a build of rustc that panics after hitting the code in question, like this:

src/librustc_mir/borrow_check/mod.rs
--- INDEX:/src/librustc_mir/borrow_check/mod.rs
+++ WORKDIR:/src/librustc_mir/borrow_check/mod.rs
@@ -1711,7 +1711,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             Reservation(WriteKind::MutableBorrow(BorrowKind::Unique))
             | Write(WriteKind::MutableBorrow(BorrowKind::Unique)) => {
                 match self.is_mutable(place, LocalMutationIsAllowed::Yes) {
-                    Ok(root_place) => self.add_used_mut(root_place, flow_state),
+                    Ok(root_place) => {
+                        self.add_used_mut(root_place, flow_state);
+                        panic!("this is what we're looking for.");
+                    }
                     Err(_place_err) => {
                         span_bug!(span, "&unique borrow for {:?} should not fail", place);
                     }

And then run the test suite: That should yield a collection of instances of input .rs files that exercise the code in question.

(Of course this technique needs to be adapted slightly if the code in question is executed during bootstrap itself; in that case you can e.g. make the panic dependent on an environment variable.)

---

Here's my suggestion at this point: If you cannot identify (via a concrete test input) what bug is fixed by your addition of the self.add_used_mut(root_place, flow_state) call in the Unique case, then that implies that neither of us are able to identify such a bug, and I would conclude you should remove that change from this PR. (All the other stuff would stay.)

(But double check first if you can find a concrete test input after considering the examples I provided above! I assume you had some initial motivation for adding that line!)

[KiChjang]

Ok, for the sake of a timely fix, I've removed the check under WriteKind::MutableBorrow(BorrowKind::Unique)).

[pnkfelix]

@bors r+

[bors]

📌 Commit 8b24644 has been approved by pnkfelix

[bors]

⌛ Testing commit 8b24644 with merge 952f344...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing 952f344 to master...