kpreid commented 24 days ago

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 commented 23 days ago •

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 commented 23 days ago

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 commented 19 days ago

Seems reasonable; let's get consensus:

@rfcbot merge

rfcbot commented 19 days ago •

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.

m-ou-se commented 19 days ago

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 commented 19 days ago •

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 commented 19 days ago

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 commented 19 days ago

@rfcbot reviewed

rfcbot commented 19 days ago

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

jhpratt commented 17 days ago •

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 commented 17 days ago •

@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 commented 17 days ago

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 commented 17 days ago

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

jhpratt commented 17 days ago

^^ 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 commented 17 days ago

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 commented 14 days ago

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 commented 14 days ago

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 commented 7 days ago

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.

jhpratt commented 7 days ago

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 commented 7 days ago

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 commented 7 days ago •

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.

tarcieri commented 6 days ago

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

jhpratt commented 6 days ago

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