Auto merge of #122684 - oli-obk:delay_interning_errors_to_after_validaiton, r=RalfJung

Delay interning errors to after validation

fixes https://github.com/rust-lang/rust/issues/122398
fixes #122548

This improves diagnostics since validation errors are usually more helpful compared with interning errors that just make broad statements about the entire constant

r? `@RalfJung`

4 files changed, 129 insertions, 87 deletions

diff --git a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs
index 4283ebc99d2..40afd9f162f 100644
--- a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs
+++ b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs
@@ -10,20 +10,21 @@ use rustc_middle::traits::Reveal;
 use rustc_middle::ty::layout::LayoutOf;
 use rustc_middle::ty::print::with_no_trimmed_paths;
 use rustc_middle::ty::{self, Ty, TyCtxt};
+use rustc_session::lint;
 use rustc_span::def_id::LocalDefId;
 use rustc_span::Span;
 use rustc_target::abi::{self, Abi};
 
 use super::{CanAccessMutGlobal, CompileTimeEvalContext, CompileTimeInterpreter};
 use crate::const_eval::CheckAlignment;
-use crate::errors;
 use crate::errors::ConstEvalError;
-use crate::interpret::eval_nullary_intrinsic;
+use crate::errors::{self, DanglingPtrInFinal};
 use crate::interpret::{
     create_static_alloc, intern_const_alloc_recursive, CtfeValidationMode, GlobalId, Immediate,
     InternKind, InterpCx, InterpError, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking,
     StackPopCleanup,
 };
+use crate::interpret::{eval_nullary_intrinsic, InternResult};
 use crate::CTRL_C_RECEIVED;
 
 // Returns a pointer to where the result lives
@@ -89,11 +90,35 @@ fn eval_body_using_ecx<'mir, 'tcx, R: InterpretationResult<'tcx>>(
     }
 
     // Intern the result
-    intern_const_alloc_recursive(ecx, intern_kind, &ret)?;
+    let intern_result = intern_const_alloc_recursive(ecx, intern_kind, &ret);
 
     // Since evaluation had no errors, validate the resulting constant.
     const_validate_mplace(&ecx, &ret, cid)?;
 
+    // Only report this after validation, as validaiton produces much better diagnostics.
+    // FIXME: ensure validation always reports this and stop making interning care about it.
+
+    match intern_result {
+        Ok(()) => {}
+        Err(InternResult::FoundDanglingPointer) => {
+            return Err(ecx
+                .tcx
+                .dcx()
+                .emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
+                .into());
+        }
+        Err(InternResult::FoundBadMutablePointer) => {
+            // only report mutable pointers if there were no dangling pointers
+            let err_diag = errors::MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind };
+            ecx.tcx.emit_node_span_lint(
+                lint::builtin::CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
+                ecx.best_lint_scope(),
+                err_diag.span,
+                err_diag,
+            )
+        }
+    }
+
     Ok(R::make_result(ret, ecx))
 }
 
diff --git a/compiler/rustc_const_eval/src/interpret/intern.rs b/compiler/rustc_const_eval/src/interpret/intern.rs
index d0f0190fea7..d4168273f29 100644
--- a/compiler/rustc_const_eval/src/interpret/intern.rs
+++ b/compiler/rustc_const_eval/src/interpret/intern.rs
@@ -16,19 +16,17 @@
 use hir::def::DefKind;
 use rustc_ast::Mutability;
 use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
-use rustc_errors::ErrorGuaranteed;
 use rustc_hir as hir;
 use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrs;
 use rustc_middle::mir::interpret::{ConstAllocation, CtfeProvenance, InterpResult};
 use rustc_middle::query::TyCtxtAt;
 use rustc_middle::ty::layout::TyAndLayout;
-use rustc_session::lint;
 use rustc_span::def_id::LocalDefId;
 use rustc_span::sym;
 
 use super::{AllocId, Allocation, InterpCx, MPlaceTy, Machine, MemoryKind, PlaceTy};
 use crate::const_eval;
