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

Stabilize RFC 2345: Allow panicking in constants #85194

Closed
nikomatsakis opened this issue May 11, 2021 · 46 comments
Closed

Stabilize RFC 2345: Allow panicking in constants #85194

nikomatsakis opened this issue May 11, 2021 · 46 comments
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 11, 2021

Stabilization report

🛑 Issue with Rust 2021 🛑

Stabilization is currently blocked on resolving how this works with Rust 2021

Summary

This feature allows panicking within constants and reporting a custom message as a compile-time error.

The following code will become valid on stable:

const fn foo(val: u8) {
    assert!(val == 1, "val is not 1");
}

const MSG: &str = "custom message";

const _: () = std::panic!(MSG);
const _: () = panic!("custom message");

The message looks like:

error: any use of this value will cause an error
 --> src/main.rs:4:15
  |
4 | const _: () = std::panic!(MSG);
  | --------------^^^^^^^^^^^^^^^^-
  |               |
  |               the evaluated program panicked at 'custom message', src/main.rs:4:15
  |
  = note: `#[deny(const_err)]` on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>
  = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

Motivation for this RFC can be found here: https://rust-lang.github.io/rfcs/2345-const-panic.html#motivation

More examples can be found here: https://github.com/rust-lang/rust/blob/673d0db5e393e9c64897005b470bfeb6d5aec61b/src/test/ui/consts/const-eval/const_panic.rs

Edge cases & tests

Some questions which should be resolved in the future

We have some "unresolved questions" but they aren't an actual blocker.

  • Should there be some additional message in the error about this being a panic turned error?
    Or do we just produce the exact message the panic would produce?
  • This change becomes really useful if Result::unwrap and Option::unwrap become const fn, doing both in one go might be a good idea.

See the brief summary comment here: #51999 (comment)

Originally posted by @JohnTitor in #51999 (comment)

@Aaron1011
Copy link
Member

Aaron1011 commented May 11, 2021

One additional unresolved question that @RalfJung had brought up previously - do we ever want to allow catching panics in constants? The only issue I can think of would be users expecting panic!() to always abort compilation - but users should be aware that panics can be caught, especially if they're writing a const fn, which can also be called at runtime.

@nikomatsakis
Copy link
Contributor Author

Is the question really about whether we want to allow catching panics in constants?

@Aaron1011
Copy link
Member

IIRC, the original question was about whether panic!() is compatible with later deciding we want to allow catching panics in constants.

@Nemo157
Copy link
Member

Nemo157 commented May 11, 2021

const MSG: &str = "custom message";
const _: () = std::panic!(MSG);

This specific example triggers the non_fmt_panic lint, and will not compile in Edition 2021. To support usecases like this we would need to make std::panic::panic_any a const fn too (or get support for traits-in-const to allow using the format machinery).

@nikomatsakis
Copy link
Contributor Author

@Aaron1011 so in other words, the question is, will someone write a constant that panics and assumes that this panic aborts compilation in some crucial way, but then we introduce a wrapper that captures the panic, and now the safety guarantee is not upheld?

@jhpratt
Copy link
Member

jhpratt commented May 11, 2021

Personally I'd like to be able to panic in a const context to perform data validation from a macro. This would allow me to eliminate a procedural macro. If a const panic could be caught, that would effectively eliminate the ability to validate data (I think, feel free to correct me).

Reference to what I hope will be possible once const_panic is stabilized: time-rs/time#322

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented May 12, 2021

@jhpratt The code in that issue would not be affected by catch_unwind being usable in const contexts, since it stores the validated data in a const before returning it.

@CodesInChaos
Copy link

CodesInChaos commented May 12, 2021

@Nemo157 In my comment on the original issue I proposed other options, which might be easier to implement:

  1. make formatting work for string slice arguments, without relying on formatting traits
  2. make the precise pattern panic!("{}", str) work, where str is a string slice
  3. make panic_any const, but only if the argument is a string slice.

@nikomatsakis
Copy link
Contributor Author

I'd like an example where there is a panic that should abort compilation but catching it would produce a problem -- it feels like the panic erases the const value being produced, so I'm not clear on what kind of error would be swallowed that would be a problem.

@RalfJung
Copy link
Member

RalfJung commented May 13, 2021

@Aaron1011 wrote

One additional unresolved question that @RalfJung had brought up previously - do we ever want to allow panicking in constants?

I am confused -- yes we do want to allow panicking in constants...?

EDIT: Oh, I guess you forgot a "catch" somewhere... could you edit the text? Looks like I am not the only one who got confused. ;)

