Catch panic (#173)

.github/workflows/R-CMD-check.yaml

@@ -11,6 +11,9 @@ name: R-CMD-check
 jobs:
   R-CMD-check:
     runs-on: ${{ matrix.config.os }}
+    defaults:
+      run:
+        shell: bash
 
     name: "${{ matrix.config.os }} (R: ${{ matrix.config.r }}, rust: ${{ matrix.config.rust }})"
 
@@ -51,7 +54,7 @@ jobs:
 
       - name: Run cargo test
         run: |
-          cargo test --manifest-path=./savvy-macro/Cargo.toml
+          # cargo test --manifest-path=./savvy-macro/Cargo.toml
           cargo test --manifest-path=./savvy-bindgen/Cargo.toml
           cargo r-test
 
@@ -83,7 +86,6 @@ jobs:
           mv savvy-macro/ ./R-package/src/savvy/
           mv savvy-bindgen/ ./R-package/src/savvy/
           mv savvy-ffi/ ./R-package/src/savvy/
-        shell: bash
 
       - name: Run cargo test of the R package
         run: cargo test --manifest-path=./R-package/src/rust/Cargo.toml

.github/workflows/msrv.yaml

@@ -13,7 +13,7 @@ jobs:
     steps:
       - uses: actions/checkout@v4
 
-      - uses: dtolnay/rust-toolchain@1.64.0
+      - uses: dtolnay/rust-toolchain@1.65.0
 
       - name: Build
         run: cargo build

CHANGELOG.md

@@ -3,6 +3,15 @@
 <!-- next-header -->
 ## [Unreleased] (ReleaseDate)
 
+### New features
+
+* Now savvy's debug build (when `DEBUG` envvar is set to `true`, i.e.,
+  `devtools::load_all()`), panic doesn't crash R session and shows bactrace.
+  This is useful for investigating what's the cause of the panic.
+
+  Please keep in mind that, in Rust, panic is an **unrecoverable error**. So,
+  not crashing doesn't mean you are saved.
+
 ## [v0.5.1] (2024-04-13)
 
 ### New features

Cargo.toml

@@ -10,7 +10,7 @@ readme = "README.md"
 exclude = ["/book", "/R-package", "README.qmd"]
 
 # Determined by `cargo msrv`
-rust-version = "1.64.0"
+rust-version = "1.65.0"
 
 [dependencies]
 savvy-ffi = { version = "0.5.1", path = "./savvy-ffi" }

R-package/tests/testthat/test-panic.R

@@ -0,0 +1,17 @@
+# test_that("panic doesn't crash R session", {
+#   code <- r"(
+# use savvy::savvy;
+#
+# #[allow(unreachable_code)]
+# #[savvy]
+# fn try_panic() -> savvy::Result<()> {
+#    panic!("safe");
+#    Ok(())
+# }
+# )"
+#
+#   savvy_override <- list(savvy = list(path = file.path(getwd(), "../../../")))
+#   savvy::savvy_source(code, use_cache_dir = TRUE, dependencies = savvy_override)
+#
+#   expect_error(try_panic())
+# })

savvy-bindgen/src/gen/rust.rs

