New pipe_return_linter

DESCRIPTION

@@ -150,6 +150,7 @@ Collate:
     'pipe_call_linter.R'
     'pipe_consistency_linter.R'
     'pipe_continuation_linter.R'
+    'pipe_return_linter.R'
     'print_linter.R'
     'quotes_linter.R'
     'redundant_equals_linter.R'

NAMESPACE

@@ -113,6 +113,7 @@ export(paste_linter)
 export(pipe_call_linter)
 export(pipe_consistency_linter)
 export(pipe_continuation_linter)
+export(pipe_return_linter)
 export(print_linter)
 export(quotes_linter)
 export(redundant_equals_linter)

NEWS.md

@@ -15,6 +15,7 @@
 * `comparison_negation_linter()` for discouraging negated comparisons when a direct negation is preferable, e.g. `!(x == y)` could be `x != y` (part of #884, @MichaelChirico).
 * `terminal_close_linter()` for discouraging using `close()` to end functions (part of #884, @MichaelChirico). Such usages are not robust to errors, where `close()` will not be run as intended. Put `close()` in an `on.exit()` hook, or use {withr} to manage connections with proper cleanup.
 * `print_linter()` for discouraging usage of `print()` on string literals like `print("Reached here")` or `print(paste("Found", nrow(DF), "rows."))` (#1894, @MichaelChirico).
+* `pipe_return_linter()` for discouraging usage of `return()` inside a {magrittr} pipeline (part of #884, @MichaelChirico).
 
 ### Lint accuracy fixes: removing false positives
 

R/pipe_return_linter.R

@@ -0,0 +1,40 @@
+#' Block usage of return() in magrittr pipelines
+#'
+#' [return()] inside a magrittr pipeline does not actually execute `return()`
+#'   like you'd expect: `\(x) { x %>% return(); FALSE }` will return `FALSE`!
+#'   It will technically work "as expected" if this is the final statement
+#'   in the function body, but such usage is misleading. Instead, assign
+#'   the pipe outcome to a variable and return that.
+#'
+#' @examples
+#' # will produce lints
+#' lint(
+#'   text = "function(x) x %>% return()",
+#'   linters = pipe_return_linter()
+#' )
+#'
+#' # okay
+#' code <- "function(x) {\n  y <- sum(x)\n  return(y)\n}"
+#' writeLines(code)
+#' lint(
+#'   text = code,
+#'   linters = pipe_return_linter()
+#' )
+#'
+#' @evalRd rd_tags("pipe_return_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+pipe_return_linter <- make_linter_from_xpath(
+  # NB: Native pipe disallows this at the parser level, so there's no need
+  #   to lint in valid R code.
+  xpath = "
+  //SPECIAL[text() = '%>%']
+    /following-sibling::expr[expr/SYMBOL_FUNCTION_CALL[text() = 'return']]
+  ",
+  lint_message = paste(
+    "Using return() as the final step of a magrittr pipeline",
+    "is an anti-pattern. Instead, assign the output of the pipeline to",
+    "a well-named object and return that.",
+    "See go/r-primer#how-magrittr-pipes-work."
+  )
+)

inst/lintr/linters.csv

@@ -70,6 +70,7 @@ paste_linter,best_practices consistency configurable
 pipe_call_linter,style readability
 pipe_consistency_linter,style readability configurable
 pipe_continuation_linter,style readability default
+pipe_return_linter,best_practices common_mistakes
 print_linter,best_practices consistency
 quotes_linter,style consistency readability default configurable
 redundant_equals_linter,best_practices readability efficiency common_mistakes

tests/testthat/test-pipe_return_linter.R

@@ -0,0 +1,47 @@
+test_that("pipe_return_linter skips allowed usages", {
+  linter <- pipe_return_linter()
+
+  normal_pipe_lines <- c(
+    "x %>%",
+    "  filter(str > 5) %>%",
+    "  summarize(str = sum(str))"
+  )
+  expect_lint(normal_pipe_lines, NULL, linter)
+
+  normal_function_lines <- c(
+    "pipeline <- function(x) {",
+    "  out <- x %>%",
+    "    filter(str > 5) %>%",
+    "    summarize(str = sum(str))",
+    "  return(out)",
+    "}"
+  )
+  expect_lint(normal_function_lines, NULL, linter)
+
+  nested_return_lines <- c(
+    "pipeline <- function(x) {",
+    "  x_squared <- x %>%",
+    "    sapply(function(xi) {",
+    "      return(xi ** 2)",
+    "    })",
+    "  return(x_squared)",
+    "}"
+  )
+  expect_lint(nested_return_lines, NULL, linter)
+})
+
+test_that("pipe_return_linter blocks simple disallowed usages", {
+  lines <- c(
+    "pipeline <- function(x) {",
+    "  out <- x %>%",
+    "    filter(str > 5) %>%",
+    "    summarize(str = sum(str)) %>%",
+    "    return()",
+    "}"
+  )
+  expect_lint(
+    lines,
+    rex::rex("Using return() as the final step of a magrittr pipeline"),
+    pipe_return_linter()
+  )
+})