Add iteration index to errors

[hadley]

Fixes #929

So far, just a quick proof of concept

To do:

• Wrap map2 and pmap functions
• Update C code as below
• Pass explicit env to check_vector()/r_abort() or move that check into R
• News bullet

[hadley]

This is verbose, but it's technically correct and possibly helpful.

[lionel-]

Probably fine for this particular function. I'm expecting this sort of errors to be internal errors rather than user errors.

[DavisVaughan]

Noting that when all of the map friends are converted over to use this, on the C side we shouldn't create and overwrite i like this:

purrr/src/map.c

Lines 39 to 42 in 426acdd

	// Create variable "i" and map to scalar integer
	SEXP i_val = PROTECT(Rf_ScalarInteger(1));
	SEXP i = Rf_install("i");
	Rf_defineVar(i, i_val, env);

Since it already exists in the function environment through i <- 0L, we should use something like Rf_findVarInFrame() to look it up and modify it directly

[DavisVaughan]

Can we improve on these two cases?

> map(-1:1, stop)
Error in `map()`:
! Computation failed in index 1
Caused by error in `withCallingHandlers()`:
! -1
Run `rlang::last_trace()` to see where the error occurred.

fn <- function(x) {
  if (x == 2) {
    rlang::abort("oh no")
  }
  x
}

map(-1:3, fn)
Error in `map()`:
! Computation failed in index 4
Caused by error in `.f()`:
! oh no
Run `rlang::last_trace()` to see where the error occurred.

[hadley]

@DavisVaughan I think our conclusion was that we can't easily improve on that, right?

library(purrr)

# call. is TRUE by default
map(-1:1, stop)
#> Error in `map()`:
#> ! Computation failed in index 1
#> Caused by error in `withCallingHandlers()`:
#> ! -1

map("a", rlang::abort)
#> Error in `map()`:
#> ! Computation failed in index 1
#> Caused by error in `map()`:
#> ! a

# have lost original name of function
fn <- function(x) {
  if (x == 2) {
    rlang::abort("oh no")
  }
  x
}
map(-1:3, fn)
#> Error in `map()`:
#> ! Computation failed in index 4
#> Caused by error in `.f()` at purrr/R/map.R:117:2:
#> ! oh no

map(-1:3, \(x) fn(x))
#> Error in `map()`:
#> ! Computation failed in index 4
#> Caused by error in `fn()`:
#> ! oh no

^{Created on 2022-09-21 with reprex v2.0.2}

[lionel-]

Looks good! But I think it'd make sense to think about call arguments to take ownership of map errors, as argued inline.

[lionel-]

Probably fine for this particular function. I'm expecting this sort of errors to be internal errors rather than user errors.

[DavisVaughan]

It is still pretty unfortunate that we get parent call information that mentions map_()

library(vctrs)
library(purrr)

map(1:5, vec_check_list)
#> Error in `map()`:
#> ! In index 1.
#> Caused by error in `map_()` at purrr/R/map.R:115:2:
#> ! `.x[[i]]` must be a list, not an integer.

It seems rare that the parent call is actually useful. I had mentioned that we might consider setting cnd$call <- NULL or hardcoding it to .f. Is that still an option? I feel like it would generally be less confusing because we'd have less chance of displaying confusing calls.

So it would look like:

> map(1:5, vec_check_list)
Error in `map()`:
! In index 1.
Caused by error:
! `.x[[i]]` must be a list, not an integer.
Run `rlang::last_trace()` to see where the error occurred.

[hadley]

If I strip the call from the error, I definitely lose useful information in the tests. So I think in most cases it is helpful, although it doesn't work so well with error checking functions. And you can already fix that if needed:

library(purrr)

my_f <- function() {
  map(1:5, \(x) vctrs::vec_check_list(x, call = NULL))
}
my_f()
#> Error in `map()`:
#> ! Computation failed in index 1
#> Caused by error:
#> ! `x` must be a list, not an integer.

^{Created on 2022-09-23 with reprex v2.0.2}

[DavisVaughan]

Suggested change

	if (!Rf_isVectorList(l_val)) {
	if (TYPEOF(l_val) != VECSXP) {

Rf_isVectorList() allows EXPRSXP for some reason and I feel like we don't want that. Does that feel right @lionel- ? https://github.com/wch/r-source/blob/79298c499218846d14500255efd622b5021c10ec/src/include/Rinlinedfuns.h#L851