Don't lint expression(paste(., sep = "")) in paste_linter

NEWS.md

@@ -1,6 +1,10 @@
 # lintr (development version)
 
-Nothing yet :)
+## New and improved features
+
+### Lint accuracy fixes: removing false positives
+
+* `paste_linter()` lints `expression(paste(., sep = ""))` because the `paste` inside an expression doesn't support the `sep` argument (#2945, @mcol).
 
 # lintr (3.3.0-1)
 
@@ -74,10 +78,6 @@ Nothing yet :)
 * `object_name_linter()` and `object_length_linter()` account for S3 class correctly when the generic is assigned with `=` (#2507, @MichaelChirico).
 * `assignment_linter()` with `operator = "="` does a better job of skipping implicit assignments, which are intended to be governed by `implicit_assignment_linter()` (#2765, @MichaelChirico).
 * `implicit_assignment_linter()` with `allow_scoped=TRUE` doesn't lint for `if (a <- 1) print(a)` (#2913, @MichaelChirico).
-* `expect_true_false_linter()` is pipe-aware, so that `42 |> expect_identical(x, ignore_attr = TRUE)` no longer lints (#1520, @MichaelChirico).
-* `T_and_F_symbol_linter()` ignores `T` and `F`:
-   + When used as symbols in formulas (`y ~ T + F`), which can represent variables in data not controlled by the author (#2637, @MichaelChirico).
-   + If followed by `[` or `[[` (#2944, @mcol).
 
 ### Lint accuracy fixes: removing false negatives
 

R/paste_linter.R

@@ -5,7 +5,9 @@
 #' The following issues are linted by default by this linter
 #'   (see arguments for which can be de-activated optionally):
 #'
-#'  1. Block usage of [base::paste()] with `sep = ""`. [base::paste0()] is a faster, more concise alternative.
+#'  1. Block usage of [base::paste()] with `sep = ""`. [base::paste0()] is a faster, more concise alternative;
+#'     this is valid unless `paste` occurs inside [base::expression], which according to [grDevices::plotmath]
+#'     does not support the `sep` argument.
 #'  2. Block usage of `paste()` or `paste0()` with `collapse = ", "`. [toString()] is a direct
 #'     wrapper for this, and alternatives like [glue::glue_collapse()] might give better messages for humans.
 #'  3. Block usage of `paste0()` that supplies `sep=` -- this is not a formal argument to `paste0`, and
@@ -75,6 +77,11 @@
 #' )
 #'
 #' lint(
+#'   text = 'expression(paste("a", "b", sep = ""))',
+#'   linters = paste_linter()
+#' )
+#'
+#' lint(
 #'   text = 'toString(c("a", "b"))',
 #'   linters = paste_linter()
 #' )
@@ -121,7 +128,14 @@ paste_linter <- function(allow_empty_sep = FALSE,
   following-sibling::SYMBOL_SUB[text() = 'sep' and following-sibling::expr[1][STR_CONST]]
     /parent::expr
   "
-
+  expression_paste_sep_xpath <- "
+  following-sibling::SYMBOL_SUB[
+    text() = 'sep'
+    and following-sibling::expr[1][STR_CONST]
+    and parent::expr/preceding-sibling::expr/SYMBOL_FUNCTION_CALL[text() = 'expression']
+  ]
+    /parent::expr
+  "
   to_string_xpath <- "
   parent::expr[
     count(expr) = 3
@@ -184,7 +198,16 @@ paste_linter <- function(allow_empty_sep = FALSE,
       paste_sep_value <- get_r_string(paste_sep_expr, xpath = "./SYMBOL_SUB[text() = 'sep']/following-sibling::expr[1]")
     }
 
-    if (!allow_empty_sep) {
+    ## check if we are inside an expression()
+    expression_paste_sep_expr <- xml_find_all(paste_calls, expression_paste_sep_xpath)
+    if (length(expression_paste_sep_expr) > 0L) {
+      optional_lints <- c(optional_lints, xml_nodes_to_lints(
+        expression_paste_sep_expr[1L],
+        source_expression = source_expression,
+        lint_message = "inside expression(...), paste does not accept a 'sep' argument.",
+        type = "warning"
+      ))
+    } else if (!allow_empty_sep) {
       optional_lints <- c(optional_lints, xml_nodes_to_lints(
         paste_sep_expr[!nzchar(paste_sep_value)],
         source_expression = source_expression,

tests/testthat/test-paste_linter.R

@@ -8,6 +8,8 @@ test_that("paste_linter skips allowed usages for sep=''", {
   expect_no_lint("sep <- ''; paste('a', sep)", linter)
   expect_no_lint("paste(sep = ',', '', 'a')", linter)
   expect_no_lint("paste0('a', 'b', 'c')", linter)
+  expect_no_lint("expression(2); paste('a', 'b', 'c', sep = ',')", linter)
+  expect_no_lint("paste('a', 'b', expression(2), sep = ',')", linter)
 })
 
 test_that("paste_linter blocks simple disallowed usages for sep=''", {
@@ -22,6 +24,23 @@ test_that("paste_linter blocks simple disallowed usages for sep=''", {
     rex::rex('paste0(...) is better than paste(..., sep = "").'),
     paste_linter()
   )
+
+  expect_lint(
+    "paste('a', 'b', expression(2), sep = '')",
+    rex::rex('paste0(...) is better than paste(..., sep = "").'),
+    paste_linter()
+  )
+
+  expect_lint(
+    "c(expression(paste('a', 'b', sep = ',')))",
+    rex::rex("inside expression(...), paste does not accept a 'sep' argument."),
+    paste_linter()
+  )
+  expect_lint(
+    "c(expression(paste('a', 'b', sep = '')))",
+    rex::rex("inside expression(...), paste does not accept a 'sep' argument."),
+    paste_linter()
+  )
 })
 
 test_that("paste_linter skips allowed usages for collapse=', '", {