From c3dbdfee06c733f36420a651cd9682e0ee4f95c3 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sun, 15 Jul 2018 15:00:58 +0100 Subject: [PATCH] MIR changes to improve NLL cannot mutate errors Alway use unique instead of mutable borrows immutable upvars. Mark variables that are references for a match guard --- src/librustc/mir/mod.rs | 3 + .../borrow_check/error_reporting.rs | 18 +++ src/librustc_mir/build/expr/as_rvalue.rs | 117 +++++++++++++++++- src/librustc_mir/build/matches/mod.rs | 2 +- src/librustc_mir/build/mod.rs | 100 ++++++++------- 5 files changed, 190 insertions(+), 50 deletions(-) diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index f6076896385..458c2f3885f 100644 --- 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 index c481d1d325b..20eae289e5f 100644 --- 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 index 7bd9a241a53..384eb1db04f 100644 --- 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, + mut block: BasicBlock, + arg: ExprRef<'tcx>, + region: &'tcx ty::RegionKind, + ) -> BlockAnd> { + 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 index 3a6c7dc9754..d75b8d506e7 100644 --- 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 index cfdb8b0048a..24228389fbf 100644 --- 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, local_decls: IndexVec>, + upvar_decls: Vec, unit_temp: Option>, /// 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,10 +602,10 @@ 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> { @@ -610,7 +613,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { span: Span, arg_count: usize, safety: Safety, - return_ty: Ty<'tcx>) + return_ty: Ty<'tcx>, + upvar_decls: Vec) -> 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, yield_ty: Option>) -> 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 ) }