Defer on globalenv

[jennybc]

An experiment for discussion. I'm finding this quite handy for developing tests that use withr intensely. A little top-level usage:

devtools::load_all("~/rrr/withr")
#> Loading withr

(owd <- getwd())
#> [1] "/Users/jenny/rrr/withr"

defer(setwd(owd))
#> Setting deferred event on global environment.
#> Execute (and clear) with `clear()`.

setwd(tempdir())
getwd()
#> [1] "/private/var/folders/vr/gzrbtprx6ybg85y5pvwm1ct40000gn/T/RtmpTFQmX4"

defer(print('hi'))
#> Setting deferred event on global environment.
#> Execute (and clear) with `clear()`.

clear()
#> [1] "hi"

getwd()
#> [1] "/Users/jenny/rrr/withr"

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

One unsavoury (?) aspect is that the handlers contain .Globalenv itself, because that is parent.frame() when defer() is called. This doesn't keep me from playing with this branch, but it causes infinite recursion problems when inspecting the current handlers. I assume we could address that if you consider merging this.

[jimhester]

Maybe this should be called something like run_global_deferred() or similar? clear() seems like it is the side effect rather than the main action in this case.

Maybe set the envir to parent.env(.GlobalEnv) in this case, to avoid the recursion issue?

[jennybc]

I renamed it to flush_global_deferred(), which seems to convey "run and delete" to me. OK? If this becomes reality, I would probably use a keyboard shortcut in any case.

It does not work to set the handler execution env to parent.env(.GlobalEnv). Or rather, it does for events like print("hi") but not for setwd(owd) where the correct value of owd is to be found in .GlobalEnv. Should I set the env to NULL in this case and replace such NULLs with .GlobalEnv in execute_handlers() or flush_global_deferred()?

[jennybc]

It looks like setting the handler execution env to NULL for deferred events on .Globalenv "just works".

[jennybc]

If you're feeling positive about this, I'll add a test.

[jimhester]

Maybe we should also have a clear_global_deferred() that just clears them without running?

[jimhester]

If you have to you could test these in a separate script in tests/ not using testthat. Although that still may not be run in the global environment when using devtools::test() interactively, it should be during R CMD check. (actually I guess devtools::test() only runs the tests in the testthat dir, so that might be the way to go).

[hadley]

