GeezerVison is showing github.com — Consider masking definition of Rf_error()? · Issue #1247 · RcppCore/Rcpp
Consider masking definition of Rf_error()? #1247DescriptionSomewhat 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. Activityeddelbuettel commented on Jan 31, 2023 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 commented on Dec 31, 2023 I will try to revisit this issue after the release, given that I got the (long-running) RcppArmadillo transition out of the way. Enchufa2 commented on Oct 8, 2024 About this one, any issue with just defining...
#define Rf_error Rcpp::stop in the proper place? eddelbuettel commented on Oct 8, 2024 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 commented on Oct 8, 2024 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 commented on Oct 8, 2024 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 commented on Oct 8, 2024 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. Reacted by Dirk Eddelbuettel eddelbuettel commented on Oct 8, 2024 Still an ugly-ish define but probably the best we can do. eddelbuettel commented on Oct 9, 2024 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
+
+#endifDemo (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 commented on Oct 9, 2024 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::stopAnd 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 commented on Oct 9, 2024 I like that better too. Let'ss see in a rev.dep how many existing CRAN packages we have to write PRs for. Enchufa2 commented on Oct 9, 2024 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 commented on Oct 9, 2024 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 commented on Oct 9, 2024 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! Reacted by Dirk Eddelbuettel eddelbuettel commented on Oct 9, 2024 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. eddelbuettel commented on Feb 27, 2026 Reacted by Tomasz Kalinowski Enchufa2 commented on Mar 4, 2026 Reacted by Dirk Eddelbuettel Enchufa2 commented on Mar 5, 2026 I emailed the remaining ones with calls in RcppExports.cpp that can be solved by running compileAttributes() with a current version of Rcpp, and updated the repo above. Three cases are yours, @eddelbuettel:
RcppQuantuccia [ fixed on GitHub ]
RcppSpdlog [ fixed on GitHub ][ version 0.0.28 on CRAN ]
RQuantLib [ fixed on GitHub ][ ... regular April release expected... ]
Reacted by Dirk Eddelbuettel
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|