Auto merge of #51964 - matthewjasper:unused-mut-mir-generation, r=nikomatsakis

[NLL] Fix various unused mut errors

Closes #51801
Closes #50897
Closes #51830
Closes #51904
cc #51918 - keeping this one open in case there are any more issues

This PR contains multiple changes. List of changes with examples of what they fix:

* Change mir generation so that the parameter variable doesn't get a name when a `ref` pattern is used as an argument
```rust
fn f(ref y: i32) {} // doesn't trigger lint
```
* Change mir generation so that by-move closure captures don't get first moved into a temporary.
```rust
let mut x = 0; // doesn't trigger lint
move || {
    x = 1;
};
```
* Treat generator upvars the same as closure upvars
```rust
let mut x = 0; // This mut is now necessary and is not linted against.
move || {
    x = 1;
    yield;
};
```

r? @nikomatsakis

Author: bors

src/librustc_mir/borrow_check/mod.rs

@@ -1182,26 +1182,37 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 // We need to report back the list of mutable upvars that were
                 // moved into the closure and subsequently used by the closure,
                 // in order to populate our used_mut set.
-                if let AggregateKind::Closure(def_id, _) = &**aggregate_kind {
-                    let BorrowCheckResult {
-                        used_mut_upvars, ..
-                    } = self.tcx.mir_borrowck(*def_id);
-                    debug!("{:?} used_mut_upvars={:?}", def_id, used_mut_upvars);
-                    for field in used_mut_upvars {
-                        match operands[field.index()] {
-                            Operand::Move(Place::Local(local)) => {
-                                self.used_mut.insert(local);
-                            }
-                            Operand::Move(ref place @ Place::Projection(_)) => {
-                                if let Some(field) = self.is_upvar_field_projection(place) {
-                                    self.used_mut_upvars.push(field);
+                match **aggregate_kind {
+                    AggregateKind::Closure(def_id, _)
+                    | AggregateKind::Generator(def_id, _, _) => {
+                        let BorrowCheckResult {
+                            used_mut_upvars, ..
+                        } = self.tcx.mir_borrowck(def_id);
+                        debug!("{:?} used_mut_upvars={:?}", def_id, used_mut_upvars);
+                        for field in used_mut_upvars {
+                            // This relies on the current way that by-value
+                            // captures of a closure are copied/moved directly
+                            // when generating MIR.
+                            match operands[field.index()] {
+                                Operand::Move(Place::Local(local))
+                                | Operand::Copy(Place::Local(local)) => {
+                                    self.used_mut.insert(local);
+                                }
+                                Operand::Move(ref place @ Place::Projection(_))
+                                | Operand::Copy(ref place @ Place::Projection(_)) => {
+                                    if let Some(field) = self.is_upvar_field_projection(place) {
+                                        self.used_mut_upvars.push(field);
+                                    }
                                 }
+                                Operand::Move(Place::Static(..))
+                                | Operand::Copy(Place::Static(..))
+                                | Operand::Constant(..) => {}
                             }
-                            Operand::Move(Place::Static(..))
-                            | Operand::Copy(..)
-                            | Operand::Constant(..) => {}
                         }
                     }
+                    AggregateKind::Adt(..)
+                    | AggregateKind::Array(..)
+                    | AggregateKind::Tuple { .. } => (),
                 }
 
                 for operand in operands {
@@ -1940,6 +1951,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     }
                 }
             }
+            RootPlace {
+                place: _,
+                is_local_mutation_allowed: LocalMutationIsAllowed::Yes,
+            } => {}
             RootPlace {
                 place: place @ Place::Projection(_),
                 is_local_mutation_allowed: _,
@@ -2115,13 +2130,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         match *place {
             Place::Projection(ref proj) => match proj.elem {
                 ProjectionElem::Field(field, _ty) => {
-                    let is_projection_from_ty_closure = proj
-                        .base
-                        .ty(self.mir, self.tcx)
-                        .to_ty(self.tcx)
-                        .is_closure();
+                    let base_ty = proj.base.ty(self.mir, self.tcx).to_ty(self.tcx);
 
-                    if is_projection_from_ty_closure {
+                    if  base_ty.is_closure() || base_ty.is_generator() {
                         Some(field)
                     } else {
                         None

src/librustc_mir/build/expr/as_rvalue.rs

@@ -186,10 +186,29 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             }
             ExprKind::Closure { closure_id, substs, upvars, movability } => {
                 // see (*) above
-                let mut operands: Vec<_> =
-                    upvars.into_iter()
-                          .map(|upvar| unpack!(block = this.as_operand(block, scope, upvar)))
-                          .collect();
+                let mut operands: Vec<_> = upvars
+                    .into_iter()
+                    .map(|upvar| {
+                        let upvar = this.hir.mirror(upvar);
+                        match Category::of(&upvar.kind) {
+                            // Use as_place to avoid creating a temporary when
+                            // moving a variable into a closure, so that
+                            // borrowck knows which variables to mark as being
+                            // used as mut. This is OK here because the upvar
+                            // expressions have no side effects and act on
+                            // disjoint places.
+                            // This occurs when capturing by copy/move, while
+                            // by reference captures use as_operand
+                            Some(Category::Place) => {
+                                let place = unpack!(block = this.as_place(block, upvar));
+                                this.consume_by_copy_or_move(place)
+                            }
+                            _ => {
+                                unpack!(block = this.as_operand(block, scope, upvar))
+                            }
+                        }
+                    })
+                    .collect();
                 let result = match substs {
                     UpvarSubsts::Generator(substs) => {
                         let movability = movability.unwrap();

src/librustc_mir/build/mod.rs

@@ -672,11 +672,18 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
     {
         // Allocate locals for the function arguments
         for &ArgInfo(ty, _, pattern, _) in arguments.iter() {
-            // If this is a simple binding pattern, give the local a nice name for debuginfo.
+            // If this is a simple binding pattern, give the local a name for
+            // debuginfo and so that error reporting knows that this is a user
+            // variable. For any other pattern the pattern introduces new
+            // variables which will be named instead.
             let mut name = None;
             if let Some(pat) = pattern {
-                if let hir::PatKind::Binding(_, _, ident, _) = pat.node {
-                    name = Some(ident.name);
+                match pat.node {
+                    hir::PatKind::Binding(hir::BindingAnnotation::Unannotated, _, ident, _)
+                    | hir::PatKind::Binding(hir::BindingAnnotation::Mutable, _, ident, _) => {
+                        name = Some(ident.name);
+                    }
+                    _ => (),
                 }
             }
 

src/test/mir-opt/end_region_7.rs

@@ -34,38 +34,31 @@ fn foo<F>(f: F) where F: FnOnce() -> i32 {
 //     ...
 //     let mut _2: ();
 //     let mut _3: [closure@NodeId(22) d:D];
-//     let mut _4: D;
 //     bb0: {
 //         StorageLive(_1);
 //         _1 = D::{{constructor}}(const 0i32,);
 //         StorageLive(_3);
-//         StorageLive(_4);
-//         _4 = move _1;
-//         _3 = [closure@NodeId(22)] { d: move _4 };
-//         drop(_4) -> [return: bb4, unwind: bb3];
+//         _3 = [closure@NodeId(22)] { d: move _1 };
+//         _2 = const foo(move _3) -> [return: bb2, unwind: bb4];
 //     }
 //     bb1: {
 //         resume;
 //     }
 //     bb2: {
-//         drop(_1) -> bb1;
+//         drop(_3) -> [return: bb5, unwind: bb3];
 //     }
 //     bb3: {
-//         drop(_3) -> bb2;
+//         drop(_1) -> bb1;
 //     }
 //     bb4: {
-//         StorageDead(_4);
-//         _2 = const foo(move _3) -> [return: bb5, unwind: bb3];
+//         drop(_3) -> bb3;
 //     }
 //     bb5: {
-//         drop(_3) -> [return: bb6, unwind: bb2];
-//     }
-//     bb6: {
 //         StorageDead(_3);
 //         _0 = ();
-//         drop(_1) -> [return: bb7, unwind: bb1];
+//         drop(_1) -> [return: bb6, unwind: bb1];
 //     }
-//     bb7: {
+//     bb6: {
 //         StorageDead(_1);
 //         return;
 //     }

src/test/mir-opt/end_region_8.rs

@@ -37,17 +37,13 @@ fn foo<F>(f: F) where F: FnOnce() -> i32 {
 //    ...
 //    let mut _3: ();
 //    let mut _4: [closure@NodeId(22) r:&'19s D];
-//    let mut _5: &'21_1rs D;
 //    bb0: {
 //        StorageLive(_1);
 //        _1 = D::{{constructor}}(const 0i32,);
 //        StorageLive(_2);
 //        _2 = &'21_1rs _1;
 //        StorageLive(_4);
-//        StorageLive(_5);
-//        _5 = _2;
-//        _4 = [closure@NodeId(22)] { r: move _5 };
-//        StorageDead(_5);
+//        _4 = [closure@NodeId(22)] { r: _2 };
 //        _3 = const foo(move _4) -> [return: bb2, unwind: bb3];
 //    }
 //    bb1: {

src/test/ui/nll/capture-mut-ref.rs

@@ -0,0 +1,25 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Check that capturing a mutable reference by move and assigning to its
+// referent doesn't make the unused mut lint think that it is mutable.
+
+#![feature(nll)]
+#![deny(unused_mut)]
+
+fn mutable_upvar() {
+    let mut x = &mut 0;
+    //~^ ERROR
+    move || {
+        *x = 1;
+    };
+}
+
+fn main() {}

src/test/ui/nll/capture-mut-ref.stderr

@@ -0,0 +1,16 @@
+error: variable does not need to be mutable
+  --> $DIR/capture-mut-ref.rs:18:9
+   |
+LL |     let mut x = &mut 0;
+   |         ----^
+   |         |
+   |         help: remove this `mut`
+   |
+note: lint level defined here
+  --> $DIR/capture-mut-ref.rs:15:9
+   |
+LL | #![deny(unused_mut)]
+   |         ^^^^^^^^^^
+
+error: aborting due to previous error
+

src/test/ui/nll/extra-unused-mut.rs

@@ -0,0 +1,64 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// extra unused mut lint tests for #51918
+
+// run-pass
+
+#![feature(generators, nll)]
+#![deny(unused_mut)]
+
+fn ref_argument(ref _y: i32) {}
+
+// #51801
+fn mutable_upvar() {
+    let mut x = 0;
+    move || {
+        x = 1;
+    };
+}
+
+// #50897
+fn generator_mutable_upvar() {
+    let mut x = 0;
+    move || {
+        x = 1;
+        yield;
+    };
+}
+
+// #51830
+fn ref_closure_argument() {
+    let _ = Some(0).as_ref().map(|ref _a| true);
+}
+
+struct Expr {
+    attrs: Vec<u32>,
+}
+
+// #51904
+fn parse_dot_or_call_expr_with(mut attrs: Vec<u32>) {
+    let x = Expr { attrs: vec![] };
+    Some(Some(x)).map(|expr|
+        expr.map(|mut expr| {
+            attrs.push(666);
+            expr.attrs = attrs;
+            expr
+        })
+    );
+}
+
+fn main() {
+    ref_argument(0);
+    mutable_upvar();
+    generator_mutable_upvar();
+    ref_closure_argument();
+    parse_dot_or_call_expr_with(Vec::new());
+}

src/test/ui/nll/generator-upvar-mutability.rs

@@ -0,0 +1,24 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Check that generators respect the muatability of their upvars.
+
+#![feature(generators, nll)]
+
+fn mutate_upvar() {
+    let x = 0;
+    move || {
+        x = 1;
+        //~^ ERROR
+        yield;
+    };
+}
+
+fn main() {}

src/test/ui/nll/generator-upvar-mutability.stderr

@@ -0,0 +1,9 @@
+error[E0594]: cannot assign to immutable item `x`
+  --> $DIR/generator-upvar-mutability.rs:18:9
+   |
+LL |         x = 1;
+   |         ^^^^^ cannot assign
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0594`.