New paste_to_string_linter (#1013)

* New paste_to_string_linter

* improve xpath readability

* remove dQuote/sQuote

* note better alternatives

* missing comma

* update documenation

* roxygenize again

DESCRIPTION

@@ -113,6 +113,7 @@ Collate:
     'paren_body_linter.R'
     'paren_brace_linter.R'
     'paste_sep_linter.R'
+    'paste_to_string_linter.R'
     'path_linters.R'
     'pipe_call_linter.R'
     'pipe_continuation_linter.R'

NAMESPACE

@@ -78,6 +78,7 @@ export(package_hooks_linter)
 export(paren_body_linter)
 export(paren_brace_linter)
 export(paste_sep_linter)
+export(paste_to_string_linter)
 export(pipe_call_linter)
 export(pipe_continuation_linter)
 export(redundant_ifelse_linter)

NEWS.md

@@ -115,6 +115,7 @@ function calls. (#850, #851, @renkun-ken)
    * `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 = ", ")`
    * `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_to_string_linter.R

@@ -0,0 +1,39 @@
+#' Block usage of paste() with collapse=", "
+#'
+#' [toString()] is a more concise and expressive wrapper for aggregating string
+#'   vectors with `paste(., collapse = ", ")`.
+#'
+#' @evalRd rd_tags("paste_to_string_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+paste_to_string_linter <- function() {
+  Linter(function(source_file) {
+    if (length(source_file$xml_parsed_content) == 0L) {
+      return(list())
+    }
+
+    xml <- source_file$xml_parsed_content
+
+    # 3 expr: the function call, the argument, and collapse=
+    # TODO(michaelchirico): catch raw-string equivalents
+    seps <- c("', '", '", "')
+    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)} ]]
+    ]")
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+
+    return(lapply(
+      bad_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"
+    ))
+  })
+}

R/xp_utils.R

@@ -1,7 +1,15 @@
 # utils for working with xpaths
 
 # like `text() %in% table`, translated to XPath 1.0
-xp_text_in_table <- function(table) paste("text() = ", quote_wrap(table, "'"), collapse = " or ")
+xp_text_in_table <- function(table) {
+  # xpath doesn't seem to have a standard way of escaping quotes, so attempt
+  #   to use "" whenever the string has ' (not a perfect solution). info on
+  #   escaping from https://stackoverflow.com/questions/14822153
+  single_quoted <- grepl("'", table, fixed = TRUE)
+  table[single_quoted] <- paste0('"', table[single_quoted], '"')
+  table[!single_quoted] <- paste0("'", table[!single_quoted], "'")
+  return(paste0("text() = ", table, collapse = " or "))
+}
 
 # convert an XML match into a Lint
 xml_nodes_to_lint <- function(xml, source_file, lint_message,

inst/lintr/linters.csv

@@ -51,6 +51,7 @@ 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
 pipe_call_linter,style readability
 pipe_continuation_linter,style readability default
 redundant_ifelse_linter,best_practices efficiency consistency

tests/testthat/test-paste_to_string_linter.R

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