From dd6b0059265c99de3bc9c9b083966b5e3a723517 Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 15 Jan 2018 10:44:13 +0000 Subject: [PATCH 1/7] Added test for #45697 --- src/test/ui/issue-45697.rs | 35 ++++++++++++++++++++++++++++++++++ src/test/ui/issue-45697.stderr | 18 +++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 src/test/ui/issue-45697.rs create mode 100644 src/test/ui/issue-45697.stderr diff --git a/src/test/ui/issue-45697.rs b/src/test/ui/issue-45697.rs new file mode 100644 index 0000000000000..7f44209d717a1 --- /dev/null +++ b/src/test/ui/issue-45697.rs @@ -0,0 +1,35 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Test that assignments to an `&mut` pointer which is found in a +// borrowed (but otherwise non-aliasable) location is illegal. + +// compile-flags: -Z emit-end-regions -Z borrowck=compare + +struct S<'a> { + pointer: &'a mut isize +} + +fn copy_borrowed_ptr<'a>(p: &'a mut S<'a>) -> S<'a> { + S { pointer: &mut *p.pointer } +} + +fn main() { + let mut x = 1; + + { + let mut y = S { pointer: &mut x }; + let z = copy_borrowed_ptr(&mut y); + *y.pointer += 1; + //~^ ERROR cannot assign to `*y.pointer` because it is borrowed (Ast) [E0506] + //~| ERROR cannot assign to `*y.pointer` because it is borrowed (Mir) [E0506] + *z.pointer += 1; + } +} diff --git a/src/test/ui/issue-45697.stderr b/src/test/ui/issue-45697.stderr new file mode 100644 index 0000000000000..007bfbfc9b01e --- /dev/null +++ b/src/test/ui/issue-45697.stderr @@ -0,0 +1,18 @@ +error[E0506]: cannot assign to `*y.pointer` because it is borrowed (Ast) + --> $DIR/issue-45697.rs:30:9 + | +29 | let z = copy_borrowed_ptr(&mut y); + | - borrow of `*y.pointer` occurs here +30 | *y.pointer += 1; + | ^^^^^^^^^^^^^^^ assignment to borrowed `*y.pointer` occurs here + +error[E0506]: cannot assign to `*y.pointer` because it is borrowed (Mir) + --> $DIR/issue-45697.rs:30:9 + | +29 | let z = copy_borrowed_ptr(&mut y); + | ------ borrow of `*y.pointer` occurs here +30 | *y.pointer += 1; + | ^^^^^^^^^^^^^^^ assignment to borrowed `*y.pointer` occurs here + +error: aborting due to 2 previous errors + From f1c1db61e454a6be93a2706ad15a09aad161613f Mon Sep 17 00:00:00 2001 From: David Wood Date: Fri, 19 Jan 2018 22:04:01 +0000 Subject: [PATCH 2/7] Updated other affected tests. --- .../compile-fail/nll/reference-carried-through-struct-field.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/compile-fail/nll/reference-carried-through-struct-field.rs b/src/test/compile-fail/nll/reference-carried-through-struct-field.rs index 1c1fc4799c3d1..e64fecbe701e7 100644 --- a/src/test/compile-fail/nll/reference-carried-through-struct-field.rs +++ b/src/test/compile-fail/nll/reference-carried-through-struct-field.rs @@ -20,7 +20,6 @@ fn foo() { let wrapper = Wrap { w: &mut x }; x += 1; //[ast]~ ERROR cannot assign to `x` because it is borrowed [E0506] //[mir]~^ ERROR cannot assign to `x` because it is borrowed [E0506] - //[mir]~^^ ERROR cannot use `x` because it was mutably borrowed [E0503] *wrapper.w += 1; } From 3daa4d255f1ecc9d0a28deb9ca3d6da79f1df438 Mon Sep 17 00:00:00 2001 From: David Wood Date: Fri, 19 Jan 2018 22:11:59 +0000 Subject: [PATCH 3/7] Introduced a new set to stop duplicate errors from MIR passes on one place/span. --- src/librustc_mir/borrow_check/mod.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index d6937c405f961..1d630b280a590 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -230,6 +230,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( }, storage_dead_or_drop_error_reported_l: FxHashSet(), storage_dead_or_drop_error_reported_s: FxHashSet(), + read_or_write_error_reported: FxHashSet(), reservation_error_reported: FxHashSet(), nonlexical_regioncx: opt_regioncx.clone(), }; @@ -300,6 +301,9 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> { storage_dead_or_drop_error_reported_l: FxHashSet, /// Same as the above, but for statics (thread-locals) storage_dead_or_drop_error_reported_s: FxHashSet, + /// This field keeps track of when borrow errors are reported in read or write passes + /// so that an error is not reported in both. + read_or_write_error_reported: FxHashSet<(Place<'tcx>, Span)>, /// This field keeps track of when borrow conflict errors are reported /// for reservations, so that we don't report seemingly duplicate /// errors for corresponding activations @@ -739,11 +743,23 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } + if self.read_or_write_error_reported.contains(&(place_span.0.clone(), place_span.1)) { + debug!("suppressing access_place write for {:?}", place_span); + return AccessErrorsReported { + mutability_error: false, + conflict_error: true, + }; + } + let mutability_error = self.check_access_permissions(place_span, rw, is_local_mutation_allowed); let conflict_error = self.check_access_for_conflict(context, place_span, sd, rw, flow_state); + if conflict_error { + self.read_or_write_error_reported.insert((place_span.0.clone(), place_span.1)); + } + AccessErrorsReported { mutability_error, conflict_error, From f92d679b78fc1e5db5d5fd6cbb9a5e076f8fee69 Mon Sep 17 00:00:00 2001 From: David Wood Date: Sat, 20 Jan 2018 02:15:57 +0000 Subject: [PATCH 4/7] Encompassed error deduplication of some existing sets in the ctxt. --- .../borrow_check/error_reporting.rs | 29 ++++--------- src/librustc_mir/borrow_check/mod.rs | 43 ++++++++----------- 2 files changed, 25 insertions(+), 47 deletions(-) diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 7bd3b6e39f053..8ded245e13ff5 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -362,33 +362,20 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { let scope_tree = borrows.0.scope_tree(); let root_place = self.prefixes(&borrow.borrowed_place, PrefixSet::All).last().unwrap(); - match root_place { - &Place::Local(local) => { - if let Some(_) = self.storage_dead_or_drop_error_reported_l.replace(local) { - debug!("report_does_not_live_long_enough({:?}): ", - (borrow, drop_span)); - return - } - } - &Place::Static(ref statik) => { - if let Some(_) = self.storage_dead_or_drop_error_reported_s - .replace(statik.def_id) - { - debug!("report_does_not_live_long_enough({:?}): ", - (borrow, drop_span)); - return - } - }, - &Place::Projection(_) => - unreachable!("root_place is an unreachable???") - }; - let borrow_span = self.mir.source_info(borrow.location).span; let proper_span = match *root_place { Place::Local(local) => self.mir.local_decls[local].source_info.span, _ => drop_span, }; + if self.access_place_error_reported.contains(&(root_place.clone(), borrow_span)) { + debug!("suppressing access_place error when borrow doesn't live long enough for {:?}", + borrow_span); + return; + } + + self.access_place_error_reported.insert((root_place.clone(), borrow_span)); + match (borrow.region, &self.describe_place(&borrow.borrowed_place)) { (RegionKind::ReScope(_), Some(name)) => { self.report_scoped_local_value_does_not_live_long_enough( diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 1d630b280a590..6b84abbe266fa 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -17,7 +17,7 @@ use rustc::hir::map::definitions::DefPathData; use rustc::infer::InferCtxt; use rustc::ty::{self, ParamEnv, TyCtxt}; use rustc::ty::maps::Providers; -use rustc::mir::{AssertMessage, BasicBlock, BorrowKind, Local, Location, Place}; +use rustc::mir::{AssertMessage, BasicBlock, BorrowKind, Location, Place}; use rustc::mir::{Mir, Mutability, Operand, Projection, ProjectionElem, Rvalue}; use rustc::mir::{Field, Statement, StatementKind, Terminator, TerminatorKind}; use rustc::mir::ClosureRegionRequirements; @@ -228,9 +228,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(_) => false, hir::BodyOwnerKind::Fn => true, }, - storage_dead_or_drop_error_reported_l: FxHashSet(), - storage_dead_or_drop_error_reported_s: FxHashSet(), - read_or_write_error_reported: FxHashSet(), + access_place_error_reported: FxHashSet(), reservation_error_reported: FxHashSet(), nonlexical_regioncx: opt_regioncx.clone(), }; @@ -295,15 +293,12 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> { /// I'm not sure this is the right approach - @eddyb could you try and /// figure this out? locals_are_invalidated_at_exit: bool, - /// This field keeps track of when storage dead or drop errors are reported - /// in order to stop duplicate error reporting and identify the conditions required - /// for a "temporary value dropped here while still borrowed" error. See #45360. - storage_dead_or_drop_error_reported_l: FxHashSet, - /// Same as the above, but for statics (thread-locals) - storage_dead_or_drop_error_reported_s: FxHashSet, - /// This field keeps track of when borrow errors are reported in read or write passes - /// so that an error is not reported in both. - read_or_write_error_reported: FxHashSet<(Place<'tcx>, Span)>, + /// This field keeps track of when borrow errors are reported in the access_place function + /// so that there is no duplicate reporting. This field cannot also be used for the conflicting + /// borrow errors that is handled by the `reservation_error_reported` field as the inclusion + /// of the `Span` type (while required to mute some errors) stops the muting of the reservation + /// errors. + access_place_error_reported: FxHashSet<(Place<'tcx>, Span)>, /// This field keeps track of when borrow conflict errors are reported /// for reservations, so that we don't report seemingly duplicate /// errors for corresponding activations @@ -730,12 +725,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { if let Activation(_, borrow_index) = rw { if self.reservation_error_reported.contains(&place_span.0) { - debug!( - "skipping access_place for activation of invalid reservation \ - place: {:?} borrow_index: {:?}", - place_span.0, - borrow_index - ); + debug!("skipping access_place for activation of invalid reservation \ + place: {:?} borrow_index: {:?}", place_span.0, borrow_index); return AccessErrorsReported { mutability_error: false, conflict_error: true, @@ -743,8 +734,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } - if self.read_or_write_error_reported.contains(&(place_span.0.clone(), place_span.1)) { - debug!("suppressing access_place write for {:?}", place_span); + if self.access_place_error_reported.contains(&(place_span.0.clone(), place_span.1)) { + debug!("suppressing access_place error for {:?}", place_span); return AccessErrorsReported { mutability_error: false, conflict_error: true, @@ -756,8 +747,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { let conflict_error = self.check_access_for_conflict(context, place_span, sd, rw, flow_state); - if conflict_error { - self.read_or_write_error_reported.insert((place_span.0.clone(), place_span.1)); + if conflict_error || mutability_error { + self.access_place_error_reported.insert((place_span.0.clone(), place_span.1)); } AccessErrorsReported { @@ -845,15 +836,15 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { place_span.0 ); this.reservation_error_reported.insert(place_span.0.clone()); - } + }, Activation(_, activating) => { debug!( "observing check_place for activation of \ borrow_index: {:?}", activating ); - } - Read(..) | Write(..) => {} + }, + Read(..) | Write(..) => {}, } match kind { From 970fb1a77f31e71e16871d22547243f1c98e6d66 Mon Sep 17 00:00:00 2001 From: David Wood Date: Sat, 27 Jan 2018 17:16:13 +0000 Subject: [PATCH 5/7] Added logging for error suppression. --- src/librustc_mir/borrow_check/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 6b84abbe266fa..95dd8dd48a0b1 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -735,7 +735,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } if self.access_place_error_reported.contains(&(place_span.0.clone(), place_span.1)) { - debug!("suppressing access_place error for {:?}", place_span); + debug!("access_place: suppressing error place_span=`{:?}` kind=`{:?}`", + place_span, kind); return AccessErrorsReported { mutability_error: false, conflict_error: true, @@ -748,6 +749,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { self.check_access_for_conflict(context, place_span, sd, rw, flow_state); if conflict_error || mutability_error { + debug!("access_place: logging error place_span=`{:?}` kind=`{:?}`", + place_span, kind); self.access_place_error_reported.insert((place_span.0.clone(), place_span.1)); } From 5cd4b4fd955ac57f64f45e175844c0e7e165989f Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 5 Feb 2018 22:31:56 +0000 Subject: [PATCH 6/7] Swapped order of left/right visits to ensure consistency in read/write pass ordering when -O is passed. --- src/librustc_mir/borrow_check/mod.rs | 14 +++++++------- .../nll/reference-carried-through-struct-field.rs | 2 +- src/test/ui/issue-45697.rs | 2 +- src/test/ui/issue-45697.stderr | 6 +++--- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 95dd8dd48a0b1..07d6ce20821ed 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -347,6 +347,13 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx match stmt.kind { StatementKind::Assign(ref lhs, ref rhs) => { + self.consume_rvalue( + ContextKind::AssignRhs.new(location), + (rhs, span), + location, + flow_state, + ); + self.mutate_place( ContextKind::AssignLhs.new(location), (lhs, span), @@ -354,13 +361,6 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx JustWrite, flow_state, ); - - self.consume_rvalue( - ContextKind::AssignRhs.new(location), - (rhs, span), - location, - flow_state, - ); } StatementKind::SetDiscriminant { ref place, diff --git a/src/test/compile-fail/nll/reference-carried-through-struct-field.rs b/src/test/compile-fail/nll/reference-carried-through-struct-field.rs index e64fecbe701e7..efa6cc273b6f4 100644 --- a/src/test/compile-fail/nll/reference-carried-through-struct-field.rs +++ b/src/test/compile-fail/nll/reference-carried-through-struct-field.rs @@ -19,7 +19,7 @@ fn foo() { let mut x = 22; let wrapper = Wrap { w: &mut x }; x += 1; //[ast]~ ERROR cannot assign to `x` because it is borrowed [E0506] - //[mir]~^ ERROR cannot assign to `x` because it is borrowed [E0506] + //[mir]~^ ERROR cannot use `x` because it was mutably borrowed [E0503] *wrapper.w += 1; } diff --git a/src/test/ui/issue-45697.rs b/src/test/ui/issue-45697.rs index 7f44209d717a1..b69ba97cc9b4d 100644 --- a/src/test/ui/issue-45697.rs +++ b/src/test/ui/issue-45697.rs @@ -29,7 +29,7 @@ fn main() { let z = copy_borrowed_ptr(&mut y); *y.pointer += 1; //~^ ERROR cannot assign to `*y.pointer` because it is borrowed (Ast) [E0506] - //~| ERROR cannot assign to `*y.pointer` because it is borrowed (Mir) [E0506] + //~| ERROR cannot use `*y.pointer` because it was mutably borrowed (Mir) [E0503] *z.pointer += 1; } } diff --git a/src/test/ui/issue-45697.stderr b/src/test/ui/issue-45697.stderr index 007bfbfc9b01e..e9b723d57b507 100644 --- a/src/test/ui/issue-45697.stderr +++ b/src/test/ui/issue-45697.stderr @@ -6,13 +6,13 @@ error[E0506]: cannot assign to `*y.pointer` because it is borrowed (Ast) 30 | *y.pointer += 1; | ^^^^^^^^^^^^^^^ assignment to borrowed `*y.pointer` occurs here -error[E0506]: cannot assign to `*y.pointer` because it is borrowed (Mir) +error[E0503]: cannot use `*y.pointer` because it was mutably borrowed (Mir) --> $DIR/issue-45697.rs:30:9 | 29 | let z = copy_borrowed_ptr(&mut y); - | ------ borrow of `*y.pointer` occurs here + | ------ borrow of `y` occurs here 30 | *y.pointer += 1; - | ^^^^^^^^^^^^^^^ assignment to borrowed `*y.pointer` occurs here + | ^^^^^^^^^^^^^^^ use of borrowed `y` error: aborting due to 2 previous errors From bb6e54d4bc7d4ce4c2372fecb84222867374b135 Mon Sep 17 00:00:00 2001 From: David Wood Date: Tue, 6 Feb 2018 17:37:49 +0000 Subject: [PATCH 7/7] Added and updated tests to enable/disable overflow checks. --- src/test/ui/issue-45697-1.rs | 35 ++++++++++++++++++++++++++++++++ src/test/ui/issue-45697-1.stderr | 18 ++++++++++++++++ src/test/ui/issue-45697.rs | 2 +- 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/issue-45697-1.rs create mode 100644 src/test/ui/issue-45697-1.stderr diff --git a/src/test/ui/issue-45697-1.rs b/src/test/ui/issue-45697-1.rs new file mode 100644 index 0000000000000..7734b14b2ab7b --- /dev/null +++ b/src/test/ui/issue-45697-1.rs @@ -0,0 +1,35 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Test that assignments to an `&mut` pointer which is found in a +// borrowed (but otherwise non-aliasable) location is illegal. + +// compile-flags: -Z emit-end-regions -Z borrowck=compare -C overflow-checks=on + +struct S<'a> { + pointer: &'a mut isize +} + +fn copy_borrowed_ptr<'a>(p: &'a mut S<'a>) -> S<'a> { + S { pointer: &mut *p.pointer } +} + +fn main() { + let mut x = 1; + + { + let mut y = S { pointer: &mut x }; + let z = copy_borrowed_ptr(&mut y); + *y.pointer += 1; + //~^ ERROR cannot assign to `*y.pointer` because it is borrowed (Ast) [E0506] + //~| ERROR cannot use `*y.pointer` because it was mutably borrowed (Mir) [E0503] + *z.pointer += 1; + } +} diff --git a/src/test/ui/issue-45697-1.stderr b/src/test/ui/issue-45697-1.stderr new file mode 100644 index 0000000000000..09f32b93acc1c --- /dev/null +++ b/src/test/ui/issue-45697-1.stderr @@ -0,0 +1,18 @@ +error[E0506]: cannot assign to `*y.pointer` because it is borrowed (Ast) + --> $DIR/issue-45697-1.rs:30:9 + | +29 | let z = copy_borrowed_ptr(&mut y); + | - borrow of `*y.pointer` occurs here +30 | *y.pointer += 1; + | ^^^^^^^^^^^^^^^ assignment to borrowed `*y.pointer` occurs here + +error[E0503]: cannot use `*y.pointer` because it was mutably borrowed (Mir) + --> $DIR/issue-45697-1.rs:30:9 + | +29 | let z = copy_borrowed_ptr(&mut y); + | ------ borrow of `y` occurs here +30 | *y.pointer += 1; + | ^^^^^^^^^^^^^^^ use of borrowed `y` + +error: aborting due to 2 previous errors + diff --git a/src/test/ui/issue-45697.rs b/src/test/ui/issue-45697.rs index b69ba97cc9b4d..4e93eccd6f649 100644 --- a/src/test/ui/issue-45697.rs +++ b/src/test/ui/issue-45697.rs @@ -11,7 +11,7 @@ // Test that assignments to an `&mut` pointer which is found in a // borrowed (but otherwise non-aliasable) location is illegal. -// compile-flags: -Z emit-end-regions -Z borrowck=compare +// compile-flags: -Z emit-end-regions -Z borrowck=compare -C overflow-checks=off struct S<'a> { pointer: &'a mut isize