Merge paste_sep_linter and paste_to_string_linter into paste_linter

DESCRIPTION

@@ -112,8 +112,7 @@ Collate:
     'package_hooks_linter.R'
     'paren_body_linter.R'
     'paren_brace_linter.R'
-    'paste_sep_linter.R'
-    'paste_to_string_linter.R'
+    'paste_linter.R'
     'path_linters.R'
     'pipe_call_linter.R'
     'pipe_continuation_linter.R'

NAMESPACE

@@ -77,8 +77,7 @@ export(outer_negation_linter)
 export(package_hooks_linter)
 export(paren_body_linter)
 export(paren_brace_linter)
-export(paste_sep_linter)
-export(paste_to_string_linter)
+export(paste_linter)
 export(pipe_call_linter)
 export(pipe_continuation_linter)
 export(redundant_ifelse_linter)

NEWS.md

@@ -114,8 +114,9 @@ function calls. (#850, #851, @renkun-ken)
    * `outer_negation_linter()` Require usage of `!any(x)` over `all(!x)` and `!all(x)` over `any(!x)`
    * `numeric_leading_zero_linter()` Require a leading `0` in fractional numeric constants, e.g. `0.1` instead of `.1`
    * `literal_coercion_linter()` Require using correctly-typed literals instead of direct coercion, e.g. `1L` instead of `as.numeric(1)`
-   * `paste_sep_linter()` Require usage of `paste0()` over `paste(sep = "")`
-   * `paste_to_string_linter()` Require usage of `toString(x)` over `paste(x, collapse = ", ")`
+   * `paste_linter()` lint for common mis-use of `paste()` and `paste()`:
+     + `paste0()` encouraged instead of `paste(sep = "")`
+     + `toString()` or `glue::glue_collapse()` encouraged instead of `paste(x, collapse = ", ")`
    * `nested_ifelse_linter()` Prevent nested calls to `ifelse()` like `ifelse(A, x, ifelse(B, y, z))`, and similar
    * `condition_message_linter` Prevent condition messages from being constructed like `stop(paste(...))` (where just `stop(...)` is preferable)
    * `redundant_ifelse_linter()` Prevent usage like `ifelse(A & B, TRUE, FALSE)` or `ifelse(C, 0, 1)` (the latter is `as.numeric(!C)`)

R/paste_linter.R

@@ -0,0 +1,74 @@
+#' Raise lints for several common poor usages of paste()
+#'
+#' The following issues are linted by default by this linter
+#'   (and each can be turned off optionally):
+#'
+#'  1. Block usage of [paste()] with `sep = ""`. [paste0()] is a faster, more concise alternative.
+#'  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.
+#'
+#' @evalRd rd_tags("paste_linter")
+#' @param allow_empty_sep Logical, default `FALSE`. If `TRUE`, usage of
+#'   `paste()` with `sep = ""` is not linted.
+#' @param allow_to_string Logical, default `FALSE`. If `TRUE`, usage of
+#'   `paste()` and `paste0()` with `collapse = ", "` is not linted.
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+paste_linter <- function(allow_empty_sep = FALSE, allow_to_string = FALSE) {
+  Linter(function(source_file) {
+    if (length(source_file$xml_parsed_content) == 0L) {
+      return(list())
+    }
+
+    xml <- source_file$xml_parsed_content
+
+    if (allow_empty_sep) {
+      empty_sep_lints <- NULL
+    } else {
+      # NB: string-length()=2 means '' or "" (*not* 'ab' or 'cd' which have length 4)
+      # TODO: adapt this for R>4.0 raw strings
+      empty_sep_xpath <- "//expr[
+        expr[SYMBOL_FUNCTION_CALL[text() = 'paste']]
+        and SYMBOL_SUB[text() = 'sep']/following-sibling::expr[1][STR_CONST[string-length(text()) = 2]]
+      ]"
+
+      empty_sep_expr <- xml2::xml_find_all(xml, empty_sep_xpath)
+
+      empty_sep_lints <- lapply(
+        empty_sep_expr,
+        xml_nodes_to_lint,
+        source_file = source_file,
+        lint_message = 'paste0(...) is better than paste(..., sep = "").',
+        type = "warning"
+      )
+    }
+
+    if (allow_to_string) {
+      to_string_lints <- NULL
+    } else {
+      # 3 expr: the function call, the argument, and collapse=
+      # TODO(michaelchirico): catch raw-string equivalents
+      seps <- c("', '", '", "')
+      to_string_xpath <- glue::glue("//expr[
+      expr[SYMBOL_FUNCTION_CALL[text() = 'paste' or text() = 'paste0']]
+        and count(expr) = 3
+        and SYMBOL_SUB[text() = 'collapse']/following-sibling::expr[1][STR_CONST[ {xp_text_in_table(seps)} ]]
+      ]")
+      to_string_expr <- xml2::xml_find_all(xml, to_string_xpath)
+
+      to_string_lints <- lapply(
+        to_string_expr,
+        xml_nodes_to_lint,
+        source_file = source_file,
+        lint_message = paste(
+          'toString(.) is more expressive than paste(., collapse = ", ").',
+          "Note also glue::glue_collapse() and and::and()",
+          "for constructing human-readable / translation-friendly lists"
+        ),
+        type = "warning"
+      )
+    }
+
+    c(empty_sep_lints, to_string_lints)
+  })
+}

