diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index 4ab6f337..f9b5762c 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.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 diff --git a/.github/workflows/msrv.yaml b/.github/workflows/msrv.yaml index 6bcb0aa4..6586839c 100644 --- a/.github/workflows/msrv.yaml +++ b/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index ef3849d3..054fb5cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,15 @@ ## [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 diff --git a/Cargo.toml b/Cargo.toml index ef835d36..7ed3b2ef 100644 --- a/Cargo.toml +++ b/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" } diff --git a/R-package/tests/testthat/test-panic.R b/R-package/tests/testthat/test-panic.R new file mode 100644 index 00000000..57988804 --- /dev/null +++ b/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()) +# }) diff --git a/savvy-bindgen/src/gen/rust.rs b/savvy-bindgen/src/gen/rust.rs index 0f4b8bdf..973bff93 100644 --- a/savvy-bindgen/src/gen/rust.rs +++ b/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,29 +41,16 @@ 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 { @@ -80,57 +58,74 @@ impl SavvyFn { 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 } diff --git a/src/lib.rs b/src/lib.rs index b151bcdc..ed15b80c 100644 --- a/src/lib.rs +++ b/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; diff --git a/src/panic_hook.rs b/src/panic_hook.rs new file mode 100644 index 00000000..83ec323c --- /dev/null +++ b/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 ``. + .take_while(|line| !line.contains("")) + .collect::>() + .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, + ); +}