Rollup merge of #150557 - dianne:no-const-block-eval-in-promotion, r=lcnr

Don't try to evaluate const blocks during constant promotion

As of #138499, trying to evaluate a const block in anything depended on by borrow-checking will result in a query cycle. Since that could happen in constant promotion, this PR adds a check for const blocks there to stop them from being evaluated.

Admittedly, this is a hack. See #124328 for discussion of a more principled fix: removing cases like this from constant promotion altogether. To simplify the conditions under which promotion can occur, we probably shouldn't be implicitly promoting division or array indexing at all if possible. That would likely require a FCW and migration period, so I figure we may as well patch up the cycle now and simplify later.

Fixes #150464

I'll also lang-nominate this for visibility. I'm not sure there's much to discuss about this PR specifically, but it does represent a change in semantics. In Rust 1.87, the code below compiled. In Rust 1.88, it became a query cycle error. After this PR, it fails to borrow-check because the temporaries can no longer be promoted.

```rust
let (x, y, z);
// We only promote array indexing if the index is known to be in-bounds.
x = &([0][const { 0 }] & 0);
// We only promote integer division if the divisor is known not to be zero.
y = &(1 / const { 1 });
// Furthermore, if the divisor is `-1`, we only promote if the dividend is
// known not to be `int::MIN`.
z = &(const { 1 } / -1);
// The borrowed temporaries can't be promoted, so they were dropped at the ends
// of their respective statements.
(x, y, z);
```

Author: JonathanBrouwer

compiler/rustc_mir_transform/src/promote_consts.rs

@@ -18,6 +18,7 @@ use rustc_const_eval::check_consts::{ConstCx, qualifs};
 use rustc_data_structures::assert_matches;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir as hir;
+use rustc_hir::def::DefKind;
 use rustc_index::{IndexSlice, IndexVec};
 use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor};
 use rustc_middle::mir::*;
@@ -329,6 +330,7 @@ impl<'tcx> Validator<'_, 'tcx> {
                 if let TempState::Defined { location: loc, .. } = self.temps[local]
                     && let Left(statement) =  self.body.stmt_at(loc)
                     && let Some((_, Rvalue::Use(Operand::Constant(c)))) = statement.kind.as_assign()
+                    && self.should_evaluate_for_promotion_checks(c.const_)
                     && let Some(idx) = c.const_.try_eval_target_usize(self.tcx, self.typing_env)
                     // Determine the type of the thing we are indexing.
                     && let ty::Array(_, len) = place_base.ty(self.body, self.tcx).ty.kind()
