New unnecessary_lambda_linter (#1541)

Author: MichaelChirico

DESCRIPTION

@@ -144,6 +144,7 @@ Collate:
     'tree-utils.R'
     'undesirable_function_linter.R'
     'undesirable_operator_linter.R'
+    'unnecessary_lambda_linter.R'
     'unneeded_concatenation_linter.R'
     'unreachable_code_linter.R'
     'unused_import_linter.R'

NAMESPACE

@@ -111,6 +111,7 @@ export(trailing_blank_lines_linter)
 export(trailing_whitespace_linter)
 export(undesirable_function_linter)
 export(undesirable_operator_linter)
+export(unnecessary_lambda_linter)
 export(unneeded_concatenation_linter)
 export(unreachable_code_linter)
 export(unused_import_linter)

NEWS.md

@@ -24,6 +24,9 @@
 
 ### New linters
 
+* `unnecessary_lambda_linter()`: detect unnecessary lambdas (anonymous functions), e.g.
+  `lapply(x, function(xi) sum(xi))` can be `lapply(x, sum)` and `purrr::map(x, ~quantile(.x, 0.75, na.rm = TRUE))`
+  can be `purrr::map(x, quantile, 0.75, na.rm = TRUE)`. Naming `probs = 0.75` can further improve readability.
 * `redundant_equals_linter()` for redundant comparisons to `TRUE` or `FALSE` like `is_treatment == TRUE` (#1500, @MichaelChirico)
 
 # lintr 3.0.1

R/unnecessary_lambda_linter.R

@@ -0,0 +1,120 @@
+#' Block usage of anonymous functions in iteration functions when unnecessary
+#'
+#' Using an anonymous function in, e.g., [lapply()] is not always necessary,
+#'   e.g. `lapply(DF, sum)` is the same as `lapply(DF, function(x) sum(x))` and
+#'   the former is more readable.
+#'
+#' @evalRd rd_tags("unnecessary_lambda_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+unnecessary_lambda_linter <- function() {
+  # 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
+  #   Filter/Reduce (which don't accept ...), as well as functions
+  #   like sweep() (where check.margin comes between FUN and ...,
+  #   and thus would need to be supplied in order to replicate
+  #   positional arguments). Technically, these latter could
+  #   be part of the linter logic (e.g., detect if the anonymous
+  #   call is using positional or keyword arguments -- we can
+  #   throw a lint for sweep() lambdas where the following arguments
+  #   are all named) but for now it seems like overkill.
+  apply_funs <- xp_text_in_table(c(
+    "lapply", "sapply", "vapply", "apply",
+    "tapply", "rapply", "eapply", "dendrapply",
+    "mapply", "by", "outer",
+    "mclapply", "mcmapply", "parApply", "parCapply", "parLapply",
+    "parLapplyLB", "parRapply", "parSapply", "parSapplyLB", "pvec",
+    purrr_mappers
+  ))
+
+  # outline:
+  #   1. match one of the identified mappers
+  #   2. match an anonymous function that can be "symbol-ized"
+  #     a. it's a one-variable function [TODO(michaelchirico): is this necessary?]
+  #     b. the function is a single call
+  #     c. that call's _first_ argument is just the function argument (a SYMBOL)
+  #       - and it has to be passed positionally (not as a keyword)
+  #     d. the function argument doesn't appear elsewhere in the call
+  # TODO(#1567): This misses some common cases, e.g. function(x) { foo(x) }
+  default_fun_xpath <- glue::glue("
+  //SYMBOL_FUNCTION_CALL[ {apply_funs} ]
+  /parent::expr
+  /following-sibling::expr[
+    FUNCTION
+    and count(SYMBOL_FORMALS) = 1
+    and expr/OP-LEFT-PAREN/following-sibling::expr[1][not(preceding-sibling::*[2][self::SYMBOL_SUB])]/SYMBOL
+    and SYMBOL_FORMALS/text() = expr/OP-LEFT-PAREN/following-sibling::expr[1]/SYMBOL/text()
+    and not(SYMBOL_FORMALS/text() = expr/OP-LEFT-PAREN/following-sibling::expr[position() > 1]//SYMBOL/text())
+  ]")
+
+  # purrr-style inline formulas-as-functions, e.g. ~foo(.x)
+  # logic is basically the same as that above, except we need
+  #   1. a formula (OP-TILDE)
+  #   2. the lone argument marker `.x` or `.`
+  purrr_symbol <- "SYMBOL[text() = '.x' or text() = '.']"
+  purrr_fun_xpath <- glue::glue("
+  //SYMBOL_FUNCTION_CALL[ {xp_text_in_table(purrr_mappers)} ]
+  /parent::expr
+  /following-sibling::expr[
+    OP-TILDE
+    and expr[OP-LEFT-PAREN/following-sibling::expr[1][not(preceding-sibling::*[2][self::SYMBOL_SUB])]/{purrr_symbol}]
+    and not(expr/OP-LEFT-PAREN/following-sibling::expr[position() > 1]//{purrr_symbol})
+  ]")
+
+  # path to calling function symbol from the matched expressions
+  fun_xpath <- "./parent::expr/expr/SYMBOL_FUNCTION_CALL"
+  # path to the symbol of the simpler function that avoids a lambda
+  symbol_xpath <- "expr/expr[SYMBOL_FUNCTION_CALL]"
+
+  Linter(function(source_expression) {
+    if (!is_lint_level(source_expression, "expression")) {
+      return(list())
+    }
+
+    xml <- source_expression$xml_parsed_content
+
+    default_fun_expr <- xml2::xml_find_all(xml, default_fun_xpath)
+
+    # TODO(michaelchirico): further message customization is possible here,
+    #   e.g. don't always refer to 'lapply()' in the example, and customize to
+    #   whether arguments need to be subsumed in '...' or not. The trouble is in
+    #   keeping track of which argument the anonymous function is supplied (2nd
+    #   argument for many calls, but 3rd e.g. for apply())
+    default_call_fun <- xml2::xml_text(xml2::xml_find_first(default_fun_expr, fun_xpath))
+    default_symbol <- xml2::xml_text(xml2::xml_find_first(default_fun_expr, symbol_xpath))
+    default_fun_lints <- xml_nodes_to_lints(
+      default_fun_expr,
+      source_expression = source_expression,
+      lint_message = paste0(
+        "Pass ", default_symbol, " directly as a symbol to ", default_call_fun, "() ",
+        "instead of wrapping it in an unnecessary anonymous function. ",
+        "For example, prefer lapply(DF, sum) to lapply(DF, function(x) sum(x))."
+      ),
+      type = "warning"
+    )
+
+    purrr_fun_expr <- xml2::xml_find_all(xml, purrr_fun_xpath)
+
+    purrr_call_fun <- xml2::xml_text(xml2::xml_find_first(purrr_fun_expr, fun_xpath))
+    purrr_symbol <- xml2::xml_text(xml2::xml_find_first(purrr_fun_expr, symbol_xpath))
+    purrr_fun_lints <- xml_nodes_to_lints(
+      purrr_fun_expr,
+      source_expression = source_expression,
+      lint_message = paste0(
+        "Pass ", purrr_symbol, " directly as a symbol to ", purrr_call_fun, "() ",
+        "instead of wrapping it in an unnecessary anonymous function. ",
+        "For example, prefer purrr::map(DF, sum) to purrr::map(DF, ~sum(.x))."
+      ),
+      type = "warning"
+    )
+
+    c(default_fun_lints, purrr_fun_lints)
+  })
+}
+
+purrr_mappers <- c(
+  "map", "walk",
+  "map_raw", "map_lgl", "map_int", "map_dbl", "map_chr",
+  "map_df", "map_dfr", "map_dfc"
+)

inst/lintr/linters.csv

@@ -72,6 +72,7 @@ trailing_blank_lines_linter,style default
 trailing_whitespace_linter,style default
 undesirable_function_linter,style efficiency configurable robustness best_practices
 undesirable_operator_linter,style efficiency configurable robustness best_practices
+unnecessary_lambda_linter,best_practices efficiency readability
 unneeded_concatenation_linter,style readability efficiency configurable
 unreachable_code_linter,best_practices readability
 unused_import_linter,best_practices common_mistakes configurable executing

man/best_practices_linters.Rd

@@ -0,0 +1,83 @@
+test_that("unnecessary_lambda_linter skips allowed usages", {
+  linter <- unnecessary_lambda_linter()
+  expect_lint("lapply(DF, sum)", NULL, linter)
+  expect_lint("apply(M, 1, sum, na.rm = TRUE)", NULL, linter)
+
+  # the first argument may be ... or have a cumbersome name, so an anonymous
+  #   function may be preferable (e.g. this is often the case for grep() calls)
+  expect_lint("sapply(x, function(xi) foo(1, xi))", NULL, linter)
+
+  # if the argument is re-used, that's also a no-go
+  expect_lint("dendrapply(x, function(xi) foo(xi, xi))", NULL, linter)
+  # at any nesting level
+  expect_lint("parLapply(cl, x, function(xi) foo(xi, 2, bar(baz(xi))))", NULL, linter)
+
+  # multi-expression case
+  expect_lint("lapply(x, function(xi) { print(xi); xi^2 })", NULL, linter)
+  # multi-expression, multi-line cases
+  expect_lint(
+    trim_some("
+      lapply(x, function(xi) {
+        print(xi); xi^2
+      })
+    "),
+    NULL,
+    linter
+  )
+  expect_lint(
+    trim_some("
+      lapply(x, function(xi) {
+        print(xi)
+        xi^2
+      })
+    "),
+    NULL,
+    linter
+  )
+  # TODO(michaelchirico): I believe there's cases where it's impossible to avoid an anonymous function due to a
+  #   conflict where you have to pass FUN= to an inner *apply function but it gets interpreted as the outer *apply's FUN
+  #   argument... but it's escaping me now.
+})
+
+test_that("unnecessary_lambda_linter blocks simple disallowed usage", {
+  expect_lint(
+    "lapply(DF, function(x) sum(x))",
+    rex::rex("Pass sum directly as a symbol to lapply()"),
+    unnecessary_lambda_linter()
+  )
+
+  expect_lint(
+    "rapply(l, function(x) is.data.frame(x))",
+    rex::rex("Pass is.data.frame directly as a symbol to rapply()"),
+    unnecessary_lambda_linter()
+  )
+
+  expect_lint(
+    "eapply(env, function(x) sum(x, na.rm = TRUE))",
+    rex::rex("Pass sum directly as a symbol to eapply()"),
+    unnecessary_lambda_linter()
+  )
+})
+
+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())
+})
+
+test_that("purrr-style anonymous functions are also caught", {
+  # TODO(michaelchirico): this is just purrr::flatten(x). We should write another
+  #   linter to encourage that usage.
+  expect_lint("purrr::map(x, ~.x)", NULL, unnecessary_lambda_linter())
+  expect_lint("purrr::map_df(x, ~lm(y, .x))", NULL, unnecessary_lambda_linter())
+  expect_lint("map_dbl(x, ~foo(bar = .x))", NULL, unnecessary_lambda_linter())
+
+  expect_lint(
+    "purrr::map(x, ~foo(.x))",
+    rex::rex("Pass foo directly as a symbol to map()"),
+    unnecessary_lambda_linter()
+  )
+  expect_lint(
+    "purrr::map_int(x, ~foo(.x, y))",
+    rex::rex("Pass foo directly as a symbol to map_int()"),
+    unnecessary_lambda_linter()
+  )
+})