Progress towards C API conformance (#280)

lionel- · web-flow · commit 11e5e4f6baee · 2025-09-09T18:37:35.000+02:00
* Use `R_newEnv()` if available

* Avoid using `Rf_findVarInFrame3()` and `R_UnboundValue`

* Update snapshots and Rbuildignore

* Update URLs

* Update NEWS and cran-comments

* Update github actions

* Move protection in clause
diff --git a/.Rbuildignore b/.Rbuildignore
@@ -16,3 +16,5 @@
 ^revdep$
 ^CRAN-RELEASE$
 ^CRAN-SUBMISSION$
+^compile_commands.json$
+^.cache$
diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml
@@ -8,9 +8,10 @@ on:
   push:
     branches: [main, master]
   pull_request:
-    branches: [main, master]
 
-name: R-CMD-check
+name: R-CMD-check.yaml
+
+permissions: read-all
 
 jobs:
   R-CMD-check:
@@ -25,24 +26,22 @@ jobs:
           - {os: macos-latest,   r: 'release'}
 
           - {os: windows-latest, r: 'release'}
-          # Use 3.6 to trigger usage of RTools35
-          - {os: windows-latest, r: '3.6'}
-          # use 4.1 to check with rtools40's older compiler
-          - {os: windows-latest, r: '4.1'}
-
-          - {os: ubuntu-latest,   r: 'devel', http-user-agent: 'release'}
-          - {os: ubuntu-latest,   r: 'release'}
-          - {os: ubuntu-latest,   r: 'oldrel-1'}
-          - {os: ubuntu-latest,   r: 'oldrel-2'}
-          - {os: ubuntu-latest,   r: 'oldrel-3'}
-          - {os: ubuntu-latest,   r: 'oldrel-4'}
+          # use 4.0 or 4.1 to check with rtools40's older compiler
+          - {os: windows-latest, r: 'oldrel-4'}
+
+          - {os: ubuntu-latest,  r: 'devel', http-user-agent: 'release'}
+          - {os: ubuntu-latest,  r: 'release'}
+          - {os: ubuntu-latest,  r: 'oldrel-1'}
+          - {os: ubuntu-latest,  r: 'oldrel-2'}
+          - {os: ubuntu-latest,  r: 'oldrel-3'}
+          - {os: ubuntu-latest,  r: 'oldrel-4'}
 
     env:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
       R_KEEP_PKG_SOURCE: yes
 
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
 
       - uses: r-lib/actions/setup-pandoc@v2
 
@@ -60,3 +59,4 @@ jobs:
       - uses: r-lib/actions/check-r-package@v2
         with:
           upload-snapshots: true
+          build_args: 'c("--no-manual","--compact-vignettes=gs+qpdf")'
diff --git a/.github/workflows/pkgdown.yaml b/.github/workflows/pkgdown.yaml
@@ -4,12 +4,13 @@ on:
   push:
     branches: [main, master]
   pull_request:
-    branches: [main, master]
   release:
     types: [published]
   workflow_dispatch:
 
-name: pkgdown
+name: pkgdown.yaml
+
+permissions: read-all
 
 jobs:
   pkgdown:
@@ -19,8 +20,10 @@ jobs:
       group: pkgdown-${{ github.event_name != 'pull_request' || github.run_id }}
     env:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
+    permissions:
+      contents: write
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
 
       - uses: r-lib/actions/setup-pandoc@v2
 
@@ -39,7 +42,7 @@ jobs:
 
       - name: Deploy to GitHub pages 🚀
         if: github.event_name != 'pull_request'
-        uses: JamesIves/github-pages-deploy-action@v4.4.1
+        uses: JamesIves/github-pages-deploy-action@v4.5.0
         with:
           clean: false
           branch: gh-pages
diff --git a/.github/workflows/pr-commands.yaml b/.github/workflows/pr-commands.yaml
@@ -4,7 +4,9 @@ on:
   issue_comment:
     types: [created]
 
-name: Commands
+name: pr-commands.yaml
+
+permissions: read-all
 
 jobs:
   document:
@@ -13,8 +15,10 @@ jobs:
     runs-on: ubuntu-latest
     env:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
+    permissions:
+      contents: write
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
 
       - uses: r-lib/actions/pr-fetch@v2
         with:
@@ -50,8 +54,10 @@ jobs:
     runs-on: ubuntu-latest
     env:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
+    permissions:
+      contents: write
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
 
       - uses: r-lib/actions/pr-fetch@v2
         with:
diff --git a/.github/workflows/test-coverage.yaml b/.github/workflows/test-coverage.yaml
@@ -4,9 +4,10 @@ on:
   push:
     branches: [main, master]
   pull_request:
-    branches: [main, master]
 
-name: test-coverage
+name: test-coverage.yaml
+
+permissions: read-all
 
 jobs:
   test-coverage:
@@ -15,36 +16,47 @@ jobs:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
 
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
 
       - uses: r-lib/actions/setup-r@v2
         with:
           use-public-rspm: true
 
       - uses: r-lib/actions/setup-r-dependencies@v2
         with:
-          extra-packages: any::covr
+          extra-packages: any::covr, any::xml2
           needs: coverage
 
       - name: Test coverage
         run: |
-          covr::codecov(
+          cov <- covr::package_coverage(
             quiet = FALSE,
             clean = FALSE,
-            install_path = file.path(Sys.getenv("RUNNER_TEMP"), "package")
+            install_path = file.path(normalizePath(Sys.getenv("RUNNER_TEMP"), winslash = "/"), "package")
           )
+          print(cov)
+          covr::to_cobertura(cov)
         shell: Rscript {0}
 
+      - uses: codecov/codecov-action@v5
+        with:
+          # Fail if error if not on PR, or if on PR and token is given
+          fail_ci_if_error: ${{ github.event_name != 'pull_request' || secrets.CODECOV_TOKEN }}
+          files: ./cobertura.xml
+          plugins: noop
+          disable_search: true
+          token: ${{ secrets.CODECOV_TOKEN }}
+
       - name: Show testthat output
         if: always()
         run: |
           ## --------------------------------------------------------------------
-          find ${{ runner.temp }}/package -name 'testthat.Rout*' -exec cat '{}' \; || true
+          find '${{ runner.temp }}/package' -name 'testthat.Rout*' -exec cat '{}' \; || true
         shell: bash
 
       - name: Upload test results
         if: failure()
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: coverage-test-failures
           path: ${{ runner.temp }}/package
diff --git a/NEWS.md b/NEWS.md
@@ -1,5 +1,8 @@
 # magrittr (development version)
 
+* Fixes for CRAN checks.
+
+
 # magrittr 2.0.3
 
 * Fixed a C level protection issue in `%>%` (#256).
diff --git a/README.Rmd b/README.Rmd
@@ -13,12 +13,12 @@ knitr::opts_chunk$set(
 library(magrittr)
 ```
 
-# magrittr <a href='https:/magrittr.tidyverse.org'><img src='man/figures/logo.png' align="right" height="139" /></a>
+# magrittr <a href='https://magrittr.tidyverse.org'><img src='man/figures/logo.png' align="right" height="139" /></a>
 
 <!-- badges: start -->
 [![CRAN status](https://www.r-pkg.org/badges/version/magrittr)](https://cran.r-project.org/package=magrittr)
 [![Codecov test coverage](https://codecov.io/gh/tidyverse/magrittr/branch/main/graph/badge.svg)](https://app.codecov.io/gh/tidyverse/magrittr?branch=main)
-[![R-CMD-check](https://github.com/smbache/magrittr/actions/workflows/R-CMD-check.yaml/badge.svg)](https://github.com/smbache/magrittr/actions/workflows/R-CMD-check.yaml)
+[![R-CMD-check](https://github.com/tidyverse/magrittr/actions/workflows/R-CMD-check.yaml/badge.svg)](https://github.com/tidyverse/magrittr/actions/workflows/R-CMD-check.yaml)
 <!-- badges: end -->
 
 ## Overview
diff --git a/README.md b/README.md
@@ -1,15 +1,15 @@
 
 <!-- README.md is generated from README.Rmd. Please edit that file -->
 
-# magrittr <a href='https:/magrittr.tidyverse.org'><img src='man/figures/logo.png' align="right" height="139" /></a>
+# magrittr <a href='https://magrittr.tidyverse.org'><img src='man/figures/logo.png' align="right" height="139" /></a>
 
 <!-- badges: start -->
 
 [![CRAN
 status](https://www.r-pkg.org/badges/version/magrittr)](https://cran.r-project.org/package=magrittr)
 [![Codecov test
 coverage](https://codecov.io/gh/tidyverse/magrittr/branch/main/graph/badge.svg)](https://app.codecov.io/gh/tidyverse/magrittr?branch=main)
-[![R-CMD-check](https://github.com/smbache/magrittr/actions/workflows/R-CMD-check.yaml/badge.svg)](https://github.com/smbache/magrittr/actions/workflows/R-CMD-check.yaml)
+[![R-CMD-check](https://github.com/tidyverse/magrittr/actions/workflows/R-CMD-check.yaml/badge.svg)](https://github.com/tidyverse/magrittr/actions/workflows/R-CMD-check.yaml)
 <!-- badges: end -->
 
 ## Overview
diff --git a/cran-comments.md b/cran-comments.md
@@ -1,3 +1,7 @@
+Fixes part of non-conformant C API calls.
+
+See https://github.com/tidyverse/magrittr/issues/279 for progress, the rest needs some ongoing updates to r-devel.
+
 ## R CMD check results
 
 There were no ERRORs, WARNINGs, or NOTEs.
diff --git a/src/pipe.c b/src/pipe.c
@@ -74,7 +74,7 @@ SEXP magrittr_pipe(SEXP call, SEXP op, SEXP args, SEXP rho) {
   SEXP env = PROTECT(Rf_eval(syms_env, rho));
 
   SEXP pipe_sym = r_env_get(rho, syms_sym);
-  if (pipe_sym == R_UnboundValue) {
+  if (pipe_sym == r_unbound_sym) {
     pipe_sym = syms_pipe;
   }
   PROTECT(pipe_sym);
@@ -90,15 +90,15 @@ SEXP magrittr_pipe(SEXP call, SEXP op, SEXP args, SEXP rho) {
     return lambda;
   }
 
-  bool use_nested = r_env_get(rho, syms_nested) != R_UnboundValue;
+  bool use_nested = r_env_get(rho, syms_nested) != r_unbound_sym;
   if (use_nested) {
     SEXP call = PROTECT(pipe_nest(exprs));
     SEXP out = Rf_eval(call, env);
     UNPROTECT(7);
     return out;
   }
 
-  bool use_lazy = r_env_get(rho, syms_lazy) != R_UnboundValue;
+  bool use_lazy = r_env_get(rho, syms_lazy) != r_unbound_sym;
   SEXP out = R_NilValue;
 
   if (use_lazy) {
@@ -198,7 +198,7 @@ static
 void clean_pipe(void* data) {
   struct cleanup_info* info = (struct cleanup_info*) data;
 
-  if (info->old == R_UnboundValue) {
+  if (info->old == r_unbound_sym) {
     r_env_unbind(info->env, syms_dot);
   } else {
     Rf_defineVar(syms_dot, info->old, info->env);
diff --git a/src/utils.c b/src/utils.c
@@ -2,6 +2,7 @@
 #include <Rinternals.h>
 
 
+SEXP r_unbound_sym = NULL;
 SEXP syms_delayed_assign = NULL;
 
 void r_env_bind_lazy(SEXP env,
@@ -114,6 +115,7 @@ SEXP r_new_environment(SEXP parent, R_len_t size) {
 
 
 void magrittr_init_utils(SEXP ns) {
+  r_unbound_sym = Rf_install(".__magrittr_unbound_value__.");
   syms_delayed_assign = Rf_install("delayedAssign");
   syms_envir = Rf_install("envir");
   syms_inherits = Rf_install("inherits");
diff --git a/src/utils.h b/src/utils.h
@@ -2,20 +2,23 @@
 #define MAGRITTR_UTILS_H
 
 
+#include <Rinternals.h>
 #include <Rversion.h>
 
+extern SEXP r_unbound_sym;
+
 void r_env_bind_lazy(SEXP env,
                      SEXP sym,
                      SEXP expr,
                      SEXP eval_env);
 
 static inline
 void r_env_unbind(SEXP env, SEXP sym) {
-#if (R_VERSION < R_Version(4, 0, 0))
+#if (R_VERSION >= R_Version(4, 0, 0))
+  R_removeVarFromFrame(sym, env);
+#else
   void r__env_unbind(SEXP, SEXP);
   r__env_unbind(env, sym);
-#else
-  R_removeVarFromFrame(sym, env);
 #endif
 }
 
@@ -24,21 +27,33 @@ SEXP r_parse_eval(const char* str, SEXP env);
 
 static inline
 SEXP r_new_environment(SEXP parent) {
+#if (R_VERSION >= R_Version(4, 1, 0))
+  return R_NewEnv(parent, FALSE, 0);
+#else
   SEXP env = Rf_allocSExp(ENVSXP);
   SET_ENCLOS(env, parent);
   return env;
+#endif
 }
 
 static inline
 SEXP r_env_get(SEXP env, SEXP sym) {
-  SEXP obj = PROTECT(Rf_findVarInFrame3(env, sym, FALSE));
+#if (R_VERSION >= R_Version(4, 5, 0))
+  SEXP obj = R_getVarEx(sym, env, FALSE, r_unbound_sym);
+#else
+  SEXP obj = Rf_findVarInFrame3(env, sym, FALSE);
+  if (obj == R_UnboundValue) {
+    obj = r_unbound_sym;
+  }
+#endif
 
   // Force lazy loaded bindings
   if (TYPEOF(obj) == PROMSXP) {
+    PROTECT(obj);
     obj = Rf_eval(obj, R_BaseEnv);
+    UNPROTECT(1);
   }
 
-  UNPROTECT(1);
   return obj;
 }
 
diff --git a/tests/testthat/_snaps/pipe.md b/tests/testthat/_snaps/pipe.md
@@ -2,13 +2,13 @@
 
     Code
       NULL %>% f() %>% g() %>% h()
-    Message <simpleMessage>
+    Message
       baz
       bar
       foo
     Code
       NULL %!>% f() %!>% g() %!>% h()
-    Message <simpleMessage>
+    Message
       foo
       bar
       baz
