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,12 @@ 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 might panic and that otherwise would
+    /// not have been evaluated, so we only promote such calls in basic blocks that are guaranteed
+    /// to execute. In other words, we only promote such calls in basic blocks that are definitely
+    /// not dead code. Here we cache the result of computing that set of basic blocks.
+    promotion_safe_blocks: Option<FxHashSet<BasicBlock>>,
 }
 
 impl<'a, 'tcx> std::ops::Deref for Validator<'a, 'tcx> {
@@ -260,7 +267,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);
@@ -587,42 +596,93 @@ 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 promotion_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
+    }
+
+    /// Returns whether the block is "safe" for promotion, which means it cannot be dead code.
+    /// We use this to avoid promoting operations that can fail in dead code.
+    fn is_promotion_safe_block(&mut self, block: BasicBlock) -> bool {
+        let body = self.body;
+        let safe_blocks =
+            self.promotion_safe_blocks.get_or_insert_with(|| Self::promotion_safe_blocks(body));
+        safe_blocks.contains(&block)
+    }
+
     fn validate_call(
         &mut self,
         callee: &Operand<'tcx>,
         args: &[Spanned<Operand<'tcx>>],
+        block: BasicBlock,
     ) -> Result<(), Unpromotable> {
+        // Validate the operands. If they fail, there's no question -- we cannot promote.
+        self.validate_operand(callee)?;
+        for arg in args {
+            self.validate_operand(&arg.node)?;
+        }
+
+        // Functions marked `#[rustc_promotable]` are explicitly allowed to be promoted, so we can
+        // accept them at this point.
         let fn_ty = callee.ty(self.body, self.tcx);
+        if let ty::FnDef(def_id, _) = *fn_ty.kind() {
+            if self.tcx.is_promotable_const_fn(def_id) {
+                return Ok(());
+            }
+        }
 
-        // Inside const/static items, we promote all (eligible) function calls.
-        // Everywhere else, we require `#[rustc_promotable]` on the callee.
-        let promote_all_const_fn = matches!(
+        // Ideally, we'd stop here and reject the rest.
+        // But for backward compatibility, we have to accept some promotion in const/static
+        // initializers. Inline consts are explicitly excluded, they are more recent so we have no
+        // backwards compatibility reason to allow more promotion inside of them.
+        let promote_all_fn = matches!(
             self.const_kind,
             Some(hir::ConstContext::Static(_) | hir::ConstContext::Const { inline: false })
         );
-        if !promote_all_const_fn {
-            if let ty::FnDef(def_id, _) = *fn_ty.kind() {
-                // Never promote runtime `const fn` calls of
-                // functions without `#[rustc_promotable]`.
-                if !self.tcx.is_promotable_const_fn(def_id) {
-                    return Err(Unpromotable);
-                }
-            }
+        if !promote_all_fn {
+            return Err(Unpromotable);
         }
-
+        // Make sure the callee is a `const fn`.
         let is_const_fn = match *fn_ty.kind() {
             ty::FnDef(def_id, _) => self.tcx.is_const_fn_raw(def_id),
             _ => false,
         };
         if !is_const_fn {
             return Err(Unpromotable);
         }
-
-        self.validate_operand(callee)?;
-        for arg in args {
-            self.validate_operand(&arg.node)?;
+        // The problem is, this may promote calls to functions that panic.
+        // We don't want to introduce compilation errors if there's a panic in a call in dead code.
+        // So we ensure that this is not dead code.
+        if !self.is_promotion_safe_block(block) {
+            return Err(Unpromotable);
         }
-
+        // This passed all checks, so let's accept.
         Ok(())
     }
 }
@@ -633,7 +693,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, promotion_safe_blocks: None };
 
     candidates
         .iter()

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

@@ -51,6 +51,15 @@ const TEST_DROP_NOT_PROMOTE: &String = {
 };
 
 
+// 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

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

@@ -47,6 +47,16 @@ LL |     let x = &String::new();
 LL | };
    | - value is dropped here
 
+error[E0716]: temporary value dropped while borrowed
+  --> $DIR/promote-not.rs:59:33
+   |
+LL |         let _x: &'static u32 = &mk_panic();
+   |                 ------------    ^^^^^^^^^^ creates a temporary value which is freed while still in use
+   |                 |
+   |                 type annotation requires that borrow lasts for `'static`
+LL |     }
+   |     - temporary value is freed at the end of this statement
+
 error[E0716]: temporary value dropped while borrowed
   --> $DIR/promote-not.rs:21:32
    |
@@ -68,7 +78,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:56:29
+  --> $DIR/promote-not.rs:65:29
    |
 LL |     let _val: &'static _ = &(Cell::new(1), 2).0;
    |               ----------    ^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -79,7 +89,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:57:29
