More robust unreachable_code_linter

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
 

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) {

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)