Skip to content

Handle comments in unreachable_code_linter #2900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Handle comments in unreachable_code_linter #2900

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f570211
handle comments
MichaelChirico Jul 26, 2025
58af7c5
ws
MichaelChirico Jul 26, 2025
06b8c71
attempt to merge XPaths
MichaelChirico Jul 28, 2025
af1e244
helper not needed
MichaelChirico Jul 28, 2025
43db3a9
helper rename of variable
MichaelChirico Jul 28, 2025
75ce8c4
Kneading tests
MichaelChirico Jul 28, 2025
170cc63
simpler wording fix
MichaelChirico Jul 28, 2025
47835c4
one more
MichaelChirico Jul 28, 2025
4c19295
preceding-sibling, not parent
MichaelChirico Jul 28, 2025
bceb977
simplify, extract to factory
MichaelChirico Jul 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 37 additions & 15 deletions R/unreachable_code_linter.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

@AshesITR AshesITR Jul 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can both remove the lint and break up the code into more digestible parts by only glueing once like

glue::glue("
  ({expr_after_control} | {last_function_call})/{unreachable_expr_ws} |
  ({expr_after_control} | {last_function_call})/{unreachable_expr_sc} |
  ({expr_after_control})/{next_or_break}/{unreachable_expr_ws} |
  ({expr_after_control})/{next_or_break}/{unreachable_expr_sc}
")

WDYT?

Copy link
Collaborator Author

@MichaelChirico MichaelChirico Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I did what you had in mind -- merge the logic for the [return/stop] and [next/break] cases, then use lint customization to give the right message in every case. That includes improving the customization beyond what existed already to mention return() / stop() specifically.

Copy link
Collaborator Author

@MichaelChirico MichaelChirico Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking back :)

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 <exprlist> 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']]
Expand Down
Loading
Loading