diff --git a/NEWS.md b/NEWS.md index da97623782..dafdc19425 100644 --- a/NEWS.md +++ b/NEWS.md @@ -21,6 +21,7 @@ * `library_call_linter()` is extended + to encourage all packages to be attached with `library(symbol)`, not `library("symbol", character.only = TRUE)` or "vectorized" approaches looping over package names (part of #884, @MichaelChirico). + to discourage many consecutive calls to `suppressMessages()` or `suppressPackageStartupMessages()` (part of #884, @MichaelChirico). +* `unnecessary_lambda_linter` is extended to encourage vectorized comparisons where possible, e.g. `sapply(x, sum) > 0` instead of `sapply(x, function(x) sum(x) > 0)` (part of #884, @MichaelChirico). Toggle this behavior with argument `allow_comparison`. ### New linters diff --git a/R/unnecessary_lambda_linter.R b/R/unnecessary_lambda_linter.R index 4b52644f96..99220c6f66 100644 --- a/R/unnecessary_lambda_linter.R +++ b/R/unnecessary_lambda_linter.R @@ -8,6 +8,10 @@ #' the anonymous function _can_ be avoided, doing so is not always more #' readable. #' +#' @param allow_comparison Logical, default `FALSE`. If `TRUE`, lambdas like +#' `function(x) foo(x) == 2`, where `foo` can be extracted to the "mapping" +#' function and `==` vectorized instead of called repeatedly, are linted. +#' #' @examples #' # will produce lints #' lint( @@ -15,6 +19,16 @@ #' linters = unnecessary_lambda_linter() #' ) #' +#' lint( +#' text = "sapply(x, function(xi) xi == 2)", +#' linters = unnecessary_lambda_linter() +#' ) +#' +#' lint( +#' text = "sapply(x, function(xi) sum(xi) > 0)", +#' linters = unnecessary_lambda_linter() +#' ) +#' #' # okay #' lint( #' text = "lapply(list(1:3, 2:4), sum)", @@ -31,10 +45,30 @@ #' linters = unnecessary_lambda_linter() #' ) #' +#' lint( +#' text = "sapply(x, function(xi) xi == 2)", +#' linters = unnecessary_lambda_linter(allow_comparison = TRUE) +#' ) +#' +#' lint( +#' text = "sapply(x, function(xi) sum(xi) > 0)", +#' linters = unnecessary_lambda_linter(allow_comparison = TRUE) +#' ) +#' +#' lint( +#' text = "sapply(x, function(xi) sum(abs(xi)) > 10)", +#' linters = unnecessary_lambda_linter() +#' ) +#' +#' lint( +#' text = "sapply(x, sum) > 0", +#' linters = unnecessary_lambda_linter() +#' ) +#' #' @evalRd rd_tags("unnecessary_lambda_linter") #' @seealso [linters] for a complete list of linters available in lintr. #' @export -unnecessary_lambda_linter <- function() { +unnecessary_lambda_linter <- function(allow_comparison = FALSE) { # include any base function like those where FUN is an argument # and ... follows positionally directly afterwards (with ... # being passed on to FUN). That excludes functions like @@ -55,6 +89,27 @@ unnecessary_lambda_linter <- function() { purrr_mappers )) + # OP-PLUS: condition for complex literal, e.g. 0+2i. + # NB: this includes 0+3 and TRUE+FALSE, which are also fine. + inner_comparison_xpath <- glue(" + //SYMBOL_FUNCTION_CALL[text() = 'sapply' or text() = 'vapply'] + /parent::expr + /parent::expr + /expr[FUNCTION] + /expr[ + ({ xp_or(infix_metadata$xml_tag[infix_metadata$comparator]) }) + and expr[ + expr/SYMBOL_FUNCTION_CALL + and expr/SYMBOL + ] + and expr[ + NUM_CONST + or STR_CONST + or (OP-PLUS and count(expr/NUM_CONST) = 2) + ] + ] + ") + # outline: # 1. match one of the identified mappers # 2. match an anonymous function that can be "symbol-ized" @@ -131,6 +186,27 @@ unnecessary_lambda_linter <- function() { type = "warning" ) + inner_comparison_lints <- NULL + if (!allow_comparison) { + inner_comparison_expr <- xml_find_all(xml, inner_comparison_xpath) + + mapper <- xp_call_name(xml_find_first(inner_comparison_expr, "parent::expr/parent::expr")) + if (length(mapper) > 0L) fun_value <- if (mapper == "sapply") "" else ", FUN.VALUE = " + + inner_comparison_lints <- xml_nodes_to_lints( + inner_comparison_expr, + source_expression = source_expression, + lint_message = sprintf( + paste( + "Compare to a constant after calling %1$s() to get the full benefits of vectorization.", + "Prefer %1$s(x, foo%2$s) == 2 over %1$s(x, function(xi) foo(xi) == 2, logical(1L))." + ), + mapper, fun_value + ), + type = "warning" + ) + } + purrr_fun_expr <- xml_find_all(xml, purrr_fun_xpath) purrr_call_fun <- xml_text(xml_find_first(purrr_fun_expr, fun_xpath)) @@ -146,6 +222,6 @@ unnecessary_lambda_linter <- function() { type = "warning" ) - c(default_fun_lints, purrr_fun_lints) + c(default_fun_lints, inner_comparison_lints, purrr_fun_lints) }) } diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 420fca4e3d..4562a57901 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -109,7 +109,7 @@ trailing_whitespace_linter,style default configurable undesirable_function_linter,style efficiency configurable robustness best_practices undesirable_operator_linter,style efficiency configurable robustness best_practices unnecessary_concatenation_linter,style readability efficiency configurable -unnecessary_lambda_linter,best_practices efficiency readability +unnecessary_lambda_linter,best_practices efficiency readability configurable unnecessary_nested_if_linter,readability best_practices unnecessary_placeholder_linter,readability best_practices unneeded_concatenation_linter,style readability efficiency configurable deprecated diff --git a/man/configurable_linters.Rd b/man/configurable_linters.Rd index f233c62ebc..cfd15b6b35 100644 --- a/man/configurable_linters.Rd +++ b/man/configurable_linters.Rd @@ -48,6 +48,7 @@ The following linters are tagged with 'configurable': \item{\code{\link{undesirable_function_linter}}} \item{\code{\link{undesirable_operator_linter}}} \item{\code{\link{unnecessary_concatenation_linter}}} +\item{\code{\link{unnecessary_lambda_linter}}} \item{\code{\link{unused_import_linter}}} } } diff --git a/man/linters.Rd b/man/linters.Rd index a4daafe26c..ff6b7bbd83 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -19,7 +19,7 @@ The following tags exist: \itemize{ \item{\link[=best_practices_linters]{best_practices} (63 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (10 linters)} -\item{\link[=configurable_linters]{configurable} (37 linters)} +\item{\link[=configurable_linters]{configurable} (38 linters)} \item{\link[=consistency_linters]{consistency} (31 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (25 linters)} @@ -140,7 +140,7 @@ The following linters exist: \item{\code{\link{undesirable_function_linter}} (tags: best_practices, configurable, efficiency, robustness, style)} \item{\code{\link{undesirable_operator_linter}} (tags: best_practices, configurable, efficiency, robustness, style)} \item{\code{\link{unnecessary_concatenation_linter}} (tags: configurable, efficiency, readability, style)} -\item{\code{\link{unnecessary_lambda_linter}} (tags: best_practices, efficiency, readability)} +\item{\code{\link{unnecessary_lambda_linter}} (tags: best_practices, configurable, efficiency, readability)} \item{\code{\link{unnecessary_nested_if_linter}} (tags: best_practices, readability)} \item{\code{\link{unnecessary_placeholder_linter}} (tags: best_practices, readability)} \item{\code{\link{unreachable_code_linter}} (tags: best_practices, readability)} diff --git a/man/unnecessary_lambda_linter.Rd b/man/unnecessary_lambda_linter.Rd index 12a44eab32..d151e7e6a9 100644 --- a/man/unnecessary_lambda_linter.Rd +++ b/man/unnecessary_lambda_linter.Rd @@ -4,7 +4,12 @@ \alias{unnecessary_lambda_linter} \title{Block usage of anonymous functions in iteration functions when unnecessary} \usage{ -unnecessary_lambda_linter() +unnecessary_lambda_linter(allow_comparison = FALSE) +} +\arguments{ +\item{allow_comparison}{Logical, default \code{FALSE}. If \code{TRUE}, lambdas like +\code{function(x) foo(x) == 2}, where \code{foo} can be extracted to the "mapping" +function and \code{==} vectorized instead of called repeatedly, are linted.} } \description{ Using an anonymous function in, e.g., \code{\link[=lapply]{lapply()}} is not always necessary, @@ -23,6 +28,16 @@ lint( linters = unnecessary_lambda_linter() ) +lint( + text = "sapply(x, function(xi) xi == 2)", + linters = unnecessary_lambda_linter() +) + +lint( + text = "sapply(x, function(xi) sum(xi) > 0)", + linters = unnecessary_lambda_linter() +) + # okay lint( text = "lapply(list(1:3, 2:4), sum)", @@ -39,10 +54,30 @@ lint( linters = unnecessary_lambda_linter() ) +lint( + text = "sapply(x, function(xi) xi == 2)", + linters = unnecessary_lambda_linter(allow_comparison = TRUE) +) + +lint( + text = "sapply(x, function(xi) sum(xi) > 0)", + linters = unnecessary_lambda_linter(allow_comparison = TRUE) +) + +lint( + text = "sapply(x, function(xi) sum(abs(xi)) > 10)", + linters = unnecessary_lambda_linter() +) + +lint( + text = "sapply(x, sum) > 0", + linters = unnecessary_lambda_linter() +) + } \seealso{ \link{linters} for a complete list of linters available in lintr. } \section{Tags}{ -\link[=best_practices_linters]{best_practices}, \link[=efficiency_linters]{efficiency}, \link[=readability_linters]{readability} +\link[=best_practices_linters]{best_practices}, \link[=configurable_linters]{configurable}, \link[=efficiency_linters]{efficiency}, \link[=readability_linters]{readability} } diff --git a/tests/testthat/test-unnecessary_lambda_linter.R b/tests/testthat/test-unnecessary_lambda_linter.R index 15d57bd192..4036e6839d 100644 --- a/tests/testthat/test-unnecessary_lambda_linter.R +++ b/tests/testthat/test-unnecessary_lambda_linter.R @@ -77,6 +77,20 @@ test_that("unnecessary_lambda_linter skips allowed usages", { expect_lint("lapply(l, function(x = 1) 'a' %in% names(x))", NULL, linter) }) +test_that("unnecessary_lambda_linter skips allowed inner comparisons", { + linter <- unnecessary_lambda_linter() + + # lapply returns a list, so not the same, though as.list is probably + # a better choice + expect_lint("lapply(x, function(xi) foo(xi) == 2)", NULL, linter) + + # this _may_ return a matrix, though outer is probably a better choice if so + expect_lint("sapply(x, function(xi) foo(xi) == y)", NULL, linter) + + # only lint "plain" calls that can be replaced by eliminating the lambda + expect_lint("sapply(x, function(xi) sum(abs(xi)) == 0)", NULL, linter) +}) + test_that("unnecessary_lambda_linter blocks simple disallowed usage", { linter <- unnecessary_lambda_linter() @@ -109,6 +123,42 @@ test_that("unnecessary_lambda_linter blocks simple disallowed usage", { ) }) +test_that("unnecessary_lambda_linter blocks simple disallowed usages", { + linter <- unnecessary_lambda_linter() + linter_allow <- unnecessary_lambda_linter(allow_comparison = TRUE) + lint_msg <- rex::rex("Compare to a constant after calling sapply() to get", anything, "sapply(x, foo)") + + expect_lint("sapply(x, function(xi) foo(xi) == 2)", lint_msg, linter) + expect_lint("sapply(x, function(xi) foo(xi) == 'a')", lint_msg, linter) + expect_lint("sapply(x, function(xi) foo(xi) == 1 + 2i)", lint_msg, linter) + + expect_lint("sapply(x, function(xi) foo(xi) == 2)", NULL, linter_allow) + expect_lint("sapply(x, function(xi) foo(xi) == 'a')", NULL, linter_allow) + expect_lint("sapply(x, function(xi) foo(xi) == 1 + 2i)", NULL, linter_allow) + + # vapply counts as well + # NB: we ignore the FUN.VALUE argument, for now + expect_lint( + "vapply(x, function(xi) foo(xi) == 2, logical(1L))", + rex::rex("Compare to a constant after calling vapply()", anything, "vapply(x, foo, FUN.VALUE = )"), + linter + ) +}) + +test_that("unnecessary_lambda_linter blocks other comparators as well", { + linter <- unnecessary_lambda_linter() + linter_allow <- unnecessary_lambda_linter(allow_comparison = TRUE) + lint_msg <- rex::rex("Compare to a constant after calling sapply() to get") + + expect_lint("sapply(x, function(xi) foo(xi) >= 2)", lint_msg, linter) + expect_lint("sapply(x, function(xi) foo(xi) != 'a')", lint_msg, linter) + expect_lint("sapply(x, function(xi) foo(xi) < 1 + 2i)", lint_msg, linter) + + expect_lint("sapply(x, function(xi) foo(xi) >= 2)", NULL, linter_allow) + expect_lint("sapply(x, function(xi) foo(xi) != 'a')", NULL, linter_allow) + expect_lint("sapply(x, function(xi) foo(xi) < 1 + 2i)", NULL, linter_allow) +}) + test_that("unnecessary_lambda_linter doesn't apply to keyword args", { expect_lint("lapply(x, function(xi) data.frame(nm = xi))", NULL, unnecessary_lambda_linter()) expect_lint("lapply(x, function(xi) return(data.frame(nm = xi)))", NULL, unnecessary_lambda_linter())