Auto merge of #46236 - davidtwco:issue-46023, r=arielb1

MIR-borrowck: immutable unique closure upvars can be mutated

Fixes #46023 and #46160 (see [this comment](https://github.com/rust-lang/rust/pull/46236#issuecomment-347204874)).

src/librustc/ich/impls_mir.rs

@@ -31,7 +31,7 @@
     lexical_scope,
     is_user_variable
 });
-impl_stable_hash_for!(struct mir::UpvarDecl { debug_name, by_ref });
+impl_stable_hash_for!(struct mir::UpvarDecl { debug_name, by_ref, mutability });
 impl_stable_hash_for!(struct mir::BasicBlockData<'tcx> { statements, terminator, is_cleanup });
 impl_stable_hash_for!(struct mir::UnsafetyViolation { source_info, description, kind });
 impl_stable_hash_for!(struct mir::UnsafetyCheckResult { violations, unsafe_blocks });

src/librustc/mir/mod.rs

@@ -546,7 +546,9 @@ pub struct UpvarDecl {
     pub debug_name: Name,
 
     /// If true, the capture is behind a reference.
-    pub by_ref: bool
+    pub by_ref: bool,
+
+    pub mutability: Mutability,
 }
 
 ///////////////////////////////////////////////////////////////////////////

src/librustc_mir/borrow_check.rs

@@ -761,47 +761,34 @@ fn check_if_reassignment_to_immutable_state(&mut self,
         let move_data = self.move_data;
 
         // determine if this path has a non-mut owner (and thus needs checking).
-        let mut l = lvalue;
-        loop {
-            match *l {
-                Lvalue::Projection(ref proj) => {
-                    l = &proj.base;
-                    continue;
-                }
-                Lvalue::Local(local) => {
-                    match self.mir.local_decls[local].mutability {
-                        Mutability::Not => break, // needs check
-                        Mutability::Mut => return,
-                    }
-                }
-                Lvalue::Static(ref static_) => {
-                    // mutation of non-mut static is always illegal,
-                    // independent of dataflow. However it will be catched by
-                    // `check_access_permissions()`, we call delay_span_bug here
-                    // to be sure that no case has been missed
-                    if !self.tcx.is_static_mut(static_.def_id) {
-                        let item_msg = match self.describe_lvalue(lvalue) {
-                            Some(name) => format!("immutable static item `{}`", name),
-                            None => "immutable static item".to_owned()
-                        };
-                        self.tcx.sess.delay_span_bug(span,
-                            &format!("cannot assign to {}, should have been caught by \
-                            `check_access_permissions()`", item_msg));
-                    }
-                    return;
-                }
-            }
+        if let Ok(()) = self.is_mutable(lvalue, LocalMutationIsAllowed::No) {
+            return;
         }
 
-        if let Some(mpi) = self.move_path_for_lvalue(lvalue) {
-            for ii in &move_data.init_path_map[mpi] {
-                if flow_state.ever_inits.curr_state.contains(ii) {
-                    let first_assign_span = self.move_data.inits[*ii].span;
-                    self.report_illegal_reassignment(
-                        context, (lvalue, span), first_assign_span);
-                    break;
+        if let Err(_) = self.is_mutable(lvalue, LocalMutationIsAllowed::Yes) {
+            return;
+        }
+
+        match self.move_path_closest_to(lvalue) {
+            Ok(mpi) => {
+                for ii in &move_data.init_path_map[mpi] {
+                    if flow_state.ever_inits.curr_state.contains(ii) {
+                        let first_assign_span = self.move_data.inits[*ii].span;
+                        self.report_illegal_reassignment(
+                            context, (lvalue, span), first_assign_span);
+                        break;
+                    }
                 }
-            }
+            },
+            Err(NoMovePathFound::ReachedStatic) => {
+                let item_msg = match self.describe_lvalue(lvalue) {
+                    Some(name) => format!("immutable static item `{}`", name),
+                    None => "immutable static item".to_owned()
+                };
+                self.tcx.sess.delay_span_bug(span,
+                    &format!("cannot assign to {}, should have been caught by \
+                    `check_access_permissions()`", item_msg));
+            },
         }
     }
 
@@ -1108,12 +1095,7 @@ fn is_mutable<'d>(&self,
                     ProjectionElem::Deref => {
                         let base_ty = proj.base.ty(self.mir, self.tcx).to_ty(self.tcx);
 
-                        // `Box<T>` owns its content, so mutable if its location is mutable
-                        if base_ty.is_box() {
-                            return self.is_mutable(&proj.base, LocalMutationIsAllowed::No);
-                        }
-
-                        // Otherwise we check the kind of deref to decide
+                        // Check the kind of deref to decide
                         match base_ty.sty {
                             ty::TyRef(_, tnm) => {
                                 match tnm.mutbl {
@@ -1121,7 +1103,13 @@ fn is_mutable<'d>(&self,
                                     hir::MutImmutable => Err(lvalue),
                                     // Mutably borrowed data is mutable, but only if we have a
                                     // unique path to the `&mut`
-                                    hir::MutMutable => self.is_unique(&proj.base),
+                                    hir::MutMutable => {
+                                        if self.is_upvar_field_projection(&proj.base).is_some() {
+                                            self.is_mutable(&proj.base, is_local_mutation_allowed)
+                                        } else {
+                                            self.is_unique(&proj.base)
+                                        }
+                                    },
                                 }
                             },
                             ty::TyRawPtr(tnm) => {
@@ -1133,8 +1121,11 @@ fn is_mutable<'d>(&self,
                                     hir::MutMutable => Ok(()),
                                 }
                             },
+                            // `Box<T>` owns its content, so mutable if its location is mutable
+                            _ if base_ty.is_box() =>
+                                self.is_mutable(&proj.base, LocalMutationIsAllowed::No),
                             // Deref should only be for reference, pointers or boxes
-                            _ => bug!("Deref of unexpected type: {:?}", base_ty)
+                            _ => bug!("Deref of unexpected type: {:?}", base_ty),
                         }
                     },
                     // All other projections are owned by their base path, so mutable if
@@ -1143,8 +1134,20 @@ fn is_mutable<'d>(&self,
                     ProjectionElem::Index(..) |
                     ProjectionElem::ConstantIndex{..} |
                     ProjectionElem::Subslice{..} |
-                    ProjectionElem::Downcast(..) =>
+                    ProjectionElem::Downcast(..) => {
+                        let field_projection = self.is_upvar_field_projection(lvalue);
+
+                        if let Some(field) = field_projection {
+                            let decl = &self.mir.upvar_decls[field.index()];
+
+                            return match decl.mutability {
+                                Mutability::Mut => self.is_unique(&proj.base),
+                                Mutability::Not => Err(lvalue),
+                            };
+                        }
+
                         self.is_mutable(&proj.base, LocalMutationIsAllowed::No)
+                    }
                 }
             }
         }

