Merge branch 'main' into list-access

NEWS.md

@@ -5,6 +5,9 @@
 
 * `x$name` never attempts to evaluate `name` (#1368).
 
+* 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).
 

R/tidyeval.R

@@ -179,11 +179,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"))
 }
@@ -207,11 +202,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))
@@ -233,10 +226,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 if (name == "$") {
       # Only the 1st argument is evaluated

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))))
     }
   }
 

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
 

tests/testthat/test-tidyeval.R

@@ -54,17 +54,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)
 

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", {