@@ -484,7 +486,9 @@ impl<'tcx> Validator<'_, 'tcx> {
                             let sz = lhs_ty.primitive_size(self.tcx);
                             // Integer division: the RHS must be a non-zero const.
                             let rhs_val = match rhs {
-                                Operand::Constant(c) => {
+                                Operand::Constant(c)
+                                    if self.should_evaluate_for_promotion_checks(c.const_) =>
+                                {
                                     c.const_.try_eval_scalar_int(self.tcx, self.typing_env)
                                 }
                                 _ => None,
@@ -502,9 +506,14 @@ impl<'tcx> Validator<'_, 'tcx> {
                                         // The RHS is -1 or unknown, so we have to be careful.
                                         // But is the LHS int::MIN?
                                         let lhs_val = match lhs {
-                                            Operand::Constant(c) => c
-                                                .const_
-                                                .try_eval_scalar_int(self.tcx, self.typing_env),
+                                            Operand::Constant(c)
+                                                if self.should_evaluate_for_promotion_checks(
+                                                    c.const_,
+                                                ) =>
+                                            {
+                                                c.const_
+                                                    .try_eval_scalar_int(self.tcx, self.typing_env)
+                                            }
                                             _ => None,
                                         };
                                         let lhs_min = sz.signed_int_min();
@@ -683,6 +692,28 @@ impl<'tcx> Validator<'_, 'tcx> {
         // This passed all checks, so let's accept.
         Ok(())
     }
+
+    /// Can we try to evaluate a given constant at this point in compilation? Attempting to evaluate
+    /// a const block before borrow-checking will result in a query cycle (#150464).
+    fn should_evaluate_for_promotion_checks(&self, constant: Const<'tcx>) -> bool {
+        match constant {
+            // `Const::Ty` is always a `ConstKind::Param` right now and that can never be turned
+            // into a mir value for promotion
+            // FIXME(mgca): do we want uses of type_const to be normalized during promotion?
+            Const::Ty(..) => false,
+            Const::Val(..) => true,
+            // Evaluating a MIR constant requires borrow-checking it. For inline consts, as of
+            // #138499, this means borrow-checking its typeck root. Since borrow-checking the
+            // typeck root requires promoting its constants, trying to evaluate an inline const here
+            // will result in a query cycle. To avoid the cycle, we can't evaluate const blocks yet.
+            // Other kinds of unevaluated's can cause query cycles too when they arise from
+            // self-reference in user code; e.g. evaluating a constant can require evaluating a
+            // const function that uses that constant, again requiring evaluation of the constant.
+            // However, this form of cycle renders both the constant and function unusable in
+            // general, so we don't need to special-case it here.
+            Const::Unevaluated(uc, _) => self.tcx.def_kind(uc.def) != DefKind::InlineConst,
+        }
+    }
 }
 
 fn validate_candidates(

tests/ui/inline-const/dont-eval-const-block-during-promotion.fail.stderr

@@ -0,0 +1,42 @@
+error[E0716]: temporary value dropped while borrowed
+  --> $DIR/dont-eval-const-block-during-promotion.rs:48:14
+   |
+LL |         x = &([0][const { 0 }] & 0);
+   |              ^^^^^^^^^^^^^^^^^^^^^^- temporary value is freed at the end of this statement
+   |              |
+   |              creates a temporary value which is freed while still in use
+...
+LL |         (x, y, z);
+   |          - borrow later used here
+   |
+   = note: consider using a `let` binding to create a longer lived value
+
+error[E0716]: temporary value dropped while borrowed
+  --> $DIR/dont-eval-const-block-during-promotion.rs:50:14
+   |
+LL |         y = &(1 / const { 1 });
+   |              ^^^^^^^^^^^^^^^^^- temporary value is freed at the end of this statement
+   |              |
+   |              creates a temporary value which is freed while still in use
+...
+LL |         (x, y, z);
+   |             - borrow later used here
+   |
+   = note: consider using a `let` binding to create a longer lived value
+
+error[E0716]: temporary value dropped while borrowed
+  --> $DIR/dont-eval-const-block-during-promotion.rs:52:14
+   |
+LL |         z = &(const { 1 } / -1);
+   |              ^^^^^^^^^^^^^^^^^^- temporary value is freed at the end of this statement
+   |              |
+   |              creates a temporary value which is freed while still in use
+LL |
+LL |         (x, y, z);
+   |                - borrow later used here
+   |
+   = note: consider using a `let` binding to create a longer lived value
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0716`.

tests/ui/inline-const/dont-eval-const-block-during-promotion.rs

@@ -0,0 +1,65 @@
+//! Test for #150464: as of #138499, trying to evaluate const blocks during constant promotion will
+//! result in a query cycle, so we shouldn't do it. Evaluation can happen when trying to promote
+//! integer division and array indexing, where it's necessary for the operation to succeed to be
+//! able to use it in a promoted constant.
+//@ revisions: pass fail
+//@[pass] check-pass
+
+use std::mem::offset_of;
+
+struct Thing(i32);
+
+fn main() {
+    // For a temporary involving array indexing to be promoted, we evaluate the index to make sure
+    // it's in-bounds. As of #150557 we treat inline constants as maybe-out-of-bounds to avoid the
+    // query cycle from evaluating them. That allows this to compile:
+    let x = &([0][const { 0 }] & 0);
+    // Likewise, integer divisors must be nonzero. Avoiding the query cycle allows this to compile:
+    let y = &(1 / const { 1 });
+    // Likewise, signed integer dividends can't be the integer minimum when the divisor is -1.
+    let z = &(const { 1 } / -1);
+    // These temporaries are all lifetime-extended, so they don't need to be promoted for references
+    // to them to be live later in the block. Generally, code with const blocks in these positions
+    // should compile as long as being promoted isn't necessary for borrow-checking to succeed.
+    (x, y, z);
+
+    // A reduced example from real code (#150464): this can't be promoted since the array is a local
+    // variable, but it still resulted in a query cycle because the index was evaluated for the
+    // bounds-check before checking that. By not evaluating the const block, we avoid the cycle.
+    // Since this doesn't rely on promotion, it should borrow-check successfully.
+    let temp = [0u8];
+    let _ = &(temp[const { 0usize }] & 0u8);
+    // #150464 was reported because `offset_of!` started desugaring to a const block in #148151.
+    let _ = &(temp[offset_of!(Thing, 0)] & 0u8);
+
+    // Similarly, at the time #150464 was reported, the index here was evaluated before checking
+    // that the indexed expression is an array. As above, this can't be promoted, but still resulted
+    // in a query cycle. By not evaluating the const block, we avoid the cycle. Since this doesn't
+    // rely on promotion, it should borrow-check successfully.
+    let temp: &[u8] = &[0u8];
+    let _ = &(temp[const { 0usize }] & 0u8);
+
+    // By no longer promoting these temporaries, they're dropped at the ends of their respective
+    // statements, so we can't refer to them thereafter. This code no longer query-cycles, but it
+    // fails to borrow-check instead.
+    #[cfg(fail)]
+    {
+        let (x, y, z);
+        x = &([0][const { 0 }] & 0);
+        //[fail]~^ ERROR: temporary value dropped while borrowed
+        y = &(1 / const { 1 });
+        //[fail]~^ ERROR: temporary value dropped while borrowed
+        z = &(const { 1 } / -1);
+        //[fail]~^ ERROR: temporary value dropped while borrowed
+        (x, y, z);
+    }
+
+    // Sanity check: those temporaries do promote if the const blocks are removed.
+    // If constant promotion is changed so that these are no longer implicitly promoted, the
+    // comments on this test file should be reworded to reflect that.
+    let (x, y, z);
+    x = &([0][0] & 0);
+    y = &(1 / 1);
+    z = &(1 / -1);
+    (x, y, z);
+}