Mask definition of Rf_error to avoid longjmp issues
Closes #1247. It turns out we are generating a call to Rf_error in RcppExports.cpp for C++ interfaces. I think that code path is obsolete, and it should never run with unwind-protect, but I prefer to keep this PR safe and simple. So to be able to undef, call Rf_error then define again, I put the definition into a separate file without guards, so that it can be included more than once. Please let me know if there's a better way and/or you'd like this definition in another place.
Checklist
- [x] Code compiles correctly
- [x]
R CMD checkstill passes all tests - [ ] Preferably, new tests were added which fail without the change
- [x] Document the changes by file in ChangeLog
Thanks for putting this together!
I can (and will) turn on the reverse-depends machinery but as that does not generally involve compilerAttributes() etc it will not be complete. But if we run locally with this any warts should become apparent before the January release.
Ok, new solution with a bit of macro trickery.
Wheeee ==:-) Almost worse :stuck_out_tongue_winking_eye: That should work. :crossed_fingers:
Mmmh, maybe I should put that into an ifdef...
Ok, it should be ready now.
Thanks, let's wait for another pair of eyes and the revdep machine then. Meanwhile, I'll investigate how many packages need to be adapted to avoid this warning.
Should we include a reference to RCPP_NO_MASK_RF_ERROR in the warning message? There are probably (not many but) some fair use cases like this out there.
Thumbs up on more informative message. 'That file' is technically a policy violation anyway (via a trick borrowed, if memory serves, from another posit package) and not something I get paid to care about anymore.
What about something like... "Use of Rf_error() replaced with Rcpp::stop(). Calls to Rf_error() in C++ contexts are unsafe: consider using Rcpp::stop() instead, or define RCPP_NO_MASK_RF_ERROR if this is a false positive." A bit long, but...
Ok, some first results are in this log file. It looks worse than it likely is as a) there are the usual 'new' packages for which I have to add their dependencies in a follow-up run and b) the packages already on 'deadline' with known-to-CRAN issues not from us. And then there is the remainder. I will take a closer look at those but it may take me another day or so. I have logs here to follow-up with.
Hm. And I mean Hmm here. An incremental run (after adding all the packages listed as missing, and running against everything not passing in first one, i.e. those who were skipped, who failed, or are new) still has a lot of failures:
Test of Rcpp 1.1.0.5 had 29 successes, 38 failures, and 48 skipped packages.
Ran from 2025-11-01 10:05:16.62 to 2025-11-01 11:17:28.20 for 1.203 hours
Average of 37.666 secs relative to 203.614 secs using 6 runners
Failed packages: bayesAB, BayesPower, BayesProject, benchr, BGVAR, coga, comat, cpr, dqrng, empichar, FIESTAutils, gapfill, GeneralizedWendland, ggiraph, gppm, IOHanalyzer, itp, locStra, LOMAR, mapi, meteoland\
, mmrm, mwcsr, o2plsda, ravetools, RcppSimdJson, rrum, RTMB, rucrdtw, s2, SAM, sf, simcdm, TDA, tidynorm, updog, VIC5, windfarmGA
Skipped packages: abn, bayeslist, bigrquerystorage, blavaan, bmggum, bmstdr, cbq, chouca, clrng, Crossover, DataVisualizations, dipsaus, disk.frame, EcoEnsemble, FlexReg, geocodebr, gpuR, HiCociety, highs, joi\
nXL, KeyboardSimulator, MatchIt, mlpack, move, multinma, networkscaleup, OpenMx, pcFactorStan, pema, ProbBreed, ProFAST, profoc, psBayesborrow, RavenR, RcppPlanc, Rfast, Rlgt, rlibkriging, rliger, rmBayes, rsta\
narm, rts2, SelfControlledCaseSeries, sparkwarc, spatialGE, starvz, tiledb, warbleR
I looked at two: dqrng and my own RcppSimdJson die on 'Aborted'. That ... does not seem like what we want, is it?
dqrng
* checking tests ... [25s/39s] ERROR
Running ‘testthat.R’ [25s/38s]
Running the tests in ‘tests/testthat.R’ failed.
Complete output:
> library(testthat)
> library(dqrng)
>
> test_check("dqrng")
terminate called after throwing an instance of 'Rcpp::exception'
what(): Error: 'min' must not be larger than 'max'!
Aborted
* checking for unstated dependencies in vignettes ... OK
* checking package vignettes ... OK
RcppSimdJson
test_fparse_fload.R........... 8 tests ^[[0;32mOK^[[0m
test_fparse_fload.R........... 8 tests ^[[0;32mOK^[[0m terminate called after throwing an instance of 'Rcpp::exception'
what(): TAPE_ERROR: The JSON document has an improper structure: missing or superfluous commas, braces, missing keys, etc. This is a fatal and unrecoverable error.
Aborted
* DONE
Status: 1 ERROR
Results table log is here.
Mind you maybe it is just like @Enchufa2 hinted: a simple compileAttributes() and we're good. That worked here:
edd@paul:~/git/rcppsimdjson(master)$ tt.r -f inst/tinytest/test_fparse_fload.R
test_fparse_fload.R........... 8 tests OK terminate called after throwing an instance of 'Rcpp::exception'
what(): TAPE_ERROR: The JSON document has an improper structure: missing or superfluous commas, braces, missing keys, etc. This is a fatal and unrecoverable error.
Aborted (core dumped)
edd@paul:~/git/rcppsimdjson(master)$ compAttr.r
edd@paul:~/git/rcppsimdjson(master)$ install.r
* installing *source* package found in current working directory ...
* installing *source* package ‘RcppSimdJson’ ...
** this is package ‘RcppSimdJson’ version ‘0.1.14’
** using staged installation
** libs
using C++ compiler: ‘g++-15 (Ubuntu 15-20250404-0ubuntu1) 15.0.1 20250404 (experimental) [master r15-9193-g08e803aa9be]’
using C++17
ccache g++-15 -std=gnu++17 -I"/usr/share/R/include" -DNDEBUG -I'/usr/local/lib/R/site-library/Rcpp/include' -DSIMDJSON_NO_COMPUTED_GOTO -I../inst/include -fopenmp -fpic -O3 -Wall -pipe -pedantic -Wno-parentheses -Wno-ignored-attributes -Wno-unused-function -c RcppExports.cpp -o RcppExports.o
ccache g++-15 -std=gnu++17 -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -flto=auto -ffat-lto-objects -Wl,-z,relro -o RcppSimdJson.so RcppExports.o deserialize.o exported-utils.o internal-utils.o rcppsimdjson_utils_check.o simdjson_example.o -fopenmp -L/usr/lib/R/lib -lR
installing to /usr/local/lib/R/site-library/00LOCK-rcppsimdjson/00new/RcppSimdJson/libs
** R
** inst
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded from temporary location
** checking absolute paths in shared objects and dynamic libraries
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (RcppSimdJson)
edd@paul:~/git/rcppsimdjson(master)$ tt.r -f inst/tinytest/test_fparse_fload.R
test_fparse_fload.R........... 2666 tests OK 4.2s
All ok, 2666 results (4.2s)
edd@paul:~/git/rcppsimdjson(master)$
PS And no side effects. After re-installing CRAN Rcpp, and reinstalling RcppSimdJson under it (and with its updated src/RcppExports.cpp) we still pass. Good!
Rebased, and ran another incremental run. Results summary in this commit: https://github.com/RcppCore/rcpp-logs/commit/ef3a42fa768e0b780be72a28e36841e353f1314b containing the summary file.
This is a bit more involved than usual:
- a fair number clearly fall into the
compileAttributes()re-run will fix this so simple PR needed bucket - several packages fail to compile RcppEigen headers (!!), this is new and maybe gcc 15 related (but likely local to the old / not overly resourced machine -- seems to work here locally under equivalent docker on newer hardware)
- two or three actually trip over the macro here but I suspect a proper NO_REMAP / header reorder could fix, also PR candidates
- one or two unclear ones remaining
We probably need a new issue to triage. May revisit tomorrow.
Ok -- we are in fact having issues here. The RcppEigen fail is not local to the machine, I just reproduced it in a container locally here for package BayesProject (first on the list above). Worse, even after a run of compileAttributes() we still have an error. So this needs more work, sadly.
Edit: Oh boy that one was work. I got BayesProject to build by adding inst/include/BayesProject.h containing
#include <RcppEigen.h>
using namespace Rcpp;
using namespace RcppEigen;
using namespace Eigen;
and re-running compileAttributes(), of course. Needless to say we are not exactly fans of flattened namespaces ...
See the next paragraph inside the 'details' though as running compileAttributes() is not needed / a more minimal patch can be had.
Edit 2: For completeness the very short diff for BayesProject is below.
This is an updated diff. As @Enchufa2 pointed out we really only need to remove the R.h include. My earlier diff also ran compileAttributes() which is not needed.
root@c95b60d71856:/tmp/checks/BayesProject# diff -ru
data/ DESCRIPTION man/ MD5 NAMESPACE R/ src/
root@c95b60d71856:/tmp/checks/BayesProject# cd ..
root@c95b60d71856:/tmp/checks# diff -ru BayesProject.orig/ BayesProject
diff -ru BayesProject.orig/src/bayes.cpp BayesProject/src/bayes.cpp
--- BayesProject.orig/src/bayes.cpp 2020-06-01 00:45:46.000000000 +0000
+++ BayesProject/src/bayes.cpp 2025-11-05 18:58:16.780202588 +0000
@@ -1,4 +1,3 @@
-#include <R.h>
#include <Rcpp.h>
using namespace Rcpp;
#include <RcppEigen.h>
root@c95b60d71856:/tmp/checks#
The older / longer diff follows.
root@c95b60d71856:/tmp/checks# diff -Nru BayesProject.orig/ BayesProject
diff -Nru BayesProject.orig/inst/include/BayesProject_types.h BayesProject/inst/include/BayesProject_types.h
--- BayesProject.orig/inst/include/BayesProject_types.h 1970-01-01 00:00:00.000000000 +0000
+++ BayesProject/inst/include/BayesProject_types.h 2025-11-05 12:51:23.874653154 +0000
@@ -0,0 +1,6 @@
+
+#include <RcppEigen.h>
+using namespace Rcpp;
+using namespace RcppEigen;
+using namespace Eigen;
+
diff -Nru BayesProject.orig/src/bayes.cpp BayesProject/src/bayes.cpp
--- BayesProject.orig/src/bayes.cpp 2020-06-01 00:45:46.000000000 +0000
+++ BayesProject/src/bayes.cpp 2025-11-05 12:40:49.344415413 +0000
@@ -1,4 +1,4 @@
-#include <R.h>
+/*#include <R.h>*/
#include <Rcpp.h>
using namespace Rcpp;
#include <RcppEigen.h>
diff -Nru BayesProject.orig/src/Makevars BayesProject/src/Makevars
--- BayesProject.orig/src/Makevars 1970-01-01 00:00:00.000000000 +0000
+++ BayesProject/src/Makevars 2025-11-05 12:42:28.491159649 +0000
@@ -0,0 +1 @@
+PKG_CXXFLAGS += -Wno-ignored-attributes
diff -Nru BayesProject.orig/src/RcppExports.cpp BayesProject/src/RcppExports.cpp
--- BayesProject.orig/src/RcppExports.cpp 2020-09-27 00:05:44.000000000 +0000
+++ BayesProject/src/RcppExports.cpp 2025-11-05 12:45:33.389215971 +0000
@@ -1,11 +1,16 @@
// Generated by using Rcpp::compileAttributes() -> do not edit by hand
// Generator token: 10BE3573-1514-4C36-9D1C-5A225CD40393
+#include "../inst/include/BayesProject_types.h"
#include <RcppEigen.h>
#include <Rcpp.h>
using namespace Rcpp;
-using namespace Eigen;
+
+#ifdef RCPP_USE_GLOBAL_ROSTREAM
+Rcpp::Rostream<true>& Rcpp::Rcout = Rcpp::Rcpp_cout_get();
+Rcpp::Rostream<false>& Rcpp::Rcerr = Rcpp::Rcpp_cerr_get();
+#endif
// bayes_vhat
MatrixXd bayes_vhat(MatrixXd x, VectorXd timePoints, double K);
root@c95b60d71856:/tmp/checks#
The src/Makevars here can probably be removed. I just wanted to reduce some line noise from Eigen.
Edit 3: Similar for GeneralizedWendland. Just as above it needs a removal of #include <R.h> before our headers as the above creates trouble when other projects (like Eigen) define something called error().
root@c95b60d71856:/tmp/checks# diff -Nru GeneralizedWendland.orig/ GeneralizedWendland
diff -Nru GeneralizedWendland.orig/inst/doc/GeneralizedWendland.pdf.asis GeneralizedWendland/inst/doc/GeneralizedWendland.pdf.asis
--- GeneralizedWendland.orig/inst/doc/GeneralizedWendland.pdf.asis 2022-06-16 21:46:11.000000000 +0000
+++ GeneralizedWendland/inst/doc/GeneralizedWendland.pdf.asis 1970-01-01 00:00:00.000000000 +0000
@@ -1,2 +0,0 @@
-%\VignetteIndexEntry{Generalized Wendland function}
-%\VignetteEngine{R.rsp::asis}
\ No newline at end of file
diff -Nru GeneralizedWendland.orig/src/Wendland.h GeneralizedWendland/src/Wendland.h
--- GeneralizedWendland.orig/src/Wendland.h 2025-10-15 19:02:57.000000000 +0000
+++ GeneralizedWendland/src/Wendland.h 2025-11-05 13:14:59.923085742 +0000
@@ -2,7 +2,7 @@
#pragma once
#include <limits>
-#include <R.h>
+//#include <R.h>
#include <Rcpp.h>
#include <RcppEigen.h>
#include <boost/math/special_functions/beta.hpp>
root@c95b60d71856:/tmp/checks#
The vignette business can probably be ignored, I am working in a container without texlive and just skip vignettes.
Edit 4: Same thing in locStra, we need to remove a #include <R.h> to not clash over error().
A more minimal diff is
root@c95b60d71856:/tmp/checks# diff -ru locStra.orig/ locStra
diff -ru locStra.orig/src/auxcode.cpp locStra/src/auxcode.cpp
--- locStra.orig/src/auxcode.cpp 2021-01-27 02:29:32.000000000 +0000
+++ locStra/src/auxcode.cpp 2025-11-05 19:04:15.956493072 +0000
@@ -1,4 +1,3 @@
-#include <R.h>
#include <Rcpp.h>
using namespace Rcpp;
#include <RcppEigen.h>
root@c95b60d71856:/tmp/checks#
The initial, longer one follows.
root@c95b60d71856:/tmp/checks# diff -Nru locStra.orig/ locStra
diff -Nru locStra.orig/inst/include/locStra_types.h locStra/inst/include/locStra_types.h
--- locStra.orig/inst/include/locStra_types.h 1970-01-01 00:00:00.000000000 +0000
+++ locStra/inst/include/locStra_types.h 2025-11-05 13:45:56.648854713 +0000
@@ -0,0 +1,3 @@
+
+#include <RcppEigen.h>
+using namespace Eigen;
diff -Nru locStra.orig/src/auxcode.cpp locStra/src/auxcode.cpp
--- locStra.orig/src/auxcode.cpp 2021-01-27 02:29:32.000000000 +0000
+++ locStra/src/auxcode.cpp 2025-11-05 13:44:20.429326159 +0000
@@ -1,4 +1,4 @@
-#include <R.h>
+//#include <R.h>
#include <Rcpp.h>
using namespace Rcpp;
#include <RcppEigen.h>
diff -Nru locStra.orig/src/RcppExports.cpp locStra/src/RcppExports.cpp
--- locStra.orig/src/RcppExports.cpp 2021-01-27 02:46:27.000000000 +0000
+++ locStra/src/RcppExports.cpp 2025-11-05 13:43:34.431117382 +0000
@@ -1,11 +1,16 @@
// Generated by using Rcpp::compileAttributes() -> do not edit by hand
// Generator token: 10BE3573-1514-4C36-9D1C-5A225CD40393
+#include "../inst/include/locStra_types.h"
#include <RcppEigen.h>
#include <Rcpp.h>
using namespace Rcpp;
-using namespace Eigen;
+
+#ifdef RCPP_USE_GLOBAL_ROSTREAM
+Rcpp::Rostream<true>& Rcpp::Rcout = Rcpp::Rcpp_cout_get();
+Rcpp::Rostream<false>& Rcpp::Rcerr = Rcpp::Rcpp_cerr_get();
+#endif
// powerMethodCpp
VectorXd powerMethodCpp(MatrixXd& X, VectorXd& v, double eps, int maxiter);
root@c95b60d71856:/tmp/checks#
Edit 5: Same for SAM
root@c95b60d71856:/tmp/checks# diff -ru SAM.orig/ SAM
diff -ru SAM.orig/src/c_api/grplasso.cpp SAM/src/c_api/grplasso.cpp
--- SAM.orig/src/c_api/grplasso.cpp 2021-06-30 14:16:42.000000000 +0000
+++ SAM/src/c_api/grplasso.cpp 2025-11-05 19:10:27.448205491 +0000
@@ -1,7 +1,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
-#include "R.h"
+//#include "R.h"
#include "math.h"
#include "R_ext/BLAS.h"
#include "R_ext/Lapack.h"
diff -ru SAM.orig/src/c_api/grpLR.cpp SAM/src/c_api/grpLR.cpp
--- SAM.orig/src/c_api/grpLR.cpp 2021-06-30 14:16:42.000000000 +0000
+++ SAM/src/c_api/grpLR.cpp 2025-11-05 19:10:37.431467371 +0000
@@ -1,7 +1,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
-#include "R.h"
+//#include "R.h"
#include "math.h"
#include "R_ext/BLAS.h"
#include "R_ext/Lapack.h"
diff -ru SAM.orig/src/c_api/grpPR.cpp SAM/src/c_api/grpPR.cpp
--- SAM.orig/src/c_api/grpPR.cpp 2021-06-30 14:16:42.000000000 +0000
+++ SAM/src/c_api/grpPR.cpp 2025-11-05 19:10:52.361859087 +0000
@@ -1,7 +1,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
-#include "R.h"
+//#include "R.h"
#include "math.h"
#include "R_ext/BLAS.h"
#include "R_ext/Lapack.h"
diff -ru SAM.orig/src/c_api/grpSVM.cpp SAM/src/c_api/grpSVM.cpp
--- SAM.orig/src/c_api/grpSVM.cpp 2021-06-30 17:13:43.000000000 +0000
+++ SAM/src/c_api/grpSVM.cpp 2025-11-05 19:11:03.818159707 +0000
@@ -1,7 +1,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
-#include "R.h"
+//#include "R.h"
#include "math.h"
#include "R_ext/BLAS.h"
#include "R_ext/Lapack.h"
root@c95b60d71856:/tmp/checks#
Edit 6: Same for TDA
root@c95b60d71856:/tmp/checks# diff -ru TDA.orig/ TDA
Only in TDA.orig/build: vignette.rds
Only in TDA.orig/inst: doc
diff -ru TDA.orig/src/diag.cpp TDA/src/diag.cpp
--- TDA.orig/src/diag.cpp 2024-01-23 16:07:14.000000000 +0000
+++ TDA/src/diag.cpp 2025-11-05 19:16:24.525589991 +0000
@@ -1,6 +1,6 @@
// for R
-#include <R.h>
-#include <R_ext/Print.h>
+//#include <R.h>
+//#include <R_ext/Print.h>
// for Rcpp
#include <Rcpp.h>
root@c95b60d71856:/tmp/checks#
Edit 7: Similar for LOMAR
root@b7af4e9981a0:/work# diff -ru LOMAR.orig/ LOMAR
diff -ru LOMAR.orig/src/diag.cpp LOMAR/src/diag.cpp
--- LOMAR.orig/src/diag.cpp 2022-10-25 09:44:07.000000000 +0000
+++ LOMAR/src/diag.cpp 2025-11-07 22:49:48.638040407 +0000
@@ -1,6 +1,6 @@
// for R
-#include <R.h>
-#include <R_ext/Print.h>
+//#include <R.h>
+//#include <R_ext/Print.h>
// for Rcpp
#include <Rcpp.h>
root@b7af4e9981a0:/work#
I believe this is a bug in RcppEigen. I see:
In file included from /home/iucar/R/x86_64-redhat-linux-gnu-library/4.5/Rcpp/include/RcppCommon.h:194,
from /home/iucar/R/x86_64-redhat-linux-gnu-library/4.5/Rcpp/include/Rcpp.h:27,
from bayes.cpp:2:
/home/iucar/R/x86_64-redhat-linux-gnu-library/4.5/Rcpp/include/Rcpp/macros/mask.h:25:5: warning: Use of Rf_error() replaced with Rcpp::stop(). Calls to Rf_error() in C++ contexts are unsafe: consider using Rcpp::stop() instead, or define RCPP_NO_MASK_RF_ERROR if this is a false positive.
25 | _Pragma("GCC warning \"Use of Rf_error() replaced with Rcpp::stop(). Calls \
| ^~~~~~~
/usr/include/R/R_ext/Error.h:84:15: note: in expansion of macro ‘Rf_error’
84 | #define error Rf_error
| ^~~~~~~~
/home/iucar/R/x86_64-redhat-linux-gnu-library/4.5/RcppEigen/include/Eigen/src/IterativeLinearSolvers/IterativeSolverBase.h:305:14: note: in expansion of macro ‘error’
305 | RealScalar error() const
| ^~~~~
If you protect that error definition with parentheses, as in (error), then BayesProject is fine. But it shouldn't be expanded into Rf_error in the first place (!). This is effectively changing Eigen's API.
I believe this is a bug in RcppEigen.
Yes/no/maybe/unsure/I don't care: It goes away if you do not include R.h and is ultimately caused by Eigen having a function called error(). Other projects and authors do too, I think it is our (and R's) duty to be minimal with global namespace issues. What it really is about is R's Rf_error() being prefered to R's error(). I fundamentally also prefer changesets that do not change the upstream as I do not want to inherit a task of having to change upstream for potentially a very long time. So I much rather re-arrange things in the client package.
A related issue is that we should probably warn when users include R.h. RcppArmadillo does, maybe RcppEigen should as well. Life is easiest if users just include the fewest headers (ie just RcppArmadillo.h or RcppEigen.h).
Ah, ok, I thought it was RcppEigen who was including R.h. Then I agree RcppEigen should warn as RcppArmadillo does. And then it's enough to comment out that include in client packages, it is not necessary to create inst/include/BayesProject_types.h, etc., right?
I am very confused as to why I now need the inst/include/$PACKAGE_types.h when we did not before. Two of the patches I prepared above have it and need it.
It's the same as always: RcppExports.cpp, as created, may use types from included libraries. Here we need a using namespace Eigen; as well in both cases. I do not understand where that slipped in before. We never add it as we (like most programmers) prefer not to flatten namespaces.
Also, to be more precise: RcppArmadillo.h warns (maybe errors even...) when it sees Rcpp.h included (as that can mess with forward declarations). It does not warn about R.h. I am getting to the more radical view that maybe we should warn in Rcpp if R.h was already included?
I am very confused as to why I now need the
inst/include/$PACKAGE_types.hwhen we did not before. Two of the patches I prepared above have it and need it.
Ah, because your are running compileAttributes too, right?, which removes using namespace Eigen from RcppExports.cpp. So they edited that file by hand?
My point was that the package, as is, can be fixed for this PR just by removing the include:
diff --git a/src/bayes.cpp b/src/bayes.cpp
index c856b42..b9e5e65 100644
--- a/src/bayes.cpp
+++ b/src/bayes.cpp
@@ -1,4 +1,3 @@
-#include <R.h>
#include <Rcpp.h>
using namespace Rcpp;
#include <RcppEigen.h>
How the client package deals with flatten namespaces or not it's their problem.
Also, to be more precise:
RcppArmadillo.hwarns (maybe errors even...) when it seesRcpp.hincluded (as that can mess with forward declarations). It does not warn aboutR.h. I am getting to the more radical view that maybe we should warn inRcppifR.hwas already included?
I think we should at least warn now and at some point throw an error.
Thumbs up on more minimal diffs. That may affect more than one package. Needing an inst/include/*h is probably a tell. Will revisit before we file PRs.
Agreed on that adding a warning is probably a good.
Also, to be more precise:
RcppArmadillo.hwarns (maybe errors even...) when it seesRcpp.hincluded
It's been a while. We needed it then for finetune wrappers and forwarding, it's been like this ever since. I think I should add a warning to RcppEigen.
Edit Issue opened at its repo.
I was trying to look at the other packages that are neither a) a simple case of compileAttributes() running to update or b) a simple case of not including R.h. But the first one (cpr) has me stumped. It has all its tests as scripts in tests/*, very old school. It bails on identical(class(x), c("simpleError", "error", "condition")) is not TRUE and of course we don't make changes there. @Enchufa2 can you take a look with fresher eyes?
meteoland is another head scratcher. (Passes with -DRCPP_NO_MASK_RF_ERROR)
mmrm is also trouble. It redefines Rf_error for TMB (I commented that out) and changed to Rcpp::stop() but I still run into abort() from error. Second set of eyes may help. (Passes with -DRCPP_NO_MASK_RF_ERROR)
VIC5 is also trouble as it adds Rf_error() in its logging. (Passes with -DRCPP_NO_MASK_RF_ERROR)
On second thought, maybe these package need the #define to no change behaviour?
Sure, I'll take a look tomorrow.
- cpr: After switching from
Rf_errortoRcpp::stop, the errors are notsimpleErrors anymore: they are Rcpp exceptions. So the tests should check for classerror, which always works, instead ofsimpleError.
diff --git a/src/bsplines.cpp b/src/bsplines.cpp
index b523ebf..dc4b2c1 100644
--- a/src/bsplines.cpp
+++ b/src/bsplines.cpp
@@ -23,7 +23,7 @@ Rcpp::NumericMatrix cpp_bsplines(arma::vec x, arma::vec iknots, arma::vec bknots
Rcpp::NumericMatrix cpp_bsplinesD1(arma::vec x, arma::vec iknots, arma::vec bknots, unsigned int order) {
if ((order - 1) <= 0) {
- Rf_error("(order - 1) <= 0");
+ Rcpp::stop("(order - 1) <= 0");
}
bbasis B0(x, iknots, bknots, order);
@@ -74,7 +74,7 @@ Rcpp::NumericMatrix cpp_bsplinesD1(arma::vec x, arma::vec iknots, arma::vec bkno
Rcpp::NumericMatrix cpp_bsplinesD2(arma::vec x, arma::vec iknots, arma::vec bknots, unsigned int order) {
if ((order - 2) <= 0) {
- Rf_error("(order - 2) <= 0");
+ Rcpp::stop("(order - 2) <= 0");
}
bbasis B0(x, iknots, bknots, order);
diff --git a/src/cpr.cpp b/src/cpr.cpp
index 7f0caa9..c7c13e3 100644
--- a/src/cpr.cpp
+++ b/src/cpr.cpp
@@ -32,7 +32,7 @@ bbasis::bbasis(arma::vec& x_, arma::vec & iknots_, arma::vec & bknots_, unsigned
}
if (!xi.is_sorted()) {
- Rf_error("Knots are not sorted.");
+ Rcpp::stop("Knots are not sorted.");
}
// define xi_star
diff --git a/tests/test-bsplineD.R b/tests/test-bsplineD.R
index 83f84ad..5d1204c 100644
--- a/tests/test-bsplineD.R
+++ b/tests/test-bsplineD.R
@@ -75,15 +75,15 @@ with(e, {
stopifnot(isTRUE( all.equal(cprD2[-100, ], baseD2[-100, ])))
x <- tryCatch(bsplineD(xvec, derivative = 1.5), error = function(e) {e})
- stopifnot(identical(class(x), c("simpleError", "error", "condition")))
+ stopifnot(inherits(x, "error"))
stopifnot(identical(x$message, "Only first and second derivatives are supported"))
x <- tryCatch(bsplineD(xvec, derivative = 3), error = function(e) {e})
- stopifnot(identical(class(x), c("simpleError", "error", "condition")))
+ stopifnot(inherits(x, "error"))
stopifnot(identical(x$message, "Only first and second derivatives are supported"))
x <- tryCatch(bsplineD(xvec, order = 2, derivative = 2), error = function(e) {e})
- stopifnot(identical(class(x), c("simpleError", "error", "condition")))
+ stopifnot(inherits(x, "error"))
stopifnot(identical(x$message, "(order - 2) <= 0"))
})
@@ -97,16 +97,16 @@ with(e, {
bmatD1 <- tryCatch(bsplineD(xvec, bknots = bknots, order = 1, derivative = 1L), error = function(e) e)
bmatD2 <- tryCatch(bsplineD(xvec, bknots = bknots, order = 1, derivative = 2L), error = function(e) e)
- stopifnot(inherits(bmat, "simpleError"))
- stopifnot(inherits(bmatD1, "simpleError"))
- stopifnot(inherits(bmatD2, "simpleError"))
+ stopifnot(inherits(bmat, "error"))
+ stopifnot(inherits(bmatD1, "error"))
+ stopifnot(inherits(bmatD2, "error"))
stopifnot(identical(bmat$message, "order needs to be an integer value >= 2."))
stopifnot(identical(bmatD1$message, "order needs to be an integer value >= 2."))
stopifnot(identical(bmatD2$message, "order needs to be an integer value >= 2."))
bmat <- tryCatch(bsplines(xvec, bknots = bknots, order = 1.9), error = function(e) e)
- stopifnot(inherits(bmat, "simpleError"))
+ stopifnot(inherits(bmat, "error"))
stopifnot(identical(bmat$message, "order needs to be an integer value >= 2."))
bmat <- tryCatch(bsplines(xvec, bknots = bknots, order = 2.9), error = function(e) e)
diff --git a/tests/test-bsplines.R b/tests/test-bsplines.R
index b24a993..bb8f2c1 100644
--- a/tests/test-bsplines.R
+++ b/tests/test-bsplines.R
@@ -68,7 +68,7 @@ with(e, {
bknots = c(-1, 1)
x <- tryCatch(bsplines(xvec, iknots = iknots, bknots = bknots), error = function(e) {e})
- stopifnot(inherits(x, "simpleError"))
+ stopifnot(inherits(x, "error"))
stopifnot(identical(x$message, "Knots are not sorted."))
x <- tryCatch(bsplines(xvec, iknots = sort(iknots), bknots = bknots), error = function(e) {e})
@@ -92,7 +92,7 @@ with(e, {
e <- new.env()
with(e, {
x <- tryCatch(bsplines(list(1:10)), error = function(e) { e })
- stopifnot(inherits(x, "simpleError"))
+ stopifnot(inherits(x, "error"))
stopifnot("error if list passed to bsplines" = identical(x$message, "x is a list. Use btensor instead of bsplines."))
})
@@ -104,7 +104,7 @@ with(e, {
bknots <- c(-1, 1)
x <- tryCatch(bsplines(xvec, df = 2, bknots = bknots), warning = function(w) {w})
- stopifnot(identical(class(x), c("simpleWarning", "warning", "condition")))
+ stopifnot(inherits(x, "warning"))
stopifnot(identical(x$message, "df being set to order"))
x <- suppressWarnings(bsplines(xvec, df = 2, bknots = bknots))
@@ -118,7 +118,7 @@ with(e, {
stopifnot(isTRUE(all.equal(x, z, check.attributes = FALSE)))
x <- tryCatch(bsplines(xvec, iknots = 0.2, df = 6, bknots = bknots), warning = function(w) {w})
- stopifnot(identical(class(x), c("simpleWarning", "warning", "condition")))
+ stopifnot(inherits(x, "warning"))
stopifnot(identical(x$message, "Both iknots and df defined, using iknots"))
x <- suppressWarnings(bsplines(xvec, iknots = 0.2, df = 6, bknots = bknots))
@@ -210,16 +210,16 @@ with(e, {
stopifnot(isTRUE( all.equal(cprD2[-100, ], baseD2[-100, ])))
x <- tryCatch(bsplineD(xvec, derivative = 1.5), error = function(e) {e})
- stopifnot(identical(class(x), c("simpleError", "error", "condition")))
+ stopifnot(inherits(x, "error"))
stopifnot(identical(x$message, "Only first and second derivatives are supported"))
rm(x)
x <- tryCatch(bsplineD(xvec, derivative = 3), error = function(e) {e})
- stopifnot(identical(class(x), c("simpleError", "error", "condition")))
+ stopifnot(inherits(x, "error"))
stopifnot(identical(x$message, "Only first and second derivatives are supported"))
x <- tryCatch(bsplineD(xvec, order = 2, derivative = 2), error = function(e) {e})
- stopifnot(identical(class(x), c("simpleError", "error", "condition")))
+ stopifnot(inherits(x, "error"))
stopifnot(identical(x$message, "(order - 2) <= 0"))
})
@@ -233,16 +233,16 @@ with(e, {
bmatD1 <- tryCatch(bsplineD(xvec, bknots = bknots, order = 1, derivative = 1L), error = function(e) e)
bmatD2 <- tryCatch(bsplineD(xvec, bknots = bknots, order = 1, derivative = 2L), error = function(e) e)
- stopifnot(inherits(bmat, "simpleError"))
- stopifnot(inherits(bmatD1, "simpleError"))
- stopifnot(inherits(bmatD2, "simpleError"))
+ stopifnot(inherits(bmat, "error"))
+ stopifnot(inherits(bmatD1, "error"))
+ stopifnot(inherits(bmatD2, "error"))
stopifnot(identical(bmat$message, "order needs to be an integer value >= 2."))
stopifnot(identical(bmatD1$message, "order needs to be an integer value >= 2."))
stopifnot(identical(bmatD2$message, "order needs to be an integer value >= 2."))
bmat <- tryCatch(bsplines(xvec, bknots = bknots, order = 1.9), error = function(e) e)
- stopifnot(inherits(bmat, "simpleError"))
+ stopifnot(inherits(bmat, "error"))
stopifnot(identical(bmat$message, "order needs to be an integer value >= 2."))
bmat <- tryCatch(bsplines(xvec, bknots = bknots, order = 2.9), error = function(e) e)
diff --git a/tests/test-order_statistics.R b/tests/test-order_statistics.R
index ecd9c3f..58b4e84 100644
--- a/tests/test-order_statistics.R
+++ b/tests/test-order_statistics.R
@@ -76,11 +76,11 @@ e <- new.env()
with(e, {
d <- tryCatch(d_order_statistic(x = x, n = 4:6, j = 2, distribution = "norm")
, error = function(e) e)
- stopifnot(inherits(d, "simpleError"))
+ stopifnot(inherits(d, "error"))
stopifnot(d$message == "length(n) == 1 is not TRUE")
p <- tryCatch(p_order_statistic(q = x, n = 4:6, j = 2, distribution = "norm")
, error = function(e) e)
- stopifnot(inherits(p, "simpleError"))
+ stopifnot(inherits(p, "error"))
stopifnot(p$message == "length(n) == 1 is not TRUE")
})
rm(e)
@@ -89,12 +89,12 @@ e <- new.env()
with(e, {
d <- tryCatch(d_order_statistic(x = x, n = numeric(0), j = 2, distribution = "norm")
, error = function(e) e)
- stopifnot(inherits(d, "simpleError"))
+ stopifnot(inherits(d, "error"))
stopifnot(d$message == "length(n) == 1 is not TRUE")
p <- tryCatch(p_order_statistic(q = x, n = numeric(0), j = 2, distribution = "norm")
, error = function(e) e)
- stopifnot(inherits(p, "simpleError"))
+ stopifnot(inherits(p, "error"))
stopifnot(p$message == "length(n) == 1 is not TRUE")
})
rm(e)
@@ -103,12 +103,12 @@ e <- new.env()
with(e, {
d <- tryCatch(d_order_statistic(x = 0, n = NA_real_, j = 2, distribution = "norm")
, error = function(e) e)
- stopifnot(inherits(d, "simpleError"))
+ stopifnot(inherits(d, "error"))
stopifnot(d$message == "!is.na(n) is not TRUE")
p <- tryCatch(p_order_statistic(q = 0, n = NA_real_, j = 2, distribution = "norm")
, error = function(e) e)
- stopifnot(inherits(p, "simpleError"))
+ stopifnot(inherits(p, "error"))
stopifnot(p$message == "!is.na(n) is not TRUE")
})
rm(e)
@@ -117,12 +117,12 @@ e <- new.env()
with(e, {
d <- tryCatch(d_order_statistic(x = 0, n = 10, j = 11, distribution = "norm")
, error = function(e) e)
- stopifnot(inherits(d, "simpleError"))
+ stopifnot(inherits(d, "error"))
stopifnot(d$message == "n >= stats::na.omit(j) is not TRUE")
p <- tryCatch(p_order_statistic(q = 0, n = 10, j = 11, distribution = "norm")
, error = function(e) e)
- stopifnot(inherits(p, "simpleError"))
+ stopifnot(inherits(p, "error"))
stopifnot(p$message == "n >= stats::na.omit(j) is not TRUE")
})
rm(e)
@@ -131,12 +131,12 @@ e <- new.env()
with(e, {
d <- tryCatch(d_order_statistic(x = 0, n = 10, j = -1, distribution = "norm")
, error = function(e) e)
- stopifnot(inherits(d, "simpleError"))
+ stopifnot(inherits(d, "error"))
stopifnot(d$message == "stats::na.omit(j) >= 1 is not TRUE")
p <- tryCatch(p_order_statistic(q = 0, n = 10, j = -1, distribution = "norm")
, error = function(e) e)
- stopifnot(inherits(p, "simpleError"))
+ stopifnot(inherits(p, "error"))
stopifnot(p$message == "stats::na.omit(j) >= 1 is not TRUE")
})
rm(e)
-
meteoland: Passes after running
compileAttributesfor me. -
mmrm: This one requires
-DRCPP_NO_MASK_RF_ERROR. -
VIC5: This works:
diff --git a/src/vic/vic_run/include/vic_log.h b/src/vic/vic_run/include/vic_log.h
index e64ed44..381c861 100644
--- a/src/vic/vic_run/include/vic_log.h
+++ b/src/vic/vic_run/include/vic_log.h
@@ -66,8 +66,8 @@ void setup_logging(int id, char log_path[], FILE **logfile);
// Print and error functions of R
void Rprintf(const char *, ...);
-void Rf_error(const char *, ...);
-void Rf_warning(const char *, ...);
+void rcpp_error(const char *, ...);
+void rcpp_warning(const char *, ...);
// Debug Level
#if LOG_LVL < 10
@@ -87,14 +87,14 @@ void Rf_warning(const char *, ...);
// Warn Level
#if LOG_LVL < 30
-#define log_warn Rf_warning
+#define log_warn rcpp_warning
#else
#define log_warn(M, ...)
#endif
-#define log_err Rf_error
+#define log_err rcpp_error
-#define check_alloc_status(A, M) if (A == NULL) {Rf_error(M "%s\n", "");}
+#define check_alloc_status(A, M) if (A == NULL) {rcpp_error(M "%s\n", "");}
#endif
diff --git a/src/vic_R.h b/src/vic_R.h
index 00f0236..ff60123 100644
--- a/src/vic_R.h
+++ b/src/vic_R.h
@@ -3,7 +3,6 @@
#include <Rcpp.h>
using namespace Rcpp;
-#undef ERROR /*replace C error with Rcpp*/
extern "C" {
#include <vic_driver_shared_all.h>
diff --git a/src/vic_run_cell.cpp b/src/vic_run_cell.cpp
index a2e4b2d..002c942 100644
--- a/src/vic_run_cell.cpp
+++ b/src/vic_run_cell.cpp
@@ -1,5 +1,22 @@
#include <vic_R.h>
+void rcpp_error(const char* format, ...) {
+ char buf[256];
+ va_list ap;
+ va_start(ap, format);
+ vsnprintf(buf, (size_t) 256, format, ap);
+ va_end(ap);
+ Rcpp::stop("%s", buf);
+}
+void rcpp_warning(const char* format, ...) {
+ char buf[256];
+ va_list ap;
+ va_start(ap, format);
+ vsnprintf(buf, (size_t) 256, format, ap);
+ va_end(ap);
+ Rcpp::warning("%s", buf);
+}
+
// [[Rcpp::export]]
List vic_run_cell(List vic_options, NumericMatrix forcing,
NumericVector soil_par,
Nice work. I clearly wasn' thinking straight in the case of cpr ("of course" it is a different error class now) and VIC5 (nice idea lowercasing).
I think I will set up a new issue with a clean table and also calmly sit down with rev.dep machine and try the 'suspected compileAttributes() does it' set one by one to confirm. From there we can then bulk email / PR, likely with a target of the July release. Sounds good?
Edit: Now happening at #1406
So we have the set of affected packages ready (in #1406) along with a set of changes for each. I am still in the midth of a multi-months upgrade for RcppArmadillo so I am not convinced we could, if we decided to, hammer this through in the roughly eight weeks until early January and our standard release date. So ok to delay / move to the next 'merge window' between that upcoming January 1.1.1 release and the anticipated July 1.1.2 release?
One small technical issue may be how to provide folks with a binary or built rather than just a branch. May have to spin up a repo or org to take advantage of r-universe. Or maybe use one of our personal repos and universes for a branch built? Could be confusing too. (To clarify, the changes we send out should work on any version of Rcpp, but this PR can likely only be merged once the changes have landed. Developers with an affected package wanting to verify may need this branch in the interim.)
I'm ok with delaying this PR to July. In that case, I propose to merge some parts of this PR in a separate one. Particularly, the bits that produce RcppExports.cpp with a call to Rf_error protected with parentheses. In this way, (1) some of them will be already fixed when we merge this PR, and (2) when we send PRs to fix the remaining ones, we ensure that this work is not rolled back.
So, as a summary, my proposal is to
- merge the
Rf_errorprotection in another PR and rebase this one; - start fixing the packages that require anything else than rerunning
compileAttributes; - delay fixes for the ones that requires rerunning
compileAttributesuntil the next release window.
Perfect (and no "chef's kiss" emoji here sadly) I actually had a similar thought of 'early merging' some non-conflicting bits. That is the way. And supporting the upcoming change with the macro protection, but not yet turning the macro on, is good. Can you prepare the split-off PR as proposed above?