Stop double protecting writable vectors

DavisVaughan · DavisVaughan · commit d52b790502da · 2024-07-28T13:44:42.000-04:00
And fix memory leak when assigning a `writable::r_vector&lt;T&gt;&amp;&amp;` temporary value to an existing `writable::r_vector&lt;T&gt;`.
diff --git a/cpp11test/src/test-doubles.cpp b/cpp11test/src/test-doubles.cpp
@@ -233,7 +233,11 @@ context("doubles-C++") {
     w = z;
     expect_true(w.size() == 5);
     expect_true(w.data() != z.data());
-    expect_true(w.is_altrep() == z.is_altrep());
+    // Shallow duplication of objects of a very small size (like 1:5) don't result in
+    // a new ALTREP object. Make sure we check ALTREP-ness of the newly duplicated object,
+    // instead of just blindly inheriting the ALTREP-ness of the thing we duplicate.
+    expect_true(w.is_altrep() != z.is_altrep());
+    expect_true(w.is_altrep() == ALTREP(w.data()));
   }
 
   test_that("writable::doubles(SEXP) move assignment") {
diff --git a/cpp11test/src/test-integers.cpp b/cpp11test/src/test-integers.cpp
@@ -223,7 +223,6 @@ context("integers-C++") {
     R_xlen_t after = cpp11::detail::store::count();
 
     expect_true(before == 0);
-    // TODO: This should be 1 but writable vectors are being double protected
-    expect_true(after - before == 2);
+    expect_true(after - before == 1);
   }
 }
diff --git a/cpp11test/src/test-list.cpp b/cpp11test/src/test-list.cpp
@@ -178,7 +178,6 @@ context("list-C++") {
     R_xlen_t after = cpp11::detail::store::count();
 
     expect_true(before == 0);
-    // TODO: This should be 1 but writable vectors are being double protected
-    expect_true(after - before == 2);
+    expect_true(after - before == 1);
   }
 }
diff --git a/inst/include/cpp11/doubles.hpp b/inst/include/cpp11/doubles.hpp
@@ -8,7 +8,7 @@
 #include "cpp11/R.hpp"          // for SEXP, SEXPREC, Rf_allocVector, REAL
 #include "cpp11/as.hpp"         // for as_sexp
 #include "cpp11/named_arg.hpp"  // for named_arg
-#include "cpp11/protect.hpp"    // for SEXP, SEXPREC, REAL_ELT, R_Preserve...
+#include "cpp11/protect.hpp"    // for safe
 #include "cpp11/r_vector.hpp"   // for vector, vector<>::proxy, vector<>::...
 #include "cpp11/sexp.hpp"       // for sexp
 
@@ -84,7 +84,6 @@ template <>
 inline r_vector<double>::r_vector(std::initializer_list<named_arg> il)
     : cpp11::r_vector<double>(safe[Rf_allocVector](REALSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = detail::store::insert(data_);
   int n_protected = 0;
 
   try {
@@ -100,7 +99,6 @@ inline r_vector<double>::r_vector(std::initializer_list<named_arg> il)
       UNPROTECT(n_protected);
     });
   } catch (const unwind_exception& e) {
-    detail::store::release(protect_);
     UNPROTECT(n_protected);
     throw e;
   }
diff --git a/inst/include/cpp11/integers.hpp b/inst/include/cpp11/integers.hpp
@@ -9,7 +9,7 @@
 #include "cpp11/as.hpp"               // for as_sexp
 #include "cpp11/attribute_proxy.hpp"  // for attribute_proxy
 #include "cpp11/named_arg.hpp"        // for named_arg
-#include "cpp11/protect.hpp"          // for store
+#include "cpp11/protect.hpp"          // for safe
 #include "cpp11/r_vector.hpp"         // for r_vector, r_vector<>::proxy
 #include "cpp11/sexp.hpp"             // for sexp
 