+  --> $DIR/promote-not.rs:66:29
    |
 LL |     let _val: &'static _ = &(Cell::new(1), 2).1;
    |               ----------    ^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -90,7 +100,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:60:29
+  --> $DIR/promote-not.rs:69:29
    |
 LL |     let _val: &'static _ = &(1/0);
    |               ----------    ^^^^^ creates a temporary value which is freed while still in use
@@ -101,7 +111,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:61:29
+  --> $DIR/promote-not.rs:70:29
    |
 LL |     let _val: &'static _ = &(1/(1-1));
    |               ----------    ^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -112,7 +122,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:62:29
+  --> $DIR/promote-not.rs:71:29
    |
 LL |     let _val: &'static _ = &((1+1)/(1-1));
    |               ----------    ^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -123,7 +133,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:63:29
+  --> $DIR/promote-not.rs:72:29
    |
 LL |     let _val: &'static _ = &(i32::MIN/-1);
    |               ----------    ^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -134,7 +144,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:64:29
+  --> $DIR/promote-not.rs:73:29
    |
 LL |     let _val: &'static _ = &(i32::MIN/(0-1));
    |               ----------    ^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -145,7 +155,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:65:29
+  --> $DIR/promote-not.rs:74:29
    |
 LL |     let _val: &'static _ = &(-128i8/-1);
    |               ----------    ^^^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -156,7 +166,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:66:29
+  --> $DIR/promote-not.rs:75:29
    |
 LL |     let _val: &'static _ = &(1%0);
    |               ----------    ^^^^^ creates a temporary value which is freed while still in use
@@ -167,7 +177,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:67:29
+  --> $DIR/promote-not.rs:76:29
    |
 LL |     let _val: &'static _ = &(1%(1-1));
    |               ----------    ^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -178,7 +188,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:68:29
+  --> $DIR/promote-not.rs:77:29
    |
 LL |     let _val: &'static _ = &([1,2,3][4]+1);
    |               ----------    ^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -189,7 +199,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:72:29
+  --> $DIR/promote-not.rs:81:29
    |
 LL |     let _val: &'static _ = &TEST_DROP;
    |               ----------    ^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -200,7 +210,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:74:29
+  --> $DIR/promote-not.rs:83:29
    |
 LL |     let _val: &'static _ = &&TEST_DROP;
    |               ----------    ^^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -211,7 +221,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:74:30
+  --> $DIR/promote-not.rs:83:30
    |
 LL |     let _val: &'static _ = &&TEST_DROP;
    |               ----------     ^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -222,7 +232,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:77:29
+  --> $DIR/promote-not.rs:86:29
    |
 LL |     let _val: &'static _ = &(&TEST_DROP,);
    |               ----------    ^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -233,7 +243,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:77:31
+  --> $DIR/promote-not.rs:86:31
    |
 LL |     let _val: &'static _ = &(&TEST_DROP,);
    |               ----------      ^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -244,7 +254,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:80:29
+  --> $DIR/promote-not.rs:89:29
    |
 LL |     let _val: &'static _ = &[&TEST_DROP; 1];
    |               ----------    ^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -255,7 +265,7 @@ LL | }
    | - temporary value is freed at the end of this statement
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:80:31
+  --> $DIR/promote-not.rs:89:31
    |
 LL |     let _val: &'static _ = &[&TEST_DROP; 1];
    |               ----------      ^^^^^^^^^    - temporary value is freed at the end of this statement
@@ -264,7 +274,7 @@ LL |     let _val: &'static _ = &[&TEST_DROP; 1];
    |               type annotation requires that borrow lasts for `'static`
 
 error[E0716]: temporary value dropped while borrowed
-  --> $DIR/promote-not.rs:89:26
+  --> $DIR/promote-not.rs:98:26
    |
 LL |     let x: &'static _ = &UnionWithCell { f1: 0 };
    |            ----------    ^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
@@ -274,7 +284,7 @@ LL |
 LL | }
    | - temporary value is freed at the end of this statement
 
-error: aborting due to 26 previous errors
+error: aborting due to 27 previous errors
 
 Some errors have detailed explanations: E0493, E0716.
 For more information about an error, try `rustc --explain E0493`.

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

@@ -5,28 +5,30 @@
 
 //@ build-pass
 
