New function_return_linter() (#1569)

Author: MichaelChirico

DESCRIPTION

@@ -94,6 +94,7 @@ Collate:
     'fixed_regex_linter.R'
     'function_argument_linter.R'
     'function_left_parentheses_linter.R'
+    'function_return_linter.R'
     'get_source_expressions.R'
     'ids_with_token.R'
     'ifelse_censor_linter.R'

NAMESPACE

@@ -58,6 +58,7 @@ export(extraction_operator_linter)
 export(fixed_regex_linter)
 export(function_argument_linter)
 export(function_left_parentheses_linter)
+export(function_return_linter)
 export(get_r_string)
 export(get_source_expressions)
 export(ids_with_token)

NEWS.md

@@ -29,6 +29,9 @@
   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)
 
+* `function_return_linter()` for handling issues in function `return()` statements. Currently handles assignments within the `return()`
+  clause, e.g. `return(x <- foo())` (@MichaelChirico)
+
 # lintr 3.0.1
 
 * Skip multi-byte tests in non UTF-8 locales (#1504)

R/function_return_linter.R

@@ -0,0 +1,32 @@
+#' Lint common mistakes/style issues cropping up from return statements
+#'
+#' `return(x <- ...)` is either distracting (because `x` is ignored), or
+#'   confusing (because assigning to `x` has some side effect that is muddled
+#'   by the dual-purpose expression).
+#'
+#' @evalRd rd_tags("function_return_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+function_return_linter <- function() {
+  xpath <- "
+  //SYMBOL_FUNCTION_CALL[text() = 'return']
+  /parent::expr/parent::expr/expr[LEFT_ASSIGN or RIGHT_ASSIGN]
+  "
+
+  Linter(function(source_expression) {
+    if (!is_lint_level(source_expression, "expression")) {
+      return(list())
+    }
+
+    xml <- source_expression$xml_parsed_content
+
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+
+    xml_nodes_to_lints(
+      bad_expr,
+      source_expression = source_expression,
+      lint_message = "Move the assignment outside of the return() clause, or skip assignment altogether.",
+      type = "warning"
+    )
+  })
+}

inst/lintr/linters.csv

@@ -29,6 +29,7 @@ extraction_operator_linter,style best_practices
 fixed_regex_linter,best_practices readability efficiency
 function_argument_linter,style consistency best_practices
 function_left_parentheses_linter,style readability default
+function_return_linter,readability best_practices
 ifelse_censor_linter,best_practices efficiency
 implicit_integer_linter,style consistency best_practices
 infix_spaces_linter,style readability default

man/best_practices_linters.Rd

@@ -0,0 +1,68 @@
+test_that("function_return_linter skips allowed usages", {
+  lines_simple <- trim_some("
+    foo <- function(x) {
+      x <- x + 1
+      return(x)
+    }
+  ")
+  expect_lint(lines_simple, NULL, function_return_linter())
+
+  # arguably an expression as complicated as this should also be assigned,
+  #   but for now that's out of the scope of this linter
+  lines_subassignment <- trim_some("
+    foo <- function(x) {
+      return(x[, {
+        col <- col + 1
+        .(grp, col)
+      }])
+    }
+  ")
+  expect_lint(lines_subassignment, NULL, function_return_linter())
+})
+
+test_that("function_return_linter blocks simple disallowed usages", {
+  linter <- function_return_linter()
+  lint_msg <- rex::rex("Move the assignment outside of the return() clause")
+
+  expect_lint(
+    trim_some("
+      foo <- function(x) {
+        return(x <- x + 1)
+      }
+    "),
+    lint_msg,
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      foo <- function(x) {
+        return(x <<- x + 1)
+      }
+    "),
+    lint_msg,
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      foo <- function(x) {
+        return(x + 1 -> x)
+      }
+    "),
+    lint_msg,
+    linter
+  )
+
+  side_effect_lines <-
+  expect_lint(
+    trim_some("
+      e <- new.env()
+      foo <- function(x) {
+        return(e$val <- x + 1)
+      }
+    "),
+    lint_msg,
+    linter
+  )
+})