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

tildes in quosures #924

Closed
brodieG opened this issue Feb 14, 2020 · 4 comments · Fixed by #951
Closed

tildes in quosures #924

brodieG opened this issue Feb 14, 2020 · 4 comments · Fixed by #951

Comments

@brodieG
Copy link
Contributor

brodieG commented Feb 14, 2020

This seems odd to me (rlang 0.4.4).

library(rlang)
`~` <- function(...) stop('boom')
~ 1 + 1
#> Error in ~1 + 1: boom
x <- quo(~1 + 1)
eval_tidy(x)
#> ~1 + 1
#> <environment: 0x7f9bca6eea00>

Two unexpected things:

  1. My overloaded tilde is ignored
  2. The returned formula doesn't have .GlobalEnv as its environment (note this is true even when run without reprex).

My mental model of how this all works is incomplete, but I feel it should be possible fetch the correct tilde to evaluate in base_tilde_eval with findVar (in function finding mode anyway, if it does such thing like get does), instead of just going directly to the primitive. Obviously a pretty unlikely corner case to hit (if indeed I'm even correct about this being an error).

Not sure what's going on with the environment. Feels like quo_env should be the global environment here? Or is that still some kind of mask env even without data?

@lionel-
Copy link
Member

lionel- commented Feb 17, 2020

It's a conjunction of multiple issues.

  • Ideally, we would not use a data mask when quosures are evaluated without data. In this case the evaluation would be done within the global env as you would expect.

  • The quosure calls leak in captured calls and captured stacks. To avoid ugly inlining, we use ~ as a symbol pointing to the quosure evaluator. This means we need a minimal quosure mask where ~ is masked, even when there is no data. The expression is thus evaluated in a child of the quosure env.

  • We couldn't overwrite ~ temporarily with rlang::with_bindings() instead of masking it because evaluation might occur in a locked environment.

  • You're right that if we notice a ~ call that isn't a quosure, we could fetch the lexical ~ definition and evaluate it instead of the base one. This is a very obscure edge case so very low priority, but that would be more consistent. I would review a patch.

Ideally quosures would be a native data type in R with special deparsing behaviour, and then quosures would behave as you'd expect in all aspects.

@brodieG
Copy link
Contributor Author

brodieG commented Feb 17, 2020

Makes sense. I'll give the patch a shot as that seems like a very small change that doesn't require me internalizing much of the code base. If I can get the tests to pass I'll submit it. Later this week likely.

@brodieG
Copy link
Contributor Author

brodieG commented Feb 24, 2020

I now have three packages failing CRAN due to stringsAsFactors, so I need to address that first. So might be another week or so.

@lionel-
Copy link
Member

lionel- commented Feb 24, 2020

No worries! There's no hurry.

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

Successfully merging a pull request may close this issue.

2 participants