Consider masking definition of Rf_error()?

[kevinushey]

Somewhat related to a recent post on R-devel, some users will still try to use Rf_error() in C++ code using Rcpp. However, (as is documented) C++ destructors won't run when a longjmp occurs in cases like this. For example:

#include <Rcpp.h>
using namespace Rcpp;

struct A {
    A() { Rprintf("A()\n"); }
    ~A() { Rprintf("~A()\n"); }
};

// [[Rcpp::export]]
SEXP uhoh() {
    A a;
    Rf_error("Oops!");
}

/*** R
uhoh()
*/

In this example, ~A() is never printed.

We could consider masking the definition of Rf_error() in these contexts. For example, something like:

#define Rf_error(...) \
    static_assert(false, "Use of Rf_error() in C++ contexts is unwise: consider using Rcpp::stop() instead.");

There might also be a way to provide our own definition of Rf_error() that "masks" the version provided by R, but I wasn't able to find something immediately obvious.

[eddelbuettel]

Tough. It is a clear issue, and one that is a little tricky to explain / document.

I am little burned out from the recent transition dances for Rcpp, BH, and the still-ongoing one for RcppArmadillo. But if you want to drive a test across 2600 reverse depends, and then tangle with all packages that still call Rf_error ...

Is it worth it? Dunno. Should we maybe consider implementing Rf_error() in terms of Rcpp::stop() ?

[eddelbuettel]

I will try to revisit this issue after the release, given that I got the (long-running) RcppArmadillo transition out of the way.

[Enchufa2]

About this one, any issue with just defining...

#define Rf_error Rcpp::stop

in the proper place?

[eddelbuettel]

Yes, though I feel uneasy about only / directly renaming.

Maybe via a wrapper that issues something like a deprecation warning? And/or what @kevinushey initially suggested and dieing in error? Other ideas?

[Enchufa2]

The problem with the initial proposal is that 1) if many packages use this, they will be broken after adding it (admiteddly, they are buggy now, but still we may want to avoid general breakeage), and 2) it doesn't seem to work with ::Rf_error() calls.

Otherwise, in favor of adding a warning via a wrapper on top of renaming.

[eddelbuettel]

I am scrathing my head about a wrapper as these appear to be functions defined in the R header files. The best / only idea I have is to mask that with a #define and that itself is rather ugly. Uggh.

[Enchufa2]

Yes, with a wrapper I meant something like:

#define Rf_error Rcpp::internal::stop_wrapper

where Rcpp::internal::stop_wrapper is a call to Rcpp::warning followed by a call to Rcpp::stop.

[eddelbuettel]

Still an ugly-ish define but probably the best we can do.

[eddelbuettel]

This may be a draft implementation. Wording to be refined

Pending diff