Maybe it could just be flush_deferred(), and if executed outside of the global environment, it would run the exit handlers and then reset them? (although as I type that I suspect that that's not actually possible)

[jennybc]

Two concrete motivating examples of wrapping withr technology to make test helpers that are easier to develop with. These helpers are faking the functionality that this PR would implement properly.

usethis::scoped_temporary_thing(), where thing is "package" or "project":

https://github.com/r-lib/usethis/blob/f76f8ff2421927c9952104cfe09d6e5a31b961c3/tests/testthat/helper.R#L35

reprex::scoped_temporary_wd() still in a PR branch, but will presumably get merged:

https://github.com/tidyverse/reprex/blob/d82c214562a7c0820473de74d116b6d4f3a0673b/tests/testthat/helper.R#L11

I actually think scoped_temporary_wd() might even be useful in testthat because this is (or at least should be) a very common pattern in tests.

[jennybc]

The problem I remarked on earlier is actually more general, i.e. it's not unique to envir = .GlobalEnv. If you set an event on environment A that is destined to be evaluated in environment A, you will have difficulty inspecting these handlers, due to recursion. For example, printing while debugging is impossible.

Although it's just a development matter, seems worth improving.

[jennybc]

This is the second fix that I tried. See the commit history for a different approach that sets the environment to NULL and has special handling in execute_handlers(). I suspect this is better because it's more localized.

[hadley]

I don't think that a new environment can work with on.exit(). on.exit() doesn't operate on environments; it operates on frames in the call stack. The environment here is passed on to do.call which causes on.exit() to be run in such a way that it can find the frame from the current environment. I don't think it can do that when you create an environment de novo.

[jennybc]

If your concern holds, is it surprising that this PR passes all tests, including one that tests the global env functionality specifically? (The R-devel failure is a pre-existing documentation glitch unrelated to this PR.)

[hadley]

No, because not calling on.exit() for the globalenv() case is harmless. I don't understand the setting_on_self problem, but I doubt this is the correct fix, so I'd recommend leaving as is for this PR.

[jennybc]

I don't understand the setting_on_self problem...

Here's is a demo you can do with with master or the CRAN version. In general, you can't inspect an environment or an environment's handlers whenever it captures itself in a handler. Which is the default case.

library(withr)

local({
  defer(print("hi"))
  browser()
})

While in browser, try these commands to inspect the environment or its handlers:

environment() # Error: C stack usage  7971008 is too close to the limit
format(environment())             # works, prevents the recursion
names(attributes(environment()))  # works
attr(environment(), "handlers")   # Error: C stack usage ...

Any direct inspection of the environment or its attributes or the "handers" attribute specifically results in infinite recursion.

[jimhester]

So I finally tried this out, and I don't really think we need to worry about this recursive case. When debugging you can use str(environment(), max.level = 2) or similar to break out of the loop.

[hadley]

So the root cause of the problem is something like this:

x <- environment()
attr(x, "env") <- x
x

This feels like somewhat of a bug in R to me, and I agree with Jim that it's probably not worth fixing. (If you really did want to fix it, I think a possible approach would be to temporarily add a class to the environment and define a print method for it)

[jennybc]

If you really did want to fix it, I think a possible approach would be to temporarily add a class to the environment and define a print method for it

Ok I will revise to not worry about this.

Playing with this print method idea is what lead to my questions yesterday about how to get an environment's "memory address" style of label.

[jimhester]

Interestingly max.level seems to not be passed to attributes in str.default(), so it doesn't work for Hadley's case, though it does work in Jenny's original example. Seems like a bug in str.default() to me, it probably should have a (is.na(max.level) || nest.lev < max.level) && in the last conditional, but anyway...

[jennybc]

@jimhester I think this is ready

[jennybc]

Here's a before vs. after demo, using defer() directly and top-level usage of local_*() function, which will be helpful for test development.

---

Before

This unintentionally reveals that local_envvar() actually does change the current value of FOO, even though it shouldn't because it's going to fail to set the handler that restores the original state.

Sys.setenv(FOO = "abc")
Sys.getenv("FOO")
#> [1] "abc"

withr::local_envvar(c(FOO = "xyz"))
#> Error in defer(set_envvar(old), envir = .local_envir): attempt to defer event on global environment
Sys.getenv("FOO")
#> [1] "xyz"

withr::defer(print("howdy!"))
#> Error in withr::defer(print("howdy!")): attempt to defer event on global environment

withr::run_global_deferred()
#> Error: 'run_global_deferred' is not an exported object from 'namespace:withr'

Sys.getenv("FOO")
#> [1] "xyz"

---

After

> Sys.setenv(FOO = "abc")
> Sys.getenv("FOO")
[1] "abc"
> withr::local_envvar(c(FOO = "xyz"))
Setting deferred event(s) on global environment.
  * Execute (and clear) with `run_global_deferred()`.
  * Clear (without executing) with `clear_global_deferred()`.
  FOO 
"abc" 
> Sys.getenv("FOO")
[1] "xyz"
> withr::defer(print("howdy!"))
> withr::run_global_deferred()
[1] "howdy!"
> Sys.getenv("FOO")
[1] "abc"

I couldn't use reprex for this because knitr also has a hand in where things are being evaluated and when they take effect.

[jennybc]

I think this also has interesting implications for scripts where you want to set lots of things to non-defaults and accumulate several events to restore things as you found them. By setting them with defer(), directly or indirectly, you can fire them all at once with run_global_deferred().

I'm deleting this comment because it's huge and again brings up interactions between knitr's execution model and withr.

[hadley]

It might be worth using reg.finalizer() to ensure that the handlers on the global environment are at least run before R exits.

[jennybc]

It might be worth using reg.finalizer() to ensure that the handlers on the global environment are at least run before R exits.

This sounds a bit scary. I could imagine having events registered here that are from failed experiments. If we use reg.finalizer() whenever global env has events, the burden of explicitly clearing those events without execution falls to the user. I'd prefer to require explicit firing than explicit clearing.

[hadley]

If we use reg.finalizer() whenever global env has events, the burden of explicitly clearing those events without execution falls to the user. I'd prefer to require explicit firing than explicit clearing.

Isn't it just as dangerous to forget to call the clean up code potentially leaving things in a bad state?

[jennybc]

Isn't it just as dangerous to forget to call the clean up code potentially leaving things in a bad state?

It depends on what we expect the primary usage to be. I'm thinking of test or function development, in which case it's just a lot of experimentation and I'd prefer the default to be "don't run unless asked".

Perhaps this is the closest reference: You can call on.exit(expr) from the global environment (it's odd that this silently "succeeds", but whatever). In my experiment, expr deletes a file. When I quit R, the expr does not get executed, i.e. the file's still there.

[jennybc]

I worked against this PR in a usethis PR (r-lib/usethis#1107). All usethis tests pass and the interactive test development experience with respect to, e.g., scoped_temporary_package() and withr::local_options(), is exactly what I was hoping for.

[jennybc]

OK I have done as requested in c980b09.

I still disagree but I care more about getting this feature in general.

As it stands, if someone sets a deferred event on .GlobalEnv, they can no longer call globalenv() in the Console. I.e. they'll see the infinite recursion problem. I now realize why this came up here in the first place. It has always been a property of withr's handlers, but prior to this PR, it was really rare for a normal person, in normal life, to have such an environment in front of the their eyeballs.

I think this is done.

[jimhester]

Thanks for working on this @jennybc, and thank you @hadley for reviewing it!