Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Mask definition of Rf_error to avoid longjmp issues#1402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
Enchufa2 wants to merge7 commits intomaster
base:master
Choose a base branch
Loading
fromfeature/mask_Rf_error

Conversation

@Enchufa2
Copy link
Member

Closes#1247. It turns out we are generating a call toRf_error inRcppExports.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, callRf_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

  • Code compiles correctly
  • R CMD check still passes all tests
  • Preferably, new tests were added which fail without the change
  • Document the changes by file inChangeLog

eddelbuettel reacted with thumbs up emoji
@eddelbuettel
Copy link
Member

Thanks for putting this together!

I can (and will) turn on the reverse-depends machinery but as that does not generally involvecompilerAttributes() etc it will not be complete. But if we run locally with this any warts should become apparent before the January release.

Copy link
Member

@eddelbuetteleddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This looks pretty good. I only have two "style" questions / comments. Maybe@kevinushey can be the referee :)

@Enchufa2
Copy link
MemberAuthor

Ok, new solution with a bit of macro trickery.

@eddelbuettel
Copy link
Member

Wheeee ==:-) Almost worse 😜 That should work. 🤞

@Enchufa2
Copy link
MemberAuthor

Mmmh, maybe I should put that into an ifdef...

eddelbuettel reacted with thumbs up emoji

@Enchufa2
Copy link
MemberAuthor

Ok, it should be readynow.

Copy link
Member

@eddelbuetteleddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good to me so far.

@Enchufa2
Copy link
MemberAuthor

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.

eddelbuettel reacted with thumbs up emoji

@Enchufa2
Copy link
MemberAuthor

Should we include a reference toRCPP_NO_MASK_RF_ERROR in the warning message? There are probably (not many but) some fair use cases likethis out there.

eddelbuettel reacted with thumbs up emoji

@eddelbuettel
Copy link
Member

eddelbuettel commentedOct 28, 2025•
edited
Loading

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.

@Enchufa2
Copy link
MemberAuthor

Enchufa2 commentedOct 28, 2025•
edited
Loading

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...

eddelbuettel reacted with thumbs up emoji

Copy link
Member

@eddelbuetteleddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nice, thank you both

@eddelbuettel
Copy link
Member

Ok, some first results arein 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.

Enchufa2 reacted with thumbs up emoji

@eddelbuettel
Copy link
Member

eddelbuettel commentedNov 1, 2025•
edited
Loading

Hm. And I meanHmm 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 hoursAverage of 37.666 secs relative to 203.614 secs using 6 runnersFailed 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, windfarmGASkipped 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 ownRcppSimdJson 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* DONEStatus: 1 ERROR

Results table log ishere.

@eddelbuettel
Copy link
Member

eddelbuettel commentedNov 1, 2025•
edited
Loading

Mind you maybe it is just like@Enchufa2 hinted: a simplecompileAttributes() 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 foundin current working directory ...* installing*source* package ‘RcppSimdJson’ ...** this is package ‘RcppSimdJson’ version ‘0.1.14’** using staged installation** libsusing C++ compiler: ‘g++-15 (Ubuntu 15-20250404-0ubuntu1) 15.0.1 20250404 (experimental) [master r15-9193-g08e803aa9be]’using C++17ccache 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.occache 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 -lRinstalling to /usr/local/lib/R/site-library/00LOCK-rcppsimdjson/00new/RcppSimdJson/libs** R** inst** byte-compile and prepare packagefor lazy loading**help*** installinghelp indices** building package indices** testingif installed package can be loaded from temporary location** checking absolute pathsin shared objects and dynamic libraries** testingif installed package can be loaded from final location** testingif 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.2sAll 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 updatedsrc/RcppExports.cpp) we still pass. Good!

Enchufa2 reacted with thumbs up emoji

@eddelbuettel
Copy link
Member

eddelbuettel commentedNov 4, 2025•
edited
Loading

Rebased, and ran another incremental run. Results summary in this commit:RcppCore/rcpp-logs@ef3a42f containing thesummary file.

This is a bit more involved than usual:

  • a fair number clearly fall into thecompileAttributes() 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.

Copy link
Member

@eddelbuetteleddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good catch on the newline

@eddelbuettel
Copy link
Member

eddelbuettel commentedNov 5, 2025•
edited
Loading

Ok -- we are in fact having issues here. TheRcppEigen fail is not local to the machine, I just reproduced it in a container locally here for packageBayesProject (first on the list above). Worse, even after a run ofcompileAttributes() we still have an error. So this needs more work, sadly.

Edit: Oh boy that one was work. I gotBayesProject to build by addinginst/include/BayesProject.h containing

#include<RcppEigen.h>usingnamespaceRcpp;usingnamespaceRcppEigen;usingnamespaceEigen;

and re-runningcompileAttributes(), of course. Needless to say we are not exactly fans of flattened namespaces ...

See the next paragraph inside the 'details' though as runningcompileAttributes() is not needed / a more minimal patch can be had.

Edit 2: For completeness the very short diff forBayesProject is below.

This is an updated diff. As@Enchufa2 pointed out we really only need to remove theR.h include. My earlier diff also rancompileAttributes() 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/ BayesProjectdiff -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/ BayesProjectdiff -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-attributesdiff -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#

Thesrc/Makevars here can probably be removed. I just wanted to reduce some line noise from Eigen.

Edit 3: Similar forGeneralizedWendland. Just as above it needs a removal of#include <R.h> before our headers as the above creates trouble when other projects (likeEigen) define something callederror().

