From e5ee53ea72dcb614512af628023c75b9aa75d8a0 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 17 Nov 2023 19:08:42 +0000 Subject: [PATCH 1/4] New pipe_return_linter --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 1 + R/pipe_return_linter.R | 40 ++++++++++++++++++++ inst/lintr/linters.csv | 1 + man/best_practices_linters.Rd | 1 + man/common_mistakes_linters.Rd | 1 + man/linters.Rd | 5 ++- man/pipe_return_linter.Rd | 37 +++++++++++++++++++ tests/testthat/test-pipe_return_linter.R | 47 ++++++++++++++++++++++++ 10 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 R/pipe_return_linter.R create mode 100644 man/pipe_return_linter.Rd create mode 100644 tests/testthat/test-pipe_return_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 7e0f6250d8..7e49d4c560 100644 --- a/DESCRIPTION +++ b/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' diff --git a/NAMESPACE b/NAMESPACE index 8be33824dd..9bd2cb19f4 100644 --- a/NAMESPACE +++ b/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) diff --git a/NEWS.md b/NEWS.md index 477b8e61ce..16eb20714e 100644 --- a/NEWS.md +++ b/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 diff --git a/R/pipe_return_linter.R b/R/pipe_return_linter.R new file mode 100644 index 0000000000..ba928af561 --- /dev/null +++ b/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." + ) +) diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 334f6167d2..5c23553573 100644 --- a/inst/lintr/linters.csv +++ b/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 diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index 4e8cc2eadd..12c94cf5e7 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -47,6 +47,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{nonportable_path_linter}}} \item{\code{\link{outer_negation_linter}}} \item{\code{\link{paste_linter}}} +\item{\code{\link{pipe_return_linter}}} \item{\code{\link{print_linter}}} \item{\code{\link{redundant_equals_linter}}} \item{\code{\link{redundant_ifelse_linter}}} diff --git a/man/common_mistakes_linters.Rd b/man/common_mistakes_linters.Rd index 73767f1f5c..6843f7c1a7 100644 --- a/man/common_mistakes_linters.Rd +++ b/man/common_mistakes_linters.Rd @@ -17,6 +17,7 @@ The following linters are tagged with 'common_mistakes': \item{\code{\link{length_test_linter}}} \item{\code{\link{missing_argument_linter}}} \item{\code{\link{missing_package_linter}}} +\item{\code{\link{pipe_return_linter}}} \item{\code{\link{redundant_equals_linter}}} \item{\code{\link{sprintf_linter}}} \item{\code{\link{unused_import_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index 91d035bb03..90cfe90a2f 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,8 +17,8 @@ see also \code{\link[=available_tags]{available_tags()}}. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (56 linters)} -\item{\link[=common_mistakes_linters]{common_mistakes} (8 linters)} +\item{\link[=best_practices_linters]{best_practices} (57 linters)} +\item{\link[=common_mistakes_linters]{common_mistakes} (9 linters)} \item{\link[=configurable_linters]{configurable} (34 linters)} \item{\link[=consistency_linters]{consistency} (24 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} @@ -102,6 +102,7 @@ The following linters exist: \item{\code{\link{pipe_call_linter}} (tags: readability, style)} \item{\code{\link{pipe_consistency_linter}} (tags: configurable, readability, style)} \item{\code{\link{pipe_continuation_linter}} (tags: default, readability, style)} +\item{\code{\link{pipe_return_linter}} (tags: best_practices, common_mistakes)} \item{\code{\link{print_linter}} (tags: best_practices, consistency)} \item{\code{\link{quotes_linter}} (tags: configurable, consistency, default, readability, style)} \item{\code{\link{redundant_equals_linter}} (tags: best_practices, common_mistakes, efficiency, readability)} diff --git a/man/pipe_return_linter.Rd b/man/pipe_return_linter.Rd new file mode 100644 index 0000000000..8527e8ab93 --- /dev/null +++ b/man/pipe_return_linter.Rd @@ -0,0 +1,37 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/pipe_return_linter.R +\name{pipe_return_linter} +\alias{pipe_return_linter} +\title{Block usage of return() in magrittr pipelines} +\usage{ +pipe_return_linter() +} +\description{ +\code{\link[=return]{return()}} inside a magrittr pipeline does not actually execute \code{return()} +like you'd expect: \verb{\\(x) \{ x \%>\% return(); FALSE \}} will return \code{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() +) + +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=common_mistakes_linters]{common_mistakes} +} diff --git a/tests/testthat/test-pipe_return_linter.R b/tests/testthat/test-pipe_return_linter.R new file mode 100644 index 0000000000..646c3478fc --- /dev/null +++ b/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() + ) +}) From fb466c3754d76954214cbc8f7c674c93c979f90b Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 18 Nov 2023 17:35:45 -0800 Subject: [PATCH 2/4] remove vestigial --- R/pipe_return_linter.R | 1 - 1 file changed, 1 deletion(-) diff --git a/R/pipe_return_linter.R b/R/pipe_return_linter.R index ba928af561..e45321b736 100644 --- a/R/pipe_return_linter.R +++ b/R/pipe_return_linter.R @@ -35,6 +35,5 @@ pipe_return_linter <- make_linter_from_xpath( "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." ) ) From 445800aeec78ff7f60aa4bbfc17f99b041e7bdd7 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 18 Nov 2023 17:35:57 -0800 Subject: [PATCH 3/4] use trim_some() in test --- tests/testthat/test-pipe_return_linter.R | 60 ++++++++++++------------ 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/tests/testthat/test-pipe_return_linter.R b/tests/testthat/test-pipe_return_linter.R index 646c3478fc..0c395d48b1 100644 --- a/tests/testthat/test-pipe_return_linter.R +++ b/tests/testthat/test-pipe_return_linter.R @@ -1,44 +1,44 @@ test_that("pipe_return_linter skips allowed usages", { linter <- pipe_return_linter() - normal_pipe_lines <- c( - "x %>%", - " filter(str > 5) %>%", - " summarize(str = sum(str))" - ) + normal_pipe_lines <- trim_some(" + 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)", - "}" - ) + normal_function_lines <- trim_some(" + 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)", - "}" - ) + nested_return_lines <- trim_some(" + 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()", - "}" - ) + lines <- trim_some(" + 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"), From e7630e430358383ee00894dee74f2790ad220b7d Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 18 Nov 2023 17:53:47 -0800 Subject: [PATCH 4/4] trailing ',' --- R/pipe_return_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/pipe_return_linter.R b/R/pipe_return_linter.R index e45321b736..fd73da8ae1 100644 --- a/R/pipe_return_linter.R +++ b/R/pipe_return_linter.R @@ -34,6 +34,6 @@ pipe_return_linter <- make_linter_from_xpath( 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.", + "a well-named object and return that." ) )