fix(filter.epi_archive): for+lazy, left-behind bindings

Author: brookslogan

R/methods-epi_archive.R

@@ -1069,16 +1069,24 @@ filter.epi_archive <- function(.data, ..., .by = NULL, .format_aware = FALSE) {
                 ">" = "If not, see `?filter.epi_archive` details for how to proceed."
               ), class = "epiprocess__filter_archive__used_version"), assign.env = e)
               for (measurement_colname in measurement_colnames) {
-                delayedAssign(measurement_colname, cli::cli_abort(c(
-                  "Using `{format_varname(measurement_colname)}`
-                 in `filter.epi_archive` may produce unexpected results.",
-                  ">" = "See `?filter.epi_archive` details for how to proceed."
-                ), class = "epiprocess__filter_archive__used_measurement"), assign.env = e)
+                # Record current `measurement_colname` and set up delayed
+                # binding for error in a child environment, so that `for` loop
+                # updating its value and `rm` cleanup don't mess things up:
+                local({
+                  local_measurement_colname <- measurement_colname
+                  delayedAssign(measurement_colname, cli::cli_abort(c(
+                    "Using `{format_varname(local_measurement_colname)}`
+                     in `filter.epi_archive` may produce unexpected results.",
+                    ">" = "See `?filter.epi_archive` details for how to proceed."
+                  ), class = "epiprocess__filter_archive__used_measurement"), assign.env = e)
+                })
               }
               break
             }
             e <- parent.env(e)
           }
+          # Don't mask similarly-named user objects:
+          rm(list = c("e", "measurement_colname"))
           TRUE
         },
         ...,

tests/testthat/_snaps/methods-epi_archive.md

@@ -0,0 +1,35 @@
+# filter.epi_archive works as expected
+
+    Code
+      ea2 %>% filter(version <= as.Date("2020-06-02"))
+    Condition <rlang_error>
+      Error in `filter()`:
+      i In argument: `version <= as.Date("2020-06-02")`.
+      Caused by error:
+      ! Using `version` in `filter.epi_archive` may produce unexpected results.
+      > See if `epix_as_of` or `epix_slide` would work instead.
+      > If not, see `?filter.epi_archive` details for how to proceed.
+
+---
+
+    Code
+      ea2 %>% filter(time_value >= as.Date("2020-06-02"), cases >= 2L)
+    Condition <rlang_error>
+      Error in `filter()`:
+      i In argument: `cases >= 2L`.
+      Caused by error:
+      ! Using `cases` in `filter.epi_archive` may produce unexpected results.
+      > See `?filter.epi_archive` details for how to proceed.
+
+---
+
+    Code
+      ea2p %>% filter(cases >= median(cases), .by = geo_value)
+    Condition <rlang_error>
+      Error in `filter()`:
+      i In argument: `cases >= median(cases)`.
+      i In group 1: `geo_value = "ca"`.
+      Caused by error:
+      ! Using `cases` in `filter.epi_archive` may produce unexpected results.
+      > See `?filter.epi_archive` details for how to proceed.
+

tests/testthat/test-methods-epi_archive.R

@@ -191,7 +191,11 @@ test_that("filter.epi_archive works as expected", {
 
   # Environment variables should be fine:
   version <- as.Date("2020-06-02") + 1
-  expect_no_error(ea2 %>% filter(geo_value == "ca", .env$version <= time_value))
+  e <- version
+  expected <- ea2 %>% filter(geo_value == "ca", as.Date("2020-06-02") + 1 <= time_value)
+  expect_equal(ea2 %>% filter(geo_value == "ca", .env$version <= time_value), expected)
+  expect_equal(ea2 %>% filter(geo_value == "ca", e <= time_value), expected)
+  expect_equal(ea2 %>% filter(geo_value == "ca", .env$e <= time_value), expected)
 
   # Error-raising:
   expect_error(
@@ -218,10 +222,18 @@ test_that("filter.epi_archive works as expected", {
     ea2 %>% filter(time_value >= as.Date("2020-06-02"), cases >= 2L),
     class = "epiprocess__filter_archive__used_measurement"
   )
+  ea2p <- ea2_data %>%
+    # to check for `for` + `delayedAssign` mishap in expect_snapshot
+    mutate(deaths = 0) %>%
+    as_epi_archive()
   expect_error(
-    ea2 %>% filter(cases >= median(cases), .by = geo_value),
+    ea2p %>% filter(cases >= median(cases), .by = geo_value),
     class = "epiprocess__filter_archive__used_measurement"
   )
+  expect_snapshot(
+    ea2p %>% filter(cases >= median(cases), .by = geo_value),
+    error = TRUE, cnd_class = TRUE
+  )
 
   # Escape hatch:
   expect_equal(