Merge branch 'main' into check-random-order

.github/workflows/pkgdown.yaml

@@ -39,7 +39,7 @@ jobs:
 
       - name: Deploy to GitHub pages 🚀
         if: github.event_name != 'pull_request'
-        uses: JamesIves/github-pages-deploy-action@v4.5.0
+        uses: JamesIves/github-pages-deploy-action@v4.6.0
         with:
           clean: false
           branch: gh-pages

DESCRIPTION

@@ -18,7 +18,7 @@ Description: Checks adherence to a given style, syntax errors and possible
     'RStudio IDE', 'Emacs', 'Vim', 'Sublime Text', 'Atom' and 'Visual
     Studio Code'.
 License: MIT + file LICENSE
-URL: https://github.com/r-lib/lintr, https://lintr.r-lib.org
+URL: https://lintr.r-lib.org, https://github.com/r-lib/lintr
 BugReports: https://github.com/r-lib/lintr/issues
 Depends:
     R (>= 3.6)

NEWS.md

@@ -22,6 +22,7 @@
 * `.lintr` config validation correctly accepts regular expressions which only compile under `perl = TRUE` (#2375, @MichaelChirico). These have always been valid (since `rex::re_matches()`, which powers the lint exclusion logic, also uses this setting), but the new up-front validation in v3.1.1 incorrectly used `perl = FALSE`.
 * `.lintr` configs set by option `lintr.linter_file` or environment variable `R_LINTR_LINTER_FILE` can point to subdirectories (#2512, @MichaelChirico).
 * `indentation_linter()` returns `ranges[1L]==1L` when the offending line has 0 spaces (#2550, @MichaelChirico).
+* `literal_coercion_linter()` doesn't surface a warning about NAs during coercion for code like `as.integer("a")` (#2566, @MichaelChirico).
 
 ## Changes to default linters
 
@@ -51,6 +52,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/literal_coercion_linter.R

@@ -97,9 +97,14 @@ literal_coercion_linter <- function() {
         needs_prefix <- is_rlang_coercer & !startsWith(coercion_str, "rlang::")
         coercion_str[needs_prefix] <- paste0("rlang::", coercion_str[needs_prefix])
       }
-      # the linter logic & rlang requirement should ensure that it's safe to run eval() here
+      # the linter logic & rlang requirement should ensure that it's safe to run eval() here;
+      #   suppressWarnings() is for cases like 'as.integer("a")' which have an NA result, #2566.
       # TODO(#2473): Avoid a recommendation like '1' that clashes with implicit_integer_linter().
-      literal_equivalent_str <- vapply(str2expression(coercion_str), function(expr) deparse1(eval(expr)), character(1L))
+      literal_equivalent_str <- vapply(
+        str2expression(coercion_str),
+        function(expr) deparse1(suppressWarnings(eval(expr))),
+        character(1L)
+      )
       lint_message <- sprintf(
         "Use %s instead of %s, i.e., use literals directly where possible, instead of coercion.",
         literal_equivalent_str, report_str

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) {

tests/testthat/test-lint.R

@@ -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-literal_coercion_linter.R

@@ -1,5 +1,5 @@
 test_that("literal_coercion_linter skips allowed usages", {
-  linter <- line_length_linter()
+  linter <- literal_coercion_linter()
 
   # naive xpath includes the "_f0" here as a literal
   expect_lint('as.numeric(x$"_f0")', NULL, linter)
@@ -23,7 +23,7 @@ test_that("literal_coercion_linter skips allowed usages", {
 })
 
 test_that("literal_coercion_linter skips allowed rlang usages", {
-  linter <- line_length_linter()
+  linter <- literal_coercion_linter()
 
   expect_lint("int(1, 2.0, 3)", NULL, linter)
   expect_lint("chr('e', 'ab', 'xyz')", NULL, linter)
@@ -40,6 +40,18 @@ test_that("literal_coercion_linter skips quoted keyword arguments", {
   expect_lint("as.numeric(foo('a' = 1))", NULL, literal_coercion_linter())
 })
 
+test_that("no warnings surfaced by running coercion", {
+  linter <- literal_coercion_linter()
+
+  expect_no_warning(
+    expect_lint("as.integer('a')", "Use NA_integer_", linter)
+  )
+
+  expect_no_warning(
+    expect_lint("as.integer(2147483648)", "Use NA_integer_", linter)
+  )
+})
+
 skip_if_not_installed("tibble")
 patrick::with_parameters_test_that(
   "literal_coercion_linter blocks simple disallowed usages",

tests/testthat/test-paste_linter.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)
+})