@@ -100,7 +100,6 @@ template <>
 inline r_vector<int>::r_vector(std::initializer_list<named_arg> il)
     : cpp11::r_vector<int>(safe[Rf_allocVector](INTSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = detail::store::insert(data_);
   int n_protected = 0;
 
   try {
@@ -116,7 +115,6 @@ inline r_vector<int>::r_vector(std::initializer_list<named_arg> il)
       UNPROTECT(n_protected);
     });
   } catch (const unwind_exception& e) {
-    detail::store::release(protect_);
     UNPROTECT(n_protected);
     throw e;
   }
diff --git a/inst/include/cpp11/list.hpp b/inst/include/cpp11/list.hpp
@@ -5,7 +5,7 @@
 #include "cpp11/R.hpp"                // for SEXP, SEXPREC, SET_VECTOR_ELT
 #include "cpp11/attribute_proxy.hpp"  // for attribute_proxy
 #include "cpp11/named_arg.hpp"        // for named_arg
-#include "cpp11/protect.hpp"          // for store
+#include "cpp11/protect.hpp"          // for safe
 #include "cpp11/r_string.hpp"         // for r_string
 #include "cpp11/r_vector.hpp"         // for r_vector, r_vector<>::proxy
 #include "cpp11/sexp.hpp"             // for sexp
