From cf9758ad748d02290a6c7021929760394142f5fa Mon Sep 17 00:00:00 2001 From: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com> Date: Tue, 4 Mar 2025 18:27:26 +0000 Subject: [PATCH] Add potential `sequence()` lints to `seq_linter()` (#2618) * Draft sequence_linter linter * Fold sequence_linter() into seq_linter() * Add tests * Add NEWS bullet * Switch to xml_find_function_calls * Change element order in xpath * Support lapply(x, seq) * Simplify xpath * Do not lint seq() calls with extra arguments * Edit false positive example * Add more info about new lints in NEWS * update * Update NEWS.md * add Hugo to DESCRIPTION (and clean it up) * change DESCRIPTION --> change Rd --------- Co-authored-by: Michael Chirico Co-authored-by: Michael Chirico --- DESCRIPTION | 13 ++++++++----- NEWS.md | 4 +++- R/seq_linter.R | 32 +++++++++++++++++++++++++++++++- man/lintr-package.Rd | 3 ++- man/seq_linter.Rd | 10 ++++++++++ tests/testthat/test-seq_linter.R | 30 ++++++++++++++++++++++++++++++ 6 files changed, 84 insertions(+), 8 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index d2643a4257..2af12955e4 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -4,14 +4,17 @@ Version: 3.2.0.9000 Authors@R: c( person("Jim", "Hester", , role = "aut"), person("Florent", "Angly", role = "aut", - comment = "fangly"), + comment = c(GitHub = "fangly")), person("Russ", "Hyde", role = "aut"), - person("Michael", "Chirico", email = "michaelchirico4@gmail.com", role = c("aut", "cre")), + person("Michael", "Chirico", email = "michaelchirico4@gmail.com", role = c("aut", "cre"), + comment = c(ORCID = "0000-0003-0787-087X")), person("Kun", "Ren", role = "aut"), person("Alexander", "Rosenstock", role = "aut", - comment = "AshesITR"), - person("Indrajeet", "Patil", , "patilindrajeet.science@gmail.com", role = "aut", - comment = c(ORCID = "0000-0003-1995-6531", Twitter = "@patilindrajeets")) + comment = c(GitHub = "AshesITR")), + person("Indrajeet", "Patil", email = "patilindrajeet.science@gmail.com", role = "aut", + comment = c(ORCID = "0000-0003-1995-6531", Twitter = "@patilindrajeets")), + person("Hugo", "Gruson", role = "aut", + comment = c(ORCID = "0000-0002-4094-1476")) ) Description: Checks adherence to a given style, syntax errors and possible semantic issues. Supports on the fly checking of R code edited with diff --git a/NEWS.md b/NEWS.md index b7b365b72e..ce71617a6e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -20,7 +20,9 @@ ## New and improved features * `brace_linter()`' has a new argument `function_bodies` (default `"multi_line"`) which controls when to require function bodies to be wrapped in curly braces, with the options `"always"`, `"multi_line"` (only require curly braces when a function body spans multiple lines), `"not_inline"` (only require curly braces when a function body starts on a new line) and `"never"` (#1807, #2240, @salim-b). -* `seq_linter()` recommends using `seq_along(x)` instead of `seq_len(length(x))` (#2577, @MichaelChirico). +* `seq_linter()`: + + recommends using `seq_along(x)` instead of `seq_len(length(x))` (#2577, @MichaelChirico). + + recommends using `sequence()` instead of `unlist(lapply(ints, seq))` (#2618, @Bisaloo) * `undesirable_operator_linter()` lints operators in prefix form, e.g. `` `%%`(x, 2)`` (#1910, @MichaelChirico). Disable this by setting `call_is_undesirable=FALSE`. * `indentation_linter()` handles `for` un-braced for loops correctly (#2564, @MichaelChirico). * Setting `exclusions` supports globs like `knitr*` to exclude files/directories with a pattern (#1554, @MichaelChirico). diff --git a/R/seq_linter.R b/R/seq_linter.R index 1fb0840712..c55e661f55 100644 --- a/R/seq_linter.R +++ b/R/seq_linter.R @@ -32,6 +32,11 @@ #' linters = seq_linter() #' ) #' +#' lint( +#' text = "unlist(lapply(x, seq_len))", +#' linters = seq_linter() +#' ) +#' #' # okay #' lint( #' text = "seq_along(x)", @@ -53,6 +58,11 @@ #' linters = seq_linter() #' ) #' +#' lint( +#' text = "sequence(x)", +#' linters = seq_linter() +#' ) +#' #' @evalRd rd_tags("seq_linter") #' @seealso [linters] for a complete list of linters available in lintr. #' @export @@ -80,6 +90,17 @@ seq_linter <- function() { parent::expr[expr/expr/SYMBOL_FUNCTION_CALL[text() = 'length']] " + map_funcs <- c("sapply", "lapply", "map") + seq_funcs <- xp_text_in_table(c("seq_len", "seq")) + # count(expr) = 3 because we only want seq() calls without extra arguments + sequence_xpath <- glue(" + parent::expr[ + count(expr) = 3 + and expr/SYMBOL[ {seq_funcs} ] + and preceding-sibling::expr/SYMBOL_FUNCTION_CALL[text() = 'unlist'] + ] + ") + ## The actual order of the nodes is document order ## In practice we need to handle length(x):1 get_fun <- function(expr, n) { @@ -138,6 +159,15 @@ seq_linter <- function() { type = "warning" ) - c(seq_lints, seq_len_lints) + xml_map_calls <- source_expression$xml_find_function_calls(map_funcs) + potential_sequence_calls <- xml_find_all(xml_map_calls, sequence_xpath) + sequence_lints <- xml_nodes_to_lints( + potential_sequence_calls, + source_expression, + "Use sequence() to generate a concatenated sequence of seq_len().", + type = "warning" + ) + + c(seq_lints, seq_len_lints, sequence_lints) }) } diff --git a/man/lintr-package.Rd b/man/lintr-package.Rd index 7e36edb2e7..323bd436fb 100644 --- a/man/lintr-package.Rd +++ b/man/lintr-package.Rd @@ -13,7 +13,7 @@ Supports on the fly checking of R code edited with Emacs, Vim, and Sublime Text. \code{\link[=lint]{lint()}}, \code{\link[=lint_package]{lint_package()}}, \code{\link[=lint_dir]{lint_dir()}}, \link{linters} } \author{ -\strong{Maintainer}: Michael Chirico \email{michaelchirico4@gmail.com} +\strong{Maintainer}: Michael Chirico \email{michaelchirico4@gmail.com} (\href{https://orcid.org/0000-0003-0787-087X}{ORCID}) Authors: \itemize{ @@ -23,6 +23,7 @@ Authors: \item Kun Ren \item Alexander Rosenstock (AshesITR) \item Indrajeet Patil \email{patilindrajeet.science@gmail.com} (\href{https://orcid.org/0000-0003-1995-6531}{ORCID}) (@patilindrajeets) + \item Hugo Gruson (\href{https://orcid.org/0000-0002-4094-1476}{ORCID}) } } diff --git a/man/seq_linter.Rd b/man/seq_linter.Rd index f9db3f7cb8..b3e1294dc9 100644 --- a/man/seq_linter.Rd +++ b/man/seq_linter.Rd @@ -40,6 +40,11 @@ lint( linters = seq_linter() ) +lint( + text = "unlist(lapply(x, seq_len))", + linters = seq_linter() +) + # okay lint( text = "seq_along(x)", @@ -61,6 +66,11 @@ lint( linters = seq_linter() ) +lint( + text = "sequence(x)", + linters = seq_linter() +) + } \seealso{ \link{linters} for a complete list of linters available in lintr. diff --git a/tests/testthat/test-seq_linter.R b/tests/testthat/test-seq_linter.R index 6bf5968e50..9424d394fc 100644 --- a/tests/testthat/test-seq_linter.R +++ b/tests/testthat/test-seq_linter.R @@ -123,6 +123,24 @@ test_that("reverse seq is ok", { ) }) +test_that("finds potential sequence() replacements", { + linter <- seq_linter() + lint_msg <- rex::rex("Use sequence()") + + expect_lint("unlist(lapply(x, seq_len))", lint_msg, linter) + + expect_lint("unlist(lapply(x, seq))", lint_msg, linter) + + # Even for prefixed purrr:: calls + expect_lint("unlist(purrr::map(x, seq_len))", lint_msg, linter) +}) + +test_that("sequence() is not recommended for complex seq() calls", { + linter <- seq_linter() + + expect_no_lint("unlist(lapply(x, seq, from = 2))", linter) +}) + test_that("Message vectorization works for multiple lints", { linter <- seq_linter() @@ -173,6 +191,18 @@ test_that("Message vectorization works for multiple lints", { ), linter ) + + expect_lint( + trim_some("{ + 1:NROW(x) + unlist(lapply(y, seq_len)) + }"), + list( + list(rex::rex("seq_len(NROW(...))", anything, "1:NROW(...)"), line_number = 2L), + list(rex::rex("sequence()"), line_number = 3L) + ), + linter + ) }) test_that("Message recommends rev() correctly", {