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

Inheritance problems in new_data_mask #551

Closed
jrnold opened this issue Jun 28, 2018 · 1 comment
Closed

Inheritance problems in new_data_mask #551

jrnold opened this issue Jun 28, 2018 · 1 comment

Comments

@jrnold
Copy link
Contributor

@jrnold jrnold commented Jun 28, 2018

In new_data_mask() when the bottom environment is not a child of top the evaluation produces confusing results. It does not raise an error initially, which I think should be the behavior. The data mask can be created, but when used for evaluation it does not work correctly. It then no longer inherits from the parent environment specified in

library("rlang")
# You can create a data mask manually from environments.
# In this example, we will create a mask from three environments.
# There is a top environment,
top <- new_environment(list(foo = 1, bar = "abc"))
# a middle environment which is a child of top and overrides bar,
bottom <-  new_environment(list(bar = 3))

# create a mask which specifies the top and bottom environments
mask <- new_data_mask(bottom, top = top)

# now it goes to the empty environment
env_parents(mask)
#> [[1]]
#> <environment: 0x7faf07515ee0>
#> 
#> [[2]]
#> <environment: R_EmptyEnv>

# so this won't work
try(eval_tidy(quo(foo + bar), data = mask))

# however, something like this will work
eval_tidy(quo(foo + 1), data = top)
#> [1] 2

I think that adding a test that the top environment is an ancestor of the bottom environment in new_data_mask() should avoid these issue. I didn't dig into the source code for the C++ function that new_data_mask() calls. I'm happy to put this in a pull request if that's how you want to handle it.

Session Info

sessionInfo()
#> R version 3.5.0 (2018-04-23)
#> Platform: x86_64-apple-darwin15.6.0 (64-bit)
#> Running under: macOS High Sierra 10.13.5
#> 
#> Matrix products: default
#> BLAS: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRblas.0.dylib
#> LAPACK: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRlapack.dylib
#> 
#> locale:
#> [1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8
#> 
#> attached base packages:
#> [1] stats     graphics  grDevices utils     datasets  methods   base     
#> 
#> other attached packages:
#> [1] rlang_0.2.1
#> 
#> loaded via a namespace (and not attached):
#>  [1] compiler_3.5.0  backports_1.1.2 magrittr_1.5    rprojroot_1.3-2
#>  [5] tools_3.5.0     htmltools_0.3.6 yaml_2.1.19     Rcpp_0.12.17   
#>  [9] stringi_1.2.3   rmarkdown_1.10  knitr_1.20      stringr_1.3.1  
#> [13] digest_0.6.15   evaluate_0.10.1
@lionel- lionel- closed this in 9a1fc75 Sep 3, 2018
@lionel-
Copy link
Member

@lionel- lionel- commented Sep 3, 2018

Thanks! It makes sense to throw an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants