extend paste_linter() for strrep() equivalents (#1652)

* extend paste_linter() for strrep() equivalents

* new tests

* Update R/paste_linter.R

Co-authored-by: Indrajeet Patil <[email protected]>

* Update paste_linter.Rd

Co-authored-by: Indrajeet Patil <[email protected]>

Author: MichaelChirico

NEWS.md

@@ -36,6 +36,9 @@
 
 * `pipe_continuation_linter()` recognizes violations involving the native R pipe `|>` (#1609, @MichaelChirico)
 
+* `paste_linter()` also catches usages like `paste(rep("*", 10L), collapse = "")` that can be written more
+  concisely as `strrep("*", 10L)` (#1108, @MichaelChirico)
+
 ### New linters
 
 * `unnecessary_lambda_linter()`: detect unnecessary lambdas (anonymous functions), e.g.

R/paste_linter.R

@@ -1,13 +1,20 @@
 #' 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):
+#'   (see arguments for which can be de-activated 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.
 #'  3. Block usage of `paste0()` that supplies `sep=` -- this is not a formal argument to `paste0`, and
 #'     is likely to be a mistake.
+#'  4. Block usage of `paste()` / `paste0()` combined with [rep()] that could be replaced by
+#'     [strrep()]. `strrep()` can handle the task of building a block of repeated strings
+#'     (e.g. often used to build "horizontal lines" for messages). This is both more readable and
+#'     skips the (likely small) overhead of putting two strings into the global string cache when only one is needed.
+#'
+#'     Only target scalar usages -- `strrep` can handle more complicated cases (e.g. `strrep(letters, 26:1)`,
+#'     but those aren't as easily translated from a `paste(collapse=)` call.
 #'
 #' @evalRd rd_tags("paste_linter")
 #' @param allow_empty_sep Logical, default `FALSE`. If `TRUE`, usage of
@@ -40,19 +47,29 @@ paste_linter <- function(allow_empty_sep = FALSE, allow_to_string = FALSE) {
     /parent::expr
   "
 
+  paste_strrep_xpath <- "
+  //SYMBOL_FUNCTION_CALL[text() = 'paste' or text() = 'paste0']
+    /parent::expr[
+      count(following-sibling::expr) = 2
+      and following-sibling::expr[1][expr[1][SYMBOL_FUNCTION_CALL[text() = 'rep']] and expr[2][STR_CONST]]
+      and following-sibling::SYMBOL_SUB[text() = 'collapse']
+    ]
+    /parent::expr
+  "
+
   Linter(function(source_expression) {
     if (!is_lint_level(source_expression, "expression")) {
       return(list())
     }
 
     xml <- source_expression$xml_parsed_content
-    lints <- list()
+    optional_lints <- list()
 
     if (!allow_empty_sep) {
       empty_sep_expr <- xml2::xml_find_all(xml, sep_xpath)
       sep_value <- get_r_string(empty_sep_expr, xpath = "./SYMBOL_SUB[text() = 'sep']/following-sibling::expr[1]")
 
-      lints <- c(lints, xml_nodes_to_lints(
+      optional_lints <- c(optional_lints, xml_nodes_to_lints(
         empty_sep_expr[!nzchar(sep_value)],
         source_expression = source_expression,
         lint_message = 'paste0(...) is better than paste(..., sep = "").',
@@ -68,7 +85,7 @@ paste_linter <- function(allow_empty_sep = FALSE, allow_to_string = FALSE) {
         xpath = "./SYMBOL_SUB[text() = 'collapse']/following-sibling::expr[1]"
       )
 
-      lints <- c(lints, xml_nodes_to_lints(
+      optional_lints <- c(optional_lints, xml_nodes_to_lints(
         to_string_expr[collapse_value == ", "],
         source_expression = source_expression,
         lint_message = paste(
@@ -81,13 +98,24 @@ paste_linter <- function(allow_empty_sep = FALSE, allow_to_string = FALSE) {
     }
 
     paste0_sep_expr <- xml2::xml_find_all(xml, paste0_sep_xpath)
-    lints <- c(lints, xml_nodes_to_lints(
+    paste0_sep_lints <- xml_nodes_to_lints(
       paste0_sep_expr,
       source_expression = source_expression,
       lint_message = "sep= is not a formal argument to paste0(); did you mean to use paste(), or collapse=?",
       type = "warning"
-    ))
+    )
+
+    paste_strrep_expr <- xml2::xml_find_all(xml, paste_strrep_xpath)
+    collapse_arg <- get_r_string(paste_strrep_expr, "SYMBOL_SUB/following-sibling::expr[1]/STR_CONST")
+    paste_strrep_expr <- paste_strrep_expr[!nzchar(collapse_arg)]
+    paste_call <- xp_call_name(paste_strrep_expr)
+    paste_strrep_lints <- xml_nodes_to_lints(
+      paste_strrep_expr,
+      source_expression = source_expression,
+      lint_message = sprintf('strrep(x, times) is better than %s(rep(x, times), collapse = "").', paste_call),
+      type = "warning"
+    )
 
-    lints
+    c(optional_lints, paste0_sep_lints, paste_strrep_lints)
   })
 }

man/paste_linter.Rd

@@ -89,3 +89,22 @@ test_that("paste_linter catches use of paste0 with sep=", {
     paste_linter()
   )
 })
+
+test_that("paste_linter skips allowed usages for strrep()", {
+  expect_lint("paste(x, collapse = '')", NULL, paste_linter())
+  expect_lint("paste(rep('*', 10), collapse = '+')", NULL, paste_linter())
+  expect_lint("paste(rep(c('a', 'b'), 2), collapse = '')", NULL, paste_linter())
+  expect_lint("paste0(rep('a', 2), 'b', collapse = '')", NULL, paste_linter())
+  # no collapse
+  expect_lint("paste(rep('*', 10))", NULL, paste_linter())
+  # combined before aggregating
+  expect_lint("paste(rep('*', 10), rep('x', 10), collapse = '')", NULL, paste_linter())
+})
+
+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)
+})