From f87bb69e3876a8d034adcd69683ce2220ae7f561 Mon Sep 17 00:00:00 2001 From: Marco Colombo Date: Tue, 21 Oct 2025 13:50:13 +0200 Subject: [PATCH 1/6] Don't lint expression(paste(., sep = "")) in paste_linter. --- NEWS.md | 1 + R/paste_linter.R | 8 +++++++- man/paste_linter.Rd | 5 +++++ tests/testthat/test-paste_linter.R | 7 +++++++ 4 files changed, 20 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index bb8417f87..1d0f8209d 100644 --- a/NEWS.md +++ b/NEWS.md @@ -76,6 +76,7 @@ files in Windows (#2882, @Bisaloo). * `T_and_F_symbol_linter()` ignores `T` and `F` used as symbols in formulas (`y ~ T + F`), which can represent variables in data not controlled by the author (#2637, @MichaelChirico). * `T_and_F_symbol_linter()` ignores `T` and `F` if followed by `[` or `[[` (#2944, @mcol). * `implicit_assignment_linter()` with `allow_scoped=TRUE` doesn't lint for `if (a <- 1) print(a)` (#2913, @MichaelChirico). +* `paste_linter()` doesn't lint `expression(paste(., sep = ""))` (#2945, @mcol). ### Lint accuracy fixes: removing false negatives diff --git a/R/paste_linter.R b/R/paste_linter.R index 89ecc8bfc..03c658001 100644 --- a/R/paste_linter.R +++ b/R/paste_linter.R @@ -75,6 +75,11 @@ #' ) #' #' lint( +#' text = 'expression(paste("a", "b", sep = ""))', +#' linters = paste_linter() +#' ) +#' +#' lint( #' text = 'toString(c("a", "b"))', #' linters = paste_linter() #' ) @@ -118,7 +123,8 @@ paste_linter <- function(allow_empty_sep = FALSE, check_file_paths <- allow_file_path %in% c("double_slash", "never") paste_sep_xpath <- " - following-sibling::SYMBOL_SUB[text() = 'sep' and following-sibling::expr[1][STR_CONST]] + following-sibling::SYMBOL_SUB[text() = 'sep' and following-sibling::expr[1][STR_CONST] + and not(parent::*/preceding::expr/SYMBOL_FUNCTION_CALL[text() = 'expression'])] /parent::expr " diff --git a/man/paste_linter.Rd b/man/paste_linter.Rd index 8139cd605..53f4ec73b 100644 --- a/man/paste_linter.Rd +++ b/man/paste_linter.Rd @@ -86,6 +86,11 @@ lint( linters = paste_linter(allow_empty_sep = TRUE) ) +lint( + text = 'expression(paste("a", "b", sep = ""))', + linters = paste_linter() +) + lint( text = 'toString(c("a", "b"))', linters = paste_linter() diff --git a/tests/testthat/test-paste_linter.R b/tests/testthat/test-paste_linter.R index b2409692f..500157e7c 100644 --- a/tests/testthat/test-paste_linter.R +++ b/tests/testthat/test-paste_linter.R @@ -8,6 +8,7 @@ test_that("paste_linter skips allowed usages for sep=''", { expect_no_lint("sep <- ''; paste('a', sep)", linter) expect_no_lint("paste(sep = ',', '', 'a')", linter) expect_no_lint("paste0('a', 'b', 'c')", linter) + expect_no_lint("expression(paste('a', b, sep = ''))", linter) }) test_that("paste_linter blocks simple disallowed usages for sep=''", { @@ -22,6 +23,12 @@ test_that("paste_linter blocks simple disallowed usages for sep=''", { rex::rex('paste0(...) is better than paste(..., sep = "").'), paste_linter() ) + + expect_lint( + "paste('a', 'b', expression(2), sep = '')", + rex::rex('paste0(...) is better than paste(..., sep = "").'), + paste_linter() + ) }) test_that("paste_linter skips allowed usages for collapse=', '", { From c592d537c6ab92bdd338ea51db582a7a2c75ab84 Mon Sep 17 00:00:00 2001 From: Marco Colombo Date: Tue, 21 Oct 2025 14:10:06 +0200 Subject: [PATCH 2/6] Reworked to fix a false negative. --- R/paste_linter.R | 2 +- tests/testthat/test-paste_linter.R | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/R/paste_linter.R b/R/paste_linter.R index 03c658001..fb107bffa 100644 --- a/R/paste_linter.R +++ b/R/paste_linter.R @@ -124,7 +124,7 @@ paste_linter <- function(allow_empty_sep = FALSE, paste_sep_xpath <- " following-sibling::SYMBOL_SUB[text() = 'sep' and following-sibling::expr[1][STR_CONST] - and not(parent::*/preceding::expr/SYMBOL_FUNCTION_CALL[text() = 'expression'])] + and not(parent::expr/preceding-sibling::expr/SYMBOL_FUNCTION_CALL[text() = 'expression'])] /parent::expr " diff --git a/tests/testthat/test-paste_linter.R b/tests/testthat/test-paste_linter.R index 500157e7c..6bebd6d76 100644 --- a/tests/testthat/test-paste_linter.R +++ b/tests/testthat/test-paste_linter.R @@ -29,6 +29,12 @@ test_that("paste_linter blocks simple disallowed usages for sep=''", { rex::rex('paste0(...) is better than paste(..., sep = "").'), paste_linter() ) + + expect_lint( + "c(expression(2), paste('a', 'b', sep = ''))", + rex::rex('paste0(...) is better than paste(..., sep = "").'), + paste_linter() + ) }) test_that("paste_linter skips allowed usages for collapse=', '", { From bd791e74fb993a1550e3e889debce1b694bccf73 Mon Sep 17 00:00:00 2001 From: Marco Colombo Date: Fri, 28 Nov 2025 10:17:03 +0100 Subject: [PATCH 3/6] Lint expression(paste(., sep = "")) differently in paste_linter. --- NEWS.md | 2 +- R/paste_linter.R | 24 ++++++++++++++++++++---- man/paste_linter.Rd | 4 +++- tests/testthat/test-paste_linter.R | 12 +++++++++--- 4 files changed, 33 insertions(+), 9 deletions(-) diff --git a/NEWS.md b/NEWS.md index 841161cac..bddbede21 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,7 +4,7 @@ ### Lint accuracy fixes: removing false positives -* `paste_linter()` doesn't lint `expression(paste(., sep = ""))` (#2945, @mcol). +* `paste_linter()` lints `expression(paste(., sep = ""))` because the `paste` inside an expression doesn't support the `sep` argument (#2945, @mcol). # lintr (3.3.0-1) diff --git a/R/paste_linter.R b/R/paste_linter.R index fb107bffa..94cc48a8b 100644 --- a/R/paste_linter.R +++ b/R/paste_linter.R @@ -5,7 +5,9 @@ #' The following issues are linted by default by this linter #' (see arguments for which can be de-activated optionally): #' -#' 1. Block usage of [base::paste()] with `sep = ""`. [base::paste0()] is a faster, more concise alternative. +#' 1. Block usage of [base::paste()] with `sep = ""`. [base::paste0()] is a faster, more concise alternative; +#' this is valid unless `paste` occurs inside [base::expression], which according to [grDevices::plotmath] +#' does not support the `sep` argument. #' 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 @@ -123,11 +125,14 @@ paste_linter <- function(allow_empty_sep = FALSE, check_file_paths <- allow_file_path %in% c("double_slash", "never") paste_sep_xpath <- " + following-sibling::SYMBOL_SUB[text() = 'sep' and following-sibling::expr[1][STR_CONST]] + /parent::expr + " + expression_paste_sep_xpath <- " following-sibling::SYMBOL_SUB[text() = 'sep' and following-sibling::expr[1][STR_CONST] - and not(parent::expr/preceding-sibling::expr/SYMBOL_FUNCTION_CALL[text() = 'expression'])] + and parent::expr/preceding-sibling::expr/SYMBOL_FUNCTION_CALL[text() = 'expression']] /parent::expr " - to_string_xpath <- " parent::expr[ count(expr) = 3 @@ -190,7 +195,18 @@ paste_linter <- function(allow_empty_sep = FALSE, paste_sep_value <- get_r_string(paste_sep_expr, xpath = "./SYMBOL_SUB[text() = 'sep']/following-sibling::expr[1]") } - if (!allow_empty_sep) { + ## check if we are inside an expression() + expression_paste_sep_expr <- xml_find_all(paste_calls, expression_paste_sep_xpath) + if (length(expression_paste_sep_expr) > 0) { + optional_lints <- c(optional_lints, xml_nodes_to_lints( + expression_paste_sep_expr[1], + source_expression = source_expression, + lint_message = "inside expression(...), paste does not accept a 'sep' argument.", + type = "warning" + )) + } + + else if (!allow_empty_sep) { optional_lints <- c(optional_lints, xml_nodes_to_lints( paste_sep_expr[!nzchar(paste_sep_value)], source_expression = source_expression, diff --git a/man/paste_linter.Rd b/man/paste_linter.Rd index 53f4ec73b..589041dcb 100644 --- a/man/paste_linter.Rd +++ b/man/paste_linter.Rd @@ -29,7 +29,9 @@ when it comes at the beginning or end of the input, to avoid requiring empty inp The following issues are linted by default by this linter (see arguments for which can be de-activated optionally): \enumerate{ -\item Block usage of \code{\link[base:paste]{base::paste()}} with \code{sep = ""}. \code{\link[base:paste]{base::paste0()}} is a faster, more concise alternative. +\item Block usage of \code{\link[base:paste]{base::paste()}} with \code{sep = ""}. \code{\link[base:paste]{base::paste0()}} is a faster, more concise alternative; +this is valid unless \code{paste} occurs inside \link[base:expression]{base::expression}, which according to \link[grDevices:plotmath]{grDevices::plotmath} +does not support the \code{sep} argument. \item Block usage of \code{paste()} or \code{paste0()} with \code{collapse = ", "}. \code{\link[=toString]{toString()}} is a direct wrapper for this, and alternatives like \code{\link[glue:glue_collapse]{glue::glue_collapse()}} might give better messages for humans. \item Block usage of \code{paste0()} that supplies \verb{sep=} -- this is not a formal argument to \code{paste0}, and diff --git a/tests/testthat/test-paste_linter.R b/tests/testthat/test-paste_linter.R index 6bebd6d76..2889655ff 100644 --- a/tests/testthat/test-paste_linter.R +++ b/tests/testthat/test-paste_linter.R @@ -8,7 +8,8 @@ test_that("paste_linter skips allowed usages for sep=''", { expect_no_lint("sep <- ''; paste('a', sep)", linter) expect_no_lint("paste(sep = ',', '', 'a')", linter) expect_no_lint("paste0('a', 'b', 'c')", linter) - expect_no_lint("expression(paste('a', b, sep = ''))", linter) + expect_no_lint("expression(2); paste('a', 'b', 'c', sep = ',')", linter) + expect_no_lint("paste('a', 'b', expression(2), sep = ',')", linter) }) test_that("paste_linter blocks simple disallowed usages for sep=''", { @@ -31,8 +32,13 @@ test_that("paste_linter blocks simple disallowed usages for sep=''", { ) expect_lint( - "c(expression(2), paste('a', 'b', sep = ''))", - rex::rex('paste0(...) is better than paste(..., sep = "").'), + "c(expression(paste('a', 'b', sep = ',')))", + rex::rex("inside expression(...), paste does not accept a 'sep' argument."), + paste_linter() + ) + expect_lint( + "c(expression(paste('a', 'b', sep = '')))", + rex::rex("inside expression(...), paste does not accept a 'sep' argument."), paste_linter() ) }) From f4c9d88e0450bd9fd878f9aee5789c263a16798f Mon Sep 17 00:00:00 2001 From: Marco Colombo Date: Fri, 28 Nov 2025 10:26:32 +0100 Subject: [PATCH 4/6] Fix lints. --- R/paste_linter.R | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/R/paste_linter.R b/R/paste_linter.R index 94cc48a8b..ac581a995 100644 --- a/R/paste_linter.R +++ b/R/paste_linter.R @@ -197,16 +197,14 @@ paste_linter <- function(allow_empty_sep = FALSE, ## check if we are inside an expression() expression_paste_sep_expr <- xml_find_all(paste_calls, expression_paste_sep_xpath) - if (length(expression_paste_sep_expr) > 0) { + if (length(expression_paste_sep_expr) > 0L) { optional_lints <- c(optional_lints, xml_nodes_to_lints( - expression_paste_sep_expr[1], + expression_paste_sep_expr[1L], source_expression = source_expression, lint_message = "inside expression(...), paste does not accept a 'sep' argument.", type = "warning" )) - } - - else if (!allow_empty_sep) { + } else if (!allow_empty_sep) { optional_lints <- c(optional_lints, xml_nodes_to_lints( paste_sep_expr[!nzchar(paste_sep_value)], source_expression = source_expression, From 66bfb7f33afb449045789f3f9d1467a6004a729f Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 28 Nov 2025 10:12:31 -0500 Subject: [PATCH 5/6] XPath style --- R/paste_linter.R | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/R/paste_linter.R b/R/paste_linter.R index ac581a995..d671841cb 100644 --- a/R/paste_linter.R +++ b/R/paste_linter.R @@ -129,8 +129,11 @@ paste_linter <- function(allow_empty_sep = FALSE, /parent::expr " expression_paste_sep_xpath <- " - following-sibling::SYMBOL_SUB[text() = 'sep' and following-sibling::expr[1][STR_CONST] - and parent::expr/preceding-sibling::expr/SYMBOL_FUNCTION_CALL[text() = 'expression']] + following-sibling::SYMBOL_SUB[ + text() = 'sep' + and following-sibling::expr[1][STR_CONST] + and parent::expr/preceding-sibling::expr/SYMBOL_FUNCTION_CALL[text() = 'expression'] + ] /parent::expr " to_string_xpath <- " From e0418615c9a09a5c3d70fd9487d9960c379de96e Mon Sep 17 00:00:00 2001 From: Marco Colombo Date: Fri, 28 Nov 2025 16:17:21 +0100 Subject: [PATCH 6/6] Move example under the right section. --- R/paste_linter.R | 10 +++++----- man/paste_linter.Rd | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/R/paste_linter.R b/R/paste_linter.R index d671841cb..b68023f79 100644 --- a/R/paste_linter.R +++ b/R/paste_linter.R @@ -65,6 +65,11 @@ #' linters = paste_linter() #' ) #' +#' lint( +#' text = 'expression(paste("a", "b", sep = ""))', +#' linters = paste_linter() +#' ) +#' #' # okay #' lint( #' text = 'paste0("a", "b")', @@ -77,11 +82,6 @@ #' ) #' #' lint( -#' text = 'expression(paste("a", "b", sep = ""))', -#' linters = paste_linter() -#' ) -#' -#' lint( #' text = 'toString(c("a", "b"))', #' linters = paste_linter() #' ) diff --git a/man/paste_linter.Rd b/man/paste_linter.Rd index 589041dcb..ce3cfcb2c 100644 --- a/man/paste_linter.Rd +++ b/man/paste_linter.Rd @@ -77,6 +77,11 @@ lint( linters = paste_linter() ) +lint( + text = 'expression(paste("a", "b", sep = ""))', + linters = paste_linter() +) + # okay lint( text = 'paste0("a", "b")', @@ -88,11 +93,6 @@ lint( linters = paste_linter(allow_empty_sep = TRUE) ) -lint( - text = 'expression(paste("a", "b", sep = ""))', - linters = paste_linter() -) - lint( text = 'toString(c("a", "b"))', linters = paste_linter()