+#![allow(arithmetic_overflow)]
+
+use std::mem;
+
 const fn assert_static<T>(_: &'static T) {}
 
-#[allow(unconditional_panic)]
-const fn fail() -> i32 {
-    1/0
-}
-const C: i32 = {
-    // Promoted that fails to evaluate in dead code -- this must work
-    // (for backwards compatibility reasons).
-    if false {
-        assert_static(&fail());
-    }
+// Function calls in const on the "main path" (not inside conditionals)
+// do get promoted.
+const fn make_thing() -> i32 {
     42
+}
+const C: () = {
+    assert_static(&make_thing());
+    // Make sure this works even when there's other stuff (like function calls) above the relevant
+    // call in the const initializer.
+    assert_static(&make_thing());
 };
 
 fn main() {
     assert_static(&["a", "b", "c"]);
     assert_static(&["d", "e", "f"]);
-    assert_eq!(C, 42);
 
     // make sure that this does not cause trouble despite overflowing
-    assert_static(&(0-1));
+    assert_static(&(0u32 - 1));
 
     // div-by-non-0 (and also not MIN/-1) is okay
     assert_static(&(1/1));
@@ -36,12 +38,16 @@ fn main() {
     assert_static(&(1%1));
 
     // in-bounds array access is okay
-    assert_static(&([1,2,3][0] + 1));
-    assert_static(&[[1,2][1]]);
+    assert_static(&([1, 2, 3][0] + 1));
+    assert_static(&[[1, 2][1]]);
 
     // Top-level projections are not part of the promoted, so no error here.
     if false {
         #[allow(unconditional_panic)]
-        assert_static(&[1,2,3][4]);
+        assert_static(&[1, 2, 3][4]);
     }
+
+    // More complicated case involving control flow and a `#[rustc_promotable]` function
+    let decision = std::hint::black_box(true);
+    let x: &'static usize = if decision { &mem::size_of::<usize>() } else { &0 };
 }

tests/ui/consts/const-eval/promoted_errors.rs

@@ -4,7 +4,7 @@ fn non_elidable<'a, 'b>(a: &'a u8, b: &'b u8) -> &'a u8 {
     a
 }
 
-// The incorrect case without `for<'a>` is tested for in `rfc1623-2.rs`
+// The incorrect case without `for<'a>` is tested for in `rfc1623-3.rs`
 static NON_ELIDABLE_FN: &for<'a> fn(&'a u8, &'a u8) -> &'a u8 =
     &(non_elidable as for<'a> fn(&'a u8, &'a u8) -> &'a u8);
 
@@ -28,6 +28,10 @@ static SOME_STRUCT: &SomeStruct = &SomeStruct {
     f: &id,
     //~^ ERROR mismatched types
     //~| ERROR mismatched types
+    //~| ERROR mismatched types
+    //~| ERROR mismatched types
+    //~| ERROR implementation of `FnOnce` is not general enough
+    //~| ERROR implementation of `FnOnce` is not general enough
     //~| ERROR implementation of `FnOnce` is not general enough
     //~| ERROR implementation of `FnOnce` is not general enough
 };

tests/ui/consts/promote-not.rs

@@ -35,6 +35,46 @@ LL |     f: &id,
    = note: `fn(&Foo<'2>) -> &Foo<'2> {id::<&Foo<'2>>}` must implement `FnOnce<(&'a Foo<'1>,)>`, for any lifetime `'1`...
    = note: ...but it actually implements `FnOnce<(&Foo<'2>,)>`, for some specific lifetime `'2`
 
-error: aborting due to 4 previous errors
+error[E0308]: mismatched types
+  --> $DIR/rfc1623-2.rs:28:8
+   |
+LL |     f: &id,
+   |        ^^^ one type is more general than the other
+   |
+   = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)`
+              found trait `Fn(&Foo<'_>)`
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
+error[E0308]: mismatched types
+  --> $DIR/rfc1623-2.rs:28:8
+   |
+LL |     f: &id,
+   |        ^^^ one type is more general than the other
+   |
+   = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)`
+              found trait `Fn(&Foo<'_>)`
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
+error: implementation of `FnOnce` is not general enough
+  --> $DIR/rfc1623-2.rs:28:8
+   |
+LL |     f: &id,
+   |        ^^^ implementation of `FnOnce` is not general enough
+   |
+   = note: `fn(&'2 Foo<'_>) -> &'2 Foo<'_> {id::<&'2 Foo<'_>>}` must implement `FnOnce<(&'1 Foo<'b>,)>`, for any lifetime `'1`...
+   = note: ...but it actually implements `FnOnce<(&'2 Foo<'_>,)>`, for some specific lifetime `'2`
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
+error: implementation of `FnOnce` is not general enough
+  --> $DIR/rfc1623-2.rs:28:8
+   |
+LL |     f: &id,
+   |        ^^^ implementation of `FnOnce` is not general enough
+   |
+   = note: `fn(&Foo<'2>) -> &Foo<'2> {id::<&Foo<'2>>}` must implement `FnOnce<(&'a Foo<'1>,)>`, for any lifetime `'1`...
+   = note: ...but it actually implements `FnOnce<(&Foo<'2>,)>`, for some specific lifetime `'2`
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
+error: aborting due to 8 previous errors
 
 For more information about this error, try `rustc --explain E0308`.