From b88ca0254c02ff2f603b3c4ed2b90e6de70cf4af Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 28 Mar 2022 22:59:58 -0700 Subject: [PATCH] New paste_to_string_linter (#1013) * New paste_to_string_linter * improve xpath readability * remove dQuote/sQuote * note better alternatives * missing comma * update documenation * roxygenize again --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 1 + R/paste_to_string_linter.R | 39 ++++++++++++++++++++ R/xp_utils.R | 10 ++++- inst/lintr/linters.csv | 1 + man/best_practices_linters.Rd | 1 + man/consistency_linters.Rd | 1 + man/linters.Rd | 5 ++- man/paste_to_string_linter.Rd | 18 +++++++++ tests/testthat/test-paste_to_string_linter.R | 33 +++++++++++++++++ 11 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 R/paste_to_string_linter.R create mode 100644 man/paste_to_string_linter.Rd create mode 100644 tests/testthat/test-paste_to_string_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 8aeb2318b..e6cfd8012 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -113,6 +113,7 @@ Collate: 'paren_body_linter.R' 'paren_brace_linter.R' 'paste_sep_linter.R' + 'paste_to_string_linter.R' 'path_linters.R' 'pipe_call_linter.R' 'pipe_continuation_linter.R' diff --git a/NAMESPACE b/NAMESPACE index 5663327b5..d644248a8 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -78,6 +78,7 @@ export(package_hooks_linter) export(paren_body_linter) export(paren_brace_linter) export(paste_sep_linter) +export(paste_to_string_linter) export(pipe_call_linter) export(pipe_continuation_linter) export(redundant_ifelse_linter) diff --git a/NEWS.md b/NEWS.md index 755a4af39..44bb8eb9a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -115,6 +115,7 @@ function calls. (#850, #851, @renkun-ken) * `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 = ", ")` * `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_to_string_linter.R b/R/paste_to_string_linter.R new file mode 100644 index 000000000..a5935ad29 --- /dev/null +++ b/R/paste_to_string_linter.R @@ -0,0 +1,39 @@ +#' 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/R/xp_utils.R b/R/xp_utils.R index acf29a094..78bd999a8 100644 --- a/R/xp_utils.R +++ b/R/xp_utils.R @@ -1,7 +1,15 @@ # utils for working with xpaths # like `text() %in% table`, translated to XPath 1.0 -xp_text_in_table <- function(table) paste("text() = ", quote_wrap(table, "'"), collapse = " or ") +xp_text_in_table <- function(table) { + # xpath doesn't seem to have a standard way of escaping quotes, so attempt + # to use "" whenever the string has ' (not a perfect solution). info on + # escaping from https://stackoverflow.com/questions/14822153 + single_quoted <- grepl("'", table, fixed = TRUE) + table[single_quoted] <- paste0('"', table[single_quoted], '"') + table[!single_quoted] <- paste0("'", table[!single_quoted], "'") + return(paste0("text() = ", table, collapse = " or ")) +} # convert an XML match into a Lint xml_nodes_to_lint <- function(xml, source_file, lint_message, diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 860c0ae1e..1ae62e5bd 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -51,6 +51,7 @@ 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 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 c6c1e771b..c99ae5c24 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -36,6 +36,7 @@ The following linters are tagged with 'best_practices': \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{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 78b9eeb17..0807693ce 100644 --- a/man/consistency_linters.Rd +++ b/man/consistency_linters.Rd @@ -24,6 +24,7 @@ The following linters are tagged with 'consistency': \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{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 d734e341c..a7d598844 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} (34 linters)} +\item{\link[=best_practices_linters]{best_practices} (35 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (5 linters)} \item{\link[=configurable_linters]{configurable} (16 linters)} -\item{\link[=consistency_linters]{consistency} (15 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)} @@ -85,6 +85,7 @@ The following linters exist: \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{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_to_string_linter.Rd b/man/paste_to_string_linter.Rd new file mode 100644 index 000000000..3f3302ef5 --- /dev/null +++ b/man/paste_to_string_linter.Rd @@ -0,0 +1,18 @@ +% 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_to_string_linter.R b/tests/testthat/test-paste_to_string_linter.R new file mode 100644 index 000000000..3e2b5d0ab --- /dev/null +++ b/tests/testthat/test-paste_to_string_linter.R @@ -0,0 +1,33 @@ +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() + ) +})