r-lib / rlang

Low-level API for programming with R
https://rlang.r-lib.org
Other
501 stars 137 forks source link

quo(1) behaving differently than expected #73

Closed gergness closed 7 years ago

gergness commented 7 years ago

I'm trying to update my package srvyr which has used tidyeval's NSE framework and fit it into the survey package.

I've traced a bug in my code to the behavior of quo on integers. Instead of returning the environment where it is called, it returns an empty environment. Is this the intended behavior?

library(rlang)
`%>%` <- magrittr::`%>%`

# Both formulas and quo work with variable names
model.frame(~mpg, data = mtcars) %>% head()
#>                    mpg
#> Mazda RX4         21.0
#> Mazda RX4 Wag     21.0
#> Datsun 710        22.8
#> Hornet 4 Drive    21.4
#> Hornet Sportabout 18.7
#> Valiant           18.1
model.frame(quo(mpg), data = mtcars) %>% head()
#>                    mpg
#> Mazda RX4         21.0
#> Mazda RX4 Wag     21.0
#> Datsun 710        22.8
#> Hornet 4 Drive    21.4
#> Hornet Sportabout 18.7
#> Valiant           18.1

# But quo returns a quosure with an empty environment when it gets an integer
# while using a formula keeps the environment where it is called
model.frame(~1, data = mtcars) %>% head()
#> data frame with 0 columns and 6 rows
model.frame(quo(1), data = mtcars) %>% head()
#> Error in eval(expr, envir, enclos): could not find function "list"

str(~1)
#> Class 'formula'  language ~1
#>   ..- attr(*, ".Environment")=<environment: 0x7f877bf970e8>
str(quo(1))
#> Classes 'quosure', 'formula'  language ~1
#>   ..- attr(*, ".Environment")=<environment: R_EmptyEnv>

FWIW, the reason I am getting this error is because the survey package uses ~1 as an indicator that each observation is an ID. It's not a particularly widespread syntactic style within the package, and could be worked around.

# From survey::svydesign
library(survey)
svydesign(id=~1,strata=~stype, weights=~pw, data=apistrat, fpc=~fpc)
#> Stratified Independent Sampling design
#> svydesign(id = ~1, strata = ~stype, weights = ~pw, data = apistrat, 
#>     fpc = ~fpc)

I think I could just use quo_name() to check whether it is 1 and handle as a special case. However, srvyr's code got pretty complex because I was working around something that turned out to be a bug in lazyeval, so wanted to avoid making that mistake again.

Thank you!

lionel- commented 7 years ago

yes this is intended behaviour. The environment is not meant for introspection but for hygienic evaluation. For that purpose, literals can be evaluated in the empty environment. If model.frame() tries to evaluate complex code in the environment of the supplied formula, this is a bug in that function. It should not make the assumption that this environment has all the functions it depends on, even if they come from the base package.

As a workaround, you can check for empty enclosures and change them to the base environment.

I think I could just use quo_name() to check whether it is 1

Why not directly check the RHS rather than serialising it to text to make the check?

gergness commented 7 years ago

Awesome, that makes sense. Thanks for the pointers!