diff --git a/.dev/roxygen_test.R b/.dev/roxygen_test.R new file mode 100644 index 000000000..c595dbafd --- /dev/null +++ b/.dev/roxygen_test.R @@ -0,0 +1,54 @@ +# Test to ensure roxygenize() has been run on the current PR +library(tools) +library(roxygen2) + +old_dir <- file.path(tempdir(), "man") +if (dir.exists(old_dir)) unlink(old_dir, recursive = TRUE) +file.copy("man", tempdir(), recursive = TRUE) +old_files <- list.files(old_dir, pattern = "\\.Rd$") +new_dir <- "man" +.Last <- function() unlink(old_dir, recursive = TRUE) + +# Rd2txt() prints to its out= argument, so we'd have to compare file contents; +# plain parse_Rd() keeps srcref info that encodes the file path, which as.character() strips. +normalize_rd <- function(rd_file) as.character(parse_Rd(rd_file)) + +rd_equal <- function(f1, f2) isTRUE(all.equal(normalize_rd(f1), normalize_rd(f2))) + +check_roxygenize_idempotent <- function(LOCALE) { + Sys.setlocale("LC_COLLATE", LOCALE) + roxygenize() + + new_files <- list.files(new_dir, pattern = "\\.Rd$") + + old_not_new <- setdiff(old_files, new_files) + if (length(old_not_new) > 0L) { + stop("Found saved .Rd files gone from a fresh run of roxygenize(): ", toString(old_not_new)) + } + + new_not_old <- setdiff(new_files, old_files) + if (length(new_not_old) > 0L) { + stop("Found new .Rd files from a fresh run of roxygenize(): ", toString(new_not_old)) + } + + for (file in new_files) { + old_file <- file.path(old_dir, file) + new_file <- file.path(new_dir, file) + if (rd_equal(old_file, new_file)) { + next + } + cat(sprintf("roxygenize() output differs from saved output for %s.\n", file)) + cat("Here's the 'diff' comparison of the two files:\n") + cat(" [---]: saved output in man/ directory\n") + cat(" [+++]: roxygenize() output of R/ sources\n") + system2("diff", c("--unified", old_file, new_file)) + stop("Failed in LOCALE=", LOCALE, ".", call. = FALSE) + } +} + +# Run the check in a few locales to ensure there's no idempotency issues w.r.t. sorting, too +for (LOCALE in c("C", "en_US", "hu_HU", "ja_JP")) { + check_roxygenize_idempotent(LOCALE) +} + +unlink(old_dir, recursive = TRUE) diff --git a/.github/workflows/ensure-metadata-tests.yaml b/.github/workflows/repo-meta-tests.yaml similarity index 70% rename from .github/workflows/ensure-metadata-tests.yaml rename to .github/workflows/repo-meta-tests.yaml index 2412de674..d5a6402eb 100644 --- a/.github/workflows/ensure-metadata-tests.yaml +++ b/.github/workflows/repo-meta-tests.yaml @@ -1,14 +1,14 @@ -# Ensure lint metadata is tested +# Various repo-level tests for code quality on: push: branches: [main] pull_request: branches: [main] -name: ensure-metadata-tests +name: repo-meta-tests jobs: - ensure-metadata-tests: + repo-meta-tests: runs-on: ubuntu-latest env: GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }} @@ -28,3 +28,8 @@ jobs: options(crayon.enabled = TRUE) callr::rscript(".dev/lint_metadata_test.R") shell: Rscript {0} + + - name: Ensure roxygen content matches man directory + run: | + callr::rscript(".dev/roxygen_test.R") + shell: Rscript {0} diff --git a/NEWS.md b/NEWS.md index c7bb1d938..55d7d5e80 100644 --- a/NEWS.md +++ b/NEWS.md @@ -22,7 +22,7 @@ ## Changes to default linters -* 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 default linter `return_linter()` for the style guide rule that terminal returns should be left implicit (#1100, #2343, #2354, and #2356, @MEO265 and @MichaelChirico). ## New and improved features @@ -62,7 +62,7 @@ * `print_linter()` for discouraging usage of `print()` on string literals like `print("Reached here")` or `print(paste("Found", nrow(DF), "rows."))` (#1894, @MichaelChirico). * `unnecessary_nesting_linter()` for discouraging overly-nested code where an early return or eliminated sub-expression (inside '{') is preferable (#2317 and part of #884, @MichaelChirico). * `consecutive_mutate_linter()` for encouraging consecutive calls to `dplyr::mutate()` to be combined (part of #884, @MichaelChirico). -* `if_switch_linter()` for encouraging `switch()` over repeated `if`/`else` tests (part of #884, @MichaelChirico). +* `if_switch_linter()` for encouraging `switch()` over repeated `if`/`else` tests (#2322 and part of #884, @MichaelChirico). * `nested_pipe_linter()` for discouraging pipes within pipes, e.g. `df1 %>% inner_join(df2 %>% select(a, b))` (part of #884, @MichaelChirico). * `nrow_subset_linter()` for discouraging usage like `nrow(subset(x, conditions))` in favor of something like `with(x, sum(conditions))` which doesn't require a full subset of `x` (#2313, #2314 and part of #884, @MichaelChirico). * `pipe_return_linter()` for discouraging usage of `return()` inside a {magrittr} pipeline (part of #884, @MichaelChirico). diff --git a/R/if_switch_linter.R b/R/if_switch_linter.R index 97b985dac..89415ed94 100644 --- a/R/if_switch_linter.R +++ b/R/if_switch_linter.R @@ -14,6 +14,12 @@ #' approach is roughly linear in the number of conditions that need to #' be evaluated, here up to 3 times). #' +#' @param max_branch_lines,max_branch_expressions Integer, default 0 indicates "no maximum". +#' If set any `if`/`else if`/.../`else` chain where any branch occupies more than +#' this number of lines (resp. expressions) will not be linted. The conjugate +#' applies to `switch()` statements -- if these parameters are set, any `switch()` +#' statement with any overly-complicated branches will be linted. See examples. +#' #' @examples #' # will produce lints #' lint( @@ -21,6 +27,64 @@ #' linters = if_switch_linter() #' ) #' +#' code <- paste( +#' "if (x == 'a') {", +#' " 1", +#' "} else if (x == 'b') {", +#' " 2", +#' "} else if (x == 'c') {", +#' " y <- x", +#' " z <- sqrt(match(y, letters))", +#' " z", +#' "}", +#' sep = "\n" +#' ) +#' writeLines(code) +#' lint( +#' text = code, +#' linters = if_switch_linter() +#' ) +#' +#' code <- paste( +#' "if (x == 'a') {", +#' " 1", +#' "} else if (x == 'b') {", +#' " 2", +#' "} else if (x == 'c') {", +#' " y <- x", +#' " z <- sqrt(", +#' " match(y, letters)", +#' " )", +#' " z", +#' "}", +#' sep = "\n" +#' ) +#' writeLines(code) +#' lint( +#' text = code, +#' linters = if_switch_linter() +#' ) +#' +#' code <- paste( +#' "switch(x,", +#' " a = {", +#' " 1", +#' " 2", +#' " 3", +#' " },", +#' " b = {", +#' " 1", +#' " 2", +#' " }", +#' ")", +#' sep = "\n" +#' ) +#' writeLines(code) +#' lint( +#' text = code, +#' linters = if_switch_linter(max_branch_lines = 2L) +#' ) +#' #' # okay #' lint( #' text = "switch(x, a = 1, b = 2, 3)", @@ -33,18 +97,105 @@ #' linters = if_switch_linter() #' ) #' +#' code <- paste( +#' "if (x == 'a') {", +#' " 1", +#' "} else if (x == 'b') {", +#' " 2", +#' "} else if (x == 'c') {", +#' " y <- x", +#' " z <- sqrt(match(y, letters))", +#' " z", +#' "}", +#' sep = "\n" +#' ) +#' writeLines(code) +#' lint( +#' text = code, +#' linters = if_switch_linter(max_branch_lines = 2L) +#' ) +#' +#' code <- paste( +#' "if (x == 'a') {", +#' " 1", +#' "} else if (x == 'b') {", +#' " 2", +#' "} else if (x == 'c') {", +#' " y <- x", +#' " z <- sqrt(", +#' " match(y, letters)", +#' " )", +#' " z", +#' "}", +#' sep = "\n" +#' ) +#' writeLines(code) +#' lint( +#' text = code, +#' linters = if_switch_linter(max_branch_expressions = 2L) +#' ) +#' +#' code <- paste( +#' "switch(x,", +#' " a = {", +#' " 1", +#' " 2", +#' " 3", +#' " },", +#' " b = {", +#' " 1", +#' " 2", +#' " }", +#' ")", +#' sep = "\n" +#' ) +#' writeLines(code) +#' lint( +#' text = code, +#' linters = if_switch_linter(max_branch_lines = 3L) +#' ) +#' #' @evalRd rd_tags("if_switch_linter") #' @seealso [linters] for a complete list of linters available in lintr. #' @export -if_switch_linter <- function() { - equal_str_cond <- "expr[1][EQ and expr[STR_CONST]]" +if_switch_linter <- function(max_branch_lines = 0L, max_branch_expressions = 0L) { + equal_str_cond <- "expr[1][EQ and expr/STR_CONST]" + + if (max_branch_lines > 0L || max_branch_expressions > 0L) { + complexity_cond <- xp_or(c( + if (max_branch_lines > 0L) paste("OP-RIGHT-BRACE/@line2 - OP-LEFT-BRACE/@line1 > 1 +", max_branch_lines), + if (max_branch_expressions > 0L) paste("count(expr) >", max_branch_expressions) + )) + branch_expr_cond <- xp_and(c( + xp_or( + # if (x) { } ... + xp_and("preceding-sibling::IF", "position() = 2"), + # if (x) { ... } else { } + xp_and("preceding-sibling::ELSE", "not(IF)") + ), + complexity_cond + )) + max_lines_cond <- glue(".//expr[{branch_expr_cond}]") + + switch_xpath <- glue(" + parent::expr + /parent::expr[expr[ + position() > 2 + and {complexity_cond} + ]] + ") + } else { + max_lines_cond <- "false" + + switch_xpath <- NULL + } # NB: IF AND {...} AND ELSE/... implies >= 3 equality conditions are present # .//expr/IF/...: the expr in `==` that's _not_ the STR_CONST # not(preceding::IF): prevent nested matches which might be incorrect globally # not(. != .): don't match if there are _any_ expr which _don't_ match the top # expr - xpath <- glue(" + if_xpath <- glue(" //IF /parent::expr[ not(preceding-sibling::IF) @@ -58,15 +209,16 @@ if_switch_linter <- function() { .//expr/IF/following-sibling::{equal_str_cond}/expr[not(STR_CONST)] != expr[1][EQ]/expr[not(STR_CONST)] ) + and not({ max_lines_cond }) ] ") Linter(linter_level = "expression", function(source_expression) { xml <- source_expression$xml_parsed_content - bad_expr <- xml_find_all(xml, xpath) + bad_expr <- xml_find_all(xml, if_xpath) - xml_nodes_to_lints( + lints <- xml_nodes_to_lints( bad_expr, source_expression = source_expression, lint_message = paste( @@ -76,5 +228,19 @@ if_switch_linter <- function() { ), type = "warning" ) + + if (!is.null(switch_xpath)) { + xml_calls <- source_expression$xml_find_function_calls("switch") + switch_expr <- xml_find_all(xml_calls, switch_xpath) + + lints <- c(lints, xml_nodes_to_lints( + switch_expr, + source_expression = source_expression, + lint_message = "Prefer repeated if/else statements over overly-complicated switch() statements.", + type = "warning" + )) + } + + lints }) } diff --git a/R/return_linter.R b/R/return_linter.R index fd3dd5831..a0e1c245f 100644 --- a/R/return_linter.R +++ b/R/return_linter.R @@ -8,7 +8,8 @@ #' 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. +#' if necessary. Similarly, functions with terminal [switch()] statements must have an +#' explicit default case. #' @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()], @@ -178,36 +179,14 @@ nested_return_lints <- function(expr, params) { if (length(child_expr) == 0L) { return(list()) } - child_node <- xml_name(child_expr) - - if (child_node[1L] == "OP-LEFT-BRACE") { - expr_idx <- which(child_node %in% c("expr", "equal_assign", "expr_or_assign_or_help")) - if (length(expr_idx) == 0L) { # empty brace expression {} - if (params$implicit) { - return(list()) - } else { - return(list(xml_nodes_to_lints( - expr, - source_expression = params$source_expression, - lint_message = params$lint_message, - type = params$type - ))) - } - } - 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")) - 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) + names(child_expr) <- xml_name(child_expr) + + if (names(child_expr)[1L] == "OP-LEFT-BRACE") { + brace_return_lints(child_expr, expr, params) + } else if (names(child_expr)[1L] == "IF") { + if_return_lints(child_expr, expr, params) + } else if (!is.na(xml_find_first(expr, "expr/SYMBOL_FUNCTION_CALL[text() = 'switch']"))) { + switch_return_lints(child_expr, expr, params) } else { xml_nodes_to_lints( xml_find_first(child_expr[[1L]], params$lint_xpath), @@ -217,3 +196,53 @@ nested_return_lints <- function(expr, params) { ) } } + +brace_return_lints <- function(child_expr, expr, params) { + expr_idx <- which(names(child_expr) %in% c("expr", "equal_assign", "expr_or_assign_or_help")) + if (length(expr_idx) == 0L) { # empty brace expression {} + if (params$implicit) { + return(list()) + } else { + return(list(xml_nodes_to_lints( + expr, + source_expression = params$source_expression, + lint_message = params$lint_message, + type = params$type + ))) + } + } + nested_return_lints(child_expr[[tail(expr_idx, 1L)]], params) +} + +if_return_lints <- function(child_expr, expr, params) { + expr_idx <- which(names(child_expr) %in% c("expr", "equal_assign", "expr_or_assign_or_help")) + 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) +} + +switch_return_lints <- function(child_expr, expr, params) { + # equal_assign/expr_or_assign_or_help not possible here + expr_idx <- which(names(child_expr) == "expr") + # switch(x, ...) | expr[1]: switch; expr[2]: x. Drop the first two, check usage in ... + return_lints <- lapply(child_expr[tail(expr_idx, -2L)], nested_return_lints, params) + # in addition to the two dropped above, a third unmatched would be the default case. + if (params$allow_implicit_else || length(expr_idx) - sum(names(child_expr) == "EQ_SUB") == 3L) { + return(return_lints) + } + implicit_else_lints <- list(xml_nodes_to_lints( + expr, + source_expression = params$source_expression, + lint_message = "All functions with terminal switch statements must have a terminal default clause.", + type = "warning" + )) + c(return_lints, implicit_else_lints) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 77907fb74..f6d614775 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -38,7 +38,7 @@ function_argument_linter,style consistency best_practices function_left_parentheses_linter,style readability default function_return_linter,readability best_practices if_not_else_linter,readability consistency configurable -if_switch_linter,best_practices readability consistency efficiency +if_switch_linter,best_practices readability consistency efficiency configurable ifelse_censor_linter,best_practices efficiency implicit_assignment_linter,style best_practices readability configurable implicit_integer_linter,style consistency best_practices configurable diff --git a/man/configurable_linters.Rd b/man/configurable_linters.Rd index 4c3da9ec1..cb1c17a54 100644 --- a/man/configurable_linters.Rd +++ b/man/configurable_linters.Rd @@ -24,6 +24,7 @@ The following linters are tagged with 'configurable': \item{\code{\link{duplicate_argument_linter}}} \item{\code{\link{fixed_regex_linter}}} \item{\code{\link{if_not_else_linter}}} +\item{\code{\link{if_switch_linter}}} \item{\code{\link{implicit_assignment_linter}}} \item{\code{\link{implicit_integer_linter}}} \item{\code{\link{indentation_linter}}} diff --git a/man/if_switch_linter.Rd b/man/if_switch_linter.Rd index e1254ff79..8a7cd302b 100644 --- a/man/if_switch_linter.Rd +++ b/man/if_switch_linter.Rd @@ -4,7 +4,14 @@ \alias{if_switch_linter} \title{Require usage of switch() over repeated if/else blocks} \usage{ -if_switch_linter() +if_switch_linter(max_branch_lines = 0L, max_branch_expressions = 0L) +} +\arguments{ +\item{max_branch_lines, max_branch_expressions}{Integer, default 0 indicates "no maximum". +If set any \code{if}/\verb{else if}/.../\verb{else} chain where any branch occupies more than +this number of lines (resp. expressions) will not be linted. The conjugate +applies to \code{switch()} statements -- if these parameters are set, any \code{switch()} +statement with any overly-complicated branches will be linted. See examples.} } \description{ \code{\link[=switch]{switch()}} statements in R are used to delegate behavior based @@ -29,6 +36,64 @@ lint( linters = if_switch_linter() ) +code <- paste( + "if (x == 'a') {", + " 1", + "} else if (x == 'b') {", + " 2", + "} else if (x == 'c') {", + " y <- x", + " z <- sqrt(match(y, letters))", + " z", + "}", + sep = "\n" +) +writeLines(code) +lint( + text = code, + linters = if_switch_linter() +) + +code <- paste( + "if (x == 'a') {", + " 1", + "} else if (x == 'b') {", + " 2", + "} else if (x == 'c') {", + " y <- x", + " z <- sqrt(", + " match(y, letters)", + " )", + " z", + "}", + sep = "\n" +) +writeLines(code) +lint( + text = code, + linters = if_switch_linter() +) + +code <- paste( + "switch(x,", + " a = {", + " 1", + " 2", + " 3", + " },", + " b = {", + " 1", + " 2", + " }", + ")", + sep = "\n" +) +writeLines(code) +lint( + text = code, + linters = if_switch_linter(max_branch_lines = 2L) +) + # okay lint( text = "switch(x, a = 1, b = 2, 3)", @@ -41,10 +106,68 @@ lint( linters = if_switch_linter() ) +code <- paste( + "if (x == 'a') {", + " 1", + "} else if (x == 'b') {", + " 2", + "} else if (x == 'c') {", + " y <- x", + " z <- sqrt(match(y, letters))", + " z", + "}", + sep = "\n" +) +writeLines(code) +lint( + text = code, + linters = if_switch_linter(max_branch_lines = 2L) +) + +code <- paste( + "if (x == 'a') {", + " 1", + "} else if (x == 'b') {", + " 2", + "} else if (x == 'c') {", + " y <- x", + " z <- sqrt(", + " match(y, letters)", + " )", + " z", + "}", + sep = "\n" +) +writeLines(code) +lint( + text = code, + linters = if_switch_linter(max_branch_expressions = 2L) +) + +code <- paste( + "switch(x,", + " a = {", + " 1", + " 2", + " 3", + " },", + " b = {", + " 1", + " 2", + " }", + ")", + sep = "\n" +) +writeLines(code) +lint( + text = code, + linters = if_switch_linter(max_branch_lines = 3L) +) + } \seealso{ \link{linters} for a complete list of linters available in lintr. } \section{Tags}{ -\link[=best_practices_linters]{best_practices}, \link[=consistency_linters]{consistency}, \link[=efficiency_linters]{efficiency}, \link[=readability_linters]{readability} +\link[=best_practices_linters]{best_practices}, \link[=configurable_linters]{configurable}, \link[=consistency_linters]{consistency}, \link[=efficiency_linters]{efficiency}, \link[=readability_linters]{readability} } diff --git a/man/linters.Rd b/man/linters.Rd index 838044aad..10c45d374 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -19,7 +19,7 @@ The following tags exist: \itemize{ \item{\link[=best_practices_linters]{best_practices} (63 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (11 linters)} -\item{\link[=configurable_linters]{configurable} (42 linters)} +\item{\link[=configurable_linters]{configurable} (43 linters)} \item{\link[=consistency_linters]{consistency} (32 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (26 linters)} @@ -74,7 +74,7 @@ The following linters exist: \item{\code{\link{function_left_parentheses_linter}} (tags: default, readability, style)} \item{\code{\link{function_return_linter}} (tags: best_practices, readability)} \item{\code{\link{if_not_else_linter}} (tags: configurable, consistency, readability)} -\item{\code{\link{if_switch_linter}} (tags: best_practices, consistency, efficiency, readability)} +\item{\code{\link{if_switch_linter}} (tags: best_practices, configurable, consistency, efficiency, readability)} \item{\code{\link{ifelse_censor_linter}} (tags: best_practices, efficiency)} \item{\code{\link{implicit_assignment_linter}} (tags: best_practices, configurable, readability, style)} \item{\code{\link{implicit_integer_linter}} (tags: best_practices, configurable, consistency, style)} diff --git a/man/return_linter.Rd b/man/return_linter.Rd index fc4fdbab0..0e99df289 100644 --- a/man/return_linter.Rd +++ b/man/return_linter.Rd @@ -20,7 +20,8 @@ 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.} +if necessary. Similarly, functions with terminal \code{\link[=switch]{switch()}} statements must have an +explicit default case.} \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 diff --git a/tests/testthat/test-if_switch_linter.R b/tests/testthat/test-if_switch_linter.R index b321e680e..e6b3e5fe5 100644 --- a/tests/testthat/test-if_switch_linter.R +++ b/tests/testthat/test-if_switch_linter.R @@ -77,3 +77,440 @@ test_that("multiple lints have right metadata", { if_switch_linter() ) }) + +test_that("max_branch_lines= and max_branch_expressions= arguments work", { + max_lines2_linter <- if_switch_linter(max_branch_lines = 2L) + max_lines4_linter <- if_switch_linter(max_branch_lines = 4L) + max_expr2_linter <- if_switch_linter(max_branch_expressions = 2L) + max_expr4_linter <- if_switch_linter(max_branch_expressions = 4L) + lint_msg <- rex::rex("Prefer switch() statements over repeated if/else equality tests") + + one_per_branch_lines <- trim_some(" + if (x == 'a') { + 1 + } else if (x == 'b') { + 2 + } else if (x == 'c') { + 3 + } + ") + expect_lint(one_per_branch_lines, lint_msg, max_lines2_linter) + expect_lint(one_per_branch_lines, lint_msg, max_lines4_linter) + expect_lint(one_per_branch_lines, lint_msg, max_expr2_linter) + expect_lint(one_per_branch_lines, lint_msg, max_expr4_linter) + + two_per_branch_lines <- trim_some(" + if (x == 'a') { + 1 + 2 + } else if (x == 'b') { + 3 + 4 + } else if (x == 'c') { + 5 + 6 + } + ") + expect_lint(two_per_branch_lines, lint_msg, max_lines2_linter) + expect_lint(two_per_branch_lines, lint_msg, max_lines4_linter) + expect_lint(two_per_branch_lines, lint_msg, max_expr2_linter) + expect_lint(two_per_branch_lines, lint_msg, max_expr4_linter) + + three_per_branch_lines <- trim_some(" + if (x == 'a') { + 1 + 2 + 3 + } else if (x == 'b') { + 4 + 5 + 6 + } else if (x == 'c') { + 7 + 8 + 9 + } + ") + expect_lint(three_per_branch_lines, NULL, max_lines2_linter) + expect_lint(three_per_branch_lines, lint_msg, max_lines4_linter) + expect_lint(three_per_branch_lines, NULL, max_expr2_linter) + expect_lint(three_per_branch_lines, lint_msg, max_expr4_linter) + + five_per_branch_lines <- trim_some(" + if (x == 'a') { + 1 + 2 + 3 + 4 + 5 + } else if (x == 'b') { + 6 + 7 + 8 + 9 + 10 + } else if (x == 'c') { + 11 + 12 + 13 + 14 + 15 + } + ") + expect_lint(five_per_branch_lines, NULL, max_lines2_linter) + expect_lint(five_per_branch_lines, NULL, max_lines4_linter) + expect_lint(five_per_branch_lines, NULL, max_expr2_linter) + expect_lint(five_per_branch_lines, NULL, max_expr4_linter) + + five_lines_three_expr_lines <- trim_some(" + if (x == 'a') { + 1 + 2 + foo( + x + ) + } else if (x == 'b') { + 6 + 7 + bar( + y + ) + } else if (x == 'c') { + 11 + 12 + baz( + z + ) + } + ") + expect_lint(five_lines_three_expr_lines, NULL, max_lines2_linter) + expect_lint(five_lines_three_expr_lines, NULL, max_lines4_linter) + expect_lint(five_lines_three_expr_lines, NULL, max_expr2_linter) + expect_lint( + five_lines_three_expr_lines, + list(lint_msg, line_number = 1L), + max_expr4_linter + ) + + five_expr_three_lines_lines <- trim_some(" + if (x == 'a') { + 1 + 2 + 3; 4; 5 + } else if (x == 'b') { + 6 + 7 + 8; 9; 10 + } else if (x == 'c') { + 11 + 12 + 13; 14; 15 + } + ") + expect_lint(five_expr_three_lines_lines, NULL, max_lines2_linter) + expect_lint( + five_expr_three_lines_lines, + list(lint_msg, line_number = 1L), + max_lines4_linter + ) + expect_lint(five_expr_three_lines_lines, NULL, max_expr2_linter) + expect_lint(five_expr_three_lines_lines, NULL, max_expr4_linter) +}) + +test_that("max_branch_lines= and max_branch_expressions= block over-complex switch() too", { + max_lines2_linter <- if_switch_linter(max_branch_lines = 2L) + max_lines4_linter <- if_switch_linter(max_branch_lines = 4L) + max_expr2_linter <- if_switch_linter(max_branch_expressions = 2L) + max_expr4_linter <- if_switch_linter(max_branch_expressions = 4L) + lint_msg <- rex::rex("Prefer repeated if/else statements over overly-complicated switch() statements.") + + one_per_branch_lines <- trim_some(" + switch(x, + a = { + 1 + }, + b = { + 2 + }, + c = { + 3 + } + ) + ") + expect_lint(one_per_branch_lines, NULL, max_lines2_linter) + expect_lint(one_per_branch_lines, NULL, max_lines4_linter) + expect_lint(one_per_branch_lines, NULL, max_expr2_linter) + expect_lint(one_per_branch_lines, NULL, max_expr4_linter) + + two_per_branch_lines <- trim_some(" + switch(x, + a = { + 1 + 2 + }, + b = { + 3 + 4 + }, + c = { + 5 + 6 + } + ) + ") + expect_lint(two_per_branch_lines, NULL, max_lines2_linter) + expect_lint(two_per_branch_lines, NULL, max_lines4_linter) + expect_lint(two_per_branch_lines, NULL, max_expr2_linter) + expect_lint(two_per_branch_lines, NULL, max_expr4_linter) + + three_per_branch_lines <- trim_some(" + switch(x, + a = { + 1 + 2 + 3 + }, + b = { + 4 + 5 + 6 + }, + c = { + 7 + 8 + 9 + } + ) + ") + expect_lint( + three_per_branch_lines, + list(lint_msg, line_number = 1L), + max_lines2_linter + ) + expect_lint(three_per_branch_lines, NULL, max_lines4_linter) + expect_lint( + three_per_branch_lines, + list(lint_msg, line_number = 1L), + max_expr2_linter + ) + expect_lint(three_per_branch_lines, NULL, max_expr4_linter) + + five_per_branch_lines <- trim_some(" + switch(x, + a = { + 1 + 2 + 3 + 4 + 5 + }, + b = { + 6 + 7 + 8 + 9 + 10 + }, + c = { + 11 + 12 + 13 + 14 + 15 + } + ) + ") + expect_lint(five_per_branch_lines, lint_msg, max_lines2_linter) + expect_lint(five_per_branch_lines, lint_msg, max_lines4_linter) + expect_lint(five_per_branch_lines, lint_msg, max_expr2_linter) + expect_lint(five_per_branch_lines, lint_msg, max_expr4_linter) + + five_lines_three_expr_lines <- trim_some(" + switch(x, + a = { + 1 + 2 + foo( + x + ) + }, + b = { + 6 + 7 + bar( + y + ) + }, + c = { + 11 + 12 + baz( + z + ) + } + ) + ") + expect_lint(five_lines_three_expr_lines, lint_msg, max_lines2_linter) + expect_lint(five_lines_three_expr_lines, lint_msg, max_lines4_linter) + expect_lint(five_lines_three_expr_lines, lint_msg, max_expr2_linter) + expect_lint(five_lines_three_expr_lines, NULL, max_expr4_linter) + + five_expr_three_lines_lines <- trim_some(" + switch(x, + a = { + 1 + 2 + 3; 4; 5 + }, + b = { + 6 + 7 + 8; 9; 10 + }, + c = { + 11 + 12 + 13; 14; 15 + } + ) + ") + expect_lint(five_expr_three_lines_lines, lint_msg, max_lines2_linter) + expect_lint(five_expr_three_lines_lines, NULL, max_lines4_linter) + expect_lint(five_expr_three_lines_lines, lint_msg, max_expr2_linter) + expect_lint(five_expr_three_lines_lines, lint_msg, max_expr4_linter) +}) + +test_that("max_branch_lines= and max_branch_expressions= interact correctly", { + linter <- if_switch_linter(max_branch_lines = 5L, max_branch_expressions = 3L) + lint_msg <- rex::rex("Prefer switch() statements over repeated if/else equality tests") + + expect_lint( + trim_some(" + if (x == 'a') { + 1 + } else if (x == 'b') { + 2 + } else if (x == 'c') { + 3 + } + "), + lint_msg, + linter + ) + + expect_lint( + trim_some(" + if (x == 'a') { + foo( + x1, + x2, + x3, + x4 + ) + } else if (x == 'b') { + 2 + } else if (x == 'c') { + 3 + } + "), + NULL, + linter + ) + + expect_lint( + trim_some(" + if (x == 'a') { + 1; 2; 3; 4 + } else if (x == 'b') { + 5 + } else if (x == 'c') { + 6 + } + "), + NULL, + linter + ) +}) + +test_that("max_branch_lines= and max_branch_expressions= work for a terminal 'else' branch", { + max_lines2_linter <- if_switch_linter(max_branch_lines = 2L) + max_expr2_linter <- if_switch_linter(max_branch_expressions = 2L) + lint_msg <- rex::rex("Prefer repeated if/else statements over overly-complicated switch() statements.") + + else_long_lines <- trim_some(" + if (x == 'a') { + 1 + } else if (x == 'b') { + 2 + } else if (x == 'c') { + 3 + } else { + 4 + 5 + 6 + } + ") + expect_lint(else_long_lines, NULL, max_lines2_linter) + expect_lint(else_long_lines, NULL, max_expr2_linter) + + default_long_lines <- trim_some(" + switch(x, + a = { + 1 + }, + b = { + 2 + }, + c = { + 3 + }, + { + 4 + 5 + 6 + } + ) + ") + expect_lint(default_long_lines, lint_msg, max_lines2_linter) + expect_lint(default_long_lines, lint_msg, max_expr2_linter) +}) + +test_that("max_branch_lines= and max_branch_expressions= are guided by the most complex branch", { + max_lines2_linter <- if_switch_linter(max_branch_lines = 2L) + max_expr2_linter <- if_switch_linter(max_branch_expressions = 2L) + lint_msg <- rex::rex("Prefer repeated if/else statements over overly-complicated switch() statements.") + + # no lint if _any_ branch is too complex + if_else_one_branch_lines <- trim_some(" + if (x == 'a') { + 1 + } else if (x == 'b') { + 2 + } else if (x == 'c') { + 3 + 4 + 5 + } + ") + expect_lint(if_else_one_branch_lines, NULL, max_lines2_linter) + expect_lint(if_else_one_branch_lines, NULL, max_expr2_linter) + + # lint if _any_ branch is too complex + switch_one_branch_lines <- trim_some(" + switch(x, + a = { + 1 + }, + b = { + 2 + }, + c = { + 3 + 4 + 5 + } + ) + ") + expect_lint(switch_one_branch_lines, lint_msg, max_lines2_linter) + expect_lint(switch_one_branch_lines, lint_msg, max_expr2_linter) +}) diff --git a/tests/testthat/test-return_linter.R b/tests/testthat/test-return_linter.R index a284193bd..b05b3abd3 100644 --- a/tests/testthat/test-return_linter.R +++ b/tests/testthat/test-return_linter.R @@ -166,56 +166,6 @@ test_that("Do not lint stop on end of function", { ) }) -test_that("Do not lint stop on end of function", { - linter <- return_linter(return_style = "explicit") - lint_msg <- rex::rex("All functions must have an explicit return().") - - expect_lint( - trim_some(" - function(x) { - switch(x, a = 1, 'b' = 2, '3' = 3, 4) - } - "), - list(lint_msg, line_number = 2L), - linter - ) - - expect_lint( - trim_some(" - function(x) { - switch(x, a = return(1), 'b' = stop(2), '3' = return(3), 4) - } - "), - list(lint_msg, line_number = 2L), - linter - ) - - expect_lint( - trim_some(" - function() { - switch( - x, - a = return(1), - 'b' = stop(2), - '3' = return(3) - ) - } - "), - list(lint_msg, line_number = 2L), - linter - ) - - expect_lint( - trim_some(" - function(x) { - switch(x, a = return(1), 'b' = stop(2), '3' = return(3), stop('End')) - } - "), - list(lint_msg, line_number = 2L), - linter - ) -}) - test_that("return_linter works in simple function", { expect_lint( trim_some(" @@ -1587,3 +1537,290 @@ test_that("= assignments are handled correctly", { implicit_linter ) }) + +test_that("terminal switch() is handled correctly", { + implicit_linter <- return_linter() + implicit_msg <- rex::rex("Use implicit return behavior; explicit return() is not needed.") + explicit_linter <- return_linter(return_style = "explicit") + explicit_msg <- rex::rex("All functions must have an explicit return().") + + no_return_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = 1, + b = 2 + ) + } + ") + expect_lint(no_return_lines, NULL, implicit_linter) + expect_lint(no_return_lines, list(explicit_msg, explicit_msg), explicit_linter) + + outer_return_lines <- trim_some(" + foo <- function(x) { + return(switch(x, + a = 1, + b = 2 + )) + } + ") + expect_lint(outer_return_lines, implicit_msg, implicit_linter) + expect_lint(outer_return_lines, NULL, explicit_linter) + + partial_return_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = return(1), + b = 2 + ) + } + ") + expect_lint(partial_return_lines, implicit_msg, implicit_linter) + expect_lint(partial_return_lines, explicit_msg, explicit_linter) + + all_return_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = return(1), + b = return(2) + ) + } + ") + expect_lint(all_return_lines, list(implicit_msg, implicit_msg), implicit_linter) + expect_lint(all_return_lines, NULL, explicit_linter) + + default_all_return_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = return(1), + return(2) + ) + } + ") + expect_lint(default_all_return_lines, list(implicit_msg, implicit_msg), implicit_linter) + expect_lint(default_all_return_lines, NULL, explicit_linter) + + default_no_return_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = 1, + 2 + ) + } + ") + expect_lint(default_no_return_lines, NULL, implicit_linter) + expect_lint(default_no_return_lines, list(explicit_msg, explicit_msg), explicit_linter) + + no_return_braced_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = { + 1 + 2 + 3 + 4 + }, + b = { + 5 + 6 + 7 + } + ) + } + ") + expect_lint(no_return_braced_lines, NULL, implicit_linter) + expect_lint( + no_return_braced_lines, + list( + list(explicit_msg, line_number = 7L), + list(explicit_msg, line_number = 12L) + ), + explicit_linter + ) + + all_return_braced_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = { + 1 + 2 + 3 + return(4) + }, + b = { + 5 + 6 + return(7) + } + ) + } + ") + expect_lint( + all_return_braced_lines, + list( + list(implicit_msg, line_number = 7L), + list(implicit_msg, line_number = 12L) + ), + implicit_linter + ) + expect_lint(all_return_braced_lines, NULL, explicit_linter) + + early_return_braced_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = { + 1 + if (TRUE) { + return(2) + } + 3 + 4 + }, + b = { + 5 + 6 + 7 + } + ) + } + ") + expect_lint(early_return_braced_lines, NULL, implicit_linter) + expect_lint( + early_return_braced_lines, + list( + list(explicit_msg, line_number = 9L), + list(explicit_msg, line_number = 14L) + ), + explicit_linter + ) + + if_no_return_braced_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = { + 1 + if (TRUE) { + 2 + } else { + 3 + } + }, + b = { + 5 + 6 + 7 + } + ) + } + ") + expect_lint(if_no_return_braced_lines, NULL, implicit_linter) + expect_lint( + if_no_return_braced_lines, + list( + list(explicit_msg, line_number = 6L), + list(explicit_msg, line_number = 8L), + list(explicit_msg, line_number = 14L) + ), + explicit_linter + ) + + if_return_braced_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = { + 1 + if (TRUE) { + return(2) + } else { + return(3) + } + }, + b = { + 5 + 6 + return(7) + } + ) + } + ") + expect_lint( + if_return_braced_lines, + list( + list(implicit_msg, line_number = 6L), + list(implicit_msg, line_number = 8L), + list(implicit_msg, line_number = 14L) + ), + implicit_linter + ) + expect_lint(if_return_braced_lines, NULL, explicit_linter) + + ok_exit_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = .Call(a_routine, x), + b = .Call(b_routine, x), + stop('invalid') + ) + } + ") + expect_lint(ok_exit_lines, NULL, implicit_linter) + expect_lint(ok_exit_lines, NULL, explicit_linter) +}) + +test_that("switch() default statements interact with allow_implicit_else", { + implicit_linter <- return_linter(allow_implicit_else = FALSE) + explicit_linter <- return_linter(allow_implicit_else = FALSE, return_style = "explicit") + implicit_msg <- rex::rex("Use implicit return behavior; explicit return() is not needed.") + explicit_msg <- rex::rex("All functions must have an explicit return().") + implicit_switch_msg <- rex::rex("All functions with terminal switch statements") + implicit_else_msg <- rex::rex("All functions with terminal if statements") + + no_default_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = 1, + b = 2 + ) + } + ") + expect_lint(no_default_lines, list(implicit_switch_msg, line_number = 2L), implicit_linter) + expect_lint(no_default_lines, list(implicit_switch_msg, explicit_msg, explicit_msg), explicit_linter) + + ifelse_default_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = 1, + b = 2, + if (x != 'c') { + 3 + } else { + 4 + } + ) + } + ") + expect_lint(ifelse_default_lines, NULL, implicit_linter) + expect_lint(ifelse_default_lines, list(explicit_msg, explicit_msg, explicit_msg, explicit_msg), explicit_linter) + + if_no_else_default_lines <- trim_some(" + foo <- function(x) { + switch(x, + a = 1, + b = 2, + if (x != 'c') { + 3 + } + ) + } + ") + expect_lint(if_no_else_default_lines, list(implicit_else_msg, line_number = 5L), implicit_linter) + expect_lint( + if_no_else_default_lines, + list( + list(explicit_msg, line_number = 3L), + list(explicit_msg, line_number = 4L), + list(implicit_else_msg, line_number = 5L), + list(explicit_msg, line_number = 6L) + ), + explicit_linter + ) +})