Skip to content

[WIP] Make 2PB reservations conflict with shared borrows #56301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
3 changes: 2 additions & 1 deletion src/librustc_codegen_llvm/abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,8 @@ impl ArgTypeExt<'ll, 'tcx> for ArgType<'tcx, Ty<'tcx>> {
OperandValue::Ref(next(), Some(next()), self.layout.align.abi).store(bx, dst);
}
PassMode::Direct(_) | PassMode::Indirect(_, None) | PassMode::Cast(_) => {
self.store(bx, next(), dst);
let next_param = next();
self.store(bx, next_param, dst);
}
}
}
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