From b0b7dc31679834005de11f455c5d86a5d1964639 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 26 Nov 2019 12:02:52 -0800 Subject: [PATCH 01/12] Add `#![feature(matches_macro)]` --- src/librustc_mir/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/librustc_mir/lib.rs b/src/librustc_mir/lib.rs index cca76700ed101..f4fb9a5e4f2a2 100644 --- a/src/librustc_mir/lib.rs +++ b/src/librustc_mir/lib.rs @@ -29,6 +29,7 @@ Rust MIR: a lowered representation of Rust. Also: an experiment! #![feature(stmt_expr_attributes)] #![feature(bool_to_option)] #![feature(trait_alias)] +#![feature(matches_macro)] #![recursion_limit="256"] From 659616585a9c813564c7c276b296dc5983a10008 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Fri, 22 Nov 2019 15:52:08 -0800 Subject: [PATCH 02/12] Use type-based qualification for statics --- src/librustc_mir/transform/check_consts/qualifs.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/qualifs.rs b/src/librustc_mir/transform/check_consts/qualifs.rs index 9ed1ca740b8e7..85e4c8c91f47c 100644 --- a/src/librustc_mir/transform/check_consts/qualifs.rs +++ b/src/librustc_mir/transform/check_consts/qualifs.rs @@ -33,9 +33,10 @@ pub trait Qualif { /// of the type. fn in_any_value_of_ty(_cx: &ConstCx<'_, 'tcx>, _ty: Ty<'tcx>) -> bool; - fn in_static(_cx: &ConstCx<'_, 'tcx>, _def_id: DefId) -> bool { - // FIXME(eddyb) should we do anything here for value properties? - false + fn in_static(cx: &ConstCx<'_, 'tcx>, def_id: DefId) -> bool { + // `mir_const_qualif` does return the qualifs in the final value of a `static`, so we could + // use value-based qualification here, but we shouldn't do this without a good reason. + Self::in_any_value_of_ty(cx, cx.tcx.type_of(def_id)) } fn in_projection_structurally( From 66e9f885de49302241eb7512d0515d3d9d566428 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Fri, 22 Nov 2019 15:52:59 -0800 Subject: [PATCH 03/12] Handle `Rvalue::Ref` in one place --- .../transform/check_consts/validation.rs | 177 +++++++++++------- 1 file changed, 106 insertions(+), 71 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index bee37f69a5ec5..824ec36e275e4 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -322,20 +322,9 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { trace!("visit_rvalue: rvalue={:?} location={:?}", rvalue, location); - // Check nested operands and places. + // Special-case reborrows to be more like a copy of a reference. if let Rvalue::Ref(_, kind, ref place) = *rvalue { - // Special-case reborrows to be more like a copy of a reference. - let mut reborrow_place = None; - if let &[ref proj_base @ .., elem] = place.projection.as_ref() { - if elem == ProjectionElem::Deref { - let base_ty = Place::ty_from(&place.base, proj_base, self.body, self.tcx).ty; - if let ty::Ref(..) = base_ty.kind { - reborrow_place = Some(proj_base); - } - } - } - - if let Some(proj) = reborrow_place { + if let Some(reborrowed_proj) = place_as_reborrow(self.tcx, self.body, place) { let ctx = match kind { BorrowKind::Shared => PlaceContext::NonMutatingUse( NonMutatingUseContext::SharedBorrow, @@ -351,14 +340,13 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { ), }; self.visit_place_base(&place.base, ctx, location); - self.visit_projection(&place.base, proj, ctx, location); - } else { - self.super_rvalue(rvalue, location); + self.visit_projection(&place.base, reborrowed_proj, ctx, location); + return; } - } else { - self.super_rvalue(rvalue, location); } + self.super_rvalue(rvalue, location); + match *rvalue { Rvalue::Use(_) | Rvalue::Repeat(..) | @@ -369,9 +357,87 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { Rvalue::Cast(CastKind::Pointer(_), ..) | Rvalue::Discriminant(..) | Rvalue::Len(_) | - Rvalue::Ref(..) | Rvalue::Aggregate(..) => {} + | Rvalue::Ref(_, kind @ BorrowKind::Mut { .. }, ref place) + | Rvalue::Ref(_, kind @ BorrowKind::Unique, ref place) + => { + let ty = place.ty(self.body, self.tcx).ty; + let is_allowed = match ty.kind { + // Inside a `static mut`, `&mut [...]` is allowed. + ty::Array(..) | ty::Slice(_) if self.const_kind() == ConstKind::StaticMut + => true, + + // FIXME(ecstaticmorse): We could allow `&mut []` inside a const context given + // that this is merely a ZST and it is already eligible for promotion. + // This may require an RFC? + /* + ty::Array(_, len) if len.try_eval_usize(cx.tcx, cx.param_env) == Some(0) + => true, + */ + + _ => false, + }; + + if !is_allowed { + self.check_op(ops::MutBorrow(kind)); + } + } + + // Taking a shared borrow of a `static` is always legal, even if that `static` has + // interior mutability. + | Rvalue::Ref(_, BorrowKind::Shared, ref place) + | Rvalue::Ref(_, BorrowKind::Shallow, ref place) + if matches!(place.base, PlaceBase::Static(_)) + => {} + + | Rvalue::Ref(_, kind @ BorrowKind::Shared, ref place) + | Rvalue::Ref(_, kind @ BorrowKind::Shallow, ref place) + => { + // FIXME: Change the `in_*` methods to take a `FnMut` so we don't have to manually + // seek the cursors beforehand. + self.qualifs.has_mut_interior.cursor.seek_before(location); + self.qualifs.indirectly_mutable.seek(location); + + let borrowed_place_has_mut_interior = HasMutInterior::in_place( + &self.item, + &|local| self.qualifs.has_mut_interior_eager_seek(local), + place.as_ref(), + ); + + if borrowed_place_has_mut_interior { + let src_derived_from_illegal_borrow = borrowed_place + .as_local() + .map_or(false, |local| self.derived_from_illegal_borrow.contains(local)); + + // Don't emit errors for borrows of values that are *themselves* the result of + // an illegal borrow (e.g., the outermost `&` in `&&Cell::new(42)`). We want to + // point the user to the place where the original illegal borrow occurred, not + // to subsequent borrows of the resulting value. + let dest_derived_from_illegal_borrow = if !src_derived_from_illegal_borrow { + self.check_op(ops::MutBorrow(kind)) == CheckOpResult::Forbidden + } else { + true + }; + + // When the target of the assignment is a local with no projections, it will be + // marked as derived from an illegal borrow if necessary. + // + // FIXME: should we also clear `derived_from_illegal_borrow` when a local is + // assigned a new value? + + if dest_derived_from_illegal_borrow { + let block = &self.body[location.block]; + let statement = &block.statements[location.statement_index]; + if let StatementKind::Assign(box (dest, _)) = &statement.kind { + if let Some(dest) = dest.as_local() { + self.derived_from_illegal_borrow.insert(dest); + } + } + } + } + } + Rvalue::Cast(CastKind::Misc, ref operand, cast_ty) => { let operand_ty = operand.ty(self.body, self.tcx); let cast_in = CastTy::from_ty(operand_ty).expect("bad input type for cast"); @@ -436,58 +502,6 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { } } - fn visit_assign(&mut self, dest: &Place<'tcx>, rvalue: &Rvalue<'tcx>, location: Location) { - trace!("visit_assign: dest={:?} rvalue={:?} location={:?}", dest, rvalue, location); - - // Error on mutable borrows or shared borrows of values with interior mutability. - // - // This replicates the logic at the start of `assign` in the old const checker. Note that - // it depends on `HasMutInterior` being set for mutable borrows as well as values with - // interior mutability. - if let Rvalue::Ref(_, kind, ref borrowed_place) = *rvalue { - // FIXME: Change the `in_*` methods to take a `FnMut` so we don't have to manually seek - // the cursors beforehand. - self.qualifs.has_mut_interior.cursor.seek_before(location); - self.qualifs.indirectly_mutable.seek(location); - - let rvalue_has_mut_interior = HasMutInterior::in_rvalue( - &self.item, - &|local| self.qualifs.has_mut_interior_eager_seek(local), - rvalue, - ); - - if rvalue_has_mut_interior { - let is_derived_from_illegal_borrow = match borrowed_place.as_local() { - // If an unprojected local was borrowed and its value was the result of an - // illegal borrow, suppress this error and mark the result of this borrow as - // illegal as well. - Some(borrowed_local) - if self.derived_from_illegal_borrow.contains(borrowed_local) => - { - true - } - - // Otherwise proceed normally: check the legality of a mutable borrow in this - // context. - _ => self.check_op(ops::MutBorrow(kind)) == CheckOpResult::Forbidden, - }; - - // When the target of the assignment is a local with no projections, mark it as - // derived from an illegal borrow if necessary. - // - // FIXME: should we also clear `derived_from_illegal_borrow` when a local is - // assigned a new value? - if is_derived_from_illegal_borrow { - if let Some(dest) = dest.as_local() { - self.derived_from_illegal_borrow.insert(dest); - } - } - } - } - - self.super_assign(dest, rvalue, location); - } - fn visit_projection_elem( &mut self, place_base: &PlaceBase<'tcx>, @@ -725,3 +739,24 @@ fn check_return_ty_is_sync(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, hir_id: HirId) } }); } + +fn place_as_reborrow( + tcx: TyCtxt<'tcx>, + body: &Body<'tcx>, + place: &'a Place<'tcx>, +) -> Option<&'a [PlaceElem<'tcx>]> { + place + .projection + .split_last() + .and_then(|(outermost, inner)| { + if outermost != &ProjectionElem::Deref { + return None; + } + + let inner_ty = Place::ty_from(&place.base, inner, body, tcx).ty; + match inner_ty.kind { + ty::Ref(..) => Some(inner), + _ => None, + } + }) +} From 5a3ddcd771e9df8b5ed47b7aad22bf526bfd8b13 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Fri, 22 Nov 2019 16:14:24 -0800 Subject: [PATCH 04/12] Update test with improved diagnostics Now, `derived_from_illegal_borrow` is also applied to reborrows, so we don't show the user a useless error message. --- src/test/ui/consts/const-multi-ref.rs | 7 +++++-- src/test/ui/consts/const-multi-ref.stderr | 10 ++-------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/test/ui/consts/const-multi-ref.rs b/src/test/ui/consts/const-multi-ref.rs index 498e99e668b43..4526904232875 100644 --- a/src/test/ui/consts/const-multi-ref.rs +++ b/src/test/ui/consts/const-multi-ref.rs @@ -1,8 +1,11 @@ +// Ensure that we point the user to the erroneous borrow but not to any subsequent borrows of that +// initial one. + const _X: i32 = { let mut a = 5; - let p = &mut a; //~ ERROR references in constants may only refer to immutable values + let p = &mut a; //~ ERROR references in constants may only refer to immutable values - let reborrow = {p}; //~ ERROR references in constants may only refer to immutable values + let reborrow = {p}; let pp = &reborrow; let ppp = &pp; ***ppp diff --git a/src/test/ui/consts/const-multi-ref.stderr b/src/test/ui/consts/const-multi-ref.stderr index 9e525ef9aac75..50533b0b1ccee 100644 --- a/src/test/ui/consts/const-multi-ref.stderr +++ b/src/test/ui/consts/const-multi-ref.stderr @@ -1,15 +1,9 @@ error[E0017]: references in constants may only refer to immutable values - --> $DIR/const-multi-ref.rs:3:13 + --> $DIR/const-multi-ref.rs:6:13 | LL | let p = &mut a; | ^^^^^^ constants require immutable values -error[E0017]: references in constants may only refer to immutable values - --> $DIR/const-multi-ref.rs:5:21 - | -LL | let reborrow = {p}; - | ^ constants require immutable values - -error: aborting due to 2 previous errors +error: aborting due to previous error For more information about this error, try `rustc --explain E0017`. From 5a10430f2c50937b5417881d2a8580becd6100cc Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Fri, 22 Nov 2019 16:23:01 -0800 Subject: [PATCH 05/12] Remove `Rvalue::Ref` handling from `HasMutInterior` --- .../transform/check_consts/qualifs.rs | 30 +------------------ 1 file changed, 1 insertion(+), 29 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/qualifs.rs b/src/librustc_mir/transform/check_consts/qualifs.rs index 85e4c8c91f47c..2d5a0a2afcd01 100644 --- a/src/librustc_mir/transform/check_consts/qualifs.rs +++ b/src/librustc_mir/transform/check_consts/qualifs.rs @@ -5,7 +5,7 @@ use rustc::ty::{self, Ty}; use rustc::hir::def_id::DefId; use syntax_pos::DUMMY_SP; -use super::{ConstKind, Item as ConstCx}; +use super::Item as ConstCx; pub fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> ConstQualifs { ConstQualifs { @@ -218,34 +218,6 @@ impl Qualif for HasMutInterior { rvalue: &Rvalue<'tcx>, ) -> bool { match *rvalue { - // Returning `true` for `Rvalue::Ref` indicates the borrow isn't - // allowed in constants (and the `Checker` will error), and/or it - // won't be promoted, due to `&mut ...` or interior mutability. - Rvalue::Ref(_, kind, ref place) => { - let ty = place.ty(cx.body, cx.tcx).ty; - - if let BorrowKind::Mut { .. } = kind { - // In theory, any zero-sized value could be borrowed - // mutably without consequences. - match ty.kind { - // Inside a `static mut`, &mut [...] is also allowed. - | ty::Array(..) - | ty::Slice(_) - if cx.const_kind == Some(ConstKind::StaticMut) - => {}, - - // FIXME(eddyb): We only return false for `&mut []` outside a const - // context which seems unnecessary given that this is merely a ZST. - | ty::Array(_, len) - if len.try_eval_usize(cx.tcx, cx.param_env) == Some(0) - && cx.const_kind == None - => {}, - - _ => return true, - } - } - } - Rvalue::Aggregate(ref kind, _) => { if let AggregateKind::Adt(def, ..) = **kind { if Some(def.did) == cx.tcx.lang_items().unsafe_cell_type() { From 511740706c0930630b896d8abb9b4f709c1faa1b Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 26 Nov 2019 11:53:10 -0800 Subject: [PATCH 06/12] Remove `derived_from_illegal_borrow` --- .../transform/check_consts/validation.rs | 42 +------------------ 1 file changed, 1 insertion(+), 41 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index 824ec36e275e4..4867e05e3e8e7 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -149,17 +149,6 @@ pub struct Validator<'a, 'mir, 'tcx> { /// The span of the current statement. span: Span, - - /// True if the local was assigned the result of an illegal borrow (`ops::MutBorrow`). - /// - /// This is used to hide errors from {re,}borrowing the newly-assigned local, instead pointing - /// the user to the place where the illegal borrow occurred. This set is only populated once an - /// error has been emitted, so it will never cause an erroneous `mir::Body` to pass validation. - /// - /// FIXME(ecstaticmorse): assert at the end of checking that if `tcx.has_errors() == false`, - /// this set is empty. Note that if we start removing locals from - /// `derived_from_illegal_borrow`, just checking at the end won't be enough. - derived_from_illegal_borrow: BitSet, } impl Deref for Validator<'_, 'mir, 'tcx> { @@ -213,7 +202,6 @@ impl Validator<'a, 'mir, 'tcx> { span: item.body.span, item, qualifs, - derived_from_illegal_borrow: BitSet::new_empty(item.body.local_decls.len()), } } @@ -406,35 +394,7 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { ); if borrowed_place_has_mut_interior { - let src_derived_from_illegal_borrow = borrowed_place - .as_local() - .map_or(false, |local| self.derived_from_illegal_borrow.contains(local)); - - // Don't emit errors for borrows of values that are *themselves* the result of - // an illegal borrow (e.g., the outermost `&` in `&&Cell::new(42)`). We want to - // point the user to the place where the original illegal borrow occurred, not - // to subsequent borrows of the resulting value. - let dest_derived_from_illegal_borrow = if !src_derived_from_illegal_borrow { - self.check_op(ops::MutBorrow(kind)) == CheckOpResult::Forbidden - } else { - true - }; - - // When the target of the assignment is a local with no projections, it will be - // marked as derived from an illegal borrow if necessary. - // - // FIXME: should we also clear `derived_from_illegal_borrow` when a local is - // assigned a new value? - - if dest_derived_from_illegal_borrow { - let block = &self.body[location.block]; - let statement = &block.statements[location.statement_index]; - if let StatementKind::Assign(box (dest, _)) = &statement.kind { - if let Some(dest) = dest.as_local() { - self.derived_from_illegal_borrow.insert(dest); - } - } - } + self.check_op(ops::MutBorrow(kind)); } } From dd32d911d6a2aaf3cfad35ca98cd0571bae4f39e Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 27 Nov 2019 14:29:09 -0800 Subject: [PATCH 07/12] Remove `CheckOpResult` --- .../transform/check_consts/validation.rs | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index 4867e05e3e8e7..4d8f9720af085 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -23,13 +23,6 @@ use super::qualifs::{self, HasMutInterior, NeedsDrop}; use super::resolver::FlowSensitiveAnalysis; use super::{ConstKind, Item, Qualif, is_lang_panic_fn}; -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub enum CheckOpResult { - Forbidden, - Unleashed, - Allowed, -} - pub type IndirectlyMutableResults<'mir, 'tcx> = old_dataflow::DataflowResultsCursor<'mir, 'tcx, IndirectlyMutableLocals<'mir, 'tcx>>; @@ -246,15 +239,15 @@ impl Validator<'a, 'mir, 'tcx> { } /// Emits an error at the given `span` if an expression cannot be evaluated in the current - /// context. Returns `Forbidden` if an error was emitted. - pub fn check_op_spanned(&mut self, op: O, span: Span) -> CheckOpResult + /// context. + pub fn check_op_spanned(&mut self, op: O, span: Span) where O: NonConstOp { trace!("check_op: op={:?}", op); if op.is_allowed_in_item(self) { - return CheckOpResult::Allowed; + return; } // If an operation is supported in miri (and is not already controlled by a feature gate) it @@ -264,20 +257,19 @@ impl Validator<'a, 'mir, 'tcx> { if is_unleashable && self.tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you { self.tcx.sess.span_warn(span, "skipping const checks"); - return CheckOpResult::Unleashed; + return; } op.emit_error(self, span); - CheckOpResult::Forbidden } /// Emits an error if an expression cannot be evaluated in the current context. - pub fn check_op(&mut self, op: impl NonConstOp) -> CheckOpResult { + pub fn check_op(&mut self, op: impl NonConstOp) { let span = self.span; self.check_op_spanned(op, span) } - fn check_static(&mut self, def_id: DefId, span: Span) -> CheckOpResult { + fn check_static(&mut self, def_id: DefId, span: Span) { let is_thread_local = self.tcx.has_attr(def_id, sym::thread_local); if is_thread_local { self.check_op_spanned(ops::ThreadLocalAccess, span) From d2bdaa8deb2728fad7de31a2077ee68a861c032a Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 26 Nov 2019 12:10:44 -0800 Subject: [PATCH 08/12] Also test shared borrows of `Cell` for good errors --- src/test/ui/consts/const-multi-ref.rs | 12 +++++++++++- src/test/ui/consts/const-multi-ref.stderr | 11 +++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/test/ui/consts/const-multi-ref.rs b/src/test/ui/consts/const-multi-ref.rs index 4526904232875..5e2be0d4f3f02 100644 --- a/src/test/ui/consts/const-multi-ref.rs +++ b/src/test/ui/consts/const-multi-ref.rs @@ -1,7 +1,7 @@ // Ensure that we point the user to the erroneous borrow but not to any subsequent borrows of that // initial one. -const _X: i32 = { +const _: i32 = { let mut a = 5; let p = &mut a; //~ ERROR references in constants may only refer to immutable values @@ -11,4 +11,14 @@ const _X: i32 = { ***ppp }; +const _: std::cell::Cell = { + let mut a = std::cell::Cell::new(5); + let p = &a; //~ ERROR cannot borrow a constant which may contain interior mutability + + let reborrow = {p}; + let pp = &reborrow; + let ppp = &pp; + a +}; + fn main() {} diff --git a/src/test/ui/consts/const-multi-ref.stderr b/src/test/ui/consts/const-multi-ref.stderr index 50533b0b1ccee..ed3837e9c9ee5 100644 --- a/src/test/ui/consts/const-multi-ref.stderr +++ b/src/test/ui/consts/const-multi-ref.stderr @@ -4,6 +4,13 @@ error[E0017]: references in constants may only refer to immutable values LL | let p = &mut a; | ^^^^^^ constants require immutable values -error: aborting due to previous error +error[E0492]: cannot borrow a constant which may contain interior mutability, create a static instead + --> $DIR/const-multi-ref.rs:16:13 + | +LL | let p = &a; + | ^^ + +error: aborting due to 2 previous errors -For more information about this error, try `rustc --explain E0017`. +Some errors have detailed explanations: E0017, E0492. +For more information about an error, try `rustc --explain E0017`. From 5b1e10b2f6b37d9980edbd799b07e22db3a37df4 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 26 Nov 2019 17:07:44 -0800 Subject: [PATCH 09/12] Remove test for unused error code This error code is never emitted, and the contents of this test are identical to that of `E0017.rs`. --- src/test/ui/error-codes/E0388.rs | 9 --------- src/test/ui/error-codes/E0388.stderr | 28 ---------------------------- 2 files changed, 37 deletions(-) delete mode 100644 src/test/ui/error-codes/E0388.rs delete mode 100644 src/test/ui/error-codes/E0388.stderr diff --git a/src/test/ui/error-codes/E0388.rs b/src/test/ui/error-codes/E0388.rs deleted file mode 100644 index 3aa4ac9655cc9..0000000000000 --- a/src/test/ui/error-codes/E0388.rs +++ /dev/null @@ -1,9 +0,0 @@ -static X: i32 = 1; -const C: i32 = 2; - -const CR: &'static mut i32 = &mut C; //~ ERROR E0017 -static STATIC_REF: &'static mut i32 = &mut X; //~ ERROR E0017 - //~| ERROR cannot borrow -static CONST_REF: &'static mut i32 = &mut C; //~ ERROR E0017 - -fn main() {} diff --git a/src/test/ui/error-codes/E0388.stderr b/src/test/ui/error-codes/E0388.stderr deleted file mode 100644 index b52d5260b13c8..0000000000000 --- a/src/test/ui/error-codes/E0388.stderr +++ /dev/null @@ -1,28 +0,0 @@ -error[E0017]: references in constants may only refer to immutable values - --> $DIR/E0388.rs:4:30 - | -LL | const CR: &'static mut i32 = &mut C; - | ^^^^^^ constants require immutable values - -error[E0017]: references in statics may only refer to immutable values - --> $DIR/E0388.rs:5:39 - | -LL | static STATIC_REF: &'static mut i32 = &mut X; - | ^^^^^^ statics require immutable values - -error[E0596]: cannot borrow immutable static item `X` as mutable - --> $DIR/E0388.rs:5:39 - | -LL | static STATIC_REF: &'static mut i32 = &mut X; - | ^^^^^^ cannot borrow as mutable - -error[E0017]: references in statics may only refer to immutable values - --> $DIR/E0388.rs:7:38 - | -LL | static CONST_REF: &'static mut i32 = &mut C; - | ^^^^^^ statics require immutable values - -error: aborting due to 4 previous errors - -Some errors have detailed explanations: E0017, E0596. -For more information about an error, try `rustc --explain E0017`. From a70ac50ec4cfcda283f0cb5292ea7ef14d861748 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 27 Nov 2019 14:04:11 -0800 Subject: [PATCH 10/12] Don't treat a reference to a `static` as a reborrow They now look the same in the MIR after #66587. --- .../transform/check_consts/validation.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index 4d8f9720af085..bc13764f0a3ed 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -705,6 +705,19 @@ fn place_as_reborrow( return None; } + // A borrow of a `static` also looks like `&(*_1)` in the MIR, but `_1` is a `const` + // that points to the allocation for the static. Don't treat these as reborrows. + if let PlaceBase::Local(local) = place.base { + let decl = &body.local_decls[local]; + if let LocalInfo::StaticRef { .. } = decl.local_info { + return None; + } + } + + // Ensure the type being derefed is a reference and not a raw pointer. + // + // This is sufficient to prevent an access to a `static mut` from being marked as a + // reborrow, even if the check above were to disappear. let inner_ty = Place::ty_from(&place.base, inner, body, tcx).ty; match inner_ty.kind { ty::Ref(..) => Some(inner), From 846be82277fef758e033538d7d49a2e834e472c4 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 26 Nov 2019 17:08:46 -0800 Subject: [PATCH 11/12] Update test for mutably borrowed statics in a const This checks `static mut` as well for E0017, and blesses tests now that we emit an error for a mut deref. --- src/test/ui/error-codes/E0017.rs | 3 +++ src/test/ui/error-codes/E0017.stderr | 24 ++++++++++++++++++------ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/test/ui/error-codes/E0017.rs b/src/test/ui/error-codes/E0017.rs index 94b6587eb815a..3bc518c2c2b71 100644 --- a/src/test/ui/error-codes/E0017.rs +++ b/src/test/ui/error-codes/E0017.rs @@ -1,8 +1,11 @@ static X: i32 = 1; const C: i32 = 2; +static mut M: i32 = 3; const CR: &'static mut i32 = &mut C; //~ ERROR E0017 static STATIC_REF: &'static mut i32 = &mut X; //~ ERROR E0017 + //~| ERROR E0019 //~| ERROR cannot borrow static CONST_REF: &'static mut i32 = &mut C; //~ ERROR E0017 +static STATIC_MUT_REF: &'static mut i32 = unsafe { &mut M }; //~ ERROR E0017 fn main() {} diff --git a/src/test/ui/error-codes/E0017.stderr b/src/test/ui/error-codes/E0017.stderr index 47863f0221405..8c8660adceb7a 100644 --- a/src/test/ui/error-codes/E0017.stderr +++ b/src/test/ui/error-codes/E0017.stderr @@ -1,28 +1,40 @@ error[E0017]: references in constants may only refer to immutable values - --> $DIR/E0017.rs:4:30 + --> $DIR/E0017.rs:5:30 | LL | const CR: &'static mut i32 = &mut C; | ^^^^^^ constants require immutable values +error[E0019]: static contains unimplemented expression type + --> $DIR/E0017.rs:6:39 + | +LL | static STATIC_REF: &'static mut i32 = &mut X; + | ^^^^^^ + error[E0017]: references in statics may only refer to immutable values - --> $DIR/E0017.rs:5:39 + --> $DIR/E0017.rs:6:39 | LL | static STATIC_REF: &'static mut i32 = &mut X; | ^^^^^^ statics require immutable values error[E0596]: cannot borrow immutable static item `X` as mutable - --> $DIR/E0017.rs:5:39 + --> $DIR/E0017.rs:6:39 | LL | static STATIC_REF: &'static mut i32 = &mut X; | ^^^^^^ cannot borrow as mutable error[E0017]: references in statics may only refer to immutable values - --> $DIR/E0017.rs:7:38 + --> $DIR/E0017.rs:9:38 | LL | static CONST_REF: &'static mut i32 = &mut C; | ^^^^^^ statics require immutable values -error: aborting due to 4 previous errors +error[E0017]: references in statics may only refer to immutable values + --> $DIR/E0017.rs:10:52 + | +LL | static STATIC_MUT_REF: &'static mut i32 = unsafe { &mut M }; + | ^^^^^^ statics require immutable values + +error: aborting due to 6 previous errors -Some errors have detailed explanations: E0017, E0596. +Some errors have detailed explanations: E0017, E0019, E0596. For more information about an error, try `rustc --explain E0017`. From ccb4eed3529eaf610f476dd6d78ecd159a897b64 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sun, 1 Dec 2019 11:43:19 -0800 Subject: [PATCH 12/12] Incorporate fixes from review --- src/librustc_mir/transform/check_consts/validation.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index bc13764f0a3ed..68bfa50a95289 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -364,12 +364,11 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { } } - // Taking a shared borrow of a `static` is always legal, even if that `static` has - // interior mutability. + // At the moment, `PlaceBase::Static` is only used for promoted MIR. | Rvalue::Ref(_, BorrowKind::Shared, ref place) | Rvalue::Ref(_, BorrowKind::Shallow, ref place) if matches!(place.base, PlaceBase::Static(_)) - => {} + => bug!("Saw a promoted during const-checking, which must run before promotion"), | Rvalue::Ref(_, kind @ BorrowKind::Shared, ref place) | Rvalue::Ref(_, kind @ BorrowKind::Shallow, ref place) @@ -708,8 +707,7 @@ fn place_as_reborrow( // A borrow of a `static` also looks like `&(*_1)` in the MIR, but `_1` is a `const` // that points to the allocation for the static. Don't treat these as reborrows. if let PlaceBase::Local(local) = place.base { - let decl = &body.local_decls[local]; - if let LocalInfo::StaticRef { .. } = decl.local_info { + if body.local_decls[local].is_ref_to_static() { return None; } }