Issues with handlers attribute

[lionel-]

Using defer() in the execution env causes a self-reference that may cause undefined behaviour (may just need a documentation note to recommend deferring expressions in private environments):

local({
  withr::defer(NULL)
  print(environment())
})
#> *infloop*

local_ functions cause side effects in arbitrary environments:

local(envir = asNamespace("rlang"), {
  withr::local_options(list())
})

names(attributes(asNamespace("rlang")))
#> [1] "handlers"

Deferring expressions in a non-top-level global frame causes the message about deferred events to be printed, even though the on-exit expressions are correctly unwound.

options(foo = FALSE)

local(envir = globalenv(), {
  withr::local_options(list(foo = TRUE))
})
#> Setting deferred event(s) on global environment.
#>   * Execute (and clear) with `deferred_run()`.
#>   * Clear (without executing) with `deferred_clear()`.

getOption("foo")
#> [1] FALSE

The handler attributes lingers around despite the handler having been called:

attr(globalenv(), "handler")
#> [[1]]
#> [[1]]$expr
#> reset_options(old)
#>
#> [[1]]$envir
#> <environment: 0x7fc2d2fa98c0>

I would expect no message in this case and the handler attribute to be cleaned up after execution. A non-top-level global frame can be detected with:

is_global_frame <- sys.parents() == 0

# TRUE if top-level
sum(is_global_frame) == 1

[jimhester]

@jennybc brought up this problem in #76 as well

[jennybc]

It feels like the self-reference is correct and inevitable. But it's really a bug or missing feature in how we can inspect deferred events. And yes I experienced and explored this thoroughly in #76, specifically #76 (comment). I even wrote some workarounds but ultimately those did not fly.

Deferring expressions in a non-top-level global frame causes the message about deferred events to be printed, even though the on-exit expressions are correctly unwound.

This seems like a very special edge case. The ability to defer on the global env was really meant as a development nicety and targets the regular, top-level global env. But I have mentioned to @jimhester that perhaps deferred_run() (and presumably deferred_clear()) should also emit a message, as a bookend to the message at defer-time. If that were true, your example would make more sense, i.e. you'd see a message about deferring AND one about running/clearing.

Your other points about side effects feel like a separate issue to me.

[lionel-]

I explored an alternative implementation that avoids the side effect in r-lib/rlang#999. However it needs a fix in base::sys.on.exit() that was implemented in R 3.5, so we can't use this approach until we drop R 3.4 support. This would fix the self reference issue as well as the side effects.

This seems like a very special edge case.

It may arise in unit tests. More generally, it comes up with source() and knit(), though in this case the stack-based clean up may not work as expected because of the iterated evaluation.

[lionel-]

The first issue can be solved later. I think the other two issues can be fixed (or attenuated) by:

• Removing the handlers attribute from the environment once the handlers have all been executed. This will prevent old handlers from being run again when an environment is put on the stack multiple times.

• Don't show the message unless there is only one global-env on the stack.

[jennybc]

Just to make sure I understand:

Don't show the message unless there is only one global-env on the stack.

There is only one global env. But you're talking about a call stack where global env appears more than once in the chain of environments? I had never considered such a situation.

[lionel-]

Yes this will happen in a number of situtations similar to:

options(foo = FALSE)

local(envir = globalenv(), {
  withr::local_options(list(foo = TRUE))
})
#> Setting deferred event(s) on global environment.
#>   * Execute (and clear) with `deferred_run()`.
#>   * Clear (without executing) with `deferred_clear()`.

# Already cleaned up
getOption("foo")
#> [1] FALSE

I think the behaviour is correct but there should be no message and the handlers attribute should be cleaned from the global env after execution. The global env is not the only environment that would benefit from cleaning up the handlers attribute as shown in:

local(envir = asNamespace("rlang"), {
  withr::local_options(list())
})

names(attributes(asNamespace("rlang")))
#> [1] "handlers"

[lionel-]

Minor: probably best to prefix the attribute with a namespace, e.g. withr_handlers.

One practical reason for this is that these attributes show up in sys.calls() and will confuse people who don't know about withr internals in debugging sessions.

[lionel-]

I'm happy to prepare a PR if you agree with these solutions by the way.

[lionel-]

Another (minor) problem with the list of attributes, is that it uses a single list for potentially multiple frames.

Here is an example where the same environment is used in multiple frames, each having their own stack of exit expressions:

evalq({
  cat("1\n")
  on.exit(cat("foo\n"))
  cat("2\n")
  evalq(on.exit(cat("bar\n")))
  cat("3\n")
})
#> 1
#> 2
#> bar
#> 3
#> foo

This doesn't work correctly with defer():

evalq({
  cat("1\n")
  defer(cat("foo\n"))
  cat("2\n")
  evalq(defer(cat("bar\n")))
  cat("3\n")
})
#> 1
#> 2
#> 3
#> bar
#> foo