diff --git a/NEWS.md b/NEWS.md index 5eddfba41..532f0e710 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,6 +4,12 @@ * Six linters fully deprecated in the previous release are now removed: `consecutive_stopifnot_linter()`, `extraction_operator_linter()`, `no_tab_linter()`, `single_quotes_linter()`, `unnecessary_nested_if_linter()`, and `unneeded_concatenation_linter()`. +## New and improved features + +### Lint accuracy fixes: removing false positives + +* `paste_linter()` lints `expression(paste(., sep = ""))` because the `paste` inside an expression doesn't support the `sep` argument (#2945, @mcol). + ## Notes * {lintr} now requires R 4.1.0 @@ -80,10 +86,6 @@ * `object_name_linter()` and `object_length_linter()` account for S3 class correctly when the generic is assigned with `=` (#2507, @MichaelChirico). * `assignment_linter()` with `operator = "="` does a better job of skipping implicit assignments, which are intended to be governed by `implicit_assignment_linter()` (#2765, @MichaelChirico). * `implicit_assignment_linter()` with `allow_scoped=TRUE` doesn't lint for `if (a <- 1) print(a)` (#2913, @MichaelChirico). -* `expect_true_false_linter()` is pipe-aware, so that `42 |> expect_identical(x, ignore_attr = TRUE)` no longer lints (#1520, @MichaelChirico). -* `T_and_F_symbol_linter()` ignores `T` and `F`: - + When used as symbols in formulas (`y ~ T + F`), which can represent variables in data not controlled by the author (#2637, @MichaelChirico). - + If followed by `[` or `[[` (#2944, @mcol). ### Lint accuracy fixes: removing false negatives diff --git a/R/paste_linter.R b/R/paste_linter.R index 89ecc8bfc..b68023f79 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 @@ -63,6 +65,11 @@ #' linters = paste_linter() #' ) #' +#' lint( +#' text = 'expression(paste("a", "b", sep = ""))', +#' linters = paste_linter() +#' ) +#' #' # okay #' lint( #' text = 'paste0("a", "b")', @@ -121,7 +128,14 @@ paste_linter <- function(allow_empty_sep = FALSE, 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 parent::expr/preceding-sibling::expr/SYMBOL_FUNCTION_CALL[text() = 'expression'] + ] + /parent::expr + " to_string_xpath <- " parent::expr[ count(expr) = 3 @@ -184,7 +198,16 @@ 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) > 0L) { + optional_lints <- c(optional_lints, xml_nodes_to_lints( + 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) { 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 8139cd605..ce3cfcb2c 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 @@ -75,6 +77,11 @@ lint( linters = paste_linter() ) +lint( + text = 'expression(paste("a", "b", sep = ""))', + linters = paste_linter() +) + # okay lint( text = 'paste0("a", "b")', diff --git a/tests/testthat/test-paste_linter.R b/tests/testthat/test-paste_linter.R index b2409692f..2889655ff 100644 --- a/tests/testthat/test-paste_linter.R +++ b/tests/testthat/test-paste_linter.R @@ -8,6 +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(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=''", { @@ -22,6 +24,23 @@ 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() + ) + + expect_lint( + "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() + ) }) test_that("paste_linter skips allowed usages for collapse=', '", {