Fixes for across() substitution (tidyverse#5898)

* expand_if_across(): use the original quosure environment for the combined quosure * not manually inject `limit` in test * filter_rows() gains a caller_env, similar to summarise_cols() etc ... * revert to baseenv() as the environment of the made up quosure. deal with unevaluated formulas and evaluated formulas differently in across() expansion. Cancel expansion for list of functions * disabling expansion for lists makes it harder for if_any() and if_all() * discriminate based on whether f_env() is a tidy eval mask * test specifically for a known tidy mask * adding comment and tests * +local() * revert expand_if_across() as it does not need change * cancel substitution beyond `~` and `function` closes tidyverse#5896 * revdepcheck 2021-06-06
DavisVaughan · Jun 3, 2021 · 5d23cb8 · 5d23cb8
1 parent a995092
commit 5d23cb8
Show file tree

Hide file tree

Showing 8 changed files with 458 additions and 233 deletions.
diff --git a/R/across.R b/R/across.R
@@ -556,7 +556,7 @@ expand_across <- function(quo) {
     var <- vars[[i]]
 
     for (j in seq_fns) {
-      fn_call <- as_across_fn_call(fns[[j]], var, env)
+      fn_call <- as_across_fn_call(fns[[j]], var, env, mask)
 
       name <- names[[k]]
       expressions[[k]] <- new_dplyr_quosure(
@@ -582,13 +582,25 @@ expand_across <- function(quo) {
 # performance implications for lists of lambdas where formulas will
 # have better performance. It is possible that we will be able to
 # inline evaluated functions with strictness annotations.
-as_across_fn_call <- function(fn, var, env) {
+as_across_fn_call <- function(fn, var, env, mask) {
   if (is_formula(fn, lhs = FALSE)) {
     # Don't need to worry about arguments passed through `...`
     # because we cancel expansion in that case
-    fn <- expr_substitute(fn, quote(.), sym(var))
-    fn <- expr_substitute(fn, quote(.x), sym(var))
-    new_quosure(f_rhs(fn), f_env(fn))
+    expr <- f_rhs(fn)
+    expr <- expr_substitute(expr, quote(.), sym(var))
+    expr <- expr_substitute(expr, quote(.x), sym(var))
+
+    # if the formula environment is the data mask
+    # it means the formula was unevaluated, and in that case
+    # we can use the original quosure environment
+    # otherwise, use the formula environment, as it was previously
+    # evaluated and might include data that is not reacha
+    f_env <- f_env(fn)
+    if (identical(f_env, mask)) {
+      f_env <- env
+    }
+
+    new_quosure(expr, f_env)
   } else {
     fn_call <- call2(as_function(fn), sym(var))
     new_quosure(fn_call, env)

diff --git a/R/filter.R b/R/filter.R
@@ -111,15 +111,15 @@ filter <- function(.data, ..., .preserve = FALSE) {
 
 #' @export
 filter.data.frame <- function(.data, ..., .preserve = FALSE) {
-  loc <- filter_rows(.data, ...)
+  loc <- filter_rows(.data, ..., caller_env = caller_env())
   dplyr_row_slice(.data, loc, preserve = .preserve)
 }
 
-filter_rows <- function(.data, ...) {
+filter_rows <- function(.data, ..., caller_env) {
   dots <- dplyr_quosures(...)
   check_filter(dots)
 
-  mask <- DataMask$new(.data, caller_env())
+  mask <- DataMask$new(.data, caller_env)
   on.exit(mask$forget("filter"), add = TRUE)
 
   env_filter <- env()

diff --git a/R/utils.r b/R/utils.r
@@ -144,9 +144,8 @@ expr_substitute <- function(expr, old, new) {
 }
 node_walk_replace <- function(node, old, new) {
   while (!is_null(node)) {
-    switch(
-      typeof(node_car(node)),
-      language = node_walk_replace(node_cdar(node), old, new),
+    switch(typeof(node_car(node)),
+      language = if (!is_call(node_car(node), c("~", "function"))) node_walk_replace(node_cdar(node), old, new),
       symbol = if (identical(node_car(node), old)) node_poke_car(node, new)
     )
     node <- node_cdr(node)

diff --git a/revdep/README.md b/revdep/README.md
@@ -1,34 +1,37 @@
 # Revdeps
 
-## Failed to check (20)
+## Failed to check (22)
 
-|package      |version |error |warning |note |
-|:------------|:-------|:-----|:-------|:----|
-|bayesdfa     |1.0.0   |1     |        |     |
-|CausalImpact |?       |      |        |     |
-|CB2          |?       |      |        |     |
-|cbar         |?       |      |        |     |
-|crossmap     |?       |      |        |     |
-|diceR        |?       |      |        |     |
-|glmmfields   |0.1.4   |1     |        |     |
-|loon.shiny   |?       |      |        |     |
-|metagam      |?       |      |        |     |
-|mlbstatsR    |0.1.0   |1     |        |     |
-|pencal       |?       |      |        |     |
-|rabhit       |?       |      |        |     |
-|rmdcev       |1.2.4   |1     |        |     |
-|NA           |?       |      |        |     |
-|rstap        |1.0.3   |1     |        |     |
-|scoper       |?       |      |        |     |
-|tigger       |?       |      |        |     |
-|trackr       |?       |      |        |     |
-|vivid        |?       |      |        |     |
-|wrswoR       |?       |      |        |     |
+|package        |version |error |warning |note |
+|:--------------|:-------|:-----|:-------|:----|
+|bayesdfa       |1.1.0   |1     |        |     |
+|CausalImpact   |?       |      |        |     |
+|CB2            |?       |      |        |     |
+|crossmap       |?       |      |        |     |
+|diceR          |?       |      |        |     |
+|glmmfields     |0.1.4   |1     |        |     |
+|loon.shiny     |?       |      |        |     |
+|MarketMatching |?       |      |        |     |
+|metagam        |?       |      |        |     |
+|mlbstatsR      |0.1.0   |1     |        |     |
+|NA             |?       |      |        |     |
+|pencal         |?       |      |        |     |
+|rabhit         |?       |      |        |     |
+|raw            |?       |      |        |     |
+|rmdcev         |1.2.4   |1     |        |     |
+|rstap          |1.0.3   |1     |        |     |
+|scoper         |?       |      |        |     |
+|SynthETIC      |?       |      |        |     |
+|tigger         |?       |      |        |     |
+|trackr         |?       |      |        |     |
+|vivid          |?       |      |        |     |
+|wrswoR         |?       |      |        |     |
 
-## New problems (2)
+## New problems (3)
 
-|package                              |version |error  |warning |note |
-|:------------------------------------|:-------|:------|:-------|:----|
-|[finreportr](problems.md#finreportr) |1.0.2   |__+2__ |        |     |
-|[readabs](problems.md#readabs)       |0.4.9   |__+2__ |        |     |
+|package                              |version |error    |warning |note |
+|:------------------------------------|:-------|:--------|:-------|:----|
+|[finreportr](problems.md#finreportr) |1.0.2   |1 __+1__ |        |     |
+|[SwimmeR](problems.md#swimmer)       |0.10.0  |__+1__   |        |     |
+|[xray](problems.md#xray)             |0.2     |__+1__   |        |     |
 
diff --git a/revdep/cran.md b/revdep/cran.md
@@ -1,41 +1,44 @@
 ## revdepcheck results
 
-We checked 2520 reverse dependencies (2519 from CRAN + 1 from Bioconductor), comparing R CMD check results across CRAN and dev versions of this package.
+We checked 2545 reverse dependencies (2544 from CRAN + 1 from Bioconductor), comparing R CMD check results across CRAN and dev versions of this package.
 
- * We saw 2 new problems
- * We failed to check 19 packages
+ * We saw 3 new problems
+ * We failed to check 21 packages
 
 Issues with CRAN packages are summarised below.
 
 ### New problems
 (This reports the first line of each new failure)
 
 * finreportr
-  checking examples ... ERROR
   checking tests ... ERROR
 
-* readabs
-  checking examples ... ERROR
+* SwimmeR
   checking tests ... ERROR
 
+* xray
+  checking examples ... ERROR
+
 ### Failed to check
 
-* bayesdfa     (NA)
-* CausalImpact (NA)
-* CB2          (NA)
-* cbar         (NA)
-* crossmap     (NA)
-* diceR        (NA)
-* glmmfields   (NA)
-* loon.shiny   (NA)
-* metagam      (NA)
-* mlbstatsR    (NA)
-* pencal       (NA)
-* rabhit       (NA)
-* rmdcev       (NA)
-* rstap        (NA)
-* scoper       (NA)
-* tigger       (NA)
-* trackr       (NA)
-* vivid        (NA)
-* wrswoR       (NA)
+* bayesdfa       (NA)
+* CausalImpact   (NA)
+* CB2            (NA)
+* crossmap       (NA)
+* diceR          (NA)
+* glmmfields     (NA)
+* loon.shiny     (NA)
+* MarketMatching (NA)
+* metagam        (NA)
+* mlbstatsR      (NA)
+* pencal         (NA)
+* rabhit         (NA)
+* raw            (NA)
+* rmdcev         (NA)
+* rstap          (NA)
+* scoper         (NA)
+* SynthETIC      (NA)
+* tigger         (NA)
+* trackr         (NA)
+* vivid          (NA)
+* wrswoR         (NA)