root@c95b60d71856:/tmp/checks# diff -Nru GeneralizedWendland.orig/ GeneralizedWendlanddiff -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 filediff -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 inlocStra, we need to remove a#include <R.h> to not clash overerror().

A more minimal diff is

root@c95b60d71856:/tmp/checks# diff -ru locStra.orig/ locStradiff -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/ locStradiff -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 forSAM

root@c95b60d71856:/tmp/checks# diff -ru SAM.orig/ SAMdiff -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 forTDA

root@c95b60d71856:/tmp/checks# diff -ru TDA.orig/ TDAOnly in TDA.orig/build: vignette.rdsOnly in TDA.orig/inst: docdiff -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/ LOMARdiff -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#

@Enchufa2
Copy link
MemberAuthor

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 thaterror definition with parentheses, as in(error), then BayesProject is fine. But it shouldn't be expanded intoRf_error in the first place (!). This is effectively changing Eigen's API.

@eddelbuettel
Copy link
Member

eddelbuettel commentedNov 5, 2025•
edited
Loading

I believe this is a bug in RcppEigen.

Yes/no/maybe/unsure/I don't care: It goes away if you do not includeR.h and is ultimately caused by Eigen having a function callederror(). 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'sRf_error() being prefered to R'serror(). I fundamentally also prefer changesets that donot 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 includeR.h. RcppArmadillo does, maybe RcppEigen should as well. Life is easiest if users just include the fewest headers (ie justRcppArmadillo.h orRcppEigen.h).

@Enchufa2
Copy link
MemberAuthor

Enchufa2 commentedNov 5, 2025•
edited
Loading

Ah, ok, I thought it was RcppEigen who was includingR.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 createinst/include/BayesProject_types.h, etc., right?

@eddelbuettel
Copy link
Member

eddelbuettel commentedNov 5, 2025•
edited
Loading

I am very confused as to why I now need theinst/include/$PACKAGE_types.h when we did not before. Two of the patches I prepared above have itand need it.

It's the same as always:RcppExports.cpp, as created, may use types from included libraries. Here we need ausing 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 seesRcpp.h included (as that can mess with forward declarations). It does not warn aboutR.h. I am getting to the more radical view thatmaybe we should warn inRcpp ifR.h was already included?

@Enchufa2
Copy link
MemberAuthor

Enchufa2 commentedNov 5, 2025•
edited
Loading

I am very confused as to why I now need theinst/include/$PACKAGE_types.h when we did not before. Two of the patches I prepared above have itand need it.

Ah, because your are runningcompileAttributes too, right?, which removesusing namespace Eigen fromRcppExports.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.cppindex 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'stheir problem.

Also, to be more precise:RcppArmadillo.h warns (maybe errors even...) when it seesRcpp.h included (as that can mess with forward declarations). It does not warn aboutR.h. I am getting to the more radical view thatmaybe we should warn inRcpp ifR.h was already included?

I think we should at least warn now and at some point throw an error.

eddelbuettel reacted with thumbs up emoji

@eddelbuettel
Copy link
Member

Thumbs up on more minimal diffs. That may affect more than one package. Needing aninst/include/*h is probably a tell. Will revisit before we file PRs.

Agreed on that adding a warning is probably a good.

@Enchufa2
Copy link
MemberAuthor

Also, to be more precise:RcppArmadillo.h warns (maybe errors even...) when it seesRcpp.h included

Yes, it does error.

eddelbuettel reacted with thumbs up emoji

@eddelbuettel
Copy link
Member

eddelbuettel commentedNov 5, 2025•
edited
Loading

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.

@eddelbuettel
Copy link
Member

eddelbuettel commentedNov 7, 2025•
edited
Loading

I was trying to look at the other packages that are neither a) a simple case ofcompileAttributes() running to update or b) a simple case of not includingR.h. But the first one (cpr) has me stumped. It has all its tests as scripts intests/*, very old school. It bails onidentical(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 redefinesRf_error for TMB (I commented that out) and changed toRcpp::stop() but I still run intoabort() from error. Second set of eyes may help. (Passes with-DRCPP_NO_MASK_RF_ERROR)

VIC5 is also trouble as it addsRf_error() in its logging. (Passes with-DRCPP_NO_MASK_RF_ERROR)

On second thought, maybe these package need the#define to no change behaviour?

@Enchufa2
Copy link
MemberAuthor

Sure, I'll take a look tomorrow.

eddelbuettel reacted with thumbs up emoji

@Enchufa2
Copy link
MemberAuthor

Enchufa2 commentedNov 9, 2025•
edited
Loading

  • cpr: After switching fromRf_error toRcpp::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.cppindex 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.cppindex 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_stardiff --git a/tests/test-bsplineD.R b/tests/test-bsplineD.Rindex 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.Rindex 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.Rindex 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 runningcompileAttributes for 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.hindex 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", "");}  #endifdiff --git a/src/vic_R.h b/src/vic_R.hindex 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.cppindex 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,
eddelbuettel reacted with thumbs up emoji

@eddelbuettel
Copy link
Member

Nice work. I clearly wasn' thinking straight in the case ofcpr ("of course" it is a different error class now) andVIC5 (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 'suspectedcompileAttributes() 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?

Enchufa2 reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@kevinusheykevinusheykevinushey left review comments

@eddelbuetteleddelbuetteleddelbuettel approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Consider masking definition of Rf_error()?

4 participants

@Enchufa2@eddelbuettel@kevinushey
[8]ページ先頭
©2009-2025 Movatter.jp