MIR changes to improve NLL cannot mutate errors

matthewjasper · matthewjasper · commit c3dbdfee06c7 · 2018-07-20T20:01:11.000+01:00
Alway use unique instead of mutable borrows immutable upvars.
Mark variables that are references for a match guard
diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs
@@ -531,6 +531,8 @@ pub enum BindingForm<'tcx> {
     Var(VarBindingForm<'tcx>),
     /// Binding for a `self`/`&self`/`&mut self` binding where the type is implicit.
     ImplicitSelf,
+    /// Reference used in a guard expression to ensure immutability.
+    RefForGuard,
 }
 
 CloneTypeFoldableAndLiftImpls! { BindingForm<'tcx>, }
@@ -555,6 +557,7 @@ mod binding_form_impl {
             match self {
                 Var(binding) => binding.hash_stable(hcx, hasher),
                 ImplicitSelf => (),
+                RefForGuard => (),
             }
         }
     }
diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs
@@ -742,6 +742,24 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                                     autoderef,
                                     &including_downcast,
                                 )?;
+                            } else if let Place::Local(local) = proj.base {
+                                if let Some(ClearCrossCrate::Set(BindingForm::RefForGuard))
+                                    = self.mir.local_decls[local].is_user_variable {
+                                    self.append_place_to_string(
+                                        &proj.base,
+                                        buf,
+                                        autoderef,
+                                        &including_downcast,
+                                    )?;
+                                } else {
+                                    buf.push_str(&"*");
+                                    self.append_place_to_string(
+                                        &proj.base,
+                                        buf,
+                                        autoderef,
+                                        &including_downcast,
+                                    )?;
+                                }
                             } else {
                                 buf.push_str(&"*");
                                 self.append_place_to_string(
diff --git a/src/librustc_mir/build/expr/as_rvalue.rs b/src/librustc_mir/build/expr/as_rvalue.rs
@@ -206,7 +206,27 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                                 this.consume_by_copy_or_move(place)
                             }
                             _ => {
-                                unpack!(block = this.as_operand(block, scope, upvar))
+                                // Turn mutable borrow captures into unique
+                                // borrow captures when capturing an immutable
+                                // variable. This is sound because the mutation
+                                // that caused the capture will cause an error.
+                                match upvar.kind {
+                                    ExprKind::Borrow {
+                                        borrow_kind: BorrowKind::Mut {
+                                            allow_two_phase_borrow: false
+                                        },
+                                        region,
+                                        arg,
+                                    } => unpack!(block = this.limit_capture_mutability(
+                                        upvar.span,
+                                        upvar.ty,
+                                        scope,
+                                        block,
+                                        arg,
+                                        region,
+                                    )),
+                                    _ => unpack!(block = this.as_operand(block, scope, upvar)),
+                                }
                             }
                         }
                     })
@@ -393,6 +413,101 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         }
     }
 
