-
Notifications
You must be signed in to change notification settings - Fork 17
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
Never allow const fn
calls in promoteds
#19
Comments
Full agreement! This means we'll need to split const qualification into "allowed in const context" and "allowed in a promoted", where the latter will be a strict subset of the former. Semantically, heap allocations and panics and ptr comparison will be allowed in the former but not in the latter. Promotion-safe is more strict than const-safe. Is that correct? One more question: The user could write all of these things in a |
When you use a constant, you are explicitly opting into this behavior. Promotion on the other hand happens silently. So if we expand on what promotion can do, we'd turn existing code into promoted code, which might change behavior.
Yes, the set of promotable code is a strict subset of const code. There's nothing that gets promoted that is not also allowed inside constants. |
Constants follow the principle that they should be equivalent to copy-pasting their definition at every use site. I agree with your reasoning for panics, but not for heap allocations: Following the aforementioned principle,
Okay, that's great. :) For heap allocations and pointer comparisons, that's easy: A promotion-safe execution never dos either. For panics, though... we do promote arithmetic, which can also panic. What's the criterion here? |
we promote arithmetics, but not their assertions. While we could do the same for panics, that requires quite some compiler complexity that I'd rather avoid having to support for forever.
Copy-pasting their value at every use site, not their code. This is not a preprocessor. Constants are evaluated once and the result is copied at every use site. We already do this e.g. for
We are not copy pasting the |
Oh right! The arithmetic doesn't panic, it just returns a bool that later triggers a panic. (And once upon a time I made arithmetic panic itself and that broke promotion.^^) Okay, this makes sense. So we can just add "does not panic" as a third criterion for code being promotion-safe. All
Hm... I always saw this as promotion happening after copy-pasting. But I guess this makes sense. Can we have value-based checks for Drop, Sync and UnsafeCell then, please? :D |
With your value visitor that is entirely reasonable now. Last time I tried to implement it I was hooking closures into the validation code and it was horrible |
For future searchers, it's spelled |
So... apparently there is desire for implicit promotion of certain const fns. What we could do is stabilize a 🕯️ summons @Centril 🕯️ Maybe we call it |
@eddyb mentions that
This would help with const fns that were const fn to start out with, but not e.g. when changing fn foo() {
panic!()
}
fn main() {
let x = &foo(); // ERROR if `foo` is `const`, `panic` if not
} |
@oli-obk Do we consider panics promotable? If not, calls to |
Also, specifically for cases like that, you can/should leave behind a MIR terminator shaped like the call (with its success and unwind edges), that would be replaced with an unconditional panic invoke (basically an always-failing EDIT: It's how we promote things like indexing or checked divisions: we promote the computation, while the checking code, including the |
Well, there's desire for many things. ;) What's the argument against explicit promotion through e.g. |
I guess the argument is that we already stabilized a few of these XD |
That sounds like "we made a mistake, so now let's go full steam ahead to get more of it". I do not see how that's an argument for introducing another attribute with its own analysis etc. |
😂
Eh... twas a joke?
Depends on how complicated the analysis is... We already do have it sort of so there's no new fundamental complexity? |
This would be yet another mode in const qualification. That file is already a mess. I'm opposed to anything that further complicates this, in particular if we have a more foundational approach to solve the same problem: explicit user annotation. |
I think too many explicit user annotations can sometimes add more complexity than they remove elsewhere. Especially when you start getting into context-sensitive situations (the annotation depending on some properties of parameters, or, worse, of associated items of parameters). At least for def-side annotations, anyway. OTOH, (being able to do Also, @RalfJung, have you seen const qualification lately ;)? |
Similar things can be said about too much magic, though -- it can hurt more than it helps.
No, did the long-standing refactoring finally happen? :D |
The dataflow integration is not there yet, but the conversion from pervasive mutation to a functional-style computation (from a MIR |
So I just had an idea, if we want no future Specifically, all stably-promotable If surjective² too (i.e. bijective), then the pattern alias is irrefutable (but this is optional). According to @oli-obk, all Also, looking at those 3, they all are like this, which seems injective: #[stable(feature = "duration", since = "1.3.0")]
#[inline]
#[rustc_promotable]
pub const fn from_x(x: u64) -> Duration {
Duration {
secs: x / X_PER_SEC,
nanos: ((x % X_PER_SEC) as u32) * NANOS_PER_X,
}
} Which makes sense, you don't want to throw away information without returning ¹injective: all input information is preserved in the output somehow, so |
Awesome, I left a bunch of drive-by comments. I was excited by the first part, but the second part still seems very much like the old mess -- in fact it seems partially redundant now (like, there's two places that check things about
That's a very interesting observation! Not sure what the plan is here, will we have Is this a good enough argument to try and phase out the three promoteable functions that are not trivially injective? |
Maybe - a crater run won't hurt. But also maybe we should make a list with all the As for the syntax, I believe @Centril prefers something like this: pattern from_secs(secs: u64): Duration = Duration {
secs,
nanos: 0,
}; Specifically, the RHS is a pattern, not an expression, so we are not dealing with just another |
@pnkfelix has worked a lot on const patterns recently, AFAIK. Given that we had some rough plans to use "const pattern analysis" for implicit promotion, it would be nice to sync @eddyb's plan with the current reality of using consts in patterns. Is there a write-up of that anywhere? |
WIP: no longer promote non-pattern const functions This is trying to pack-pedal a bit on promotion feature creep, as proposed by @eddyb [here](rust-lang/const-eval#19 (comment)): possibly, a sane subset of `const fn` that we could promote are those that are just constructors -- the same subset that we might want to allow in pattern position at some point. So, this removes the `rustc_promotable` attribute from the three functions they identified that do not fit this pattern. The first step is to run crater to see if there is code in the wild that relies on this being promotable. r? @oli-obk
Also Cc @ecstatic-morse, who seems to also be involved in the const-pattern effort. |
…akis no longer promote non-pattern const functions This is trying to pack-pedal a bit on promotion feature creep, as proposed by @eddyb [here](rust-lang/const-eval#19 (comment)): possibly, a sane subset of `const fn` that we could promote are those that are just constructors -- the same subset that we might want to allow in pattern position at some point. So, this removes the `rustc_promotable` attribute from the three functions they identified that do not fit this pattern. The first step is to run crater to see if there is code in the wild that relies on this being promotable. r? @oli-obk
…akis no longer promote non-pattern const functions This is trying to pack-pedal a bit on promotion feature creep, as proposed by @eddyb [here](rust-lang/const-eval#19 (comment)): possibly, a sane subset of `const fn` that we could promote are those that are just constructors -- the same subset that we might want to allow in pattern position at some point. So, this removes the `rustc_promotable` attribute from the three functions they identified that do not fit this pattern. The first step is to run crater to see if there is code in the wild that relies on this being promotable. r? @oli-obk
de-promote Duration::from_secs In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse): * `INT::min_value`, `INT::max_value` * `std::mem::size_of`, `std::mem::align_of` * `RangeInclusive::new` (backing `x..=y`) * `std::ptr::null`, `std::ptr::null_mut` * `RawWaker::new`, `RawWakerVTable::new` ??? * `Duration::from_secs` I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does. rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run. See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
de-promote Duration::from_secs In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse): * `INT::min_value`, `INT::max_value` * `std::mem::size_of`, `std::mem::align_of` * `RangeInclusive::new` (backing `x..=y`) * `std::ptr::null`, `std::ptr::null_mut` * `RawWaker::new`, `RawWakerVTable::new` ??? * `Duration::from_secs` I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does. rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run. See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
de-promote Duration::from_secs In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse): * `INT::min_value`, `INT::max_value` * `std::mem::size_of`, `std::mem::align_of` * `RangeInclusive::new` (backing `x..=y`) * `std::ptr::null`, `std::ptr::null_mut` * `RawWaker::new`, `RawWakerVTable::new` ??? * `Duration::from_secs` I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does. rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run. See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
de-promote Duration::from_secs In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse): * `INT::min_value`, `INT::max_value` * `std::mem::size_of`, `std::mem::align_of` * `RangeInclusive::new` (backing `x..=y`) * `std::ptr::null`, `std::ptr::null_mut` * `RawWaker::new`, `RawWakerVTable::new` ??? * `Duration::from_secs` I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does. rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run. See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
de-promote Duration::from_secs In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse): * `INT::min_value`, `INT::max_value` * `std::mem::size_of`, `std::mem::align_of` * `RangeInclusive::new` (backing `x..=y`) * `std::ptr::null`, `std::ptr::null_mut` * `RawWaker::new`, `RawWakerVTable::new` ??? * `Duration::from_secs` I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does. rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run. See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
A lot has happened in the last 2 years. :) See the updated promotion docs in #67 for details. @oli-obk the only remaining promotion of So I think this issue can be closed, finally. |
I want to reiterate on the discussion about
const fn
calls to promoteds. During the stabilization ofmin_const_fn
, we recently punted on the topic by not promoting anyconst fn
calls, except a specific list of "known to be safe" functions likestd::mem::size_of
.I want to argue for forbidding the promotion of
const fn
calls forever (leaving in the existing escape hatch for the libstd is not very expensive, and we may just want to start linting about it for a few years and remove it at some point).I show how three future extensions to the const evaluator are either incompatible with promoting
const fn
calls or increase the compiler complexity, suprises for users and general lanugage complexity.panicky code in
const fn
The function
foo
, defined aswill panic when executed. If used in a promoted, e.g.
this would create a compile-time error about a panic reached during const eval. Unfortunately that means that
would also cause this compile-time error, even though the user has no reason to suspect this to not compile.
While we can demote such compile-time errors to panics by compiling them to a call to the panic machinery with the original message, this would break the backtrace and would generally require some rather complex code to support this pretty useless case.
heap allocations in
const fn
The function
foo
, defined aswill create a heap allocation if executed. When evaluated during const eval, it will not create a heap allocation, but sort of create a new unnamed static item for every call.
This means that
would be perfectly legal code. When used in a promoted,
we'd also not create a heap allocation, but rather another unnamed static.
This also means that
would do that, even though the user expects that to produce a new heap allocation, place it in a temporary and return a reference to it. In reality there would be only one single allocation. The user might even be tempted to mutate memory via that
*mut u32
, which would definitely be UB. While I don't expect the above code to be written, I imagine that there are many ways for accidentally sneaking a&
operator into some expression that will end up causing a promotion to happen.Defined runtime code can be promoted and suddenly stop compiling
Will work just fine at compile-time, but at runtime, comparing pointers to different allocations can be problematic. While this case is deterministic between compile-time and runtime due to the "check every element" fallback,
depends on its arguments. E.g.
foo(&42, &42)
may returnfalse
at compile-time andtrue
at runtime due to deduplication in llvm.If we extend this to promotion,
will get evaluated at compile-time, but have a result different from runtime, even though the user did not request that.
Conclusion
Since I presume that we'll want all of the above const eval features (we even support the first one on stable rustc) at some point or another, I suggest that we never allow promoting const fn calls without an explicit request for constness, e.g. by inserting a constant and promoting a use of that constant, or by a future extension to the language for unnamed constant expressions.
The text was updated successfully, but these errors were encountered: