From 87182897dde9cf4b67e2301dab34ff1ea4949c8f Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 28 Mar 2022 18:59:31 +0000 Subject: [PATCH 1/7] New paste_to_string_linter --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 1 + R/paste_to_string_linter.R | 37 ++++++++++++++++++++ R/xp_utils.R | 10 +++++- inst/lintr/linters.csv | 1 + man/best_practices_linters.Rd | 1 + man/consistency_linters.Rd | 1 + man/linters.Rd | 7 ++-- man/paste_to_string_linter.Rd | 18 ++++++++++ tests/testthat/test-paste_to_string_linter.R | 33 +++++++++++++++++ 11 files changed, 107 insertions(+), 4 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 725ee0b46..605dc1e77 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -111,6 +111,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 23ccda32f..6545414a2 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -76,6 +76,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 a3c1f665b..3394a34ac 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 * `redundant_ifelse_linter()` Prevent usage like `ifelse(A & B, TRUE, FALSE)` or `ifelse(C, 0, 1)` (the latter is `as.numeric(!C)`) * `else_same_line_linter()` Require `else` to come on the same line as the preceding `}`, if present diff --git a/R/paste_to_string_linter.R b/R/paste_to_string_linter.R new file mode 100644 index 000000000..9fe3de5a5 --- /dev/null +++ b/R/paste_to_string_linter.R @@ -0,0 +1,37 @@ +#' 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 + + paste_cond_fmt <- "expr[SYMBOL_FUNCTION_CALL[%s]]" + collapse_cond_fmt <- "SYMBOL_SUB[text() = 'collapse']/following-sibling::expr[1][STR_CONST[%s]]" + + # 3 expr: the function call, the argument, and collapse= + # TODO(michaelchirico): catch raw-string equivalents + xpath <- sprintf( + "//expr[count(expr) = 3 and %s and %s]", + sprintf(collapse_cond_fmt, xp_text_in_table(c("', '", '", "'))), + sprintf(paste_cond_fmt, xp_text_in_table(c("paste", "paste0"))) + ) + bad_expr <- xml2::xml_find_all(xml, xpath) + + return(lapply( + bad_expr, + xml_nodes_to_lint, + source_file = source_file, + lint_message = 'toString(.) is more expressive than paste(., collapse = ", ")', + type = "warning" + )) + }) +} diff --git a/R/xp_utils.R b/R/xp_utils.R index acf29a094..a00da4195 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] <- dQuote(table[single_quoted], '"') + table[!single_quoted] <- sQuote(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 749fc291e..e524eb776 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -49,6 +49,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 c8f3cd81b..46eb2f28f 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -35,6 +35,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 8f2cedb75..6bc4e0a94 100644 --- a/man/consistency_linters.Rd +++ b/man/consistency_linters.Rd @@ -22,6 +22,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 21ed55af2..cc4187288 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,13 +17,13 @@ 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} (32 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} (14 linters)} +\item{\link[=consistency_linters]{consistency} (15 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (28 linters)} -\item{\link[=efficiency_linters]{efficiency} (12 linters)} +\item{\link[=efficiency_linters]{efficiency} (13 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} \item{\link[=readability_linters]{readability} (35 linters)} \item{\link[=robustness_linters]{robustness} (11 linters)} @@ -83,6 +83,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..7246291c6 --- /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{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() + ) +}) From ab5e80e184eeb2834e2184fff17cf29d07b12548 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 28 Mar 2022 21:28:40 +0000 Subject: [PATCH 2/7] improve xpath readability --- R/paste_to_string_linter.R | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/R/paste_to_string_linter.R b/R/paste_to_string_linter.R index 9fe3de5a5..fc51db4de 100644 --- a/R/paste_to_string_linter.R +++ b/R/paste_to_string_linter.R @@ -14,16 +14,14 @@ paste_to_string_linter <- function() { xml <- source_file$xml_parsed_content - paste_cond_fmt <- "expr[SYMBOL_FUNCTION_CALL[%s]]" - collapse_cond_fmt <- "SYMBOL_SUB[text() = 'collapse']/following-sibling::expr[1][STR_CONST[%s]]" - # 3 expr: the function call, the argument, and collapse= # TODO(michaelchirico): catch raw-string equivalents - xpath <- sprintf( - "//expr[count(expr) = 3 and %s and %s]", - sprintf(collapse_cond_fmt, xp_text_in_table(c("', '", '", "'))), - sprintf(paste_cond_fmt, xp_text_in_table(c("paste", "paste0"))) - ) + 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( From 5645902674bdb9738f0b179db533df0b8b918068 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 28 Mar 2022 21:29:58 +0000 Subject: [PATCH 3/7] remove dQuote/sQuote --- R/xp_utils.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/xp_utils.R b/R/xp_utils.R index a00da4195..78bd999a8 100644 --- a/R/xp_utils.R +++ b/R/xp_utils.R @@ -6,8 +6,8 @@ xp_text_in_table <- function(table) { # 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] <- dQuote(table[single_quoted], '"') - table[!single_quoted] <- sQuote(table[!single_quoted], "'") + table[single_quoted] <- paste0('"', table[single_quoted], '"') + table[!single_quoted] <- paste0("'", table[!single_quoted], "'") return(paste0("text() = ", table, collapse = " or ")) } From c1a485f55d96df5b107f2c9daa50ef4eff49e760 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 28 Mar 2022 21:33:32 +0000 Subject: [PATCH 4/7] note better alternatives --- R/paste_to_string_linter.R | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/R/paste_to_string_linter.R b/R/paste_to_string_linter.R index fc51db4de..60c411d5c 100644 --- a/R/paste_to_string_linter.R +++ b/R/paste_to_string_linter.R @@ -28,7 +28,11 @@ paste_to_string_linter <- function() { bad_expr, xml_nodes_to_lint, source_file = source_file, - lint_message = 'toString(.) is more expressive than paste(., collapse = ", ")', + 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" )) }) From 9cf6f61a0a3a334ddb17d6814566671f37eefc61 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 28 Mar 2022 21:42:35 +0000 Subject: [PATCH 5/7] missing comma --- R/paste_to_string_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/paste_to_string_linter.R b/R/paste_to_string_linter.R index 60c411d5c..a5935ad29 100644 --- a/R/paste_to_string_linter.R +++ b/R/paste_to_string_linter.R @@ -29,7 +29,7 @@ paste_to_string_linter <- function() { xml_nodes_to_lint, source_file = source_file, lint_message = paste( - 'toString(.) is more expressive than paste(., collapse = ", ").' + 'toString(.) is more expressive than paste(., collapse = ", ").', "Note also glue::glue_collapse() and and::and()", "for constructing human-readable / translation-friendly lists" ), From ffd80a31eaaf0da0b22d6626fcf9ef1c3933a021 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 28 Mar 2022 21:52:58 +0000 Subject: [PATCH 6/7] update documenation --- man/paste_to_string_linter.Rd | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/man/paste_to_string_linter.Rd b/man/paste_to_string_linter.Rd index 7246291c6..3f3302ef5 100644 --- a/man/paste_to_string_linter.Rd +++ b/man/paste_to_string_linter.Rd @@ -7,7 +7,7 @@ paste_to_string_linter() } \description{ -\code{toString} is a more concise and expressive wrapper for aggregating string +\code{\link[=toString]{toString()}} is a more concise and expressive wrapper for aggregating string vectors with \code{paste(., collapse = ", ")}. } \seealso{ From a3c7d35c7183b61394560379a9fff01ec3554723 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 28 Mar 2022 22:47:11 +0000 Subject: [PATCH 7/7] roxygenize again --- man/linters.Rd | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/man/linters.Rd b/man/linters.Rd index 3bab39dfe..bd0d4479c 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} (13 linters)}