read subset elements before writing when both index the same vector

Author: kevinushey

inst/NEWS.Rd

@@ -30,7 +30,12 @@
       \item Sugar expressions hold nested expressions by value so they can be
       stored, evaluate expressions that may read the target before assigning
       them in place, and signal an error for operands of different lengths
-      (Kevin in \ghpr{1513} closing \ghit{1512})
+      (Kevin in \ghpr{1513} closing \ghit{1512}). Same-length assignment
+      of an expression that is not elementwise, such as \code{x = rev(x)},
+      now goes through a temporary vector.
+      \item Assignment between overlapping subsets of the same vector, as in
+      \code{x[i] = x[j]}, reads the source elements before writing (Kevin in
+      \ghpr{1513})
       \item The sugar operators \code{!}, \code{&&} and \code{||} on single
       logical results such as \code{all()} now compile, and follow R's
       handling of \code{NA} (Kevin in \ghpr{1513})

inst/include/Rcpp/vector/Subsetter.h

@@ -110,16 +110,28 @@ class SubsetProxy {
 
     SubsetProxy& operator=(const SubsetProxy& other) {
         if (other.indices_n == 1) {
+            // a single source element is only ever overwritten with itself
             for (R_xlen_t i=0; i < indices_n; ++i) {
                 lhs[ indices[i] ] = other.lhs[other.indices[0]];
             }
         }
-        else if (indices_n == other.indices_n) {
-            for (R_xlen_t i=0; i < indices_n; ++i)
-                lhs[ indices[i] ] = other.lhs[other.indices[i]];
+        else if (indices_n != other.indices_n) {
+            stop("index error");
+        }
+        else if (lhs.get__() == other.lhs.get__()) {
+            // both subsets index the same vector, so a source element may
+            // be overwritten before it is read; copy the source elements
+            // first
+            Vector<RTYPE, StoragePolicy> tmp = no_init(other.indices_n);
+            for (R_xlen_t i=0; i < other.indices_n; ++i) {
+                tmp[i] = other.lhs[other.indices[i]];
             }
+            return *this = tmp;
+        }
         else {
-            stop("index error");
+            for (R_xlen_t i=0; i < indices_n; ++i) {
+                lhs[ indices[i] ] = other.lhs[other.indices[i]];
+            }
         }
         return *this;
     }
@@ -238,7 +250,7 @@ class SubsetProxy {
     Vector<RTYPE, StoragePolicy> operator __OPERATOR__ (                              \
         const SubsetProxy<RTYPE_OTHER, StoragePolicyOther, RHS_RTYPE_OTHER,           \
                           RHS_NA_OTHER, RHS_T_OTHER>& other) {                        \
-        Vector<RTYPE, StoragePolicy> result(indices_n);                               \
+        Vector<RTYPE, StoragePolicy> result = no_init(indices_n);                     \
         if (other.indices_n == 1) {                                                   \
             for (R_xlen_t i = 0; i < indices_n; ++i)                                  \
                 result[i] = lhs[indices[i]] __OPERATOR__ other.lhs[other.indices[0]]; \

inst/tinytest/cpp/Subset.cpp

@@ -102,6 +102,31 @@ NumericVector subset_assign_vector_size_1(NumericVector x, int i) {
     return x;
 }
 
+// [[Rcpp::export]]
+NumericVector subset_assign_alias(NumericVector x) {
+    // overlapping subsets of the same vector
+    x[IntegerVector::create(1, 2)] = x[IntegerVector::create(0, 1)];
+    return x;
+}
+
+// [[Rcpp::export]]
+CharacterVector subset_assign_alias_string(CharacterVector x) {
+    x[IntegerVector::create(1, 2)] = x[IntegerVector::create(0, 1)];
+    return x;
+}
+
+// [[Rcpp::export]]
+NumericVector subset_assign_alias_disjoint(NumericVector x) {
+    x[IntegerVector::create(0, 1)] = x[IntegerVector::create(2, 3)];
+    return x;
+}
+
+// [[Rcpp::export]]
+NumericVector subset_assign_alias_single(NumericVector x) {
+    x[IntegerVector::create(0, 1, 2)] = x[IntegerVector::create(2)];
+    return x;
+}
+
 // [[Rcpp::export]]
 NumericVector subset_sugar_add(NumericVector x, IntegerVector y)
 {

inst/tinytest/test_subset.R

@@ -65,6 +65,13 @@ expect_error(subset_assign_subset5(1:6), info = "index error")
 
 expect_identical(subset_assign_vector_size_1(1:6,7), c(7,7,7,4,5,6))
 
+## assigning between subsets of the same vector reads every source element
+## before any is overwritten
+expect_identical(subset_assign_alias(c(1, 2, 3)), c(1, 1, 2), info = "x[1:2] = x[0:1]")
+expect_identical(subset_assign_alias_string(c("a", "b", "c")), c("a", "a", "b"), info = "x[1:2] = x[0:1], strings")
+expect_identical(subset_assign_alias_disjoint(c(1, 2, 3, 4)), c(3, 4, 3, 4), info = "x[0:1] = x[2:3]")
+expect_identical(subset_assign_alias_single(c(1, 2, 3)), c(3, 3, 3), info = "x[0:2] = x[2]")
+
 x <- rnorm(10)
 y <- sample(10, 5)
 expect_identical(subset_sugar_add(x, y - 1L), x[y] + x[y])