Auto merge of #80579 - RalfJung:no-fallible-promotion, r=oli-obk

avoid promoting division, modulo and indexing operations that could fail

For division, `x / y` will still be promoted if `y` is a non-zero integer literal; however, `1/(1+1)` will not be promoted any more.

While at it, also see if we can reject promoting floating-point arithmetic (which are [complicated](rust-lang/unsafe-code-guidelines#237) so maybe we should not promote them).

This will need a crater run to see if there's code out there that relies on these things being promoted.

If we can land this, promoteds in `fn`/`const fn` cannot fail to evaluate any more, which should let us do some simplifications in codegen/Miri!

Cc rust-lang/rfcs#3027
Fixes #61821
r? `@oli-obk`

compiler/rustc_lint_defs/src/builtin.rs

@@ -238,41 +238,22 @@ declare_lint! {
     ///
     /// ```rust,compile_fail
     /// #![allow(unconditional_panic)]
-    /// let x: &'static i32 = &(1 / 0);
+    /// const C: i32 = 1/0;
     /// ```
     ///
     /// {{produces}}
     ///
     /// ### Explanation
     ///
-    /// This lint detects code that is very likely incorrect. If this lint is
-    /// allowed, then the code will not be evaluated at compile-time, and
-    /// instead continue to generate code to evaluate at runtime, which may
-    /// panic during runtime.
+    /// This lint detects constants that fail to evaluate. Allowing the lint will accept the
+    /// constant declaration, but any use of this constant will still lead to a hard error. This is
+    /// a future incompatibility lint; the plan is to eventually entirely forbid even declaring
+    /// constants that cannot be evaluated.  See [issue #71800] for more details.
     ///
-    /// Note that this lint may trigger in either inside or outside of a
-    /// [const context]. Outside of a [const context], the compiler can
-    /// sometimes evaluate an expression at compile-time in order to generate
-    /// more efficient code. As the compiler becomes better at doing this, it
-    /// needs to decide what to do when it encounters code that it knows for
-    /// certain will panic or is otherwise incorrect. Making this a hard error
-    /// would prevent existing code that exhibited this behavior from
-    /// compiling, breaking backwards-compatibility. However, this is almost
-    /// certainly incorrect code, so this is a deny-by-default lint. For more
-    /// details, see [RFC 1229] and [issue #28238].
-    ///
-    /// Note that there are several other more specific lints associated with
-    /// compile-time evaluation, such as [`arithmetic_overflow`],
-    /// [`unconditional_panic`].
-    ///
-    /// [const context]: https://doc.rust-lang.org/reference/const_eval.html#const-context
-    /// [RFC 1229]: https://github.com/rust-lang/rfcs/blob/master/text/1229-compile-time-asserts.md
-    /// [issue #28238]: https://github.com/rust-lang/rust/issues/28238
-    /// [`arithmetic_overflow`]: deny-by-default.html#arithmetic-overflow
-    /// [`unconditional_panic`]: deny-by-default.html#unconditional-panic
+    /// [issue #71800]: https://github.com/rust-lang/rust/issues/71800
     pub CONST_ERR,
     Deny,
-    "constant evaluation detected erroneous expression",
+    "constant evaluation encountered erroneous expression",
     report_in_external_macro
 }
 

compiler/rustc_mir/src/transform/promote_consts.rs

@@ -415,10 +415,11 @@ impl<'tcx> Validator<'_, 'tcx> {
     // FIXME(eddyb) maybe cache this?
     fn validate_local(&self, local: Local) -> Result<(), Unpromotable> {
         if let TempState::Defined { location: loc, .. } = self.temps[local] {
-            let num_stmts = self.body[loc.block].statements.len();
+            let block = &self.body[loc.block];
+            let num_stmts = block.statements.len();
 
             if loc.statement_index < num_stmts {
-                let statement = &self.body[loc.block].statements[loc.statement_index];
+                let statement = &block.statements[loc.statement_index];
                 match &statement.kind {
                     StatementKind::Assign(box (_, rhs)) => self.validate_rvalue(rhs),
                     _ => {
@@ -430,7 +431,7 @@ impl<'tcx> Validator<'_, 'tcx> {
                     }
                 }
             } else {
-                let terminator = self.body[loc.block].terminator();
+                let terminator = block.terminator();
                 match &terminator.kind {
                     TerminatorKind::Call { func, args, .. } => self.validate_call(func, args),
                     TerminatorKind::Yield { .. } => Err(Unpromotable),
@@ -452,22 +453,15 @@ impl<'tcx> Validator<'_, 'tcx> {
                 match elem {
                     ProjectionElem::Deref => {
                         let mut promotable = false;
-                        // The `is_empty` predicate is introduced to exclude the case
-                        // where the projection operations are [ .field, * ].
-                        // The reason is because promotion will be illegal if field
-                        // accesses precede the dereferencing.
+                        // We need to make sure this is a `Deref` of a local with no further projections.
                         // Discussion can be found at
                         // https://github.com/rust-lang/rust/pull/74945#discussion_r463063247
-                        // There may be opportunity for generalization, but this needs to be
-                        // accounted for.
-                        if place_base.projection.is_empty() {
+                        if let Some(local) = place_base.as_local() {
                             // This is a special treatment for cases like *&STATIC where STATIC is a
                             // global static variable.
                             // This pattern is generated only when global static variables are directly
                             // accessed and is qualified for promotion safely.
-                            if let TempState::Defined { location, .. } =
-                                self.temps[place_base.local]
-                            {
+                            if let TempState::Defined { location, .. } = self.temps[local] {
                                 let def_stmt = self.body[location.block]
                                     .statements
                                     .get(location.statement_index);
@@ -505,6 +499,50 @@ impl<'tcx> Validator<'_, 'tcx> {
                     ProjectionElem::ConstantIndex { .. } | ProjectionElem::Subslice { .. } => {}
 
                     ProjectionElem::Index(local) => {
+                        if !self.explicit {
+                            let mut promotable = false;
+                            // Only accept if we can predict the index and are indexing an array.
+                            let val = if let TempState::Defined { location: loc, .. } =
+                                self.temps[local]
+                            {
+                                let block = &self.body[loc.block];
+                                if loc.statement_index < block.statements.len() {
+                                    let statement = &block.statements[loc.statement_index];
+                                    match &statement.kind {
+                                        StatementKind::Assign(box (
+                                            _,
+                                            Rvalue::Use(Operand::Constant(c)),
+                                        )) => c.literal.try_eval_usize(self.tcx, self.param_env),
+                                        _ => None,
+                                    }
+                                } else {
+                                    None
+                                }
+                            } else {
+                                None
+                            };
+                            if let Some(idx) = val {
+                                // Determine the type of the thing we are indexing.
+                                let ty = place_base.ty(self.body, self.tcx).ty;
+                                match ty.kind() {
+                                    ty::Array(_, len) => {
+                                        // It's an array; determine its length.
+                                        if let Some(len) =
+                                            len.try_eval_usize(self.tcx, self.param_env)
+                                        {
+                                            // If the index is in-bounds, go ahead.
+                                            if idx < len {
+                                                promotable = true;
+                                            }
+                                        }
+                                    }
+                                    _ => {}
+                                }
+                            }
+                            if !promotable {
+                                return Err(Unpromotable);
+                            }
+                        }
                         self.validate_local(local)?;
                     }
 
@@ -589,9 +627,7 @@ impl<'tcx> Validator<'_, 'tcx> {
 
     fn validate_rvalue(&self, rvalue: &Rvalue<'tcx>) -> Result<(), Unpromotable> {
         match rvalue {
-            Rvalue::Use(operand)
-            | Rvalue::Repeat(operand, _)
-            | Rvalue::UnaryOp(UnOp::Not | UnOp::Neg, operand) => {
+            Rvalue::Use(operand) | Rvalue::Repeat(operand, _) => {
                 self.validate_operand(operand)?;
             }
 
@@ -616,10 +652,26 @@ impl<'tcx> Validator<'_, 'tcx> {
                 self.validate_operand(operand)?;
             }
 
+            Rvalue::NullaryOp(op, _) => match op {
+                NullOp::Box => return Err(Unpromotable),
+                NullOp::SizeOf => {}
+            },
+
+            Rvalue::UnaryOp(op, operand) => {
+                match op {
+                    // These operations can never fail.
+                    UnOp::Neg | UnOp::Not => {}
+                }
+
+                self.validate_operand(operand)?;
+            }
+
             Rvalue::BinaryOp(op, lhs, rhs) | Rvalue::CheckedBinaryOp(op, lhs, rhs) => {
                 let op = *op;
-                if let ty::RawPtr(_) | ty::FnPtr(..) = lhs.ty(self.body, self.tcx).kind() {
-                    // raw pointer operations are not allowed inside consts and thus not promotable
+                let lhs_ty = lhs.ty(self.body, self.tcx);
+
+                if let ty::RawPtr(_) | ty::FnPtr(..) = lhs_ty.kind() {
+                    // Raw and fn pointer operations are not allowed inside consts and thus not promotable.
                     assert!(matches!(
                         op,
                         BinOp::Eq
@@ -634,7 +686,22 @@ impl<'tcx> Validator<'_, 'tcx> {
                 }
 
                 match op {
-                    // FIXME: reject operations that can fail -- namely, division and modulo.
+                    BinOp::Div | BinOp::Rem => {
+                        if !self.explicit && lhs_ty.is_integral() {
+                            // Integer division: the RHS must be a non-zero const.
+                            let const_val = match rhs {
+                                Operand::Constant(c) => {
+                                    c.literal.try_eval_bits(self.tcx, self.param_env, lhs_ty)
+                                }
+                                _ => None,
+                            };
+                            match const_val {
+                                Some(x) if x != 0 => {}        // okay
+                                _ => return Err(Unpromotable), // value not known or 0 -- not okay
+                            }
+                        }
+                    }
+                    // The remaining operations can never fail.
                     BinOp::Eq
                     | BinOp::Ne
                     | BinOp::Le
@@ -645,8 +712,6 @@ impl<'tcx> Validator<'_, 'tcx> {
                     | BinOp::Add
                     | BinOp::Sub
                     | BinOp::Mul
-                    | BinOp::Div
-                    | BinOp::Rem
                     | BinOp::BitXor
                     | BinOp::BitAnd
                     | BinOp::BitOr
@@ -658,11 +723,6 @@ impl<'tcx> Validator<'_, 'tcx> {
                 self.validate_operand(rhs)?;
             }
 
-            Rvalue::NullaryOp(op, _) => match op {
-                NullOp::Box => return Err(Unpromotable),
-                NullOp::SizeOf => {}
-            },
-
             Rvalue::AddressOf(_, place) => {
                 // We accept `&raw *`, i.e., raw reborrows -- creating a raw pointer is
                 // no problem, only using it is.

src/test/ui/consts/array-literal-index-oob.rs

@@ -6,6 +6,4 @@
 fn main() {
     &{ [1, 2, 3][4] };
     //~^ WARN operation will panic
-    //~| WARN reaching this expression at runtime will panic or abort
-    //~| WARN erroneous constant used [const_err]
 }

src/test/ui/consts/array-literal-index-oob.stderr

@@ -10,25 +10,5 @@ note: the lint level is defined here
 LL | #![warn(const_err, unconditional_panic)]
    |                    ^^^^^^^^^^^^^^^^^^^
 
-warning: reaching this expression at runtime will panic or abort
-  --> $DIR/array-literal-index-oob.rs:7:8
-   |
-LL |     &{ [1, 2, 3][4] };
-   |     ---^^^^^^^^^^^^--
-   |        |
-   |        indexing out of bounds: the len is 3 but the index is 4
-   |
-note: the lint level is defined here
-  --> $DIR/array-literal-index-oob.rs:4:9
-   |
-LL | #![warn(const_err, unconditional_panic)]
-   |         ^^^^^^^^^
-
-warning: erroneous constant used
-  --> $DIR/array-literal-index-oob.rs:7:5
-   |
-LL |     &{ [1, 2, 3][4] };
-   |     ^^^^^^^^^^^^^^^^^ referenced constant has errors
-
-warning: 3 warnings emitted
+warning: 1 warning emitted
 

src/test/ui/consts/const-eval/const-eval-query-stack.rs

@@ -1,4 +1,5 @@
-// compile-flags: -Ztreat-err-as-bug
+//~ERROR constructed but no error reported
+// compile-flags: -Ztreat-err-as-bug=2
 // build-fail
 // failure-status: 101
 // rustc-env:RUST_BACKTRACE=1
@@ -15,8 +16,11 @@
 
 #![allow(unconditional_panic)]
 
+#[warn(const_err)]
+const X: i32 = 1 / 0; //~WARN any use of this value will cause an error
+
 fn main() {
-    let x: &'static i32 = &(1 / 0);
-    //~^ ERROR reaching this expression at runtime will panic or abort [const_err]
+    let x: &'static i32 = &X;
+    //~^ ERROR evaluation of constant expression failed
     println!("x={}", x);
 }

src/test/ui/consts/const-eval/const-eval-query-stack.stderr

@@ -1,18 +1,26 @@
-error: reaching this expression at runtime will panic or abort
-  --> $DIR/const-eval-query-stack.rs:19:28
+warning: any use of this value will cause an error
+  --> $DIR/const-eval-query-stack.rs:20:16
    |
-LL |     let x: &'static i32 = &(1 / 0);
-   |                           -^^^^^^^
-   |                            |
-   |                            dividing by zero
+LL | const X: i32 = 1 / 0;
+   | ---------------^^^^^-
+   |                |
+   |                attempt to divide `1_i32` by zero
+   |
+note: the lint level is defined here
+  --> $DIR/const-eval-query-stack.rs:19:8
    |
-   = note: `#[deny(const_err)]` on by default
+LL | #[warn(const_err)]
+   |        ^^^^^^^^^
 
+error[E0080]: evaluation of constant expression failed
+  --> $DIR/const-eval-query-stack.rs:23:27
+   |
+LL |     let x: &'static i32 = &X;
+   |                           ^-
+   |                            |
+   |                            referenced constant has errors
 query stack during panic:
-#0 [eval_to_allocation_raw] const-evaluating + checking `main::promoted[1]`
-#1 [eval_to_const_value_raw] simplifying constant for the type system `main::promoted[1]`
-#2 [eval_to_const_value_raw] simplifying constant for the type system `main::promoted[1]`
-#3 [normalize_generic_arg_after_erasing_regions] normalizing `main::promoted[1]`
-#4 [optimized_mir] optimizing MIR for `main`
-#5 [collect_and_partition_mono_items] collect_and_partition_mono_items
+#0 [normalize_generic_arg_after_erasing_regions] normalizing `main::promoted[1]`
+#1 [optimized_mir] optimizing MIR for `main`
+#2 [collect_and_partition_mono_items] collect_and_partition_mono_items
 end of query stack