Make min and max check sizes of x and ... for clear intent

Author: brookslogan

R/type-vctr.R

@@ -546,6 +546,19 @@ vec_cast_or_na <- function(x, to, ...) {
 #' @export
 min.vctrs_vctr <- function(x, ..., na.rm = FALSE) {
   if (dots_n(...) != 0L) {
+    sizes <- list_sizes(list(x, ...))
+    if (any(sizes != 1L)) {
+      cli::cli_abort(c(
+        "Can't use `...` unless {.arg x} and each of {.arg ...} are of size 1.",
+        "i" = "Size of {.arg x} was {vec_size(x)}",
+        "i" = "{cli::qty(rlang::dots_n(...))}
+               Size{?s} of {.arg ...} {?was/were} {list_sizes(list(...))}",
+        ">" = "If you wanted a size-1 result with the overall {.fn min},
+               use {.code min(c(<args>))}",
+        ">" = "If you wanted a vectorized/parallel {.fn pmin},
+               use {.code pmin(<args>)}"
+      ), class = "vctrs_error_min_intent_uncertain")
+    }
     x <- vec_c(x, ...)
   }
 
@@ -571,6 +584,19 @@ min.vctrs_vctr <- function(x, ..., na.rm = FALSE) {
 #' @export
 max.vctrs_vctr <- function(x, ..., na.rm = FALSE) {
   if (dots_n(...) != 0L) {
+    sizes <- list_sizes(list(x, ...))
+    if (any(sizes != 1L)) {
+      cli::cli_abort(c(
+        "Can't use `...` unless {.arg x} and each of {.arg ...} are of size 1.",
+        "i" = "Size of {.arg x} was {vec_size(x)}",
+        "i" = "{cli::qty(rlang::dots_n(...))}
+               Size{?s} of {.arg ...} {?was/were} {list_sizes(list(...))}",
+        ">" = "If you wanted a size-1 result with the overall {.fn max},
+               use {.code max(c(<args>))}",
+        ">" = "If you wanted a vectorized/parallel {.fn pmax},
+               use {.code pmax(<args>)}"
+      ), class = "vctrs_error_max_intent_uncertain")
+    }
     x <- vec_c(x, ...)
   }
 

tests/testthat/_snaps/type-vctr.md

@@ -37,3 +37,39 @@
         A   B   C 
       xxx xxx xxx 
 
+# `min` and `max` either combine `x` and `...` or abort (#1372)
+
+    Code
+      min(rep(x, 5), y)
+    Condition <vctrs_error_min_intent_uncertain>
+      Error:
+      ! Can't use `...` unless `x` and each of `...` are of size 1.
+      i Size of `x` was 5
+      i Size of `...` was 1
+      > If you wanted a size-1 result with the overall `min()`, use `min(c(<args>))`
+      > If you wanted a vectorized/parallel `pmin()`, use `pmin(<args>)`
+
+---
+
+    Code
+      min(x, rep(y, 5), z)
+    Condition <vctrs_error_min_intent_uncertain>
+      Error:
+      ! Can't use `...` unless `x` and each of `...` are of size 1.
+      i Size of `x` was 1
+      i Sizes of `...` were 5 and 1
+      > If you wanted a size-1 result with the overall `min()`, use `min(c(<args>))`
+      > If you wanted a vectorized/parallel `pmin()`, use `pmin(<args>)`
+
+---
+
+    Code
+      max(rep(x, 5), y)
+    Condition <vctrs_error_max_intent_uncertain>
+      Error:
+      ! Can't use `...` unless `x` and each of `...` are of size 1.
+      i Size of `x` was 5
+      i Size of `...` was 1
+      > If you wanted a size-1 result with the overall `max()`, use `max(c(<args>))`
+      > If you wanted a vectorized/parallel `pmax()`, use `pmax(<args>)`
+

tests/testthat/test-type-vctr.R

@@ -632,16 +632,41 @@ test_that("Summary generics behave identically to base for empty vctrs (#88)", {
   )
 })
 
-test_that("methods combine `x` and `...` when generic expects them to (#1372)", {
+test_that("`min` and `max` either combine `x` and `...` or abort (#1372)", {
   x <- new_vctr(1)
   y <- new_vctr(3)
+  z <- new_vctr(5)
 
   expect_identical(min(x, y), x)
   expect_identical(min(y, x), x)
   expect_identical(max(x, y), y)
   expect_identical(max(y, x), y)
+
+  expect_identical(min(x, y, z), x)
+  expect_identical(max(x, y, z), z)
+
+  expect_error(min(rep(x, 5), y), class = "vctrs_error_min_intent_uncertain")
+  expect_error(min(x, rep(y, 5)), class = "vctrs_error_min_intent_uncertain")
+  expect_error(min(rep(x, 0), y), class = "vctrs_error_min_intent_uncertain")
+  expect_error(min(x, rep(y, 0)), class = "vctrs_error_min_intent_uncertain")
+  expect_error(max(rep(x, 5), y), class = "vctrs_error_max_intent_uncertain")
+  expect_error(max(x, rep(y, 5)), class = "vctrs_error_max_intent_uncertain")
+  expect_error(max(rep(x, 0), y), class = "vctrs_error_max_intent_uncertain")
+  expect_error(max(x, rep(y, 0)), class = "vctrs_error_max_intent_uncertain")
+
+  expect_snapshot(min(rep(x, 5), y), error = TRUE, cnd_class = TRUE)
+  expect_snapshot(min(x, rep(y, 5), z), error = TRUE, cnd_class = TRUE)
+  expect_snapshot(max(rep(x, 5), y), error = TRUE, cnd_class = TRUE)
+})
+
+test_that("`range` combines `...` (#1372)", {
+  x <- new_vctr(1)
+  y <- new_vctr(3)
+  z <- new_vctr(as.numeric(13:11))
+
   expect_identical(range(x, y), c(x, y))
   expect_identical(range(y, x), c(x, y))
+  expect_identical(range(x, y, z), c(x, z[[1]]))
 })
 
 test_that("generic predicates return logical vectors (#251)", {