Hi Brodie, I wouldn't use the rlang tests as a measure of backward compatibility. Your patch changes the structure of the `parent.frame()` hierarchy, which is likely to be disruptive with packages that do weird NSE stuff (and so, not rlang ;-p). Currently things are set up so that the parent frame of the evaluated call is the "target" `eval()` context, which itself has for parent the "closure" `eval()` context.
For example, with: ``` foo <- function() eval(quote(bar())) bar <- function() EXPR() ``` The current hierarchy (as defined by `parent.frame()`) is: ``` █ 1. └─global::foo() 2. └─base::eval(quote(bar())) 3. └─base::eval(quote(bar())) <- This is the same frame env as foo() 4. └─global::bar() 5. └─global::EXPR() ``` With your patch, it becomes: ``` █ 1. └─global::foo() 2. ├─base::eval(quote(bar())) 3. │ └─base::eval(quote(bar())) 4. └─global::bar() 5. └─global::EXPR() ``` I agree the second hierarchy is preferable. See also the documentation of `filter.callframes` in `?Rprof` which patches up things so that the `eval()` frames are filtered out from the trailing backtrace branch. This wouldn't be necessary with the second hierarchy. Best, Lionel On 6/1/20, brodie gaslam via R-devel <r-devel@r-project.org> wrote: > I ran into an interesting issue with `evalq` (and also > `eval(quote(...))`): > > f <- function() { > list( > sys.parent(1), > evalq(sys.parent(1)), > evalq((function() sys.parent(2))()), # add an anon fun layer > evalq((function() sys.parent(1))()) > ) > } > res <- f() > str(res) > ## List of 4 > ## $ : int 0 # sys.parent(1) > ## $ : int 2 # evalq(sys.parent(1)) > ## $ : int 0 # evalq((function() sys.parent(2))()) > ## $ : int 1 # evalq((function() sys.parent(1))()) > > In order of least to most surprising: > > 1. `sys.parent(1)` and `evalq(sys.parent(1))` are not the same > 2. `evalq(sys.parent(1))` and `evalq((function() sys.parent(2))())` > are not the same > 3. `evalq((function() sys.parent(1))())` returns a lower frame number > than `evalq(sys.parent(1))` > > The root cause of this is that the `evalq` **closure** sets a context, > but then the C-level `do_eval` it invokes sets another one[1] with the > new `evalq` context as the calling frame (`cptr->sysparent`)[2]. This > then interacts with how `sys.parent` resolves parents when a target > frame appears more than once in the context stack. `sys.parent` > returns the oldest context that matches[3], and in this case `f`'s > frame appears twice because `evalq` adds it via `do_eval`. > > One option is to change what `sysparent` of the `evalq` `envir`. > For example, if we set it to be the same as it would be for commands > outside the `evalq` we get: > > str(res) > ## List of 4 > ## $ : int 0 # sys.parent(1) > ## $ : int 0 # evalq(sys.parent(1)) > ## $ : int 0 # evalq((function() sys.parent(2))()) > ## $ : int 1 # evalq((function() sys.parent(1))()) > > There is precedent for doing this in S3 generics and their methods > where method `sysparent` is set to be that of the generic. Now > `evalq` no longer interferes with the resolution of calling frames. > It seems reasonable to set evaluation environments without affecting > what the calling frame is. Indeed that happens when we do something like > `environment(fun) <- blah` as the calling frame is unaffected when `fun` is > invoked. > > I attach a patch that implements this change. The patch is a > hack-job intended solely for illustrative purposes, though it does > pass `make check-all` on a current version of r-devel. I also ran the > `rlang` tests as those probably push the envelope in this area. There > only one failed with 2,613 passing. The failed one is for a > deprecated function that was specifically checking for the repeated > `evalq` contexts[7]. > > I also attach a document with additional examples and commentary for > those interested. > > Best, > > Brodie. > > PS: for a loosely related issue see #15531[8]. > > [1]: > https://github.com/wch/r-source/blob/tags/R-4-0-0/src/main/eval.c#L3329 > [2]: > https://github.com/wch/r-source/blob/tags/R-4-0-0/src/main/context.c#L260 > [3]: > https://github.com/wch/r-source/blob/tags/R-4-0-0/src/main/context.c#L433 > [4]: > https://github.com/wch/r-source/blob/tags/R-4-0-0/src/main/eval.c#L1815 > [5]: https://cran.r-project.org/doc/manuals/r-devel/R-ints.html#Contexts > [6]: https://bugs.r-project.org/bugzilla/show_bug.cgi?id=15531 > [7]: > https://github.com/r-lib/rlang/blob/v0.4.6/tests/testthat/test-retired.R#L437 > [8]: https://bugs.r-project.org/bugzilla/show_bug.cgi?id=15531 > > > ______________________________________________ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel