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.

Sign up for GitHub

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

Add iteration index to errors #945

Merged
merged 26 commits into from
Sep 23, 2022
Merged

Add iteration index to errors #945

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
de4e7fb
Add itearation index to errors
hadley Sep 15, 2022
4e6941f
Cleaner framing; update snapshots
hadley Sep 15, 2022
d71bcc5
Only wrap errors that happen during iteration
hadley Sep 15, 2022
a5c3870
Remove now duplicated index from error
hadley Sep 15, 2022
9cafb9d
Remove redundant test
hadley Sep 15, 2022
7f69118
Switch back to Rf_errorcall()
hadley Sep 15, 2022
5b6dbef
Merge commit 'db7bd02ec0fe9ed10d98c61a96d3f1339adf8203'
hadley Sep 17, 2022
d939dda
Merged origin/main into map-errors
hadley Sep 21, 2022
00cb90a
Wrap map2() and modify()
hadley Sep 21, 2022
5a9f410
Name arguments by convention
hadley Sep 21, 2022
c01d747
Use existing i variable
hadley Sep 21, 2022
adebf3b
Use simpler function
hadley Sep 21, 2022
f2bf469
Use r_abort_call() in map.c
hadley Sep 21, 2022
8aef34f
Use consistent message for map2 recycling
hadley Sep 21, 2022
8ec3eca
Add news bullet
hadley Sep 21, 2022
e54cf3e
WS
hadley Sep 21, 2022
33029e7
Avoid coercion
hadley Sep 21, 2022
02416fa
Match new dplyr errors
hadley Sep 21, 2022
6d53304
Add missing full stops
hadley Sep 21, 2022
a96cef8
Use explicit caller_env argument
hadley Sep 22, 2022
f2961ec
Tweak with_indexed_errors
hadley Sep 22, 2022
5f0d5b9
Use explicit error_call in predicate using functions
hadley Sep 22, 2022
b4c9e93
Use obj_type_friendly in coerce.c
hadley Sep 22, 2022
2e7e817
Add missing full stop
hadley Sep 22, 2022
a8b56ad
Merge commit 'b1b446f850d8bfbc45c312b90724df83317b3b33'
hadley Sep 22, 2022
8d5f13e
Better argument order
hadley Sep 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@
earlier so in those versions of R, the examples are automatically converted
to a regular section with a note that they might not work (#936).

* When map functions fail, they now report the element they failed at (#945).

### Flattening and simplification

* New `list_c()`, `list_rbind()`, and `list_cbind()` make it easy to
Expand Down
9 changes: 6 additions & 3 deletions R/map-raw.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ map_raw <- function(.x, .f, ...) {
lifecycle::deprecate_soft("1.0.0", "map_raw()", "map_vec()")

.f <- as_mapper(.f, ...)
.Call(map_impl, environment(), ".x", ".f", "raw", FALSE)
i <- 0L
.Call(map_impl, environment(), "raw", FALSE)
}

#' @export
Expand All @@ -24,7 +25,8 @@ map2_raw <- function(.x, .y, .f, ...) {
}
map2_raw_ <- function(.x, .y, .f, ...) {
.f <- as_mapper(.f, ...)
.Call(map2_impl, environment(), ".x", ".y", ".f", "raw", FALSE)
i <- 0L
.Call(map2_impl, environment(), "raw", FALSE)
}

#' @rdname map_raw
Expand All @@ -46,7 +48,8 @@ pmap_raw <- function(.l, .f, ...) {
.l <- as.list(.l)
}

.Call(pmap_impl, environment(), ".l", ".f", "raw", FALSE)
i <- 0L
.Call(pmap_impl, environment(), "raw", FALSE)
}

#' @export
Expand Down
43 changes: 38 additions & 5 deletions R/map.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,35 +113,50 @@
#' map_dbl("r.squared")
map <- function(.x, .f, ..., .progress = FALSE) {
.f <- as_mapper(.f, ...)
.Call(map_impl, environment(), ".x", ".f", "list", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(map_impl, environment(), "list", .progress)
)
}

#' @rdname map
#' @export
map_lgl <- function(.x, .f, ..., .progress = FALSE) {
.f <- as_mapper(.f, ...)
.Call(map_impl, environment(), ".x", ".f", "logical", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(map_impl, environment(), "logical", .progress)
)
}

#' @rdname map
#' @export
map_int <- function(.x, .f, ..., .progress = FALSE) {
.f <- as_mapper(.f, ...)
.Call(map_impl, environment(), ".x", ".f", "integer", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(map_impl, environment(), "integer", .progress)
)
}

#' @rdname map
#' @export
map_dbl <- function(.x, .f, ..., .progress = FALSE) {
.f <- as_mapper(.f, ...)
.Call(map_impl, environment(), ".x", ".f", "double", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(map_impl, environment(), "double", .progress)
)
}

#' @rdname map
#' @export
map_chr <- function(.x, .f, ..., .progress = FALSE) {
.f <- as_mapper(.f, ...)
.Call(map_impl, environment(), ".x", ".f", "character", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(map_impl, environment(), "character", .progress)
)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#' @rdname map
Expand All @@ -160,3 +175,21 @@ walk <- function(.x, .f, ..., .progress = FALSE) {
map(.x, .f, ..., .progress = .progress)
invisible(.x)
}

with_indexed_errors <- function(expr, i, error_call = caller_env()) {
withCallingHandlers(
expr,
error = function(cnd) {
if (i == 0L) {
# error happened before or after loop
hadley marked this conversation as resolved.
Show resolved Hide resolved
cnd_signal(cnd)
hadley marked this conversation as resolved.
Show resolved Hide resolved
} else {
cli::cli_abort(
"Can't compute index {i}.",
hadley marked this conversation as resolved.
Show resolved Hide resolved
parent = cnd,
call = error_call
)
}
}
)
}
25 changes: 20 additions & 5 deletions R/map2.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,31 +32,46 @@
#' map2(mods, by_cyl, predict)
map2 <- function(.x, .y, .f, ..., .progress = FALSE) {
.f <- as_mapper(.f, ...)
.Call(map2_impl, environment(), ".x", ".y", ".f", "list", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(map2_impl, environment(), "list", .progress)
)
}
#' @export
#' @rdname map2
map2_lgl <- function(.x, .y, .f, ..., .progress = FALSE) {
.f <- as_mapper(.f, ...)
.Call(map2_impl, environment(), ".x", ".y", ".f", "logical", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(map2_impl, environment(), "logical", .progress)
)
}
#' @export
#' @rdname map2
map2_int <- function(.x, .y, .f, ..., .progress = FALSE) {
.f <- as_mapper(.f, ...)
.Call(map2_impl, environment(), ".x", ".y", ".f", "integer", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(map2_impl, environment(), "integer", .progress)
)
}
#' @export
#' @rdname map2
map2_dbl <- function(.x, .y, .f, ..., .progress = FALSE) {
.f <- as_mapper(.f, ...)
.Call(map2_impl, environment(), ".x", ".y", ".f", "double", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(map2_impl, environment(), "double", .progress)
)
}
#' @export
#' @rdname map2
map2_chr <- function(.x, .y, .f, ..., .progress = FALSE) {
.f <- as_mapper(.f, ...)
.Call(map2_impl, environment(), ".x", ".y", ".f", "character", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(map2_impl, environment(), "character", .progress)
)
}

#' @rdname map2
Expand Down
26 changes: 20 additions & 6 deletions R/pmap.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ pmap <- function(.l, .f, ..., .progress = FALSE) {
.l <- as.list(.l)
}

.Call(pmap_impl, environment(), ".l", ".f", "list", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(pmap_impl, environment(), "list", .progress)
)
}

#' @export
Expand All @@ -91,7 +94,10 @@ pmap_lgl <- function(.l, .f, ..., .progress = FALSE) {
.l <- as.list(.l)
}

.Call(pmap_impl, environment(), ".l", ".f", "logical", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(pmap_impl, environment(), "logical", .progress)
)
}
#' @export
#' @rdname pmap
Expand All @@ -101,7 +107,10 @@ pmap_int <- function(.l, .f, ..., .progress = FALSE) {
.l <- as.list(.l)
}

