Skip to content

Commit a40646c

Browse files
matthewjaspermatthewjasper
committed
Make 2PB reservations conflict with shared borrows
Creating a mutable reference asserts uniqueness in certain potential memory models for Rust. Make this an error for now.
1 parent aeff91d commit a40646c

29 files changed

+223
-235
lines changed

‎src/librustc/ich/impls_mir.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ impl_stable_hash_for!(struct mir::UnsafetyCheckResult { violations, unsafe_block
4040
impl_stable_hash_for!(enum mir::BorrowKind {
4141
Shared,
4242
Shallow,
43+
Guard,
4344
Unique,
4445
Mut { allow_two_phase_borrow },
4546
});

‎src/librustc/mir/mod.rs

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -497,25 +497,50 @@ pub enum BorrowKind {
497497

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

519+
/// Same as `Shared`, but only exists to prevent mutation that could make
520+
/// matches non-exhaustive. Removed after borrow checking.
521+
///
522+
/// This is used when lowering matches: when matching on a place we want to
523+
/// ensure that place have the same value from the start of the match until
524+
/// an arm is selected. We create a shallow borrow of `x` for the following
525+
/// match:
526+
///
527+
/// let mut x = &Some(0);
528+
/// match *x {
529+
/// None => (),
530+
/// Some(_) if { x = &None; false } => (),
531+
/// Some(_) => (),
532+
/// }
533+
///
534+
/// This can't be a `Shared` borrow because it doesn't conflict with the
535+
/// two-phase borrow reservation in
536+
///
537+
/// match x { // Guard borrow here
538+
/// Some(ref mut r) // reservation from borrow of x
539+
/// if true => (),
540+
/// _ => (),
541+
/// }
542+
Guard,
543+
519544
/// Data must be immutable but not aliasable. This kind of borrow
520545
/// cannot currently be expressed by the user and is used only in
521546
/// implicit closure bindings. It is needed when the closure is
@@ -564,7 +589,10 @@ pub enum BorrowKind {
564589
impl BorrowKind {
565590
pub fn allows_two_phase_borrow(&self) -> bool {
566591
match *self {
567-
BorrowKind::Shared | BorrowKind::Shallow | BorrowKind::Unique => false,
592+
BorrowKind::Shared
593+
| BorrowKind::Shallow
594+
| BorrowKind::Guard
595+
| BorrowKind::Unique => false,
568596
BorrowKind::Mut { allow_two_phase_borrow } => allow_two_phase_borrow,
569597
}
570598
}
@@ -2314,6 +2342,7 @@ impl<'tcx> Debug for Rvalue<'tcx> {
23142342
let kind_str = match borrow_kind {
23152343
BorrowKind::Shared => "",
23162344
BorrowKind::Shallow => "shallow ",
2345+
BorrowKind::Guard => "guard ",
23172346
BorrowKind::Mut { .. } | BorrowKind::Unique => "mut ",
23182347
};
23192348

‎src/librustc/mir/tcx.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -346,9 +346,9 @@ impl BorrowKind {
346346
// and hence is a safe "over approximation".
347347
BorrowKind::Unique => hir::MutMutable,
348348

349-
// We have no type corresponding to a shallow borrow, so use
350-
// `&` as an approximation.
351-
BorrowKind::Shallow => hir::MutImmutable,
349+
// We have no type corresponding to a shallow or guard borrows, so
350+
// use `&` as an approximation.
351+
BorrowKind::Guard | BorrowKind::Shallow => hir::MutImmutable,
352352
}
353353
}
354354
}

‎src/librustc/mir/visit.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,9 @@ macro_rules! make_mir_visitor {
599599
BorrowKind::Shallow => PlaceContext::NonMutatingUse(
600600
NonMutatingUseContext::ShallowBorrow(*r)
601601
),
602+
BorrowKind::Guard => PlaceContext::NonMutatingUse(
603+
NonMutatingUseContext::GuardBorrow(*r)
604+
),
602605
BorrowKind::Unique => PlaceContext::NonMutatingUse(
603606
NonMutatingUseContext::UniqueBorrow(*r)
604607
),
@@ -992,6 +995,8 @@ pub enum NonMutatingUseContext<'tcx> {
992995
SharedBorrow(Region<'tcx>),
993996
/// Shallow borrow.
994997
ShallowBorrow(Region<'tcx>),
998+
/// Guard borrow.
999+
GuardBorrow(Region<'tcx>),
9951000
/// Unique borrow.
9961001
UniqueBorrow(Region<'tcx>),
9971002
/// Used as base for another place, e.g. `x` in `x.y`. Will not mutate the place.
@@ -1059,6 +1064,7 @@ impl<'tcx> PlaceContext<'tcx> {
10591064
match *self {
10601065
PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) |
10611066
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) |
1067+
PlaceContext::NonMutatingUse(NonMutatingUseContext::GuardBorrow(..)) |
10621068
PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow(..)) |
10631069
PlaceContext::MutatingUse(MutatingUseContext::Borrow(..)) => true,
10641070
_ => false,

‎src/librustc_codegen_ssa/mir/analyze.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,7 @@ impl<'mir, 'a: 'mir, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx>
250250
PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) |
251251
PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow(..)) |
252252
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) |
253+
PlaceContext::NonMutatingUse(NonMutatingUseContext::GuardBorrow(..)) |
253254
PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) => {
254255
self.not_ssa(local);
255256
}

‎src/librustc_mir/borrow_check/borrow_set.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ impl<'tcx> fmt::Display for BorrowData<'tcx> {
9090
let kind = match self.kind {
9191
mir::BorrowKind::Shared => "",
9292
mir::BorrowKind::Shallow => "shallow ",
93+
mir::BorrowKind::Guard => "guard ",
9394
mir::BorrowKind::Unique => "uniq ",
9495
mir::BorrowKind::Mut { .. } => "mut ",
9596
};
@@ -280,7 +281,8 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
280281
// The use of TMP in a shared borrow does not
281282
// count as an actual activation.
282283
PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) |
283-
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) =>
284+
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) |
285+
PlaceContext::NonMutatingUse(NonMutatingUseContext::GuardBorrow(..)) =>
284286
TwoPhaseActivation::NotActivated,
285287
_ => {
286288
// Double check: This borrow is indeed a two-phase borrow (that is,

‎src/librustc_mir/borrow_check/error_reporting.rs

Lines changed: 31 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -409,7 +409,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
409409
}
410410

411411
(BorrowKind::Mut { .. }, _, _, BorrowKind::Shallow, _, _)
412-
| (BorrowKind::Unique, _, _, BorrowKind::Shallow, _, _) => {
412+
| (BorrowKind::Unique, _, _, BorrowKind::Shallow, _, _)
413+
| (BorrowKind::Mut { .. }, _, _, BorrowKind::Guard, _, _)
414+
| (BorrowKind::Unique, _, _, BorrowKind::Guard, _, _) => {
413415
let mut err = tcx.cannot_mutate_in_match_guard(
414416
span,
415417
issued_span,
@@ -472,17 +474,23 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
472474
Origin::Mir,
473475
)
474476
}
475-
476477
(BorrowKind::Shallow, _, _, BorrowKind::Unique, _, _)
477-
| (BorrowKind::Shallow, _, _, BorrowKind::Mut { .. }, _, _) => {
478-
// Shallow borrows are uses from the user's point of view.
478+
| (BorrowKind::Shallow, _, _, BorrowKind::Mut { .. }, _, _)
479+
| (BorrowKind::Guard, _, _, BorrowKind::Unique, _, _)
480+
| (BorrowKind::Guard, _, _, BorrowKind::Mut { .. }, _, _) => {
481+
// Shallow and guard borrows are uses from the user's point of view.
479482
self.report_use_while_mutably_borrowed(context, (place, span), issued_borrow);
480483
return;
481484
}
482485
(BorrowKind::Shared, _, _, BorrowKind::Shared, _, _)
483486
| (BorrowKind::Shared, _, _, BorrowKind::Shallow, _, _)
487+
| (BorrowKind::Shared, _, _, BorrowKind::Guard, _, _)
484488
| (BorrowKind::Shallow, _, _, BorrowKind::Shared, _, _)
485-
| (BorrowKind::Shallow, _, _, BorrowKind::Shallow, _, _) => unreachable!(),
489+
| (BorrowKind::Shallow, _, _, BorrowKind::Shallow, _, _)
490+
| (BorrowKind::Shallow, _, _, BorrowKind::Guard, _, _)
491+
| (BorrowKind::Guard, _, _, BorrowKind::Shared, _, _)
492+
| (BorrowKind::Guard, _, _, BorrowKind::Shallow, _, _)
493+
| (BorrowKind::Guard, _, _, BorrowKind::Guard, _, _) => unreachable!(),
486494
};
487495

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

12101218
let tcx = self.infcx.tcx;
1211-
let mut err = if loan.kind == BorrowKind::Shallow {
1212-
tcx.cannot_mutate_in_match_guard(
1213-
span,
1214-
loan_span,
1215-
&self.describe_place(place).unwrap_or_else(|| "_".to_owned()),
1216-
"assign",
1217-
Origin::Mir,
1218-
)
1219-
} else {
1220-
tcx.cannot_assign_to_borrowed(
1221-
span,
1222-
loan_span,
1223-
&self.describe_place(place).unwrap_or_else(|| "_".to_owned()),
1224-
Origin::Mir,
1225-
)
1219+
let mut err = match loan.kind {
1220+
BorrowKind::Shallow | BorrowKind::Guard => {
1221+
tcx.cannot_mutate_in_match_guard(
1222+
span,
1223+
loan_span,
1224+
&self.describe_place(place).unwrap_or_else(|| "_".to_owned()),
1225+
"assign",
1226+
Origin::Mir,
1227+
)
1228+
}
1229+
BorrowKind::Shared | BorrowKind::Unique | BorrowKind::Mut { .. } => {
1230+
tcx.cannot_assign_to_borrowed(
1231+
span,
1232+
loan_span,
1233+
&self.describe_place(place).unwrap_or_else(|| "_".to_owned()),
1234+
Origin::Mir,
1235+
)
1236+
}
12261237
};
12271238

12281239
loan_spans.var_span_label(

‎src/librustc_mir/borrow_check/mod.rs

Lines changed: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1008,12 +1008,27 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
10081008
Control::Continue
10091009
}
10101010

1011-
(Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared)
1012-
| (Read(_), BorrowKind::Shallow) | (Reservation(..), BorrowKind::Shallow) => {
1011+
(Read(_), BorrowKind::Shared)
1012+
| (Read(_), BorrowKind::Shallow)
1013+
| (Read(_), BorrowKind::Guard) => {
10131014
Control::Continue
10141015
}
10151016

1016-
(Write(WriteKind::Move), BorrowKind::Shallow) => {
1017+
(Reservation(..), BorrowKind::Shallow)
1018+
| (Reservation(..), BorrowKind::Guard) => {
1019+
// `Shallow` and `Guard` borrows only exist to prevent
1020+
// mutation, which a `Reservation` is not, even though it
1021+
// creates a mutable reference.
1022+
//
1023+
// We don't allow `Shared` borrows here, since it is
1024+
// undecided whether the mutable reference that is
1025+
// created by the `Reservation` should be unique when it's
1026+
// created.
1027+
Control::Continue
1028+
}
1029+
1030+
(Write(WriteKind::Move), BorrowKind::Shallow)
1031+
| (Write(WriteKind::Move), BorrowKind::Guard) => {
10171032
// Handled by initialization checks.
10181033
Control::Continue
10191034
}
@@ -1037,7 +1052,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
10371052
Control::Break
10381053
}
10391054

1040-
(Reservation(kind), BorrowKind::Unique)
1055+
(Reservation(kind), BorrowKind::Shared)
1056+
| (Reservation(kind), BorrowKind::Unique)
10411057
| (Reservation(kind), BorrowKind::Mut { .. })
10421058
| (Activation(kind, _), _)
10431059
| (Write(kind), _) => {
@@ -1149,7 +1165,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
11491165
BorrowKind::Shallow => {
11501166
(Shallow(Some(ArtificialField::ShallowBorrow)), Read(ReadKind::Borrow(bk)))
11511167
},
1152-
BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))),
1168+
BorrowKind::Shared | BorrowKind::Guard => (Deep, Read(ReadKind::Borrow(bk))),
11531169
BorrowKind::Unique | BorrowKind::Mut { .. } => {
11541170
let wk = WriteKind::MutableBorrow(bk);
11551171
if allow_two_phase_borrow(&self.infcx.tcx, bk) {
@@ -1168,10 +1184,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
11681184
flow_state,
11691185
);
11701186

1171-
let action = if bk == BorrowKind::Shallow {
1172-
InitializationRequiringAction::MatchOn
1173-
} else {
1174-
InitializationRequiringAction::Borrow
1187+
let action = match bk {
1188+
BorrowKind::Shallow | BorrowKind::Guard => {
1189+
InitializationRequiringAction::MatchOn
1190+
}
1191+
BorrowKind::Shared | BorrowKind::Unique | BorrowKind::Mut { .. } => {
1192+
InitializationRequiringAction::Borrow
1193+
}
11751194
};
11761195

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

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

@@ -1832,7 +1851,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
18321851
let is_local_mutation_allowed = match borrow_kind {
18331852
BorrowKind::Unique => LocalMutationIsAllowed::Yes,
18341853
BorrowKind::Mut { .. } => is_local_mutation_allowed,
1835-
BorrowKind::Shared | BorrowKind::Shallow => unreachable!(),
1854+
BorrowKind::Shared | BorrowKind::Shallow | BorrowKind::Guard => unreachable!(),
18361855
};
18371856
match self.is_mutable(place, is_local_mutation_allowed) {
18381857
Ok(root_place) => {
@@ -1863,9 +1882,11 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
18631882
| Reservation(wk @ WriteKind::StorageDeadOrDrop)
18641883
| Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shared))
18651884
| Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shallow))
1885+
| Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Guard))
18661886
| Write(wk @ WriteKind::StorageDeadOrDrop)
18671887
| Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shared))
1868-
| Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shallow)) => {
1888+
| Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shallow))
1889+
| Write(wk @ WriteKind::MutableBorrow(BorrowKind::Guard)) => {
18691890
if let (Err(_place_err), true) = (
18701891
self.is_mutable(place, is_local_mutation_allowed),
18711892
self.errors_buffer.is_empty()
@@ -1911,6 +1932,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
19111932
| Read(ReadKind::Borrow(BorrowKind::Mut { .. }))
19121933
| Read(ReadKind::Borrow(BorrowKind::Shared))
19131934
| Read(ReadKind::Borrow(BorrowKind::Shallow))
1935+
| Read(ReadKind::Borrow(BorrowKind::Guard))
19141936
| Read(ReadKind::Copy) => {
19151937
// Access authorized
19161938
return false;

‎src/librustc_mir/borrow_check/nll/invalidation.rs

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,7 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cx, 'tcx, 'gcx> {
332332
BorrowKind::Shallow => {
333333
(Shallow(Some(ArtificialField::ShallowBorrow)), Read(ReadKind::Borrow(bk)))
334334
},
335-
BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))),
335+
BorrowKind::Guard | BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))),
336336
BorrowKind::Unique | BorrowKind::Mut { .. } => {
337337
let wk = WriteKind::MutableBorrow(bk);
338338
if allow_two_phase_borrow(&self.tcx, bk) {
@@ -442,8 +442,11 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cx, 'tcx, 'gcx> {
442442
// have already taken the reservation
443443
}
444444

445-
(Read(_), BorrowKind::Shallow) | (Reservation(..), BorrowKind::Shallow)
446-
| (Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared) => {
445+
(Read(_), BorrowKind::Shallow)
446+
| (Read(_), BorrowKind::Guard)
447+
| (Read(_), BorrowKind::Shared)
448+
| (Reservation(..), BorrowKind::Shallow)
449+
| (Reservation(..), BorrowKind::Guard) => {
447450
// Reads/reservations don't invalidate shared or shallow borrows
448451
}
449452

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

463-
(Reservation(_), BorrowKind::Unique)
466+
(Reservation(..), BorrowKind::Shared)
467+
| (Reservation(_), BorrowKind::Unique)
464468
| (Reservation(_), BorrowKind::Mut { .. })
465469
| (Activation(_, _), _)
466470
| (Write(_), _) => {

0 commit comments

Comments
 (0)