From 1f398abcb6e11ea0dfe8e647a87fa2763005f89a Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 5 Jan 2024 17:31:38 +0000 Subject: [PATCH] const prop nonsense eliminated --- compiler/rustc_const_eval/src/errors.rs | 7 +-- .../rustc_const_eval/src/interpret/operand.rs | 6 +-- .../rustc_const_eval/src/interpret/place.rs | 19 ++----- .../src/interpret/projection.rs | 49 ++++++++----------- .../rustc_middle/src/mir/interpret/error.rs | 2 - .../src/dataflow_const_prop.rs | 7 ++- 6 files changed, 32 insertions(+), 58 deletions(-) diff --git a/compiler/rustc_const_eval/src/errors.rs b/compiler/rustc_const_eval/src/errors.rs index 171cc89d6ad..75e21c3217e 100644 --- a/compiler/rustc_const_eval/src/errors.rs +++ b/compiler/rustc_const_eval/src/errors.rs @@ -860,9 +860,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( @@ -871,9 +868,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; diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index b39b219b46a..80d4bda4827 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/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 }) } diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 639b269ac25..041a8094fe8 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/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 { diff --git a/compiler/rustc_const_eval/src/interpret/projection.rs b/compiler/rustc_const_eval/src/interpret/projection.rs index 9a034ba22b9..bd60e066f72 100644 --- a/compiler/rustc_const_eval/src/interpret/projection.rs +++ b/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) } diff --git a/compiler/rustc_middle/src/mir/interpret/error.rs b/compiler/rustc_middle/src/mir/interpret/error.rs index 1b4e9c28635..474e156a131 100644 --- a/compiler/rustc_middle/src/mir/interpret/error.rs +++ b/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. diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs index ad12bce9b02..d5f22b2cdbc 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.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 =