sugar expressions give wrong results under aliasing, length mismatch, and captured temporaries

[kevinushey]

While looking at sugar performance (#1510), I found several ways the sugar expression-template machinery gives silently wrong results or reads out of bounds. All of these reproduce on current master (d7be2c3).

Reproducer

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
NumericVector alias_rev(NumericVector x) { x = rev(x); return x; }

// [[Rcpp::export]]
NumericVector alias_rev_copy(NumericVector x) { NumericVector y = x; y = rev(x); return y; }

// [[Rcpp::export]]
NumericVector alias_mixed(NumericVector x) { x = x + rev(x); return x; }

// [[Rcpp::export]]
void rebind_same_len(NumericVector x) { x = x * 2.0; }

// [[Rcpp::export]]
void rebind_diff_len(NumericVector x) { x = head(x, 2) * 2.0; }

// [[Rcpp::export]]
NumericVector plus(NumericVector x, NumericVector y) { return x + y; }

// [[Rcpp::export]]
NumericVector dangling_auto(NumericVector x) {
  auto e = x + x * 2.0;
  NumericVector big(1000000, 42.0);
  return e;
}

alias_rev(c(1, 2, 3, 4, 5))
#> [1] 5 4 3 4 5          # expected 5 4 3 2 1
alias_rev_copy(c(1, 2, 3, 4, 5))
#> [1] 5 4 3 4 5          # expected 5 4 3 2 1
alias_mixed(c(1, 2, 3, 4, 5))
#> [1]  6  6  6 10 11     # expected 6 6 6 6 6

v <- c(1, 2, 3); w <- v
rebind_same_len(v)
v; w
#> [1] 2 4 6              # both v and w modified
#> [1] 2 4 6
v <- c(1, 2, 3)
rebind_diff_len(v)
v
#> [1] 1 2 3              # unmodified

plus(c(1, 2, 3, 4, 5), c(10, 20))
#> [1] 11 22  3  4  5     # reads past the end of y; only a warning
plus(c(1, 2), c(10, 20, 30, 40))
#> [1] 11 22              # silently truncated

dangling_auto(c(1, 2, 3))
#> [1] 1 2 3              # expected 3 6 9

1. Assigning an expression into one of its own operands gives wrong results

When a sugar expression has the same length as the target, Vector::assign_sugar_expression() (inst/include/Rcpp/vector/Vector.h) evaluates it directly into the target's existing buffer. That is only correct when element i of the result depends only on element i of each operand. For non-elementwise expressions like rev(), earlier writes clobber values that are read later. This also happens when the target is a shallow copy of an operand (y = x; y = rev(x)), since both share the same SEXP.

2. Assignment semantics depend on the length of the expression

Because of the same code path, x = <expr> writes in place into the existing SEXP if the lengths match, but allocates a new vector and rebinds x if they don't. In the in-place case, the R object passed in by the caller is modified, along with any other R bindings that share it (w above). From the caller's point of view, whether an R object gets mutated depends on the length of an intermediate result.

Always evaluating into a freshly allocated vector would fix both 1 and 2. That does change behavior, though, for any code that (knowingly or not) relies on the in-place write.

3. Mismatched operand lengths read out of bounds rather than recycling

Binary sugar operators take their size from the left operand (e.g. Plus_Vector_Vector::size() returns lhs.size()) and never compare it to the right operand. If the right operand is shorter, elements past its end are read. The bounds check added in #1310 emits a warning but still performs the read. If the right operand is longer, the result is silently truncated. Base R would recycle with a warning in both cases.

Checking operand lengths once, when the expression is constructed, would fix this (either recycling to match R, or signalling an error). It would also make the per-element bounds check redundant for sugar operands, which is the remaining performance gap noted in #1510 / #1511.

4. Sub-expressions are held by reference, so they can dangle

Every sugar node stores its operands as const T& (e.g. Rev holds const VEC_TYPE& object), including temporary sub-expressions. Any expression that outlives the full-expression it was built in, such as one captured with auto, or returned from a helper function with a deduced return type, then refers to destroyed temporaries. This is the classic expression-template pitfall (Eigen documents the same caveat for auto).

A complete fix would store nested sugar expressions by value and only plain vectors by reference, similar to Eigen's ref_selector. That touches every sugar class, so documenting the restriction may be the pragmatic first step.