@@ -78,7 +78,6 @@ template <>
 inline r_vector<SEXP>::r_vector(std::initializer_list<SEXP> il)
     : cpp11::r_vector<SEXP>(safe[Rf_allocVector](VECSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = detail::store::insert(data_);
   auto it = il.begin();
   for (R_xlen_t i = 0; i < capacity_; ++i, ++it) {
     SET_VECTOR_ELT(data_, i, *it);
@@ -89,7 +88,6 @@ template <>
 inline r_vector<SEXP>::r_vector(std::initializer_list<named_arg> il)
     : cpp11::r_vector<SEXP>(safe[Rf_allocVector](VECSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = detail::store::insert(data_);
   int n_protected = 0;
 
   try {
@@ -105,7 +103,6 @@ inline r_vector<SEXP>::r_vector(std::initializer_list<named_arg> il)
       UNPROTECT(n_protected);
     });
   } catch (const unwind_exception& e) {
-    detail::store::release(protect_);
     UNPROTECT(n_protected);
     throw e;
   }
diff --git a/inst/include/cpp11/logicals.hpp b/inst/include/cpp11/logicals.hpp
@@ -7,7 +7,7 @@
 #include "cpp11/R.hpp"                // for SEXP, SEXPREC, Rf_all...
 #include "cpp11/attribute_proxy.hpp"  // for attribute_proxy
 #include "cpp11/named_arg.hpp"        // for named_arg
-#include "cpp11/protect.hpp"          // for store
+#include "cpp11/protect.hpp"          // for safe
 #include "cpp11/r_bool.hpp"           // for r_bool
 #include "cpp11/r_vector.hpp"         // for r_vector, r_vector<>::proxy
 #include "cpp11/sexp.hpp"             // for sexp
@@ -80,7 +80,6 @@ inline bool operator==(const r_vector<r_bool>::proxy& lhs, r_bool rhs) {
 template <>
 inline r_vector<r_bool>::r_vector(std::initializer_list<r_bool> il)
     : cpp11::r_vector<r_bool>(Rf_allocVector(LGLSXP, il.size())), capacity_(il.size()) {
-  protect_ = detail::store::insert(data_);
   auto it = il.begin();
   for (R_xlen_t i = 0; i < capacity_; ++i, ++it) {
     SET_LOGICAL_ELT(data_, i, *it);
@@ -91,7 +90,6 @@ template <>
 inline r_vector<r_bool>::r_vector(std::initializer_list<named_arg> il)
     : cpp11::r_vector<r_bool>(safe[Rf_allocVector](LGLSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = detail::store::insert(data_);
   int n_protected = 0;
 
   try {
@@ -107,7 +105,6 @@ inline r_vector<r_bool>::r_vector(std::initializer_list<named_arg> il)
       UNPROTECT(n_protected);
     });
   } catch (const unwind_exception& e) {
-    detail::store::release(protect_);
     UNPROTECT(n_protected);
     throw e;
   }
diff --git a/inst/include/cpp11/r_vector.hpp b/inst/include/cpp11/r_vector.hpp
@@ -214,8 +214,6 @@ using has_begin_fun = std::decay<decltype(*begin(std::declval<T>()))>;
 template <typename T>
 class r_vector : public cpp11::r_vector<T> {
  private:
-  SEXP protect_ = R_NilValue;
-
   // These are necessary because type names are not directly accessible in
   // template inheritance
   using typename cpp11::r_vector<T>::underlying_type;
@@ -224,6 +222,9 @@ class r_vector : public cpp11::r_vector<T> {
   using cpp11::r_vector<T>::data_p_;
   using cpp11::r_vector<T>::is_altrep_;
   using cpp11::r_vector<T>::length_;
+  using cpp11::r_vector<T>::protect_;
+
+  using cpp11::r_vector<T>::get_p;
 
   R_xlen_t capacity_ = 0;
 
@@ -302,8 +303,6 @@ class r_vector : public cpp11::r_vector<T> {
 
   explicit r_vector(const R_xlen_t size);
 
-  ~r_vector();
-
   r_vector(const r_vector& rhs);
   r_vector(r_vector&& rhs);
 
@@ -660,27 +659,20 @@ inline typename r_vector<T>::iterator r_vector<T>::end() const {
 
 template <typename T>
 inline r_vector<T>::r_vector(const SEXP& data)
-    : cpp11::r_vector<T>(safe[Rf_shallow_duplicate](data)),
-      protect_(detail::store::insert(data_)),
-      capacity_(length_) {}
+    : cpp11::r_vector<T>(safe[Rf_shallow_duplicate](data)), capacity_(length_) {}
 
 template <typename T>
 inline r_vector<T>::r_vector(const SEXP& data, bool is_altrep)
     : cpp11::r_vector<T>(safe[Rf_shallow_duplicate](data), is_altrep),
-      protect_(detail::store::insert(data_)),
       capacity_(length_) {}
 
 template <typename T>
 inline r_vector<T>::r_vector(SEXP&& data)
-    : cpp11::r_vector<T>(data),
-      protect_(detail::store::insert(data_)),
-      capacity_(length_) {}
+    : cpp11::r_vector<T>(data), capacity_(length_) {}
 
 template <typename T>
 inline r_vector<T>::r_vector(SEXP&& data, bool is_altrep)
-    : cpp11::r_vector<T>(data, is_altrep),
-      protect_(detail::store::insert(data_)),
-      capacity_(length_) {}
+    : cpp11::r_vector<T>(data, is_altrep), capacity_(length_) {}
 
 template <typename T>
 template <typename Iter>
@@ -709,11 +701,6 @@ inline r_vector<T>::r_vector(const R_xlen_t size) : r_vector() {
   resize(size);
 }
 
-template <typename T>
-inline r_vector<T>::~r_vector() {
-  detail::store::release(protect_);
-}
-
 #ifdef LONG_VECTOR_SUPPORT
 template <typename T>
 inline typename r_vector<T>::proxy r_vector<T>::operator[](const int pos) const {
@@ -793,22 +780,15 @@ inline typename r_vector<T>::iterator r_vector<T>::find(const r_string& name) co
 
 template <typename T>
 inline r_vector<T>::r_vector(const r_vector<T>& rhs)
-    : cpp11::r_vector<T>(safe[Rf_shallow_duplicate](rhs)),
-      protect_(detail::store::insert(data_)),
-      capacity_(rhs.capacity_) {}
+    : cpp11::r_vector<T>(safe[Rf_shallow_duplicate](rhs)), capacity_(rhs.capacity_) {}
 
 template <typename T>
 inline r_vector<T>::r_vector(r_vector<T>&& rhs)
-    : cpp11::r_vector<T>(rhs), protect_(rhs.protect_), capacity_(rhs.capacity_) {
-  rhs.data_ = R_NilValue;
-  rhs.protect_ = R_NilValue;
-}
+    : cpp11::r_vector<T>(rhs), capacity_(rhs.capacity_) {}
 
 template <typename T>
 inline r_vector<T>::r_vector(const cpp11::r_vector<T>& rhs)
-    : cpp11::r_vector<T>(safe[Rf_shallow_duplicate](rhs)),
-      protect_(detail::store::insert(data_)),
-      capacity_(rhs.length_) {}
+    : cpp11::r_vector<T>(safe[Rf_shallow_duplicate](rhs)), capacity_(rhs.length_) {}
 
 // We don't release the old object until the end in case we throw an exception
 // during the duplicate.
@@ -818,17 +798,21 @@ inline r_vector<T>& r_vector<T>::operator=(const r_vector<T>& rhs) {
     return *this;
   }
 
-  cpp11::r_vector<T>::operator=(rhs);
-
-  auto old_protect = protect_;
+  SEXP old_protect = protect_;
 
+  // We are in writable mode, so we must duplicate the `rhs` (since it isn't a temporary
+  // we can just take ownership of) and recompute the properties from the duplicate.
   data_ = safe[Rf_shallow_duplicate](rhs.data_);
   protect_ = detail::store::insert(data_);
+  is_altrep_ = ALTREP(data_);
+  data_p_ = get_p(is_altrep_, data_);
+  length_ = rhs.length_;
 
-  detail::store::release(old_protect);
-
+  // Specific to writable
   capacity_ = rhs.capacity_;
 
+  detail::store::release(old_protect);
+
   return *this;
 }
 
@@ -838,17 +822,22 @@ inline r_vector<T>& r_vector<T>::operator=(r_vector<T>&& rhs) {
     return *this;
   }
 
-  cpp11::r_vector<T>::operator=(rhs);
-
   SEXP old_protect = protect_;
 
+  // `rhs` is a temporary. Take ownership of its properties and then set its `protect_`
+  // value to `R_NilValue` since we will handle that now. We don't want the `rhs`
+  // destructor to release the object we are taking ownership of.
   data_ = rhs.data_;
   protect_ = rhs.protect_;
+  is_altrep_ = rhs.is_altrep_;
+  data_p_ = rhs.data_p_;
+  length_ = rhs.length_;
 
-  detail::store::release(old_protect);
-
+  // Specific to writable
   capacity_ = rhs.capacity_;
 
+  detail::store::release(old_protect);
+
   rhs.data_ = R_NilValue;
   rhs.protect_ = R_NilValue;
 
diff --git a/inst/include/cpp11/raws.hpp b/inst/include/cpp11/raws.hpp
@@ -8,7 +8,7 @@
 #include "cpp11/R.hpp"                // for RAW, SEXP, SEXPREC, Rf_allocVector
 #include "cpp11/attribute_proxy.hpp"  // for attribute_proxy
 #include "cpp11/named_arg.hpp"        // for named_arg
-#include "cpp11/protect.hpp"          // for store
+#include "cpp11/protect.hpp"          // for safe
 #include "cpp11/r_vector.hpp"         // for r_vector, r_vector<>::proxy
 #include "cpp11/sexp.hpp"             // for sexp
 
@@ -88,7 +88,6 @@ template <>
 inline r_vector<uint8_t>::r_vector(std::initializer_list<uint8_t> il)
     : cpp11::r_vector<uint8_t>(safe[Rf_allocVector](RAWSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = detail::store::insert(data_);
   auto it = il.begin();
   for (R_xlen_t i = 0; i < capacity_; ++i, ++it) {
     data_p_[i] = *it;
@@ -99,7 +98,6 @@ template <>
 inline r_vector<uint8_t>::r_vector(std::initializer_list<named_arg> il)
     : cpp11::r_vector<uint8_t>(safe[Rf_allocVector](RAWSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = detail::store::insert(data_);
   int n_protected = 0;
 
   try {
@@ -116,7 +114,6 @@ inline r_vector<uint8_t>::r_vector(std::initializer_list<named_arg> il)
       UNPROTECT(n_protected);
     });
   } catch (const unwind_exception& e) {
-    detail::store::release(protect_);
     UNPROTECT(n_protected);
     throw e;
   }
diff --git a/inst/include/cpp11/strings.hpp b/inst/include/cpp11/strings.hpp
@@ -7,7 +7,7 @@
 #include "cpp11/as.hpp"               // for as_sexp
 #include "cpp11/attribute_proxy.hpp"  // for attribute_proxy
 #include "cpp11/named_arg.hpp"        // for named_arg
-#include "cpp11/protect.hpp"          // for store
+#include "cpp11/protect.hpp"          // for safe
 #include "cpp11/r_string.hpp"         // for r_string
 #include "cpp11/r_vector.hpp"         // for r_vector, r_vector<>::proxy
 #include "cpp11/sexp.hpp"             // for sexp
@@ -94,19 +94,15 @@ inline SEXP alloc_if_charsxp(const SEXP data) {
 
 template <>
 inline r_vector<r_string>::r_vector(const SEXP& data)
-    : cpp11::r_vector<r_string>(alloc_or_copy(data)),
-      protect_(detail::store::insert(data_)),
-      capacity_(length_) {
+    : cpp11::r_vector<r_string>(alloc_or_copy(data)), capacity_(length_) {
   if (TYPEOF(data) == CHARSXP) {
     SET_STRING_ELT(data_, 0, data);
   }
 }
 
 template <>
 inline r_vector<r_string>::r_vector(SEXP&& data)
-    : cpp11::r_vector<r_string>(alloc_if_charsxp(data)),
-      protect_(detail::store::insert(data_)),
-      capacity_(length_) {
+    : cpp11::r_vector<r_string>(alloc_if_charsxp(data)), capacity_(length_) {
   if (TYPEOF(data) == CHARSXP) {
     SET_STRING_ELT(data_, 0, data);
   }
@@ -120,7 +116,6 @@ template <>
 inline r_vector<r_string>::r_vector(std::initializer_list<named_arg> il)
     : cpp11::r_vector<r_string>(safe[Rf_allocVector](STRSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = detail::store::insert(data_);
   int n_protected = 0;
 
   try {
@@ -136,7 +131,6 @@ inline r_vector<r_string>::r_vector(std::initializer_list<named_arg> il)
       UNPROTECT(n_protected);
     });
   } catch (const unwind_exception& e) {
-    detail::store::release(protect_);
     UNPROTECT(n_protected);
     throw e;
   }

Original file line number	Diff line number	Diff line change
`@@ -223,7 +223,6 @@ context("integers-C++") {`
`223`	`223`	`R_xlen_t after = cpp11::detail::store::count();`
`224`	`224`
`225`	`225`	`expect_true(before == 0);`
`226`		`- // TODO: This should be 1 but writable vectors are being double protected`
`227`		`- expect_true(after - before == 2);`
	`226`	`+ expect_true(after - before == 1);`
`228`	`227`	`}`
`229`	`228`	`}`
Original file line number	Diff line number	Diff line change
`@@ -178,7 +178,6 @@ context("list-C++") {`
`178`	`178`	`R_xlen_t after = cpp11::detail::store::count();`
`179`	`179`
`180`	`180`	`expect_true(before == 0);`
`181`		`- // TODO: This should be 1 but writable vectors are being double protected`
`182`		`- expect_true(after - before == 2);`
	`181`	`+ expect_true(after - before == 1);`
`183`	`182`	`}`
`184`	`183`	`}`