Skip to content

Commit

Permalink
better handle panic
Browse files Browse the repository at this point in the history
  • Loading branch information
shrektan committed Sep 19, 2023
1 parent 207a6c5 commit 2cd5dff
Show file tree
Hide file tree
Showing 4 changed files with 3 additions and 56 deletions.
12 changes: 0 additions & 12 deletions R/extendr-wrappers.R
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,5 @@ mday <- function(ref_date) .Call(wrap__mday, ref_date)
#' @export
yday <- function(ref_date) .Call(wrap__yday, ref_date)

#' Set The Panic Hook
#'
#' At the current version of extendr, when panic occurs, the error message won't
#' be redirected to R's stderr stream. This hook will hack and correct the behavior.
#' However, since the R's error throwing can only be changed in the macro level.
#' This hack is not very elegent but at least it's better than before.
#' TODO: Remove this when extendr fixes this issue.
#' @references
#' extendr discussion: https://github.com/extendr/extendr/issues/278#
#' @noRd
set_panic_hook <- function() invisible(.Call(wrap__set_panic_hook))


# nolint end
2 changes: 0 additions & 2 deletions R/zzz.R
Original file line number Diff line number Diff line change
@@ -1,4 +1,2 @@
.onLoad <- function(libname, pkgname) {
# TODO after https://github.com/extendr/extendr/issues/278 resolved, we should remove this
set_panic_hook()
}
39 changes: 1 addition & 38 deletions src/rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ fn rust_ymd(x: Robj) -> Robj {
.map(|i| if i.is_na() { None } else { str2date(i) })
.collect(),
_ => {
panic!("x must be numeric or string vector");
throw_r_error("x must be numeric or string vector");
}
};
value.to_rdate()
Expand Down Expand Up @@ -291,42 +291,6 @@ mod test {
}
}

/// Set The Panic Hook
///
/// At the current version of extendr, when panic occurs, the error message won't
/// be redirected to R's stderr stream. This hook will hack and correct the behavior.
/// However, since the R's error throwing can only be changed in the macro level.
/// This hack is not very elegent but at least it's better than before.
/// TODO: Remove this when extendr fixes this issue.
/// @references
/// extendr discussion: https://github.com/extendr/extendr/issues/278#
/// @noRd
#[extendr]
fn set_panic_hook() {
std::panic::set_hook(Box::new(|panic_info| {
let msg_loc = match panic_info.location() {
Some(location) => format!(
"panic occurred in file '{}' at line {}",
location.file(),
location.line(),
),
None => "".to_string(),
};
let msg_main = match panic_info.payload().downcast_ref::<&str>() {
Some(s) => format!("Rust error msg: {:?}", s),
None => "".to_string(),
};
use std::ffi::CString;
let msg = format!("{}, {:?}\n", msg_main, msg_loc);
if msg.len() > 0 {
let msg: CString = CString::new(msg).unwrap();
unsafe {
libR_sys::REprintf(msg.as_ptr() as *const std::os::raw::c_char);
}
}
}));
}

// Macro to generate exports.
// This ensures exported functions are registered with R.
// See corresponding C code in `entrypoint.c`.
Expand All @@ -344,5 +308,4 @@ extendr_module! {
fn wday;
fn mday;
fn yday;
fn set_panic_hook;
}
6 changes: 2 additions & 4 deletions tests/testthat/test-ymd.R
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ test_that("ymd ... works", {
expect_equal(ymd(210101, 220101), ymd(c(210101, 220101)))
})

test_that("panic hook works", {
expect_error(ymd(list(1)))
out <- capture.output(try(ymd(list(1)), silent = TRUE), type = "message", file = NULL)
expect_match(out, "x must be numeric or string vector")
test_that("panic works", {
expect_error(ymd(list(1)), "x must be numeric or string vector")
})

0 comments on commit 2cd5dff

Please sign in to comment.