From 781d630b3ae059e7e8f2176c7261a48ba17a53d3 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 10 Jan 2024 17:15:02 -0600 Subject: [PATCH] Translations for namespaced functions must exist (#1432) I'm not sure how this broke, but the code for handling this already exists in `translate_sql()`, so I'm not sure why we started doing some `::` handling in `partial_eval()`. Fixes #1426 --- NEWS.md | 3 +++ R/tidyeval.R | 18 +++++------------- R/translate-sql.R | 2 +- tests/testthat/_snaps/translate-sql.md | 22 +++++++++++++++++----- tests/testthat/test-tidyeval.R | 11 ----------- tests/testthat/test-translate-sql.R | 16 +++++++++++++--- 6 files changed, 39 insertions(+), 33 deletions(-) diff --git a/NEWS.md b/NEWS.md index 72666da9f..c5a3c5fc5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # dbplyr (development version) +* Namespaced dplyr calls now error if the function doesn't exist, or + a translation is not available (#1426). + * `db_sql_render()` correctly passes on `...` when re-calling with `sql_options` set (#1394). diff --git a/R/tidyeval.R b/R/tidyeval.R index f6d9fcc9c..68a1f1724 100644 --- a/R/tidyeval.R +++ b/R/tidyeval.R @@ -162,11 +162,6 @@ partial_eval_sym <- function(sym, data, env) { } } -is_namespaced_dplyr_call <- function(call) { - packages <- c("base", "dplyr", "stringr", "lubridate", "clock") - is_symbol(call[[1]], "::") && is_symbol(call[[2]], packages) -} - is_mask_pronoun <- function(call) { is_call(call, c("$", "[["), n = 2) && is_symbol(call[[2]], c(".data", ".env")) } @@ -190,11 +185,9 @@ partial_eval_call <- function(call, data, env) { call[[1]] <- fun <- sym(fun_name) } - # So are compound calls, EXCEPT dplyr::foo() - if (is.call(fun)) { - if (is_namespaced_dplyr_call(fun)) { - call[[1]] <- fun[[3]] - } else if (is_mask_pronoun(fun)) { + # Compound calls, apart from `::` aren't translatable + if (is_call(fun) && !is_call(fun, "::")) { + if (is_mask_pronoun(fun)) { stop("Use local() or remote() to force evaluation of functions", call. = FALSE) } else { return(eval_bare(call, env)) @@ -216,10 +209,9 @@ partial_eval_call <- function(call, data, env) { } else { # Process call arguments recursively, unless user has manually called # remote/local - name <- as_string(call[[1]]) - if (name == "local") { + if (is_call(call, "local")) { eval_bare(call[[2]], env) - } else if (name == "remote") { + } else if (is_call(call, "remote")) { call[[2]] } else { call[-1] <- lapply(call[-1], partial_eval, data = data, env = env) diff --git a/R/translate-sql.R b/R/translate-sql.R index 044b16374..fe281017c 100644 --- a/R/translate-sql.R +++ b/R/translate-sql.R @@ -185,7 +185,7 @@ sql_data_mask <- function(expr, if (env_has(special_calls2, name) || env_has(special_calls, name)) { env_get(special_calls2, name, inherit = TRUE) } else { - cli_abort("No known translation for {.fun {pkg}::{name}}") + cli_abort("No known translation", call = call2(call2("::", sym(pkg), sym(name)))) } } diff --git a/tests/testthat/_snaps/translate-sql.md b/tests/testthat/_snaps/translate-sql.md index 60fefbb90..db49b687c 100644 --- a/tests/testthat/_snaps/translate-sql.md +++ b/tests/testthat/_snaps/translate-sql.md @@ -18,20 +18,32 @@ Condition Error: ! There is no package called NOSUCHPACKAGE - ---- - Code test_translate_sql(dbplyr::NOSUCHFUNCTION()) Condition Error: ! "NOSUCHFUNCTION" is not an exported object from dbplyr + Code + test_translate_sql(base::abbreviate(x)) + Condition + Error in `base::abbreviate()`: + ! No known translation --- Code - test_translate_sql(base::abbreviate(x)) + lz %>% mutate(x = NOSUCHPACKAGE::foo()) Condition Error: - ! No known translation for `base::abbreviate()` + ! There is no package called NOSUCHPACKAGE + Code + lz %>% mutate(x = dbplyr::NOSUCHFUNCTION()) + Condition + Error: + ! "NOSUCHFUNCTION" is not an exported object from dbplyr + Code + lz %>% mutate(x = base::abbreviate(x)) + Condition + Error in `base::abbreviate()`: + ! No known translation diff --git a/tests/testthat/test-tidyeval.R b/tests/testthat/test-tidyeval.R index ef105ce8c..ac1fefee3 100644 --- a/tests/testthat/test-tidyeval.R +++ b/tests/testthat/test-tidyeval.R @@ -30,17 +30,6 @@ test_that("using environment of inlined quosures", { expect_equal(capture_dot(lf, f(!!quo)), quote(f(x + 20))) }) -test_that("namespaced calls to dplyr functions are stripped", { - lf <- lazy_frame(x = 1, y = 2) - - expect_equal(partial_eval(quote(dplyr::n()), lf), expr(n())) - expect_equal(partial_eval(quote(base::paste(x, "a")), lf), expr(paste(x, "a"))) - # hack to avoid check complaining about not declared imports - expect_equal(partial_eval(rlang::parse_expr("stringr::str_to_lower(x)"), lf), expr(str_to_lower(x))) - expect_equal(partial_eval(rlang::parse_expr("lubridate::today()"), lf), expr(today())) - expect_equal(partial_eval(rlang::parse_expr("clock::add_years(x, 1)"), lf), expr(add_years(x, 1))) -}) - test_that("use quosure environment for unevaluted formulas", { lf <- lazy_frame(x = 1, y = 2) diff --git a/tests/testthat/test-translate-sql.R b/tests/testthat/test-translate-sql.R index 3a1d8873b..fb7ace800 100644 --- a/tests/testthat/test-translate-sql.R +++ b/tests/testthat/test-translate-sql.R @@ -14,9 +14,19 @@ test_that("namespace calls are translated", { expect_equal(test_translate_sql(dplyr::n(), window = FALSE), sql("COUNT(*)")) expect_equal(test_translate_sql(base::ceiling(x)), sql("CEIL(`x`)")) - expect_snapshot(error = TRUE, test_translate_sql(NOSUCHPACKAGE::foo())) - expect_snapshot(error = TRUE, test_translate_sql(dbplyr::NOSUCHFUNCTION())) - expect_snapshot(error = TRUE, test_translate_sql(base::abbreviate(x))) + expect_snapshot(error = TRUE, { + test_translate_sql(NOSUCHPACKAGE::foo()) + test_translate_sql(dbplyr::NOSUCHFUNCTION()) + test_translate_sql(base::abbreviate(x)) + }) + + lz <- lazy_frame(x = 1) + # Also test full pipeline to ensure that they make it through partial_eval + expect_snapshot(error = TRUE, { + lz %>% mutate(x = NOSUCHPACKAGE::foo()) + lz %>% mutate(x = dbplyr::NOSUCHFUNCTION()) + lz %>% mutate(x = base::abbreviate(x)) + }) }) test_that("Wrong number of arguments raises error", {