+    fn limit_capture_mutability(
+        &mut self,
+        upvar_span: Span,
+        upvar_ty: Ty<'tcx>,
+        temp_lifetime: Option<region::Scope>,
+        mut block: BasicBlock,
+        arg: ExprRef<'tcx>,
+        region: &'tcx ty::RegionKind,
+    ) -> BlockAnd<Operand<'tcx>> {
+        let this = self;
+
+        let source_info = this.source_info(upvar_span);
+        let temp = this.local_decls.push(LocalDecl::new_temp(upvar_ty, upvar_span));
+
+        this.cfg.push(block, Statement {
+            source_info,
+            kind: StatementKind::StorageLive(temp)
+        });
+
+        let arg_place = unpack!(block = this.as_place(block, arg));
+
+        let mutability = match arg_place {
+            Place::Local(local) => this.local_decls[local].mutability,
+            Place::Projection(box Projection {
+                base: Place::Local(local),
+                elem: ProjectionElem::Deref,
+            }) => {
+                debug_assert!(
+                    if let Some(ClearCrossCrate::Set(BindingForm::RefForGuard))
+                        = this.local_decls[local].is_user_variable {
+                        true
+                    } else {
+                        false
+                    },
+                    "Unexpected capture place",
+                );
+                this.local_decls[local].mutability
+            }
+            Place::Projection(box Projection {
+                ref base,
+                elem: ProjectionElem::Field(upvar_index, _),
+            })
+            | Place::Projection(box Projection {
+                base: Place::Projection(box Projection {
+                    ref base,
+                    elem: ProjectionElem::Field(upvar_index, _),
+                }),
+                elem: ProjectionElem::Deref,
+            }) => {
+                // Not projected from the implicit `self` in a closure.
+                debug_assert!(
+                    match *base {
+                        Place::Local(local) => local == Local::new(1),
+                        Place::Projection(box Projection {
+                            ref base,
+                            elem: ProjectionElem::Deref,
+                        }) => *base == Place::Local(Local::new(1)),
+                        _ => false,
+                    },
+                    "Unexpected capture place"
+                );
+                // Not in a closure
+                debug_assert!(
+                    this.upvar_decls.len() > upvar_index.index(),
+                    "Unexpected capture place"
+                );
+                this.upvar_decls[upvar_index.index()].mutability
+            }
+            _ => bug!("Unexpected capture place"),
+        };
+
+        let borrow_kind = match mutability {
+            Mutability::Not => BorrowKind::Unique,
+            Mutability::Mut => BorrowKind::Mut { allow_two_phase_borrow: false },
+        };
+
+        this.cfg.push_assign(
+            block,
+            source_info,
+            &Place::Local(temp),
+            Rvalue::Ref(region, borrow_kind, arg_place),
+        );
+
+        // In constants, temp_lifetime is None. We should not need to drop
+        // anything because no values with a destructor can be created in
+        // a constant at this time, even if the type may need dropping.
+        if let Some(temp_lifetime) = temp_lifetime {
+            this.schedule_drop_storage_and_value(
+                upvar_span, temp_lifetime, &Place::Local(temp), upvar_ty,
+            );
+        }
+
+        block.and(Operand::Move(Place::Local(temp)))
+    }
+
     // Helper to get a `-1` value of the appropriate type
     fn neg_1_literal(&mut self, span: Span, ty: Ty<'tcx>) -> Operand<'tcx> {
         let param_ty = ty::ParamEnv::empty().and(self.hir.tcx().lift_to_global(&ty).unwrap());
diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs
@@ -1198,7 +1198,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                 visibility_scope,
                 // FIXME: should these secretly injected ref_for_guard's be marked as `internal`?
                 internal: false,
-                is_user_variable: None,
+                is_user_variable: Some(ClearCrossCrate::Set(BindingForm::RefForGuard)),
             });
             LocalsForNode::Three { val_for_guard, ref_for_guard, for_arm_body }
         } else {
diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs
@@ -286,6 +286,7 @@ struct Builder<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
     /// (A match binding can have two locals; the 2nd is for the arm's guard.)
     var_indices: NodeMap<LocalsForNode>,
     local_decls: IndexVec<Local, LocalDecl<'tcx>>,
+    upvar_decls: Vec<UpvarDecl>,
     unit_temp: Option<Place<'tcx>>,
 
     /// cached block with the RESUME terminator; this is created
@@ -472,11 +473,52 @@ fn construct_fn<'a, 'gcx, 'tcx, A>(hir: Cx<'a, 'gcx, 'tcx>,
 
     let tcx = hir.tcx();
     let span = tcx.hir.span(fn_id);
+
+    // Gather the upvars of a closure, if any.
+    let upvar_decls: Vec<_> = tcx.with_freevars(fn_id, |freevars| {
+        freevars.iter().map(|fv| {
+            let var_id = fv.var_id();
+            let var_hir_id = tcx.hir.node_to_hir_id(var_id);
+            let closure_expr_id = tcx.hir.local_def_id(fn_id);
+            let capture = hir.tables().upvar_capture(ty::UpvarId {
+                var_id: var_hir_id,
+                closure_expr_id: LocalDefId::from_def_id(closure_expr_id),
+            });
+            let by_ref = match capture {
+                ty::UpvarCapture::ByValue => false,
+                ty::UpvarCapture::ByRef(..) => true
+            };
+            let mut decl = UpvarDecl {
+                debug_name: keywords::Invalid.name(),
+                var_hir_id: ClearCrossCrate::Set(var_hir_id),
+                by_ref,
+                mutability: Mutability::Not,
+            };
+            if let Some(hir::map::NodeBinding(pat)) = tcx.hir.find(var_id) {
+                if let hir::PatKind::Binding(_, _, ident, _) = pat.node {
+                    decl.debug_name = ident.name;
+
+                    if let Some(&bm) = hir.tables.pat_binding_modes().get(pat.hir_id) {
+                        if bm == ty::BindByValue(hir::MutMutable) {
+                            decl.mutability = Mutability::Mut;
+                        } else {
+                            decl.mutability = Mutability::Not;
+                        }
+                    } else {
+                        tcx.sess.delay_span_bug(pat.span, "missing binding mode");
+                    }
+                }
+            }
+            decl
+        }).collect()
+    });
+
     let mut builder = Builder::new(hir.clone(),
         span,
         arguments.len(),
         safety,
-        return_ty);
+        return_ty,
+        upvar_decls);
 
     let fn_def_id = tcx.hir.local_def_id(fn_id);
     let call_site_scope = region::Scope::CallSite(body.value.hir_id.local_id);
