{{}} is causing error on travis

[njtierney]

As suggested by @jimhester , I am submitting a bug report here, although I'm not sure if this is the most informative title. When running coverage tests on travis, I get the following error, reproduced also locally below:

covr::package_coverage("/Users/ntie0001/github/njtierney/brolgar/")
#> Error: Failure in `/private/var/folders/mw/gj7418356js6s29x7wn8crfmljy4wh/T/RtmpEgP8cm/R_LIBS1477940c7ef40/brolgar/brolgar-tests/testthat.Rout.fail`
#> )
#> 17: dplyr::mutate_at(., .vars = dplyr::vars({
#>        {
#>            {
#>                covr:::count("keys_near.R:42:29:42:31:29:31:512:512")
#>                var
#>            }
#>        }
#>    }), .funs = funs)
#> 18: check_dot_cols(.vars, .cols)
#> 19: dplyr::vars({
#>        {
#>            {
#>                covr:::count("keys_near.R:42:29:42:31:29:31:512:512")
#>                var
#>            }
#>        }
#>    })
#> 20: quos(...)
#> 21: rlang::abort(x)
#> 
#> ══ testthat results  ══════════════════════════════════════════════════════════════
#> OK: 136 SKIPPED: 0 WARNINGS: 0 FAILED: 1
#> 1. Error: (unknown) (@test-keys-near.R#2) 
#> 
#> Error: testthat unit tests failed
#> Execution halted

^{Created on 2019-07-18 by the reprex package (v0.3.0)}

Session info

``` r devtools::session_info() #> ─ Session info ────────────────────────────────────────────────────────── #> setting value #> version R version 3.6.1 (2019-07-05) #> os macOS Mojave 10.14.5 #> system x86_64, darwin15.6.0 #> ui X11 #> language (EN) #> collate en_AU.UTF-8 #> ctype en_AU.UTF-8 #> tz Australia/Melbourne #> date 2019-07-18 #> #> ─ Packages ────────────────────────────────────────────────────────────── #> package * version date lib source #> assertthat 0.2.1 2019-03-21 [1] CRAN (R 3.6.0) #> backports 1.1.4 2019-04-10 [1] CRAN (R 3.6.0) #> callr 3.3.0 2019-07-04 [1] CRAN (R 3.6.0) #> cli 1.1.0 2019-03-19 [1] CRAN (R 3.6.0) #> covr 3.2.1 2018-10-18 [1] CRAN (R 3.6.0) #> crayon 1.3.4 2017-09-16 [1] CRAN (R 3.6.0) #> desc 1.2.0 2018-05-01 [1] CRAN (R 3.6.0) #> devtools 2.1.0 2019-07-06 [1] CRAN (R 3.6.0) #> digest 0.6.20 2019-07-04 [1] CRAN (R 3.6.0) #> evaluate 0.14 2019-05-28 [1] CRAN (R 3.6.0) #> fs 1.3.1 2019-05-06 [1] CRAN (R 3.6.0) #> glue 1.3.1 2019-03-12 [1] CRAN (R 3.6.0) #> highr 0.8 2019-03-20 [1] CRAN (R 3.6.0) #> htmltools 0.3.6 2017-04-28 [1] CRAN (R 3.6.0) #> knitr 1.23 2019-05-18 [1] CRAN (R 3.6.0) #> lazyeval 0.2.2 2019-03-15 [1] CRAN (R 3.6.0) #> magrittr 1.5 2014-11-22 [1] CRAN (R 3.6.0) #> memoise 1.1.0 2017-04-21 [1] CRAN (R 3.6.0) #> pkgbuild 1.0.3 2019-03-20 [1] CRAN (R 3.6.0) #> pkgload 1.0.2 2018-10-29 [1] CRAN (R 3.6.0) #> prettyunits 1.0.2 2015-07-13 [1] CRAN (R 3.6.0) #> processx 3.4.0 2019-07-03 [1] CRAN (R 3.6.0) #> ps 1.3.0 2018-12-21 [1] CRAN (R 3.6.0) #> R6 2.4.0 2019-02-14 [1] CRAN (R 3.6.0) #> Rcpp 1.0.1 2019-03-17 [1] CRAN (R 3.6.0) #> remotes 2.1.0 2019-06-24 [1] CRAN (R 3.6.0) #> rex 1.1.2 2017-10-19 [1] CRAN (R 3.6.0) #> rlang 0.4.0 2019-06-25 [1] CRAN (R 3.6.0) #> rmarkdown 1.14 2019-07-12 [1] CRAN (R 3.6.0) #> rprojroot 1.3-2 2018-01-03 [1] CRAN (R 3.6.0) #> sessioninfo 1.1.1 2018-11-05 [1] CRAN (R 3.6.0) #> stringi 1.4.3 2019-03-12 [1] CRAN (R 3.6.0) #> stringr 1.4.0 2019-02-10 [1] CRAN (R 3.6.0) #> testthat 2.1.1 2019-04-23 [1] CRAN (R 3.6.0) #> usethis 1.5.1 2019-07-04 [1] CRAN (R 3.6.0) #> withr 2.1.2 2018-03-15 [1] CRAN (R 3.6.0) #> xfun 0.8 2019-06-25 [1] CRAN (R 3.6.0) #> yaml 2.2.0 2018-07-25 [1] CRAN (R 3.6.0) #> #> [1] /Library/Frameworks/R.framework/Versions/3.6/Resources/library ```