-use crate::errors::{DanglingPtrInFinal, MutablePtrInFinal, NestedStaticInThreadLocal};
+use crate::errors::NestedStaticInThreadLocal;
 
 pub trait CompileTimeMachine<'mir, 'tcx: 'mir, T> = Machine<
         'mir,
@@ -134,6 +132,12 @@ pub enum InternKind {
     Promoted,
 }
 
+#[derive(Debug)]
+pub enum InternResult {
+    FoundBadMutablePointer,
+    FoundDanglingPointer,
+}
+
 /// Intern `ret` and everything it references.
 ///
 /// This *cannot raise an interpreter error*. Doing so is left to validation, which
@@ -149,7 +153,7 @@ pub fn intern_const_alloc_recursive<
     ecx: &mut InterpCx<'mir, 'tcx, M>,
     intern_kind: InternKind,
     ret: &MPlaceTy<'tcx>,
-) -> Result<(), ErrorGuaranteed> {
+) -> Result<(), InternResult> {
     // We are interning recursively, and for mutability we are distinguishing the "root" allocation
     // that we are starting in, and all other allocations that we are encountering recursively.
     let (base_mutability, inner_mutability, is_static) = match intern_kind {
@@ -201,7 +205,7 @@ pub fn intern_const_alloc_recursive<
     // Whether we encountered a bad mutable pointer.
     // We want to first report "dangling" and then "mutable", so we need to delay reporting these
     // errors.
-    let mut found_bad_mutable_pointer = false;
+    let mut result = Ok(());
 
     // Keep interning as long as there are things to intern.
     // We show errors if there are dangling pointers, or mutable pointers in immutable contexts
@@ -251,7 +255,10 @@ pub fn intern_const_alloc_recursive<
             // on the promotion analysis not screwing up to ensure that it is sound to intern
             // promoteds as immutable.
             trace!("found bad mutable pointer");
-            found_bad_mutable_pointer = true;
+            // Prefer dangling pointer errors over mutable pointer errors
+            if result.is_ok() {
+                result = Err(InternResult::FoundBadMutablePointer);
+            }
         }
         if ecx.tcx.try_get_global_alloc(alloc_id).is_some() {
             // Already interned.
@@ -269,21 +276,15 @@ pub fn intern_const_alloc_recursive<
         // pointers before deciding which allocations can be made immutable; but for now we are
         // okay with losing some potential for immutability here. This can anyway only affect
         // `static mut`.
-        todo.extend(intern_shallow(ecx, alloc_id, inner_mutability).map_err(|()| {
-            ecx.tcx.dcx().emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
-        })?);
-    }
-    if found_bad_mutable_pointer {
-        let err_diag = MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind };
-        ecx.tcx.emit_node_span_lint(
-            lint::builtin::CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
-            ecx.best_lint_scope(),
-            err_diag.span,
-            err_diag,
-        )
+        match intern_shallow(ecx, alloc_id, inner_mutability) {
+            Ok(nested) => todo.extend(nested),
+            Err(()) => {
+                ecx.tcx.dcx().delayed_bug("found dangling pointer during const interning");
+                result = Err(InternResult::FoundDanglingPointer);
+            }
+        }
     }
-
-    Ok(())
+    result
 }
 
 /// Intern `ret`. This function assumes that `ret` references no other allocation.
diff --git a/compiler/rustc_const_eval/src/interpret/mod.rs b/compiler/rustc_const_eval/src/interpret/mod.rs
index 474d35b2aa3..7ede90ad13f 100644
--- a/compiler/rustc_const_eval/src/interpret/mod.rs
+++ b/compiler/rustc_const_eval/src/interpret/mod.rs
@@ -23,6 +23,7 @@ pub use rustc_middle::mir::interpret::*; // have all the `interpret` symbols in
 pub use self::eval_context::{format_interp_error, Frame, FrameInfo, InterpCx, StackPopCleanup};
 pub use self::intern::{
     intern_const_alloc_for_constprop, intern_const_alloc_recursive, HasStaticRootDefId, InternKind,
+    InternResult,
 };
 pub use self::machine::{compile_time_machine, AllocMap, Machine, MayLeak, StackPopJump};
 pub use self::memory::{AllocKind, AllocRef, AllocRefMut, FnVal, Memory, MemoryKind};