.Call(pmap_impl, environment(), ".l", ".f", "integer", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(pmap_impl, environment(), "integer", .progress)
)
}
#' @export
#' @rdname pmap
Expand All @@ -111,7 +120,10 @@ pmap_dbl <- function(.l, .f, ..., .progress = FALSE) {
.l <- as.list(.l)
}

.Call(pmap_impl, environment(), ".l", ".f", "double", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(pmap_impl, environment(), "double", .progress)
)
}
#' @export
#' @rdname pmap
Expand All @@ -121,7 +133,10 @@ pmap_chr <- function(.l, .f, ..., .progress = FALSE) {
.l <- as.list(.l)
}

.Call(pmap_impl, environment(), ".l", ".f", "character", .progress)
i <- 0L
with_indexed_errors(i = i,
.Call(pmap_impl, environment(), "character", .progress)
)
}

#' @export
Expand All @@ -133,7 +148,6 @@ pmap_vec <- function(.l, .f, ..., .ptype = NULL, .progress = FALSE) {
simplify_impl(out, ptype = .ptype)
}


#' @export
#' @rdname pmap
pwalk <- function(.l, .f, ..., .progress = FALSE) {
Expand Down
4 changes: 2 additions & 2 deletions src/coerce.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
#include <stdio.h>

void cant_coerce(SEXP from, SEXP to, int i) {
Rf_errorcall(R_NilValue, "Can't coerce element %i from a %s to a %s",
i + 1, Rf_type2char(TYPEOF(from)), Rf_type2char(TYPEOF(to)));
Rf_errorcall(R_NilValue, "Can't coerce from a %s to a %s.",
Rf_type2char(TYPEOF(from)), Rf_type2char(TYPEOF(to)));
}

int real_to_logical(double x, SEXP from, SEXP to, int i) {
Expand Down
34 changes: 25 additions & 9 deletions src/conditions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,9 @@ SEXP caller_env() {
return out;
}

#define BUFSIZE 8192
void r_abort(const char* fmt, ...) {
char buf[BUFSIZE];
va_list dots;
va_start(dots, fmt);
vsnprintf(buf, BUFSIZE, fmt, dots);
va_end(dots);
buf[BUFSIZE - 1] = '\0';

void r_abort0(SEXP env, char* buf) {
SEXP message = PROTECT(Rf_mkString(buf));
SEXP env = PROTECT(caller_env());

SEXP fn = PROTECT(
Rf_lang3(Rf_install("::"), Rf_install("rlang"), Rf_install("abort"))
Expand All @@ -45,6 +37,30 @@ void r_abort(const char* fmt, ...) {
while (1); // No return
}

#define BUFSIZE 8192
void r_abort(const char* fmt, ...) {
char buf[BUFSIZE];
va_list dots;
va_start(dots, fmt);
vsnprintf(buf, BUFSIZE, fmt, dots);
va_end(dots);
buf[BUFSIZE - 1] = '\0';

SEXP env = PROTECT(caller_env());
r_abort0(env, buf);
}

void r_abort_call(SEXP env, const char* fmt, ...) {
hadley marked this conversation as resolved.
Show resolved Hide resolved
char buf[BUFSIZE];
va_list dots;
va_start(dots, fmt);
vsnprintf(buf, BUFSIZE, fmt, dots);
va_end(dots);
buf[BUFSIZE - 1] = '\0';

r_abort0(env, buf);
}

const char* rlang_obj_type_friendly_full(SEXP x, bool value, bool length) {
const char* (*rlang_ptr)(SEXP x, bool value, bool length) = NULL;
if (rlang_ptr == NULL) {
Expand Down
1 change: 1 addition & 0 deletions src/conditions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ void __attribute__ ((noreturn)) stop_bad_element_type(SEXP x, R_xlen_t index, co
void __attribute__ ((noreturn)) stop_bad_element_length(SEXP x, R_xlen_t index, R_xlen_t expected_length, const char* what, const char* arg, bool recycle) __attribute__((noreturn));
SEXP caller_env();
void __attribute__ ((noreturn)) r_abort(const char* fmt, ...);
void __attribute__ ((noreturn)) r_abort_call(SEXP env, const char* fmt, ...);

const char* rlang_obj_type_friendly_full(SEXP x, bool value, bool length);

Expand Down
12 changes: 6 additions & 6 deletions src/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ SEXP purrr_init_library(SEXP);
extern SEXP coerce_impl(SEXP, SEXP);
extern SEXP pluck_impl(SEXP, SEXP, SEXP, SEXP, SEXP);
extern SEXP flatten_impl(SEXP);
extern SEXP map_impl(SEXP, SEXP, SEXP, SEXP, SEXP);
extern SEXP map2_impl(SEXP, SEXP, SEXP, SEXP, SEXP, SEXP);
extern SEXP pmap_impl(SEXP, SEXP, SEXP, SEXP, SEXP);
extern SEXP map_impl(SEXP, SEXP, SEXP);
extern SEXP map2_impl(SEXP, SEXP, SEXP);
extern SEXP pmap_impl(SEXP, SEXP, SEXP);
extern SEXP transpose_impl(SEXP, SEXP);
extern SEXP vflatten_impl(SEXP, SEXP);

Expand All @@ -25,9 +25,9 @@ static const R_CallMethodDef CallEntries[] = {
{"coerce_impl", (DL_FUNC) &coerce_impl, 2},
{"pluck_impl", (DL_FUNC) &pluck_impl, 5},
{"flatten_impl", (DL_FUNC) &flatten_impl, 1},
{"map_impl", (DL_FUNC) &map_impl, 5},
{"map2_impl", (DL_FUNC) &map2_impl, 6},
{"pmap_impl", (DL_FUNC) &pmap_impl, 5},
{"map_impl", (DL_FUNC) &map_impl, 3},
{"map2_impl", (DL_FUNC) &map2_impl, 3},
{"pmap_impl", (DL_FUNC) &pmap_impl, 3},
{"transpose_impl", (DL_FUNC) &transpose_impl, 2},
{"vflatten_impl", (DL_FUNC) &vflatten_impl, 2},
{"purrr_eval", (DL_FUNC) &Rf_eval, 2},
Expand Down
Loading