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

Avoid eager evaluation of arg in vec_as_location*() and vec_recycle() #1150

Closed
krlmlr opened this issue Jun 15, 2020 · 11 comments · Fixed by #1509
Closed

Avoid eager evaluation of arg in vec_as_location*() and vec_recycle() #1150

krlmlr opened this issue Jun 15, 2020 · 11 comments · Fixed by #1509

Comments

@krlmlr
Copy link
Member

krlmlr commented Jun 15, 2020

for pretty error messages in tibble.

Do we need to replace .Call() by something different?

It would be great to move the check_dots_empty() check to C code too.

system.time(vctrs::vec_as_location(1, 1L, arg = Sys.sleep(1)))
#>    user  system elapsed 
#>   0.023   0.000   1.025

Created on 2020-06-15 by the reprex package (v0.3.0)

krlmlr added a commit to tidyverse/tibble that referenced this issue Jun 15, 2020
@lionel-
Copy link
Member

lionel- commented Jun 15, 2020

This would require a new kind of vctrs_arg object that takes an environment and a symbol name.

@krlmlr
Copy link
Member Author

krlmlr commented Jun 25, 2020

The way I see it we could use a technique similar to 9ef15b6 (#998) at the front-end. Are you suggesting we need to introduce a new internal structure to pass along the environment and the name where to look up arg?

@krlmlr krlmlr changed the title Avoid eager evaluation of arg in vec_as_location*() Avoid eager evaluation of arg in vec_as_location*() and vec_recycle() Jun 25, 2020
@krlmlr
Copy link
Member Author

krlmlr commented Jul 6, 2020

Would you review a PR?

@krlmlr
Copy link
Member Author

krlmlr commented Jul 22, 2021

Simpler reprex:

library(vctrs)

vec_as_subscript(1, arg = { print("oof"); deparse(quote(i)) })
#> [1] "oof"
#> [1] 1

Created on 2021-07-20 by the reprex package (v2.0.0)

The deparse() call is slow, it should be run only if there is an actual error.

@krlmlr
Copy link
Member Author

krlmlr commented Oct 27, 2021

@lionel-: I now understand your remark regarding the new vctrs_arg. What should the constructor look like: new_deferred_arg(struct vctrs_arg* parent, SEXP environment, const char* arg_name) ?

I think we can do this in baby steps. I have found 8 instances of new_wrapper_arg(), all have parent = NULL . Do we even need the parent in new_deferred_arg()?

@lionel-
Copy link
Member

lionel- commented Oct 27, 2021

I think you can omit parent, I can't think of a case where this wouldn't be the root arg, since it's for function arguments.

Maybe call it lazy_arg rather than deferred_arg? Shorter and refers to lazy evaluation.

@krlmlr
Copy link
Member Author

krlmlr commented Oct 27, 2021

I need to pass two items to the new lazy_arg_fill(): environment and symbol name. I'm thinking about passing a formula from the R side instead, this would also minimize the code changes necessary. How do you feel about it? How do I evaluate a formula in rlang?

@krlmlr
Copy link
Member Author

krlmlr commented Oct 27, 2021

Example: in vec_as_location(), I would do

  .Call(
    vctrs_as_location,
    ...,
    arg = ~arg
  )

instead of

  .Call(
    vctrs_as_location,
    ...,
    arg = arg
  )

.

@krlmlr
Copy link
Member Author

krlmlr commented Oct 28, 2021

I'll go with using the C-callable equivalents new_quosure() and eval_tidy() for a first draft.

@lionel-
Copy link
Member

lionel- commented Oct 28, 2021

I'd rather not bring quosures into this. Let's use a list that contains an argument name and an environment. These are internal data structures.

@krlmlr
Copy link
Member Author

krlmlr commented Oct 29, 2021

Lists and quosures suffer from the same problem, we'd need to protect these objects globally. Going with an internal caller-allocated struct.

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

Successfully merging a pull request may close this issue.

2 participants