diff --git a/NEWS.md b/NEWS.md index 907848c67..9ec55bec2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -41,6 +41,7 @@ * `function_argument_linter()` detects usage of `missing()` for the linted argument (#1546, @MichaelChirico). The simplest fix for `function_argument_linter()` lints is typically to set that argument to `NULL` by default, in which case it's usually preferable to update function logic checking `missing()` to check `is.null()` instead. * `equals_na_linter()` checks for `x %in% NA`, which is a more convoluted form of `is.na(x)` (#2088, @MichaelChirico). * `commas_linter()` gains an option `allow_trailing` (default `FALSE`) to allow trailing commas while indexing. (#2104, @MEO265) +* `unreachable_code_linter()` finds unreachable code even in the presence of a comment or semicolon after `return()` or `stop()` (#2127, @MEO265). ### New linters diff --git a/R/unreachable_code_linter.R b/R/unreachable_code_linter.R index a1cb0aa55..9d18560e6 100644 --- a/R/unreachable_code_linter.R +++ b/R/unreachable_code_linter.R @@ -26,22 +26,15 @@ #' @seealso [linters] for a complete list of linters available in lintr. #' @export unreachable_code_linter <- function() { - # NB: - # - * returns all children, including the terminal }, so the position - # is not last(), but last()-1. If there's no }, this linter doesn't apply. - # this is also why we need /* and not /expr -- position() must include all nodes - # - use not(OP-DOLLAR) to prevent matching process$stop(), #1051 - # - land on the culprit expression + # NB: use not(OP-DOLLAR) to prevent matching process$stop(), #1051 xpath <- " //FUNCTION /following-sibling::expr - /*[ - self::expr - and expr[1][not(OP-DOLLAR or OP-AT) and SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop']] - and (position() != last() - 1 or not(following-sibling::OP-RIGHT-BRACE)) - and @line2 < following-sibling::*[1]/@line2 - ] - /following-sibling::*[1] + /expr[expr[1][not(OP-DOLLAR or OP-AT) and SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop']]] + /following-sibling::*[ + not(self::OP-RIGHT-BRACE or self::OP-SEMICOLON) + and (not(self::COMMENT) or @line2 > preceding-sibling::*[1]/@line2) + ][1] " Linter(function(source_expression) { diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R index 47e40e6b1..dab550771 100644 --- a/tests/testthat/test-unreachable_code_linter.R +++ b/tests/testthat/test-unreachable_code_linter.R @@ -78,6 +78,55 @@ test_that("unreachable_code_linter finds unreachable comments", { ) }) +test_that("unreachable_code_linter finds expressions in the same line", { + msg <- rex::rex("Code and comments coming after a top-level return() or stop()") + linter <- unreachable_code_linter() + + lines <- trim_some(" + foo <- function(x) { + return( + y^2 + ); 3 + 1 + } + ") + expect_lint(lines, msg, linter) + + lines <- trim_some(" + foo <- function(x) { + return(y^2); 3 + 1 + } + ") + expect_lint(lines, msg, linter) + + lines <- trim_some(" + foo <- function(x) { + return(y^2); 3 + 1 # Test + } + ") + expect_lint(lines, msg, linter) +}) + +test_that("unreachable_code_linter finds expressions and comments after comment in return line", { + msg <- rex::rex("Code and comments coming after a top-level return() or stop()") + linter <- unreachable_code_linter() + + lines <- trim_some(" + foo <- function(x) { + return(y^2) #Test comment + #Test comment 2 + } + ") + expect_lint(lines, msg, linter) + + lines <- trim_some(" + foo <- function(x) { + return(y^2) # Test + 3 + 1 + } + ") + expect_lint(lines, msg, linter) +}) + test_that("unreachable_code_linter finds a double return", { lines <- trim_some(" foo <- function(x) { @@ -178,7 +227,5 @@ test_that("unreachable_code_linter ignores terminal nolint end comments", { # be followed by return(invisible()) or similar), but could be included to # catch comments for completeness / robustness as a standalone function. # Terminal if statements are a bit messy, but would have some payoff. -# TODO(michaelchirico): similarly, return(x); x+1 should also lint, even though -# the styler won't allow this in our current setup. # TODO(michaelchirico): again similarly, this could also apply to cases without # explicit returns (where it can only apply to comments)