diff --git a/compiler/rustc_const_eval/src/interpret/validity.rs b/compiler/rustc_const_eval/src/interpret/validity.rs
index 9911c59d4b8..b8a1733e45a 100644
--- a/compiler/rustc_const_eval/src/interpret/validity.rs
+++ b/compiler/rustc_const_eval/src/interpret/validity.rs
@@ -449,67 +449,41 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
             // `!` is a ZST and we want to validate it.
             if let Ok((alloc_id, _offset, _prov)) = self.ecx.ptr_try_get_alloc_id(place.ptr()) {
                 let mut skip_recursive_check = false;
-                // Let's see what kind of memory this points to.
-                // `unwrap` since dangling pointers have already been handled.
-                let alloc_kind = self.ecx.tcx.try_get_global_alloc(alloc_id).unwrap();
-                let alloc_actual_mutbl = match alloc_kind {
-                    GlobalAlloc::Static(did) => {
-                        // Special handling for pointers to statics (irrespective of their type).
-                        assert!(!self.ecx.tcx.is_thread_local_static(did));
-                        assert!(self.ecx.tcx.is_static(did));
-                        // Mode-specific checks
-                        match self.ctfe_mode {
-                            Some(
-                                CtfeValidationMode::Static { .. }
-                                | CtfeValidationMode::Promoted { .. },
-                            ) => {
-                                // We skip recursively checking other statics. These statics must be sound by
-                                // themselves, and the only way to get broken statics here is by using
-                                // unsafe code.
-                                // The reasons we don't check other statics is twofold. For one, in all
-                                // sound cases, the static was already validated on its own, and second, we
-                                // trigger cycle errors if we try to compute the value of the other static
-                                // and that static refers back to us (potentially through a promoted).
-                                // This could miss some UB, but that's fine.
-                                skip_recursive_check = true;
-                            }
-                            Some(CtfeValidationMode::Const { .. }) => {
-                                // We can't recursively validate `extern static`, so we better reject them.
-                                if self.ecx.tcx.is_foreign_item(did) {
-                                    throw_validation_failure!(self.path, ConstRefToExtern);
-                                }
-                            }
-                            None => {}
+                let alloc_actual_mutbl = mutability(self.ecx, alloc_id);
+                if let GlobalAlloc::Static(did) = self.ecx.tcx.global_alloc(alloc_id) {
+                    let DefKind::Static { nested, .. } = self.ecx.tcx.def_kind(did) else { bug!() };
+                    // Special handling for pointers to statics (irrespective of their type).
+                    assert!(!self.ecx.tcx.is_thread_local_static(did));
+                    assert!(self.ecx.tcx.is_static(did));
+                    // Mode-specific checks
+                    match self.ctfe_mode {
+                        Some(
+                            CtfeValidationMode::Static { .. } | CtfeValidationMode::Promoted { .. },
+                        ) => {
+                            // We skip recursively checking other statics. These statics must be sound by
+                            // themselves, and the only way to get broken statics here is by using
+                            // unsafe code.
+                            // The reasons we don't check other statics is twofold. For one, in all
+                            // sound cases, the static was already validated on its own, and second, we
+                            // trigger cycle errors if we try to compute the value of the other static
+                            // and that static refers back to us (potentially through a promoted).
+                            // This could miss some UB, but that's fine.
+                            // We still walk nested allocations, as they are fundamentally part of this validation run.
+                            // This means we will also recurse into nested statics of *other*
+                            // statics, even though we do not recurse into other statics directly.
+                            // That's somewhat inconsistent but harmless.
+                            skip_recursive_check = !nested;
                         }
-                        // Return alloc mutability. For "root" statics we look at the type to account for interior
-                        // mutability; for nested statics we have no type and directly use the annotated mutability.
-                        let DefKind::Static { mutability, nested } = self.ecx.tcx.def_kind(did)
-                        else {
-                            bug!()
-                        };
-                        match (mutability, nested) {
-                            (Mutability::Mut, _) => Mutability::Mut,
-                            (Mutability::Not, true) => Mutability::Not,
-                            (Mutability::Not, false)
-                                if !self
-                                    .ecx
-                                    .tcx
-                                    .type_of(did)
-                                    .no_bound_vars()
-                                    .expect("statics should not have generic parameters")
-                                    .is_freeze(*self.ecx.tcx, ty::ParamEnv::reveal_all()) =>
-                            {
-                                Mutability::Mut
+                        Some(CtfeValidationMode::Const { .. }) => {
+                            // We can't recursively validate `extern static`, so we better reject them.
+                            if self.ecx.tcx.is_foreign_item(did) {
+                                throw_validation_failure!(self.path, ConstRefToExtern);
                             }
-                            (Mutability::Not, false) => Mutability::Not,
                         }
+                        None => {}
                     }
-                    GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
-                    GlobalAlloc::Function(..) | GlobalAlloc::VTable(..) => {
-                        // These are immutable, we better don't allow mutable pointers here.
-                        Mutability::Not
-                    }
-                };
+                }
+
                 // Mutability check.
                 // If this allocation has size zero, there is no actual mutability here.
                 let (size, _align, _alloc_kind) = self.ecx.get_alloc_info(alloc_id);
@@ -708,17 +682,58 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
     fn in_mutable_memory(&self, op: &OpTy<'tcx, M::Provenance>) -> bool {
         if let Some(mplace) = op.as_mplace_or_imm().left() {
             if let Some(alloc_id) = mplace.ptr().provenance.and_then(|p| p.get_alloc_id()) {
-                let mutability = match self.ecx.tcx.global_alloc(alloc_id) {
-                    GlobalAlloc::Static(_) => {
-                        self.ecx.memory.alloc_map.get(alloc_id).unwrap().1.mutability
+                return mutability(self.ecx, alloc_id).is_mut();
+            }
+        }
+        false
+    }
+}
+
+/// Returns whether the allocation is mutable, and whether it's actually a static.
+/// For "root" statics we look at the type to account for interior
+/// mutability; for nested statics we have no type and directly use the annotated mutability.
+fn mutability<'mir, 'tcx: 'mir>(
+    ecx: &InterpCx<'mir, 'tcx, impl Machine<'mir, 'tcx>>,
+    alloc_id: AllocId,
+) -> Mutability {
+    // Let's see what kind of memory this points to.
+    // We're not using `try_global_alloc` since dangling pointers have already been handled.
+    match ecx.tcx.global_alloc(alloc_id) {
+        GlobalAlloc::Static(did) => {
+            let DefKind::Static { mutability, nested } = ecx.tcx.def_kind(did) else { bug!() };
+            if nested {
+                assert!(
+                    ecx.memory.alloc_map.get(alloc_id).is_none(),
+                    "allocations of nested statics are already interned: {alloc_id:?}, {did:?}"
+                );
+                // Nested statics in a `static` are never interior mutable,
+                // so just use the declared mutability.
+                mutability
+            } else {
+                let mutability = match mutability {
+                    Mutability::Not
+                        if !ecx
+                            .tcx
+                            .type_of(did)
+                            .no_bound_vars()
+                            .expect("statics should not have generic parameters")
+                            .is_freeze(*ecx.tcx, ty::ParamEnv::reveal_all()) =>
+                    {
+                        Mutability::Mut
                     }
-                    GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
-                    _ => span_bug!(self.ecx.tcx.span, "not a memory allocation"),
+                    _ => mutability,
                 };
-                return mutability == Mutability::Mut;
+                if let Some((_, alloc)) = ecx.memory.alloc_map.get(alloc_id) {
+                    assert_eq!(alloc.mutability, mutability);
+                }
+                mutability
             }
         }
-        false
+        GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
+        GlobalAlloc::Function(..) | GlobalAlloc::VTable(..) => {
+            // These are immutable, we better don't allow mutable pointers here.
+            Mutability::Not
+        }
     }
 }