Throw exceptions rather than returning NAs on indexing out of bounds; allow unsafe subsetting if users really want it

Author: kevinushey

inst/include/Rcpp/vector/Subsetter.h

@@ -47,8 +47,10 @@ namespace Rcpp {
         Subsetter() {};
 
         // helper function used for the subset methods when going from logical to int
+        // operates like R's which, but returns NA when it encounters an NA
         template <template <class> class OtherStoragePolicy>
         static Vector<INTSXP, StoragePolicy> which_na( const Vector<LGLSXP, OtherStoragePolicy>& x) {
+            
             std::vector<int> output;
             int n = x.size();
             output.reserve(n);
@@ -70,6 +72,9 @@ namespace Rcpp {
         // Subsetting for logicals
         template <template <class> class OtherStoragePolicy>
         inline Vector<RTYPE, StoragePolicy> subset_impl( const VECTOR& this_, const Vector<LGLSXP, OtherStoragePolicy>& x ) const {
+            if (this_.size() != x.size()) {
+                stop("unequal vector sizes");
+            }
             Vector<INTSXP, StoragePolicy> tmp = which_na(x);
             if (!tmp.size()) return Vector<RTYPE, StoragePolicy>(0);
             else return subset_impl(this_, tmp);
@@ -78,40 +83,32 @@ namespace Rcpp {
         // Subsetting for characters
         template <template <class> class OtherStoragePolicy>
         inline Vector<RTYPE, StoragePolicy> subset_impl( const VECTOR& this_, const Vector<STRSXP, OtherStoragePolicy>& x ) const {
-      
+            
             if (Rf_isNull( Rf_getAttrib(this_, R_NamesSymbol) )) {
-                int n = x.size();
-                Vector<RTYPE, StoragePolicy> output(n);
-                for (int i=0; i < n; ++i) {
-                    output[i] = traits::get_na<RTYPE>();
-                }
-                return output;
+                stop("Tried to subset a vector with no names using a CharacterVector");
             }
 
             Vector<STRSXP, StoragePolicy> names = as< Vector<STRSXP, StoragePolicy> >(Rf_getAttrib(this_, R_NamesSymbol));
             Vector<INTSXP, StoragePolicy> idx = match(x, names); // match returns 1-based index
-            // apparently, we don't see sugar, so we have to do this manually
+            
+            // apparently, we don't see sugar, so we have to populate an (index - 1) manually
             Vector<INTSXP, StoragePolicy> idxm1 = no_init(idx.size());
             for (int i=0; i < idx.size(); ++i) {
                 idxm1[i] = idx[i] - 1;
             }
+            
             Vector<RTYPE, StoragePolicy> output = subset_impl(this_, idxm1);
             int n = output.size();
             if (n == 0) return Vector<RTYPE, StoragePolicy>(0);
             Vector<STRSXP, StoragePolicy> out_names = no_init(n);
             for (int i=0; i < n; ++i) {
-                if (idx[i] == NA_INTEGER) out_names[i] = NA_STRING;
-                else out_names[i] = names[ idx[i] - 1 ];
+                out_names[i] = names[ idx[i] - 1 ];
             }
             output.attr("names") = out_names;
             return output;
         }
 
-        const VECTOR& vec;
-        const T& other;
-  
         // Subsetting for integers -- note that it is 0-based
-        // hidden until we decide whether to go with 0 or 1 based indexing
         template <template <class> class OtherStoragePolicy>
         inline Vector<RTYPE, StoragePolicy> 
         subset_impl( const VECTOR this_, const Vector<INTSXP, OtherStoragePolicy>& x ) const {
@@ -120,8 +117,10 @@ namespace Rcpp {
             Vector<RTYPE, StoragePolicy> output = no_init(n);
             for (int i=0; i < n; ++i) {
                 if (x[i] == NA_INTEGER) output[i] = traits::get_na<RTYPE>();
-                else if (x[i] < 0) stop("Cannot subset with an integer vector with elements <= 0");
-                else if (x[i] > this_.size() - 1) output[i] = traits::get_na<RTYPE>();
+                #ifndef RCPP_NO_BOUNDS_CHECK
+                else if (x[i] < 0) stop("Index error: tried to index < 0");
+                else if (x[i] > this_.size() - 1) stop("Index error: tried to index above vector size");
+                #endif
                 else output[i] = (this_)[ x[i] ];
             }
 
@@ -133,13 +132,25 @@ namespace Rcpp {
                 Vector<STRSXP, StoragePolicy> outnames = no_init(n);
                 for (int i=0; i < n; ++i) {
                     if (x[i] == NA_INTEGER) outnames[i] = NA_STRING;
+                    #ifndef RCPP_NO_BOUNDS_CHECK
                     else if (x[i] > this_.size() - 1) outnames[i] = NA_STRING;
+                    #endif
                     else if (x[i] >= 0) outnames[i] = thisnames[ x[i] ];
                 }
                 output.attr("names") = outnames;
             }
             return wrap(output);
         }
+        
+        // Subsetting for numerics -- coerce to integer
+        template <template <class> class OtherStoragePolicy>
+        Vector<RTYPE, StoragePolicy>
+        subset_impl( const VECTOR this_, const Vector<REALSXP, OtherStoragePolicy>& x ) const {
+            return subset_impl(this_, as< Vector<INTSXP, OtherStoragePolicy> >(x) );
+        }
+        
+        const VECTOR& vec;
+        const T& other;
 
     };
 

inst/unitTests/runit.subset.R

@@ -29,13 +29,14 @@ if (.runThisTest) {
         x <- rnorm(5)
         names(x) <- letters[1:5]
 
-        checkIdentical( x[c(1, 2, 10)], subset_test_int(x, c(0L, 1L, 9L)),
-            "integer subsetting")
+        checkIdentical( x[c(1, 2, 3)], subset_test_int(x, c(0L, 1L, 2L)) )
+        checkException( subset_test_int(x, -1L) )
+        checkException( subset_test_int(x, length(x)) )
 
         checkIdentical( x[ c('b', 'a') ], subset_test_char(x, c('b', 'a')),
             "character subsetting")
 
-        checkIdentical( c(1, 2, 3)['a'], subset_test_char( c(1, 2, 3), 'a' ),
+        checkException( subset_test_char( c(1, 2, 3), 'a' ),
             "character subsetting -- no names on x")
 
         lgcl <- c(TRUE, FALSE, NA, TRUE, TRUE)
@@ -52,8 +53,8 @@ if (.runThisTest) {
 
         l <- as.list(x)
         checkIdentical(
-            l[c('b', 'Q')],
-            subset_test_list(x, c('b', 'Q')),
+            l[c('b', 'c')],
+            subset_test_list(x, c('b', 'c')),
             "list subsetting")
 
         checkIdentical(