From de4e7fba05d4c3a38222ce2884c7de8b6caca568 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Sep 2022 12:18:48 -0500 Subject: [PATCH 01/23] Add itearation index to errors Fixes #929 --- R/map.R | 30 +++++++++++++++++++++++++----- tests/testthat/_snaps/map.md | 6 ++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/R/map.R b/R/map.R index 50b02a72..d2767cf0 100644 --- a/R/map.R +++ b/R/map.R @@ -108,35 +108,35 @@ #' map_dbl("r.squared") map <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - .Call(map_impl, environment(), ".x", ".f", "list", .progress) + call_mapper(map_impl, environment(), ".x", ".f", "list", .progress = .progress) } #' @rdname map #' @export map_lgl <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - .Call(map_impl, environment(), ".x", ".f", "logical", .progress) + call_mapper(map_impl, environment(), ".x", ".f", "logical", .progress = .progress) } #' @rdname map #' @export map_chr <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - .Call(map_impl, environment(), ".x", ".f", "character", .progress) + call_mapper(map_impl, environment(), ".x", ".f", "character", .progress = .progress) } #' @rdname map #' @export map_int <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - .Call(map_impl, environment(), ".x", ".f", "integer", .progress) + call_mapper(map_impl, environment(), ".x", ".f", "integer", .progress = .progress) } #' @rdname map #' @export map_dbl <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - .Call(map_impl, environment(), ".x", ".f", "double", .progress) + call_mapper(map_impl, environment(), ".x", ".f", "double", .progress = .progress) } #' @rdname map @@ -145,3 +145,23 @@ walk <- function(.x, .f, ...) { map(.x, .f, ...) invisible(.x) } + +call_mapper <- function(callable, + env, + ..., + .progress = FALSE, + .error_call = caller_env()) { + force(.error_call) + .progress <- .progress %||% NULL + + try_fetch( + .Call(callable, env, ..., .progress), + error = function(cnd) { + cli::cli_abort( + "Computation failed in index {env$i}", + parent = cnd, + call = .error_call + ) + } + ) +} diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index 2f024547..e90d05e1 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -4,6 +4,8 @@ map(environment(), identity) Condition Error in `map()`: + ! Computation failed in index + Caused by error in `withCallingHandlers()`: ! `.x` must be a vector, not an environment. --- @@ -12,6 +14,8 @@ map(quote(a), identity) Condition Error in `map()`: + ! Computation failed in index + Caused by error in `withCallingHandlers()`: ! `.x` must be a vector, not a symbol. # error message follows style guide when result is not length 1 @@ -20,5 +24,7 @@ purrr::map_int(x, "a") Condition Error in `purrr::map_int()`: + ! Computation failed in index 2 + Caused by error in `withCallingHandlers()`: ! Result 2 must have length 1, not 2. From 4e6941f22c731c6d3a7f1a13c7fe6976d749054c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Sep 2022 13:15:17 -0500 Subject: [PATCH 02/23] Cleaner framing; update snapshots --- R/map.R | 40 ++++++++++++++++++------------ src/map.c | 2 +- tests/testthat/_snaps/keep.md | 8 ++++-- tests/testthat/_snaps/map-depth.md | 8 +++++- tests/testthat/_snaps/map-if-at.md | 4 ++- tests/testthat/_snaps/map.md | 30 +++++++++++++++++++--- tests/testthat/_snaps/map2.md | 2 +- tests/testthat/_snaps/modify.md | 8 ++++-- tests/testthat/test-map.R | 12 +++++++++ 9 files changed, 87 insertions(+), 27 deletions(-) diff --git a/R/map.R b/R/map.R index d2767cf0..ef05684b 100644 --- a/R/map.R +++ b/R/map.R @@ -108,35 +108,50 @@ #' map_dbl("r.squared") map <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - call_mapper(map_impl, environment(), ".x", ".f", "list", .progress = .progress) + i <- 0 + with_indexed_errors(i = i, + .Call(map_impl, environment(), ".x", ".f", "list", .progress) + ) } #' @rdname map #' @export map_lgl <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - call_mapper(map_impl, environment(), ".x", ".f", "logical", .progress = .progress) + i <- 0 + with_indexed_errors(i = i, + .Call(map_impl, environment(), ".x", ".f", "logical", .progress) + ) } #' @rdname map #' @export map_chr <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - call_mapper(map_impl, environment(), ".x", ".f", "character", .progress = .progress) + i <- 0 + with_indexed_errors(i = i, + .Call(map_impl, environment(), ".x", ".f", "character", .progress) + ) } #' @rdname map #' @export map_int <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - call_mapper(map_impl, environment(), ".x", ".f", "integer", .progress = .progress) + i <- 0 + with_indexed_errors(i = i, + .Call(map_impl, environment(), ".x", ".f", "integer", .progress) + ) } #' @rdname map #' @export map_dbl <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - call_mapper(map_impl, environment(), ".x", ".f", "double", .progress = .progress) + i <- 0 + with_indexed_errors(i = i, + .Call(map_impl, environment(), ".x", ".f", "double", .progress) + ) } #' @rdname map @@ -146,21 +161,14 @@ walk <- function(.x, .f, ...) { invisible(.x) } -call_mapper <- function(callable, - env, - ..., - .progress = FALSE, - .error_call = caller_env()) { - force(.error_call) - .progress <- .progress %||% NULL - +with_indexed_errors <- function(expr, i, error_call = caller_env()) { try_fetch( - .Call(callable, env, ..., .progress), + expr, error = function(cnd) { cli::cli_abort( - "Computation failed in index {env$i}", + "Computation failed in index {i}", parent = cnd, - call = .error_call + call = error_call ) } ) diff --git a/src/map.c b/src/map.c index e91cf077..b5b94b5a 100644 --- a/src/map.c +++ b/src/map.c @@ -53,7 +53,7 @@ SEXP call_loop(SEXP env, SEXP call, int n, SEXPTYPE type, int force_args, SEXP res = PROTECT(R_forceAndCall(call, force_args, env)); if (type != VECSXP && Rf_length(res) != 1) { - stop_bad_element_length(res, i + 1, 1, "Result", NULL, false); + r_abort("Result must be length 1, not %i", Rf_length(res)); } set_vector_value(out, i, res, 0); diff --git a/tests/testthat/_snaps/keep.md b/tests/testthat/_snaps/keep.md index dbb85e3c..acc36091 100644 --- a/tests/testthat/_snaps/keep.md +++ b/tests/testthat/_snaps/keep.md @@ -3,11 +3,15 @@ Code keep(1:3, ~NA) Condition - Error in `keep()`: + Error in `map_lgl()`: + ! Computation failed in index 1 + Caused by error in `keep()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. Code discard(1:3, ~NA) Condition - Error in `discard()`: + Error in `map_lgl()`: + ! Computation failed in index 1 + Caused by error in `discard()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/map-depth.md b/tests/testthat/_snaps/map-depth.md index 363803df..a2339fc7 100644 --- a/tests/testthat/_snaps/map-depth.md +++ b/tests/testthat/_snaps/map-depth.md @@ -3,7 +3,13 @@ Code map_depth(x1, 6, length) Condition - Error in `map_depth()`: + Error in `.fmap()`: + ! Computation failed in index 1 + Caused by error in `.fmap()`: + ! Computation failed in index 1 + Caused by error in `.fmap()`: + ! Computation failed in index 1 + Caused by error in `map_depth()`: ! List not deep enough --- diff --git a/tests/testthat/_snaps/map-if-at.md b/tests/testthat/_snaps/map-if-at.md index 15540823..ad23db01 100644 --- a/tests/testthat/_snaps/map-if-at.md +++ b/tests/testthat/_snaps/map-if-at.md @@ -3,6 +3,8 @@ Code map_if(1:3, ~NA, ~"foo") Condition - Error in `map_if()`: + Error in `map_lgl()`: + ! Computation failed in index 1 + Caused by error in `map_if()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index e90d05e1..ce887090 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -4,7 +4,7 @@ map(environment(), identity) Condition Error in `map()`: - ! Computation failed in index + ! Computation failed in index 0 Caused by error in `withCallingHandlers()`: ! `.x` must be a vector, not an environment. @@ -14,10 +14,34 @@ map(quote(a), identity) Condition Error in `map()`: - ! Computation failed in index + ! Computation failed in index 0 Caused by error in `withCallingHandlers()`: ! `.x` must be a vector, not a symbol. +# all inform about location of problem + + Code + map_int(1:3, ~ fail_at_3(.x, 2:1)) + Condition + Error in `map_int()`: + ! Computation failed in index 3 + Caused by error in `withCallingHandlers()`: + ! Result must be length 1, not 2 + Code + map_int(1:3, ~ fail_at_3("x")) + Condition + Error in `map_int()`: + ! Computation failed in index 1 + Caused by error: + ! Can't coerce element 1 from a character to a integer + Code + map(1:3, ~ fail_at_3(stop("Doesn't work"))) + Condition + Error in `map()`: + ! Computation failed in index 1 + Caused by error in `fail_at_3()`: + ! Doesn't work + # error message follows style guide when result is not length 1 Code @@ -26,5 +50,5 @@ Error in `purrr::map_int()`: ! Computation failed in index 2 Caused by error in `withCallingHandlers()`: - ! Result 2 must have length 1, not 2. + ! Result must be length 1, not 2 diff --git a/tests/testthat/_snaps/map2.md b/tests/testthat/_snaps/map2.md index 8f96fc4b..cb7683c7 100644 --- a/tests/testthat/_snaps/map2.md +++ b/tests/testthat/_snaps/map2.md @@ -4,7 +4,7 @@ map2_int(1:4, 5:8, range) Condition Error in `map2_int()`: - ! Result 1 must have length 1, not 2. + ! Result must be length 1, not 2 # fails on non-vectors diff --git a/tests/testthat/_snaps/modify.md b/tests/testthat/_snaps/modify.md index 8d6606ff..fa3aa0d9 100644 --- a/tests/testthat/_snaps/modify.md +++ b/tests/testthat/_snaps/modify.md @@ -3,7 +3,9 @@ Code modify_if(list(1, 2), ~NA, ~"foo") Condition - Error in `modify_if()`: + Error in `map_lgl()`: + ! Computation failed in index 1 + Caused by error in `modify_if()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. --- @@ -11,6 +13,8 @@ Code modify_if(1:2, ~ c(TRUE, FALSE), ~"foo") Condition - Error in `modify_if()`: + Error in `map_lgl()`: + ! Computation failed in index 1 + Caused by error in `modify_if()`: ! `.p()` must return a single `TRUE` or `FALSE`, not a logical vector. diff --git a/tests/testthat/test-map.R b/tests/testthat/test-map.R index 9da3f20f..c22ba5f5 100644 --- a/tests/testthat/test-map.R +++ b/tests/testthat/test-map.R @@ -13,6 +13,18 @@ test_that("fails on non-vectors", { expect_snapshot(map(quote(a), identity), error = TRUE) }) +test_that("all inform about location of problem", { + fail_at_3 <- function(x, bad) { + if (x == 3) bad else x + } + + expect_snapshot(error = TRUE, { + map_int(1:3, ~ fail_at_3(.x, 2:1)) + map_int(1:3, ~ fail_at_3("x")) + map(1:3, ~ fail_at_3(stop("Doesn't work"))) + }) +}) + test_that("0 length input gives 0 length output", { out1 <- map(list(), identity) expect_equal(out1, list()) From d71bcc5a3f59342b66c6425524accfc57e29e98f Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Sep 2022 13:17:34 -0500 Subject: [PATCH 03/23] Only wrap errors that happen during iteration --- R/map.R | 16 +++++++++++----- src/map.c | 2 ++ tests/testthat/_snaps/map.md | 8 ++------ 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/R/map.R b/R/map.R index ef05684b..29ffef1a 100644 --- a/R/map.R +++ b/R/map.R @@ -165,11 +165,17 @@ with_indexed_errors <- function(expr, i, error_call = caller_env()) { try_fetch( expr, error = function(cnd) { - cli::cli_abort( - "Computation failed in index {i}", - parent = cnd, - call = error_call - ) + if (i == 0) { + # error happened before or after loop + cnd_signal(cnd) + } else { + cli::cli_abort( + "Computation failed in index {i}", + parent = cnd, + call = error_call + ) + } + } ) } diff --git a/src/map.c b/src/map.c index b5b94b5a..72da9c0f 100644 --- a/src/map.c +++ b/src/map.c @@ -59,6 +59,8 @@ SEXP call_loop(SEXP env, SEXP call, int n, SEXPTYPE type, int force_args, set_vector_value(out, i, res, 0); UNPROTECT(1); } + + INTEGER(i_val)[0] = 0; cli_progress_done(bar); UNPROTECT(3); diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index ce887090..c1d65bae 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -3,9 +3,7 @@ Code map(environment(), identity) Condition - Error in `map()`: - ! Computation failed in index 0 - Caused by error in `withCallingHandlers()`: + Error in `withCallingHandlers()`: ! `.x` must be a vector, not an environment. --- @@ -13,9 +11,7 @@ Code map(quote(a), identity) Condition - Error in `map()`: - ! Computation failed in index 0 - Caused by error in `withCallingHandlers()`: + Error in `withCallingHandlers()`: ! `.x` must be a vector, not a symbol. # all inform about location of problem From a5c3870dd587ad90a4a7f4cfeea253b31d99d0e0 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Sep 2022 13:23:17 -0500 Subject: [PATCH 04/23] Remove now duplicated index from error --- src/coerce.c | 4 ++-- tests/testthat/_snaps/map.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coerce.c b/src/coerce.c index 9dfb750d..19b77238 100644 --- a/src/coerce.c +++ b/src/coerce.c @@ -4,8 +4,8 @@ #include 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) { diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index c1d65bae..63b96dbf 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -29,7 +29,7 @@ Error in `map_int()`: ! Computation failed in index 1 Caused by error: - ! Can't coerce element 1 from a character to a integer + ! Can't coerce from a character to a integer Code map(1:3, ~ fail_at_3(stop("Doesn't work"))) Condition From 9cafb9d3b3a56716efff10a53ec3d1bcb37c81f6 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Sep 2022 13:27:13 -0500 Subject: [PATCH 05/23] Remove redundant test --- tests/testthat/_snaps/map.md | 10 ---------- tests/testthat/test-map.R | 6 ------ 2 files changed, 16 deletions(-) diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index 63b96dbf..b62e4ca0 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -38,13 +38,3 @@ Caused by error in `fail_at_3()`: ! Doesn't work -# error message follows style guide when result is not length 1 - - Code - purrr::map_int(x, "a") - Condition - Error in `purrr::map_int()`: - ! Computation failed in index 2 - Caused by error in `withCallingHandlers()`: - ! Result must be length 1, not 2 - diff --git a/tests/testthat/test-map.R b/tests/testthat/test-map.R index c22ba5f5..4f52621f 100644 --- a/tests/testthat/test-map.R +++ b/tests/testthat/test-map.R @@ -84,12 +84,6 @@ test_that("primitive dispatch correctly", { expect_identical(map(list(x, x), as.character), list("dispatched!", "dispatched!")) }) - -test_that("error message follows style guide when result is not length 1", { - x <- list(list(a = 1L), list(a = 2:3)) - expect_snapshot(purrr::map_int(x, "a"), error = TRUE) -}) - test_that("map() with empty input copies names", { named_list <- named(list()) expect_identical( map(named_list, identity), named(list())) From 7f691189e3750c2b8b170d34d9a6af1264158658 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Sep 2022 13:28:54 -0500 Subject: [PATCH 06/23] Switch back to Rf_errorcall() --- src/map.c | 2 +- tests/testthat/_snaps/map.md | 2 +- tests/testthat/_snaps/map2.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/map.c b/src/map.c index 72da9c0f..ab14546a 100644 --- a/src/map.c +++ b/src/map.c @@ -53,7 +53,7 @@ SEXP call_loop(SEXP env, SEXP call, int n, SEXPTYPE type, int force_args, SEXP res = PROTECT(R_forceAndCall(call, force_args, env)); if (type != VECSXP && Rf_length(res) != 1) { - r_abort("Result must be length 1, not %i", Rf_length(res)); + Rf_errorcall(R_NilValue, "Result must be length 1, not %i", Rf_length(res)); } set_vector_value(out, i, res, 0); diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index b62e4ca0..9bb136a8 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -21,7 +21,7 @@ Condition Error in `map_int()`: ! Computation failed in index 3 - Caused by error in `withCallingHandlers()`: + Caused by error: ! Result must be length 1, not 2 Code map_int(1:3, ~ fail_at_3("x")) diff --git a/tests/testthat/_snaps/map2.md b/tests/testthat/_snaps/map2.md index cb7683c7..c4720ed7 100644 --- a/tests/testthat/_snaps/map2.md +++ b/tests/testthat/_snaps/map2.md @@ -3,7 +3,7 @@ Code map2_int(1:4, 5:8, range) Condition - Error in `map2_int()`: + Error: ! Result must be length 1, not 2 # fails on non-vectors From 00cb90a0eb59d5c27d431a2c16bedd1430e5cfcc Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 16:58:21 -0500 Subject: [PATCH 07/23] Wrap map2() and modify() --- R/map2.R | 25 ++++++++++++++++++++----- R/pmap.R | 26 ++++++++++++++++++++------ tests/testthat/_snaps/map2.md | 16 ++++++++++------ tests/testthat/_snaps/modify.md | 4 ++-- tests/testthat/_snaps/pmap.md | 16 ++++++++++------ 5 files changed, 62 insertions(+), 25 deletions(-) diff --git a/R/map2.R b/R/map2.R index 3f0a03b3..79a06655 100644 --- a/R/map2.R +++ b/R/map2.R @@ -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 <- 0 + with_indexed_errors(i = i, + .Call(map2_impl, environment(), ".x", ".y", ".f", "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 <- 0 + with_indexed_errors(i = i, + .Call(map2_impl, environment(), ".x", ".y", ".f", "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 <- 0 + with_indexed_errors(i = i, + .Call(map2_impl, environment(), ".x", ".y", ".f", "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 <- 0 + with_indexed_errors(i = i, + .Call(map2_impl, environment(), ".x", ".y", ".f", "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 <- 0 + with_indexed_errors(i = i, + .Call(map2_impl, environment(), ".x", ".y", ".f", "character", .progress) + ) } #' @rdname map2 diff --git a/R/pmap.R b/R/pmap.R index 88e0e17a..b1d407eb 100644 --- a/R/pmap.R +++ b/R/pmap.R @@ -80,7 +80,10 @@ pmap <- function(.l, .f, ..., .progress = FALSE) { .l <- as.list(.l) } - .Call(pmap_impl, environment(), ".l", ".f", "list", .progress) + i <- 0 + with_indexed_errors(i = i, + .Call(pmap_impl, environment(), ".l", ".f", "list", .progress) + ) } #' @export @@ -91,7 +94,10 @@ pmap_lgl <- function(.l, .f, ..., .progress = FALSE) { .l <- as.list(.l) } - .Call(pmap_impl, environment(), ".l", ".f", "logical", .progress) + i <- 0 + with_indexed_errors(i = i, + .Call(pmap_impl, environment(), ".l", ".f", "logical", .progress) + ) } #' @export #' @rdname pmap @@ -101,7 +107,10 @@ pmap_int <- function(.l, .f, ..., .progress = FALSE) { .l <- as.list(.l) } - .Call(pmap_impl, environment(), ".l", ".f", "integer", .progress) + i <- 0 + with_indexed_errors(i = i, + .Call(pmap_impl, environment(), ".l", ".f", "integer", .progress) + ) } #' @export #' @rdname pmap @@ -111,7 +120,10 @@ pmap_dbl <- function(.l, .f, ..., .progress = FALSE) { .l <- as.list(.l) } - .Call(pmap_impl, environment(), ".l", ".f", "double", .progress) + i <- 0 + with_indexed_errors(i = i, + .Call(pmap_impl, environment(), ".l", ".f", "double", .progress) + ) } #' @export #' @rdname pmap @@ -121,7 +133,10 @@ pmap_chr <- function(.l, .f, ..., .progress = FALSE) { .l <- as.list(.l) } - .Call(pmap_impl, environment(), ".l", ".f", "character", .progress) + i <- 0 + with_indexed_errors(i = i, + .Call(pmap_impl, environment(), ".l", ".f", "character", .progress) + ) } #' @export @@ -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) { diff --git a/tests/testthat/_snaps/map2.md b/tests/testthat/_snaps/map2.md index fae1809b..423b09f5 100644 --- a/tests/testthat/_snaps/map2.md +++ b/tests/testthat/_snaps/map2.md @@ -3,12 +3,16 @@ Code map2_int(1, 1, ~"x") Condition - Error: + Error in `map2_int()`: + ! Computation failed in index 1 + Caused by error: ! Can't coerce from a character to a integer Code map2_int(1, 1, ~ 1:2) Condition - Error: + Error in `map2_int()`: + ! Computation failed in index 1 + Caused by error: ! Result must be length 1, not 2 Code map2_vec(1, 1, ~1, .ptype = character()) @@ -21,12 +25,12 @@ Code map2(environment(), "a", identity) Condition - Error in `map2()`: + Error in `withCallingHandlers()`: ! `.x` must be a vector, not an environment. Code map2("a", environment(), "a", identity) Condition - Error in `map2()`: + Error in `withCallingHandlers()`: ! `.y` must be a vector, not an environment. # recycles inputs @@ -34,14 +38,14 @@ Code map2(1:2, 1:3, `+`) Condition - Error in `map2()`: + Error in `withCallingHandlers()`: ! Mapped vectors must have consistent lengths: * `.x` has length 2 * `.y` has length 3 Code map2(1:2, integer(), `+`) Condition - Error in `map2()`: + Error in `withCallingHandlers()`: ! Mapped vectors must have consistent lengths: * `.x` has length 2 * `.y` has length 0 diff --git a/tests/testthat/_snaps/modify.md b/tests/testthat/_snaps/modify.md index 266eb500..3fe6b1ea 100644 --- a/tests/testthat/_snaps/modify.md +++ b/tests/testthat/_snaps/modify.md @@ -49,14 +49,14 @@ Code modify2(1:3, integer(), `+`) Condition - Error in `map2()`: + Error in `withCallingHandlers()`: ! Mapped vectors must have consistent lengths: * `.x` has length 3 * `.y` has length 0 Code modify2(1:3, 1:4, `+`) Condition - Error in `map2()`: + Error in `withCallingHandlers()`: ! Mapped vectors must have consistent lengths: * `.x` has length 3 * `.y` has length 4 diff --git a/tests/testthat/_snaps/pmap.md b/tests/testthat/_snaps/pmap.md index 83923c5f..c7fd42ef 100644 --- a/tests/testthat/_snaps/pmap.md +++ b/tests/testthat/_snaps/pmap.md @@ -3,12 +3,16 @@ Code pmap_int(list(1), ~"x") Condition - Error: + Error in `pmap_int()`: + ! Computation failed in index 1 + Caused by error: ! Can't coerce from a character to a integer Code pmap_int(list(1), ~ 1:2) Condition - Error: + Error in `pmap_int()`: + ! Computation failed in index 1 + Caused by error: ! Result must be length 1, not 2 Code pmap_vec(list(1), ~1, .ptype = character()) @@ -21,12 +25,12 @@ Code pmap(environment(), identity) Condition - Error in `pmap()`: + Error in `withCallingHandlers()`: ! `.l` must be a list, not an environment. Code pmap(list(environment()), identity) Condition - Error in `pmap()`: + Error in `withCallingHandlers()`: ! `.l[[1]]` must be a vector, not an environment. # recycles inputs @@ -34,11 +38,11 @@ Code pmap(list(1:2, 1:3), `+`) Condition - Error in `pmap()`: + Error in `withCallingHandlers()`: ! `.l[[2]]` must have length 1 or 2, not 3. Code pmap(list(1:2, integer()), `+`) Condition - Error in `pmap()`: + Error in `withCallingHandlers()`: ! `.l[[2]]` must have length 1 or 2, not 0. From 5a9f4101bc894f833d1d869e00975356fa498049 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 17:02:46 -0500 Subject: [PATCH 08/23] Name arguments by convention --- R/map-raw.R | 6 +++--- R/map.R | 10 +++++----- R/map2.R | 10 +++++----- R/pmap.R | 10 +++++----- src/init.c | 12 ++++++------ src/map.c | 33 ++++++++++++--------------------- 6 files changed, 36 insertions(+), 45 deletions(-) diff --git a/R/map-raw.R b/R/map-raw.R index 8e246957..0ef7b4aa 100644 --- a/R/map-raw.R +++ b/R/map-raw.R @@ -13,7 +13,7 @@ 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) + .Call(map_impl, environment(), "raw", FALSE) } #' @export @@ -24,7 +24,7 @@ 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) + .Call(map2_impl, environment(), "raw", FALSE) } #' @rdname map_raw @@ -46,7 +46,7 @@ pmap_raw <- function(.l, .f, ...) { .l <- as.list(.l) } - .Call(pmap_impl, environment(), ".l", ".f", "raw", FALSE) + .Call(pmap_impl, environment(), "raw", FALSE) } #' @export diff --git a/R/map.R b/R/map.R index 2ac3cb34..cba0e505 100644 --- a/R/map.R +++ b/R/map.R @@ -115,7 +115,7 @@ map <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) i <- 0 with_indexed_errors(i = i, - .Call(map_impl, environment(), ".x", ".f", "list", .progress) + .Call(map_impl, environment(), "list", .progress) ) } @@ -125,7 +125,7 @@ map_lgl <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) i <- 0 with_indexed_errors(i = i, - .Call(map_impl, environment(), ".x", ".f", "logical", .progress) + .Call(map_impl, environment(), "logical", .progress) ) } @@ -135,7 +135,7 @@ map_int <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) i <- 0 with_indexed_errors(i = i, - .Call(map_impl, environment(), ".x", ".f", "integer", .progress) + .Call(map_impl, environment(), "integer", .progress) ) } @@ -145,7 +145,7 @@ map_dbl <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) i <- 0 with_indexed_errors(i = i, - .Call(map_impl, environment(), ".x", ".f", "double", .progress) + .Call(map_impl, environment(), "double", .progress) ) } @@ -155,7 +155,7 @@ map_chr <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) i <- 0 with_indexed_errors(i = i, - .Call(map_impl, environment(), ".x", ".f", "character", .progress) + .Call(map_impl, environment(), "character", .progress) ) } diff --git a/R/map2.R b/R/map2.R index 79a06655..56f14c5d 100644 --- a/R/map2.R +++ b/R/map2.R @@ -34,7 +34,7 @@ map2 <- function(.x, .y, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) i <- 0 with_indexed_errors(i = i, - .Call(map2_impl, environment(), ".x", ".y", ".f", "list", .progress) + .Call(map2_impl, environment(), "list", .progress) ) } #' @export @@ -43,7 +43,7 @@ map2_lgl <- function(.x, .y, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) i <- 0 with_indexed_errors(i = i, - .Call(map2_impl, environment(), ".x", ".y", ".f", "logical", .progress) + .Call(map2_impl, environment(), "logical", .progress) ) } #' @export @@ -52,7 +52,7 @@ map2_int <- function(.x, .y, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) i <- 0 with_indexed_errors(i = i, - .Call(map2_impl, environment(), ".x", ".y", ".f", "integer", .progress) + .Call(map2_impl, environment(), "integer", .progress) ) } #' @export @@ -61,7 +61,7 @@ map2_dbl <- function(.x, .y, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) i <- 0 with_indexed_errors(i = i, - .Call(map2_impl, environment(), ".x", ".y", ".f", "double", .progress) + .Call(map2_impl, environment(), "double", .progress) ) } #' @export @@ -70,7 +70,7 @@ map2_chr <- function(.x, .y, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) i <- 0 with_indexed_errors(i = i, - .Call(map2_impl, environment(), ".x", ".y", ".f", "character", .progress) + .Call(map2_impl, environment(), "character", .progress) ) } diff --git a/R/pmap.R b/R/pmap.R index b1d407eb..da9e16ff 100644 --- a/R/pmap.R +++ b/R/pmap.R @@ -82,7 +82,7 @@ pmap <- function(.l, .f, ..., .progress = FALSE) { i <- 0 with_indexed_errors(i = i, - .Call(pmap_impl, environment(), ".l", ".f", "list", .progress) + .Call(pmap_impl, environment(), "list", .progress) ) } @@ -96,7 +96,7 @@ pmap_lgl <- function(.l, .f, ..., .progress = FALSE) { i <- 0 with_indexed_errors(i = i, - .Call(pmap_impl, environment(), ".l", ".f", "logical", .progress) + .Call(pmap_impl, environment(), "logical", .progress) ) } #' @export @@ -109,7 +109,7 @@ pmap_int <- function(.l, .f, ..., .progress = FALSE) { i <- 0 with_indexed_errors(i = i, - .Call(pmap_impl, environment(), ".l", ".f", "integer", .progress) + .Call(pmap_impl, environment(), "integer", .progress) ) } #' @export @@ -122,7 +122,7 @@ pmap_dbl <- function(.l, .f, ..., .progress = FALSE) { i <- 0 with_indexed_errors(i = i, - .Call(pmap_impl, environment(), ".l", ".f", "double", .progress) + .Call(pmap_impl, environment(), "double", .progress) ) } #' @export @@ -135,7 +135,7 @@ pmap_chr <- function(.l, .f, ..., .progress = FALSE) { i <- 0 with_indexed_errors(i = i, - .Call(pmap_impl, environment(), ".l", ".f", "character", .progress) + .Call(pmap_impl, environment(), "character", .progress) ) } diff --git a/src/init.c b/src/init.c index e7fb638f..9bd3e6c8 100644 --- a/src/init.c +++ b/src/init.c @@ -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); @@ -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}, diff --git a/src/map.c b/src/map.c index ab14546a..dd93b322 100644 --- a/src/map.c +++ b/src/map.c @@ -67,12 +67,9 @@ SEXP call_loop(SEXP env, SEXP call, int n, SEXPTYPE type, int force_args, return out; } -SEXP map_impl(SEXP env, SEXP x_name_, SEXP f_name_, SEXP type_, SEXP progress) { - const char* x_name = CHAR(Rf_asChar(x_name_)); - const char* f_name = CHAR(Rf_asChar(f_name_)); - - SEXP x = Rf_install(x_name); - SEXP f = Rf_install(f_name); +SEXP map_impl(SEXP env, SEXP type_, SEXP progress) { + SEXP x = Rf_install(".x"); + SEXP f = Rf_install(".f"); SEXP i = Rf_install("i"); SEXPTYPE type = Rf_str2type(CHAR(Rf_asChar(type_))); @@ -101,14 +98,10 @@ SEXP map_impl(SEXP env, SEXP x_name_, SEXP f_name_, SEXP type_, SEXP progress) { return out; } -SEXP map2_impl(SEXP env, SEXP x_name_, SEXP y_name_, SEXP f_name_, SEXP type_, SEXP progress) { - const char* x_name = CHAR(Rf_asChar(x_name_)); - const char* y_name = CHAR(Rf_asChar(y_name_)); - const char* f_name = CHAR(Rf_asChar(f_name_)); - - SEXP x = Rf_install(x_name); - SEXP y = Rf_install(y_name); - SEXP f = Rf_install(f_name); +SEXP map2_impl(SEXP env, SEXP type_, SEXP progress) { + SEXP x = Rf_install(".x"); + SEXP y = Rf_install(".y"); + SEXP f = Rf_install(".f"); SEXP i = Rf_install("i"); SEXPTYPE type = Rf_str2type(CHAR(Rf_asChar(type_))); @@ -142,14 +135,13 @@ SEXP map2_impl(SEXP env, SEXP x_name_, SEXP y_name_, SEXP f_name_, SEXP type_, S return out; } -SEXP pmap_impl(SEXP env, SEXP l_name_, SEXP f_name_, SEXP type_, SEXP progress) { - const char* l_name = CHAR(Rf_asChar(l_name_)); - SEXP l = Rf_install(l_name); +SEXP pmap_impl(SEXP env, SEXP type_, SEXP progress) { + SEXP l = Rf_install(".l"); SEXP l_val = PROTECT(Rf_eval(l, env)); SEXPTYPE type = Rf_str2type(CHAR(Rf_asChar(type_))); if (!Rf_isVectorList(l_val)) { - stop_bad_type(l_val, "a list", NULL, l_name); + stop_bad_type(l_val, "a list", NULL, ".l"); } // Check all elements are lists and find recycled length @@ -160,7 +152,7 @@ SEXP pmap_impl(SEXP env, SEXP l_name_, SEXP f_name_, SEXP type_, SEXP progress) SEXP j_val = VECTOR_ELT(l_val, j); if (!Rf_isVector(j_val) && !Rf_isNull(j_val)) { - stop_bad_element_type(j_val, j + 1, "a vector", NULL, l_name); + stop_bad_element_type(j_val, j + 1, "a vector", NULL, ".l"); } int nj = Rf_length(j_val); @@ -183,8 +175,7 @@ SEXP pmap_impl(SEXP env, SEXP l_name_, SEXP f_name_, SEXP type_, SEXP progress) SEXP l_names = PROTECT(Rf_getAttrib(l_val, R_NamesSymbol)); int has_names = !Rf_isNull(l_names); - const char* f_name = CHAR(Rf_asChar(f_name_)); - SEXP f = Rf_install(f_name); + SEXP f = Rf_install(".f"); SEXP i = Rf_install("i"); SEXP one = PROTECT(Rf_ScalarInteger(1)); From c01d7470505ea644d00953b767440ce002356963 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 17:06:39 -0500 Subject: [PATCH 09/23] Use existing i variable And make sure it's an integer --- R/map-raw.R | 3 +++ R/map.R | 10 +++++----- R/map2.R | 10 +++++----- R/pmap.R | 10 +++++----- src/map.c | 6 ++---- 5 files changed, 20 insertions(+), 19 deletions(-) diff --git a/R/map-raw.R b/R/map-raw.R index 0ef7b4aa..79040547 100644 --- a/R/map-raw.R +++ b/R/map-raw.R @@ -13,6 +13,7 @@ map_raw <- function(.x, .f, ...) { lifecycle::deprecate_soft("1.0.0", "map_raw()", "map_vec()") .f <- as_mapper(.f, ...) + i <- 0L .Call(map_impl, environment(), "raw", FALSE) } @@ -24,6 +25,7 @@ map2_raw <- function(.x, .y, .f, ...) { } map2_raw_ <- function(.x, .y, .f, ...) { .f <- as_mapper(.f, ...) + i <- 0L .Call(map2_impl, environment(), "raw", FALSE) } @@ -46,6 +48,7 @@ pmap_raw <- function(.l, .f, ...) { .l <- as.list(.l) } + i <- 0L .Call(pmap_impl, environment(), "raw", FALSE) } diff --git a/R/map.R b/R/map.R index cba0e505..73363bb6 100644 --- a/R/map.R +++ b/R/map.R @@ -113,7 +113,7 @@ #' map_dbl("r.squared") map <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(map_impl, environment(), "list", .progress) ) @@ -123,7 +123,7 @@ map <- function(.x, .f, ..., .progress = FALSE) { #' @export map_lgl <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(map_impl, environment(), "logical", .progress) ) @@ -133,7 +133,7 @@ map_lgl <- function(.x, .f, ..., .progress = FALSE) { #' @export map_int <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(map_impl, environment(), "integer", .progress) ) @@ -143,7 +143,7 @@ map_int <- function(.x, .f, ..., .progress = FALSE) { #' @export map_dbl <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(map_impl, environment(), "double", .progress) ) @@ -153,7 +153,7 @@ map_dbl <- function(.x, .f, ..., .progress = FALSE) { #' @export map_chr <- function(.x, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(map_impl, environment(), "character", .progress) ) diff --git a/R/map2.R b/R/map2.R index 56f14c5d..46cc56ed 100644 --- a/R/map2.R +++ b/R/map2.R @@ -32,7 +32,7 @@ #' map2(mods, by_cyl, predict) map2 <- function(.x, .y, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(map2_impl, environment(), "list", .progress) ) @@ -41,7 +41,7 @@ map2 <- function(.x, .y, .f, ..., .progress = FALSE) { #' @rdname map2 map2_lgl <- function(.x, .y, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(map2_impl, environment(), "logical", .progress) ) @@ -50,7 +50,7 @@ map2_lgl <- function(.x, .y, .f, ..., .progress = FALSE) { #' @rdname map2 map2_int <- function(.x, .y, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(map2_impl, environment(), "integer", .progress) ) @@ -59,7 +59,7 @@ map2_int <- function(.x, .y, .f, ..., .progress = FALSE) { #' @rdname map2 map2_dbl <- function(.x, .y, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(map2_impl, environment(), "double", .progress) ) @@ -68,7 +68,7 @@ map2_dbl <- function(.x, .y, .f, ..., .progress = FALSE) { #' @rdname map2 map2_chr <- function(.x, .y, .f, ..., .progress = FALSE) { .f <- as_mapper(.f, ...) - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(map2_impl, environment(), "character", .progress) ) diff --git a/R/pmap.R b/R/pmap.R index da9e16ff..1e41715e 100644 --- a/R/pmap.R +++ b/R/pmap.R @@ -80,7 +80,7 @@ pmap <- function(.l, .f, ..., .progress = FALSE) { .l <- as.list(.l) } - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(pmap_impl, environment(), "list", .progress) ) @@ -94,7 +94,7 @@ pmap_lgl <- function(.l, .f, ..., .progress = FALSE) { .l <- as.list(.l) } - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(pmap_impl, environment(), "logical", .progress) ) @@ -107,7 +107,7 @@ pmap_int <- function(.l, .f, ..., .progress = FALSE) { .l <- as.list(.l) } - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(pmap_impl, environment(), "integer", .progress) ) @@ -120,7 +120,7 @@ pmap_dbl <- function(.l, .f, ..., .progress = FALSE) { .l <- as.list(.l) } - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(pmap_impl, environment(), "double", .progress) ) @@ -133,7 +133,7 @@ pmap_chr <- function(.l, .f, ..., .progress = FALSE) { .l <- as.list(.l) } - i <- 0 + i <- 0L with_indexed_errors(i = i, .Call(pmap_impl, environment(), "character", .progress) ) diff --git a/src/map.c b/src/map.c index dd93b322..a5437e93 100644 --- a/src/map.c +++ b/src/map.c @@ -36,10 +36,8 @@ void check_vector(SEXP x, const char *name, SEXP env) { // call must involve i SEXP call_loop(SEXP env, SEXP call, int n, SEXPTYPE type, int force_args, SEXP progress) { - // 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); + SEXP i_val = Rf_findVarInFrame(env, i); SEXP bar = PROTECT(cli_progress_bar(n, progress)); SEXP out = PROTECT(Rf_allocVector(type, n)); @@ -63,7 +61,7 @@ SEXP call_loop(SEXP env, SEXP call, int n, SEXPTYPE type, int force_args, INTEGER(i_val)[0] = 0; cli_progress_done(bar); - UNPROTECT(3); + UNPROTECT(2); return out; } From adebf3b8d84f8a4de723abef22244940e5c8457f Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 17:07:15 -0500 Subject: [PATCH 10/23] Use simpler function --- R/map.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/map.R b/R/map.R index 73363bb6..a578e98a 100644 --- a/R/map.R +++ b/R/map.R @@ -177,7 +177,7 @@ walk <- function(.x, .f, ..., .progress = FALSE) { } with_indexed_errors <- function(expr, i, error_call = caller_env()) { - try_fetch( + withCallingHandlers( expr, error = function(cnd) { if (i == 0) { From f2bf469fcdfaf0c14414f876a919c5446cee3bab Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 17:10:07 -0500 Subject: [PATCH 11/23] Use r_abort_call() in map.c --- src/conditions.c | 34 ++++++++++++++++++++++++--------- src/conditions.h | 1 + src/map.c | 29 ++++++++++++++++++++++------ tests/testthat/_snaps/map.md | 4 ++-- tests/testthat/_snaps/map2.md | 8 ++++---- tests/testthat/_snaps/modify.md | 4 ++-- tests/testthat/_snaps/pmap.md | 8 ++++---- 7 files changed, 61 insertions(+), 27 deletions(-) diff --git a/src/conditions.c b/src/conditions.c index 4e0503ae..5ba69fde 100644 --- a/src/conditions.c +++ b/src/conditions.c @@ -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")) @@ -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, ...) { + 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) { diff --git a/src/conditions.h b/src/conditions.h index 19a2374d..4a8ff53e 100644 --- a/src/conditions.h +++ b/src/conditions.h @@ -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); diff --git a/src/map.c b/src/map.c index a5437e93..bdaeca67 100644 --- a/src/map.c +++ b/src/map.c @@ -29,8 +29,11 @@ void check_vector(SEXP x, const char *name, SEXP env) { if (Rf_isNull(x) || Rf_isVector(x) || Rf_isPairList(x)) { return; } - - stop_bad_type(x, "a vector", NULL, name); + r_abort_call( + env, + "`%s` must be a vector, not %s.", + name, rlang_obj_type_friendly_full(x, true, false) + ); } // call must involve i @@ -110,7 +113,8 @@ SEXP map2_impl(SEXP env, SEXP type_, SEXP progress) { int nx = Rf_length(x_val), ny = Rf_length(y_val); if (nx != ny && nx != 1 && ny != 1) { - r_abort( + r_abort_call( + env, "Mapped vectors must have consistent lengths:\n" "* `.x` has length %d\n" "* `.y` has length %d", @@ -139,7 +143,11 @@ SEXP pmap_impl(SEXP env, SEXP type_, SEXP progress) { SEXPTYPE type = Rf_str2type(CHAR(Rf_asChar(type_))); if (!Rf_isVectorList(l_val)) { - stop_bad_type(l_val, "a list", NULL, ".l"); + r_abort_call( + env, + "`.l` must be a list, not %s.", + rlang_obj_type_friendly_full(l_val, true, false) + ); } // Check all elements are lists and find recycled length @@ -150,7 +158,12 @@ SEXP pmap_impl(SEXP env, SEXP type_, SEXP progress) { SEXP j_val = VECTOR_ELT(l_val, j); if (!Rf_isVector(j_val) && !Rf_isNull(j_val)) { - stop_bad_element_type(j_val, j + 1, "a vector", NULL, ".l"); + r_abort_call( + env, + "`.l[[%i]]` must be a vector, not %s.", + j + 1, + rlang_obj_type_friendly_full(j_val, true, false) + ); } int nj = Rf_length(j_val); @@ -162,7 +175,11 @@ SEXP pmap_impl(SEXP env, SEXP type_, SEXP progress) { if (n == -1) { n = nj; } else if (nj != n) { - stop_bad_element_length(j_val, j + 1, n, NULL, ".l", true); + r_abort_call( + env, + "`.l[[%i]]` must have length 1 or %i, not %i.", + j + 1, n, nj + ); } } diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index b7e1ef72..09bcb07e 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -3,7 +3,7 @@ Code map(environment(), identity) Condition - Error in `withCallingHandlers()`: + Error in `map()`: ! `.x` must be a vector, not an environment. --- @@ -11,7 +11,7 @@ Code map(quote(a), identity) Condition - Error in `withCallingHandlers()`: + Error in `map()`: ! `.x` must be a vector, not a symbol. # all inform about location of problem diff --git a/tests/testthat/_snaps/map2.md b/tests/testthat/_snaps/map2.md index 423b09f5..66e43dc3 100644 --- a/tests/testthat/_snaps/map2.md +++ b/tests/testthat/_snaps/map2.md @@ -25,12 +25,12 @@ Code map2(environment(), "a", identity) Condition - Error in `withCallingHandlers()`: + Error in `map2()`: ! `.x` must be a vector, not an environment. Code map2("a", environment(), "a", identity) Condition - Error in `withCallingHandlers()`: + Error in `map2()`: ! `.y` must be a vector, not an environment. # recycles inputs @@ -38,14 +38,14 @@ Code map2(1:2, 1:3, `+`) Condition - Error in `withCallingHandlers()`: + Error in `map2()`: ! Mapped vectors must have consistent lengths: * `.x` has length 2 * `.y` has length 3 Code map2(1:2, integer(), `+`) Condition - Error in `withCallingHandlers()`: + Error in `map2()`: ! Mapped vectors must have consistent lengths: * `.x` has length 2 * `.y` has length 0 diff --git a/tests/testthat/_snaps/modify.md b/tests/testthat/_snaps/modify.md index 3fe6b1ea..266eb500 100644 --- a/tests/testthat/_snaps/modify.md +++ b/tests/testthat/_snaps/modify.md @@ -49,14 +49,14 @@ Code modify2(1:3, integer(), `+`) Condition - Error in `withCallingHandlers()`: + Error in `map2()`: ! Mapped vectors must have consistent lengths: * `.x` has length 3 * `.y` has length 0 Code modify2(1:3, 1:4, `+`) Condition - Error in `withCallingHandlers()`: + Error in `map2()`: ! Mapped vectors must have consistent lengths: * `.x` has length 3 * `.y` has length 4 diff --git a/tests/testthat/_snaps/pmap.md b/tests/testthat/_snaps/pmap.md index c7fd42ef..8432748c 100644 --- a/tests/testthat/_snaps/pmap.md +++ b/tests/testthat/_snaps/pmap.md @@ -25,12 +25,12 @@ Code pmap(environment(), identity) Condition - Error in `withCallingHandlers()`: + Error in `pmap()`: ! `.l` must be a list, not an environment. Code pmap(list(environment()), identity) Condition - Error in `withCallingHandlers()`: + Error in `pmap()`: ! `.l[[1]]` must be a vector, not an environment. # recycles inputs @@ -38,11 +38,11 @@ Code pmap(list(1:2, 1:3), `+`) Condition - Error in `withCallingHandlers()`: + Error in `pmap()`: ! `.l[[2]]` must have length 1 or 2, not 3. Code pmap(list(1:2, integer()), `+`) Condition - Error in `withCallingHandlers()`: + Error in `pmap()`: ! `.l[[2]]` must have length 1 or 2, not 0. From 8aef34ffff59b2e87ffdc1699cad78da05755181 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 17:18:50 -0500 Subject: [PATCH 12/23] Use consistent message for map2 recycling --- src/map.c | 7 ++----- tests/testthat/_snaps/map2.md | 8 ++------ tests/testthat/_snaps/modify.md | 8 ++------ 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/map.c b/src/map.c index bdaeca67..42df4b93 100644 --- a/src/map.c +++ b/src/map.c @@ -115,11 +115,8 @@ SEXP map2_impl(SEXP env, SEXP type_, SEXP progress) { if (nx != ny && nx != 1 && ny != 1) { r_abort_call( env, - "Mapped vectors must have consistent lengths:\n" - "* `.x` has length %d\n" - "* `.y` has length %d", - nx, - ny + "`.y must have length 1 or %i, not %i.", + nx, ny ); } int n = (nx == 1) ? ny : nx; diff --git a/tests/testthat/_snaps/map2.md b/tests/testthat/_snaps/map2.md index 66e43dc3..2933771b 100644 --- a/tests/testthat/_snaps/map2.md +++ b/tests/testthat/_snaps/map2.md @@ -39,14 +39,10 @@ map2(1:2, 1:3, `+`) Condition Error in `map2()`: - ! Mapped vectors must have consistent lengths: - * `.x` has length 2 - * `.y` has length 3 + ! `.y must have length 1 or 2, not 3. Code map2(1:2, integer(), `+`) Condition Error in `map2()`: - ! Mapped vectors must have consistent lengths: - * `.x` has length 2 - * `.y` has length 0 + ! `.y must have length 1 or 2, not 0. diff --git a/tests/testthat/_snaps/modify.md b/tests/testthat/_snaps/modify.md index 266eb500..6027c61d 100644 --- a/tests/testthat/_snaps/modify.md +++ b/tests/testthat/_snaps/modify.md @@ -50,16 +50,12 @@ modify2(1:3, integer(), `+`) Condition Error in `map2()`: - ! Mapped vectors must have consistent lengths: - * `.x` has length 3 - * `.y` has length 0 + ! `.y must have length 1 or 3, not 0. Code modify2(1:3, 1:4, `+`) Condition Error in `map2()`: - ! Mapped vectors must have consistent lengths: - * `.x` has length 3 - * `.y` has length 4 + ! `.y must have length 1 or 3, not 4. # modify_if() requires predicate functions From 8ec3eca26277d05a347c9b2ce5c6d2636e7b38e3 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 17:19:27 -0500 Subject: [PATCH 13/23] Add news bullet --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index 0ad0795a..c034c0b5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -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 From e54cf3e57015163491d1f26de7b7269b0d4677d5 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 17:22:57 -0500 Subject: [PATCH 14/23] WS --- R/map.R | 1 - 1 file changed, 1 deletion(-) diff --git a/R/map.R b/R/map.R index a578e98a..4ff3b0c7 100644 --- a/R/map.R +++ b/R/map.R @@ -190,7 +190,6 @@ with_indexed_errors <- function(expr, i, error_call = caller_env()) { call = error_call ) } - } ) } From 33029e7af47d91b64e7e5f437a68a84257320bc4 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 17:23:13 -0500 Subject: [PATCH 15/23] Avoid coercion --- R/map.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/map.R b/R/map.R index 4ff3b0c7..7f47bd7d 100644 --- a/R/map.R +++ b/R/map.R @@ -180,7 +180,7 @@ with_indexed_errors <- function(expr, i, error_call = caller_env()) { withCallingHandlers( expr, error = function(cnd) { - if (i == 0) { + if (i == 0L) { # error happened before or after loop cnd_signal(cnd) } else { From 02416faead66c805af4bb6351c164d1b30c3808c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 17:27:23 -0500 Subject: [PATCH 16/23] Match new dplyr errors --- R/map.R | 2 +- tests/testthat/_snaps/keep.md | 4 ++-- tests/testthat/_snaps/map-depth.md | 12 ++++++------ tests/testthat/_snaps/map-if-at.md | 2 +- tests/testthat/_snaps/map.md | 6 +++--- tests/testthat/_snaps/map2.md | 4 ++-- tests/testthat/_snaps/modify.md | 2 +- tests/testthat/_snaps/pmap.md | 4 ++-- 8 files changed, 18 insertions(+), 18 deletions(-) diff --git a/R/map.R b/R/map.R index 7f47bd7d..c2fd57f2 100644 --- a/R/map.R +++ b/R/map.R @@ -185,7 +185,7 @@ with_indexed_errors <- function(expr, i, error_call = caller_env()) { cnd_signal(cnd) } else { cli::cli_abort( - "Computation failed in index {i}", + "Can't compute index {i}", parent = cnd, call = error_call ) diff --git a/tests/testthat/_snaps/keep.md b/tests/testthat/_snaps/keep.md index acc36091..63beece7 100644 --- a/tests/testthat/_snaps/keep.md +++ b/tests/testthat/_snaps/keep.md @@ -4,14 +4,14 @@ keep(1:3, ~NA) Condition Error in `map_lgl()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error in `keep()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. Code discard(1:3, ~NA) Condition Error in `map_lgl()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error in `discard()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/map-depth.md b/tests/testthat/_snaps/map-depth.md index d5e930a3..d5237922 100644 --- a/tests/testthat/_snaps/map-depth.md +++ b/tests/testthat/_snaps/map-depth.md @@ -4,11 +4,11 @@ map_depth(x1, 6, length) Condition Error in `.fmap()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error in `.fmap()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error in `.fmap()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error in `map_depth()`: ! List not deep enough @@ -26,11 +26,11 @@ modify_depth(x1, 5, length) Condition Error in `map()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error in `map()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error in `map()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error in `modify_depth()`: ! List not deep enough diff --git a/tests/testthat/_snaps/map-if-at.md b/tests/testthat/_snaps/map-if-at.md index ad23db01..1cb74e5f 100644 --- a/tests/testthat/_snaps/map-if-at.md +++ b/tests/testthat/_snaps/map-if-at.md @@ -4,7 +4,7 @@ map_if(1:3, ~NA, ~"foo") Condition Error in `map_lgl()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error in `map_if()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index 09bcb07e..3e950794 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -20,21 +20,21 @@ map_int(1:3, ~ fail_at_3(.x, 2:1)) Condition Error in `map_int()`: - ! Computation failed in index 3 + ! Can't compute index 3 Caused by error: ! Result must be length 1, not 2 Code map_int(1:3, ~ fail_at_3("x")) Condition Error in `map_int()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error: ! Can't coerce from a character to a integer Code map(1:3, ~ fail_at_3(stop("Doesn't work"))) Condition Error in `map()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error in `fail_at_3()`: ! Doesn't work diff --git a/tests/testthat/_snaps/map2.md b/tests/testthat/_snaps/map2.md index 2933771b..d4643102 100644 --- a/tests/testthat/_snaps/map2.md +++ b/tests/testthat/_snaps/map2.md @@ -4,14 +4,14 @@ map2_int(1, 1, ~"x") Condition Error in `map2_int()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error: ! Can't coerce from a character to a integer Code map2_int(1, 1, ~ 1:2) Condition Error in `map2_int()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error: ! Result must be length 1, not 2 Code diff --git a/tests/testthat/_snaps/modify.md b/tests/testthat/_snaps/modify.md index 6027c61d..22314589 100644 --- a/tests/testthat/_snaps/modify.md +++ b/tests/testthat/_snaps/modify.md @@ -63,7 +63,7 @@ modify_if(list(1, 2), ~NA, ~"foo") Condition Error in `map_lgl()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error in `modify_if()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/pmap.md b/tests/testthat/_snaps/pmap.md index 8432748c..f1af525e 100644 --- a/tests/testthat/_snaps/pmap.md +++ b/tests/testthat/_snaps/pmap.md @@ -4,14 +4,14 @@ pmap_int(list(1), ~"x") Condition Error in `pmap_int()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error: ! Can't coerce from a character to a integer Code pmap_int(list(1), ~ 1:2) Condition Error in `pmap_int()`: - ! Computation failed in index 1 + ! Can't compute index 1 Caused by error: ! Result must be length 1, not 2 Code From 6d5330486cf7da8d8bc6d86b13bf3912c8544fc0 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 17:28:28 -0500 Subject: [PATCH 17/23] Add missing full stops --- R/map.R | 2 +- src/coerce.c | 2 +- tests/testthat/_snaps/keep.md | 4 ++-- tests/testthat/_snaps/map-depth.md | 12 ++++++------ tests/testthat/_snaps/map-if-at.md | 2 +- tests/testthat/_snaps/map.md | 8 ++++---- tests/testthat/_snaps/map2.md | 6 +++--- tests/testthat/_snaps/modify.md | 2 +- tests/testthat/_snaps/pmap.md | 6 +++--- 9 files changed, 22 insertions(+), 22 deletions(-) diff --git a/R/map.R b/R/map.R index c2fd57f2..5ac3488a 100644 --- a/R/map.R +++ b/R/map.R @@ -185,7 +185,7 @@ with_indexed_errors <- function(expr, i, error_call = caller_env()) { cnd_signal(cnd) } else { cli::cli_abort( - "Can't compute index {i}", + "Can't compute index {i}.", parent = cnd, call = error_call ) diff --git a/src/coerce.c b/src/coerce.c index 19b77238..56614157 100644 --- a/src/coerce.c +++ b/src/coerce.c @@ -4,7 +4,7 @@ #include void cant_coerce(SEXP from, SEXP to, int i) { - Rf_errorcall(R_NilValue, "Can't coerce from a %s to a %s", + Rf_errorcall(R_NilValue, "Can't coerce from a %s to a %s.", Rf_type2char(TYPEOF(from)), Rf_type2char(TYPEOF(to))); } diff --git a/tests/testthat/_snaps/keep.md b/tests/testthat/_snaps/keep.md index 63beece7..d081c7a2 100644 --- a/tests/testthat/_snaps/keep.md +++ b/tests/testthat/_snaps/keep.md @@ -4,14 +4,14 @@ keep(1:3, ~NA) Condition Error in `map_lgl()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error in `keep()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. Code discard(1:3, ~NA) Condition Error in `map_lgl()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error in `discard()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/map-depth.md b/tests/testthat/_snaps/map-depth.md index d5237922..9adf967f 100644 --- a/tests/testthat/_snaps/map-depth.md +++ b/tests/testthat/_snaps/map-depth.md @@ -4,11 +4,11 @@ map_depth(x1, 6, length) Condition Error in `.fmap()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error in `.fmap()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error in `.fmap()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error in `map_depth()`: ! List not deep enough @@ -26,11 +26,11 @@ modify_depth(x1, 5, length) Condition Error in `map()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error in `map()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error in `map()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error in `modify_depth()`: ! List not deep enough diff --git a/tests/testthat/_snaps/map-if-at.md b/tests/testthat/_snaps/map-if-at.md index 1cb74e5f..9ce259c0 100644 --- a/tests/testthat/_snaps/map-if-at.md +++ b/tests/testthat/_snaps/map-if-at.md @@ -4,7 +4,7 @@ map_if(1:3, ~NA, ~"foo") Condition Error in `map_lgl()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error in `map_if()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index 3e950794..af9eb41f 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -20,21 +20,21 @@ map_int(1:3, ~ fail_at_3(.x, 2:1)) Condition Error in `map_int()`: - ! Can't compute index 3 + ! Can't compute index 3. Caused by error: ! Result must be length 1, not 2 Code map_int(1:3, ~ fail_at_3("x")) Condition Error in `map_int()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error: - ! Can't coerce from a character to a integer + ! Can't coerce from a character to a integer. Code map(1:3, ~ fail_at_3(stop("Doesn't work"))) Condition Error in `map()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error in `fail_at_3()`: ! Doesn't work diff --git a/tests/testthat/_snaps/map2.md b/tests/testthat/_snaps/map2.md index d4643102..8ade04cd 100644 --- a/tests/testthat/_snaps/map2.md +++ b/tests/testthat/_snaps/map2.md @@ -4,14 +4,14 @@ map2_int(1, 1, ~"x") Condition Error in `map2_int()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error: - ! Can't coerce from a character to a integer + ! Can't coerce from a character to a integer. Code map2_int(1, 1, ~ 1:2) Condition Error in `map2_int()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error: ! Result must be length 1, not 2 Code diff --git a/tests/testthat/_snaps/modify.md b/tests/testthat/_snaps/modify.md index 22314589..1ae279a2 100644 --- a/tests/testthat/_snaps/modify.md +++ b/tests/testthat/_snaps/modify.md @@ -63,7 +63,7 @@ modify_if(list(1, 2), ~NA, ~"foo") Condition Error in `map_lgl()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error in `modify_if()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/pmap.md b/tests/testthat/_snaps/pmap.md index f1af525e..b89b6f93 100644 --- a/tests/testthat/_snaps/pmap.md +++ b/tests/testthat/_snaps/pmap.md @@ -4,14 +4,14 @@ pmap_int(list(1), ~"x") Condition Error in `pmap_int()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error: - ! Can't coerce from a character to a integer + ! Can't coerce from a character to a integer. Code pmap_int(list(1), ~ 1:2) Condition Error in `pmap_int()`: - ! Can't compute index 1 + ! Can't compute index 1. Caused by error: ! Result must be length 1, not 2 Code From a96cef8b3c90b22562c2f59b595855adc65e2fe4 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 22 Sep 2022 07:46:46 -0500 Subject: [PATCH 18/23] Use explicit caller_env argument Extracting repeated code into map_, map2_, and pmap_ --- R/deprec-invoke.R | 2 +- R/map-raw.R | 25 +++------------ R/map.R | 36 ++++++++-------------- R/map2.R | 34 ++++++++------------ R/pmap.R | 53 +++++++++----------------------- src/init.c | 12 ++++---- src/map.c | 24 +++++++-------- tests/testthat/_snaps/map-raw.md | 3 -- 8 files changed, 62 insertions(+), 127 deletions(-) diff --git a/R/deprec-invoke.R b/R/deprec-invoke.R index 2fdcf8dd..db1c2ae4 100644 --- a/R/deprec-invoke.R +++ b/R/deprec-invoke.R @@ -147,7 +147,7 @@ invoke_map_raw <- function(.f, .x = list(NULL), ..., .env = NULL) { .env <- .env %||% parent.frame() .f <- as_invoke_function(.f) - map2_raw_(.f, .x, invoke, ..., .env = .env) + map2_(.f, .x, invoke, ..., .type = "raw") } #' @rdname invoke diff --git a/R/map-raw.R b/R/map-raw.R index 79040547..f45d2968 100644 --- a/R/map-raw.R +++ b/R/map-raw.R @@ -11,45 +11,28 @@ #' @export map_raw <- function(.x, .f, ...) { lifecycle::deprecate_soft("1.0.0", "map_raw()", "map_vec()") - - .f <- as_mapper(.f, ...) - i <- 0L - .Call(map_impl, environment(), "raw", FALSE) + map_(.x, .f, ..., .type = "raw") } #' @export #' @rdname map_raw map2_raw <- function(.x, .y, .f, ...) { lifecycle::deprecate_soft("1.0.0", "map2_raw()", "map2_vec()") - map2_raw_(.x, .y, .f, ...) -} -map2_raw_ <- function(.x, .y, .f, ...) { - .f <- as_mapper(.f, ...) - i <- 0L - .Call(map2_impl, environment(), "raw", FALSE) + map2_(.x, .y, .f, ..., .type = "raw") } #' @rdname map_raw #' @export imap_raw <- function(.x, .f, ...) { lifecycle::deprecate_soft("1.0.0", "imap_raw()", "imap_vec()") - - .f <- as_mapper(.f, ...) - map2_raw(.x, vec_index(.x), .f, ...) + map2_(.x, vec_index(.x), .f, ..., .type = "raw") } #' @export #' @rdname map_raw pmap_raw <- function(.l, .f, ...) { lifecycle::deprecate_soft("1.0.0", "pmap_raw()", "pmap_vec()") - - .f <- as_mapper(.f, ...) - if (is.data.frame(.l)) { - .l <- as.list(.l) - } - - i <- 0L - .Call(pmap_impl, environment(), "raw", FALSE) + pmap_(.l, .f, ..., .type = "raw") } #' @export diff --git a/R/map.R b/R/map.R index 5ac3488a..ed1ffb9f 100644 --- a/R/map.R +++ b/R/map.R @@ -112,53 +112,44 @@ #' map(summary) |> #' map_dbl("r.squared") map <- function(.x, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - i <- 0L - with_indexed_errors(i = i, - .Call(map_impl, environment(), "list", .progress) - ) + map_(.x, .f, ..., .type = "list", .progress = .progress) } #' @rdname map #' @export map_lgl <- function(.x, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - i <- 0L - with_indexed_errors(i = i, - .Call(map_impl, environment(), "logical", .progress) - ) + map_(.x, .f, ..., .type = "logical", .progress = .progress) } #' @rdname map #' @export map_int <- function(.x, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - i <- 0L - with_indexed_errors(i = i, - .Call(map_impl, environment(), "integer", .progress) - ) + map_(.x, .f, ..., .type = "integer", .progress = .progress) } #' @rdname map #' @export map_dbl <- function(.x, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - i <- 0L - with_indexed_errors(i = i, - .Call(map_impl, environment(), "double", .progress) - ) + map_(.x, .f, ..., .type = "double", .progress = .progress) } #' @rdname map #' @export map_chr <- function(.x, .f, ..., .progress = FALSE) { + map_(.x, .f, ..., .type = "character", .progress = .progress) +} + +map_ <- function(.x, .f, ..., .type, .progress = FALSE, .error_call = caller_env()) { .f <- as_mapper(.f, ...) i <- 0L - with_indexed_errors(i = i, - .Call(map_impl, environment(), "character", .progress) + with_indexed_errors( + i = i, + error_call = .error_call, + .Call(map_impl, environment(), .type, .progress, .error_call) ) } + #' @rdname map #' @param .ptype If `NULL`, the default, the output type is the common type #' of the elements of the result. Otherwise, supply a "prototype" giving @@ -182,7 +173,6 @@ with_indexed_errors <- function(expr, i, error_call = caller_env()) { error = function(cnd) { if (i == 0L) { # error happened before or after loop - cnd_signal(cnd) } else { cli::cli_abort( "Can't compute index {i}.", diff --git a/R/map2.R b/R/map2.R index 46cc56ed..8449a455 100644 --- a/R/map2.R +++ b/R/map2.R @@ -31,46 +31,36 @@ #' mods <- by_cyl |> map(\(df) lm(mpg ~ wt, data = df)) #' map2(mods, by_cyl, predict) map2 <- function(.x, .y, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - i <- 0L - with_indexed_errors(i = i, - .Call(map2_impl, environment(), "list", .progress) - ) + map2_(.x, .y, .f, ..., .type = "list", .progress = .progress) } #' @export #' @rdname map2 map2_lgl <- function(.x, .y, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - i <- 0L - with_indexed_errors(i = i, - .Call(map2_impl, environment(), "logical", .progress) - ) + map2_(.x, .y, .f, ..., .type = "logical", .progress = .progress) } #' @export #' @rdname map2 map2_int <- function(.x, .y, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - i <- 0L - with_indexed_errors(i = i, - .Call(map2_impl, environment(), "integer", .progress) - ) + map2_(.x, .y, .f, ..., .type = "integer", .progress = .progress) } #' @export #' @rdname map2 map2_dbl <- function(.x, .y, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - i <- 0L - with_indexed_errors(i = i, - .Call(map2_impl, environment(), "double", .progress) - ) + map2_(.x, .y, .f, ..., .type = "double", .progress = .progress) } #' @export #' @rdname map2 map2_chr <- function(.x, .y, .f, ..., .progress = FALSE) { + map2_(.x, .y, .f, ..., .type = "character", .progress = .progress) +} + +map2_ <- function(.x, .y, .f, ..., .type, .progress = FALSE, .error_call = caller_env()) { .f <- as_mapper(.f, ...) i <- 0L - with_indexed_errors(i = i, - .Call(map2_impl, environment(), "character", .progress) + with_indexed_errors( + i = i, + error_call = .error_call, + .Call(map2_impl, environment(), .type, .progress, .error_call) ) } diff --git a/R/pmap.R b/R/pmap.R index 1e41715e..114418de 100644 --- a/R/pmap.R +++ b/R/pmap.R @@ -75,70 +75,45 @@ #' map2_dbl(df$x, df$y, min) #' pmap_dbl(df, min) pmap <- function(.l, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - if (is.data.frame(.l)) { - .l <- as.list(.l) - } - - i <- 0L - with_indexed_errors(i = i, - .Call(pmap_impl, environment(), "list", .progress) - ) + pmap_(.l, .f, ..., .type = "list", .progress = .progress) } #' @export #' @rdname pmap pmap_lgl <- function(.l, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - if (is.data.frame(.l)) { - .l <- as.list(.l) - } - - i <- 0L - with_indexed_errors(i = i, - .Call(pmap_impl, environment(), "logical", .progress) - ) + pmap_(.l, .f, ..., .type = "logical", .progress = .progress) } #' @export #' @rdname pmap pmap_int <- function(.l, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - if (is.data.frame(.l)) { - .l <- as.list(.l) - } - - i <- 0L - with_indexed_errors(i = i, - .Call(pmap_impl, environment(), "integer", .progress) - ) + pmap_(.l, .f, ..., .type = "integer", .progress = .progress) } #' @export #' @rdname pmap pmap_dbl <- function(.l, .f, ..., .progress = FALSE) { - .f <- as_mapper(.f, ...) - if (is.data.frame(.l)) { - .l <- as.list(.l) - } - - i <- 0L - with_indexed_errors(i = i, - .Call(pmap_impl, environment(), "double", .progress) - ) + pmap_(.l, .f, ..., .type = "double", .progress = .progress) } #' @export #' @rdname pmap pmap_chr <- function(.l, .f, ..., .progress = FALSE) { + pmap_(.l, .f, ..., .type = "character", .progress = .progress) +} + +pmap_ <- function(.l, .f, ..., .type, .progress = FALSE, .error_call = caller_env()) { .f <- as_mapper(.f, ...) if (is.data.frame(.l)) { .l <- as.list(.l) } - i <- 0L - with_indexed_errors(i = i, - .Call(pmap_impl, environment(), "character", .progress) + + with_indexed_errors( + i = i, + error_call = .error_call, + .Call(pmap_impl, environment(), .type, .progress, .error_call) ) } + #' @export #' @rdname pmap pmap_vec <- function(.l, .f, ..., .ptype = NULL, .progress = FALSE) { diff --git a/src/init.c b/src/init.c index 9bd3e6c8..d05d2cc9 100644 --- a/src/init.c +++ b/src/init.c @@ -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); -extern SEXP map2_impl(SEXP, SEXP, SEXP); -extern SEXP pmap_impl(SEXP, SEXP, SEXP); +extern SEXP map_impl(SEXP, SEXP, SEXP, SEXP); +extern SEXP map2_impl(SEXP, SEXP, SEXP, SEXP); +extern SEXP pmap_impl(SEXP, SEXP, SEXP, SEXP); extern SEXP transpose_impl(SEXP, SEXP); extern SEXP vflatten_impl(SEXP, SEXP); @@ -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, 3}, - {"map2_impl", (DL_FUNC) &map2_impl, 3}, - {"pmap_impl", (DL_FUNC) &pmap_impl, 3}, + {"map_impl", (DL_FUNC) &map_impl, 4}, + {"map2_impl", (DL_FUNC) &map2_impl, 4}, + {"pmap_impl", (DL_FUNC) &pmap_impl, 4}, {"transpose_impl", (DL_FUNC) &transpose_impl, 2}, {"vflatten_impl", (DL_FUNC) &vflatten_impl, 2}, {"purrr_eval", (DL_FUNC) &Rf_eval, 2}, diff --git a/src/map.c b/src/map.c index 42df4b93..f4a7abdb 100644 --- a/src/map.c +++ b/src/map.c @@ -25,12 +25,12 @@ void copy_names(SEXP from, SEXP to) { UNPROTECT(1); } -void check_vector(SEXP x, const char *name, SEXP env) { +void check_vector(SEXP x, const char *name, SEXP error_call) { if (Rf_isNull(x) || Rf_isVector(x) || Rf_isPairList(x)) { return; } r_abort_call( - env, + error_call, "`%s` must be a vector, not %s.", name, rlang_obj_type_friendly_full(x, true, false) ); @@ -68,14 +68,14 @@ SEXP call_loop(SEXP env, SEXP call, int n, SEXPTYPE type, int force_args, return out; } -SEXP map_impl(SEXP env, SEXP type_, SEXP progress) { +SEXP map_impl(SEXP env, SEXP type_, SEXP progress, SEXP error_call) { SEXP x = Rf_install(".x"); SEXP f = Rf_install(".f"); SEXP i = Rf_install("i"); SEXPTYPE type = Rf_str2type(CHAR(Rf_asChar(type_))); SEXP x_val = PROTECT(Rf_eval(x, env)); - check_vector(x_val, ".x", env); + check_vector(x_val, ".x", error_call); int n = Rf_length(x_val); if (n == 0) { @@ -99,7 +99,7 @@ SEXP map_impl(SEXP env, SEXP type_, SEXP progress) { return out; } -SEXP map2_impl(SEXP env, SEXP type_, SEXP progress) { +SEXP map2_impl(SEXP env, SEXP type_, SEXP progress, SEXP error_call) { SEXP x = Rf_install(".x"); SEXP y = Rf_install(".y"); SEXP f = Rf_install(".f"); @@ -107,14 +107,14 @@ SEXP map2_impl(SEXP env, SEXP type_, SEXP progress) { SEXPTYPE type = Rf_str2type(CHAR(Rf_asChar(type_))); SEXP x_val = PROTECT(Rf_eval(x, env)); - check_vector(x_val, ".x", env); + check_vector(x_val, ".x", error_call); SEXP y_val = PROTECT(Rf_eval(y, env)); - check_vector(y_val, ".y", env); + check_vector(y_val, ".y", error_call); int nx = Rf_length(x_val), ny = Rf_length(y_val); if (nx != ny && nx != 1 && ny != 1) { r_abort_call( - env, + error_call, "`.y must have length 1 or %i, not %i.", nx, ny ); @@ -134,14 +134,14 @@ SEXP map2_impl(SEXP env, SEXP type_, SEXP progress) { return out; } -SEXP pmap_impl(SEXP env, SEXP type_, SEXP progress) { +SEXP pmap_impl(SEXP env, SEXP type_, SEXP progress, SEXP error_call) { SEXP l = Rf_install(".l"); SEXP l_val = PROTECT(Rf_eval(l, env)); SEXPTYPE type = Rf_str2type(CHAR(Rf_asChar(type_))); if (!Rf_isVectorList(l_val)) { r_abort_call( - env, + error_call, "`.l` must be a list, not %s.", rlang_obj_type_friendly_full(l_val, true, false) ); @@ -156,7 +156,7 @@ SEXP pmap_impl(SEXP env, SEXP type_, SEXP progress) { if (!Rf_isVector(j_val) && !Rf_isNull(j_val)) { r_abort_call( - env, + error_call, "`.l[[%i]]` must be a vector, not %s.", j + 1, rlang_obj_type_friendly_full(j_val, true, false) @@ -173,7 +173,7 @@ SEXP pmap_impl(SEXP env, SEXP type_, SEXP progress) { n = nj; } else if (nj != n) { r_abort_call( - env, + error_call, "`.l[[%i]]` must have length 1 or %i, not %i.", j + 1, n, nj ); diff --git a/tests/testthat/_snaps/map-raw.md b/tests/testthat/_snaps/map-raw.md index 787e873b..e5b0db6c 100644 --- a/tests/testthat/_snaps/map-raw.md +++ b/tests/testthat/_snaps/map-raw.md @@ -18,9 +18,6 @@ Warning: `imap_raw()` was deprecated in purrr 1.0.0. Please use `imap_vec()` instead. - Warning: - `map2_raw()` was deprecated in purrr 1.0.0. - Please use `map2_vec()` instead. Code . <- pmap_raw(list(), ~.x) Condition From f2961ecc792115525ba1a60db28b8fa379fb9674 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 22 Sep 2022 07:47:46 -0500 Subject: [PATCH 19/23] Tweak with_indexed_errors --- R/map.R | 4 ++-- tests/testthat/_snaps/keep.md | 4 ++-- tests/testthat/_snaps/map-depth.md | 12 ++++++------ tests/testthat/_snaps/map-if-at.md | 2 +- tests/testthat/_snaps/map.md | 6 +++--- tests/testthat/_snaps/map2.md | 4 ++-- tests/testthat/_snaps/modify.md | 2 +- tests/testthat/_snaps/pmap.md | 4 ++-- 8 files changed, 19 insertions(+), 19 deletions(-) diff --git a/R/map.R b/R/map.R index ed1ffb9f..bc4f2d4b 100644 --- a/R/map.R +++ b/R/map.R @@ -172,10 +172,10 @@ with_indexed_errors <- function(expr, i, error_call = caller_env()) { expr, error = function(cnd) { if (i == 0L) { - # error happened before or after loop + # Error happened before or after loop } else { cli::cli_abort( - "Can't compute index {i}.", + "In index {i}.", parent = cnd, call = error_call ) diff --git a/tests/testthat/_snaps/keep.md b/tests/testthat/_snaps/keep.md index d081c7a2..1c28c33a 100644 --- a/tests/testthat/_snaps/keep.md +++ b/tests/testthat/_snaps/keep.md @@ -4,14 +4,14 @@ keep(1:3, ~NA) Condition Error in `map_lgl()`: - ! Can't compute index 1. + ! In index 1. Caused by error in `keep()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. Code discard(1:3, ~NA) Condition Error in `map_lgl()`: - ! Can't compute index 1. + ! In index 1. Caused by error in `discard()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/map-depth.md b/tests/testthat/_snaps/map-depth.md index 9adf967f..01d53459 100644 --- a/tests/testthat/_snaps/map-depth.md +++ b/tests/testthat/_snaps/map-depth.md @@ -4,11 +4,11 @@ map_depth(x1, 6, length) Condition Error in `.fmap()`: - ! Can't compute index 1. + ! In index 1. Caused by error in `.fmap()`: - ! Can't compute index 1. + ! In index 1. Caused by error in `.fmap()`: - ! Can't compute index 1. + ! In index 1. Caused by error in `map_depth()`: ! List not deep enough @@ -26,11 +26,11 @@ modify_depth(x1, 5, length) Condition Error in `map()`: - ! Can't compute index 1. + ! In index 1. Caused by error in `map()`: - ! Can't compute index 1. + ! In index 1. Caused by error in `map()`: - ! Can't compute index 1. + ! In index 1. Caused by error in `modify_depth()`: ! List not deep enough diff --git a/tests/testthat/_snaps/map-if-at.md b/tests/testthat/_snaps/map-if-at.md index 9ce259c0..3f10b53b 100644 --- a/tests/testthat/_snaps/map-if-at.md +++ b/tests/testthat/_snaps/map-if-at.md @@ -4,7 +4,7 @@ map_if(1:3, ~NA, ~"foo") Condition Error in `map_lgl()`: - ! Can't compute index 1. + ! In index 1. Caused by error in `map_if()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index af9eb41f..9ce189b4 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -20,21 +20,21 @@ map_int(1:3, ~ fail_at_3(.x, 2:1)) Condition Error in `map_int()`: - ! Can't compute index 3. + ! In index 3. Caused by error: ! Result must be length 1, not 2 Code map_int(1:3, ~ fail_at_3("x")) Condition Error in `map_int()`: - ! Can't compute index 1. + ! In index 1. Caused by error: ! Can't coerce from a character to a integer. Code map(1:3, ~ fail_at_3(stop("Doesn't work"))) Condition Error in `map()`: - ! Can't compute index 1. + ! In index 1. Caused by error in `fail_at_3()`: ! Doesn't work diff --git a/tests/testthat/_snaps/map2.md b/tests/testthat/_snaps/map2.md index 8ade04cd..3932231d 100644 --- a/tests/testthat/_snaps/map2.md +++ b/tests/testthat/_snaps/map2.md @@ -4,14 +4,14 @@ map2_int(1, 1, ~"x") Condition Error in `map2_int()`: - ! Can't compute index 1. + ! In index 1. Caused by error: ! Can't coerce from a character to a integer. Code map2_int(1, 1, ~ 1:2) Condition Error in `map2_int()`: - ! Can't compute index 1. + ! In index 1. Caused by error: ! Result must be length 1, not 2 Code diff --git a/tests/testthat/_snaps/modify.md b/tests/testthat/_snaps/modify.md index 1ae279a2..7dde299a 100644 --- a/tests/testthat/_snaps/modify.md +++ b/tests/testthat/_snaps/modify.md @@ -63,7 +63,7 @@ modify_if(list(1, 2), ~NA, ~"foo") Condition Error in `map_lgl()`: - ! Can't compute index 1. + ! In index 1. Caused by error in `modify_if()`: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/pmap.md b/tests/testthat/_snaps/pmap.md index b89b6f93..60bde9d7 100644 --- a/tests/testthat/_snaps/pmap.md +++ b/tests/testthat/_snaps/pmap.md @@ -4,14 +4,14 @@ pmap_int(list(1), ~"x") Condition Error in `pmap_int()`: - ! Can't compute index 1. + ! In index 1. Caused by error: ! Can't coerce from a character to a integer. Code pmap_int(list(1), ~ 1:2) Condition Error in `pmap_int()`: - ! Can't compute index 1. + ! In index 1. Caused by error: ! Result must be length 1, not 2 Code From 5f0d5b95679ccc071a84d97d7391edc80214e915 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 22 Sep 2022 07:53:09 -0500 Subject: [PATCH 20/23] Use explicit error_call in predicate using functions --- R/utils.R | 4 ++-- tests/testthat/_snaps/detect.md | 6 +++--- tests/testthat/_snaps/keep.md | 8 ++++---- tests/testthat/_snaps/map-if-at.md | 4 ++-- tests/testthat/_snaps/modify.md | 4 ++-- tests/testthat/test-detect.R | 2 +- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/R/utils.R b/R/utils.R index 0d080b29..3a43902f 100644 --- a/R/utils.R +++ b/R/utils.R @@ -41,8 +41,8 @@ where_if <- function(.x, .p, ..., .error_call = caller_env()) { stopifnot(length(.p) == length(.x)) .p } else { - .p <- as_predicate(.p, ..., .mapper = TRUE, .error_call = .error_call) - map_lgl(.x, .p, ...) + .p <- as_predicate(.p, ..., .mapper = TRUE, .error_call = NULL) + map_(.x, .p, ..., .type = "logical", .error_call = .error_call) } } diff --git a/tests/testthat/_snaps/detect.md b/tests/testthat/_snaps/detect.md index a6533335..916ee460 100644 --- a/tests/testthat/_snaps/detect.md +++ b/tests/testthat/_snaps/detect.md @@ -9,10 +9,10 @@ --- Code - detect(list(1:2, 2), function(...) NA) + detect_index(list(1:2, 2), is.na) Condition - Error in `detect()`: - ! `.f()` must return a single `TRUE` or `FALSE`, not `NA`. + Error in `detect_index()`: + ! `.f()` must return a single `TRUE` or `FALSE`, not a logical vector. # `.right` argument is retired diff --git a/tests/testthat/_snaps/keep.md b/tests/testthat/_snaps/keep.md index 1c28c33a..a63f47ec 100644 --- a/tests/testthat/_snaps/keep.md +++ b/tests/testthat/_snaps/keep.md @@ -3,15 +3,15 @@ Code keep(1:3, ~NA) Condition - Error in `map_lgl()`: + Error in `keep()`: ! In index 1. - Caused by error in `keep()`: + Caused by error: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. Code discard(1:3, ~NA) Condition - Error in `map_lgl()`: + Error in `discard()`: ! In index 1. - Caused by error in `discard()`: + Caused by error: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/map-if-at.md b/tests/testthat/_snaps/map-if-at.md index 3f10b53b..3717f1c6 100644 --- a/tests/testthat/_snaps/map-if-at.md +++ b/tests/testthat/_snaps/map-if-at.md @@ -3,8 +3,8 @@ Code map_if(1:3, ~NA, ~"foo") Condition - Error in `map_lgl()`: + Error in `map_if()`: ! In index 1. - Caused by error in `map_if()`: + Caused by error: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. diff --git a/tests/testthat/_snaps/modify.md b/tests/testthat/_snaps/modify.md index 7dde299a..8dd64c7b 100644 --- a/tests/testthat/_snaps/modify.md +++ b/tests/testthat/_snaps/modify.md @@ -62,9 +62,9 @@ Code modify_if(list(1, 2), ~NA, ~"foo") Condition - Error in `map_lgl()`: + Error in `modify_if()`: ! In index 1. - Caused by error in `modify_if()`: + Caused by error: ! `.p()` must return a single `TRUE` or `FALSE`, not `NA`. # user friendly error for non-supported cases diff --git a/tests/testthat/test-detect.R b/tests/testthat/test-detect.R index 0d1a3b1e..192e7bd4 100644 --- a/tests/testthat/test-detect.R +++ b/tests/testthat/test-detect.R @@ -23,7 +23,7 @@ test_that("has_element checks whether a list contains an object", { test_that("`detect()` requires a predicate function", { expect_snapshot(detect(list(1:2, 2), is.na), error = TRUE) - expect_snapshot(detect(list(1:2, 2), function(...) NA), error = TRUE) + expect_snapshot(detect_index(list(1:2, 2), is.na), error = TRUE) }) From b4c9e936d9e0f9fba5590f42b7e083c1b073791b Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 22 Sep 2022 08:02:20 -0500 Subject: [PATCH 21/23] Use obj_type_friendly in coerce.c --- src/coerce.c | 10 ++++++++-- tests/testthat/_snaps/map.md | 2 +- tests/testthat/_snaps/map2.md | 2 +- tests/testthat/_snaps/pmap.md | 2 +- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/coerce.c b/src/coerce.c index 56614157..ef3dff11 100644 --- a/src/coerce.c +++ b/src/coerce.c @@ -3,9 +3,15 @@ #include #include +#include "conditions.h" + void cant_coerce(SEXP from, SEXP to, int i) { - Rf_errorcall(R_NilValue, "Can't coerce from a %s to a %s.", - Rf_type2char(TYPEOF(from)), Rf_type2char(TYPEOF(to))); + Rf_errorcall( + R_NilValue, + "Can't coerce from %s to %s.", + rlang_obj_type_friendly_full(from, false, false), + rlang_obj_type_friendly_full(to, false, false) + ); } int real_to_logical(double x, SEXP from, SEXP to, int i) { diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index 9ce189b4..ac731fe5 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -29,7 +29,7 @@ Error in `map_int()`: ! In index 1. Caused by error: - ! Can't coerce from a character to a integer. + ! Can't coerce from a character vector to an integer vector. Code map(1:3, ~ fail_at_3(stop("Doesn't work"))) Condition diff --git a/tests/testthat/_snaps/map2.md b/tests/testthat/_snaps/map2.md index 3932231d..d52087b3 100644 --- a/tests/testthat/_snaps/map2.md +++ b/tests/testthat/_snaps/map2.md @@ -6,7 +6,7 @@ Error in `map2_int()`: ! In index 1. Caused by error: - ! Can't coerce from a character to a integer. + ! Can't coerce from a character vector to an integer vector. Code map2_int(1, 1, ~ 1:2) Condition diff --git a/tests/testthat/_snaps/pmap.md b/tests/testthat/_snaps/pmap.md index 60bde9d7..7c2f31db 100644 --- a/tests/testthat/_snaps/pmap.md +++ b/tests/testthat/_snaps/pmap.md @@ -6,7 +6,7 @@ Error in `pmap_int()`: ! In index 1. Caused by error: - ! Can't coerce from a character to a integer. + ! Can't coerce from a character vector to an integer vector. Code pmap_int(list(1), ~ 1:2) Condition From 2e7e817d44dca28be60c80ae0ce4647e1eb9f2e2 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 22 Sep 2022 08:02:50 -0500 Subject: [PATCH 22/23] Add missing full stop --- src/map.c | 2 +- tests/testthat/_snaps/map.md | 2 +- tests/testthat/_snaps/map2.md | 2 +- tests/testthat/_snaps/pmap.md | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/map.c b/src/map.c index f4a7abdb..9840edae 100644 --- a/src/map.c +++ b/src/map.c @@ -54,7 +54,7 @@ SEXP call_loop(SEXP env, SEXP call, int n, SEXPTYPE type, int force_args, SEXP res = PROTECT(R_forceAndCall(call, force_args, env)); if (type != VECSXP && Rf_length(res) != 1) { - Rf_errorcall(R_NilValue, "Result must be length 1, not %i", Rf_length(res)); + Rf_errorcall(R_NilValue, "Result must be length 1, not %i.", Rf_length(res)); } set_vector_value(out, i, res, 0); diff --git a/tests/testthat/_snaps/map.md b/tests/testthat/_snaps/map.md index ac731fe5..7b129306 100644 --- a/tests/testthat/_snaps/map.md +++ b/tests/testthat/_snaps/map.md @@ -22,7 +22,7 @@ Error in `map_int()`: ! In index 3. Caused by error: - ! Result must be length 1, not 2 + ! Result must be length 1, not 2. Code map_int(1:3, ~ fail_at_3("x")) Condition diff --git a/tests/testthat/_snaps/map2.md b/tests/testthat/_snaps/map2.md index d52087b3..6e32a3f7 100644 --- a/tests/testthat/_snaps/map2.md +++ b/tests/testthat/_snaps/map2.md @@ -13,7 +13,7 @@ Error in `map2_int()`: ! In index 1. Caused by error: - ! Result must be length 1, not 2 + ! Result must be length 1, not 2. Code map2_vec(1, 1, ~1, .ptype = character()) Condition diff --git a/tests/testthat/_snaps/pmap.md b/tests/testthat/_snaps/pmap.md index 7c2f31db..7bca9be0 100644 --- a/tests/testthat/_snaps/pmap.md +++ b/tests/testthat/_snaps/pmap.md @@ -13,7 +13,7 @@ Error in `pmap_int()`: ! In index 1. Caused by error: - ! Result must be length 1, not 2 + ! Result must be length 1, not 2. Code pmap_vec(list(1), ~1, .ptype = character()) Condition From 8d5f13e80738fa343030bb0654ec7478608530e1 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 22 Sep 2022 14:08:50 -0500 Subject: [PATCH 23/23] Better argument order --- R/deprec-invoke.R | 2 +- R/map-raw.R | 8 ++++---- R/map.R | 12 ++++++------ R/map2.R | 12 ++++++------ R/pmap.R | 12 ++++++------ 5 files changed, 23 insertions(+), 23 deletions(-) diff --git a/R/deprec-invoke.R b/R/deprec-invoke.R index db1c2ae4..303a4853 100644 --- a/R/deprec-invoke.R +++ b/R/deprec-invoke.R @@ -147,7 +147,7 @@ invoke_map_raw <- function(.f, .x = list(NULL), ..., .env = NULL) { .env <- .env %||% parent.frame() .f <- as_invoke_function(.f) - map2_(.f, .x, invoke, ..., .type = "raw") + map2_("raw", .f, .x, invoke, ...) } #' @rdname invoke diff --git a/R/map-raw.R b/R/map-raw.R index f45d2968..3568b435 100644 --- a/R/map-raw.R +++ b/R/map-raw.R @@ -11,28 +11,28 @@ #' @export map_raw <- function(.x, .f, ...) { lifecycle::deprecate_soft("1.0.0", "map_raw()", "map_vec()") - map_(.x, .f, ..., .type = "raw") + map_("raw", .x, .f, ...) } #' @export #' @rdname map_raw map2_raw <- function(.x, .y, .f, ...) { lifecycle::deprecate_soft("1.0.0", "map2_raw()", "map2_vec()") - map2_(.x, .y, .f, ..., .type = "raw") + map2_("raw", .x, .y, .f, ...) } #' @rdname map_raw #' @export imap_raw <- function(.x, .f, ...) { lifecycle::deprecate_soft("1.0.0", "imap_raw()", "imap_vec()") - map2_(.x, vec_index(.x), .f, ..., .type = "raw") + map2_("raw", .x, vec_index(.x), .f, ...) } #' @export #' @rdname map_raw pmap_raw <- function(.l, .f, ...) { lifecycle::deprecate_soft("1.0.0", "pmap_raw()", "pmap_vec()") - pmap_(.l, .f, ..., .type = "raw") + pmap_("raw", .l, .f, ...) } #' @export diff --git a/R/map.R b/R/map.R index bc4f2d4b..3c6a6d25 100644 --- a/R/map.R +++ b/R/map.R @@ -112,34 +112,34 @@ #' map(summary) |> #' map_dbl("r.squared") map <- function(.x, .f, ..., .progress = FALSE) { - map_(.x, .f, ..., .type = "list", .progress = .progress) + map_("list", .x, .f, ..., .progress = .progress) } #' @rdname map #' @export map_lgl <- function(.x, .f, ..., .progress = FALSE) { - map_(.x, .f, ..., .type = "logical", .progress = .progress) + map_("logical", .x, .f, ..., .progress = .progress) } #' @rdname map #' @export map_int <- function(.x, .f, ..., .progress = FALSE) { - map_(.x, .f, ..., .type = "integer", .progress = .progress) + map_("integer", .x, .f, ..., .progress = .progress) } #' @rdname map #' @export map_dbl <- function(.x, .f, ..., .progress = FALSE) { - map_(.x, .f, ..., .type = "double", .progress = .progress) + map_("double", .x, .f, ..., .progress = .progress) } #' @rdname map #' @export map_chr <- function(.x, .f, ..., .progress = FALSE) { - map_(.x, .f, ..., .type = "character", .progress = .progress) + map_("character", .x, .f, ..., .progress = .progress) } -map_ <- function(.x, .f, ..., .type, .progress = FALSE, .error_call = caller_env()) { +map_ <- function(.type, .x, .f, ..., .progress = FALSE, .error_call = caller_env()) { .f <- as_mapper(.f, ...) i <- 0L with_indexed_errors( diff --git a/R/map2.R b/R/map2.R index 8449a455..0f5d5dfd 100644 --- a/R/map2.R +++ b/R/map2.R @@ -31,30 +31,30 @@ #' mods <- by_cyl |> map(\(df) lm(mpg ~ wt, data = df)) #' map2(mods, by_cyl, predict) map2 <- function(.x, .y, .f, ..., .progress = FALSE) { - map2_(.x, .y, .f, ..., .type = "list", .progress = .progress) + map2_("list", .x, .y, .f, ..., .progress = .progress) } #' @export #' @rdname map2 map2_lgl <- function(.x, .y, .f, ..., .progress = FALSE) { - map2_(.x, .y, .f, ..., .type = "logical", .progress = .progress) + map2_("logical", .x, .y, .f, ..., .progress = .progress) } #' @export #' @rdname map2 map2_int <- function(.x, .y, .f, ..., .progress = FALSE) { - map2_(.x, .y, .f, ..., .type = "integer", .progress = .progress) + map2_("integer", .x, .y, .f, ..., .progress = .progress) } #' @export #' @rdname map2 map2_dbl <- function(.x, .y, .f, ..., .progress = FALSE) { - map2_(.x, .y, .f, ..., .type = "double", .progress = .progress) + map2_("double", .x, .y, .f, ..., .progress = .progress) } #' @export #' @rdname map2 map2_chr <- function(.x, .y, .f, ..., .progress = FALSE) { - map2_(.x, .y, .f, ..., .type = "character", .progress = .progress) + map2_("character", .x, .y, .f, ..., .progress = .progress) } -map2_ <- function(.x, .y, .f, ..., .type, .progress = FALSE, .error_call = caller_env()) { +map2_ <- function(.type, .x, .y, .f, ..., .progress = FALSE, .error_call = caller_env()) { .f <- as_mapper(.f, ...) i <- 0L with_indexed_errors( diff --git a/R/pmap.R b/R/pmap.R index 114418de..bcc69d78 100644 --- a/R/pmap.R +++ b/R/pmap.R @@ -75,31 +75,31 @@ #' map2_dbl(df$x, df$y, min) #' pmap_dbl(df, min) pmap <- function(.l, .f, ..., .progress = FALSE) { - pmap_(.l, .f, ..., .type = "list", .progress = .progress) + pmap_("list", .l, .f, ..., .progress = .progress) } #' @export #' @rdname pmap pmap_lgl <- function(.l, .f, ..., .progress = FALSE) { - pmap_(.l, .f, ..., .type = "logical", .progress = .progress) + pmap_("logical", .l, .f, ..., .progress = .progress) } #' @export #' @rdname pmap pmap_int <- function(.l, .f, ..., .progress = FALSE) { - pmap_(.l, .f, ..., .type = "integer", .progress = .progress) + pmap_("integer", .l, .f, ..., .progress = .progress) } #' @export #' @rdname pmap pmap_dbl <- function(.l, .f, ..., .progress = FALSE) { - pmap_(.l, .f, ..., .type = "double", .progress = .progress) + pmap_("double", .l, .f, ..., .progress = .progress) } #' @export #' @rdname pmap pmap_chr <- function(.l, .f, ..., .progress = FALSE) { - pmap_(.l, .f, ..., .type = "character", .progress = .progress) + pmap_("character", .l, .f, ..., .progress = .progress) } -pmap_ <- function(.l, .f, ..., .type, .progress = FALSE, .error_call = caller_env()) { +pmap_ <- function(.type, .l, .f, ..., .progress = FALSE, .error_call = caller_env()) { .f <- as_mapper(.f, ...) if (is.data.frame(.l)) { .l <- as.list(.l)