@@ -519,46 +561,7 @@ fn construct_fn<'a, 'gcx, 'tcx, A>(hir: Cx<'a, 'gcx, 'tcx>,
     info!("fn_id {:?} has attrs {:?}", closure_expr_id,
           tcx.get_attrs(closure_expr_id));
 
-    // Gather the upvars of a closure, if any.
-    let upvar_decls: Vec<_> = tcx.with_freevars(fn_id, |freevars| {
-        freevars.iter().map(|fv| {
-            let var_id = fv.var_id();
-            let var_hir_id = tcx.hir.node_to_hir_id(var_id);
-            let closure_expr_id = tcx.hir.local_def_id(fn_id);
-            let capture = hir.tables().upvar_capture(ty::UpvarId {
-                var_id: var_hir_id,
-                closure_expr_id: LocalDefId::from_def_id(closure_expr_id),
-            });
-            let by_ref = match capture {
-                ty::UpvarCapture::ByValue => false,
-                ty::UpvarCapture::ByRef(..) => true
-            };
-            let mut decl = UpvarDecl {
-                debug_name: keywords::Invalid.name(),
-                var_hir_id: ClearCrossCrate::Set(var_hir_id),
-                by_ref,
-                mutability: Mutability::Not,
-            };
-            if let Some(hir::map::NodeBinding(pat)) = tcx.hir.find(var_id) {
-                if let hir::PatKind::Binding(_, _, ident, _) = pat.node {
-                    decl.debug_name = ident.name;
-
-                    if let Some(&bm) = hir.tables.pat_binding_modes().get(pat.hir_id) {
-                        if bm == ty::BindByValue(hir::MutMutable) {
-                            decl.mutability = Mutability::Mut;
-                        } else {
-                            decl.mutability = Mutability::Not;
-                        }
-                    } else {
-                        tcx.sess.delay_span_bug(pat.span, "missing binding mode");
-                    }
-                }
-            }
-            decl
-        }).collect()
-    });
-
-    let mut mir = builder.finish(upvar_decls, yield_ty);
+    let mut mir = builder.finish(yield_ty);
     mir.spread_arg = spread_arg;
     mir
 }
