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

Capture String Panic Messages #962

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

George-lewis
Copy link

@George-lewis George-lewis commented Feb 10, 2025

📬 Issue #, if available:

N/A

✍️ Description of changes:

This is something I noticed from reading the code, but not a behavior that I have directly observed.

The panic layer contains the following code for extracting a message from a panic:

fn build_panic_diagnostic(err: &Box<dyn Any + Send>) -> Diagnostic {
    let error_message = if let Some(msg) = err.downcast_ref::<&str>() {
        format!("Lambda panicked: {msg}")
    } else {
        "Lambda panicked".to_string()
    };
    Diagnostic {
        error_type: type_name_of_val(err),
        error_message,
    }
}

I believe that this will lose the message when the panic payload is not an &str.

Consider the following example:

use std::{any::Any, hint, panic::{self, panic_any}};

fn main() {
    panic::set_hook(Box::new(|_| {}));

    // String
    // note: without black_box Rust will optimize the String into an &str :)
    let exc = panic::catch_unwind(|| panic!("{x}", x = hint::black_box("fmt"))).unwrap_err();
    handle_panic(&exc);

    // &'static str
    let exc = panic::catch_unwind(|| panic!("static")).unwrap_err();
    handle_panic(&exc);

    // you can panic with anything
    struct MyType;
    let exc = panic::catch_unwind(|| panic_any(MyType)).unwrap_err();
    handle_panic(&exc);
}

fn handle_panic(payload: &Box<dyn Any + Send>) {
    if let Some(inner) = payload.downcast_ref::<&str>() {
        println!("&str: {}", inner);
    } else if let Some(inner) = payload.downcast_ref::<String>() {
        println!("String: {}", inner);
    } else {
        println!("Not a string");
    }
}

This prints:

String: fmt 0
&str: static
Not a string

If this were to happen in lambda code I believe that only the second case would be correctly handled by the panic layer.

In the most typical case of panic!("message"), the payload will be an &'static str, but if formatting is used you will get a String instead, which can not be extracted via .downcast_ref<&str>(). Moreover, the panic payload can be any arbitrary type conforming to 'static + Any + Send, via panic_any(), although this is rare and there isn't much that can be done in this case anyway.

By adding an additional branch to downcast to String, we ensure that we capture the error message in most cases. This also better aligns us with the standard library's implementation.

🔏 By submitting this pull request

  • I confirm that I've ran cargo +nightly fmt.
  • I confirm that I've ran cargo clippy --fix.
  • I confirm that I've made a best effort attempt to update all relevant documentation.
  • I confirm that my contribution is made under the terms of the Apache 2.0 license.

@George-lewis George-lewis marked this pull request as ready for review February 10, 2025 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant