-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
GCI: During reachability analysis don't try to evaluate the initializer of overly generic free const items #153269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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{{.*}} |
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Going off on a tangent, the whole "don't eval the initializer for non-lifetime-parametric free const items" thing doesn't apply to mGCA's type-level consts which I'm pretty sure is very intentional (IINM) and thus fine, it's also not GCI-specific, e.g., I'm only concerned about const items with bodies anyway since the whole idea is to prevent const eval's "bespoke" handling of "too generic" consts "being user observable" / load-bearing for program correctness (the other motivation being consts should behave like fns in this specific scenario) or rephrased "type based" > "value based".
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah type const stuff is whatever here. those should be handled by whatever normal:tm: thing we do for wfchecks of type system stuff |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure if this is true 🤔 I would expect it to be possible to get
TooGenericback when there are trivial bounds on the const item which prevent normalization in the const's body.I guess that's fine for now anyhow... The whole setup here needs to be completely reworked to not evaluate stuff with false bounds or with generic parameters. And once that's done we should start evaluating with
TypingEnv::fully_monomorphizedwhich would fix that 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intend to get back to that work.... somewhat soon, I think mGCA stuff is finally calming down so I should be able to put more time towards pushing on that again