src/librustc_mir/build/mod.rs

@@ -443,10 +443,20 @@ fn construct_fn<'a, 'gcx, 'tcx, A>(hir: Cx<'a, 'gcx, 'tcx>,
             let mut decl = UpvarDecl {
                 debug_name: keywords::Invalid.name(),
                 by_ref,
+                mutability: Mutability::Not,
             };
             if let Some(hir::map::NodeBinding(pat)) = tcx.hir.find(var_id) {
                 if let hir::PatKind::Binding(_, _, ref ident, _) = pat.node {
                     decl.debug_name = ident.node;
+
+                    let bm = *hir.tables.pat_binding_modes()
+                                        .get(pat.hir_id)
+                                        .expect("missing binding mode");
+                    if bm == ty::BindByValue(hir::MutMutable) {
+                        decl.mutability = Mutability::Mut;
+                    } else {
+                        decl.mutability = Mutability::Not;
+                    }
                 }
             }
             decl
@@ -571,7 +581,7 @@ fn args_and_body(&mut self,
             }
 
             self.local_decls.push(LocalDecl {
-                mutability: Mutability::Not,
+                mutability: Mutability::Mut,
                 ty,
                 source_info: SourceInfo {
                     scope: ARGUMENT_VISIBILITY_SCOPE,

src/test/compile-fail/issue-46023.rs

+// Copyright 2016 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.
+
+// revisions: ast mir
+//[mir]compile-flags: -Z emit-end-regions -Z borrowck=mir
+
+fn main() {
+    let x = 0;
+
+    (move || {
+        x = 1;
+        //[mir]~^ ERROR cannot assign to immutable item `x` [E0594]
+        //[ast]~^^ ERROR cannot assign to captured outer variable in an `FnMut` closure [E0594]
+    })()
+}

src/test/run-pass/issue-16671.rs

@@ -8,6 +8,8 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+//compile-flags: -Z borrowck=compare -Z emit-end-regions
+
 #![deny(warnings)]
 
 fn foo<F: FnOnce()>(_f: F) { }