tidyverse / purrr

A functional programming toolkit for R
https://purrr.tidyverse.org/
Other
1.28k stars 272 forks source link

Find a solution for the inverted chained error problem #1022

Closed DavisVaughan closed 1 year ago

DavisVaughan commented 1 year ago

In https://github.com/tidyverse/tidyr/pull/1430 we had an issue where the chained error of map() results in a confusing error message.

It seems like any case where we need to pass error_call through to .f can result in an "inverted" chained error. Really fn() is the one calling map() here, but it is reported like map() called fn(), which is quite confusing.

The current hack is to avoid map() altogether and switch back to lapply(), but that is obviously not great.

We might need something like with_no_indexed_errors() that we can wrap the map() with to turn off the error chaining. In the tidyr case I don't think we'd ever want to report the index of the failure, and we wouldn't want map() mentioned in the error at all.

library(purrr)
library(rlang)

fn_one <- function(x, error_call = caller_env()) {
  if (x == 3L) {
    abort("Something is wrong.", call = error_call)
  }

  x
}

fn <- function(x) {
  error_call <- current_env()
  map(x, \(.x) fn_one(.x, error_call = error_call))
}

fn(1:5)
#> Error in `map()`:
#> ℹ In index: 3.
#> Caused by error in `fn()`:
#> ! Something is wrong.

#> Backtrace:
#>     ▆
#>  1. └─global fn(1:5)
#>  2.   └─purrr::map(x, function(.x) fn_one(.x, error_call = error_call))
#>  3.     └─purrr:::map_("list", .x, .f, ..., .progress = .progress)
#>  4.       ├─purrr:::with_indexed_errors(...)
#>  5.       │ └─base::withCallingHandlers(...)
#>  6.       └─.f(.x[[i]], ...)
#>  7.         └─global fn_one(.x, error_call = error_call)
#>  8.           └─rlang::abort("Something is wrong.", call = error_call)

With lapply():

fn2 <- function(x) {
  error_call <- current_env()
  lapply(x, \(.x) fn_one(.x, error_call = error_call))
}

fn2(1:5)
#> Error in `fn2()`:
#> ! Something is wrong.

#> Backtrace:
#>     ▆
#>  1. └─global fn2(1:5)
#>  2.   └─base::lapply(x, function(.x) fn_one(.x, error_call = error_call))
#>  3.     └─FUN(X[[i]], ...)
#>  4.       └─global fn_one(.x, error_call = error_call)
#>  5.         └─rlang::abort("Something is wrong.", call = error_call)
hadley commented 1 year ago

To solve this problem we need something like:

with_indexed_errors <- function(expr, i, error_call = caller_env()) {
  if (something) {
    expr
  } else {
    withCallingHandlers(
      expr,
      error = function(cnd) {
        if (i == 0L) {
          # Error happened before or after loop
        } else {
          cli::cli_abort(
            c(i = "In index: {i}."),
            parent = cnd,
            call = error_call,
            class = "purrr_error_iteration"
          )
        }
      }
    )
  }
}

But what should something be? I think there are three possibilities:

Since it seems like map() and friends are going to need error_call anyway, the second option seems the most parsimonious (even though it introduces a new error call pattern that we haven't used before).

OTOH if you want an error to come from map(), the chances are that you'll want to customise "In index {i}" to something problem specific, so maybe error_call isn't that generally useful for map() functions?

hadley commented 1 year ago

It just occurred to me that there's another approach: deliberately unwrap the error.

with_no_indexed_errors <- function(expr) {
  tryCatch(expr, 
    purrr_indexed_error = function(err) stop(err$parent)
  )
}

This is probably the sort of wrapping we'd want to do in tidyr, so we can change from "index {i}" to "column {col}".