-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Allow panicking with string literal messages inside constants #52011
Allow panicking with string literal messages inside constants #52011
Conversation
This comment has been minimized.
This comment has been minimized.
70760f5
to
a26713a
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
lol... uhm... there's a This seems very fragile (like if an |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@oli-obk I would test this both in rlib context and in binary context as the later requires the panic_impl lang item. The tests would look like this: #![no_std]
#![crate_type = "lib"]
const Z: () = panic!("cheese");
const Y: () = unreachable!();
const X: () = unimplemented!(); #![crate_type = "bin"]
#![feature(lang_items)]
#![feature(panic_implementation)]
#![no_main]
#![no_std]
use core::panic::PanicInfo;
const Z: () = panic!("cheese");
const Y: () = unreachable!();
const X: () = unimplemented!();
#[lang = "eh_personality"]
fn eh() {}
#[panic_implementation]
fn panic(_info: &PanicInfo) -> ! {
loop {}
}
|
I already had it implemented. I just failed at building the test cases |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
957ea32
to
203453a
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
203453a
to
180df96
Compare
☔ The latest upstream changes (presumably #51702) made this pull request unmergeable. Please resolve the merge conflicts. |
src/librustc/mir/interpret/error.rs
Outdated
msg: String, | ||
line: u32, | ||
col: u32, | ||
file: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want to use Symbol
for msg
and file
.
let len = len.to_bits(ecx.memory.pointer_size())?; | ||
let bytes = ecx.memory.read_bytes(ptr, Size::from_bytes(len as u64))?; | ||
let str = ::std::str::from_utf8(bytes).map_err(|err| EvalErrorKind::ValidationFailure(err.to_string()))?; | ||
Ok(str.to_string()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and intern to Symbol
here, without allocating a copy of the string slice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, because the original value also came from a symbol? Or for making cloning the error cheap?
} | ||
} | ||
|
||
fn to_num<'a, 'tcx, 'mir>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be named to_u32
.
#![no_std] | ||
#![crate_type = "lib"] | ||
|
||
const Z: () = panic!("cheese"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to require no feature-gate - I don't think this should be insta-stable. Or is it impossible to feature-gate because panic!
has allow_internal_unstable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious. I should be able to work around this easily. Maybe const stability of const fns should not be allowed to be circumvented by rustc macros?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried. I have no idea how this test is still passing. We seem to be having a stability hole.
LL | const Z: () = panic!("cheese"); | ||
| ^^^^^^^^^^^^^^----------------^ | ||
| | | ||
| evaluated panic invocation 'cheese', $DIR/const_panic_libcore.rs:14:15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"evaluated" doesn't feel right in user output. Maybe mimick the runtime output and say:
panicked at 'cheese', $DIR/const_panic_libcore.rs:14:15
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had that. Our test suite is not able to cope with that because it greps for panicked at
and reports a compiler panic. Once the test suite is adjusted, I can fix the message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like const panic at 'cheese'
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just change the test suite to search for ' panicked at '
, i.e. take advantage of the fact that there should be a non-space character before the space before "panicked"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once #52197 is merged we don't need any more text hacks. I'll just wait for that
cc @alexcrichton @nikomatsakis on #52011 (comment) (insta-stable feature) |
180df96
to
188e451
Compare
(For easy checking: this PR is blocked on #52197.) |
☔ The latest upstream changes (presumably #52318) made this pull request unmergeable. Please resolve the merge conflicts. |
188e451
to
6633df0
Compare
Unblocked and fixed |
Ping from triage @eddyb! This PR needs your review. |
Failure log:
|
⌛ Testing commit 768ffb714d0ebab8faa9ff6d97307219615016ca with merge 741e7c3c07a2b4a7630b9c35516eec4105991fca... |
💔 Test failed - status-appveyor |
A lang item is not defined. Same error as #52011 (comment). ---- [ui] ui\const-eval\const_panic_libcore_main.rs stdout ----
diff of stderr:
- error: this constant cannot be used
- --> $DIR/const_panic_libcore_main.rs:20:1
- |
- LL | const Z: () = panic!("cheese");
- | ^^^^^^^^^^^^^^----------------^
- | |
- | the evaluated program panicked at 'cheese', $DIR/const_panic_libcore_main.rs:20:15
- |
- = note: #[deny(const_err)] on by default
- = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
+ error: language item required, but not found: `eh_unwind_resume`
11
- error: this constant cannot be used
- --> $DIR/const_panic_libcore_main.rs:23:1
- |
- LL | const Y: () = unreachable!();
- | ^^^^^^^^^^^^^^--------------^
- | |
- | the evaluated program panicked at 'internal error: entered unreachable code', $DIR/const_panic_libcore_main.rs:23:15
- |
- = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
-
- error: this constant cannot be used
- --> $DIR/const_panic_libcore_main.rs:26:1
- |
- LL | const X: () = unimplemented!();
- | ^^^^^^^^^^^^^^----------------^
- | |
- | the evaluated program panicked at 'not yet implemented', $DIR/const_panic_libcore_main.rs:26:15
- |
- = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
-
- error: aborting due to 3 previous errors
+ error: aborting due to previous error
33
34 |
☔ The latest upstream changes (presumably #52953) made this pull request unmergeable. Please resolve the merge conflicts. |
768ffb7
to
0db77dd
Compare
0db77dd
to
bd6ae6a
Compare
rebased and added lang item to test @bors r=eddyb |
📌 Commit bd6ae6a has been approved by |
} else { | ||
bug!("panic arg is not a str") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have that same code in miri...^^
…anics_constantly, r=eddyb Allow panicking with string literal messages inside constants r? @eddyb cc #51999 we can't implement things like `panic!("foo: {}", x)` right now because we can't call trait methods (most notably `Display::fmt`) inside constants. Also most of these impls probably have loops and conditions, so it's messy anyway. But hey `panic!("foo")` works at least. cc @japaric got any test ideas for `#![no_std]`?
☀️ Test successful - status-appveyor, status-travis |
r? @eddyb
cc #51999
we can't implement things like
panic!("foo: {}", x)
right now because we can't call trait methods (most notablyDisplay::fmt
) inside constants. Also most of these impls probably have loops and conditions, so it's messy anyway.But hey
panic!("foo")
works at least.cc @japaric got any test ideas for
#![no_std]
?