Skip to content
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

Closed
oli-obk opened this issue Dec 10, 2018 · 25 comments
Closed

Never allow const fn calls in promoteds #19

oli-obk opened this issue Dec 10, 2018 · 25 comments

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Dec 10, 2018

I want to reiterate on the discussion about const fn calls to promoteds. During the stabilization of min_const_fn, we recently punted on the topic by not promoting any const fn calls, except a specific list of "known to be safe" functions like std::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 as

const fn foo() { [()][42] }

will panic when executed. If used in a promoted, e.g.

let x: &'static () = &foo();

this would create a compile-time error about a panic reached during const eval. Unfortunately that means that

let x = &foo();

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 as

const fn foo() -> *mut u32 { Box::leak(Box::new(42)) }

will 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

const FOO: *mut u32 = foo();

would be perfectly legal code. When used in a promoted,

fn bar() {
    let x: &'static *mut u32 = &foo()
}

we'd also not create a heap allocation, but rather another unnamed static.

This also means that

fn bar() {
    let x: *mut u32 = *&foo();
}

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

const fn eq(x: &[u32], y: &[u32]) -> bool {
    if x.len() != y.len() {
        return false;
    }
    if x.as_ptr() == y.as_ptr() {
        return true;
    }
    x.iter().eq(y.iter())
}

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,

const fn foo(x: &u32, y: &u32) -> bool {
    x.as_ptr() == y.as_ptr()
}

depends on its arguments. E.g. foo(&42, &42) may return false at compile-time and true at runtime due to deduplication in llvm.

If we extend this to promotion,

let x = &foo(&42, &42);

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.

@RalfJung
Copy link
Member

RalfJung commented Dec 13, 2018

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.

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 const as well, not just in a const fn. Why are things any different (better, presumably) then?

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 13, 2018

The user could write all of these things in a const as well, not just in a const fn. Why are things any different (better, presumably) then?

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.

Promotion-safe is more strict than const-safe. Is that correct?

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.

@RalfJung
Copy link
Member

When you use a constant, you are explicitly opting into this behavior.

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, const FOO: *mut i32 = Box::leak(Box::new(0)) should not be accepted.

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.

Okay, that's great. :)
As usual, I propose we aim for getting a precise semantic understanding of what makes code const-(un)safe and promotion-(un)safe -- meaning a criterion on program executions, not source code. Something that you could even code into the miri engine (also see #17).

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?

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 13, 2018

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.

Constants follow the principle that they should be equivalent to copy-pasting their definition at every use site.

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 const FOO: &&&&u32 = &&&&1;. We only copy the value, so the outer most reference, everything else stays the same. We do reserve the right to arbitrarily duplicate any reference in that constant though.

I agree with your reasoning for panics, but not for heap allocations: Following the aforementioned principle, const FOO: *mut i32 = Box::leak(Box::new(0)) should not be accepted.

We are not copy pasting the Box::leak(Box::New(0)), but the memory of the evaluated version of FOO (so an Allocation with size_of<usize>() bytes whose relocation points to another allocation of 4 bytes which contains a 0. There's nothing relating to the heap in this value. Values do not care about heap, stack, static, ... The only thing we do care about is the identity of the memory of static items.

@RalfJung
Copy link
Member

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.

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 #[rustc_promoteable] functions must satisfy this.

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 const FOO: &&&&u32 = &&&&1;. We only copy the value, so the outer most reference, everything else stays the same. We do reserve the right to arbitrarily duplicate any reference in that constant though.

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

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 13, 2018

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

cc rust-lang/rust#53819

@shepmaster
Copy link
Member

For future searchers, it's spelled rustc_promotable.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 13, 2019

So... apparently there is desire for implicit promotion of certain const fns.

What we could do is stabilize a #[promotable] attribute for const fns which is much stricter about the things that you may do. We'd basically restrict it to integer math, trivial casts, aggregate construction, let bindings and calls to other #[promotable] functions or intrinsics. (this covers all the current use cases)

🕯️ summons @Centril 🕯️

Maybe we call it #[pure] 😆

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 13, 2019

@eddyb mentions that

This is another reason for my position that "cheating" with "exposed bodies" is actually good overall, as it significantly reduces complexity while allowing maximal features but still keeping "significance of fn bodies to stability" opt-in.

This would help with const fns that were const fn to start out with, but not e.g. when changing foo from not const to const in the following code:

fn foo() {
    panic!()
}
fn main() {
    let x = &foo(); // ERROR if `foo` is `const`, `panic` if not
}

@eddyb
Copy link
Member

eddyb commented Mar 13, 2019

@oli-obk Do we consider panics promotable? If not, calls to const fns that could panic wouldn't get promoted under the "exposed bodies" system, preserving runtime behavior.

@eddyb
Copy link
Member

eddyb commented Mar 13, 2019

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 Assert terminator) at monomorphization time.

EDIT: It's how we promote things like indexing or checked divisions: we promote the computation, while the checking code, including the Assert, stays where it was.

@RalfJung
Copy link
Member

So... apparently there is desire for implicit promotion of certain const fns.

Well, there's desire for many things. ;) What's the argument against explicit promotion through e.g. const blocks?

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 13, 2019

What's the argument against explicit promotion through e.g. const blocks?

I guess the argument is that we already stabilized a few of these XD

@RalfJung
Copy link
Member

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.

@Centril
Copy link

Centril commented Mar 13, 2019

🕯 summons @Centril 🕯

😂

Maybe we call it #[pure] 😆

Eh... twas a joke?

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.

Depends on how complicated the analysis is... We already do have it sort of so there's no new fundamental complexity?

@RalfJung
Copy link
Member

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.

@eddyb
Copy link
Member

eddyb commented Mar 14, 2019

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, const blocks seem more like the garden variety of "explicit over implicit", with some fundamental advantages anyway.

(being able to do let s: &str = const { &format("...", ...) }; in the future seems neat)

Also, @RalfJung, have you seen const qualification lately ;)?

@RalfJung
Copy link
Member

I think too many explicit user annotations can sometimes add more complexity than they remove elsewhere.

Similar things can be said about too much magic, though -- it can hurt more than it helps.
AFAIK the lang team decided on a moratorium for adding more promotable things until we have this all put onto more solid grounds.

Also, @RalfJung, have you seen const qualification lately ;)?

No, did the long-standing refactoring finally happen? :D

@eddyb
Copy link
Member

eddyb commented Mar 14, 2019

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 Rvalue or Call terminator) started during the Berlin AllHands, and was merged some time later (rust-lang/rust#58403).

@eddyb
Copy link
Member

eddyb commented Mar 14, 2019

So I just had an idea, if we want no future const fns to be promotable by default:
There is a proper subset of "promotable const fn", and it's... drumroll pattern aliases!

Specifically, all stably-promotable const fns today (via #[rustc_promotable]) are constructors, which happen to be valid pattern aliases when injective¹!

If surjective² too (i.e. bijective), then the pattern alias is irrefutable (but this is optional).

According to @oli-obk, all #[rustc_promotable] const fns, other than 3 Duration constructors, are trivially injective. This is a good sign, even if we can't fix those 3 grandfathered-in stragglers.

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 Result.
And I know how I would turn it into a pattern (check that nanos % NANOS_PER_X == 0, nanos / NANOS_PER_X < X_PER_SEC and "x = secs * X_PER_SEC + nanos / NANOS_PER_X doesn't overflow"), but I don't know if we can automate it much (seems like a SAT solver thing).


¹injective: all input information is preserved in the output somehow, so f(x) = f(y) implies x = y
²surjective: all output type values can be produced, so you can always find corresponding input(s)

@RalfJung
Copy link
Member

the conversion from pervasive mutation to a functional-style computation (from a MIR Rvalue or Call terminator) started during the Berlin AllHands, and was merged some time later (rust-lang/rust#58403).

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 Cast).

There is a proper subset of "promotable const fn", and it's... drumroll pattern aliases!

That's a very interesting observation! Not sure what the plan is here, will we have pattern fn or so?^^

Is this a good enough argument to try and phase out the three promoteable functions that are not trivially injective?

@eddyb
Copy link
Member

eddyb commented Mar 14, 2019

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 #[rustc_promotable] functions and their bodies, just so we can triple-check the others are all trivial constructors.

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 const fn variant, but rather we have more restrictions by construction, and we can lower the pattern to a constructor.

@RalfJung
Copy link
Member

@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?

bors added a commit to rust-lang/rust that referenced this issue Dec 22, 2019
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
@RalfJung
Copy link
Member

Also Cc @ecstatic-morse, who seems to also be involved in the const-pattern effort.

Centril added a commit to Centril/rust that referenced this issue Jan 3, 2020
…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
Centril added a commit to Centril/rust that referenced this issue Jan 4, 2020
…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
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 6, 2020
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.
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 6, 2020
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.
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 6, 2020
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.
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 6, 2020
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.
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 6, 2020
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.
@RalfJung
Copy link
Member

RalfJung commented May 9, 2021

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 const fn is inside const/static bodies. There is a lot of code out there relying on this, so I currently consider it unlikely that we can take back this decision. This means we have to be careful never to evaluate constants in const/static bodies in potentially dead code -- but we probably can do that (we already have a test for this, src/test/ui/consts/const-eval/promoted_errors.rs; I'll expand that test a bit in rust-lang/rust#85112).

So I think this issue can be closed, finally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants