Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use the formula environment as the newly created quosure environment. #5886

Merged
merged 6 commits into from May 19, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 6 additions & 21 deletions R/across.R
Expand Up @@ -571,33 +571,18 @@ expand_across <- function(quo) {
}

# TODO: Take unevaluated `.fns` and inline calls to `function`. This
# will enable support for R 4.1 lambdas. This plan is somewhat at odds
# with the current UI of `across()` which takes a list of function. We
# would need to intepret calls to `list()` specially. And then we
# would miss calls to `list2()` etc. This is another way in which the
# current UI is unsatisfactory from the viewpoint of optimisation.
#
# Note that the current implementation is not 100% correct in that
# regard as we ignore the environment of formulas and instead use the
# quosure environment. This means this doesn't work as expected:
#
# ```
# local({
# prefix <- "foo"
# f <- ~ paste(prefix, .x)
# })
# mutate(data, across(sel, f))
# ```
#
# If we inspected unevaluated calls instead, we would see a symbol `f`
# and fall through the `as_function()` branch.
# will enable support for R 4.1 lambdas. Note that unlike formulas,
# only unevaluated `function` calls can be inlined. This will have
# 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) {
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), env)
new_quosure(f_rhs(fn), f_env(fn))
} else {
fn_call <- call2(as_function(fn), sym(var))
new_quosure(fn_call, env)
Expand Down
29 changes: 25 additions & 4 deletions tests/testthat/test-across.R
Expand Up @@ -528,15 +528,36 @@ test_that("can pass quosure through `across()`", {

test_that("across() inlines formulas", {
env <- env()
f <- ~ toupper(.x)

expect_equal(
as_across_fn_call(~ toupper(.x), quote(foo), env),
new_quosure(quote(toupper(foo)), env)
as_across_fn_call(f, quote(foo), env),
new_quosure(quote(toupper(foo)), f_env(f))
)

f <- ~ list(.x, ., .x)
expect_equal(
as_across_fn_call(~ list(.x, ., .x), quote(foo), env),
new_quosure(quote(list(foo, foo, foo)), env)
as_across_fn_call(f, quote(foo), env),
new_quosure(quote(list(foo, foo, foo)), f_env(f))
)
})

test_that("across() uses local formula environment (#5881)", {
f <- local({
prefix <- "foo"
~ paste(prefix, .x)
})
df <- data.frame(x = "x")
expect_equal(
mutate(df, across(x, f)),
data.frame(x = "foo x")
)
})

test_that("unevaluated formulas (currently) fail", {
df <- data.frame(x = "x")
expect_error(
mutate(df, across(x, quote(~ paste("foo", .x))))
)
})

Expand Down