diff --git a/DESCRIPTION b/DESCRIPTION index b153bfb8d..eac3f6c9e 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -112,8 +112,7 @@ Collate: 'package_hooks_linter.R' 'paren_body_linter.R' 'paren_brace_linter.R' - 'paste_sep_linter.R' - 'paste_to_string_linter.R' + 'paste_linter.R' 'path_linters.R' 'pipe_call_linter.R' 'pipe_continuation_linter.R' diff --git a/NAMESPACE b/NAMESPACE index 781d457a9..0619bd8f5 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -79,8 +79,7 @@ export(outer_negation_linter) export(package_hooks_linter) export(paren_body_linter) export(paren_brace_linter) -export(paste_sep_linter) -export(paste_to_string_linter) +export(paste_linter) export(pipe_call_linter) export(pipe_continuation_linter) export(redundant_ifelse_linter) diff --git a/NEWS.md b/NEWS.md index d326b51e1..97e378076 100644 --- a/NEWS.md +++ b/NEWS.md @@ -116,8 +116,9 @@ function calls. (#850, #851, @renkun-ken) * `outer_negation_linter()` Require usage of `!any(x)` over `all(!x)` and `!all(x)` over `any(!x)` * `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 = ", ")` + * `paste_linter()` lint for common mis-use of `paste()` and `paste()`: + + `paste0()` encouraged instead of `paste(sep = "")` + + `toString()` or `glue::glue_collapse()` encouraged instead of `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)`) diff --git a/R/paste_linter.R b/R/paste_linter.R new file mode 100644 index 000000000..9d28dc358 --- /dev/null +++ b/R/paste_linter.R @@ -0,0 +1,74 @@ +#' 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): +#' +#' 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. +#' +#' @evalRd rd_tags("paste_linter") +#' @param allow_empty_sep Logical, default `FALSE`. If `TRUE`, usage of +#' `paste()` with `sep = ""` is not linted. +#' @param allow_to_string Logical, default `FALSE`. If `TRUE`, usage of +#' `paste()` and `paste0()` with `collapse = ", "` is not linted. +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +paste_linter <- function(allow_empty_sep = FALSE, allow_to_string = FALSE) { + Linter(function(source_file) { + if (length(source_file$xml_parsed_content) == 0L) { + return(list()) + } + + xml <- source_file$xml_parsed_content + + if (allow_empty_sep) { + empty_sep_lints <- NULL + } else { + # NB: string-length()=2 means '' or "" (*not* 'ab' or 'cd' which have length 4) + # TODO: adapt this for R>4.0 raw strings + empty_sep_xpath <- "//expr[ + expr[SYMBOL_FUNCTION_CALL[text() = 'paste']] + and SYMBOL_SUB[text() = 'sep']/following-sibling::expr[1][STR_CONST[string-length(text()) = 2]] + ]" + + empty_sep_expr <- xml2::xml_find_all(xml, empty_sep_xpath) + + empty_sep_lints <- lapply( + empty_sep_expr, + xml_nodes_to_lint, + source_file = source_file, + lint_message = 'paste0(...) is better than paste(..., sep = "").', + type = "warning" + ) + } + + if (allow_to_string) { + to_string_lints <- NULL + } else { + # 3 expr: the function call, the argument, and collapse= + # TODO(michaelchirico): catch raw-string equivalents + seps <- c("', '", '", "') + to_string_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)} ]] + ]") + to_string_expr <- xml2::xml_find_all(xml, to_string_xpath) + + to_string_lints <- lapply( + to_string_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" + ) + } + + c(empty_sep_lints, to_string_lints) + }) +} diff --git a/R/paste_sep_linter.R b/R/paste_sep_linter.R deleted file mode 100644 index a4150eb9c..000000000 --- a/R/paste_sep_linter.R +++ /dev/null @@ -1,33 +0,0 @@ -#' Block usage of paste() with sep="" -#' -#' [paste0()] is a faster, more concise alternative to using `paste(sep = "")`. -#' -#' @evalRd rd_tags("paste_sep_linter") -#' @seealso [linters] for a complete list of linters available in lintr. -#' @export -paste_sep_linter <- function() { - Linter(function(source_file) { - if (length(source_file$xml_parsed_content) == 0L) { - return(list()) - } - - xml <- source_file$xml_parsed_content - - # NB: string-length()=2 means '' or "" (*not* 'ab' or 'cd' which have length 4) - # TODO: adapt this for R>4.0 raw strings - xpath <- "//expr[ - expr[SYMBOL_FUNCTION_CALL[text() = 'paste']] - and SYMBOL_SUB[text() = 'sep']/following-sibling::expr[1][STR_CONST[string-length(text()) = 2]] - ]" - - bad_expr <- xml2::xml_find_all(xml, xpath) - - return(lapply( - bad_expr, - xml_nodes_to_lint, - source_file = source_file, - lint_message = 'paste0(...) is better than paste(..., sep = "").', - type = "warning" - )) - }) -} diff --git a/R/paste_to_string_linter.R b/R/paste_to_string_linter.R deleted file mode 100644 index a5935ad29..000000000 --- a/R/paste_to_string_linter.R +++ /dev/null @@ -1,39 +0,0 @@ -#' 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" - )) - }) -} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index fbe99fd75..f971b8c18 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -50,8 +50,7 @@ outer_negation_linter,readability efficiency best_practices 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 +paste_linter,best_practices consistency pipe_call_linter,style readability pipe_continuation_linter,style readability default redundant_ifelse_linter,best_practices efficiency consistency diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index c99ae5c24..0720eb81e 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -35,8 +35,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{literal_coercion_linter}}} \item{\code{\link{nonportable_path_linter}}} \item{\code{\link{outer_negation_linter}}} -\item{\code{\link{paste_sep_linter}}} -\item{\code{\link{paste_to_string_linter}}} +\item{\code{\link{paste_linter}}} \item{\code{\link{redundant_ifelse_linter}}} \item{\code{\link{regex_subset_linter}}} \item{\code{\link{seq_linter}}} diff --git a/man/consistency_linters.Rd b/man/consistency_linters.Rd index 0807693ce..378c4904c 100644 --- a/man/consistency_linters.Rd +++ b/man/consistency_linters.Rd @@ -23,8 +23,7 @@ The following linters are tagged with 'consistency': \item{\code{\link{no_tab_linter}}} \item{\code{\link{numeric_leading_zero_linter}}} \item{\code{\link{object_name_linter}}} -\item{\code{\link{paste_sep_linter}}} -\item{\code{\link{paste_to_string_linter}}} +\item{\code{\link{paste_linter}}} \item{\code{\link{redundant_ifelse_linter}}} \item{\code{\link{seq_linter}}} \item{\code{\link{single_quotes_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index 9befecfd9..973eb524d 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,10 +17,10 @@ Documentation for linters is structured into tags to allow for easier discovery. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (35 linters)} +\item{\link[=best_practices_linters]{best_practices} (34 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (5 linters)} \item{\link[=configurable_linters]{configurable} (16 linters)} -\item{\link[=consistency_linters]{consistency} (17 linters)} +\item{\link[=consistency_linters]{consistency} (16 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (28 linters)} \item{\link[=efficiency_linters]{efficiency} (14 linters)} @@ -84,8 +84,7 @@ The following linters exist: \item{\code{\link{package_hooks_linter}} (tags: correctness, package_development, style)} \item{\code{\link{paren_body_linter}} (tags: default, readability, style)} \item{\code{\link{paren_brace_linter}} (tags: default, readability, style)} -\item{\code{\link{paste_sep_linter}} (tags: best_practices, consistency)} -\item{\code{\link{paste_to_string_linter}} (tags: best_practices, consistency)} +\item{\code{\link{paste_linter}} (tags: best_practices, consistency)} \item{\code{\link{pipe_call_linter}} (tags: readability, style)} \item{\code{\link{pipe_continuation_linter}} (tags: default, readability, style)} \item{\code{\link{redundant_ifelse_linter}} (tags: best_practices, consistency, efficiency)} diff --git a/man/paste_linter.Rd b/man/paste_linter.Rd new file mode 100644 index 000000000..724be6a6a --- /dev/null +++ b/man/paste_linter.Rd @@ -0,0 +1,32 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/paste_linter.R +\name{paste_linter} +\alias{paste_linter} +\title{Raise lints for several common poor usages of paste()} +\usage{ +paste_linter(allow_empty_sep = FALSE, allow_to_string = FALSE) +} +\arguments{ +\item{allow_empty_sep}{Logical, default \code{FALSE}. If \code{TRUE}, usage of +\code{paste()} with \code{sep = ""} is not linted.} + +\item{allow_to_string}{Logical, default \code{FALSE}. If \code{TRUE}, usage of +\code{paste()} and \code{paste0()} with \code{collapse = ", "} is not linted.} +} +\description{ +The following issues are linted by default by this linter +(and each can be turned off optionally): +} +\details{ +\enumerate{ +\item Block usage of \code{\link[=paste]{paste()}} with \code{sep = ""}. \code{\link[=paste0]{paste0()}} is a faster, more concise alternative. +\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. +} +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=consistency_linters]{consistency} +} diff --git a/man/paste_sep_linter.Rd b/man/paste_sep_linter.Rd deleted file mode 100644 index 82f8f08b8..000000000 --- a/man/paste_sep_linter.Rd +++ /dev/null @@ -1,17 +0,0 @@ -% Generated by roxygen2: do not edit by hand -% Please edit documentation in R/paste_sep_linter.R -\name{paste_sep_linter} -\alias{paste_sep_linter} -\title{Block usage of paste() with sep=""} -\usage{ -paste_sep_linter() -} -\description{ -\code{\link[=paste0]{paste0()}} is a faster, more concise alternative to using \code{paste(sep = "")}. -} -\seealso{ -\link{linters} for a complete list of linters available in lintr. -} -\section{Tags}{ -\link[=best_practices_linters]{best_practices}, \link[=consistency_linters]{consistency} -} diff --git a/man/paste_to_string_linter.Rd b/man/paste_to_string_linter.Rd deleted file mode 100644 index 3f3302ef5..000000000 --- a/man/paste_to_string_linter.Rd +++ /dev/null @@ -1,18 +0,0 @@ -% Generated by roxygen2: do not edit by hand -% Please edit documentation in R/paste_to_string_linter.R -\name{paste_to_string_linter} -\alias{paste_to_string_linter} -\title{Block usage of paste() with collapse=", "} -\usage{ -paste_to_string_linter() -} -\description{ -\code{\link[=toString]{toString()}} is a more concise and expressive wrapper for aggregating string -vectors with \code{paste(., collapse = ", ")}. -} -\seealso{ -\link{linters} for a complete list of linters available in lintr. -} -\section{Tags}{ -\link[=best_practices_linters]{best_practices}, \link[=consistency_linters]{consistency} -} diff --git a/tests/testthat/test-paste_linter.R b/tests/testthat/test-paste_linter.R new file mode 100644 index 000000000..c078f1377 --- /dev/null +++ b/tests/testthat/test-paste_linter.R @@ -0,0 +1,66 @@ +test_that("paste_linter skips allowed usages for sep=''", { + expect_lint("paste('a', 'b', 'c')", NULL, paste_linter()) + expect_lint("paste('a', 'b', 'c', sep = ',')", NULL, paste_linter()) + expect_lint("paste('a', 'b', collapse = '')", NULL, paste_linter()) + expect_lint("cat(paste('a', 'b'), sep = '')", NULL, paste_linter()) + expect_lint("sep <- ''; paste('a', sep)", NULL, paste_linter()) + expect_lint("paste(sep = ',', '', 'a')", NULL, paste_linter()) + + expect_lint("paste0('a', 'b', 'c')", NULL, paste_linter()) +}) + +test_that("paste_linter blocks simple disallowed usages for sep=''", { + expect_lint( + "paste(sep = '', 'a', 'b')", + rex::rex('paste0(...) is better than paste(..., sep = "").'), + paste_linter() + ) + + expect_lint( + "paste('a', 'b', sep = '')", + rex::rex('paste0(...) is better than paste(..., sep = "").'), + paste_linter() + ) +}) + +test_that("paste_linter skips allowed usages for collapse=', '", { + expect_lint("paste('a', 'b', 'c')", NULL, paste_linter()) + expect_lint("paste(x, sep = ', ')", NULL, paste_linter()) + expect_lint("paste(x, collapse = ',')", NULL, paste_linter()) + expect_lint("paste(foo(x), collapse = '/')", NULL, paste_linter()) + # harder to catch statically + expect_lint("collapse <- ', '; paste(x, collapse = collapse)", NULL, paste_linter()) + + # paste(..., sep=sep, collapse=", ") is not a trivial swap to toString + expect_lint("paste(x, y, sep = '.', collapse = ', ')", NULL, paste_linter()) + # any call involving ...length() > 1 will implicitly use the default sep + expect_lint("paste(x, y, collapse = ', ')", NULL, paste_linter()) + expect_lint("paste0(x, y, collapse = ', ')", NULL, paste_linter()) + + expect_lint("toString(x)", NULL, paste_linter()) + + # string match of ", " is OK -- lint only _exact_ match + expect_lint('paste(x, collapse = ", \n")', NULL, paste_linter()) +}) + +test_that("paste_linter blocks simple disallowed usages for collapse=', '", { + expect_lint( + "paste(collapse = ', ', x)", + 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)) +}) diff --git a/tests/testthat/test-paste_sep_linter.R b/tests/testthat/test-paste_sep_linter.R deleted file mode 100644 index 2503b6f2a..000000000 --- a/tests/testthat/test-paste_sep_linter.R +++ /dev/null @@ -1,24 +0,0 @@ -test_that("paste_sep_linter skips allowed usages", { - expect_lint("paste('a', 'b', 'c')", NULL, paste_sep_linter()) - expect_lint("paste('a', 'b', 'c', sep = ',')", NULL, paste_sep_linter()) - expect_lint("paste('a', 'b', collapse = '')", NULL, paste_sep_linter()) - expect_lint("cat(paste('a', 'b'), sep = '')", NULL, paste_sep_linter()) - expect_lint("sep <- ''; paste('a', sep)", NULL, paste_sep_linter()) - expect_lint("paste(sep = ',', '', 'a')", NULL, paste_sep_linter()) - - expect_lint("paste0('a', 'b', 'c')", NULL, paste_sep_linter()) -}) - -test_that("paste_sep_linter blocks simple disallowed usages", { - expect_lint( - "paste(sep = '', 'a', 'b')", - rex::rex('paste0(...) is better than paste(..., sep = "").'), - paste_sep_linter() - ) - - expect_lint( - "paste('a', 'b', sep = '')", - rex::rex('paste0(...) is better than paste(..., sep = "").'), - paste_sep_linter() - ) -}) diff --git a/tests/testthat/test-paste_to_string_linter.R b/tests/testthat/test-paste_to_string_linter.R deleted file mode 100644 index 3e2b5d0ab..000000000 --- a/tests/testthat/test-paste_to_string_linter.R +++ /dev/null @@ -1,33 +0,0 @@ -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() - ) -})