inst/lintr/linters.csv

@@ -50,8 +50,7 @@ outer_negation_linter,readability efficiency best_practices
 package_hooks_linter,style correctness package_development
 paren_body_linter,style readability default
 paren_brace_linter,style readability default
-paste_sep_linter,best_practices consistency
-paste_to_string_linter,best_practices consistency
+paste_linter,best_practices consistency
 pipe_call_linter,style readability
 pipe_continuation_linter,style readability default
 redundant_ifelse_linter,best_practices efficiency consistency

tests/testthat/test-paste_linter.R

@@ -0,0 +1,66 @@
+test_that("paste_linter skips allowed usages for sep=''", {
+  expect_lint("paste('a', 'b', 'c')", NULL, paste_linter())
+  expect_lint("paste('a', 'b', 'c', sep = ',')", NULL, paste_linter())
+  expect_lint("paste('a', 'b', collapse = '')", NULL, paste_linter())
+  expect_lint("cat(paste('a', 'b'), sep = '')", NULL, paste_linter())
+  expect_lint("sep <- ''; paste('a', sep)", NULL, paste_linter())
+  expect_lint("paste(sep = ',', '', 'a')", NULL, paste_linter())
+
+  expect_lint("paste0('a', 'b', 'c')", NULL, paste_linter())
+})
+
+test_that("paste_linter blocks simple disallowed usages for sep=''", {
+  expect_lint(
+    "paste(sep = '', 'a', 'b')",
+    rex::rex('paste0(...) is better than paste(..., sep = "").'),
+    paste_linter()
+  )
+
+  expect_lint(
+    "paste('a', 'b', sep = '')",
+    rex::rex('paste0(...) is better than paste(..., sep = "").'),
+    paste_linter()
+  )
+})
+
+test_that("paste_linter skips allowed usages for collapse=', '", {
+  expect_lint("paste('a', 'b', 'c')", NULL, paste_linter())
+  expect_lint("paste(x, sep = ', ')", NULL, paste_linter())
+  expect_lint("paste(x, collapse = ',')", NULL, paste_linter())
+  expect_lint("paste(foo(x), collapse = '/')", NULL, paste_linter())
+  # harder to catch statically
+  expect_lint("collapse <- ', '; paste(x, collapse = collapse)", NULL, paste_linter())
+
+  # paste(..., sep=sep, collapse=", ") is not a trivial swap to toString
+  expect_lint("paste(x, y, sep = '.', collapse = ', ')", NULL, paste_linter())
+  # any call involving ...length() > 1 will implicitly use the default sep
+  expect_lint("paste(x, y, collapse = ', ')", NULL, paste_linter())
+  expect_lint("paste0(x, y, collapse = ', ')", NULL, paste_linter())
+
+  expect_lint("toString(x)", NULL, paste_linter())
+
+  # string match of ", " is OK -- lint only _exact_ match
+  expect_lint('paste(x, collapse = ", \n")', NULL, paste_linter())
+})
+
+test_that("paste_linter blocks simple disallowed usages for collapse=', '", {
+  expect_lint(
+    "paste(collapse = ', ', x)",
+    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))
+})