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

Add a substitute argument to push() #63

Closed
wlandau opened this issue Apr 4, 2023 · 5 comments
Closed

Add a substitute argument to push() #63

wlandau opened this issue Apr 4, 2023 · 5 comments
Assignees

Comments

@wlandau
Copy link
Owner

wlandau commented Apr 4, 2023

Analogous to future::future(). Default should probably be TRUE.

@shikokuchuo
Copy link
Contributor

With this change: shikokuchuo/mirai#49 (comment)

Your substitute argument becomes redundant and can be reverted.

@wlandau
Copy link
Owner Author

wlandau commented Apr 10, 2023

I updated crew to trust mirai to recognize language objects. However, I still prefer substitute for crew. On reason is that I deparse the command as text and make it available for the user to see later. I also prefer to be pedantic about the types of arguments to crew functions. It would be rare for the user to supply e.g. quote(x + y) and want substitute = TRUE, but I prefer even this situation to be unambiguous.

@shikokuchuo
Copy link
Contributor

shikokuchuo commented Apr 10, 2023

I would have agreed with you completely last week, but while I was fixing a subtle bug at the C level, I got more hands on experience of Rf_eval().

As a result, I no longer think the previous behaviour is in any way more correct - the reason being the .Call() barrier which forces the symbols passed to it, so symbols typically don't reach Rf_eval().

The current behaviour of mirai essentially mirrors what happens at the C level.

I am also quite confident of the implementation as it is so simple, and you are more than welcome to use it for crew:

expr <- substitute(.expr)
.expr <- if (is.symbol(expr) && is.language(get0(expr))) .expr else expr

The key to the simplicity is that the R function is.language() returns true for SYMSXP, LANGSXP and EXPRSXP.

From the above, the only time .expr is not substituted is if .expr is passed a symbol i.e. a name of an object. In such a case, it should be unambiguous that the user wants to pass what it refers to, as otherwise it will only ever return the symbol itself.

If the user explicitly passes something like quote(x + y) then that would still be substituted (as it is not a symbol) and returns the language object - the result of evaluating quote() - which is exactly what Rf_eval() does.

Just sharing my thoughts - I recognise that both approaches are valid - indeed that's part of the beauty of the R language! But I think that having a substitute argument is going to invite more confusion. But feel free to disregard!

@wlandau
Copy link
Owner Author

wlandau commented Apr 10, 2023

Thanks for explaining your thinking. I do not share the same opinion about how to treat symbol arguments (I think users may want to test mirai() using a trivial command that just returns an element of the data), but you make good points. (NB the symbol scenario does affect crew because crew only sends calls to crew_eval().)

@shikokuchuo
Copy link
Contributor

shikokuchuo commented Apr 10, 2023

You make a good point - it is possible that someone wants to test mirai with something like mirai(x, x = 1), and x also happens to be a language object in the calling environment.

But now we are talking about probabilities - the joint probability that (i) there exists a language object (not any other type of object) on the search path with the exact same name as the arbitrary symbol specified, and that (ii) the real intention of the mirai is to just evaluate the symbol (which is kind of pointless for a mirai).

So that's why I am willing to make this assumption. As it must be a much rarer occurrence than those who will try and pass in a language object without knowing to set substitute = FALSE.

Sorry for being so long-winded - I just want to make sure my logic is correct, as this discovery was fairly recent.

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

No branches or pull requests

2 participants