Rollup merge of rust-lang#153269 - fmease:gci-reach-no-eval, r=BoxyUwU

GCI: During reachability analysis don't try to evaluate the initializer of overly generic free const items

We generally don't want the initializer of free const items to get evaluated if they have any non-lifetime generic parameters. However, while I did account for that in HIR analysis & mono item collection (rust-lang#136168 & rust-lang#136429), I didn't account for reachability analysis so far which means that on main we still evaluate such items if they are *public* for example.

The closed PR rust-lang#142293 from a year ago did address that as a byproduct but of course it wasn't merged since its primary goal was misguided. This PR extracts & improves upon the relevant parts of that PR which are necessary to fix said issue.

Follow up to rust-lang#136168 & rust-lang#136429.
Partially supersedes rust-lang#142293.
Part of rust-lang#113521.

r? @BoxyUwU

Author: JonathanBrouwer

compiler/rustc_passes/src/reachable.rs

@@ -214,18 +214,26 @@ impl<'tcx> ReachableContext<'tcx> {
                         self.visit_const_item_rhs(init);
                     }
                     hir::ItemKind::Const(_, _, _, init) => {
-                        // Only things actually ending up in the final constant value are reachable
-                        // for codegen. Everything else is only needed during const-eval, so even if
-                        // const-eval happens in a downstream crate, all they need is
-                        // `mir_for_ctfe`.
+                        if self.tcx.generics_of(item.owner_id).own_requires_monomorphization() {
+                            // In this case, we don't want to evaluate the const initializer.
+                            // In lieu of that, we have to consider everything mentioned in it
+                            // as reachable, since it *may* end up in the final value.
+                            self.visit_const_item_rhs(init);
+                            return;
+                        }
+
                         match self.tcx.const_eval_poly_to_alloc(item.owner_id.def_id.into()) {
                             Ok(alloc) => {
+                                // Only things actually ending up in the final constant value are
+                                // reachable for codegen. Everything else is only needed during
+                                // const-eval, so even if const-eval happens in a downstream crate,
+                                // all they need is `mir_for_ctfe`.
                                 let alloc = self.tcx.global_alloc(alloc.alloc_id).unwrap_memory();
                                 self.propagate_from_alloc(alloc);
                             }
-                            // We can't figure out which value the constant will evaluate to. In
-                            // lieu of that, we have to consider everything mentioned in the const
-                            // initializer reachable, since it *may* end up in the final value.
+                            // Trivially unsatisfiable bounds on the item prevented us from
+                            // normalizing the initializer. Similar to the other case, we have to
+                            // everything mentioned in it as reachable.
                             Err(ErrorHandled::TooGeneric(_)) => self.visit_const_item_rhs(init),
                             // If there was an error evaluating the const, nothing can be reachable
                             // via it, and anyway compilation will fail.

tests/codegen-llvm/dont_codegen_private_const_fn_only_used_in_const_eval.rs

@@ -0,0 +1,38 @@
+// Check that we — where possible — don't codegen functions that are only used to evaluate
+// static / const items, but never used in runtime code.
+
+//@ compile-flags: --crate-type=lib -Copt-level=0
+
+#![feature(generic_const_items)] // only used in the last few test cases
+
+pub static STATIC: () = func0();
+const fn func0() {}
+// CHECK-NOT: define{{.*}}func0{{.*}}
+
+pub const CONSTANT: () = func1();
+const fn func1() {}
+// CHECK-NOT: define{{.*}}func1{{.*}}
+
+// We generally don't want to evaluate the initializer of free const items if they have
+// non-region params (and even if we did, const eval would fail anyway with "too polymorphic"
+// if the initializer actually referenced such a param).
+//
+// As a result of not being able to look at the final value, during reachability analysis we
+// can't tell for sure if for example certain functions end up in the final value or if they're
+// only used during const eval. We fall back to a conservative HIR-based approach.
+
+// `func2` isn't needed at runtime but the compiler can't tell for the reason mentioned above.
+pub const POLY_CONST_0<const C: bool>: () = func2();
+const fn func2() {}
+// CHECK: define{{.*}}func2{{.*}}
+
+// `func3` isn't needed at runtime but the compiler can't tell for the reason mentioned above.
+pub const POLY_CONST_1<const C: bool>: () = if C { func3() };
+const fn func3() {}
+// CHECK: define{{.*}}func3{{.*}}
+
+// `func4` *is* needed at runtime (here, the HIR-based approach gets it right).
+pub const POLY_CONST_2<const C: bool>: Option<fn() /* or a TAIT */> =
+    if C { Some(func4) } else { None };
+const fn func4() {}
+// CHECK: define{{.*}}func4{{.*}}

tests/codegen-llvm/private-const-fn-only-used-in-const-eval.rs

@@ -1,5 +1,5 @@
 error[E0080]: evaluation panicked: explicit panic
-  --> $DIR/def-site-eval.rs:13:20
+  --> $DIR/def-site-eval.rs:32:20
    |
 LL | const _<'_a>: () = panic!();
    |                    ^^^^^^^^ evaluation of `_` failed here

tests/ui/generic-const-items/def-site-eval.fail.stderr

@@ -1,15 +1,32 @@
-//! Test that we only evaluate free const items (their def site to be clear)
-//! whose generics don't require monomorphization.
+// Test that we don't evaluate the initializer of free const items if they have
+// non-region generic parameters (i.e., ones that "require monomorphization").
+//
+// To peek behind the curtains for a bit, at the time of writing there are three places where we
+// usually evaluate the initializer: "analysis", mono item collection & reachability analysis.
+// We must ensure that all of them take the generics into account.
+//
+//@ revisions: fail pass
+//@[pass] check-pass
+
 #![feature(generic_const_items)]
 #![expect(incomplete_features)]
+#![crate_type = "lib"] // (*)
 
-//@ revisions: fail pass
-//@[pass] check-pass
+// All of these constants are intentionally unused since we want to test the
+// behavior at the def site, not at use sites.
 
 const _<_T>: () = panic!();
 const _<const _N: usize>: () = panic!();
 
+// Check *public* const items specifically to exercise reachability analysis which normally
+// evaluates const initializers to look for function pointers in the final const value.
+//
+// (*): While reachability analysis also runs for purely binary crates (to find e.g., extern items)
+//      setting the crate type to library (1) makes the case below 'more realistic' since
+//      hypothetical downstream crates that require runtime MIR could actually exist.
+//      (2) It ensures that we exercise the relevant part of the compiler under test.
+pub const K<_T>: () = panic!();
+pub const Q<const _N: usize>: () = loop {};
+
 #[cfg(fail)]
 const _<'_a>: () = panic!(); //[fail]~ ERROR evaluation panicked: explicit panic
-
-fn main() {}

tests/ui/generic-const-items/def-site-eval.rs

@@ -1,25 +1,25 @@
 error[E0080]: entering unreachable code
-  --> $DIR/trivially-unsatisfied-bounds-0.rs:6:1
+  --> $DIR/trivially-unsatisfied-bounds.rs:17:1
    |
 LL | / const UNUSABLE: () = ()
 LL | | where
-LL | |     String: Copy;
-   | |_________________^ evaluation of `UNUSABLE` failed here
+LL | |     for<'_delay> String: Copy;
+   | |______________________________^ evaluation of `UNUSABLE` failed here
 
 error[E0277]: the trait bound `String: Copy` is not satisfied
-  --> $DIR/trivially-unsatisfied-bounds-0.rs:11:13
+  --> $DIR/trivially-unsatisfied-bounds.rs:24:13
    |
 LL |     let _ = UNUSABLE;
    |             ^^^^^^^^ the trait `Copy` is not implemented for `String`
    |
 note: required by a bound in `UNUSABLE`
-  --> $DIR/trivially-unsatisfied-bounds-0.rs:8:13
+  --> $DIR/trivially-unsatisfied-bounds.rs:19:26
    |
 LL | const UNUSABLE: () = ()
    |       -------- required by a bound in this constant
 LL | where
-LL |     String: Copy;
-   |             ^^^^ required by this bound in `UNUSABLE`
+LL |     for<'_delay> String: Copy;
+   |                          ^^^^ required by this bound in `UNUSABLE`
 
 error: aborting due to 2 previous errors
 

tests/ui/generic-const-items/trivially-unsatisfied-bounds-0.rs

@@ -0,0 +1,33 @@
+// Exercise trivially unsatisfied bounds on free const items.
+// Their interaction with the evaluation of the initializer is interesting.
+//
+//@ revisions: mentioned unmentioned
+
+#![feature(generic_const_items)]
+
+// FIXME(generic_const_items): Try to get rid of error "entering unreachable error", it's
+// unnecessary and actually caused by MIR pass `ImpossiblePredicates` replacing the body with the
+// terminator `Unreachable` due to the unsatisfied bound which is subsequently reached.
+//
+// NOTE(#142293): However, don't think about suppressing the evaluation of the initializer if the
+// bounds are "impossible". That'd be a SemVer hazard since it could cause downstream to fail to
+// compile if upstream added a new trait impl which is undesirable[^1].
+// [^1]: Strictly speaking that's already possible due to the one-impl rule.
+
+const UNUSABLE: () = () //~ ERROR entering unreachable code
+where
+    for<'_delay> String: Copy;
+
+fn scope() {
+    // Ensure that we successfully reject references of consts with trivially unsatisfied bounds.
+    #[cfg(mentioned)]
+    let _ = UNUSABLE; //[mentioned]~ ERROR the trait bound `String: Copy` is not satisfied
+}
+
+const _BAD: () = <() as Unimplemented>::CT
+where
+    for<'_delay> (): Unimplemented;
+
+trait Unimplemented { const CT: (); }
+
+fn main() {}