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

env_print incorrectly labels binding as promise #554

Closed
hadley opened this issue Jul 3, 2018 · 10 comments
Closed

env_print incorrectly labels binding as promise #554

hadley opened this issue Jul 3, 2018 · 10 comments
Labels
doc:adv-r 📖 Needed for adv-r env

Comments

@hadley
Copy link
Member

hadley commented Jul 3, 2018

library(rlang)

power1 <- function(exponent) {
  function(x) {
    x ^ exponent
  }
}

square <- power1(2)
square(2)
#> [1] 4

env <- get_env(square)
env_print(env)
#> <environment: 0x7ff5c3cf7988>
#>   parent: <environment: global>
#>   bindings:
#>    * exponent: <promise>

env$exponent
#> [1] 2
env_print(env)
#> <environment: 0x7ff5c3cf7988>
#>   parent: <environment: global>
#>   bindings:
#>    * exponent: <promise>

Created on 2018-07-03 by the reprex package (v0.2.0).

@hadley hadley added the env label Jul 3, 2018
@lionel-
Copy link
Member

lionel- commented Jul 3, 2018

We should probably climb forced promises trails to get the right types.

@hadley
Copy link
Member Author

hadley commented Aug 13, 2018

I think there's something slightly more subtle going on:

library(rlang)

e <- env()
env_bind_exprs(e, x = 1)
env_binding_are_promise(e)
#>    x 
#> TRUE
rlang:::env_binding_type_sum(e)
#>         x 
#> "promise"

e$x
#> [1] 1
env_binding_are_promise(e)
#>     x 
#> FALSE
rlang:::env_binding_type_sum(e)
#>         x 
#> "promise"

@hadley
Copy link
Member Author

hadley commented Aug 13, 2018

Oh it's because env_get() retrieves the promise object:

library(rlang)

e <- env()
env_bind_exprs(e, x = 1)
e$x
#> [1] 1
env_get(e, "x")
#> <promise: 0x7fae45ac2b50>

@hadley hadley added the doc:adv-r 📖 Needed for adv-r label Aug 13, 2018
@hadley
Copy link
Member Author

hadley commented Aug 13, 2018

What is the desired behaviour here? Maybe env_get() should get an eval argument?

@lionel-
Copy link
Member

lionel- commented Aug 13, 2018

I was wondering about that. base::get() does evaluate promises, and we should probably too because promises are not meant to be returned on the R side.

@hadley
Copy link
Member Author

hadley commented Aug 13, 2018

What about active bindings?

@lionel-
Copy link
Member

lionel- commented Aug 13, 2018

env_get() would return the value of the active binding.

@lionel-
Copy link
Member

lionel- commented Aug 13, 2018

(i.e. the result of calling the active function)

@hadley
Copy link
Member Author

hadley commented Aug 13, 2018

I can implement this, I think - it just needs a couple of if statements at the end of rlang_env_get(), right?

@lionel-
Copy link
Member

lionel- commented Aug 13, 2018

yup a new branch calling r_eval() should do it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc:adv-r 📖 Needed for adv-r env
Projects
None yet
Development

No branches or pull requests

2 participants