From f5702116fb566126ae52554679fd2dcb55d486cf Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 26 Jul 2025 21:01:31 +0000 Subject: [PATCH 01/10] handle comments --- R/unreachable_code_linter.R | 52 ++- tests/testthat/test-unreachable_code_linter.R | 300 +++++++++++------- 2 files changed, 216 insertions(+), 136 deletions(-) diff --git a/R/unreachable_code_linter.R b/R/unreachable_code_linter.R index f2e9f8d56b..acfdda2d22 100644 --- a/R/unreachable_code_linter.R +++ b/R/unreachable_code_linter.R @@ -76,33 +76,55 @@ #' @seealso [linters] for a complete list of linters available in lintr. #' @export unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclude_end", "# nocov end")) { + # nolint next: object_usage_linter. Used in glue() in statically-difficult fashion to detect. expr_after_control <- " (//REPEAT | //ELSE | //FOR)/following-sibling::expr[1] | (//IF | //WHILE)/following-sibling::expr[2] " + + unreachable_expr_cond_ws <- " + following-sibling::*[ + not(self::OP-RIGHT-BRACE or self::OP-SEMICOLON or self::ELSE or preceding-sibling::ELSE) + and (not(self::COMMENT) or @line2 > preceding-sibling::*[not(self::COMMENT)][1]/@line2) + ][1]" + # when a semicolon is present, the condition is a bit different due to nodes + unreachable_expr_cond_sc <- " + parent::exprlist[OP-SEMICOLON] + /following-sibling::*[ + not(self::OP-RIGHT-BRACE) + and (not(self::COMMENT) or @line1 > preceding-sibling::exprlist/expr/@line2) + ][1] + " + # NB: use not(OP-DOLLAR) to prevent matching process$stop(), #1051 - xpath_return_stop <- glue(" + xpath_return_stop_fmt <- " ( {expr_after_control} - | (//FUNCTION | //OP-LAMBDA)[following-sibling::expr[1]/*[1][self::OP-LEFT-BRACE]]/following-sibling::expr[1] + | + (//FUNCTION | //OP-LAMBDA) + /following-sibling::expr[OP-LEFT-BRACE][last()] ) - /expr[expr[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] - ") - xpath_next_break <- glue(" + /{unreachable_expr_cond} + " + xpath_return_stop <- paste( + glue(xpath_return_stop_fmt, unreachable_expr_cond = unreachable_expr_cond_ws), + glue(xpath_return_stop_fmt, unreachable_expr_cond = unreachable_expr_cond_sc), + sep = " | " + ) + xpath_next_break_fmt <- " ({expr_after_control}) - /expr[NEXT or BREAK] - /following-sibling::*[ - not(self::OP-RIGHT-BRACE or self::OP-SEMICOLON) - and (not(self::COMMENT) or @line2 > preceding-sibling::*[1]/@line2) - ][1] - ") + //expr[NEXT or BREAK] + /{unreachable_expr_cond} + " + xpath_next_break <- paste( + glue(xpath_next_break_fmt, unreachable_expr_cond = unreachable_expr_cond_ws), + glue(xpath_next_break_fmt, unreachable_expr_cond = unreachable_expr_cond_sc), + sep = " | " + ) xpath_if_while <- " (//WHILE | //IF)[following-sibling::expr[1]/NUM_CONST[text() = 'FALSE']] diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R index b54d3b11e6..2a9cf20d40 100644 --- a/tests/testthat/test-unreachable_code_linter.R +++ b/tests/testthat/test-unreachable_code_linter.R @@ -4,7 +4,7 @@ test_that("unreachable_code_linter works in simple function", { return(bar) } ") - expect_lint(lines, NULL, unreachable_code_linter()) + expect_no_lint(lines, unreachable_code_linter()) }) test_that("unreachable_code_linter works in sub expressions", { @@ -55,44 +55,43 @@ test_that("unreachable_code_linter works in sub expressions", { linter ) - lines <- trim_some(" - foo <- function(bar) { - if (bar) { - return(bar) # Test comment - } - while (bar) { - return(bar) # 5 + 3 - } - repeat { - return(bar) # Test comment - } - - } - ") - - expect_lint(lines, NULL, linter) + expect_no_lint( # nofuzz + trim_some(" + foo <- function(bar) { + if (bar) { + return(bar) # Test comment + } + while (bar) { + return(bar) # 5 + 3 + } + repeat { + return(bar) # Test comment + } - lines <- trim_some(" - foo <- function(bar) { - if (bar) { - return(bar); x <- 2 - } else { - return(bar); x <- 3 - } - while (bar) { - return(bar); 5 + 3 } - repeat { - return(bar); test() - } - for(i in 1:3) { - return(bar); 5 + 4 - } - } - ") + "), + linter + ) - expect_lint( - lines, + expect_lint( + trim_some(" + foo <- function(bar) { + if (bar) { + return(bar); x <- 2 + } else { + return(bar); x <- 3 + } + while (bar) { + return(bar); 5 + 3 + } + repeat { + return(bar); test() + } + for(i in 1:3) { + return(bar); 5 + 4 + } + } + "), list( list(line_number = 3L, message = msg), list(line_number = 5L, message = msg), @@ -102,6 +101,40 @@ test_that("unreachable_code_linter works in sub expressions", { ), linter ) + + expect_lint( + trim_some(" + foo <- function(bar) { + if (bar) { + return(bar); # comment + x <- 2 + } else { + return(bar); # comment + x <- 3 + } + while (bar) { + return(bar); # comment + 5 + 3 + } + repeat { + return(bar); # comment + test() + } + for(i in 1:3) { + return(bar); # comment + 5 + 4 + } + } + "), + list( + list(line_number = 4L, message = msg), + list(line_number = 7L, message = msg), + list(line_number = 11L, message = msg), + list(line_number = 15L, message = msg), + list(line_number = 19L, message = msg) + ), + linter + ) }) test_that("unreachable_code_linter works with next and break in sub expressions", { @@ -144,48 +177,47 @@ test_that("unreachable_code_linter works with next and break in sub expressions" linter ) - lines <- trim_some(" - foo <- function(bar) { - if (bar) { - break # Test comment - } else { - next # Test comment - } - while (bar) { - next # 5 + 3 - } - repeat { - next # Test comment - } - for(i in 1:3) { - break # 5 + 4 - } - } - ") - - expect_lint(lines, NULL, linter) - - lines <- trim_some(" - foo <- function(bar) { - if (bar) { - next; x <- 2 - } else { - break; x <- 3 - } - while (bar) { - break; 5 + 3 - } - repeat { - next; test() - } - for(i in 1:3) { - break; 5 + 4 + expect_no_lint( # nofuzz + trim_some(" + foo <- function(bar) { + if (bar) { + break # Test comment + } else { + next # Test comment + } + while (bar) { + next # 5 + 3 + } + repeat { + next # Test comment + } + for(i in 1:3) { + break # 5 + 4 + } } - } - ") + "), + linter + ) expect_lint( - lines, + trim_some(" + foo <- function(bar) { + if (bar) { + next; x <- 2 + } else { + break; x <- 3 + } + while (bar) { + break; 5 + 3 + } + repeat { + next; test() + } + for(i in 1:3) { + break; 5 + 4 + } + } + "), list( list(line_number = 3L, message = msg), list(line_number = 5L, message = msg), @@ -195,14 +227,49 @@ test_that("unreachable_code_linter works with next and break in sub expressions" ), linter ) + + # also with comments + expect_lint( + trim_some(" + foo <- function(bar) { + if (bar) { + next; # comment + x <- 2 + } else { + break; # comment + x <- 3 + } + while (bar) { + break; # comment + 5 + 3 + } + repeat { + next; # comment + test() + } + for(i in 1:3) { + break; # comment + 5 + 4 + } + } + "), + list( + list(line_number = 4L, message = msg), + list(line_number = 7L, message = msg), + list(line_number = 11L, message = msg), + list(line_number = 15L, message = msg), + list(line_number = 19L, message = msg) + ), + linter + ) }) test_that("unreachable_code_linter ignores expressions that aren't functions", { - expect_lint("x + 1", NULL, unreachable_code_linter()) + expect_no_lint("x + 1", unreachable_code_linter()) }) test_that("unreachable_code_linter ignores anonymous/inline functions", { - expect_lint("lapply(rnorm(10), function(x) x + 1)", NULL, unreachable_code_linter()) + expect_no_lint("lapply(rnorm(10), function(x) x + 1)", unreachable_code_linter()) }) test_that("unreachable_code_linter passes on multi-line functions", { @@ -212,27 +279,31 @@ test_that("unreachable_code_linter passes on multi-line functions", { return(y) } ") - expect_lint(lines, NULL, unreachable_code_linter()) + expect_no_lint(lines, unreachable_code_linter()) }) -test_that("unreachable_code_linter ignores comments on the same expression", { - lines <- trim_some(" - foo <- function(x) { - return( - y^2 - ) # y^3 - } - ") - expect_lint(lines, NULL, unreachable_code_linter()) +test_that("unreachable_code_linter ignores comments on the same expression", { # nofuzz + linter <- unreachable_code_linter() + + expect_no_lint( + trim_some(" + foo <- function(x) { + return( + y^2 + ) # y^3 + } + "), + linter + ) }) -test_that("unreachable_code_linter ignores comments on the same line", { +test_that("unreachable_code_linter ignores comments on the same line", { # nofuzz lines <- trim_some(" foo <- function(x) { return(y^2) # y^3 } ") - expect_lint(lines, NULL, unreachable_code_linter()) + expect_no_lint(lines, unreachable_code_linter()) }) test_that("unreachable_code_linter identifies simple unreachable code", { @@ -268,7 +339,7 @@ test_that("unreachable_code_linter finds unreachable comments", { ) }) -test_that("unreachable_code_linter finds expressions in the same line", { +test_that("unreachable_code_linter finds expressions in the same line", { # nofuzz msg <- rex::rex("Remove code and comments coming after return() or stop()") linter <- unreachable_code_linter() @@ -349,7 +420,7 @@ test_that("unreachable_code_linter finds code after stop()", { test_that("unreachable_code_linter ignores code after foo$stop(), which might be stopping a subprocess, for example", { linter <- unreachable_code_linter() - expect_lint( + expect_no_lint( trim_some(" foo <- function(x) { bar <- get_process() @@ -357,10 +428,9 @@ test_that("unreachable_code_linter ignores code after foo$stop(), which might be TRUE } "), - NULL, linter ) - expect_lint( + expect_no_lint( trim_some(" foo <- function(x) { bar <- get_process() @@ -368,7 +438,6 @@ test_that("unreachable_code_linter ignores code after foo$stop(), which might be TRUE } "), - NULL, linter ) }) @@ -381,7 +450,7 @@ test_that("unreachable_code_linter ignores terminal nolint end comments", { lintr.exclude_end = "#\\s*TestNoLintEnd" )) - expect_lint( + expect_no_lint( trim_some(" foo <- function() { do_something @@ -391,11 +460,10 @@ test_that("unreachable_code_linter ignores terminal nolint end comments", { # TestNoLintEnd } "), - NULL, list(linter, one_linter = assignment_linter()) ) - expect_lint( + expect_no_lint( trim_some(" foo <- function() { do_something @@ -405,7 +473,6 @@ test_that("unreachable_code_linter ignores terminal nolint end comments", { # TestNoLintEnd } "), - NULL, linter ) }) @@ -593,14 +660,14 @@ test_that("function shorthand is handled", { test_that("Do not lint inline else after stop", { - expect_lint("if (x > 3L) stop() else x + 3", NULL, unreachable_code_linter()) + expect_no_lint("if (x > 3L) stop() else x + 3", unreachable_code_linter()) }) test_that("Do not lint inline else after stop in inline function", { linter <- unreachable_code_linter() - expect_lint("function(x) if (x > 3L) stop() else x + 3", NULL, linter) - expect_lint("function(x) if (x > 3L) { stop() } else {x + 3}", NULL, linter) + expect_no_lint("function(x) if (x > 3L) stop() else x + 3", linter) + expect_no_lint("function(x) if (x > 3L) { stop() } else {x + 3}", linter) }) test_that("Do not lint inline else after stop in inline lambda function", { @@ -608,8 +675,8 @@ test_that("Do not lint inline else after stop in inline lambda function", { linter <- unreachable_code_linter() - expect_lint("\\(x) if (x > 3L) stop() else x + 3", NULL, linter) - expect_lint("\\(x){ if (x > 3L) stop() else x + 3 }", NULL, linter) + expect_no_lint("\\(x) if (x > 3L) stop() else x + 3", linter) + expect_no_lint("\\(x){ if (x > 3L) stop() else x + 3 }", linter) }) test_that("allow_comment_regex= works", { @@ -619,18 +686,17 @@ test_that("allow_comment_regex= works", { linter_xxxx <- unreachable_code_linter(allow_comment_regex = "#.*xxxx") linter_x1x2 <- unreachable_code_linter(allow_comment_regex = c("#x", "#y")) - expect_lint( + expect_no_lint( trim_some(" function() { return(1) # nocov end } "), - NULL, linter_covr ) - expect_lint( + expect_no_lint( trim_some(" function() { return(1) @@ -638,22 +704,20 @@ test_that("allow_comment_regex= works", { # nocov end } "), - NULL, linter_covr ) - expect_lint( + expect_no_lint( trim_some(" function() { return(1) # ABCDxxxx } "), - NULL, linter_xxxx ) - expect_lint( + expect_no_lint( trim_some(" function() { return(1) @@ -661,22 +725,20 @@ test_that("allow_comment_regex= works", { # ABCDxxxx } "), - NULL, linter_xxxx ) - expect_lint( + expect_no_lint( trim_some(" function() { return(1) #x } "), - NULL, linter_x1x2 ) - expect_lint( + expect_no_lint( trim_some(" function() { return(1) @@ -684,12 +746,11 @@ test_that("allow_comment_regex= works", { #yDEF } "), - NULL, linter_x1x2 ) # might contain capture groups, #2678 - expect_lint( + expect_no_lint( trim_some(" function() { stop('a') @@ -697,7 +758,6 @@ test_that("allow_comment_regex= works", { # ab } "), - NULL, unreachable_code_linter(allow_comment_regex = "#\\s*(a|ab|abc)") ) }) @@ -710,18 +770,17 @@ test_that("allow_comment_regex= obeys covr's custom exclusion when set", { linter_covr <- unreachable_code_linter() - expect_lint( + expect_no_lint( trim_some(" function() { return(1) # TestNoCovEnd } "), - NULL, linter_covr ) - expect_lint( + expect_no_lint( trim_some(" function() { return(1) @@ -729,7 +788,6 @@ test_that("allow_comment_regex= obeys covr's custom exclusion when set", { # TestNoCovEnd } "), - NULL, linter_covr ) }) From 58af7c53c9ba70b9ae54afc70392ba74897e2206 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 26 Jul 2025 14:18:04 -0700 Subject: [PATCH 02/10] ws --- tests/testthat/test-unreachable_code_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R index 2a9cf20d40..e619455006 100644 --- a/tests/testthat/test-unreachable_code_linter.R +++ b/tests/testthat/test-unreachable_code_linter.R @@ -73,7 +73,7 @@ test_that("unreachable_code_linter works in sub expressions", { linter ) - expect_lint( + expect_lint( trim_some(" foo <- function(bar) { if (bar) { From 06b8c71c788e6d1719b6405dd0b5e80615f4f412 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 27 Jul 2025 23:17:24 -0700 Subject: [PATCH 03/10] attempt to merge XPaths --- R/unreachable_code_linter.R | 70 ++++++++++++++----------------------- 1 file changed, 26 insertions(+), 44 deletions(-) diff --git a/R/unreachable_code_linter.R b/R/unreachable_code_linter.R index acfdda2d22..16617559ed 100644 --- a/R/unreachable_code_linter.R +++ b/R/unreachable_code_linter.R @@ -76,7 +76,6 @@ #' @seealso [linters] for a complete list of linters available in lintr. #' @export unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclude_end", "# nocov end")) { - # nolint next: object_usage_linter. Used in glue() in statically-difficult fashion to detect. expr_after_control <- " (//REPEAT | //ELSE | //FOR)/following-sibling::expr[1] | (//IF | //WHILE)/following-sibling::expr[2] @@ -96,38 +95,24 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud ][1] " + terminal_fun_expr <- + "(//FUNCTION | //OP-LAMBDA)/following-sibling::expr[OP-LEFT-BRACE][last()]" + # NB: use not(OP-DOLLAR) to prevent matching process$stop(), #1051 - xpath_return_stop_fmt <- " - ( - {expr_after_control} - | - (//FUNCTION | //OP-LAMBDA) - /following-sibling::expr[OP-LEFT-BRACE][last()] - ) - //expr[expr[1][ - not(OP-DOLLAR or OP-AT) - and SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop'] - ]] - /{unreachable_expr_cond} - " - xpath_return_stop <- paste( - glue(xpath_return_stop_fmt, unreachable_expr_cond = unreachable_expr_cond_ws), - glue(xpath_return_stop_fmt, unreachable_expr_cond = unreachable_expr_cond_sc), - sep = " | " - ) - xpath_next_break_fmt <- " - ({expr_after_control}) - //expr[NEXT or BREAK] - /{unreachable_expr_cond} - " - xpath_next_break <- paste( - glue(xpath_next_break_fmt, unreachable_expr_cond = unreachable_expr_cond_ws), - glue(xpath_next_break_fmt, unreachable_expr_cond = unreachable_expr_cond_sc), - sep = " | " - ) + terminal_call_cond <- "expr[1][ + not(OP-DOLLAR or OP-AT) + and SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop'] + ]" + + xpath_after_terminal_node <- glue(" + ({expr_after_control} | {terminal_fun_expr})//expr[{terminal_call_cond}]/{unreachable_expr_cond_ws} + | ({expr_after_control} | {terminal_fun_expr})//expr[{terminal_call_cond}]/{unreachable_expr_cond_sc} + | ({expr_after_control})//expr[NEXT or BREAK]/{unreachable_expr_cond_ws} + | ({expr_after_control})//expr[NEXT or BREAK]/{unreachable_expr_cond_sc} + ") xpath_if_while <- " - (//WHILE | //IF)[following-sibling::expr[1]/NUM_CONST[text() = 'FALSE']] + (//IF | //WHILE)[following-sibling::expr[1]/NUM_CONST[text() = 'FALSE']] /parent::expr " @@ -163,21 +148,18 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud # also build with '|', not rex::rex(or(.)), the latter which will double-escape the regex. allow_comment_regex <- paste(union(allow_comment_regex, settings$exclude_end), collapse = "|") - expr_return_stop <- xml_find_all(xml, xpath_return_stop) - - lints_return_stop <- xml_nodes_to_lints( - drop_valid_comments(expr_return_stop, allow_comment_regex), - source_expression = source_expression, - lint_message = "Remove code and comments coming after return() or stop().", - type = "warning" - ) - - expr_next_break <- xml_find_all(xml, xpath_next_break) + expr_after_terminal_node <- xml_find_all(xml, xpath_after_terminal_node) + terminal_node <- xml_text(xml_find_first(expr_after_terminal_node, " + parent::exprlist//expr/expr[SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop']] + | preceding-sibling::expr/expr[SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop']] + | parent::exprlist//expr[NEXT or BREAK] + | preceding-sibling::expr[NEXT or BREAK] + ")) - lints_next_break <- xml_nodes_to_lints( - drop_valid_comments(expr_next_break, allow_comment_regex), + lints_after_terminal_node <- xml_nodes_to_lints( + drop_valid_comments(expr_after_terminal_node, allow_comment_regex), source_expression = source_expression, - lint_message = "Remove code and comments coming after `next` or `break`.", + lint_message = sprintf("Remove code and comments coming after %s.", terminal_node), type = "warning" ) @@ -199,6 +181,6 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud type = "warning" ) - c(lints_return_stop, lints_next_break, lints_if_while, lints_else) + c(lints_after_terminal_node, lints_if_while, lints_else) }) } From af1e244d7831fa392a20a9c67dd4ba60ca391bef Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 27 Jul 2025 23:21:55 -0700 Subject: [PATCH 04/10] helper not needed --- R/unreachable_code_linter.R | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/R/unreachable_code_linter.R b/R/unreachable_code_linter.R index 16617559ed..1e17546911 100644 --- a/R/unreachable_code_linter.R +++ b/R/unreachable_code_linter.R @@ -135,12 +135,6 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud expr[vapply(expr, xml2::xml_length, integer(1L)) != 0L] } - drop_valid_comments <- function(expr, valid_comment_re) { - is_valid_comment <- xml2::xml_name(expr) == "COMMENT" & - re_matches_logical(xml_text(expr), valid_comment_re) - expr[!is_valid_comment] - } - Linter(linter_level = "expression", function(source_expression) { xml <- source_expression$xml_parsed_content @@ -149,6 +143,11 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud allow_comment_regex <- paste(union(allow_comment_regex, settings$exclude_end), collapse = "|") expr_after_terminal_node <- xml_find_all(xml, xpath_after_terminal_node) + + is_valid_comment <- xml2::xml_name(expr_after_terminal_node) == "COMMENT" & + re_matches_logical(xml_text(expr_after_terminal_node), valid_comment_re) + + expr_after_terminal_node <- expr_after_terminal_node[!is_valid_comment] terminal_node <- xml_text(xml_find_first(expr_after_terminal_node, " parent::exprlist//expr/expr[SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop']] | preceding-sibling::expr/expr[SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop']] @@ -157,7 +156,7 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud ")) lints_after_terminal_node <- xml_nodes_to_lints( - drop_valid_comments(expr_after_terminal_node, allow_comment_regex), + expr_after_terminal_node, source_expression = source_expression, lint_message = sprintf("Remove code and comments coming after %s.", terminal_node), type = "warning" From 43db3a97acc1c7f4bc5643bdc058a21c57763e7a Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 27 Jul 2025 23:23:57 -0700 Subject: [PATCH 05/10] helper rename of variable --- R/unreachable_code_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/unreachable_code_linter.R b/R/unreachable_code_linter.R index 1e17546911..c99f6c5fd8 100644 --- a/R/unreachable_code_linter.R +++ b/R/unreachable_code_linter.R @@ -145,7 +145,7 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud expr_after_terminal_node <- xml_find_all(xml, xpath_after_terminal_node) is_valid_comment <- xml2::xml_name(expr_after_terminal_node) == "COMMENT" & - re_matches_logical(xml_text(expr_after_terminal_node), valid_comment_re) + re_matches_logical(xml_text(expr_after_terminal_node), allow_comment_regex) expr_after_terminal_node <- expr_after_terminal_node[!is_valid_comment] terminal_node <- xml_text(xml_find_first(expr_after_terminal_node, " From 75ce8c4892b3abb14c818d76a49d88484fb07a5e Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 27 Jul 2025 23:31:34 -0700 Subject: [PATCH 06/10] Kneading tests --- R/unreachable_code_linter.R | 2 ++ tests/testthat/test-unreachable_code_linter.R | 20 +++++++++---------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/R/unreachable_code_linter.R b/R/unreachable_code_linter.R index c99f6c5fd8..ecbae1589b 100644 --- a/R/unreachable_code_linter.R +++ b/R/unreachable_code_linter.R @@ -154,6 +154,8 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud | parent::exprlist//expr[NEXT or BREAK] | preceding-sibling::expr[NEXT or BREAK] ")) + terminal_node <- + ifelse(terminal_node %in% c("return", "stop"), paste0(terminal_node, "()"), paste0("`", terminal_node, "`")) lints_after_terminal_node <- xml_nodes_to_lints( expr_after_terminal_node, diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R index e619455006..fff9909c66 100644 --- a/tests/testthat/test-unreachable_code_linter.R +++ b/tests/testthat/test-unreachable_code_linter.R @@ -9,7 +9,7 @@ test_that("unreachable_code_linter works in simple function", { test_that("unreachable_code_linter works in sub expressions", { linter <- unreachable_code_linter() - msg <- rex::rex("Remove code and comments coming after return() or stop()") + msg <- rex::rex("Remove code and comments coming after return()") lines <- trim_some(" foo <- function(bar) { @@ -139,7 +139,7 @@ test_that("unreachable_code_linter works in sub expressions", { test_that("unreachable_code_linter works with next and break in sub expressions", { linter <- unreachable_code_linter() - msg <- rex::rex("Remove code and comments coming after `next` or `break`") + msg <- rex::rex("Remove code and comments coming after `next`") lines <- trim_some(" foo <- function(bar) { @@ -318,7 +318,7 @@ test_that("unreachable_code_linter identifies simple unreachable code", { lines, list( line_number = 3L, - message = rex::rex("Remove code and comments coming after return() or stop()") + message = rex::rex("Remove code and comments coming after return()") ), unreachable_code_linter() ) @@ -334,13 +334,13 @@ test_that("unreachable_code_linter finds unreachable comments", { ") expect_lint( lines, - rex::rex("Remove code and comments coming after return() or stop()"), + rex::rex("Remove code and comments coming after return()"), unreachable_code_linter() ) }) test_that("unreachable_code_linter finds expressions in the same line", { # nofuzz - msg <- rex::rex("Remove code and comments coming after return() or stop()") + msg <- rex::rex("Remove code and comments coming after return()") linter <- unreachable_code_linter() lines <- trim_some(" @@ -368,7 +368,7 @@ test_that("unreachable_code_linter finds expressions in the same line", { # nofu }) test_that("unreachable_code_linter finds expressions and comments after comment in return line", { - msg <- rex::rex("Remove code and comments coming after return() or stop()") + msg <- rex::rex("Remove code and comments coming after return()") linter <- unreachable_code_linter() lines <- trim_some(" @@ -397,7 +397,7 @@ test_that("unreachable_code_linter finds a double return", { ") expect_lint( lines, - rex::rex("Remove code and comments coming after return() or stop()"), + rex::rex("Remove code and comments coming after return()"), unreachable_code_linter() ) }) @@ -412,7 +412,7 @@ test_that("unreachable_code_linter finds code after stop()", { ") expect_lint( lines, - rex::rex("Remove code and comments coming after return() or stop()"), + rex::rex("Remove code and comments coming after return()"), unreachable_code_linter() ) }) @@ -621,7 +621,7 @@ test_that("unreachable_code_linter identifies unreachable code in mixed conditio list(false_msg, line_number = 2L), list(false_msg, line_number = 5L), list(true_msg, line_number = 10L), - list(rex::rex("Remove code and comments coming after return() or stop()."), line_number = 13L) + list(rex::rex("Remove code and comments coming after stop()."), line_number = 13L) ), linter ) @@ -652,7 +652,7 @@ test_that("function shorthand is handled", { "), list( line_number = 3L, - message = rex::rex("Remove code and comments coming after return() or stop()") + message = rex::rex("Remove code and comments coming after return()") ), unreachable_code_linter() ) From 170cc635a0201d6b1d7be243bda39668064959f6 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 27 Jul 2025 23:34:21 -0700 Subject: [PATCH 07/10] simpler wording fix --- tests/testthat/test-unreachable_code_linter.R | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R index fff9909c66..a3dff624ba 100644 --- a/tests/testthat/test-unreachable_code_linter.R +++ b/tests/testthat/test-unreachable_code_linter.R @@ -139,7 +139,8 @@ test_that("unreachable_code_linter works in sub expressions", { test_that("unreachable_code_linter works with next and break in sub expressions", { linter <- unreachable_code_linter() - msg <- rex::rex("Remove code and comments coming after `next`") + next_msg <- rex::rex("Remove code and comments coming after `next`") + break_msg <- rex::rex("Remove code and comments coming after `break`") lines <- trim_some(" foo <- function(bar) { @@ -168,11 +169,11 @@ test_that("unreachable_code_linter works with next and break in sub expressions" expect_lint( lines, list( - list(line_number = 4L, message = msg), - list(line_number = 7L, message = msg), - list(line_number = 10L, message = msg), - list(line_number = 15L, message = msg), - list(line_number = 18L, message = msg) + list(line_number = 4L, message = next_msg), + list(line_number = 7L, message = break_msg), + list(line_number = 10L, message = next_msg), + list(line_number = 15L, message = next_msg), + list(line_number = 18L, message = break_msg) ), linter ) @@ -219,11 +220,11 @@ test_that("unreachable_code_linter works with next and break in sub expressions" } "), list( - list(line_number = 3L, message = msg), - list(line_number = 5L, message = msg), - list(line_number = 8L, message = msg), - list(line_number = 11L, message = msg), - list(line_number = 14L, message = msg) + list(line_number = 3L, message = next_msg), + list(line_number = 5L, message = break_msg), + list(line_number = 8L, message = break_msg), + list(line_number = 11L, message = next_msg), + list(line_number = 14L, message = break_msg) ), linter ) @@ -412,7 +413,7 @@ test_that("unreachable_code_linter finds code after stop()", { ") expect_lint( lines, - rex::rex("Remove code and comments coming after return()"), + rex::rex("Remove code and comments coming after stop()"), unreachable_code_linter() ) }) From 47835c42ffce5cb689e49039df5841da84c8d9a7 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 27 Jul 2025 23:35:34 -0700 Subject: [PATCH 08/10] one more --- tests/testthat/test-unreachable_code_linter.R | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R index a3dff624ba..8d0f2cc3f1 100644 --- a/tests/testthat/test-unreachable_code_linter.R +++ b/tests/testthat/test-unreachable_code_linter.R @@ -255,11 +255,11 @@ test_that("unreachable_code_linter works with next and break in sub expressions" } "), list( - list(line_number = 4L, message = msg), - list(line_number = 7L, message = msg), - list(line_number = 11L, message = msg), - list(line_number = 15L, message = msg), - list(line_number = 19L, message = msg) + list(line_number = 4L, message = next_msg), + list(line_number = 7L, message = break_msg), + list(line_number = 11L, message = break_msg), + list(line_number = 15L, message = next_msg), + list(line_number = 19L, message = break_msg) ), linter ) From 4c19295b518d87249ff58a8fb4886f25627b0fe5 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 27 Jul 2025 23:41:10 -0700 Subject: [PATCH 09/10] preceding-sibling, not parent --- R/unreachable_code_linter.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/unreachable_code_linter.R b/R/unreachable_code_linter.R index ecbae1589b..35d4aa443c 100644 --- a/R/unreachable_code_linter.R +++ b/R/unreachable_code_linter.R @@ -149,9 +149,9 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud expr_after_terminal_node <- expr_after_terminal_node[!is_valid_comment] terminal_node <- xml_text(xml_find_first(expr_after_terminal_node, " - parent::exprlist//expr/expr[SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop']] + preceding-sibling::exprlist//expr/expr[SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop']] | preceding-sibling::expr/expr[SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop']] - | parent::exprlist//expr[NEXT or BREAK] + | preceding-sibling::exprlist//expr[NEXT or BREAK] | preceding-sibling::expr[NEXT or BREAK] ")) terminal_node <- From bceb97788f0d22b424cf301d06af78bdf0ae85f0 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 27 Jul 2025 23:46:14 -0700 Subject: [PATCH 10/10] simplify, extract to factory --- R/unreachable_code_linter.R | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/R/unreachable_code_linter.R b/R/unreachable_code_linter.R index 35d4aa443c..155f49216b 100644 --- a/R/unreachable_code_linter.R +++ b/R/unreachable_code_linter.R @@ -111,6 +111,15 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud | ({expr_after_control})//expr[NEXT or BREAK]/{unreachable_expr_cond_sc} ") + xpath_terminal_node <- " + (preceding-sibling::exprlist//expr | preceding-sibling::expr) + /*[ + self::expr/SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop'] + or self::NEXT + or self::BREAK + ] + " + xpath_if_while <- " (//IF | //WHILE)[following-sibling::expr[1]/NUM_CONST[text() = 'FALSE']] /parent::expr @@ -148,12 +157,7 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud re_matches_logical(xml_text(expr_after_terminal_node), allow_comment_regex) expr_after_terminal_node <- expr_after_terminal_node[!is_valid_comment] - terminal_node <- xml_text(xml_find_first(expr_after_terminal_node, " - preceding-sibling::exprlist//expr/expr[SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop']] - | preceding-sibling::expr/expr[SYMBOL_FUNCTION_CALL[text() = 'return' or text() = 'stop']] - | preceding-sibling::exprlist//expr[NEXT or BREAK] - | preceding-sibling::expr[NEXT or BREAK] - ")) + terminal_node <- xml_text(xml_find_first(expr_after_terminal_node, xpath_terminal_node)) terminal_node <- ifelse(terminal_node %in% c("return", "stop"), paste0(terminal_node, "()"), paste0("`", terminal_node, "`"))