promotion: do not promote const-fn calls in const when that may fail without the entire const failing

Author: RalfJung

compiler/rustc_mir_transform/src/lib.rs

@@ -343,13 +343,6 @@ fn mir_promoted(
         body.tainted_by_errors = Some(error_reported);
     }
 
-    let mut required_consts = Vec::new();
-    let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts);
-    for (bb, bb_data) in traversal::reverse_postorder(&body) {
-        required_consts_visitor.visit_basic_block_data(bb, bb_data);
-    }
-    body.required_consts = required_consts;
-
     // What we need to run borrowck etc.
     let promote_pass = promote_consts::PromoteTemps::default();
     pm::run_passes(
@@ -359,6 +352,14 @@ fn mir_promoted(
         Some(MirPhase::Analysis(AnalysisPhase::Initial)),
     );
 
+    // Promotion generates new consts; we run this after promotion to ensure they are accounted for.
+    let mut required_consts = Vec::new();
+    let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts);
+    for (bb, bb_data) in traversal::reverse_postorder(&body) {
+        required_consts_visitor.visit_basic_block_data(bb, bb_data);
+    }
+    body.required_consts = required_consts;
+
     let promoted = promote_pass.promoted_fragments.into_inner();
     (tcx.alloc_steal_mir(body), tcx.alloc_steal_promoted(promoted))
 }

compiler/rustc_mir_transform/src/promote_consts.rs

@@ -13,6 +13,7 @@
 //! move analysis runs after promotion on broken MIR.
 
 use either::{Left, Right};
+use rustc_data_structures::fx::FxHashSet;
 use rustc_hir as hir;
 use rustc_middle::mir;
 use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor};
@@ -175,6 +176,11 @@ fn collect_temps_and_candidates<'tcx>(
 struct Validator<'a, 'tcx> {
     ccx: &'a ConstCx<'a, 'tcx>,
     temps: &'a mut IndexSlice<Local, TempState>,
+    /// For backwards compatibility, we are promoting function calls in `const`/`static`
+    /// initializers. But we want to avoid evaluating code that otherwise would not have been
+    /// evaluated, so we only do thos in basic blocks that are guaranteed to evaluate. Here we cache
+    /// the result of computing that set of basic blocks.
+    const_fn_safe_blocks: Option<FxHashSet<BasicBlock>>,
 }
 
 impl<'a, 'tcx> std::ops::Deref for Validator<'a, 'tcx> {
@@ -260,7 +266,9 @@ impl<'tcx> Validator<'_, 'tcx> {
                     self.validate_rvalue(rhs)
                 }
                 Right(terminator) => match &terminator.kind {
-                    TerminatorKind::Call { func, args, .. } => self.validate_call(func, args),
+                    TerminatorKind::Call { func, args, .. } => {
+                        self.validate_call(func, args, loc.block)
+                    }
                     TerminatorKind::Yield { .. } => Err(Unpromotable),
                     kind => {
                         span_bug!(terminator.source_info.span, "{:?} not promotable", kind);
@@ -564,20 +572,59 @@ impl<'tcx> Validator<'_, 'tcx> {
         Ok(())
     }
 
+    /// Computes the sets of blocks of this MIR that are definitely going to be executed
+    /// if the function returns successfully. That makes it safe to promote calls in them
+    /// that might fail.
+    fn const_fn_safe_blocks(body: &mir::Body<'tcx>) -> FxHashSet<BasicBlock> {
+        let mut safe_blocks = FxHashSet::default();
+        let mut safe_block = START_BLOCK;
+        loop {
+            safe_blocks.insert(safe_block);
+            // Let's see if we can find another safe block.
+            safe_block = match body.basic_blocks[safe_block].terminator().kind {
+                TerminatorKind::Goto { target } => target,
+                TerminatorKind::Call { target: Some(target), .. }
+                | TerminatorKind::Drop { target, .. } => {
+                    // This calls a function or the destructor. `target` does not get executed if
+                    // the callee loops or panics. But in both cases the const already fails to
+                    // evaluate, so we are fine considering `target` a safe block for promotion.
+                    target
+                }
+                TerminatorKind::Assert { target, .. } => {
+                    // Similar to above, we only consider successful execution.
+                    target
+                }
+                _ => {
+                    // No next safe block.
+                    break;
+                }
+            };
+        }
+        safe_blocks
+    }
+
+    fn is_const_fn_safe_block(&mut self, block: BasicBlock) -> bool {
+        let body = self.body;
+        let safe_blocks =
+            self.const_fn_safe_blocks.get_or_insert_with(|| Self::const_fn_safe_blocks(body));
+        safe_blocks.contains(&block)
+    }
+
     fn validate_call(
         &mut self,
         callee: &Operand<'tcx>,
         args: &[Spanned<Operand<'tcx>>],
+        block: BasicBlock,
     ) -> Result<(), Unpromotable> {
         let fn_ty = callee.ty(self.body, self.tcx);
 
-        // Inside const/static items, we promote all (eligible) function calls.
+        // Inside const/static items, we may promote (eligible) function calls.
         // Everywhere else, we require `#[rustc_promotable]` on the callee.
-        let promote_all_const_fn = matches!(
+        let promote_const_fn = matches!(
             self.const_kind,
             Some(hir::ConstContext::Static(_) | hir::ConstContext::Const { inline: false })
         );
-        if !promote_all_const_fn {
+        if !promote_const_fn {
             if let ty::FnDef(def_id, _) = *fn_ty.kind() {
                 // Never promote runtime `const fn` calls of
                 // functions without `#[rustc_promotable]`.
@@ -595,6 +642,11 @@ impl<'tcx> Validator<'_, 'tcx> {
             return Err(Unpromotable);
         }
 
+        if !self.is_const_fn_safe_block(block) {
+            // This function may error, and it may not even be executed as part of this `const`, so don't promote.
+            return Err(Unpromotable);
+        }
+
         self.validate_operand(callee)?;
         for arg in args {
             self.validate_operand(&arg.node)?;
@@ -610,7 +662,7 @@ fn validate_candidates(
     temps: &mut IndexSlice<Local, TempState>,
     candidates: &[Candidate],
 ) -> Vec<Candidate> {
-    let mut validator = Validator { ccx, temps };
+    let mut validator = Validator { ccx, temps, const_fn_safe_blocks: None };
 
     candidates
         .iter()

tests/ui/consts/const-eval/promoted_errors.noopt.stderr

@@ -41,6 +41,15 @@ const TEST_INTERIOR_MUT: () = {
 
 const TEST_DROP: String = String::new();
 
+// We do not promote function calls in `const` initializers in dead code.
+const fn mk_panic() -> u32 { panic!() }
+const fn mk_false() -> bool { false }
+const Y: () = {
+    if mk_false() {
+        let _x: &'static u32 = &mk_panic(); //~ ERROR temporary value dropped while borrowed
+    }
+};
+
 fn main() {
     // We must not promote things with interior mutability. Not even if we "project it away".
     let _val: &'static _ = &(Cell::new(1), 2).0; //~ ERROR temporary value dropped while borrowed