Skip to content

Commit

Permalink
Make 2PB reservations conflict with shared borrows
Browse files Browse the repository at this point in the history
Creating a mutable reference asserts uniqueness in certain potential
memory models for Rust. Make this an error for now.
  • Loading branch information
matthewjasper committed Nov 27, 2018
1 parent aeff91d commit a40646c
Show file tree
Hide file tree
Showing 29 changed files with 223 additions and 235 deletions.
1 change: 1 addition & 0 deletions src/librustc/ich/impls_mir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ impl_stable_hash_for!(struct mir::UnsafetyCheckResult { violations, unsafe_block
impl_stable_hash_for!(enum mir::BorrowKind {
Shared,
Shallow,
Guard,
Unique,
Mut { allow_two_phase_borrow },
});
Expand Down
45 changes: 37 additions & 8 deletions src/librustc/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -497,25 +497,50 @@ pub enum BorrowKind {

/// The immediately borrowed place must be immutable, but projections from
/// it don't need to be. For example, a shallow borrow of `a.b` doesn't
/// conflict with a mutable borrow of `a.b.c`.
/// conflict with a mutable borrow of `*(a.b)`.
///
/// This is used when lowering matches: when matching on a place we want to
/// ensure that place have the same value from the start of the match until
/// an arm is selected. This prevents this code from compiling:
/// an arm is selected. We create a shallow borrow of `x` for the following
/// match:
///
/// let mut x = &Some(0);
/// match *x {
/// None => (),
/// Some(_) if { x = &None; false } => (),
/// Some(_) if { x = &None; /* error */ false } => (),
/// Some(_) => (),
/// }
///
/// This can't be a shared borrow because mutably borrowing (*x as Some).0
/// should not prevent `if let None = x { ... }`, for example, because the
/// mutating `(*x as Some).0` can't affect the discriminant of `x`.
/// We can also report errors with this kind of borrow differently.
/// This can't be a shared borrow because mutably borrowing (*x).0
/// should not prevent `match (*x).1 { ... }`, for example, because the
/// mutating `(*x).0` can't affect `(*x).1`.
Shallow,

/// Same as `Shared`, but only exists to prevent mutation that could make
/// matches non-exhaustive. Removed after borrow checking.
///
/// This is used when lowering matches: when matching on a place we want to
/// ensure that place have the same value from the start of the match until
/// an arm is selected. We create a shallow borrow of `x` for the following
/// match:
///
/// let mut x = &Some(0);
/// match *x {
/// None => (),
/// Some(_) if { x = &None; false } => (),
/// Some(_) => (),
/// }
///
/// This can't be a `Shared` borrow because it doesn't conflict with the
/// two-phase borrow reservation in
///
/// match x { // Guard borrow here
/// Some(ref mut r) // reservation from borrow of x
/// if true => (),
/// _ => (),
/// }
Guard,

/// Data must be immutable but not aliasable. This kind of borrow
/// cannot currently be expressed by the user and is used only in
/// implicit closure bindings. It is needed when the closure is
Expand Down Expand Up @@ -564,7 +589,10 @@ pub enum BorrowKind {
impl BorrowKind {
pub fn allows_two_phase_borrow(&self) -> bool {
match *self {
BorrowKind::Shared | BorrowKind::Shallow | BorrowKind::Unique => false,
BorrowKind::Shared
| BorrowKind::Shallow
| BorrowKind::Guard
| BorrowKind::Unique => false,
BorrowKind::Mut { allow_two_phase_borrow } => allow_two_phase_borrow,
}
}
Expand Down Expand Up @@ -2314,6 +2342,7 @@ impl<'tcx> Debug for Rvalue<'tcx> {
let kind_str = match borrow_kind {
BorrowKind::Shared => "",
BorrowKind::Shallow => "shallow ",
BorrowKind::Guard => "guard ",
BorrowKind::Mut { .. } | BorrowKind::Unique => "mut ",
};

Expand Down
6 changes: 3 additions & 3 deletions src/librustc/mir/tcx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,9 +346,9 @@ impl BorrowKind {
// and hence is a safe "over approximation".
BorrowKind::Unique => hir::MutMutable,

// We have no type corresponding to a shallow borrow, so use
// `&` as an approximation.
BorrowKind::Shallow => hir::MutImmutable,
// We have no type corresponding to a shallow or guard borrows, so
// use `&` as an approximation.
BorrowKind::Guard | BorrowKind::Shallow => hir::MutImmutable,
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/librustc/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,9 @@ macro_rules! make_mir_visitor {
BorrowKind::Shallow => PlaceContext::NonMutatingUse(
NonMutatingUseContext::ShallowBorrow(*r)
),
BorrowKind::Guard => PlaceContext::NonMutatingUse(
NonMutatingUseContext::GuardBorrow(*r)
),
BorrowKind::Unique => PlaceContext::NonMutatingUse(
NonMutatingUseContext::UniqueBorrow(*r)
),
Expand Down Expand Up @@ -992,6 +995,8 @@ pub enum NonMutatingUseContext<'tcx> {
SharedBorrow(Region<'tcx>),
/// Shallow borrow.
ShallowBorrow(Region<'tcx>),
/// Guard borrow.
GuardBorrow(Region<'tcx>),
/// Unique borrow.
UniqueBorrow(Region<'tcx>),
/// Used as base for another place, e.g. `x` in `x.y`. Will not mutate the place.
Expand Down Expand Up @@ -1059,6 +1064,7 @@ impl<'tcx> PlaceContext<'tcx> {
match *self {
PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::GuardBorrow(..)) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow(..)) |
PlaceContext::MutatingUse(MutatingUseContext::Borrow(..)) => true,
_ => false,
Expand Down
1 change: 1 addition & 0 deletions src/librustc_codegen_ssa/mir/analyze.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ impl<'mir, 'a: 'mir, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx>
PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow(..)) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::GuardBorrow(..)) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) => {
self.not_ssa(local);
}
Expand Down
4 changes: 3 additions & 1 deletion src/librustc_mir/borrow_check/borrow_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ impl<'tcx> fmt::Display for BorrowData<'tcx> {
let kind = match self.kind {
mir::BorrowKind::Shared => "",
mir::BorrowKind::Shallow => "shallow ",
mir::BorrowKind::Guard => "guard ",
mir::BorrowKind::Unique => "uniq ",
mir::BorrowKind::Mut { .. } => "mut ",
};
Expand Down Expand Up @@ -280,7 +281,8 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
// The use of TMP in a shared borrow does not
// count as an actual activation.
PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) =>
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::GuardBorrow(..)) =>
TwoPhaseActivation::NotActivated,
_ => {
// Double check: This borrow is indeed a two-phase borrow (that is,
Expand Down
51 changes: 31 additions & 20 deletions src/librustc_mir/borrow_check/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
}

(BorrowKind::Mut { .. }, _, _, BorrowKind::Shallow, _, _)
| (BorrowKind::Unique, _, _, BorrowKind::Shallow, _, _) => {
| (BorrowKind::Unique, _, _, BorrowKind::Shallow, _, _)
| (BorrowKind::Mut { .. }, _, _, BorrowKind::Guard, _, _)
| (BorrowKind::Unique, _, _, BorrowKind::Guard, _, _) => {
let mut err = tcx.cannot_mutate_in_match_guard(
span,
issued_span,
Expand Down Expand Up @@ -472,17 +474,23 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
Origin::Mir,
)
}

(BorrowKind::Shallow, _, _, BorrowKind::Unique, _, _)
| (BorrowKind::Shallow, _, _, BorrowKind::Mut { .. }, _, _) => {
// Shallow borrows are uses from the user's point of view.
| (BorrowKind::Shallow, _, _, BorrowKind::Mut { .. }, _, _)
| (BorrowKind::Guard, _, _, BorrowKind::Unique, _, _)
| (BorrowKind::Guard, _, _, BorrowKind::Mut { .. }, _, _) => {
// Shallow and guard borrows are uses from the user's point of view.
self.report_use_while_mutably_borrowed(context, (place, span), issued_borrow);
return;
}
(BorrowKind::Shared, _, _, BorrowKind::Shared, _, _)
| (BorrowKind::Shared, _, _, BorrowKind::Shallow, _, _)
| (BorrowKind::Shared, _, _, BorrowKind::Guard, _, _)
| (BorrowKind::Shallow, _, _, BorrowKind::Shared, _, _)
| (BorrowKind::Shallow, _, _, BorrowKind::Shallow, _, _) => unreachable!(),
| (BorrowKind::Shallow, _, _, BorrowKind::Shallow, _, _)
| (BorrowKind::Shallow, _, _, BorrowKind::Guard, _, _)
| (BorrowKind::Guard, _, _, BorrowKind::Shared, _, _)
| (BorrowKind::Guard, _, _, BorrowKind::Shallow, _, _)
| (BorrowKind::Guard, _, _, BorrowKind::Guard, _, _) => unreachable!(),
};

if issued_spans == borrow_spans {
Expand Down Expand Up @@ -1208,21 +1216,24 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
let loan_span = loan_spans.args_or_use();

let tcx = self.infcx.tcx;
let mut err = if loan.kind == BorrowKind::Shallow {
tcx.cannot_mutate_in_match_guard(
span,
loan_span,
&self.describe_place(place).unwrap_or_else(|| "_".to_owned()),
"assign",
Origin::Mir,
)
} else {
tcx.cannot_assign_to_borrowed(
span,
loan_span,
&self.describe_place(place).unwrap_or_else(|| "_".to_owned()),
Origin::Mir,
)
let mut err = match loan.kind {
BorrowKind::Shallow | BorrowKind::Guard => {
tcx.cannot_mutate_in_match_guard(
span,
loan_span,
&self.describe_place(place).unwrap_or_else(|| "_".to_owned()),
"assign",
Origin::Mir,
)
}
BorrowKind::Shared | BorrowKind::Unique | BorrowKind::Mut { .. } => {
tcx.cannot_assign_to_borrowed(
span,
loan_span,
&self.describe_place(place).unwrap_or_else(|| "_".to_owned()),
Origin::Mir,
)
}
};

loan_spans.var_span_label(
Expand Down
46 changes: 34 additions & 12 deletions src/librustc_mir/borrow_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1008,12 +1008,27 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
Control::Continue
}

(Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared)
| (Read(_), BorrowKind::Shallow) | (Reservation(..), BorrowKind::Shallow) => {
(Read(_), BorrowKind::Shared)
| (Read(_), BorrowKind::Shallow)
| (Read(_), BorrowKind::Guard) => {
Control::Continue
}

(Write(WriteKind::Move), BorrowKind::Shallow) => {
(Reservation(..), BorrowKind::Shallow)
| (Reservation(..), BorrowKind::Guard) => {
// `Shallow` and `Guard` borrows only exist to prevent
// mutation, which a `Reservation` is not, even though it
// creates a mutable reference.
//
// We don't allow `Shared` borrows here, since it is
// undecided whether the mutable reference that is
// created by the `Reservation` should be unique when it's
// created.
Control::Continue
}

(Write(WriteKind::Move), BorrowKind::Shallow)
| (Write(WriteKind::Move), BorrowKind::Guard) => {
// Handled by initialization checks.
Control::Continue
}
Expand All @@ -1037,7 +1052,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
Control::Break
}

(Reservation(kind), BorrowKind::Unique)
(Reservation(kind), BorrowKind::Shared)
| (Reservation(kind), BorrowKind::Unique)
| (Reservation(kind), BorrowKind::Mut { .. })
| (Activation(kind, _), _)
| (Write(kind), _) => {
Expand Down Expand Up @@ -1149,7 +1165,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
BorrowKind::Shallow => {
(Shallow(Some(ArtificialField::ShallowBorrow)), Read(ReadKind::Borrow(bk)))
},
BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))),
BorrowKind::Shared | BorrowKind::Guard => (Deep, Read(ReadKind::Borrow(bk))),
BorrowKind::Unique | BorrowKind::Mut { .. } => {
let wk = WriteKind::MutableBorrow(bk);
if allow_two_phase_borrow(&self.infcx.tcx, bk) {
Expand All @@ -1168,10 +1184,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
flow_state,
);

let action = if bk == BorrowKind::Shallow {
InitializationRequiringAction::MatchOn
} else {
InitializationRequiringAction::Borrow
let action = match bk {
BorrowKind::Shallow | BorrowKind::Guard => {
InitializationRequiringAction::MatchOn
}
BorrowKind::Shared | BorrowKind::Unique | BorrowKind::Mut { .. } => {
InitializationRequiringAction::Borrow
}
};

self.check_if_path_or_subpath_is_moved(
Expand Down Expand Up @@ -1421,7 +1440,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {

// only mutable borrows should be 2-phase
assert!(match borrow.kind {
BorrowKind::Shared | BorrowKind::Shallow => false,
BorrowKind::Shared | BorrowKind::Shallow | BorrowKind::Guard => false,
BorrowKind::Unique | BorrowKind::Mut { .. } => true,
});

Expand Down Expand Up @@ -1832,7 +1851,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
let is_local_mutation_allowed = match borrow_kind {
BorrowKind::Unique => LocalMutationIsAllowed::Yes,
BorrowKind::Mut { .. } => is_local_mutation_allowed,
BorrowKind::Shared | BorrowKind::Shallow => unreachable!(),
BorrowKind::Shared | BorrowKind::Shallow | BorrowKind::Guard => unreachable!(),
};
match self.is_mutable(place, is_local_mutation_allowed) {
Ok(root_place) => {
Expand Down Expand Up @@ -1863,9 +1882,11 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
| Reservation(wk @ WriteKind::StorageDeadOrDrop)
| Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shared))
| Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shallow))
| Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Guard))
| Write(wk @ WriteKind::StorageDeadOrDrop)
| Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shared))
| Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shallow)) => {
| Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shallow))
| Write(wk @ WriteKind::MutableBorrow(BorrowKind::Guard)) => {
if let (Err(_place_err), true) = (
self.is_mutable(place, is_local_mutation_allowed),
self.errors_buffer.is_empty()
Expand Down Expand Up @@ -1911,6 +1932,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
| Read(ReadKind::Borrow(BorrowKind::Mut { .. }))
| Read(ReadKind::Borrow(BorrowKind::Shared))
| Read(ReadKind::Borrow(BorrowKind::Shallow))
| Read(ReadKind::Borrow(BorrowKind::Guard))
| Read(ReadKind::Copy) => {
// Access authorized
return false;
Expand Down
12 changes: 8 additions & 4 deletions src/librustc_mir/borrow_check/nll/invalidation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cx, 'tcx, 'gcx> {
BorrowKind::Shallow => {
(Shallow(Some(ArtificialField::ShallowBorrow)), Read(ReadKind::Borrow(bk)))
},
BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))),
BorrowKind::Guard | BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))),
BorrowKind::Unique | BorrowKind::Mut { .. } => {
let wk = WriteKind::MutableBorrow(bk);
if allow_two_phase_borrow(&self.tcx, bk) {
Expand Down Expand Up @@ -442,8 +442,11 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cx, 'tcx, 'gcx> {
// have already taken the reservation
}

(Read(_), BorrowKind::Shallow) | (Reservation(..), BorrowKind::Shallow)
| (Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared) => {
(Read(_), BorrowKind::Shallow)
| (Read(_), BorrowKind::Guard)
| (Read(_), BorrowKind::Shared)
| (Reservation(..), BorrowKind::Shallow)
| (Reservation(..), BorrowKind::Guard) => {
// Reads/reservations don't invalidate shared or shallow borrows
}

Expand All @@ -460,7 +463,8 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cx, 'tcx, 'gcx> {
this.generate_invalidates(borrow_index, context.loc);
}

(Reservation(_), BorrowKind::Unique)
(Reservation(..), BorrowKind::Shared)
| (Reservation(_), BorrowKind::Unique)
| (Reservation(_), BorrowKind::Mut { .. })
| (Activation(_, _), _)
| (Write(_), _) => {
Expand Down
Loading

0 comments on commit a40646c

Please sign in to comment.