unnecessary_lambda_linter() finds explicit return cases (#2193)

* condense XPath

* whitespace

* add tests

* stray paren

* better naming

* simplify

* improved naming (again)

* condense test

* revert bad simplification

* fix bad simplification, add new tests

* less repetitive/slightly simpler

* no nested glue

* much simpler (but maybe too simple)

* new test for unhandled case

* remove repetitive expr[last()]

* handle return

* note about non-position-1 lambdas in docs

Author: MichaelChirico

NEWS.md

@@ -67,6 +67,7 @@
 * `unused_import_linter()` gains an argument `interpret_glue` (default `TRUE`) paralleling that in `object_usage_linter()` to toggle whether `glue::glue()` expressions should be inspected for exported object usage (#2042, @MichaelChirico).
 * `sort_linter()` only lints on `order()` of a single vector, excluding e.g. `x[order(x, y)]` and `x[order(y, x)]` (#2156, @MichaelChirico).
 * `redundant_ifelse_linter()` is aware of `dplyr::if_else()`'s `missing=` argument, so that `if_else(A, TRUE, FALSE, missing = FALSE)` doesn't lint, but `if_else(A, TRUE, FALSE, NA)` does (#1941, @MichaelChirico). Note that `dplyr::coalesce()` or `tidyr::replace_na()` may still be preferable.
+* `unnecessary_lambda_linter()` checks for cases using explicit returns, e.g. `lapply(x, \(xi) return(sum(xi)))` (#1567, @MichaelChirico).
 * `default_undesirable_functions` is updated to also include `Sys.unsetenv()` (#2192, @IndrajeetPatil).
 
 ### New linters

R/unnecessary_lambda_linter.R

@@ -4,6 +4,10 @@
 #'   e.g. `lapply(DF, sum)` is the same as `lapply(DF, function(x) sum(x))` and
 #'   the former is more readable.
 #'
+#' Cases like `lapply(x, \(xi) grep("ptn", xi))` are excluded because, though
+#'   the anonymous function _can_ be avoided, doing so is not always more
+#'   readable.
+#'
 #' @examples
 #' # will produce lints
 #' lint(
@@ -66,7 +70,7 @@ unnecessary_lambda_linter <- function() {
     /following-sibling::expr[(FUNCTION or OP-LAMBDA) and count(SYMBOL_FORMALS) = 1]
     /expr[last()][
       count(.//SYMBOL[self::* = preceding::SYMBOL_FORMALS[1]]) = 1
-      and count(.//SYMBOL_FUNCTION_CALL) = 1
+      and count(.//SYMBOL_FUNCTION_CALL[text() != 'return']) = 1
       and preceding-sibling::SYMBOL_FORMALS =
         //expr[
           position() = 2
@@ -95,7 +99,7 @@ unnecessary_lambda_linter <- function() {
   # 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 <- glue("(expr|expr[OP-LEFT-BRACE]/expr[1])/expr[SYMBOL_FUNCTION_CALL]")
+  symbol_xpath <- "expr[last()]//expr[SYMBOL_FUNCTION_CALL[text() != 'return']]"
 
   Linter(function(source_expression) {
     if (!is_lint_level(source_expression, "expression")) {

man/lint.Rd

@@ -6,6 +6,7 @@ test_that("unnecessary_lambda_linter skips allowed usages", {
   # 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)
+  expect_lint("sapply(x, function(xi) return(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)
@@ -41,10 +42,7 @@ test_that("unnecessary_lambda_linter skips allowed usages", {
   expect_lint("lapply(x, function(xi) tbl %in% xi)", NULL, linter)
   # would require multiple lapply() loops
   expect_lint("lapply(x, function(xi) foo(bar(xi)))", 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.
+  expect_lint("lapply(x, function(xi) return(foo(bar(xi))))", NULL, linter)
 })
 
 test_that("unnecessary_lambda_linter blocks simple disallowed usage", {
@@ -55,6 +53,11 @@ test_that("unnecessary_lambda_linter blocks simple disallowed usage", {
     rex::rex("Pass sum directly as a symbol to lapply()"),
     linter
   )
+  expect_lint(
+    "lapply(DF, function(x) return(sum(x)))",
+    rex::rex("Pass sum directly as a symbol to lapply()"),
+    linter
+  )
 
   expect_lint(
     "rapply(l, function(x) is.data.frame(x))",
@@ -67,10 +70,16 @@ test_that("unnecessary_lambda_linter blocks simple disallowed usage", {
     rex::rex("Pass sum directly as a symbol to eapply()"),
     linter
   )
+  expect_lint(
+    "eapply(env, function(x) return(sum(x, na.rm = TRUE)))",
+    rex::rex("Pass sum directly as a symbol to eapply()"),
+    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())
+  expect_lint("lapply(x, function(xi) return(data.frame(nm = xi)))", NULL, unnecessary_lambda_linter())
 })
 
 test_that("purrr-style anonymous functions are also caught", {
@@ -119,6 +128,32 @@ test_that("cases with braces are caught", {
     linter
   )
 
+  expect_lint(
+    "lapply(x, function(xi) { return(print(xi)) })",
+    lint_msg,
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      lapply(x, function(xi) {
+        print(xi)
+      })
+    "),
+    lint_msg,
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      lapply(x, function(xi) {
+        return(print(xi))
+      })
+    "),
+    lint_msg,
+    linter
+  )
+
   expect_lint(
     trim_some("
       lapply(x, function(xi) {