Remove usage of Rf_findVarInFrame3() (#386)

* Use `R_existsVarInFrame()` in `exists()`

* Remove usage of `Rf_findVarInFrame3()`

* Rename to `r_env_get()`

* Extract out `r_env_has()`

* Update FAQ vignette

Author: DavisVaughan

NEWS.md

@@ -1,5 +1,17 @@
 # cpp11 (development version)
 
+* The `environment` class no longer uses the non-API function
+  `Rf_findVarInFrame3()` (#367).
+
+  * The `exists()` method now uses the new `R_existsVarInFrame()` function.
+
+  * The `SEXP` conversion operator now uses the new `R_getVar()` function. Note
+    that this is stricter than `Rf_findVarInFrame3()` in 3 ways. The object
+    must exist in the environment (i.e. `R_UnboundValue` is no longer returned),
+    the object cannot be `R_MissingArg`, and if the object was a promise, that
+    promise is now evaluated. We have backported this new strictness to older
+    versions of R as well.
+
 * Fixed an issue with the `writable::matrix` copy constructor where the
   underlying SEXP should have been copied but was not. It is now consistent with
   the behavior of the equivalent `writable::r_vector` copy constructor.

cpp11test/src/test-environment.cpp

@@ -24,14 +24,36 @@ context("environment-C++") {
 
     x.remove("bar");
     expect_true(x.size() == 1);
-    expect_true(x["bar"] == R_UnboundValue);
+    // Object must exist in the environment when we convert to SEXP
+    auto bar_proxy = x["bar"];
+    expect_error_as(static_cast<SEXP>(bar_proxy), cpp11::unwind_exception);
 
     x.remove("foo");
     expect_true(x.size() == 0);
-    expect_true(x["foo"] == R_UnboundValue);
+    // Object must exist in the environment when we convert to SEXP
+    auto foo_proxy = x["foo"];
+    expect_error_as(static_cast<SEXP>(foo_proxy), cpp11::unwind_exception);
 
     expect_true(static_cast<SEXP>(x) == x_sxp);
 
     UNPROTECT(1);
   }
+
+  test_that("environment doesn't leak `R_MissingArg`") {
+    auto new_env = cpp11::package("base")["new.env"];
+    SEXP x_sxp = PROTECT(new_env());
+
+    // Install `R_MissingArg` as a value in the environment
+    Rf_defineVar(Rf_install("missing"), R_MissingArg, x_sxp);
+
+    cpp11::environment x(x_sxp);
+    expect_true(x.size() == 1);
+
+    // Upon conversion to SEXP, we error on `R_MissingArg`.
+    // Base R's `R_getVar()` tries hard not to leak this, so we do too.
+    auto proxy = x["missing"];
+    expect_error_as(static_cast<SEXP>(proxy), cpp11::unwind_exception);
+
+    UNPROTECT(1);
+  }
 }

inst/include/cpp11/R.hpp

@@ -10,6 +10,7 @@
 
 #define R_NO_REMAP
 #define STRICT_R_HEADERS
+#include "R_ext/Boolean.h"
 #include "Rinternals.h"
 #include "Rversion.h"
 
@@ -60,6 +61,53 @@ namespace detail {
 // which can throw warnings with `-Wsign-compare` on Windows.
 inline SEXPTYPE r_typeof(SEXP x) { return static_cast<SEXPTYPE>(TYPEOF(x)); }
 
+/// Get an object from an environment
+///
+/// SAFETY: Keep as a pure C function. Call like an R API function, i.e. wrap in `safe[]`
+/// as required.
+inline SEXP r_env_get(SEXP env, SEXP sym) {
+#if defined(R_VERSION) && R_VERSION >= R_Version(4, 5, 0)
+  const Rboolean inherits = FALSE;
+  return R_getVar(sym, env, inherits);
+#else
+  SEXP out = Rf_findVarInFrame3(env, sym, TRUE);
+
+  // Replicate the 3 checks from `R_getVar()` (along with exact error message):
+  // - Object must be found in the `env`
+  // - `R_MissingArg` can't leak from an `env` anymore
+  // - Promises can't leak from an `env` anymore
+
+  if (out == R_MissingArg) {
+    Rf_errorcall(R_NilValue, "argument \"%s\" is missing, with no default",
+                 CHAR(PRINTNAME(sym)));
+  }
+
+  if (out == R_UnboundValue) {
+    Rf_errorcall(R_NilValue, "object '%s' not found", CHAR(PRINTNAME(sym)));
+  }
+
+  if (r_typeof(out) == PROMSXP) {
+    PROTECT(out);
+    out = Rf_eval(out, env);
+    UNPROTECT(1);
+  }
+
+  return out;
+#endif
+}
+
+/// Check if an object exists in an environment
+///
+/// SAFETY: Keep as a pure C function. Call like an R API function, i.e. wrap in `safe[]`
+/// as required.
+inline bool r_env_has(SEXP env, SEXP sym) {
+#if R_VERSION >= R_Version(4, 2, 0)
+  return R_existsVarInFrame(env, sym);
+#else
+  return Rf_findVarInFrame3(env, sym, FALSE) != R_UnboundValue;
+#endif
+}
+
 }  // namespace detail
 
 template <typename T>

inst/include/cpp11/environment.hpp

@@ -3,7 +3,7 @@
 #include <string>  // for string, basic_string
 
 #include "Rversion.h"         // for R_VERSION, R_Version
-#include "cpp11/R.hpp"        // for SEXP, SEXPREC, Rf_install, Rf_findVarIn...
+#include "cpp11/R.hpp"        // for SEXP, SEXPREC, Rf_install, r_env_get...
 #include "cpp11/as.hpp"       // for as_sexp
 #include "cpp11/protect.hpp"  // for protect, protect::function, safe, unwin...
 #include "cpp11/sexp.hpp"     // for sexp
@@ -34,7 +34,7 @@ class environment {
       safe[Rf_defineVar](name_, as_sexp(value), parent_);
       return *this;
     }
-    operator SEXP() const { return safe[Rf_findVarInFrame3](parent_, name_, TRUE); };
+    operator SEXP() const { return safe[detail::r_env_get](parent_, name_); };
     operator sexp() const { return SEXP(); };
   };
 
@@ -45,12 +45,8 @@ class environment {
   proxy operator[](const char* name) const { return operator[](safe[Rf_install](name)); }
   proxy operator[](const std::string& name) const { return operator[](name.c_str()); }
 
-  bool exists(SEXP name) const {
-    SEXP res = safe[Rf_findVarInFrame3](env_, name, FALSE);
-    return res != R_UnboundValue;
-  }
+  bool exists(SEXP name) const { return safe[detail::r_env_has](env_, name); }
   bool exists(const char* name) const { return exists(safe[Rf_install](name)); }
-
   bool exists(const std::string& name) const { return exists(name.c_str()); }
 
   void remove(SEXP name) {

vignettes/FAQ.Rmd

@@ -153,14 +153,15 @@ cpp11::environment create_environment() {
 #### 9. How do I assign and retrieve values in an environment? What happens if I try to get a value that doesn't exist?
 
 Use `[]` to retrieve or assign values from an environment by name.
-If a value does not exist it will return `R_UnboundValue`.
+If a value does not exist, it will error.
+To check for existence ahead of time, use the `exists()` method.
 
 ```{cpp11}
 #include <cpp11.hpp>
 
 [[cpp11::register]]
 bool foo_exists(cpp11::environment x) {
-  return x["foo"] != R_UnboundValue;
+  return x.exists("foo");
 }
 
 [[cpp11::register]]