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

with_handlers doesn't preserve handler order #718

Closed
burchill opened this issue Jan 16, 2019 · 6 comments
Closed

with_handlers doesn't preserve handler order #718

burchill opened this issue Jan 16, 2019 · 6 comments
Labels
Milestone

Comments

@burchill
Copy link
Contributor

@burchill burchill commented Jan 16, 2019

For withCallingHandlers and tryCatch, conditions are checked by the applicable handlers in the order in which they were specified. E.g., with the following handlers, a warning message would be be checked by both handlers:

# prints "CONDITION", then "WARNING"
withCallingHandlers(warning("woops!"),
              condition = function(x) print("CONDITION"),
              warning = function(x) {print("WARNING"); cnd_muffle(x)})

However, with_handlers only respects order within exiting/calling classes. The following only checks the warning handler:

# prints only "WARNING"
with_handlers(warning("woops!"),
              condition = calling(function(x) print("CONDITION")),
              warning = exiting(function(x) { print("WARNING")}))

Would it not make sense to change this arguably unintuitive arrangement? Letting users specify the order the handlers are checked would give them more control and make input more consistent with other base condition-handling code. It seems like the following code could do the same thing but would preserve order:

with_handlers <- function(.expr, ...) {
  handlers <- list2(...)
  is_calling <- map_lgl(handlers, inherits, "rlang_box_calling_handler")
  handlers <- map_if(handlers, is_calling, unbox)
  handlers <- map(handlers, as_function)
  expr <- quote(.expr)
  
  for (i in 1:length(handlers)) {
    handler <- handlers[i]
    if (is_calling[[i]]) 
      expr <- expr(withCallingHandlers(!!expr, !!!handler))
    else
      expr <- expr(tryCatch(!!expr, !!!handler))
  }
  .Call(rlang_eval, expr, environment())
}

Are there any downsides to doing it this way?

@lionel-
Copy link
Member

@lionel- lionel- commented Jan 16, 2019

I think the calling handlers should all come first. But it looks like they currently come last!

@burchill
Copy link
Contributor Author

@burchill burchill commented Jan 17, 2019

That does make more sense I'll admit, but I still feel like letting the users control the order makes the most. (But this ain't my package, in the end)

@lionel-
Copy link
Member

@lionel- lionel- commented Jan 17, 2019

Adding calling handlers outside of exiting handlers doesn't make sense because they will never be called.

@burchill
Copy link
Contributor Author

@burchill burchill commented Jan 17, 2019

That's only true within a particular condition class. Let's say you wanted to muffle everything that wasn't a simpleWarning, which you would want to exit for, using an "all callers first" approach.

First, as far as I know, you actually couldn't do that without a bunch of janky workarounds / having two handlers with the same name. But second, even if you could, it would require code something like this:

with_handlers(expr,
  interrupt = calling(cnd_muffle), 
  message = calling(cnd_muffle), 
  warning = calling(cnd_muffle), # Technically would block `simpleWarning`, making it impossible
  simpleWarning = exiting(...)
)

Checking handlers in order (regardless of calling/exiting type) would 1) actually let you do it and 2) use much less code:

with_handlers(expr,
  simpleWarning = exiting(...),
  condition = calling(cnd_muffle)
)

Now that I think about it, one actually faces similar hard limitations with the current "all exiting handlers first" approach. Imagine the inverse of the above--that you wanted to exit for every condition that wasn't a simpleWarning, but muffle those. I don't think you could do it with the current set-up without major hacks.

Given these scenarios, I actually believe more strongly that evaluating them in order is the right way to go. It's consistent with previous code, gives more control to the user (in an intuitive way), and avoids the hard limitations of the alternatives, all with having no discernible downsides!

@lionel-
Copy link
Member

@lionel- lionel- commented Jan 17, 2019

you actually couldn't do that without a bunch of janky workarounds

Handlers are ordinary functions. Just write a new handler that conditionally calls cnd_muffle().

all with having no discernible downsides

You're forgetting complexity of the implementation. You don't want to rewrap in withCallingHandlers / tryCatch if not necessary, for performance and cleanliness of backtraces. I would consider an implementation that avoids unnecessary wrappings.

gives more control to the user (in an intuitive way),

I wouldn't say that the evaluation timing of handlers is intuitive. It's easy to forget/overlook that calling handlers have to be on the inside to be able to recover an error. Your proposed UI might be more flexible for expert users, but less usable for casual users.

@burchill
Copy link
Contributor Author

@burchill burchill commented Jan 18, 2019

Handlers are ordinary functions. Just write a new handler that conditionally calls cnd_muffle().

Yeah, that's fair. Good point.

You're forgetting complexity of the implementation. You don't want to rewrap in withCallingHandlers / tryCatch if not necessary, for performance and cleanliness of backtraces. I would consider an implementation that avoids unnecessary wrappings.

Yeah, that's what I was asking about when I was wondering about downsides. Theoretically, it could be wrapped a lot (if there were a lot of handlers), and I had the feeling it might do something screwy in that regard. Thanks!

@lionel- lionel- added the cnd label Jan 22, 2019
@lionel- lionel- added this to the 0.4.0 milestone Jun 11, 2019
@lionel- lionel- closed this in 772985b Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants