From 08f7436c1f04d51f1b15c1937c4fe4ab4cd68633 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kirill=20M=C3=BCller?= Date: Thu, 15 Aug 2024 19:38:30 +0200 Subject: [PATCH 1/3] Fix edge case when coercing data frames to matrices --- R/data-mask.R | 11 ++++++++++- tests/testthat/test-data-mask.R | 7 +++++++ 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 tests/testthat/test-data-mask.R diff --git a/R/data-mask.R b/R/data-mask.R index b72a708289..5b04db7091 100644 --- a/R/data-mask.R +++ b/R/data-mask.R @@ -11,7 +11,16 @@ DataMask <- R6Class("DataMask", frame <- caller_env(n = 2) local_mask(self, frame) - names_bindings <- chr_unserialise_unicode(names2(data)) + names <- names(data) + + if (is.null(names)) { + abort("Can't transform a data frame with `NULL` names.") + } + if (vec_any_missing(names)) { + abort("Can't transform a data frame with missing names.") + } + + names_bindings <- chr_unserialise_unicode(names) if (any(names_bindings == "")) { # `names2()` converted potential `NA` names to `""` already abort("Can't transform a data frame with `NA` or `\"\"` names.", call = error_call) diff --git a/tests/testthat/test-data-mask.R b/tests/testthat/test-data-mask.R new file mode 100644 index 0000000000..675f8001a9 --- /dev/null +++ b/tests/testthat/test-data-mask.R @@ -0,0 +1,7 @@ +test_that("Empty matrix can be coerced to a data frame (#7004)", { + skip_if_not(getRversion() >= "4.4") + expect_error( + slice(as.data.frame(matrix(nrow = 0, ncol = 0)), 1), + NA + ) +}) From d55ec6304239316e97379154ba8782a22bb909df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kirill=20M=C3=BCller?= Date: Thu, 15 Aug 2024 23:58:30 +0200 Subject: [PATCH 2/3] Update test and call --- R/data-mask.R | 10 ++++++++-- tests/testthat/_snaps/filter.md | 2 +- tests/testthat/_snaps/mutate.md | 2 +- tests/testthat/_snaps/summarise.md | 2 +- tests/testthat/test-data-mask.R | 4 ++-- 5 files changed, 13 insertions(+), 7 deletions(-) diff --git a/R/data-mask.R b/R/data-mask.R index 5b04db7091..b50aa182c3 100644 --- a/R/data-mask.R +++ b/R/data-mask.R @@ -14,10 +14,16 @@ DataMask <- R6Class("DataMask", names <- names(data) if (is.null(names)) { - abort("Can't transform a data frame with `NULL` names.") + cli::cli_abort( + "Can't transform a data frame with `NULL` names.", + call = error_call + ) } if (vec_any_missing(names)) { - abort("Can't transform a data frame with missing names.") + cli::cli_abort( + "Can't transform a data frame with missing names.", + call = error_call + ) } names_bindings <- chr_unserialise_unicode(names) diff --git a/tests/testthat/_snaps/filter.md b/tests/testthat/_snaps/filter.md index 3d24697ea5..39a0f26903 100644 --- a/tests/testthat/_snaps/filter.md +++ b/tests/testthat/_snaps/filter.md @@ -243,7 +243,7 @@ filter(df2) Condition Error in `filter()`: - ! Can't transform a data frame with `NA` or `""` names. + ! Can't transform a data frame with missing names. # can't use `.by` with `.preserve` diff --git a/tests/testthat/_snaps/mutate.md b/tests/testthat/_snaps/mutate.md index de71d9da8c..8c4c2f7de7 100644 --- a/tests/testthat/_snaps/mutate.md +++ b/tests/testthat/_snaps/mutate.md @@ -361,5 +361,5 @@ mutate(df2) Condition Error in `mutate()`: - ! Can't transform a data frame with `NA` or `""` names. + ! Can't transform a data frame with missing names. diff --git a/tests/testthat/_snaps/summarise.md b/tests/testthat/_snaps/summarise.md index 3f80a60a5a..8040d43a9d 100644 --- a/tests/testthat/_snaps/summarise.md +++ b/tests/testthat/_snaps/summarise.md @@ -77,7 +77,7 @@ summarise(df2) Condition Error in `summarise()`: - ! Can't transform a data frame with `NA` or `""` names. + ! Can't transform a data frame with missing names. # summarise() gives meaningful errors diff --git a/tests/testthat/test-data-mask.R b/tests/testthat/test-data-mask.R index 675f8001a9..e82f4b4b08 100644 --- a/tests/testthat/test-data-mask.R +++ b/tests/testthat/test-data-mask.R @@ -1,7 +1,7 @@ test_that("Empty matrix can be coerced to a data frame (#7004)", { skip_if_not(getRversion() >= "4.4") - expect_error( + expect_equal( slice(as.data.frame(matrix(nrow = 0, ncol = 0)), 1), - NA + data.frame() ) }) From f548654bb9f885caa70b4258358949677dce01aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kirill=20M=C3=BCller?= Date: Thu, 15 Aug 2024 23:59:10 +0200 Subject: [PATCH 3/3] NEWS --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index abb7240cc9..7d32827179 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # dplyr (development version) +* Fixed an edge case when coercing data frames to matrices (#7004). + * Fixed an issue where duckplyr's ALTREP data frames were being materialized early due to internal usage of `ncol()` (#7049).