diff --git a/R/unreachable_code_linter.R b/R/unreachable_code_linter.R index f2e9f8d56..155f49216 100644 --- a/R/unreachable_code_linter.R +++ b/R/unreachable_code_linter.R @@ -80,32 +80,48 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud (//REPEAT | //ELSE | //FOR)/following-sibling::expr[1] | (//IF | //WHILE)/following-sibling::expr[2] " - # NB: use not(OP-DOLLAR) to prevent matching process$stop(), #1051 - xpath_return_stop <- glue(" - ( - {expr_after_control} - | (//FUNCTION | //OP-LAMBDA)[following-sibling::expr[1]/*[1][self::OP-LEFT-BRACE]]/following-sibling::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(" - ({expr_after_control}) - /expr[NEXT or BREAK] + + 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 or self::OP-SEMICOLON) - and (not(self::COMMENT) or @line2 > preceding-sibling::*[1]/@line2) + not(self::OP-RIGHT-BRACE) + and (not(self::COMMENT) or @line1 > preceding-sibling::exprlist/expr/@line2) ][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 + 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_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 <- " - (//WHILE | //IF)[following-sibling::expr[1]/NUM_CONST[text() = 'FALSE']] + (//IF | //WHILE)[following-sibling::expr[1]/NUM_CONST[text() = 'FALSE']] /parent::expr " @@ -128,12 +144,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 @@ -141,21 +151,20 @@ 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) + expr_after_terminal_node <- xml_find_all(xml, xpath_after_terminal_node) - 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" - ) + is_valid_comment <- xml2::xml_name(expr_after_terminal_node) == "COMMENT" & + re_matches_logical(xml_text(expr_after_terminal_node), allow_comment_regex) - expr_next_break <- xml_find_all(xml, xpath_next_break) + expr_after_terminal_node <- expr_after_terminal_node[!is_valid_comment] + 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, "`")) - lints_next_break <- xml_nodes_to_lints( - drop_valid_comments(expr_next_break, allow_comment_regex), + lints_after_terminal_node <- xml_nodes_to_lints( + expr_after_terminal_node, 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" ) @@ -177,6 +186,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) }) } diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R index b54d3b11e..8d0f2cc3f 100644 --- a/tests/testthat/test-unreachable_code_linter.R +++ b/tests/testthat/test-unreachable_code_linter.R @@ -4,12 +4,12 @@ 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", { 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) { @@ -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, + 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,11 +101,46 @@ 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", { linter <- unreachable_code_linter() - msg <- rex::rex("Remove code and comments coming after `next` or `break`") + 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) { @@ -135,74 +169,108 @@ 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 ) - 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_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 + } } - } - ") - - expect_lint(lines, NULL, linter) + "), + 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_lint( + 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 = 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 + ) + # also with comments expect_lint( - lines, + 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 = 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 = 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 ) }) 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 +280,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", { @@ -247,7 +319,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() ) @@ -263,13 +335,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", { - msg <- rex::rex("Remove code and comments coming after return() or stop()") +test_that("unreachable_code_linter finds expressions in the same line", { # nofuzz + msg <- rex::rex("Remove code and comments coming after return()") linter <- unreachable_code_linter() lines <- trim_some(" @@ -297,7 +369,7 @@ test_that("unreachable_code_linter finds expressions in the same line", { }) 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(" @@ -326,7 +398,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() ) }) @@ -341,7 +413,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 stop()"), unreachable_code_linter() ) }) @@ -349,7 +421,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 +429,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 +439,6 @@ test_that("unreachable_code_linter ignores code after foo$stop(), which might be TRUE } "), - NULL, linter ) }) @@ -381,7 +451,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 +461,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 +474,6 @@ test_that("unreachable_code_linter ignores terminal nolint end comments", { # TestNoLintEnd } "), - NULL, linter ) }) @@ -554,7 +622,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 ) @@ -585,7 +653,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() ) @@ -593,14 +661,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 +676,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 +687,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 +705,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 +726,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 +747,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 +759,6 @@ test_that("allow_comment_regex= works", { # ab } "), - NULL, unreachable_code_linter(allow_comment_regex = "#\\s*(a|ab|abc)") ) }) @@ -710,18 +771,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 +789,6 @@ test_that("allow_comment_regex= obeys covr's custom exclusion when set", { # TestNoCovEnd } "), - NULL, linter_covr ) })