[lionel-]

@jimhester Is it possible for covr to avoid rewrapping expressions in curly blocks, if already a block?

[jimhester]

Not easily, that code in covr is already pretty complicated and I would prefer not to introduce additional complexity to work around this issue.

Could rlang instead be less strict about throwing an error when the code inside {{}} contains calls or multiple expressions?

[lionel-]

Of course I don't know the code and I am probably missing important things, but ensuring expressions are blocks without rewrapping them seems like a very simple metaprogramming operation.

I think we have to throw an informative error when {{ contains complex expressions. Also we might add {{{ in the future: http://rpubs.com/lionel-/superstache

[lionel-]

@jimhester Does a fix along these lines seem to make sense?

---
 R/trace_calls.R                   | 24 +++++++++++++++++++++---
 tests/testthat/test-trace_calls.R | 19 +++++++------------
 2 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/R/trace_calls.R b/R/trace_calls.R
index 41de12a..1bc0563 100644
--- a/R/trace_calls.R
+++ b/R/trace_calls.R
@@ -13,9 +13,9 @@ trace_calls <- function (x, parent_functions = NULL, parent_ref = NULL) {

   # Construct the calls by hand to avoid a NOTE from R CMD check
   count <- function(key, val) {
-    call("{",
-      as.call(list(call(":::", as.symbol("covr"), as.symbol("count")), key)),
-      val)
+    count_call <- as.call(list(call(":::", as.symbol("covr"), as.symbol("count")), key))
+    brace_call <- call("{", count_call, val)
+    structure(brace_call, codecov_brace = TRUE)
   }

   if (is.null(parent_functions)) {
@@ -39,6 +39,20 @@ trace_calls <- function (x, parent_functions = NULL, parent_ref = NULL) {
     }
   }
   else if (is.call(x)) {
+    # Splice `{` blocks created for counting hits
+    if (identical(x[[1]], as.name("{"))) {
+      args <- Map(trace_calls, x[-1], attr(x, "srcref")[-1], MoreArgs = list(parent_functions = parent_functions))
+      args <- lapply(args, function(x) {
+        if (is_codecov_brace(x)) {
+          as.list(x)[-1]
+        } else {
+          list(x)
+        }
+      })
+      args <- do.call(c, args)
+      return(as.call(c(list(as.name("{")), args)))
+    }
+
     if ((identical(x[[1]], as.name("<-")) || identical(x[[1]], as.name("="))) && # nolint
         (is.call(x[[3]]) && identical(x[[3]][[1]], as.name("function")))) {
       parent_functions <- c(parent_functions, as.character(x[[2]]))
@@ -93,6 +107,10 @@ trace_calls <- function (x, parent_functions = NULL, parent_ref = NULL) {
   }
 }

+is_codecov_brace <- function(x) {
+  !is.null(attr(x, "codecov_brace"))
+}
+
 .counters <- new.env(parent = emptyenv())

 #' initialize a new counter
diff --git a/tests/testthat/test-trace_calls.R b/tests/testthat/test-trace_calls.R
index de1df2b..1da1909 100644
--- a/tests/testthat/test-trace_calls.R
+++ b/tests/testthat/test-trace_calls.R
@@ -37,10 +37,10 @@ test_that("one-line functions with braces are traced correctly", {
     x + 1
   }

-  expect_equal(as.character(body(trace_calls(fun))[[2]][[2]][[1]]),
+  expect_equal(as.character(body(trace_calls(fun))[[2]][[1]]),
       c(":::", "covr", "count"))

-  expect_equal(body(trace_calls(fun))[[2]][[3]], body(fun)[[2]])
+  expect_equal(body(trace_calls(fun))[[3]], body(fun)[[2]])
 })

 test_that("one-line functions with no calls and braces are traced correctly", {
@@ -53,18 +53,14 @@ test_that("one-line functions with no calls and braces are traced correctly", {
   }

   e2 <- body(trace_calls(fun))[[2]]
-  expect_true(length(e2) > 1 &&
-              identical(as.character(e2[[2]][[1]]), c(":::", "covr", "count")))
+  expect_true(identical(as.character(e2[[1]]), c(":::", "covr", "count")))

   fun <- function(x) {
     x
   }

   e2 <- body(trace_calls(fun))[[2]]
-
-  # the second expr should be a block
-  expect_true(length(e2) > 1 &&
-    identical(as.character(e2[[2]][[1]]), c(":::", "covr", "count")))
+  expect_true(identical(as.character(e2[[1]]), c(":::", "covr", "count")))
 })

@@ -80,12 +76,11 @@ test_that("last evaled expression is traced", {

   body <- body(trace_calls(fun))

-  expect_equal(length(body), 3)
+  expect_equal(length(body), 5)

   # last expression: the implicit return expression
-  e3 <- body[[3]]
-  expect_true(length(e3) > 1 &&
-    identical(as.character(e3[[2]][[1]]), c(":::", "covr", "count")))
+  e <- body[[length(body) - 1]]
+  expect_true(identical(as.character(e[[1]]), c(":::", "covr", "count")))

 })

-- 

[lionel-]

oops of course not rewrapping doesn't help in this case (it will help in other cases) since that still causes an interpretation error in rlang as {{ contains multiple expressions.

One workaround is to only look at the last expression in { and ignore the rest. This way we allow { instrumentation and we still have an informative error when the user supplies a call or literal instead of a symbol e.g. {{ foo(var) }}.

[jimhester]

This seems to be a sufficient change to rlang to fix the issue

diff --git a/src/internal/expr-interp.c b/src/internal/expr-interp.c
index 2c53e112..7dbfae36 100644
--- a/src/internal/expr-interp.c
+++ b/src/internal/expr-interp.c
@@ -74,7 +74,7 @@ struct expansion_info which_bang_op(sexp* second, struct expansion_info info) {
   return info;
 }
 struct expansion_info which_curly_op(sexp* second, struct expansion_info info) {
-  if (!r_is_call(second, "{")) {
+  if (!r_is_call(second, "{") || r_length(second) > 2) {
     return info;
   }

diff --git a/tests/testthat/test-quasiquotation.R b/tests/testthat/test-quasiquotation.R
index eb069563..36f5ac2e 100644
--- a/tests/testthat/test-quasiquotation.R
+++ b/tests/testthat/test-quasiquotation.R
@@ -582,6 +582,7 @@ test_that("{{ is a quote-unquote operator", {
   expect_identical_(fn(bar), expr(list(!!quo(bar))))
   expect_identical_(expr(list({{ letters }})), expr(list(!!quo(!!letters))))
   expect_error_(expr(list({{ quote(foo) }})), "must be a symbol")
+  expect_equal(expr(list({{ foo ; bar }})), quote(list({{foo ; bar}})))
 })

 test_that("{{ only works in quoting functions", {

[lionel-]

It makes the error go away but it still causes covr code to be interpreted in a completely different way than non-covr code. I'm pretty sure it will cause errors in other cases.

[lionel-]

If tidyeval code is to be consistently covered by covr, I think we need:

• covr not rewrapping {. I think this is a good general principle for metaprogramming / instrumentation, avoiding modification of AST as much as possible.

• covr descending nested {. This means that for code like:

  1. {
  2.   {
  3.     foo
  4.   }
  5. }

  covr needs to insert at line 3 counters for all lines 1, 2, 3, 4, 5. This rule seems a bit ad hoc, though it certainly contributes to minimal modification of AST.

• rlang only interpreting the last expression in nested {{, in order to allow { instrumentation.

This doesn't feel entirely right (there's a mix of principled and ad hoc behaviour here), but this would allow coverage of tidyeval functions without errors.

[lionel-]

Alternatively, covr could maintain a list of user-supplied predicates that prevent instrumentation. Then packages would just add something like covr::prevent_if(rlang::covr_guard) at the top of their testthat.R.

[njtierney]

Thank you both so much for looking into this! :)

I was wondering what the best practice way forward for me is here - should I use the dev version of covr in Suggests, or should I wait for the next version of covr to be on CRAN?

[jimhester]

I would use the dev version of covr until it is on CRAN.

[njtierney]

OK great, thanks! :)

[IndrajeetPatil]

@jimhester Do you think this also solves the following issue with lintr: https://github.com/jimhester/lintr/issues/388

So far I've been using a workaround suggested in that thread, but maybe I'll no longer have to use it?