@RalfJung
Copy link
Member

it feels like the panic erases the const value being produced, so I'm not clear on what kind of error would be swallowed that would be a problem.

I guess if a const fn would perform some validation and then return (), if you can catch panics you can circumvent this validation.

But I don't see how this would be any more problematic at compile-time than it is at run-time.

@RalfJung
Copy link
Member

Another question that came up is if we should change the error that is emitted when a const/static panic such that it can not be allowed. I am sympathetic to that idea. The only reason CTFE error can be allowed at all is backwards compatibility, and there is no such concern with panics.

@nikomatsakis
Copy link
Contributor Author

I see. I definitely think it should be a hard error.

@lachlansneff
Copy link

Are unwrap and expect going to be stabilized at the same time as this?

@RalfJung
Copy link
Member

Option::unwrap can be stabilized; the others require parts of the format! machinery that are not const-compatible (they will be, eventually, but that'll require a lot of other infrastructure first).

@nikomatsakis nikomatsakis added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels May 18, 2021
@scottmcm
Copy link
Member

Based on the conversation in the lang team meeting today, we're good to move to

@rfcbot fcp merge


There was a conversation that potentially allowing the catching of panics is not really a concern, as we could create a more specific uncatchable-in-const error if needed (strawmen: abort or sys::exit). But also that the usual situations in which it's important to be able to catch panics don't really apply at compile-time, so not having it might also be fine.

@rfcbot
Copy link

rfcbot commented May 18, 2021

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 18, 2021
@nikomatsakis
Copy link
Contributor Author

panic in const questions writeup

do we want to allow catching panics in constants?

Why not? There may be some library that wishes to unconditionally abort compilation, but a wrapper library could prevent that from happening. However, as Ralf noted here, it's not clear why this should be more problematic at compilation time than runtime. It's also easy to imagine legitimate uses for catch in panics.

Niko's conclusion: We should document that panics may eventually be catchable and if folks want unconditional compilation aborts to raise those examples with us. We can add another mechanism to enable it (analogous to abort at runtime).

hard error or not?

Ralf wrote:

Another question that came up is if we should change the error that is emitted when a const/static panic such that it can not be allowed.

Niko's conclusion: It should be a hard error. Raising a concern for this.

@rfcbot concern hard-error

unwrap/expect

Question: can we stabilize unwrap/expect?

Ralf's Answer: unwrap yes, but expect requires more machinery

Seems fne.

@nikomatsakis
Copy link
Contributor Author

@rfcbot reviewed

I'm generally in favor, once my concern is resolved.

@cramertj
Copy link
Member

do we want to allow catching panics in constants?

I think I have vaguely the opposite preference. It's not clear to me why one would want to catch a panic that occurs at compile-time. The usual use-cases I'm aware of for catching panics involve recovery from unexpected runtime errors or building test harnesses, neither of which seem applicable. That said, I don't feel super strongly about this, and I think my preference here is largely a reflection of my usual distaste for catching panics (I can't remember the last time I used a mode other than panic='abort').

@Aaron1011
Copy link
Member

I've opened #85704 to turn const panics into a hard error.

@nikomatsakis
Copy link
Contributor Author

I don't understand why we would resolve the question of whether a panic can be caught or not at this time. I'd rather wait and gain experience. The catch_unwind function is not const-safe, so at the moment, they cannot, but we can simply document that it may become const safe in the future, and that if anyone anticipates a particular problem with that, we would like to know about it.

@Aaron1011
Copy link
Member

@nikomatsakis: With #85704 merged, a const panic now results in a hard error.

@RalfJung
Copy link
Member

RalfJung commented Jun 1, 2021

Yeah, #80734 should have fixed exactly that ICE... and indeed the test cases there seem to work fine, no ICE. @m-ou-se could you report an issue with a self-contained example triggering the ICE?

@m-ou-se
Copy link
Member

m-ou-se commented Jun 1, 2021

Why won't this make panic!(MSG) work?

Because panic!(MSG) won't work at all in Rust 2021, simply because that invokes format_args!(MSG), which doesn't work.

could you report an issue with a self-contained example triggering the ICE?

Oh, didn't realize that was already fixed. Apparently I hit an edge case by accident, which still ICE's: #85907

@RalfJung
Copy link
Member

RalfJung commented Jun 1, 2021

Because panic!(MSG) won't work at all in Rust 2021, simply because that invokes format_args!(MSG), which doesn't work.

Ah yeah I see.
I assume there's no good way to make as_str work on format_args!("{}", "msg")?

@m-ou-se
Copy link
Member

