Rollup merge of #66654 - ecstatic-morse:check-consts-ref, r=eddyb,matthewjasper

Handle const-checks for `&mut` outside of `HasMutInterior`

Addresses [this comment](#64470 (comment)).

Const-checking relied on `HasMutInterior` to forbid `&mut` in a const context. This was strange because all we needed to do was look for an `Rvalue::Ref` with a certain `BorrowKind`, whereas the `Qualif` traits are specifically meant to get the qualifs for a *value*. This PR removes that logic from `HasMutInterior` and moves it into `check_consts::Validator`.

As a result, we can now properly handle qualifications for `static`s, which had to be ignored previously since you can e.g. borrow a static `Cell` from another `static`. We also remove the `derived_from_illegal_borrow` logic, since it is no longer necessary; we give good errors for subsequent reborrows/borrows of illegal borrows.

Author: RalfJung

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"]
 

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 {
@@ -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(
@@ -217,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() {

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>>;
 
@@ -149,17 +142,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<Local>,
 }
 
 impl Deref for Validator<'_, 'mir, 'tcx> {
@@ -213,7 +195,6 @@ impl Validator<'a, 'mir, 'tcx> {
             span: item.body.span,
             item,
             qualifs,
-            derived_from_illegal_borrow: BitSet::new_empty(item.body.local_decls.len()),
         }
     }
 
@@ -258,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<O>(&mut self, op: O, span: Span) -> CheckOpResult
+    /// context.
+    pub fn check_op_spanned<O>(&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
@@ -276,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)
@@ -322,20 +302,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 +320,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 +337,58 @@ 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));
+                }
+            }
+
+            // 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)
+            => {
+                // 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 {
+                    self.check_op(ops::MutBorrow(kind));
+                }
+            }
+
             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 +453,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>,
@@ -724,3 +689,36 @@ 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;
+            }
+
+            // 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 {
+                if body.local_decls[local].is_ref_to_static() {
+                    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),
+                _ => None,
+            }
+        })
+}

src/test/ui/consts/const-multi-ref.rs

@@ -1,11 +1,24 @@
-const _X: i32 = {
+// Ensure that we point the user to the erroneous borrow but not to any subsequent borrows of that
+// initial one.
+
+const _: 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
 };
 
+const _: std::cell::Cell<i32> = {
+    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() {}

src/test/ui/consts/const-multi-ref.stderr

@@ -1,15 +1,16 @@
 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
+error[E0492]: cannot borrow a constant which may contain interior mutability, create a static instead
+  --> $DIR/const-multi-ref.rs:16:13
    |
-LL |     let reborrow = {p};
-   |                     ^ constants require immutable values
+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`.