shift on matrix: news and improve error (#5462)

* news and improve error * Michael feedback, actionable error
Rdatatable · Dec 8, 2023 · a40ec8e · a40ec8e
1 parent 6bde008
commit a40ec8e
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 1 deletion.
diff --git a/NEWS.md b/NEWS.md
@@ -2,6 +2,10 @@
 
 # data.table [v1.14.99](https://github.com/Rdatatable/data.table/milestone/20)  (in development)
 
+## BREAKING CHANGE
+
+1. `shift` and `nafill` will now raise error `input must not be matrix or array` when `matrix` or `array` is provided on input, rather than giving useless result, [#5287](https://github.com/Rdatatable/data.table/issues/5287). Thanks to @ethanbsmith for reporting.
+
 ## NEW FEATURES
 
 1. `nafill()` now applies `fill=` to the front/back of the vector when `type="locf|nocb"`, [#3594](https://github.com/Rdatatable/data.table/issues/3594). Thanks to @ben519 for the feature request. It also now returns a named object based on the input names. Note that if you are considering joining and then using `nafill(...,type='locf|nocb')` afterwards, please review `roll=`/`rollends=` which should achieve the same result in one step more efficiently. `nafill()` is for when filling-while-joining (i.e. `roll=`/`rollends=`/`nomatch=`) cannot be applied.

diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw
@@ -18108,3 +18108,6 @@ test(2238.6, "a" %notin% integer(), TRUE)
 test(2238.7, "a" %notin% NULL, TRUE)
 test(2238.8, NA %notin% 1:5, TRUE)
 test(2238.9, NA %notin% c(1:5, NA), FALSE)
+
+# shift actionable error on matrix input #5287
+test(2239.1, shift(matrix(1:10, ncol = 1)), error="consider wrapping")
diff --git a/src/shift.c b/src/shift.c
@@ -8,6 +8,8 @@ SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type)
   if (!xlength(obj)) return(obj); // NULL, list()
   SEXP x;
   if (isVectorAtomic(obj)) {
+    if (!isNull(getAttrib(obj, R_DimSymbol)))
+      error(_("shift input must not be matrix or array, consider wrapping it into data.table() or c()"));
     x = PROTECT(allocVector(VECSXP, 1)); nprotect++;
     SET_VECTOR_ELT(x, 0, obj);
   } else {

diff --git a/src/utils.c b/src/utils.c
@@ -348,7 +348,7 @@ SEXP coerceAs(SEXP x, SEXP as, SEXP copyArg) {
   if (!isNull(getAttrib(x, R_DimSymbol)))
     error(_("'x' must not be matrix or array"));
   if (!isNull(getAttrib(as, R_DimSymbol)))
-    error(_("'as' must not be matrix or array"));
+    error(_("input must not be matrix or array"));
   bool verbose = GetVerbose()>=2; // verbose level 2 required
   if (!LOGICAL(copyArg)[0] && TYPEOF(x)==TYPEOF(as) && class1(x)==class1(as)) {
     if (verbose)