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 #89006

Closed
oli-obk opened this issue Sep 16, 2021 · 24 comments
Closed

Stabilize RFC 2345: Allow panicking in constants #89006

oli-obk opened this issue Sep 16, 2021 · 24 comments
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Sep 16, 2021

Stabilization report

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");
}

// can be also computed by manually creating a `&[u8]` and
// from_utf8_unchecked.
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)

Previous attempt: #85194 (This was blocked because it was not possible to use a generated panic message, only literal strings were allowed, which has been resolved in #88954)

Originally posted by @JohnTitor in #51999 (comment)

cc @rust-lang/wg-const-eval

@oli-obk oli-obk added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Sep 16, 2021
@kpreid
Copy link
Contributor

kpreid commented Sep 16, 2021

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?

My perspective as someone who is frequently trying to make good error messages:

  1. Yes, there should be some indication that the message text originated from user code — otherwise you'll get confused users asking “what does this compiler error mean” when using a library's const fns.
  2. On the other hand, it is very helpful to readability if the message is presented as a message in itself rather than a quoted fragment with both leading and trailing text inside a larger error message. Quotation with content escapes, and line wrapping, also harm scannability, and make it less likely that a user will understand that this is the relevant information. (Rust's run-time panics unfortunately already have this problem.)

For example, the existing example

4 | const _: () = std::panic!("{}", MSG);
  | --------------^^^^^^^^^^^^^^^^^^^^^^-
  |               |
  |               the evaluated program panicked at 'custom message', src/main.rs:4:15

would in my opinion be better formatted as

4 | const _: () = std::panic!("{}", MSG);
  | --------------^^^^^^^^^^^^^^^^^^^^^^-
  |               |
  |               the evaluated program panicked at src/main.rs:4:15 with:
  |                   custom message

The first sample has the virtue of more closely matching runtime behavior; but I think there should be generally higher expectations for readability from messages during compilation than those which must be formatted by handler code embedded in the final binary.

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Sep 17, 2021

would in my opinion be better formatted as

4 | const _: () = std::panic!("{}", MSG);
  | --------------^^^^^^^^^^^^^^^^^^^^^^-
  |               |
  |               the evaluated program panicked at src/main.rs:4:15 with:
  |                   custom message

I agree. I'd also expect multiline panic messages to stay indented, so that they can be easily distinguished from the following compilation errors.

I would prefer if both const panics and compile_error were consistent in that, and change this:

error: 
hello
world

 --> src/main.rs:1:1
  |
1 | / compile_error!{"
2 | | hello
3 | | world
4 | | "}
  | |__^

error: could not compile `playground` due to previous error

to the indented style you're proposing.

@RalfJung
Copy link
Member

There was some long discussion about error formatting in a previous PR as well, more than a year ago... maybe someone can dig up the link.

I think const panic messages should be consistent with other kinds of errors that stop const-evaluation. So, I am not a fan of special-casing const panic error formatting. But I am open to improving const-eval error rendering in general.

@joshtriplett
Copy link
Member

Seems reasonable; let's get consensus:

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Sep 21, 2021

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

No concerns currently listed.

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 Sep 21, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Sep 21, 2021

Important to note: This will make assert!(1 == 2, "...") work in const, but assert_eq!(1, 2, "...") will still not work, since it uses more complicated formatting.

@c410-f3r
Copy link
Contributor

c410-f3r commented Sep 21, 2021

It is not the scope of this issue but I would like to chime to draw the attention of also considering "constifying" Result::unwrap() and Option::unwrap(). Without them, people will probably write

const FOO: () = {
    let _ = match SOME_OPERATION {
        Err(_err) => panic!("SOME ERROR"),
        Ok(elem) => elem
    };
};

instead of the much simpler

const FOO: () = {
    let _ = SOME_OPERATION.unwrap();
};

@RalfJung
Copy link
Member

Result::unwrap is way out of reach for now since it formats the Err value.

But Option::unwrap and Option::expect would be reasonable to stabilize I think -- they are basically forms of assert.

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Sep 21, 2021
@rfcbot
Copy link

rfcbot commented Sep 21, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 21, 2021
@jhpratt
Copy link
Member

jhpratt commented Sep 23, 2021

I just checked: Option::unwrap and Option::expect can not be stabilized.

Ignoring the message that const_panic is unstable, the compiler still complains:

error[E0493]: destructors cannot be evaluated at compile-time
   --> library/core/src/option.rs:733:25
    |
733 |     pub const fn unwrap(self) -> T {
    |                         ^^^^ constant functions cannot evaluate destructors
...
738 |     }
    |     - value is dropped here

with the same error for expect. Until impl const Drop is a thing, it'll have to wait for stdlib. However, it would be possible to define a macro that does this. That is obviously not for stdlib to do.

With regard to Result methods, even complex const-panic doesn't unblock anything. I have a full list of blockers up at #82814. Pretty much everything needs impl const Trait.

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Sep 23, 2021

@jhpratt Option::unwrap doesn't only work with const Trait bounds, it also works with precise drop tracking(which allows the compiler to figure out that no dropping actually happens), can't tell you which is likely to be stabilized earlier.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=329872502390d743de89e55b93b65263

#![feature(const_panic)]
#![feature(const_precise_live_drops)] 

pub const fn unwrap<T>(this: Option<T>) -> T {
    match this {
        Some(val) => val,
        None => panic!("called `Option::unwrap()` on a `None` value"),
    }
}

Tracking issue for precise live drops

@jhpratt
Copy link
Member

jhpratt commented Sep 23, 2021

Certainly. I suspected that was the error I was going to run into, actually. I just didn't bother to keep adding features once I confirmed my suspicion that it wouldn't compile with just const_panic.

@Nugine
Copy link
Contributor

Nugine commented Sep 23, 2021

It's possible to make char::from_u32_unchecked a const fn if we have const_panic stablized.

#[inline]
#[stable(feature = "char_from_unchecked", since = "1.5.0")]
pub unsafe fn from_u32_unchecked(i: u32) -> char {
// SAFETY: the caller must guarantee that `i` is a valid char value.
if cfg!(debug_assertions) { char::from_u32(i).unwrap() } else { unsafe { transmute(i) } }
}

#[stable(feature = "try_from", since = "1.34.0")]
impl TryFrom<u32> for char {
type Error = CharTryFromError;
#[inline]
fn try_from(i: u32) -> Result<Self, Self::Error> {
if (i > MAX as u32) || (i >= 0xD800 && i <= 0xDFFF) {
Err(CharTryFromError(()))
} else {
// SAFETY: checked that it's a legal unicode value
Ok(unsafe { transmute(i) })
}
}
}

@jhpratt
Copy link
Member

jhpratt commented Sep 23, 2021

^^ that would require const unwrap when written as-is

If there are the appropriate MIRI checks in place (I don't know if there are) it could just be a straight transmute, which is already stabilized. It could theoretically introduce UB in debug builds, but that's the caller's problem (don't use unchecked if it's not valid).

@Nugine
Copy link
Contributor

Nugine commented Sep 23, 2021

I found it quite simple to constify from_u32_unchecked.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=23a94bb268784e92ae2cecf236ecafe4

#![feature(const_panic)]

pub const fn from_u32(i: u32) -> Option<char> {
    if (i > '\u{10ffff}' as u32) || (i >= 0xD800 && i <= 0xDFFF) {
        None
    } else {
        // SAFETY: checked that it's a legal unicode value
        Some(unsafe { core::mem::transmute(i) })
    }
}

pub const unsafe fn from_u32_unchecked(i: u32) -> char {
    if cfg!(debug_assertions) {
        match from_u32(i) {
            Some(val) => val,
            None => panic!("illegal unicode value"),
        }
    } else {
        // SAFETY: checked that it's a legal unicode value
        core::mem::transmute(i)
    }
}

@c410-f3r
Copy link
Contributor

Result::unwrap is way out of reach for now since it formats the Err value.

But Option::unwrap and Option::expect would be reasonable to stabilize I think -- they are basically forms of assert.

Thanks for the explanation, @RalfJung !

Although not yet in the scope of this issue, may I suggest also "constifying" Resull::ok? In the absence of const Result::unwrap, such method would be very useful.

const FOO: () = {
    let _ = SOME_RSLT.ok().unwrap();
};

@bjorn3
Copy link
Member

bjorn3 commented Sep 26, 2021

Although not yet in the scope of this issue, may I suggest also "constifying" Resull::ok?

That will require const drop too. Result::ok() drops the error variant.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 3, 2021
@rfcbot
Copy link

rfcbot commented Oct 3, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Oct 3, 2021
@jhpratt
Copy link
Member

jhpratt commented Oct 3, 2021

Just to avoid conflicts with others, I have a commit locally that stabilizes some things only blocked by this. Planning on opening PRs later tonight or tomorrow.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Oct 3, 2021

Just to avoid conflicts with others, I have a commit locally that stabilizes some things only blocked by this. Planning on opening PRs later tonight or tomorrow.

Are you planning a PR to stablizes const_panic or just features that depend on const_panic? This is a stablisation issue not a PR.

@jhpratt
Copy link
Member

jhpratt commented Oct 3, 2021

The latter; I was only planning on stabilizing things that depend on const_panic (plus one that was blocked process-wise, not technically). I can submit the actual stabilization of const_panic if desired, though.

Edit: Working on stabilizing the const_panic feature first. May or may not be done tonight.

Manishearth added a commit to Manishearth/rust that referenced this issue Oct 4, 2021
…shtriplett

Stabilize `const_panic`

Closes rust-lang#51999

FCP completed in rust-lang#89006

`@rustbot` label +A-const-eval +A-const-fn +T-lang

cc `@oli-obk` for review (not `r?`'ing as not on lang team)
Manishearth added a commit to Manishearth/rust that referenced this issue Oct 4, 2021
…shtriplett

Stabilize `const_panic`

Closes rust-lang#51999

FCP completed in rust-lang#89006

``@rustbot`` label +A-const-eval +A-const-fn +T-lang

cc ``@oli-obk`` for review (not `r?`'ing as not on lang team)
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Oct 4, 2021
…shtriplett

Stabilize `const_panic`

Closes rust-lang#51999

FCP completed in rust-lang#89006

```@rustbot``` label +A-const-eval +A-const-fn +T-lang

cc ```@oli-obk``` for review (not `r?`'ing as not on lang team)
@tarcieri
Copy link
Contributor

tarcieri commented Oct 5, 2021

With 9866b09 having landed, it looks like this can be closed?

@jhpratt
Copy link
Member

jhpratt commented Oct 5, 2021

Ah dang, missed the opportunity to link it in like I did the tracking issue. Yes, this can be closed.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 7, 2021
XrXr added a commit to XrXr/reference that referenced this issue Dec 24, 2021
Since const panics are now [stabilized], I figure it would be good to
document some of its gotchas. I couldn't really find documents for
the specifics I ran into. I am no const eval expert, and what I
wrote is basically a paraphrase of Ralf's comments[^1][^2],
but I hope to get the ball rolling for adding some docs.

I deliberately chose to not mention the guarantee about syntactically
referenced constants in functions that require code generation as it
seems hard to explain completely without some ambiguity. It also seems
to me that the rules are not completely stable[^3][^4] yet.

[stabilized]: rust-lang/rust#89006
[^1]: rust-lang/rust#91877 (comment)
[^2]: rust-lang/rust#91877 (comment)
[^3]: rust-lang/rust#71800
[^4]: rust-lang/rust#51999 (comment)
XrXr added a commit to XrXr/reference that referenced this issue Dec 24, 2021
Since const panics are now [stabilized], I figure it would be good to
document some of its gotchas. I couldn't really find documents for
the specifics I ran into. I am no const eval expert, and what I
wrote is basically a paraphrase of Ralf's comments[^1][^2],
but I hope to get the ball rolling for adding some docs.

I deliberately chose to not mention the guarantee about syntactically
referenced constants in functions that require code generation as it
seems hard to explain completely without some ambiguity. It also seems
to me that the rules are not completely stable[^3][^4] yet.

[stabilized]: rust-lang/rust#89006
[^1]: rust-lang/rust#91877 (comment)
[^2]: rust-lang/rust#91877 (comment)
[^3]: rust-lang/rust#71800
[^4]: rust-lang/rust#51999 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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