Don't catch errors in Stat computations

[thomasp85]

Currently any error from a Stat's compute_*() method is caught and converted to a warning. This makes debugging difficult as the backtrace is lost and you cannot recover() at the correct location. The reason for the design is (according to @hadley) that stat_smooth() may fail in some panels but not all and this should not throw an exception but simply not plot the layer in that panel.

This issue is to discuss whether we should move this responsibility down to each Stat and allow errors to bubble up in the expected way. I am (as may be clear) in favour of letting Stats fail clearly by default, but the change may very well be breaking in unexpected ways that we need to consider.

[yutannihilation]

What about making Stat raise a custom error so that ggplot2 can decide whether to fall it back to an warning or not? I think catching and wrapping the error with a friendly message "computation failed:" is still useful. Raw backtrace is enough for us, but a user might need a better error message.

[thomasp85]

But if it is only really for the benefit of a single Stat it could just as well be caught there, right?

I don't think I really understand what you are suggesting as an alternative...

[yutannihilation]

Sorry, I misunderstood. I was thinking about a possible convention about the error between compute_group() and compute_layer() something like:

StatFoo <- ggproto(...,
  ...,
  compute_group = function(...) {
    ...
    if (something_is_wrong) {
      abort("(some useful message)", "ggplot2_stat_unexpected_error")
    }
  }
)

tryCatch(do.call(self$compute_panel, args),
  ggplot2_stat_unexpected_error = function(e) {
    # errors from Stats that follow the convention comes here
    abort(glue("Computation failed"), parent = e) # NOTE: this won't work because this will be caught by `error` condition below...
  },
  error = function(e) {
    # errors from legacy Stats come here
    warn(glue("Computation failed:\n", e$message))
    new_data_frame()
  }
)

But, I found, since the unexpected error is unexpected, custom errors are not very useful here...

[yutannihilation]

Sorry, I went off the topic. Basically, I feel it's a good idea to stop catching errors in compute_layer().

[yutannihilation]

This makes debugging difficult as the backtrace is lost

I think this point was already resolved magically by #4856, so I think we can close this issue. Please reopen if it's still worth to consider making it fail.