From 34f56c44a9a9d027964fe992ce966ad407c1d9f3 Mon Sep 17 00:00:00 2001 From: Basile Desloges Date: Thu, 16 Nov 2017 17:44:24 +0100 Subject: [PATCH] mir-borrowck: Add permission check for `WriteKind::Mutate` --- src/librustc_mir/borrow_check.rs | 161 +++++++++++++++++++++++++------ 1 file changed, 131 insertions(+), 30 deletions(-) diff --git a/src/librustc_mir/borrow_check.rs b/src/librustc_mir/borrow_check.rs index d83656dc25ab..ec3954f73492 100644 --- a/src/librustc_mir/borrow_check.rs +++ b/src/librustc_mir/borrow_check.rs @@ -271,6 +271,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx self.access_lvalue(context, (output, span), (Deep, Read(ReadKind::Copy)), + LocalMutationIsAllowed::No, flow_state); self.check_if_path_is_moved(context, InitializationRequiringAction::Use, (output, span), flow_state); @@ -300,7 +301,9 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx StatementKind::StorageDead(local) => { self.access_lvalue(ContextKind::StorageDead.new(location), (&Lvalue::Local(local), span), - (Shallow(None), Write(WriteKind::StorageDeadOrDrop)), flow_state); + (Shallow(None), Write(WriteKind::StorageDeadOrDrop)), + LocalMutationIsAllowed::Yes, + flow_state); } } } @@ -322,6 +325,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx self.access_lvalue(ContextKind::Drop.new(loc), (drop_lvalue, span), (Deep, Write(WriteKind::StorageDeadOrDrop)), + LocalMutationIsAllowed::Yes, flow_state); } TerminatorKind::DropAndReplace { location: ref drop_lvalue, @@ -391,6 +395,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx ContextKind::StorageDead.new(loc), (&root_lvalue, self.mir.source_info(borrow.location).span), (Deep, Write(WriteKind::StorageDeadOrDrop)), + LocalMutationIsAllowed::Yes, flow_state ); } @@ -399,6 +404,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx ContextKind::StorageDead.new(loc), (&root_lvalue, self.mir.source_info(borrow.location).span), (Shallow(None), Write(WriteKind::StorageDeadOrDrop)), + LocalMutationIsAllowed::Yes, flow_state ); } @@ -445,6 +451,8 @@ enum ShallowOrDeep { Deep, } +/// Kind of access to a value: read or write +/// (For informational purposes only) #[derive(Copy, Clone, PartialEq, Eq, Debug)] enum ReadOrWrite { /// From the RFC: "A *read* means that the existing data may be @@ -457,12 +465,16 @@ enum ReadOrWrite { Write(WriteKind), } +/// Kind of read access to a value +/// (For informational purposes only) #[derive(Copy, Clone, PartialEq, Eq, Debug)] enum ReadKind { Borrow(BorrowKind), Copy, } +/// Kind of write access to a value +/// (For informational purposes only) #[derive(Copy, Clone, PartialEq, Eq, Debug)] enum WriteKind { StorageDeadOrDrop, @@ -471,6 +483,20 @@ enum WriteKind { Move, } +/// When checking permissions for an lvalue access, this flag is used to indicate that an immutable +/// local lvalue can be mutated. +/// +/// FIXME: @nikomatsakis suggested that this flag could be removed with the following modifications: +/// - Merge `check_access_permissions()` and `check_if_reassignment_to_immutable_state()` +/// - Split `is_mutable()` into `is_assignable()` (can be directly assigned) and +/// `is_declared_mutable()` +/// - Take flow state into consideration in `is_assignable()` for local variables +#[derive(Copy, Clone, PartialEq, Eq, Debug)] +enum LocalMutationIsAllowed { + Yes, + No +} + #[derive(Copy, Clone)] enum InitializationRequiringAction { Update, @@ -510,6 +536,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { context: Context, lvalue_span: (&Lvalue<'tcx>, Span), kind: (ShallowOrDeep, ReadOrWrite), + is_local_mutation_allowed: LocalMutationIsAllowed, flow_state: &InProgress<'cx, 'gcx, 'tcx>) { let (sd, rw) = kind; @@ -526,9 +553,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } // Check permissions - self.check_access_permissions(lvalue_span, rw); + let mut error_reported = self.check_access_permissions(lvalue_span, + rw, + is_local_mutation_allowed); - let mut error_reported = false; self.each_borrow_involving_path( context, (sd, lvalue_span.0), flow_state, |this, _index, borrow, common_prefix| { match (rw, borrow.kind) { @@ -614,7 +642,11 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } - self.access_lvalue(context, lvalue_span, (kind, Write(WriteKind::Mutate)), flow_state); + self.access_lvalue(context, + lvalue_span, + (kind, Write(WriteKind::Mutate)), + LocalMutationIsAllowed::Yes, + flow_state); // check for reassignments to immutable local variables self.check_if_reassignment_to_immutable_state(context, lvalue_span, flow_state); @@ -632,7 +664,11 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { BorrowKind::Unique | BorrowKind::Mut => (Deep, Write(WriteKind::MutableBorrow(bk))), }; - self.access_lvalue(context, (lvalue, span), access_kind, flow_state); + self.access_lvalue(context, + (lvalue, span), + access_kind, + LocalMutationIsAllowed::No, + flow_state); self.check_if_path_is_moved(context, InitializationRequiringAction::Borrow, (lvalue, span), flow_state); } @@ -651,8 +687,11 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Rvalue::Discriminant(..) => ArtificialField::Discriminant, _ => unreachable!(), }; - self.access_lvalue( - context, (lvalue, span), (Shallow(Some(af)), Read(ReadKind::Copy)), flow_state); + self.access_lvalue(context, + (lvalue, span), + (Shallow(Some(af)), Read(ReadKind::Copy)), + LocalMutationIsAllowed::No, + flow_state); self.check_if_path_is_moved(context, InitializationRequiringAction::Use, (lvalue, span), flow_state); } @@ -690,6 +729,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { self.access_lvalue(context, (lvalue, span), (Deep, Read(ReadKind::Copy)), + LocalMutationIsAllowed::No, flow_state); // Finally, check if path was already moved. @@ -701,6 +741,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { self.access_lvalue(context, (lvalue, span), (Deep, Write(WriteKind::Move)), + LocalMutationIsAllowed::Yes, flow_state); // Finally, check if path was already moved. @@ -735,9 +776,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } Lvalue::Static(ref static_) => { // mutation of non-mut static is always illegal, - // independent of dataflow. + // 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) { - self.report_assignment_to_static(context, (lvalue, span)); + 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; } @@ -949,41 +998,101 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } /// Check the permissions for the given lvalue and read or write kind - fn check_access_permissions(&self, (lvalue, span): (&Lvalue<'tcx>, Span), kind: ReadOrWrite) { + /// + /// Returns true if an error is reported, false otherwise. + fn check_access_permissions(&self, + (lvalue, span): (&Lvalue<'tcx>, Span), + kind: ReadOrWrite, + is_local_mutation_allowed: LocalMutationIsAllowed) + -> bool { + debug!("check_access_permissions({:?}, {:?}, {:?})", + lvalue, kind, is_local_mutation_allowed); + let mut error_reported = false; match kind { Write(WriteKind::MutableBorrow(BorrowKind::Unique)) => { if let Err(_lvalue_err) = self.is_unique(lvalue) { - span_bug!(span, "&unique borrow for `{}` should not fail", - self.describe_lvalue(lvalue)); + span_bug!(span, "&unique borrow for {:?} should not fail", lvalue); } }, Write(WriteKind::MutableBorrow(BorrowKind::Mut)) => { - if let Err(lvalue_err) = self.is_mutable(lvalue) { + if let Err(lvalue_err) = self.is_mutable(lvalue, is_local_mutation_allowed) { + error_reported = true; + + let item_msg = match self.describe_lvalue(lvalue) { + Some(name) => format!("immutable item `{}`", name), + None => "immutable item".to_owned() + }; + let mut err = self.tcx.cannot_borrow_path_as_mutable(span, - &format!("immutable item `{}`", - self.describe_lvalue(lvalue)), + &item_msg, Origin::Mir); err.span_label(span, "cannot borrow as mutable"); if lvalue != lvalue_err { - err.note(&format!("Value not mutable causing this error: `{}`", - self.describe_lvalue(lvalue_err))); + if let Some(name) = self.describe_lvalue(lvalue_err) { + err.note(&format!("Value not mutable causing this error: `{}`", name)); + } } err.emit(); } }, - _ => {}// Access authorized + Write(WriteKind::Mutate) => { + if let Err(lvalue_err) = self.is_mutable(lvalue, is_local_mutation_allowed) { + error_reported = true; + + let item_msg = match self.describe_lvalue(lvalue) { + Some(name) => format!("immutable item `{}`", name), + None => "immutable item".to_owned() + }; + + let mut err = self.tcx.cannot_assign(span, + &item_msg, + Origin::Mir); + err.span_label(span, "cannot mutate"); + + if lvalue != lvalue_err { + if let Some(name) = self.describe_lvalue(lvalue_err) { + err.note(&format!("Value not mutable causing this error: `{}`", name)); + } + } + + err.emit(); + } + }, + Write(WriteKind::Move) | + Write(WriteKind::StorageDeadOrDrop) | + Write(WriteKind::MutableBorrow(BorrowKind::Shared)) => { + if let Err(_lvalue_err) = self.is_mutable(lvalue, is_local_mutation_allowed) { + self.tcx.sess.delay_span_bug(span, + &format!("Accessing `{:?}` with the kind `{:?}` shouldn't be possible", + lvalue, + kind)); + } + }, + Read(ReadKind::Borrow(BorrowKind::Unique)) | + Read(ReadKind::Borrow(BorrowKind::Mut)) | + Read(ReadKind::Borrow(BorrowKind::Shared)) | + Read(ReadKind::Copy) => {} // Access authorized } + + error_reported } /// Can this value be written or borrowed mutably - fn is_mutable<'d>(&self, lvalue: &'d Lvalue<'tcx>) -> Result<(), &'d Lvalue<'tcx>> { + fn is_mutable<'d>(&self, + lvalue: &'d Lvalue<'tcx>, + is_local_mutation_allowed: LocalMutationIsAllowed) + -> Result<(), &'d Lvalue<'tcx>> { match *lvalue { Lvalue::Local(local) => { let local = &self.mir.local_decls[local]; match local.mutability { - Mutability::Not => Err(lvalue), + Mutability::Not => + match is_local_mutation_allowed { + LocalMutationIsAllowed::Yes => Ok(()), + LocalMutationIsAllowed::No => Err(lvalue), + }, Mutability::Mut => Ok(()) } }, @@ -1001,7 +1110,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // `Box` owns its content, so mutable if its location is mutable if base_ty.is_box() { - return self.is_mutable(&proj.base); + return self.is_mutable(&proj.base, LocalMutationIsAllowed::No); } // Otherwise we check the kind of deref to decide @@ -1035,7 +1144,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ProjectionElem::ConstantIndex{..} | ProjectionElem::Subslice{..} | ProjectionElem::Downcast(..) => - self.is_mutable(&proj.base) + self.is_mutable(&proj.base, LocalMutationIsAllowed::No) } } } @@ -1604,14 +1713,6 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } err.emit(); } - - fn report_assignment_to_static(&mut self, - _context: Context, - (lvalue, span): (&Lvalue<'tcx>, Span)) { - let mut err = self.tcx.cannot_assign_static( - span, &self.describe_lvalue(lvalue), Origin::Mir); - err.emit(); - } } impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {