const_panic: Allow panicking in const fn

[josephlr]

We need to make sure certain panic lang items are skipped when qualifying
min_const_fn. Fixes issue found here: #51999 (comment)

Signed-off-by: Joe Richey [email protected]

[rust-highfive]

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

[RalfJung]

Cc @rust-lang/wg-const-eval

[petrochenkov]

r? @RalfJung

[ecstatic-morse]

As written, this PR would allow const-stable const fn in the standard library to rely on the unstable const_panic feature. This is not a big deal, since you can already trigger a panic inside a const fn with an out-of-bounds array access and we would likely catch this during review anyways. However, I think it makes this PR not worth it, since the only burden on the end-user currently is that #![feature(const_fn)] is required in addition to #![feature(const_panic)].

I discuss the long-term solution for this unfortunate UI as part of #76618.

[josephlr]

As written, this PR would allow const-stable const fn in the standard library to rely on the unstable const_panic feature.

Good catch, I'll amend this PR to make sure the const_panic behaves like other unstable const fns (namely it should only be used inside of macros or functions marked with #[allow_internal_unstable(const_panic)])

However, I think it makes this PR not worth it, since the only burden on the end-user currently is that #![feature(const_fn)] is required in addition to #![feature(const_panic)].

@ecstatic-morse, note that this change is not just to increase usability/consistency, but is a necessary prerequisite to stabilize const_panic. It's also important for authors of nightly-only libraries to have a way to only rely on a "reasonable" subset of const_fn that is on track to be stablized, rather than pulling in all of const_fn.

To that end, I think we should keep this PR so that const_panic behaves like similar const_* features. However, I think exploring your idea in #76618 has merit (I have personally found the UI confusing).

EDIT: It looks like we already use panic! in a few places inside feature-gated const fns, so making that not work might break things. Furthermore, adding the feature check in qualify_min_const_fn makes the error messages significantly worse. I think we should leave this as-is, and track fixing this (and related issues) in #76618. This problem isn't unique to panicking, it would also apply to any externally unstable const fn operation used in a stable libstd fucntion (like mut refs).

[RalfJung]

Good catch, I'll amend this PR to make sure the const_panic behaves like other unstable const fns (namely it should only be used inside of macros or functions marked with #[allow_internal_unstable(const_panic)])

Ah, I had forgotten about that, thanks @ecstatic-morse!
@josephlr I don't entirely see how you want to do that, without replicating something min-const-fn-like in the general const-fn check. Please don't do that ad-hoc.

note that this change is not just to increase usability/consistency, but is a necessary prerequisite to stabilize const_panic

The way we usually did things (I think) is that this change is part of stabilization. There is no good way to do it in advance.

To that end, I think we should keep this PR so that const_panic behaves like similar const_* features.

AFAIK it already does, though? It should behave like the other language-level const_ features. Those are very different from library features.

It's also important for authors of nightly-only libraries to have a way to only rely on a "reasonable" subset of const_fn that is on track to be stablized, rather than pulling in all of const_fn.

Agreed, and that's a good argument for #76618.

I think we should leave this as-is

Do you mean leave the PR as-is or leave the code in master as-is and close the PR?

[josephlr]

Do you mean leave the PR as-is or leave the code in master as-is and close the PR?

I meant to merge the PR as is (without adding the allow_internal_unstable check).

[RalfJung]

Do you mean leave the PR as-is or leave the code in master as-is and close the PR?

I meant to merge the PR as is (without adding the allow_internal_unstable check).

But the PR as-is breaks our const stability story, as @ecstatic-morse pointed out. That is a no-go.

[josephlr]

Do you mean leave the PR as-is or leave the code in master as-is and close the PR?

I meant to merge the PR as is (without adding the allow_internal_unstable check).

But the PR as-is breaks our const stability story, as @ecstatic-morse pointed out. That is a no-go.

Sorry my point was that, even without this PR, it's already possible for libstd/libcore library functions to call panic! without a #[allow_internal_unstable] annotation and without the user needing to specify #![feature(const_fn)]. See this code for example: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=e337b9c6ad9b1f9dd98a5ebdd7e8ff2b

AFAIK it already does, though? It should behave like the other language-level const_ features. Those are very different from library features.

Looking at #![feature(const_mut_refs)] that doesn't requires #[const_fn], see https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=c8a8ceeb732b472b30e8709288f61b5a

This working but const_panic not working seemed inconsistent to me.

[RalfJung]

Sorry my point was that, even without this PR, it's already possible for libstd/libcore library functions to call panic! without a #[allow_internal_unstable] annotation and without the user needing to specify #![feature(const_fn)]

Not sure what this demonstrates... the point is that a stable std/core function cannot do this. Option::unwrap is unstable.

Looking at #![feature(const_mut_refs)] that doesn't requires #[const_fn], see https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=c8a8ceeb732b472b30e8709288f61b5a

This working but const_panic not working seemed inconsistent to me.

Interesting. So how does this ensure it is not being used in a stable const fn? (My hypothesis is that it doesn't, and we should fix it, at which point const_mut_refs would behave like const_panic already does.)

[josephlr]

Not sure what this demonstrates... the point is that a stable std/core function cannot do this. Option::unwrap is unstable.

Oh my bad, this makes total sense. I'll see if I can get something working, if not I will close this PR in favor of #76618 or just stabilizing const_panic.

Interesting. So how does this ensure it is not being used in a stable const fn? (My hypothesis is that it doesn't, and we should fix it, at which point const_mut_refs would behave like const_panic already does.)

OK this makes more sense.

[josephlr]

After some investigation, @RalfJung and @ecstatic-morse are correct. Actually having this work with good error messages makes the code very complex for little gain. Closing this PR in favor of just directly stabilizing panicing in const-fn

@ecstatic-morse Nice job in catching this subtle issue.