Merge pull request #1515 from rstudio/fix/length-error

guard against exception-raising `__bool__` in `length()`

R/RcppExports.R

@@ -254,8 +254,8 @@ py_len_impl <- function(x, defaultValue = NULL) {
     .Call(`_reticulate_py_len_impl`, x, defaultValue)
 }
 
-py_bool_impl <- function(x) {
-    .Call(`_reticulate_py_bool_impl`, x)
+py_bool_impl <- function(x, silent = FALSE) {
+    .Call(`_reticulate_py_bool_impl`, x, silent)
 }
 
 py_has_method <- function(object, name) {

R/python.R

@@ -715,10 +715,16 @@ length.python.builtin.object <- function(x) {
 
   # otherwise, try to invoke the object's __len__ method
   n <- py_len_impl(x, NA_integer_)
-  if (is.na(n))
-    # if the object didn't have a __len__ method, or __len__ raised an
-    # Exception, try instead to invoke its __bool__ method
-    return(as.integer(py_bool_impl(x)))
+
+  # if the object didn't have a __len__() method, or __len__() raised an
+  # Exception, try instead to invoke its __bool__() method.
+  if (is.na(n)) {
+    n <- as.integer(py_bool_impl(x, TRUE))
+    # py_bool_impl( ,TRUE) can also return NA if __bool__() raised an exception.
+    # length() is used extensively in R and must be safe to call, so we don't
+    # want to propagate the Python Exception and signal an R error, but also
+    # don't want to return a false result. We balance concerns by returning NA.
+  }
 
   n
 }

src/RcppExports.cpp

@@ -670,13 +670,14 @@ BEGIN_RCPP
 END_RCPP
 }
 // py_bool_impl
-SEXP py_bool_impl(PyObjectRef x);
-RcppExport SEXP _reticulate_py_bool_impl(SEXP xSEXP) {
+SEXP py_bool_impl(PyObjectRef x, bool silent);
+RcppExport SEXP _reticulate_py_bool_impl(SEXP xSEXP, SEXP silentSEXP) {
 BEGIN_RCPP
     Rcpp::RObject rcpp_result_gen;
     Rcpp::RNGScope rcpp_rngScope_gen;
     Rcpp::traits::input_parameter< PyObjectRef >::type x(xSEXP);
-    rcpp_result_gen = Rcpp::wrap(py_bool_impl(x));
+    Rcpp::traits::input_parameter< bool >::type silent(silentSEXP);
+    rcpp_result_gen = Rcpp::wrap(py_bool_impl(x, silent));
     return rcpp_result_gen;
 END_RCPP
 }
@@ -818,7 +819,7 @@ static const R_CallMethodDef CallEntries[] = {
     {"_reticulate_py_set_interrupt_impl", (DL_FUNC) &_reticulate_py_set_interrupt_impl, 0},
     {"_reticulate_py_list_length", (DL_FUNC) &_reticulate_py_list_length, 1},
     {"_reticulate_py_len_impl", (DL_FUNC) &_reticulate_py_len_impl, 2},
-    {"_reticulate_py_bool_impl", (DL_FUNC) &_reticulate_py_bool_impl, 1},
+    {"_reticulate_py_bool_impl", (DL_FUNC) &_reticulate_py_bool_impl, 2},
     {"_reticulate_py_has_method", (DL_FUNC) &_reticulate_py_has_method, 2},
     {"_reticulate_py_id", (DL_FUNC) &_reticulate_py_id, 1},
     {"_reticulate_py_capsule", (DL_FUNC) &_reticulate_py_capsule, 1},

src/python.cpp

@@ -2558,14 +2558,23 @@ bool py_has_attr_impl(PyObjectRef x, const std::string& name) {
 class PyErrorScopeGuard {
 private:
   PyObject *er_type, *er_value, *er_traceback;
+  bool pending_restore;
 
 public:
   PyErrorScopeGuard() {
     PyErr_Fetch(&er_type, &er_value, &er_traceback);
+    pending_restore = true;
+  }
+
+  void release(bool restore = false) {
+    if (restore)
+      PyErr_Restore(er_type, er_value, er_traceback);
+    pending_restore = false;
   }
 
   ~PyErrorScopeGuard() {
-    PyErr_Restore(er_type, er_value, er_traceback);
+    if (pending_restore)
+      PyErr_Restore(er_type, er_value, er_traceback);
   }
 };
 
@@ -3664,15 +3673,24 @@ SEXP py_len_impl(PyObjectRef x, SEXP defaultValue = R_NilValue) {
 }
 
 // [[Rcpp::export]]
-SEXP py_bool_impl(PyObjectRef x) {
+SEXP py_bool_impl(PyObjectRef x, bool silent = false) {
+  int result;
+  if(silent) {
+    PyErrorScopeGuard _g;
 
-  // evaluate Python `not not x`
-  int result = PyObject_IsTrue(x);
+    // evaluate Python `not not x`
+    result = PyObject_IsTrue(x);
+    // result==-1 should only happen if the object has a
+    // __bool__() method that intentionally raises an exception.
+    if(result == -1)
+      result = NA_LOGICAL;
+
+  } else {
+
+    result = PyObject_IsTrue(x);
+    if(result == -1)
+      throw PythonException(py_fetch_error());
 
-  if (result == -1) {
-  // Should only happen if the object has a `__bool__` method that
-  // intentionally throws an exception.
-    throw PythonException(py_fetch_error());
   }
 
   return Rf_ScalarLogical(result);