@@ -571,7 +574,7 @@ fn construct_const<'a, 'gcx, 'tcx>(hir: Cx<'a, 'gcx, 'tcx>,
     let ty = hir.tables().expr_ty_adjusted(ast_expr);
     let owner_id = tcx.hir.body_owner(body_id);
     let span = tcx.hir.span(owner_id);
-    let mut builder = Builder::new(hir.clone(), span, 0, Safety::Safe, ty);
+    let mut builder = Builder::new(hir.clone(), span, 0, Safety::Safe, ty, vec![]);
 
     let mut block = START_BLOCK;
     let expr = builder.hir.mirror(ast_expr);
@@ -590,7 +593,7 @@ fn construct_const<'a, 'gcx, 'tcx>(hir: Cx<'a, 'gcx, 'tcx>,
                               TerminatorKind::Unreachable);
     }
 
-    builder.finish(vec![], None)
+    builder.finish(None)
 }
 
 fn construct_error<'a, 'gcx, 'tcx>(hir: Cx<'a, 'gcx, 'tcx>,
@@ -599,18 +602,19 @@ fn construct_error<'a, 'gcx, 'tcx>(hir: Cx<'a, 'gcx, 'tcx>,
     let owner_id = hir.tcx().hir.body_owner(body_id);
     let span = hir.tcx().hir.span(owner_id);
     let ty = hir.tcx().types.err;
-    let mut builder = Builder::new(hir, span, 0, Safety::Safe, ty);
+    let mut builder = Builder::new(hir, span, 0, Safety::Safe, ty, vec![]);
     let source_info = builder.source_info(span);
     builder.cfg.terminate(START_BLOCK, source_info, TerminatorKind::Unreachable);
-    builder.finish(vec![], None)
+    builder.finish(None)
 }
 
 impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
     fn new(hir: Cx<'a, 'gcx, 'tcx>,
            span: Span,
            arg_count: usize,
            safety: Safety,
-           return_ty: Ty<'tcx>)
+           return_ty: Ty<'tcx>,
+           upvar_decls: Vec<UpvarDecl>)
            -> Builder<'a, 'gcx, 'tcx> {
         let lint_level = LintLevel::Explicit(hir.root_lint_level);
         let mut builder = Builder {
@@ -628,6 +632,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             breakable_scopes: vec![],
             local_decls: IndexVec::from_elem_n(LocalDecl::new_return_place(return_ty,
                                                                              span), 1),
+            upvar_decls,
             var_indices: NodeMap(),
             unit_temp: None,
             cached_resume_block: None,
@@ -645,7 +650,6 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
     }
 
     fn finish(self,
-              upvar_decls: Vec<UpvarDecl>,
               yield_ty: Option<Ty<'tcx>>)
               -> Mir<'tcx> {
         for (index, block) in self.cfg.basic_blocks.iter().enumerate() {
@@ -661,7 +665,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                  yield_ty,
                  self.local_decls,
                  self.arg_count,
-                 upvar_decls,
+                 self.upvar_decls,
                  self.fn_span
         )
     }

Original file line number	Diff line number	Diff line change
`@@ -531,6 +531,8 @@ pub enum BindingForm<'tcx> {`
`531`	`531`	`Var(VarBindingForm<'tcx>),`
`532`	`532`	/// Binding for a `self`/`&self`/`&mut self` binding where the type is implicit.
`533`	`533`	`ImplicitSelf,`
	`534`	`+ /// Reference used in a guard expression to ensure immutability.`
	`535`	`+ RefForGuard,`
`534`	`536`	`}`
`535`	`537`
`536`	`538`	`CloneTypeFoldableAndLiftImpls! { BindingForm<'tcx>, }`
`@@ -555,6 +557,7 @@ mod binding_form_impl {`
`555`	`557`	`match self {`
`556`	`558`	`Var(binding) => binding.hash_stable(hcx, hasher),`
`557`	`559`	`ImplicitSelf => (),`
	`560`	`+ RefForGuard => (),`
`558`	`561`	`}`
`559`	`562`	`}`
`560`	`563`	`}`