Use alternate any_multiple_needles detection

NEWS.md

@@ -1,5 +1,8 @@
 # vctrs (development version)
 
+* Fixed a rare `vec_locate_matches()` bug that could occur when using a max/min
+  `filter` (tidyverse/dplyr#6835).
+
 # vctrs 0.6.2
 
 * Fixed conditional S3 registration to avoid a CRAN check NOTE that appears in

src/match.c

@@ -1684,6 +1684,9 @@ r_obj* expand_compact_indices(const int* v_o_haystack,
     multiple == VCTRS_MULTIPLE_error ||
     multiple == VCTRS_MULTIPLE_warning;
 
+  // Used to enforce `check_multiple_needles`
+  r_ssize loc_needles_previous = r_globals.na_int;
+
   bool check_multiple_haystack = false;
   switch (relationship) {
   // Expecting `haystack` can match any number of `needles`
@@ -1824,11 +1827,22 @@ r_obj* expand_compact_indices(const int* v_o_haystack,
     }
 
     if (check_multiple_needles) {
-      if (loc < size_needles) {
-        any_multiple_needles = size_match > 1;
-      } else {
-        // Guaranteed second match if in the "extra" matches section
+      if (size_match > 1) {
+        // Easy, obvious, case.
+        // This containment group had >1 matches for this `needle` so we
+        // immediately handle multiple `needles` matches.
         any_multiple_needles = true;
+      } else if (loc_needles == loc_needles_previous) {
+        // We've recorded a match for this `needle` before. Remember that
+        // `needles` are processed in increasing order across all containment
+        // groups due to `v_o_loc_needles` so this simple tracking of the
+        // previous `needle` works.
+        any_multiple_needles = true;
+      } else {
+        // There was exactly 1 match for the `needle` in this containment group,
+        // and we've never recorded a match for this `needle` before.
+        // In that case we record that we've seen it for the next iteration.
+        loc_needles_previous = loc_needles;
       }
 
       if (any_multiple_needles) {

tests/testthat/_snaps/match.md

@@ -424,6 +424,16 @@
       ! Each value of `needles` can match at most 1 value from `haystack`.
       x Location 1 of `needles` matches multiple values.
 
+# `relationship` errors when we have >1 size 1 matches across containers (tidyverse/dplyr#6835)
+
+    Code
+      vec_locate_matches(x, y, condition = c("<=", ">="), filter = c("none", "none"),
+      relationship = "one-to-one")
+    Condition
+      Error in `vec_locate_matches()`:
+      ! Each value of `needles` can match at most 1 value from `haystack`.
+      x Location 1 of `needles` matches multiple values.
+
 # `relationship` errors respect argument tags and error call
 
     Code

tests/testthat/test-match.R

@@ -1312,6 +1312,59 @@ test_that("`relationship` still errors if `filter` hasn't removed all multiple m
   expect_identical(out$haystack, c(2L, 2L))
 })
 
+test_that("`relationship` errors when we have >1 size 1 matches across containers (tidyverse/dplyr#6835)", {
+  # Carefully designed to ensure we get 2 nested containment groups that split
+  # up the rows of `y`, but each of the nested containment groups contain exactly
+  # 1 match, so `size_match` in `expand_compact_indices()` won't ever be >1
+  x <- data_frame(a = 1L, b = 5L)
+  y <- data_frame(a = c(1L, 2L), b = c(4L, 3L))
+
+  expect_snapshot(error = TRUE, {
+    vec_locate_matches(
+      x,
+      y,
+      condition = c("<=", ">="),
+      filter = c("none", "none"),
+      relationship = "one-to-one"
+    )
+  })
+})
+
+test_that("`relationship` doesn't error when the first match from a different container gets filtered out (tidyverse/dplyr#6835)", {
+  # Carefully designed to ensure we get 2 nested containment groups that split
+  # up the rows of `y`. Row 1 (processed first) doesn't hold the minimum `b`
+  # value, so it gets filtered out. Row 2 is in the "extra" matches section
+  # but is actually the first (and only) real match, so we don't want to error
+  # on it.
+  x <- data_frame(a = 1L, b = 5L)
+  y <- data_frame(a = c(1L, 2L), b = c(4L, 3L))
+
+  out <- vec_locate_matches(
+    x,
+    y,
+    condition = c("<=", ">="),
+    filter = c("none", "min"),
+    relationship = "one-to-one"
+  )
+  expect_identical(out$needles, 1L)
+  expect_identical(out$haystack, 2L)
+
+  # Similar to the above example, but with a `max` filter. Row 1 doesn't hold
+  # the max `c` value so it is filtered out even though it is a `>=` match.
+  x <- data_frame(a = 1L, b = 5L, c = 3L)
+  y <- data_frame(a = c(1L, 2L), b = c(4L, 3L), c = c(1L, 2L))
+
+  out <- vec_locate_matches(
+    x,
+    y,
+    condition = c("<=", ">=", ">="),
+    filter = c("none", "none", "max"),
+    relationship = "one-to-one"
+  )
+  expect_identical(out$needles, 1L)
+  expect_identical(out$haystack, 2L)
+})
+
 test_that("`relationship` errors respect argument tags and error call", {
   expect_snapshot({
     (expect_error(vec_locate_matches(1L, c(1L, 1L), relationship = "one-to-one", needles_arg = "foo", haystack_arg = "bar", error_call = call("fn"))))