@@ -21,16 +21,7 @@ impl SavvyFn {
 
         let ret_ty = self.return_type.inner();
 
-        // If the original return type is not wrapped with Result, it needs to be wrapped with Ok()
-        let wrapped_with_result = match &self.return_type {
-            SavvyFnReturnType::UserDefinedStruct(UserDefinedStructReturnType {
-                wrapped_with_result,
-                ..
-            }) => *wrapped_with_result,
-            _ => true,
-        };
-
-        let out: syn::ItemFn = match &self.fn_type {
+        let mut out: syn::ItemFn = match &self.fn_type {
             // A bare function
             SavvyFnType::BareFunction => parse_quote!(
                 #(#attrs)*
@@ -50,87 +41,91 @@ impl SavvyFn {
                 } else {
                     None
                 };
-                if wrapped_with_result {
-                    parse_quote!(
-                        #(#attrs)*
-                        #[allow(non_snake_case)]
-                        unsafe fn #fn_name_inner(self__: savvy::ffi::SEXP, #(#args_pat: #args_ty),* ) #ret_ty {
-                            let self__ = <&#mut_token #ty>::try_from(savvy::Sexp(self__))?;
-                            #(#stmts_additional)*
-
-                            self__.#fn_name_orig(#(#args_pat),*)
-                        }
-                    )
-                } else {
-                    parse_quote!(
-                        #(#attrs)*
-                        #[allow(non_snake_case)]
-                        unsafe fn #fn_name_inner(self__: savvy::ffi::SEXP, #(#args_pat: #args_ty),* ) #ret_ty {
-                            let self__ = <&#mut_token #ty>::try_from(savvy::Sexp(self__))?;
-                            #(#stmts_additional)*
-
-                            Ok(self__.#fn_name_orig(#(#args_pat),*))
-                        }
-                    )
-                }
+                parse_quote!(
+                    #(#attrs)*
+                    #[allow(non_snake_case)]
+                    unsafe fn #fn_name_inner(self__: savvy::ffi::SEXP, #(#args_pat: #args_ty),* ) #ret_ty {
+                        let self__ = <&#mut_token #ty>::try_from(savvy::Sexp(self__))?;
+                        #(#stmts_additional)*
+
+                        self__.#fn_name_orig(#(#args_pat),*)
+                    }
+                )
             }
             // A method with self
             SavvyFnType::Method {
                 ty,
                 mutability: _,
                 reference: false,
             } => {
-                if wrapped_with_result {
-                    parse_quote!(
-                        #(#attrs)*
-                        #[allow(non_snake_case)]
-                        unsafe fn #fn_name_inner(self__: savvy::ffi::SEXP, #(#args_pat: #args_ty),* ) #ret_ty {
-                            let self__ = <#ty>::try_from(savvy::Sexp(self__))?;
-                            #(#stmts_additional)*
-
-                            self__.#fn_name_orig(#(#args_pat),*)
-                        }
-                    )
-                } else {
-                    parse_quote!(
-                        #(#attrs)*
-                        #[allow(non_snake_case)]
-                        unsafe fn #fn_name_inner(self__: savvy::ffi::SEXP, #(#args_pat: #args_ty),* ) #ret_ty {
-                            let self__ = <#ty>::try_from(savvy::Sexp(self__))?;
-                            #(#stmts_additional)*
-
-                            Ok(self__.#fn_name_orig(#(#args_pat),*))
-                        }
-                    )
-                }
+                parse_quote!(
+                    #(#attrs)*
+                    #[allow(non_snake_case)]
+                    unsafe fn #fn_name_inner(self__: savvy::ffi::SEXP, #(#args_pat: #args_ty),* ) #ret_ty {
+                        let self__ = <#ty>::try_from(savvy::Sexp(self__))?;
+                        #(#stmts_additional)*
+
+                        self__.#fn_name_orig(#(#args_pat),*)
+                    }
+                )
             }
 
             // An associated function
             SavvyFnType::AssociatedFunction(ty) => {
-                if wrapped_with_result {
-                    parse_quote!(
-                        #(#attrs)*
-                        #[allow(non_snake_case)]
-                        unsafe fn #fn_name_inner(#(#args_pat: #args_ty),* ) #ret_ty {
-                            #(#stmts_additional)*
-
-                            #ty::#fn_name_orig(#(#args_pat),*)
-                        }
-                    )
-                } else {
-                    parse_quote!(
-                        #(#attrs)*
-                        #[allow(non_snake_case)]
-                        unsafe fn #fn_name_inner(#(#args_pat: #args_ty),* ) #ret_ty {
-                            #(#stmts_additional)*
+                parse_quote!(
+                    #(#attrs)*
+                    #[allow(non_snake_case)]
+                    unsafe fn #fn_name_inner(#(#args_pat: #args_ty),* ) #ret_ty {
+                        #(#stmts_additional)*
 
-                            Ok(#ty::#fn_name_orig(#(#args_pat),*))
-                        }
-                    )
-                }
+                        #ty::#fn_name_orig(#(#args_pat),*)
+                    }
+                )
             }
         };
 
+        // If the original return type is not wrapped with Result, it needs to be wrapped with Ok()
+
+        let wrapped_with_result = match &self.return_type {
+            SavvyFnReturnType::UserDefinedStruct(UserDefinedStructReturnType {
+                wrapped_with_result,
+                ..
+            }) => *wrapped_with_result,
+            _ => true,
+        };
+
+        if !wrapped_with_result {
+            let return_expr = out.block.stmts.pop().unwrap();
+            let new_return_expr: syn::Expr = parse_quote!(Ok(#return_expr));
+            out.block.stmts.push(syn::Stmt::Expr(new_return_expr, None));
+        }
+
+        // Wrap with catch_unwind(). This is needed not only for catching panic
+        // gracefully on debug build, but also for preventing the unwinding over
+        // the FFI boundary between Rust and C (= R API). Since the
+        // cross-language unwind is undefined behavior, abort the process here.
+        // This makes the R session crash.
+
+        #[cfg(debug_assertions)]
+        let error_handler: syn::Expr = parse_quote!(Err("panic happened".into()));
+        #[cfg(not(debug_assertions))]
+        let error_handler: syn::Expr = parse_quote!(std::process::abort());
+
+        let orig_body = &mut out.block;
+        let new_body: syn::Block = parse_quote!({
+            let orig_hook = std::panic::take_hook();
+            std::panic::set_hook(Box::new(savvy::panic_hook::panic_hook));
+
+            let result = std::panic::catch_unwind(|| #orig_body);
+
+            std::panic::set_hook(orig_hook);
+
+            match result {
+                Ok(orig_result) => orig_result,
+                Err(_) => #error_handler,
+            }
+        });
+        out.block = Box::new(new_body);
         out
     }
 

src/lib.rs

@@ -56,6 +56,7 @@
 pub mod error;
 pub mod ffi;
 pub mod io;
+pub mod panic_hook;
 pub mod protect;
 pub mod sexp;
 pub mod unwind_protect;

src/panic_hook.rs

@@ -0,0 +1,35 @@
+pub fn panic_hook(panic_info: &std::panic::PanicInfo) {
+    // Forcibly captures a full backtrace regardless of RUST_BACKTRACE
+    let bt = std::backtrace::Backtrace::force_capture().to_string();
+
+    // Since savvy generates many wrappers, the backtrace is boringly deep. Try
+    // cutting the uninteresting part.
+    let bt_short = bt
+        .lines()
+        .skip_while(|line| !line.contains("std::panic::catch_unwind"))
+        // C stacks are not visible from Rust's side and shown as `<unknown>`.
+        .take_while(|line| !line.contains("<unknown>"))
+        .collect::<Vec<&str>>()
+        .join("\n");
+
+    let bt = if bt_short.is_empty() { bt } else { bt_short };
+
+    crate::io::r_eprint(
+        &format!(
+            "panic occured!
+
+Original message:
+{panic_info}
+
+Backtrace:
+
+...snip... (frames of savvy framework)
+
+{bt}
+
+...snip... (frames of R)
+"
+        ),
+        true,
+    );
+}