Lint for paste0(collapse='') instead of paste(collapse='') (#2559)

* Lint for paste0(collapse='') instead of paste(collapse='')

* Fix in {lintr}

* Also skip on '...', thanks to base R

* roxygenize

* trailing whitespace

Author: MichaelChirico

NEWS.md

@@ -51,6 +51,7 @@
 * `vector_logic_linter()` is extended to recognize incorrect usage of scalar operators `&&` and `||` inside subsetting expressions like `dplyr::filter(x, A && B)` (#2166, @MichaelChirico).
 * `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico).
 * `make_linter_from_xpath()` errors up front when `lint_message` is missing (instead of delaying this error until the linter is used, #2541, @MichaelChirico).
+* `paste_linter()`  is extended to recommend using `paste()` instead of `paste0()` for simply aggregating a character vector with `collapse=`, i.e., when `sep=` is irrelevant (#1108, @MichaelChirico).
 
 ### New linters
 

R/deprecated.R

@@ -15,6 +15,6 @@ lintr_deprecated <- function(what, alternative = NULL, version = NULL,
     ". ",
     if (length(alternative) > 0L) c("Use ", alternative, " instead.")
   )
-  msg <- paste0(msg, collapse = "")
+  msg <- paste(msg, collapse = "")
   signal(msg, call. = FALSE, domain = NA)
 }

R/expect_lint.R

@@ -61,7 +61,7 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") {
 
   lints <- lint(file, ...)
   n_lints <- length(lints)
-  lint_str <- if (n_lints) paste0(c("", lints), collapse = "\n") else ""
+  lint_str <- if (n_lints) paste(c("", lints), collapse = "\n") else ""
 
   wrong_number_fmt <- "got %d lints instead of %d%s"
   if (is.null(checks)) {

R/lint.R

@@ -666,7 +666,7 @@ highlight_string <- function(message, column_number = NULL, ranges = NULL) {
 }
 
 fill_with <- function(character = " ", length = 1L) {
-  paste0(collapse = "", rep.int(character, length))
+  paste(collapse = "", rep.int(character, length))
 }
 
 has_positional_logical <- function(dots) {

R/paste_linter.R

@@ -58,6 +58,11 @@
 #'   linters = paste_linter(allow_file_path = "never")
 #' )
 #'
+#' lint(
+#'   text = 'paste0(x, collapse = "")',
+#'   linters = paste_linter()
+#' )
+#'
 #' # okay
 #' lint(
 #'   text = 'paste0("a", "b")',
@@ -99,6 +104,11 @@
 #'   linters = paste_linter()
 #' )
 #'
+#' lint(
+#'   text = 'paste(x, collapse = "")',
+#'   linters = paste_linter()
+#' )
+#'
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
 paste_linter <- function(allow_empty_sep = FALSE,
@@ -157,6 +167,15 @@ paste_linter <- function(allow_empty_sep = FALSE,
   empty_paste_note <-
     'Note that paste() converts empty inputs to "", whereas file.path() leaves it empty.'
 
+  paste0_collapse_xpath <- "
+  parent::expr
+    /parent::expr[
+      SYMBOL_SUB[text() = 'collapse']
+      and count(expr) = 3
+      and not(expr/SYMBOL[text() = '...'])
+    ]
+  "
+
   Linter(linter_level = "expression", function(source_expression) {
     paste_calls <- source_expression$xml_find_function_calls("paste")
     paste0_calls <- source_expression$xml_find_function_calls("paste0")
@@ -219,6 +238,14 @@ paste_linter <- function(allow_empty_sep = FALSE,
       type = "warning"
     )
 
+    paste0_collapse_expr <- xml_find_all(paste0_calls, paste0_collapse_xpath)
+    paste0_collapse_lints <- xml_nodes_to_lints(
+      paste0_collapse_expr,
+      source_expression = source_expression,
+      lint_message = "Use paste(), not paste0(), to collapse a character vector when sep= is not used.",
+      type = "warning"
+    )
+
     if (check_file_paths) {
       paste_sep_slash_expr <- paste_sep_expr[paste_sep_value == "/"]
       optional_lints <- c(optional_lints, xml_nodes_to_lints(
@@ -248,7 +275,7 @@ paste_linter <- function(allow_empty_sep = FALSE,
       ))
     }
 
-    c(optional_lints, paste0_sep_lints, paste_strrep_lints)
+    c(optional_lints, paste0_sep_lints, paste_strrep_lints, paste0_collapse_lints)
   })
 }
 

R/utils.R

@@ -111,7 +111,7 @@ get_content <- function(lines, info) {
     lines[length(lines)] <- substr(lines[length(lines)], 1L, info$col2)
     lines[1L] <- substr(lines[1L], info$col1, nchar(lines[1L]))
   }
-  paste0(collapse = "\n", lines)
+  paste(lines, collapse = "\n")
 }
 
 logical_env <- function(x) {

man/paste_linter.Rd

@@ -117,7 +117,7 @@ test_that("lint() results from file or text should be consistent", {
   linters <- list(assignment_linter(), infix_spaces_linter())
   lines <- c("x<-1", "x+1")
   file <- withr::local_tempfile(lines = lines)
-  text <- paste0(lines, collapse = "\n")
+  text <- paste(lines, collapse = "\n")
   file <- normalizePath(file)
 
   lint_from_file <- lint(file, linters = linters)

tests/testthat/test-lint.R

@@ -52,20 +52,13 @@ test_that("paste_linter blocks simple disallowed usages for collapse=', '", {
     rex::rex('toString(.) is more expressive than paste(., collapse = ", ")'),
     paste_linter()
   )
-
-  expect_lint(
-    "paste0(foo(x), collapse = ', ')",
-    rex::rex('toString(.) is more expressive than paste(., collapse = ", ")'),
-    paste_linter()
-  )
 })
 
 test_that("paste_linter respects non-default arguments", {
   expect_lint("paste(sep = '', 'a', 'b')", NULL, paste_linter(allow_empty_sep = TRUE))
   expect_lint("paste('a', 'b', sep = '')", NULL, paste_linter(allow_empty_sep = TRUE))
 
   expect_lint("paste(collapse = ', ', x)", NULL, paste_linter(allow_to_string = TRUE))
-  expect_lint("paste0(foo(x), collapse = ', ')", NULL, paste_linter(allow_to_string = TRUE))
 })
 
 test_that("paste_linter works for raw strings", {
@@ -107,11 +100,11 @@ test_that("paste_linter skips allowed usages for strrep()", {
 })
 
 test_that("paste_linter blocks simple disallowed usages", {
-  linter <- paste_linter()
-  lint_msg <- rex::rex("strrep(x, times) is better than paste")
-
-  expect_lint("paste0(rep('*', 20L), collapse='')", lint_msg, linter)
-  expect_lint("paste(rep('#', width), collapse='')", lint_msg, linter)
+  expect_lint(
+    "paste(rep('#', width), collapse='')",
+    rex::rex("strrep(x, times) is better than paste"),
+    paste_linter()
+  )
 })
 
 test_that("paste_linter skips allowed usages for file paths", {
@@ -156,9 +149,6 @@ test_that("paste_linter ignores non-path cases with paste0", {
   expect_lint("paste0(x)", NULL, linter)
   expect_lint("paste0('a')", NULL, linter)
   expect_lint("paste0('a', 1)", NULL, linter)
-
-  # paste0(..., collapse=collapse) not directly mapped to file.path
-  expect_lint("paste0(x, collapse = '/')", NULL, linter)
 })
 
 test_that("paste_linter detects paths built with '/' and paste0", {
@@ -245,3 +235,39 @@ test_that("raw strings are detected in file path logic", {
   expect_lint("paste(x, y, sep = R'{//}')", NULL, linter)
   expect_lint("paste(x, y, sep = R'{/}')", lint_msg, linter)
 })
+
+test_that("paste0(collapse=...) is caught", {
+  linter <- paste_linter()
+  lint_msg <- rex::rex("Use paste(), not paste0(), to collapse a character vector when sep= is not used.")
+
+  expect_lint("paste(x, collapse = '')", NULL, linter)
+  expect_lint("paste0(a, b, collapse = '')", NULL, linter)
+  # pass-through can pass any number of arguments
+  expect_lint("paste0(..., collapse = '')", NULL, linter)
+  expect_lint("paste0(x, collapse = '')", lint_msg, linter)
+  expect_lint("paste0(x, collapse = 'xxx')", lint_msg, linter)
+  expect_lint("paste0(foo(x, y, z), collapse = '')", lint_msg, linter)
+})
+
+test_that("paste0(collapse=...) cases interacting with other rules are handled", {
+  linter <- paste_linter()
+  lint_msg <- rex::rex("Use paste(), not paste0(), to collapse a character vector when sep= is not used.")
+
+  # multiple lints when collapse= happens to be ", "
+  expect_lint(
+    "paste0(foo(x), collapse = ', ')",
+    list(rex::rex('toString(.) is more expressive than paste(., collapse = ", ")'), lint_msg),
+    linter
+  )
+  expect_lint("paste0(foo(x), collapse = ', ')", lint_msg, paste_linter(allow_to_string = TRUE))
+
+  expect_lint(
+    "paste0(rep('*', 20L), collapse='')",
+    list(rex::rex("strrep(x, times) is better than paste"), lint_msg),
+    linter
+  )
+
+  # paste0(..., collapse=collapse) not directly mapped to file.path
+  expect_lint("paste0(x, collapse = '/')", lint_msg, linter)
+  expect_lint("paste0(x, y, collapse = '/')", NULL, linter)
+})