diff --git a/NEWS.md b/NEWS.md index 0fdee44e75..87aa39592f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -19,7 +19,7 @@ ## Changes to default linters -* New default linter `return_linter()` for the style guide rule that terminal returns should be left implicit (#1100, @MEO265). +* New default linter `return_linter()` for the style guide rule that terminal returns should be left implicit (#1100, #2354, and #2356, @MEO265 and @MichaelChirico). ## New and improved features @@ -27,7 +27,11 @@ * `library_call_linter()` is extended + to encourage all packages to be attached with `library(symbol)`, not `library("symbol", character.only = TRUE)` or "vectorized" approaches looping over package names (part of #884, @MichaelChirico). + to discourage many consecutive calls to `suppressMessages()` or `suppressPackageStartupMessages()` (part of #884, @MichaelChirico). -* `return_linter()` also has an argument `return_style` (`"implicit"` by default) which checks that all functions confirm to the specified return style of `"implicit"` or `"explicit"` (part of #884, @MichaelChirico, @AshesITR and @MEO265). +* `return_linter()` also has arguments for fine-tuning which functions get linted: + + `return_style` (`"implicit"` by default) which checks that all functions confirm to the specified return style of `"implicit"` or `"explicit"` (#2271 and part of #884, @MichaelChirico, @AshesITR and @MEO265). + + `allow_implicit_else` (default `TRUE`) which, when `FALSE`, checks that all terminal `if` statements are paired with a corresponding `else` statement (part of #884, @MichaelChirico). + + `return_functions` to customize which functions are equivalent to `return()` as "exit" clauses, e.g. `rlang::abort()` can be considered in addition to the default functions like `stop()` and `q()` from base (#2271 and part of #884, @MichaelChirico and @MEO265). + + `except` to customize which functions are ignored entirely (i.e., whether they have a return of the specified style is not checked; #2271 and part of #884, @MichaelChirico and @MEO265). Namespace hooks like `.onAttach()` and `.onLoad()` are always ignored. * `unnecessary_lambda_linter` is extended to encourage vectorized comparisons where possible, e.g. `sapply(x, sum) > 0` instead of `sapply(x, function(x) sum(x) > 0)` (part of #884, @MichaelChirico). Toggle this behavior with argument `allow_comparison`. * `backport_linter()` is slightly faster by moving expensive computations outside the linting function (#2339, #2348, @AshesITR and @MichaelChirico). * `Linter()` has a new argument `linter_level` (default `NA`). This is used by `lint()` to more efficiently check for expression levels than the idiom `if (!is_lint_level(...)) { return(list()) }` (#2351, @AshesITR). diff --git a/R/return_linter.R b/R/return_linter.R index 493d34d3d8..4e3731cbaf 100644 --- a/R/return_linter.R +++ b/R/return_linter.R @@ -6,6 +6,9 @@ #' the default, enforeces the Tidyverse guide recommendation to leave terminal #' returns implicit. `"explicit"` style requires that `return()` always be #' explicitly supplied. +#' @param allow_implicit_else Logical, default `TRUE`. If `FALSE`, functions with a terminal +#' `if` clause must always have an `else` clause, making the `NULL` alternative explicit +#' if necessary. #' @param return_functions Character vector of functions that are accepted as terminal calls #' when `return_style = "explicit"`. These are in addition to exit functions #' from base that are always allowed: [stop()], [q()], [quit()], [invokeRestart()], @@ -32,6 +35,13 @@ #' linters = return_linter(return_style = "explicit") #' ) #' +#' code <- "function(x) {\n if (x > 0) 2\n}" +#' writeLines(code) +#' lint( +#' text = code, +#' linters = return_linter(allow_implicit_else = FALSE) +#' ) +#' #' # okay #' code <- "function(x) {\n x + 1\n}" #' writeLines(code) @@ -47,6 +57,12 @@ #' linters = return_linter(return_style = "explicit") #' ) #' +#' code <- "function(x) {\n if (x > 0) 2 else NULL\n}" +#' writeLines(code) +#' lint( +#' text = code, +#' linters = return_linter(allow_implicit_else = FALSE) +#' ) #' #' @evalRd rd_tags("return_linter") #' @seealso @@ -55,13 +71,19 @@ #' @export return_linter <- function( return_style = c("implicit", "explicit"), + allow_implicit_else = TRUE, return_functions = NULL, except = NULL) { return_style <- match.arg(return_style) + if (!allow_implicit_else || return_style == "explicit") { + except_xpath <- glue("parent::expr[not( + preceding-sibling::expr/SYMBOL[{ xp_text_in_table(union(special_funs, except)) }] + )]") + } + if (return_style == "implicit") { body_xpath <- "(//FUNCTION | //OP-LAMBDA)/following-sibling::expr[1]" - # nolint next: object_usage. False positive from {codetools} says 'params' isn't used. params <- list( implicit = TRUE, type = "style", @@ -89,9 +111,7 @@ return_linter <- function( return_functions <- union(base_return_functions, return_functions) body_xpath <- glue(" - (//FUNCTION | //OP-LAMBDA)[parent::expr[not( - preceding-sibling::expr[SYMBOL[{ xp_text_in_table(except) }]] - )]] + (//FUNCTION | //OP-LAMBDA)[{ except_xpath }] /following-sibling::expr[OP-LEFT-BRACE and expr[last()]/@line1 != @line1] /expr[last()] ") @@ -106,6 +126,8 @@ return_linter <- function( ) } + params$allow_implicit_else <- allow_implicit_else + Linter(linter_level = "expression", function(source_expression) { xml <- source_expression$xml_parsed_content if (is.null(xml)) return(list()) @@ -113,8 +135,22 @@ return_linter <- function( body_expr <- xml_find_all(xml, body_xpath) params$source_expression <- source_expression + + if (params$implicit && !params$allow_implicit_else) { + # can't incorporate this into the body_xpath for implicit return style, + # since we still lint explicit returns for except= functions. + allow_implicit_else <- is.na(xml_find_first(body_expr, except_xpath)) + } else { + allow_implicit_else <- rep(params$allow_implicit_else, length(body_expr)) + } # nested_return_lints not "vectorized" due to xml_children() - lapply(body_expr, nested_return_lints, params) + Map( + function(expr, allow_implicit_else) { + params$allow_implicit_else <- allow_implicit_else + nested_return_lints(expr, params) + }, + body_expr, allow_implicit_else + ) }) } @@ -142,7 +178,17 @@ nested_return_lints <- function(expr, params) { nested_return_lints(child_expr[[tail(expr_idx, 1L)]], params) } else if (child_node[1L] == "IF") { expr_idx <- which(child_node %in% c("expr", "equal_assign", "expr_or_assign_or_help")) - lapply(child_expr[expr_idx[-1L]], nested_return_lints, params) + return_lints <- lapply(child_expr[expr_idx[-1L]], nested_return_lints, params) + if (params$allow_implicit_else || length(expr_idx) == 3L) { + return(return_lints) + } + implicit_else_lints <- list(xml_nodes_to_lints( + expr, + source_expression = params$source_expression, + lint_message = "All functions with terminal if statements must have a corresponding terminal else clause", + type = "warning" + )) + c(return_lints, implicit_else_lints) } else { xml_nodes_to_lints( xml_find_first(child_expr[[1L]], params$lint_xpath), diff --git a/man/return_linter.Rd b/man/return_linter.Rd index 4bcbd175e1..f6bdfaf202 100644 --- a/man/return_linter.Rd +++ b/man/return_linter.Rd @@ -6,6 +6,7 @@ \usage{ return_linter( return_style = c("implicit", "explicit"), + allow_implicit_else = TRUE, return_functions = NULL, except = NULL ) @@ -16,6 +17,10 @@ the default, enforeces the Tidyverse guide recommendation to leave terminal returns implicit. \code{"explicit"} style requires that \code{return()} always be explicitly supplied.} +\item{allow_implicit_else}{Logical, default \code{TRUE}. If \code{FALSE}, functions with a terminal +\code{if} clause must always have an \verb{else} clause, making the \code{NULL} alternative explicit +if necessary.} + \item{return_functions}{Character vector of functions that are accepted as terminal calls when \code{return_style = "explicit"}. These are in addition to exit functions from base that are always allowed: \code{\link[=stop]{stop()}}, \code{\link[=q]{q()}}, \code{\link[=quit]{quit()}}, \code{\link[=invokeRestart]{invokeRestart()}}, @@ -46,6 +51,13 @@ lint( linters = return_linter(return_style = "explicit") ) +code <- "function(x) {\n if (x > 0) 2\n}" +writeLines(code) +lint( + text = code, + linters = return_linter(allow_implicit_else = FALSE) +) + # okay code <- "function(x) {\n x + 1\n}" writeLines(code) @@ -61,6 +73,12 @@ lint( linters = return_linter(return_style = "explicit") ) +code <- "function(x) {\n if (x > 0) 2 else NULL\n}" +writeLines(code) +lint( + text = code, + linters = return_linter(allow_implicit_else = FALSE) +) } \seealso{ diff --git a/tests/testthat/test-return_linter.R b/tests/testthat/test-return_linter.R index 7d56665d97..f017b0e080 100644 --- a/tests/testthat/test-return_linter.R +++ b/tests/testthat/test-return_linter.R @@ -1251,3 +1251,273 @@ test_that("empty terminal '{' expression is handled correctly", { expect_lint(weird, list(implicit_msg, line_number = 5L), implicit_linter) expect_lint(weird, list(explicit_msg, line_number = 3L), explicit_linter) }) + +test_that("non-if returns are skipped under allow_implicit_else = FALSE", { + expect_lint( + trim_some(" + foo <- function(bar) { + bar + } + "), + NULL, + return_linter(allow_implicit_else = FALSE) + ) +}) + +test_that("if/else don't throw a lint under allow_implicit_else = FALSE", { + expect_lint( + trim_some(" + foo <- function(bar) { + if (TRUE) { + bar + } else { + NULL + } + } + "), + NULL, + return_linter(allow_implicit_else = FALSE) + ) +}) + +test_that("implicit else outside a function doesn't lint under allow_implicit_else = FALSE", { + expect_lint("if(TRUE) TRUE", NULL, return_linter(allow_implicit_else = FALSE)) +}) + +test_that("allow_implicit_else = FALSE identifies a simple implicit else", { + expect_lint( + trim_some(" + foo <- function(bar) { + if (TRUE) { + bar + } + } + "), + rex::rex("All functions with terminal if statements must have a corresponding terminal else clause"), + return_linter(allow_implicit_else = FALSE) + ) +}) + +test_that("allow_implicit_else = FALSE finds implicit else with nested if+else", { + lint_msg <- rex::rex("All functions with terminal if statements must have a corresponding terminal else clause") + + expect_lint( + trim_some(" + foo <- function() { + if (TRUE) { + if (TRUE) { + FALSE + } else { + TRUE + } + } + } + "), + lint_msg, + return_linter(allow_implicit_else = FALSE) + ) + + expect_lint( + trim_some(" + foo <- function() { + if (TRUE) { + if (TRUE) { + return(FALSE) + } else { + return(TRUE) + } + } + } + "), + lint_msg, + return_linter(return_style = "explicit", allow_implicit_else = FALSE) + ) +}) + +test_that("allow_implicit_else = FALSE works on anonymous/inline functions", { + expect_lint( + "lapply(rnorm(10), function(x) if (TRUE) x + 1)", + rex::rex("All functions with terminal if statements must"), + return_linter(allow_implicit_else = FALSE) + ) +}) + +test_that("side-effect functions like .onLoad ignore the lack of explicit else under allow_implicit_else = FALSE", { + expect_lint( + trim_some(" + .onAttach <- function(libname, pkgname) { + if (TRUE) foo() + } + "), + NULL, + return_linter(allow_implicit_else = FALSE) + ) + + expect_lint( + trim_some(" + .onAttach <- function(libname, pkgname) { + if (TRUE) return(foo()) + } + "), + NULL, + return_linter(return_style = "explicit", allow_implicit_else = FALSE) + ) +}) + +test_that("implicit else lint has the correct metadata", { + linter <- return_linter(return_style = "explicit", allow_implicit_else = FALSE) + lint_msg <- "All functions with terminal if statements" + + expect_lint( + trim_some(" + foo <- function(x, y = 3) { + if (x) { + return(x) + } + } + "), + list(lint_msg, line_number = 2L), + linter + ) + + expect_lint( + trim_some("{ + foo <- function(x, y = 3) { + if (x) { + return(x) + } + } + + bar <- function(x, y = 3) { + if (x) { + return(x) + } + } + + baz <- function(x, y = 3) { + if (x) return(x) + } + }"), + list( + list(lint_msg, line_number = 3L), + list(lint_msg, line_number = 9L), + list(lint_msg, line_number = 15L) + ), + linter + ) +}) + +test_that("Correct lints thrown when lacking explicit return and explicit else", { + linter <- return_linter(return_style = "explicit", allow_implicit_else = FALSE) + explicit_return_msg <- rex::rex("All functions must have an explicit return().") + implicit_else_msg <- rex::rex("All functions with terminal if statements") + + expect_lint( + trim_some(" + foo <- function(x, y = 3) { + if (x) { + x + } + } + "), + list( + list(implicit_else_msg, line_number = 2L), + list(explicit_return_msg, line_number = 3L) + ), + linter + ) + + expect_lint( + trim_some(" + function(x, y) { + if (x) { + 1 + } else if (y) { + 2 + } + } + "), + list( + list(explicit_return_msg, line_number = 3L), + list(implicit_else_msg, line_number = 4L), + list(explicit_return_msg, line_number = 5L) + ), + linter + ) +}) + +test_that("Mixing exempted functions doesn't miss lints", { + # in the current implementation, a local copy of 'params' is + # edited in a loop; this test ensures that behavior continues to be correct + expect_lint( + trim_some("{ + foo <- function() { + 1 + } + + bar <- function() { + if (TRUE) { + return(2) + } + } + + baz <- function() { + if (TRUE) { + 3 + } + } + }"), + list( + list("Use implicit return behavior", line_number = 8L), + list("All functions with terminal if statements", line_number = 13L) + ), + return_linter(allow_implicit_else = FALSE, except = "bar") + ) +}) + +test_that("= assignments are handled correctly", { + implicit_linter <- return_linter(allow_implicit_else = FALSE) + implicit_msg <- rex::rex("All functions with terminal if statements") + explicit_linter <- return_linter(return_style = "explicit") + explicit_msg <- rex::rex("All functions must have an explicit return().") + + expect_lint( + trim_some(" + .onLoad = function() { + 1 + } + "), + NULL, + explicit_linter + ) + + expect_lint( + trim_some(" + .onLoad = function() { + if (TRUE) 1 + } + "), + NULL, + implicit_linter + ) + + expect_lint( + trim_some(" + foo = function() { + 1 + } + "), + explicit_msg, + explicit_linter + ) + + expect_lint( + trim_some(" + foo = function() { + if (TRUE) 1 + } + "), + implicit_msg, + implicit_linter + ) +})