Head:     feature/mask_error Variadic templates refinement (#1336)
Tag:      1.0.13 (14)

Unstaged changes (1)
modified   inst/include/RcppCommon.h
@@ -188,4 +188,6 @@ namespace Rcpp {
 
 #include <Rcpp/internal/wrap.h>
 
+#include <Rcpp/internal/stop_wrapper.h>
+
 #endif

Staged changes (1)
new file   inst/include/Rcpp/internal/stop_wrapper.h
@@ -0,0 +1,16 @@
+
+#ifndef RCPP_INTERNAL_STOP_WRAPPER_H
+#define RCPP_INTERNAL_STOP_WRAPPER_H
+
+namespace Rcpp {
+    namespace internal {
+        inline void stop_wrapper(const char *txt) {
+            Rcpp::warning("(Do not call Rf_error directly, rather call Rcpp::stop");
+            Rcpp::stop(txt);
+        }
+    }
+}
+
+#define Rf_error ::Rcpp::internal::stop_wrapper
+
+#endif

Demo (using @kevinushey's stub from above, unchanged) -- Code

#include <Rcpp.h>
using namespace Rcpp;

struct A {
    A() { Rprintf("A()\n"); }
    ~A() { Rprintf("~A()\n"); }
};

// [[Rcpp::export]]
SEXP uhoh() {
    A a;
    Rf_error("Oops!");
}

/*** R
uhoh()
*/

Result

edd@rob:~/git/rcpp(feature/mask_error)$ Rscript -e 'Rcpp::sourceCpp("/tmp/r/rcpperror.cpp")'

> uhoh()
A()
~A()
Error in eval(ei, envir) : Oops!
Calls: <Anonymous> ... source -> withVisible -> eval -> eval -> uhoh -> .Call
In addition: Warning message:
In uhoh() : (Do not call Rf_error directly, rather call Rcpp::stop
Execution halted
edd@rob:~/git/rcpp(feature/mask_error)$

Misses a closing paren in the warning text which I'll add.

[Enchufa2]

Mmmh, on second thought, the problem with this approach is that we are bugging the user instead of the developer. It should be a compilation warning instead of a runtime one. What about

#define Rf_error _Pragma("GCC warning \"Invalid use of Rf_error, use Rcpp::stop instead\"") \
    Rcpp::stop

And it should works with clang too. This doesn't change Rcpp at all, but only the downstream code that includes a call to Rf_error, generating a compilation warning in the right place, but the resulting code will work fine for the end user.

GCC:

test.cpp:15:13: warning: Invalid use of Rf_error, use Rcpp::stop instead
   15 |     Rf_error("Oops!");
      |             ^~~~~~~~~~                              

Clang:

test.cpp:15:5: warning: Invalid use of Rf_error, use Rcpp::stop instead [-W#pragma-messages]
   15 |     Rf_error("Oops!");
      |     ^

[eddelbuettel]

I like that better too. Let'ss see in a rev.dep how many existing CRAN packages we have to write PRs for.

[Enchufa2]

Please note that there's a test file that requires an undef because it really needs to call Rf_error for the test (otherwise, the test segfaults):

diff --git a/inst/include/RcppCommon.h b/inst/include/RcppCommon.h
index 5cbe895b..9d032a63 100644
--- a/inst/include/RcppCommon.h
+++ b/inst/include/RcppCommon.h
@@ -188,4 +188,7 @@ namespace Rcpp {
 
 #include <Rcpp/internal/wrap.h>
 
+#define Rf_error _Pragma("GCC warning \"Invalid use of Rf_error, use Rcpp::stop instead\"") \
+    Rcpp::stop
+
 #endif
diff --git a/inst/tinytest/cpp/stack.cpp b/inst/tinytest/cpp/stack.cpp
index c3fa4178..ec9a7469 100644
--- a/inst/tinytest/cpp/stack.cpp
+++ b/inst/tinytest/cpp/stack.cpp
@@ -23,6 +23,7 @@
 
 #include <Rcpp.h>
 using namespace Rcpp;
+#undef Rf_error
 
 // Class that indicates to R caller whether C++ stack was unwound
 struct unwindIndicator {

[eddelbuettel]

It's generally better form to protect any #undef with a preceding #ifdef. We kinda 'know' it's defined here but would you mind adding that?

(We probably also want a reference to an entry in the Rcpp FAQ and/or an issue ticket with some context and explanation in the message text.)

[Enchufa2]

I can add the ifdef if you prefer, but note that, according to the C99 and C++11 standards,

A preprocessing directive of the form

# undef identifier new-line

causes the specified identifier no longer to be defined as a macro name. It is ignored if the specified identifier
is not currently defined as a macro name.

:)

EDIT: Even K&R state that!

[eddelbuettel]

Good find. I prefer explicit code hygiene, if in doubt. But now that you mention it I see such 'naked' #undef in R's code too (src/main/*.c) so I rest my case. I may still wrap mine -- it just seems clearer on intent.

[kevinushey]

Nice -- I very much like the idea of using a pragma here. Including a warning could also have other unexpected side effects (e.g. if the function calling Rf_error happened to be called with a condition handler active).

Is it worth considering a way for users to opt-out, in case they really do need to use Rf_error in some specific context?

[Enchufa2]

Is it worth considering a way for users to opt-out, in case they really do need to use Rf_error in some specific context?

#undef is the way out. This of course should be documented.

[eddelbuettel]

We can also have the definition itself inside an #ifndef Rcpp_Leave_Rf_Error_Alone (or equivalent). I think that would be more in line with common practice.