Auto merge of #119627 - oli-obk:const_prop_lint_n̵o̵n̵sense, r=cjgillot

Remove all ConstPropNonsense

We track all locals and projections on them ourselves within the const propagator and only use the InterpCx to actually do some low level operations or read from constants (via `OpTy` we get for said constants).

This helps moving the const prop lint out from the normal pipeline and running it just based on borrowck information. This in turn allows us to make progress on #108730 (comment)

there are various follow up cleanups that can be done after this PR (e.g. not matching on Rvalue twice and doing binop checks twice), but lets try landing this one first.

r? `@RalfJung`

Author: bors

compiler/rustc_const_eval/src/errors.rs

@@ -864,9 +864,6 @@ impl<'tcx> ReportErrorExt for InvalidProgramInfo<'tcx> {
             InvalidProgramInfo::FnAbiAdjustForForeignAbi(_) => {
                 rustc_middle::error::middle_adjust_for_foreign_abi_error
             }
-            InvalidProgramInfo::ConstPropNonsense => {
-                panic!("We had const-prop nonsense, this should never be printed")
-            }
         }
     }
     fn add_args<G: EmissionGuarantee>(
@@ -875,9 +872,7 @@ impl<'tcx> ReportErrorExt for InvalidProgramInfo<'tcx> {
         builder: &mut DiagnosticBuilder<'_, G>,
     ) {
         match self {
-            InvalidProgramInfo::TooGeneric
-            | InvalidProgramInfo::AlreadyReported(_)
-            | InvalidProgramInfo::ConstPropNonsense => {}
+            InvalidProgramInfo::TooGeneric | InvalidProgramInfo::AlreadyReported(_) => {}
             InvalidProgramInfo::Layout(e) => {
                 // The level doesn't matter, `diag` is consumed without it being used.
                 let dummy_level = Level::Bug;

compiler/rustc_const_eval/src/interpret/operand.rs

@@ -643,11 +643,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         let layout = self.layout_of_local(frame, local, layout)?;
         let op = *frame.locals[local].access()?;
         if matches!(op, Operand::Immediate(_)) {
-            if layout.is_unsized() {
-                // ConstProp marks *all* locals as `Immediate::Uninit` since it cannot
-                // efficiently check whether they are sized. We have to catch that case here.
-                throw_inval!(ConstPropNonsense);
-            }
+            assert!(!layout.is_unsized());
         }
         Ok(OpTy { op, layout })
     }

compiler/rustc_const_eval/src/interpret/place.rs

@@ -519,11 +519,7 @@ where
         } else {
             // Unsized `Local` isn't okay (we cannot store the metadata).
             match frame_ref.locals[local].access()? {
-                Operand::Immediate(_) => {
-                    // ConstProp marks *all* locals as `Immediate::Uninit` since it cannot
-                    // efficiently check whether they are sized. We have to catch that case here.
-                    throw_inval!(ConstPropNonsense);
-                }
+                Operand::Immediate(_) => bug!(),
                 Operand::Indirect(mplace) => Place::Ptr(*mplace),
             }
         };
@@ -816,17 +812,8 @@ where
         // avoid force_allocation.
         let src = match self.read_immediate_raw(src)? {
             Right(src_val) => {
-                // FIXME(const_prop): Const-prop can possibly evaluate an
-                // unsized copy operation when it thinks that the type is
-                // actually sized, due to a trivially false where-clause
-                // predicate like `where Self: Sized` with `Self = dyn Trait`.
-                // See #102553 for an example of such a predicate.
-                if src.layout().is_unsized() {
-                    throw_inval!(ConstPropNonsense);
-                }
-                if dest.layout().is_unsized() {
-                    throw_inval!(ConstPropNonsense);
-                }
+                assert!(!src.layout().is_unsized());
+                assert!(!dest.layout().is_unsized());
                 assert_eq!(src.layout().size, dest.layout().size);
                 // Yay, we got a value that we can write directly.
                 return if layout_compat {

compiler/rustc_const_eval/src/interpret/projection.rs

@@ -153,11 +153,7 @@ where
 
         // Offset may need adjustment for unsized fields.
         let (meta, offset) = if field_layout.is_unsized() {
-            if base.layout().is_sized() {
-                // An unsized field of a sized type? Sure...
-                // But const-prop actually feeds us such nonsense MIR! (see test `const_prop/issue-86351.rs`)
-                throw_inval!(ConstPropNonsense);
-            }
+            assert!(!base.layout().is_sized());
             let base_meta = base.meta();
             // Re-use parent metadata to determine dynamic field layout.
             // With custom DSTS, this *will* execute user-defined code, but the same
@@ -205,29 +201,26 @@ where
         // see https://github.com/rust-lang/rust/issues/93688#issuecomment-1032929496.)
         // So we just "offset" by 0.
         let layout = base.layout().for_variant(self, variant);
-        if layout.abi.is_uninhabited() {
-            // `read_discriminant` should have excluded uninhabited variants... but ConstProp calls
-            // us on dead code.
-            // In the future we might want to allow this to permit code like this:
-            // (this is a Rust/MIR pseudocode mix)
-            // ```
-            // enum Option2 {
-            //   Some(i32, !),
-            //   None,
-            // }
-            //
-            // fn panic() -> ! { panic!() }
-            //
-            // let x: Option2;
-            // x.Some.0 = 42;
-            // x.Some.1 = panic();
-            // SetDiscriminant(x, Some);
-            // ```
-            // However, for now we don't generate such MIR, and this check here *has* found real
-            // bugs (see https://github.com/rust-lang/rust/issues/115145), so we will keep rejecting
-            // it.
-            throw_inval!(ConstPropNonsense)
-        }
+        // In the future we might want to allow this to permit code like this:
+        // (this is a Rust/MIR pseudocode mix)
+        // ```
+        // enum Option2 {
+        //   Some(i32, !),
+        //   None,
+        // }
+        //
+        // fn panic() -> ! { panic!() }
+        //
+        // let x: Option2;
+        // x.Some.0 = 42;
+        // x.Some.1 = panic();
+        // SetDiscriminant(x, Some);
+        // ```
+        // However, for now we don't generate such MIR, and this check here *has* found real
+        // bugs (see https://github.com/rust-lang/rust/issues/115145), so we will keep rejecting
+        // it.
+        assert!(!layout.abi.is_uninhabited());
+
         // This cannot be `transmute` as variants *can* have a smaller size than the entire enum.
         base.offset(Size::ZERO, layout, self)
     }

compiler/rustc_const_eval/src/util/mod.rs

@@ -14,7 +14,7 @@ pub use self::type_name::type_name;
 /// Classify whether an operator is "left-homogeneous", i.e., the LHS has the
 /// same type as the result.
 #[inline]
-pub(crate) fn binop_left_homogeneous(op: mir::BinOp) -> bool {
+pub fn binop_left_homogeneous(op: mir::BinOp) -> bool {
     use rustc_middle::mir::BinOp::*;
     match op {
         Add | AddUnchecked | Sub | SubUnchecked | Mul | MulUnchecked | Div | Rem | BitXor
@@ -26,7 +26,7 @@ pub(crate) fn binop_left_homogeneous(op: mir::BinOp) -> bool {
 /// Classify whether an operator is "right-homogeneous", i.e., the RHS has the
 /// same type as the LHS.
 #[inline]
-pub(crate) fn binop_right_homogeneous(op: mir::BinOp) -> bool {
+pub fn binop_right_homogeneous(op: mir::BinOp) -> bool {
     use rustc_middle::mir::BinOp::*;
     match op {
         Add | AddUnchecked | Sub | SubUnchecked | Mul | MulUnchecked | Div | Rem | BitXor

compiler/rustc_middle/src/mir/interpret/error.rs

@@ -200,8 +200,6 @@ pub enum InvalidProgramInfo<'tcx> {
     /// (which unfortunately typeck does not reject).
     /// Not using `FnAbiError` as that contains a nested `LayoutError`.
     FnAbiAdjustForForeignAbi(call::AdjustForForeignAbiError),
-    /// We are runnning into a nonsense situation due to ConstProp violating our invariants.
-    ConstPropNonsense,
 }
 
 /// Details of why a pointer had to be in-bounds.

compiler/rustc_mir_transform/src/const_prop.rs

@@ -1,21 +1,12 @@
 //! Propagates constants for early reporting of statically known
 //! assertion failures
 
-use rustc_const_eval::interpret::{
-    self, compile_time_machine, AllocId, ConstAllocation, FnArg, Frame, ImmTy, InterpCx,
-    InterpResult, OpTy, PlaceTy, Pointer,
-};
-use rustc_data_structures::fx::FxHashSet;
 use rustc_index::bit_set::BitSet;
 use rustc_index::IndexVec;
 use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor};
 use rustc_middle::mir::*;
-use rustc_middle::query::TyCtxtAt;
-use rustc_middle::ty::layout::TyAndLayout;
-use rustc_middle::ty::{self, ParamEnv, TyCtxt};
-use rustc_span::def_id::DefId;
+use rustc_middle::ty::{ParamEnv, TyCtxt};
 use rustc_target::abi::Size;
-use rustc_target::spec::abi::Abi as CallAbi;
 
 /// The maximum number of bytes that we'll allocate space for a local or the return value.
 /// Needed for #66397, because otherwise we eval into large places and that can cause OOM or just
@@ -49,162 +40,6 @@ pub(crate) macro throw_machine_stop_str($($tt:tt)*) {{
     throw_machine_stop!(Zst)
 }}
 
-pub(crate) struct ConstPropMachine<'mir, 'tcx> {
-    /// The virtual call stack.
-    stack: Vec<Frame<'mir, 'tcx>>,
-    pub written_only_inside_own_block_locals: FxHashSet<Local>,
-    pub can_const_prop: IndexVec<Local, ConstPropMode>,
-}
-
-impl ConstPropMachine<'_, '_> {
-    pub fn new(can_const_prop: IndexVec<Local, ConstPropMode>) -> Self {
-        Self {
-            stack: Vec::new(),
-            written_only_inside_own_block_locals: Default::default(),
-            can_const_prop,
-        }
-    }
-}
-
-impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx> {
-    compile_time_machine!(<'mir, 'tcx>);
-
-    const PANIC_ON_ALLOC_FAIL: bool = true; // all allocations are small (see `MAX_ALLOC_LIMIT`)
-
-    const POST_MONO_CHECKS: bool = false; // this MIR is still generic!
-
-    type MemoryKind = !;
-
-    #[inline(always)]
-    fn enforce_alignment(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool {
-        false // no reason to enforce alignment
-    }
-
-    #[inline(always)]
-    fn enforce_validity(_ecx: &InterpCx<'mir, 'tcx, Self>, _layout: TyAndLayout<'tcx>) -> bool {
-        false // for now, we don't enforce validity
-    }
-
-    fn load_mir(
-        _ecx: &InterpCx<'mir, 'tcx, Self>,
-        _instance: ty::InstanceDef<'tcx>,
-    ) -> InterpResult<'tcx, &'tcx Body<'tcx>> {
-        throw_machine_stop_str!("calling functions isn't supported in ConstProp")
-    }
-
-    fn panic_nounwind(_ecx: &mut InterpCx<'mir, 'tcx, Self>, _msg: &str) -> InterpResult<'tcx> {
-        throw_machine_stop_str!("panicking isn't supported in ConstProp")
-    }
-
-    fn find_mir_or_eval_fn(
-        _ecx: &mut InterpCx<'mir, 'tcx, Self>,
-        _instance: ty::Instance<'tcx>,
-        _abi: CallAbi,
-        _args: &[FnArg<'tcx>],
-        _destination: &PlaceTy<'tcx>,
-        _target: Option<BasicBlock>,
-        _unwind: UnwindAction,
-    ) -> InterpResult<'tcx, Option<(&'mir Body<'tcx>, ty::Instance<'tcx>)>> {
-        Ok(None)
-    }
-
-    fn call_intrinsic(
-        _ecx: &mut InterpCx<'mir, 'tcx, Self>,
-        _instance: ty::Instance<'tcx>,
-        _args: &[OpTy<'tcx>],
-        _destination: &PlaceTy<'tcx>,
-        _target: Option<BasicBlock>,
-        _unwind: UnwindAction,
-    ) -> InterpResult<'tcx> {
-        throw_machine_stop_str!("calling intrinsics isn't supported in ConstProp")
-    }
-
-    fn assert_panic(
-        _ecx: &mut InterpCx<'mir, 'tcx, Self>,
-        _msg: &rustc_middle::mir::AssertMessage<'tcx>,
-        _unwind: rustc_middle::mir::UnwindAction,
-    ) -> InterpResult<'tcx> {
-        bug!("panics terminators are not evaluated in ConstProp")
-    }
-
-    fn binary_ptr_op(
-        _ecx: &InterpCx<'mir, 'tcx, Self>,
-        _bin_op: BinOp,
-        _left: &ImmTy<'tcx>,
-        _right: &ImmTy<'tcx>,
-    ) -> InterpResult<'tcx, (ImmTy<'tcx>, bool)> {
-        // We can't do this because aliasing of memory can differ between const eval and llvm
-        throw_machine_stop_str!("pointer arithmetic or comparisons aren't supported in ConstProp")
-    }
-
-    fn before_access_local_mut<'a>(
-        ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
-        frame: usize,
-        local: Local,
-    ) -> InterpResult<'tcx> {
-        assert_eq!(frame, 0);
-        match ecx.machine.can_const_prop[local] {
-            ConstPropMode::NoPropagation => {
-                throw_machine_stop_str!(
-                    "tried to write to a local that is marked as not propagatable"
-                )
-            }
-            ConstPropMode::OnlyInsideOwnBlock => {
-                ecx.machine.written_only_inside_own_block_locals.insert(local);
-            }
-            ConstPropMode::FullConstProp => {}
-        }
-        Ok(())
-    }
-
-    fn before_access_global(
-        _tcx: TyCtxtAt<'tcx>,
-        _machine: &Self,
-        _alloc_id: AllocId,
-        alloc: ConstAllocation<'tcx>,
-        _static_def_id: Option<DefId>,
-        is_write: bool,
-    ) -> InterpResult<'tcx> {
-        if is_write {
-            throw_machine_stop_str!("can't write to global");
-        }
-        // If the static allocation is mutable, then we can't const prop it as its content
-        // might be different at runtime.
-        if alloc.inner().mutability.is_mut() {
-            throw_machine_stop_str!("can't access mutable globals in ConstProp");
-        }
-
-        Ok(())
-    }
-
-    #[inline(always)]
-    fn expose_ptr(_ecx: &mut InterpCx<'mir, 'tcx, Self>, _ptr: Pointer) -> InterpResult<'tcx> {
-        throw_machine_stop_str!("exposing pointers isn't supported in ConstProp")
-    }
-
-    #[inline(always)]
-    fn init_frame_extra(
-        _ecx: &mut InterpCx<'mir, 'tcx, Self>,
-        frame: Frame<'mir, 'tcx>,
-    ) -> InterpResult<'tcx, Frame<'mir, 'tcx>> {
-        Ok(frame)
-    }
-
-    #[inline(always)]
-    fn stack<'a>(
-        ecx: &'a InterpCx<'mir, 'tcx, Self>,
-    ) -> &'a [Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>] {
-        &ecx.machine.stack
-    }
-
-    #[inline(always)]
-    fn stack_mut<'a>(
-        ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
-    ) -> &'a mut Vec<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>> {
-        &mut ecx.machine.stack
-    }
-}
-
 /// The mode that `ConstProp` is allowed to run in for a given `Local`.
 #[derive(Clone, Copy, Debug, PartialEq)]
 pub enum ConstPropMode {

compiler/rustc_mir_transform/src/const_prop_lint.rs

@@ -403,7 +403,12 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> {
             operand,
             &mut |elem, op| match elem {
                 TrackElem::Field(idx) => self.ecx.project_field(op, idx.as_usize()).ok(),
-                TrackElem::Variant(idx) => self.ecx.project_downcast(op, idx).ok(),
+                TrackElem::Variant(idx) => {
+                    if op.layout.for_variant(&self.ecx, idx).abi.is_uninhabited() {
+                        return None;
+                    }
+                    self.ecx.project_downcast(op, idx).ok()
+                }
                 TrackElem::Discriminant => {
                     let variant = self.ecx.read_discriminant(op).ok()?;
                     let discr_value =

compiler/rustc_mir_transform/src/dataflow_const_prop.rs

@@ -201,45 +201,39 @@ impl<'a> DecorateLint<'a, ()> for UnsafeOpInUnsafeFn {
     }
 }
 
-pub(crate) enum AssertLint<P> {
-    ArithmeticOverflow(Span, AssertKind<P>),
-    UnconditionalPanic(Span, AssertKind<P>),
+pub(crate) struct AssertLint<P> {
+    pub span: Span,
+    pub assert_kind: AssertKind<P>,
+    pub lint_kind: AssertLintKind,
+}
+
+pub(crate) enum AssertLintKind {
+    ArithmeticOverflow,
+    UnconditionalPanic,
 }
 
 impl<'a, P: std::fmt::Debug> DecorateLint<'a, ()> for AssertLint<P> {
     fn decorate_lint<'b>(self, diag: &'b mut DiagnosticBuilder<'a, ()>) {
-        let span = self.span();
-        let assert_kind = self.panic();
-        let message = assert_kind.diagnostic_message();
-        assert_kind.add_args(&mut |name, value| {
+        let message = self.assert_kind.diagnostic_message();
+        self.assert_kind.add_args(&mut |name, value| {
             diag.arg(name, value);
         });
-        diag.span_label(span, message);
+        diag.span_label(self.span, message);
     }
 
     fn msg(&self) -> DiagnosticMessage {
-        match self {
-            AssertLint::ArithmeticOverflow(..) => fluent::mir_transform_arithmetic_overflow,
-            AssertLint::UnconditionalPanic(..) => fluent::mir_transform_operation_will_panic,
+        match self.lint_kind {
+            AssertLintKind::ArithmeticOverflow => fluent::mir_transform_arithmetic_overflow,
+            AssertLintKind::UnconditionalPanic => fluent::mir_transform_operation_will_panic,
         }
     }
 }
 
-impl<P> AssertLint<P> {
+impl AssertLintKind {
     pub fn lint(&self) -> &'static Lint {
         match self {
-            AssertLint::ArithmeticOverflow(..) => lint::builtin::ARITHMETIC_OVERFLOW,
-            AssertLint::UnconditionalPanic(..) => lint::builtin::UNCONDITIONAL_PANIC,
-        }
-    }
-    pub fn span(&self) -> Span {
-        match self {
-            AssertLint::ArithmeticOverflow(sp, _) | AssertLint::UnconditionalPanic(sp, _) => *sp,
-        }
-    }
-    pub fn panic(self) -> AssertKind<P> {
-        match self {
-            AssertLint::ArithmeticOverflow(_, p) | AssertLint::UnconditionalPanic(_, p) => p,
+            AssertLintKind::ArithmeticOverflow => lint::builtin::ARITHMETIC_OVERFLOW,
+            AssertLintKind::UnconditionalPanic => lint::builtin::UNCONDITIONAL_PANIC,
         }
     }
 }

compiler/rustc_mir_transform/src/errors.rs

@@ -33,7 +33,6 @@ fn main() {
     INT_MIN % NEG_ONE;
     //~^ ERROR: this operation will panic at runtime
     //~| ERROR: any number modulo -1 will panic/overflow or result in 0
-    // ONLY caught by rustc
+    // Not caught by lint, we don't look into static items, even if entirely immutable.
     INT_MIN % STATIC_NEG_ONE;
-    //~^ ERROR: this operation will panic at runtime
 }

src/tools/clippy/tests/ui/modulo_one.rs

@@ -12,12 +12,6 @@ error: this operation will panic at runtime
 LL |     INT_MIN % NEG_ONE;
    |     ^^^^^^^^^^^^^^^^^ attempt to compute `i64::MIN % -1_i64`, which would overflow
 
-error: this operation will panic at runtime
-  --> $DIR/modulo_one.rs:37:5
-   |
-LL |     INT_MIN % STATIC_NEG_ONE;
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^ attempt to compute `i64::MIN % -1_i64`, which would overflow
-
 error: any number modulo 1 will be 0
   --> $DIR/modulo_one.rs:8:5
    |
@@ -57,5 +51,5 @@ error: any number modulo -1 will panic/overflow or result in 0
 LL |     INT_MIN % NEG_ONE;
    |     ^^^^^^^^^^^^^^^^^
 
-error: aborting due to 9 previous errors
+error: aborting due to 8 previous errors
 

src/tools/clippy/tests/ui/modulo_one.stderr

@@ -30,6 +30,14 @@ LL |     black_box((S::<i32>::FOO, S::<u32>::FOO));
    |
    = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
 
+note: erroneous constant encountered
+  --> $DIR/const-err-late.rs:19:31
+   |
+LL |     black_box((S::<i32>::FOO, S::<u32>::FOO));
+   |                               ^^^^^^^^^^^^^
+   |
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
 error: aborting due to 2 previous errors
 
 For more information about this error, try `rustc --explain E0080`.

tests/ui/consts/const-err-late.stderr

@@ -8,15 +8,16 @@ impl<T> Generic<T> {
     const ARRAY_FIELD: Generic<(i32, [T; 0])> = Generic((0, []));
 }
 
-pub const fn array<T>() ->  &'static T {
+pub const fn array<T>() -> &'static T {
+    #[allow(unconditional_panic)]
     &Generic::<T>::ARRAY[0]
 }
 
-pub const fn newtype_array<T>() ->  &'static T {
+pub const fn newtype_array<T>() -> &'static T {
     &Generic::<T>::NEWTYPE_ARRAY.0[0]
 }
 
-pub const fn array_field<T>() ->  &'static T {
+pub const fn array_field<T>() -> &'static T {
     &(Generic::<T>::ARRAY_FIELD.0).1[0]
 }