m-ou-se commented Jun 1, 2021

I assume there's no good way to make as_str work on format_args!("{}", "msg")?

That would be relatively simple to implement as a special case in the builtin macro. But making it work for format_args!("{}", SOME_CONST_STR) would be a bit harder. That could work by adding an extra case to match in the as_str() implementation I suppose. It'd have to compare the function pointer to check if it's a &str and not some other type though, which might open a small can of worms.

@Mark-Simulacrum
Copy link
Member

We discussed this in the 2021-06-15 lang meeting and determined it makes sense to close this issue specifically tracking stabilization at this time. There's a clear blocker to stabilizing at this time around the migration story to the next edition. The lang team is still interested in this getting stabilized, but until that issue is resolved doesn't want to track stabilization on this issue. A new one can get filed and FCP re-proposed once there's a new draft.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 22, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 27, 2021
…r, r=kennytm

Make `fmt::Arguments::as_str` unstably const

Motivation: mostly to move "panic!() in const contexts" forward, making use of `as_str` was mentioned in rust-lang#85194 (comment) and seems like the simplest way forward.
@nbdd0121
Copy link
Contributor

nbdd0121 commented Jul 4, 2021

Is it possible to special-case Rust 2021 panic!("{}", expr) to mean Rust 2015 panic!(expr)?

@m-ou-se
Copy link
Member

m-ou-se commented Jul 5, 2021

@nbdd0121 Those are not the same:

  • panic!("{}", 123); throws a String containing "123". panic!(123) throws an i32 with the value 123.
  • panic!("{}", &some_temporary) is valid, while panic!(&some_temporary) is not.

We can make panic_any(expr) work in constants, but that function only exists in std, not in core.

@clarfonthey
Copy link
Contributor

So, I see that there are a lot of blockers on the 2021-edition-style panicking, but I don't actually know what the plan is for this. Got lead here from the tracking issue for #53188, which is blocked on this but still open.

Is the plan to just never allow panicking in const functions, ever? That seems wrong. Are we blocking it on const trait impls to ensure that arbitrary panic! can be const? It seems like closing this issue without pointing to a replacement is a bit confusing.

@jhpratt
Copy link
Member

jhpratt commented Sep 6, 2021

Personally I'd like to see const panic with only string literals supported — this would permit things along the lines of (debug) assertions so long as a custom message is provided. Something is better than nothing.

@clarfonthey
Copy link
Contributor

I should add that @m-ou-se opened #78356 to help make the literal-only case identical on 2021 edition, which is mostly the primary blocker.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 6, 2021

@clarfonthey There's multiple ways forward:

In addition, I've heard this idea floating around to make const_panic enable some kind of -Z unleash-the-miri-inside-of-you-like thing on panic!(). (cc @oli-obk) The idea is that since compilation is going to fail/panic anyway, the compiler doesn't have to follow the strict const evaluation rules anymore and can just try to evaluate anything to try to generate the full panic message, regardless of if Display implementations etc. were marked as const. (Miri can already evaluate pretty much all of them, as long as they don't open files etc.)

@jhpratt
Copy link
Member

jhpratt commented Sep 6, 2021

  • panic!("literal") already works in Rust 2021 starting in 1.56:

Still unstably by the looks of it? playground

Just asking for clarity purposes because your wording makes it sound like that subset has been stabilized.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 6, 2021

Yes, gated behind const_panic, like all const panic, yes.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 6, 2021

@clarfonthey Note that this issue was just for the stabilization report.

The tracking issue for const_panic is here, and still open and active: #51999

@clarfonthey
Copy link
Contributor

Oh! That makes a lot of sense now.

@CodesInChaos
Copy link

I'd like to see at least panic!(literal) stabilized as soon as possible. I believe that case has no open concerns, and it's already much clearer than the index-out-of-bounds hacks people use at the moment.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Sep 7, 2021

I'd like to see at least panic!(literal) stabilized as soon as possible. I believe that case has no open concerns, and it's already much clearer than the index-out-of-bounds hacks people use at the moment.

The issue is that we want all code that works in Rust 2018 to work (or auto-fixed to working code) in Rust 2021. If we stablise panic!(literal), then panic!("{}") works in Rust 2018 but not work in Rust 2021. I see three options here for the MVP:

  • Change the lint to suggest panic!("{{}}") instead of panic!("{}", "{}").
  • Only const-stablise panic!(literal) in Rust 2018 if literal doesn't contain '{' and '}'.
  • Treat panic!("{}", literal) to same as panic!(escaped_liteal).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests