Better model panics #1716
Better model panics #1716
Conversation
There was a problem hiding this comment.
Thank you, @IamTheCarl! Looks good and works great.
I've left two comments with suggestions for potential improvements. None of those are critical, so I'm going to merge this now. But feel free to send a follow-up PR, if you agree with them.
I also made a small edit to your pull request description, to prevent #1569 from being closed when I merge this.
| // Yes, all of this is to add `fj::abi::initialize_panic_handling();` to the top of the function. | ||
| item.block.stmts.insert( | ||
| 0, | ||
| Stmt::Semi( | ||
| Expr::Call(ExprCall { | ||
| attrs: vec![], | ||
| func: Box::new(Expr::Path(ExprPath { | ||
| attrs: vec![], | ||
| qself: None, | ||
| path: syn::Path { | ||
| leading_colon: None, | ||
| segments: { | ||
| let mut segments = Punctuated::new(); | ||
|
|
||
| segments.push(PathSegment { | ||
| ident: Ident::new( | ||
| "fj", | ||
| Span::call_site(), | ||
| ), | ||
| arguments: PathArguments::None, | ||
| }); | ||
|
|
||
| segments.push(PathSegment { | ||
| ident: Ident::new( | ||
| "abi", | ||
| Span::call_site(), | ||
| ), | ||
| arguments: PathArguments::None, | ||
| }); | ||
|
|
||
| segments.push(PathSegment { | ||
| ident: Ident::new( | ||
| "initialize_panic_handling", | ||
| Span::call_site(), | ||
| ), | ||
| arguments: PathArguments::None, | ||
| }); | ||
|
|
||
| segments | ||
| }, | ||
| }, | ||
| })), | ||
| paren_token: Paren { | ||
| span: Span::call_site(), | ||
| }, | ||
| args: Punctuated::new(), | ||
| }), | ||
| syn::token::Semi::default(), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
I'm far from an expert in procedural macros and might be wrong about what's happening here, but couldn't this be done much easier using the quote! macro? See expand.rs for examples.
There was a problem hiding this comment.
I'll give it a try tonight. I've still got my experiments to do with collecting a backtrace after all.
| eprintln!("TOKENS: {}", tokens); | ||
|
|
There was a problem hiding this comment.
It it intentional that this was left in here? Or just leftover debug output?
There was a problem hiding this comment.
Leftover debug output... I thought I got them all...
Mostly
resolvesaddresses #1569 . (edited by @hannobraun: "resolves" is a magic word that closes the issue; GitHub will readily take instructions from random words within the pull request description, but doesn't know what "mostly" means)It currently does not print the stack trace, but it does print the fact that it's a panic and a line number where the panic occurred. Given a little more time I might be able to get the stack trace.
The key is to override the panic hook within the model. This cannot be done externally, so the initialization code is injected into the model function by the
#[fj::model]macro. That new panic hook then places all panics into a global variable for theon_panicfunction to grab. I suspect this could get a bit funky when multiple threads are involved, but the worst I would expect is it giving a "no details provided" message or the wrong error message on a panic.