implement #[panic_implementation] #50338
Conversation
rust-highfive
added
the
S-waiting-on-review
src/libcore/panicking.rs
Outdated
|
|
||
| #[cfg(not(stage0))] | ||
| #[allow(improper_ctypes)] // PanicInfo contains a trait object which is not FFI safe | ||
| extern "C" { |
There was a problem hiding this comment.
Why not extern "Rust"? Would that solve the improper_ctypes warning?
There was a problem hiding this comment.
The lint seems to apply regardless of the ABI ("C" or "Rust").
There was a problem hiding this comment.
The comment should indicate why it is fine to pass such an object anyway (it never actually passes a FFI boundary and is a Rust-to-Rust call)
|
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 |
src/libcore/panicking.rs
Outdated
|
|
||
| #[cfg(not(stage0))] | ||
| #[allow(improper_ctypes)] // PanicInfo contains a trait object which is not FFI safe | ||
| extern "C" { |
There was a problem hiding this comment.
The comment should indicate why it is fine to pass such an object anyway (it never actually passes a FFI boundary and is a Rust-to-Rust call)
|
The changes look good otherwise. |
src/libcore/panicking.rs
Outdated
| #[cfg(not(stage0))] | ||
| #[cold] #[inline(never)] | ||
| pub fn panic_fmt(fmt: fmt::Arguments, file_line_col: &(&'static str, u32, u32)) -> ! { | ||
| struct NoPayload; |
There was a problem hiding this comment.
is this "null" payload used?
I fixed those. I seem to have broken a bit the panicking (/ unwinding?) stuff; I'm seeing some proc-macro related tests fail. I won't have time to dig further any time soon but here's the list of failing tests if someone wants to investigate:
|
src/libcore/panicking.rs
Outdated
|
|
||
| #[cfg(not(stage0))] | ||
| #[cold] #[inline(never)] | ||
| pub fn panic_payload<M>(msg: M, file_line_col: &(&'static str, u32, u32)) -> ! |
There was a problem hiding this comment.
I forget, but is this something we want to enable for libcore? I thought that we didn't have a path to actually supporting this yet?
(support in the sense of transmitting the value all the way to the caught value of thread::spawn)
There was a problem hiding this comment.
I forget, but is this something we want to enable for libcore?
Well, this (adding payload support to core::panic!) is in the accepted RFC. Though we don't have to add it right now; we can stabilize #[panic_implementation] w/o payload support and add payload support later on in a backwards compatible fashion (or at least I think it's possible).
| @@ -1148,6 +1148,48 @@ fn check_fn<'a, 'gcx, 'tcx>(inherited: &'a Inherited<'a, 'gcx, 'tcx>, | |||
| } | |||
| } | |||
|
|
|||
| // Check that a function marked as `#[panic_implementation]` has signature `fn(&PanicInfo) -> !` | |||
There was a problem hiding this comment.
I'm personally more of a fan of generating AST nodes which correspond to the structure we want which avoids reaching into the internals of the typechecker where possible. For example I don't think this disallows something like this:
#[panic_implementation]
fn foo<T>(a: &PanicInfo) -> ! {
}right?
I was thinking that we'd basically expand #[panic_implementation] to #[lang = "panic_impl"] where the lang item internally dispatches to #[panic_implementation] (automatically typechecking it along the way). That may be slightly more difficult to architect, though, but I'm curious what you think
There was a problem hiding this comment.
The language items are not type-checked at all at the moment, so there is nothing to hand the type-checking over to.
panic_fmt has no benefits over the new language item and IIRC (from when I wrote the instructions) complicates the implementation of this feature unnecessarily.
There was a problem hiding this comment.
Right yeah, my point is that we generate a language item which internally dispatches to the function annotated #[panic_implementation], which by construction will typecheck the panic implementation and not require typechekcing of lang ites.
There was a problem hiding this comment.
@alexcrichton you mean something like this?
// this
// (signature is totally wrong)
#[panic_implementation]
fn foo<T>(pi: &i32) {
// body
}
// expands into
#[lang = "panic_impl"]
fn __secret_name_maybe(pi: &PanicInfo) -> ! {
// user input
fn foo<T>(pi: &i32) {
// body
}
let f: fn(&PanicInfo) -> ! = foo;
f()
}That would work typechecking wise, but will it report error messages with meaningful spans? (My experience with proc macros says that most errors will wrongly report the whole #[panic_implementation] item as their span)
There was a problem hiding this comment.
I don't think this disallows something like this:
fn foo(a: &PanicInfo) -> ! {
I can add a check and test to reject that.
There was a problem hiding this comment.
@japaric yeah that latter idea is what I was thinking, where basically the compiler manufactures the actual lang item which is hooked up to call the #[panic_implementation]. That way the compiler has full control over ABI and whatnot
There was a problem hiding this comment.
Sadly in many cases this approach results in kind-of suboptimal errors.
For example
fn foo() {
fn bar<T>(i: i32) -> ! { loop {} }
let foo: fn(i32) -> ! = bar;
}
gives
error[E0282]: type annotations needed
--> src/main.rs:3:29
|
3 | let foo: fn(i32) -> ! = bar;
| ^^^ cannot infer type for `T`
but if expanded within a macro it would be something like this instead:
error[E0282]: type annotations needed
--> src/main.rs:3:29
|
3 | #[panic_implementation]
| fn panic<T>(...) -> ! { loop {} }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type for `T`
which is just… incomprehensible.
That being said, I have a long time wish to eventually implement universal checking of the language item signatures (similar to one we do for intrinsics), which would resolve this issue universally. Thus, I’d recommend going for the simplest complete approach possible for now.
src/libstd/panicking.rs
Outdated
| @@ -180,7 +180,7 @@ fn default_hook(info: &PanicInfo) { | |||
|
|
|||
| let location = info.location().unwrap(); // The current implementation always returns Some | |||
|
|
|||
| let msg = match info.payload().downcast_ref::<&'static str>() { | |||
| let msg = match info.payload().downcast_ref::<&str>() { | |||
There was a problem hiding this comment.
How come this was necessary? I thought Any-style things could only be with 'static types?
|
☔ The latest upstream changes (presumably #49789) made this pull request unmergeable. Please resolve the merge conflicts. |
shepmaster
added
S-waiting-on-author
|
Ping from triage, @japaric ! You've got merge conflicts and some review comments pending. |
|
Ping from triage @japaric! Will you have time to work on this soon? |
japaric
force-pushed
the
panic-impl
branch
2 times, most recently
from
6cbc8af to
5830bc2
Compare
|
I have reverted the payload in core::panic! stuff hoping that it makes this PR less controversial. See also #50338 (comment). This has been rebased and passes the test suite when using stage 2. Turns out all the weird errors around proc-macros that I was seeing only occur when testing stage 1 ( |
|
Could some tests also be added for situations like:
|
|
☔ The latest upstream changes (presumably #50629) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping from triage @japaric! The reviewer provided some feedback, do you have time to address it? |
|
@alexcrichton requested tests have been added. r? |
This comment has been minimized.
This comment has been minimized.
pietroalbini
added
S-waiting-on-review
|
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 |
kennytm
added
S-waiting-on-author
|
@bors r=alexcrichton |
|
📌 Commit 8ad15de has been approved by |
bors
added
S-waiting-on-bors
implement #[panic_implementation] This implements the `#[panic_implementation]` attribute as instructed in #44489 (comment) I haven't run the full test suite yet but at least all the compile-fail tests pass. r? @nagisa
|
☀️ Test successful - status-appveyor, status-travis |
this change was merged in rust-lang/rust#50338.
this change was merged in rust-lang/rust#50338.
this change was merged in rust-lang/rust#50338.
…imulacrum stabilize #[panic_handler] closes rust-lang#44489 ### Update(2018-09-07) This was proposed for stabilization in rust-lang#44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in rust-lang#44489 (comment) Documentation PRs: - Reference. rust-lang/reference#362 - Nomicon. rust-lang/nomicon#75 --- `#[panic_implementation]` was implemented recently in rust-lang#50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. rust-lang#44489) but this PR is meant to start a discussion about those issues / questions with the language team. (\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in rust-lang#43054 Some unresolved questions from rust-lang#44489: > Should the Display of PanicInfo format the panic information as "panicked at 'reason', > src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as "reason". The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable. > Is this design compatible, or can it be extended to work, with unwinding implementations for > no-std environments? I believe @whitequark made more progress with unwinding in no-std since their last comment in rust-lang#44489. Perhaps they can give us an update? --- Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation. cc @rust-lang/lang cc @jackpot51 @alevy @phil-opp
…imulacrum stabilize #[panic_handler] closes rust-lang#44489 ### Update(2018-09-07) This was proposed for stabilization in rust-lang#44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in rust-lang#44489 (comment) Documentation PRs: - Reference. rust-lang/reference#362 - Nomicon. rust-lang/nomicon#75 --- `#[panic_implementation]` was implemented recently in rust-lang#50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. rust-lang#44489) but this PR is meant to start a discussion about those issues / questions with the language team. (\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in rust-lang#43054 Some unresolved questions from rust-lang#44489: > Should the Display of PanicInfo format the panic information as "panicked at 'reason', > src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as "reason". The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable. > Is this design compatible, or can it be extended to work, with unwinding implementations for > no-std environments? I believe @whitequark made more progress with unwinding in no-std since their last comment in rust-lang#44489. Perhaps they can give us an update? --- Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation. cc @rust-lang/lang cc @jackpot51 @alevy @phil-opp
stabilize #[panic_handler] closes #44489 ### Update(2018-09-07) This was proposed for stabilization in #44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in #44489 (comment) Documentation PRs: - Reference. rust-lang/reference#362 - Nomicon. rust-lang/nomicon#75 --- `#[panic_implementation]` was implemented recently in #50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. #44489) but this PR is meant to start a discussion about those issues / questions with the language team. (\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in #43054 Some unresolved questions from #44489: > Should the Display of PanicInfo format the panic information as "panicked at 'reason', > src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as "reason". The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable. > Is this design compatible, or can it be extended to work, with unwinding implementations for > no-std environments? I believe @whitequark made more progress with unwinding in no-std since their last comment in #44489. Perhaps they can give us an update? --- Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation. cc @rust-lang/lang cc @jackpot51 @alevy @phil-opp
This implements the
#[panic_implementation]attribute as instructed in #44489 (comment)I haven't run the full test suite yet but at least all the compile-fail tests pass.
r? @nagisa