From fa92c47079a22989b636dc67d91b6534c0470b55 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 14 Dec 2023 14:45:50 +0800 Subject: [PATCH 01/11] Split comment_linters into two separate files for each linter (#2437) * split out todo * split out commented_code * remove todo_comment_linter * remove commented_code_linter * restore --- DESCRIPTION | 3 +- ...ment_linters.R => commented_code_linter.R} | 118 ++++-------------- R/todo_comment_linter.R | 64 ++++++++++ man/commented_code_linter.Rd | 2 +- man/todo_comment_linter.Rd | 2 +- 5 files changed, 94 insertions(+), 95 deletions(-) rename R/{comment_linters.R => commented_code_linter.R} (55%) create mode 100644 R/todo_comment_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 8feb6297e..987e74b42 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -74,7 +74,7 @@ Collate: 'cache.R' 'class_equals_linter.R' 'commas_linter.R' - 'comment_linters.R' + 'commented_code_linter.R' 'comments.R' 'comparison_negation_linter.R' 'condition_call_linter.R' @@ -184,6 +184,7 @@ Collate: 'strings_as_factors_linter.R' 'system_file_linter.R' 'terminal_close_linter.R' + 'todo_comment_linter.R' 'trailing_blank_lines_linter.R' 'trailing_whitespace_linter.R' 'tree_utils.R' diff --git a/R/comment_linters.R b/R/commented_code_linter.R similarity index 55% rename from R/comment_linters.R rename to R/commented_code_linter.R index cc8ab06c2..fcc4af9ef 100644 --- a/R/comment_linters.R +++ b/R/commented_code_linter.R @@ -1,29 +1,3 @@ -ops <- list( - "+", - # "-", - "=", - "==", - "!=", - "<=", - ">=", - "<-", - "<<-", - "<", - ">", - "->", - "->>", - "%%", - "/", - "^", - "*", - "**", - "|", - "||", - "&", - "&&", - rex("%", except_any_of("%"), "%") -) - #' Commented code linter #' #' Check that there is no commented code outside roxygen blocks. @@ -60,6 +34,32 @@ ops <- list( #' @seealso [linters] for a complete list of linters available in lintr. #' @export commented_code_linter <- function() { + ops <- list( + "+", + # "-", + "=", + "==", + "!=", + "<=", + ">=", + "<-", + "<<-", + "<", + ">", + "->", + "->>", + "%%", + "/", + "^", + "*", + "**", + "|", + "||", + "&", + "&&", + rex("%", except_any_of("%"), "%") + ) + code_candidate_regex <- rex( some_of("#"), any_spaces, @@ -117,69 +117,3 @@ parsable <- function(x) { res <- try_silently(parse(text = x)) !inherits(res, "try-error") } - - -#' TODO comment linter -#' -#' Check that the source contains no TODO comments (case-insensitive). -#' -#' @param todo Vector of strings that identify TODO comments. -#' -#' @examples -#' # will produce lints -#' lint( -#' text = "x + y # TODO", -#' linters = todo_comment_linter() -#' ) -#' -#' lint( -#' text = "pi <- 1.0 # FIXME", -#' linters = todo_comment_linter() -#' ) -#' -#' lint( -#' text = "x <- TRUE # hack", -#' linters = todo_comment_linter(todo = c("todo", "fixme", "hack")) -#' ) -#' -#' # okay -#' lint( -#' text = "x + y # my informative comment", -#' linters = todo_comment_linter() -#' ) -#' -#' lint( -#' text = "pi <- 3.14", -#' linters = todo_comment_linter() -#' ) -#' -#' lint( -#' text = "x <- TRUE", -#' linters = todo_comment_linter() -#' ) -#' -#' @evalRd rd_tags("todo_comment_linter") -#' @seealso [linters] for a complete list of linters available in lintr. -#' @export -todo_comment_linter <- function(todo = c("todo", "fixme")) { - todo_comment_regex <- rex(one_or_more("#"), any_spaces, or(todo)) - Linter(function(source_expression) { - tokens <- with_id(source_expression, ids_with_token(source_expression, "COMMENT")) - are_todo <- re_matches(tokens[["text"]], todo_comment_regex, ignore.case = TRUE) - tokens <- tokens[are_todo, ] - lapply( - split(tokens, seq_len(nrow(tokens))), - function(token) { - Lint( - filename = source_expression[["filename"]], - line_number = token[["line1"]], - column_number = token[["col1"]], - type = "style", - message = "Remove TODO comments.", - line = source_expression[["lines"]][[as.character(token[["line1"]])]], - ranges = list(c(token[["col1"]], token[["col2"]])) - ) - } - ) - }) -} diff --git a/R/todo_comment_linter.R b/R/todo_comment_linter.R new file mode 100644 index 000000000..68225d80d --- /dev/null +++ b/R/todo_comment_linter.R @@ -0,0 +1,64 @@ +#' TODO comment linter +#' +#' Check that the source contains no TODO comments (case-insensitive). +#' +#' @param todo Vector of strings that identify TODO comments. +#' +#' @examples +#' # will produce lints +#' lint( +#' text = "x + y # TODO", +#' linters = todo_comment_linter() +#' ) +#' +#' lint( +#' text = "pi <- 1.0 # FIXME", +#' linters = todo_comment_linter() +#' ) +#' +#' lint( +#' text = "x <- TRUE # hack", +#' linters = todo_comment_linter(todo = c("todo", "fixme", "hack")) +#' ) +#' +#' # okay +#' lint( +#' text = "x + y # my informative comment", +#' linters = todo_comment_linter() +#' ) +#' +#' lint( +#' text = "pi <- 3.14", +#' linters = todo_comment_linter() +#' ) +#' +#' lint( +#' text = "x <- TRUE", +#' linters = todo_comment_linter() +#' ) +#' +#' @evalRd rd_tags("todo_comment_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +todo_comment_linter <- function(todo = c("todo", "fixme")) { + todo_comment_regex <- rex(one_or_more("#"), any_spaces, or(todo)) + Linter(function(source_expression) { + tokens <- with_id(source_expression, ids_with_token(source_expression, "COMMENT")) + are_todo <- re_matches(tokens[["text"]], todo_comment_regex, ignore.case = TRUE) + tokens <- tokens[are_todo, ] + lapply( + split(tokens, seq_len(nrow(tokens))), + function(token) { + Lint( + filename = source_expression[["filename"]], + line_number = token[["line1"]], + column_number = token[["col1"]], + type = "style", + message = "Remove TODO comments.", + line = source_expression[["lines"]][[as.character(token[["line1"]])]], + ranges = list(c(token[["col1"]], token[["col2"]])) + ) + } + ) + }) +} diff --git a/man/commented_code_linter.Rd b/man/commented_code_linter.Rd index 23c7616eb..49f29c057 100644 --- a/man/commented_code_linter.Rd +++ b/man/commented_code_linter.Rd @@ -1,5 +1,5 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/comment_linters.R +% Please edit documentation in R/commented_code_linter.R \name{commented_code_linter} \alias{commented_code_linter} \title{Commented code linter} diff --git a/man/todo_comment_linter.Rd b/man/todo_comment_linter.Rd index 0dfa16d64..5b12d7b58 100644 --- a/man/todo_comment_linter.Rd +++ b/man/todo_comment_linter.Rd @@ -1,5 +1,5 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/comment_linters.R +% Please edit documentation in R/todo_comment_linter.R \name{todo_comment_linter} \alias{todo_comment_linter} \title{TODO comment linter} From 82efd40fd124e01a153790a21d0b2055ec06cbf0 Mon Sep 17 00:00:00 2001 From: Hugo Gruson Date: Thu, 14 Dec 2023 09:48:27 +0000 Subject: [PATCH 02/11] Mention and link to correct fct in use_lintr() (#2440) Co-authored-by: Michael Chirico --- R/use_lintr.R | 2 +- man/use_lintr.Rd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/use_lintr.R b/R/use_lintr.R index 829adf3d7..d72711b5e 100644 --- a/R/use_lintr.R +++ b/R/use_lintr.R @@ -8,7 +8,7 @@ #' #' * `tidyverse` creates a minimal lintr config, based on the default linters ([linters_with_defaults()]). #' These are suitable for following [the tidyverse style guide](https://style.tidyverse.org/). -#' * `full` creates a lintr config using all available linters via [linters_with_tags()]. +#' * `full` creates a lintr config using all available linters via [all_linters()]. #' #' @return Path to the generated configuration, invisibly. #' diff --git a/man/use_lintr.Rd b/man/use_lintr.Rd index 701e1042c..c4cf98b74 100644 --- a/man/use_lintr.Rd +++ b/man/use_lintr.Rd @@ -14,7 +14,7 @@ If the \code{.lintr} file already exists, an error will be thrown.} \itemize{ \item \code{tidyverse} creates a minimal lintr config, based on the default linters (\code{\link[=linters_with_defaults]{linters_with_defaults()}}). These are suitable for following \href{https://style.tidyverse.org/}{the tidyverse style guide}. -\item \code{full} creates a lintr config using all available linters via \code{\link[=linters_with_tags]{linters_with_tags()}}. +\item \code{full} creates a lintr config using all available linters via \code{\link[=all_linters]{all_linters()}}. }} } \value{ From 6cbb24c76a2010f6440bd9d8a4672eb922d4177b Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Thu, 14 Dec 2023 16:58:19 +0100 Subject: [PATCH 03/11] Minor improvement to `condition_call_linter()` docs (#2445) --- R/condition_call_linter.R | 6 +++--- man/condition_call_linter.Rd | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/R/condition_call_linter.R b/R/condition_call_linter.R index 983556869..8b13c8a92 100644 --- a/R/condition_call_linter.R +++ b/R/condition_call_linter.R @@ -6,10 +6,10 @@ #' #' @param display_call Logical specifying expected behaviour regarding `call.` #' argument in conditions. -#' - `NA` forces providing `call.=` but ignores its value (this can be used in +#' - `NA` forces providing `call. =` but ignores its value (this can be used in #' cases where you expect a mix of `call. = FALSE` and `call. = TRUE`) -#' - lints `call. = FALSE` -#' - forces `call. = FALSE` (lints `call. = TRUE` or missing `call.=` value) +#' - `TRUE` lints `call. = FALSE` +#' - `FALSE` forces `call. = FALSE` (lints `call. = TRUE` or missing `call. =` value) #' #' #' @examples diff --git a/man/condition_call_linter.Rd b/man/condition_call_linter.Rd index 73302b7fe..a3b3ca7eb 100644 --- a/man/condition_call_linter.Rd +++ b/man/condition_call_linter.Rd @@ -10,10 +10,10 @@ condition_call_linter(display_call = FALSE) \item{display_call}{Logical specifying expected behaviour regarding \code{call.} argument in conditions. \itemize{ -\item \code{NA} forces providing \verb{call.=} but ignores its value (this can be used in +\item \code{NA} forces providing \verb{call. =} but ignores its value (this can be used in cases where you expect a mix of \code{call. = FALSE} and \code{call. = TRUE}) -\item lints \code{call. = FALSE} -\item forces \code{call. = FALSE} (lints \code{call. = TRUE} or missing \verb{call.=} value) +\item \code{TRUE} lints \code{call. = FALSE} +\item \code{FALSE} forces \code{call. = FALSE} (lints \code{call. = TRUE} or missing \verb{call. =} value) }} } \description{ From f865f94b220421d16bdf7bc59184f1c6b3c91c76 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 15 Dec 2023 14:05:29 +0800 Subject: [PATCH 04/11] Convert todo_comment_linter to XPath approach (#2438) --- R/todo_comment_linter.R | 31 +++++++++++++------------------ man/todo_comment_linter.Rd | 2 +- 2 files changed, 14 insertions(+), 19 deletions(-) diff --git a/R/todo_comment_linter.R b/R/todo_comment_linter.R index 68225d80d..8b7169bae 100644 --- a/R/todo_comment_linter.R +++ b/R/todo_comment_linter.R @@ -2,7 +2,7 @@ #' #' Check that the source contains no TODO comments (case-insensitive). #' -#' @param todo Vector of strings that identify TODO comments. +#' @param todo Vector of case-insensitive strings that identify TODO comments. #' #' @examples #' # will produce lints @@ -42,23 +42,18 @@ #' @export todo_comment_linter <- function(todo = c("todo", "fixme")) { todo_comment_regex <- rex(one_or_more("#"), any_spaces, or(todo)) - Linter(function(source_expression) { - tokens <- with_id(source_expression, ids_with_token(source_expression, "COMMENT")) - are_todo <- re_matches(tokens[["text"]], todo_comment_regex, ignore.case = TRUE) - tokens <- tokens[are_todo, ] - lapply( - split(tokens, seq_len(nrow(tokens))), - function(token) { - Lint( - filename = source_expression[["filename"]], - line_number = token[["line1"]], - column_number = token[["col1"]], - type = "style", - message = "Remove TODO comments.", - line = source_expression[["lines"]][[as.character(token[["line1"]])]], - ranges = list(c(token[["col1"]], token[["col2"]])) - ) - } + + Linter(linter_level = "expression", function(source_expression) { + xml <- source_expression$xml_parsed_content + + comment_expr <- xml_find_all(xml, "//COMMENT") + are_todo <- re_matches(xml_text(comment_expr), todo_comment_regex, ignore.case = TRUE) + + xml_nodes_to_lints( + comment_expr[are_todo], + source_expression = source_expression, + lint_message = "Remove TODO comments.", + type = "style" ) }) } diff --git a/man/todo_comment_linter.Rd b/man/todo_comment_linter.Rd index 5b12d7b58..e2c92aa68 100644 --- a/man/todo_comment_linter.Rd +++ b/man/todo_comment_linter.Rd @@ -7,7 +7,7 @@ todo_comment_linter(todo = c("todo", "fixme")) } \arguments{ -\item{todo}{Vector of strings that identify TODO comments.} +\item{todo}{Vector of case-insensitive strings that identify TODO comments.} } \description{ Check that the source contains no TODO comments (case-insensitive). From ca1b16d5301b266d791de69290235d0f61c7b3c5 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 15 Dec 2023 15:13:39 +0800 Subject: [PATCH 05/11] catch NA %in% x (#2436) Co-authored-by: AshesITR --- NEWS.md | 1 + R/any_is_na_linter.R | 21 +++++++++++++++++---- tests/testthat/test-any_is_na_linter.R | 20 +++++++++++++++++--- 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/NEWS.md b/NEWS.md index dde5f6c48..529ef7752 100644 --- a/NEWS.md +++ b/NEWS.md @@ -41,6 +41,7 @@ * `unreachable_code_linter()` has an argument `allow_comment_regex` for customizing which "terminal" comments to exclude (#2327, @MichaelChirico). `# nolint end` comments are always excluded, as are {covr} exclusions (e.g. `# nocov end`) by default. * `format()` and `print()` methods for `lint` and `lints` classes get a new option `width` to control the printing width of lint messages (#1884, @MichaelChirico). The default is controlled by a new option `lintr.format_width`; if unset, no wrapping occurs (matching earlier behavior). * New function node caching for big efficiency gains to most linters (e.g. overall `lint_package()` improvement of 14-27% and core linting improvement up to 30%; #2357, @AshesITR). Most linters are written around function usage, and XPath performance searching for many functions is poor. The new `xml_find_function_calls()` entry in the `get_source_expressions()` output caches all function call nodes instead. See the vignette on creating linters for more details on how to use it. +* `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico). ### New linters diff --git a/R/any_is_na_linter.R b/R/any_is_na_linter.R index a0ea91e33..5aa1a0cad 100644 --- a/R/any_is_na_linter.R +++ b/R/any_is_na_linter.R @@ -36,7 +36,7 @@ #' @seealso [linters] for a complete list of linters available in lintr. #' @export any_is_na_linter <- function() { - xpath <- " + any_xpath <- " parent::expr /following-sibling::expr[1][expr[1][SYMBOL_FUNCTION_CALL[text() = 'is.na']]] /parent::expr[ @@ -45,15 +45,28 @@ any_is_na_linter <- function() { ] " + in_xpath <- "//SPECIAL[text() = '%in%']/preceding-sibling::expr[NUM_CONST[starts-with(text(), 'NA')]]" + Linter(linter_level = "expression", function(source_expression) { + xml <- source_expression$xml_parsed_content xml_calls <- source_expression$xml_find_function_calls("any") - bad_expr <- xml_find_all(xml_calls, xpath) - xml_nodes_to_lints( - bad_expr, + any_expr <- xml_find_all(xml_calls, any_xpath) + any_lints <- xml_nodes_to_lints( + any_expr, source_expression = source_expression, lint_message = "anyNA(x) is better than any(is.na(x)).", type = "warning" ) + + in_expr <- xml_find_all(xml, in_xpath) + in_lints <- xml_nodes_to_lints( + in_expr, + source_expression = source_expression, + lint_message = "anyNA(x) is better than NA %in% x.", + type = "warning" + ) + + c(any_lints, in_lints) }) } diff --git a/tests/testthat/test-any_is_na_linter.R b/tests/testthat/test-any_is_na_linter.R index 4873e1611..398ca53cc 100644 --- a/tests/testthat/test-any_is_na_linter.R +++ b/tests/testthat/test-any_is_na_linter.R @@ -26,17 +26,31 @@ test_that("any_is_na_linter blocks simple disallowed usages", { expect_lint("foo(any(is.na(x)))", lint_message, linter) }) +test_that("NA %in% x is also found", { + linter <- any_is_na_linter() + lint_message <- rex::rex("anyNA(x) is better than NA %in% x.") + + expect_lint("NA %in% x", lint_message, linter) + expect_lint("NA_real_ %in% x", lint_message, linter) + expect_lint("NA_not_a_sentinel_ %in% x", NULL, linter) +}) + test_that("lints vectorize", { - lint_message <- rex::rex("anyNA(x) is better than any(is.na(x)).") + any_message <- rex::rex("any(is.na(x))") + in_message <- rex::rex("NA %in% x") expect_lint( trim_some("{ any(is.na(foo(x))) any(is.na(y), na.rm = TRUE) + NA %in% a + NA_complex_ %in% b }"), list( - list(lint_message, line_number = 2L), - list(lint_message, line_number = 3L) + list(any_message, line_number = 2L), + list(any_message, line_number = 3L), + list(in_message, line_number = 4L), + list(in_message, line_number = 5L) ), any_is_na_linter() ) From 267a9c8b819032d7235a18b6cfc253afbdaabb9c Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 15 Dec 2023 15:41:05 +0800 Subject: [PATCH 06/11] pipeline support (#2426) Co-authored-by: AshesITR Co-authored-by: Indrajeet Patil --- DESCRIPTION | 2 +- NEWS.md | 2 +- R/nrow_subset_linter.R | 18 ++++++++++++------ tests/testthat/test-nrow_subset_linter.R | 11 +++++++++++ 4 files changed, 25 insertions(+), 8 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 987e74b42..f00ad990c 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -141,6 +141,7 @@ Collate: 'nested_ifelse_linter.R' 'nested_pipe_linter.R' 'nonportable_path_linter.R' + 'shared_constants.R' 'nrow_subset_linter.R' 'numeric_leading_zero_linter.R' 'nzchar_linter.R' @@ -173,7 +174,6 @@ Collate: 'seq_linter.R' 'settings.R' 'settings_utils.R' - 'shared_constants.R' 'sort_linter.R' 'source_utils.R' 'spaces_inside_linter.R' diff --git a/NEWS.md b/NEWS.md index 529ef7752..fa97f51f2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -59,7 +59,7 @@ * `consecutive_mutate_linter()` for encouraging consecutive calls to `dplyr::mutate()` to be combined (part of #884, @MichaelChirico). * `if_switch_linter()` for encouraging `switch()` over repeated `if`/`else` tests (part of #884, @MichaelChirico). * `nested_pipe_linter()` for discouraging pipes within pipes, e.g. `df1 %>% inner_join(df2 %>% select(a, b))` (part of #884, @MichaelChirico). -* `nrow_subset_linter()` for discouraging usage like `nrow(subset(x, conditions))` in favor of something like `with(x, sum(conditions))` which doesn't require a full subset of `x` (part of #884, @MichaelChirico). +* `nrow_subset_linter()` for discouraging usage like `nrow(subset(x, conditions))` in favor of something like `with(x, sum(conditions))` which doesn't require a full subset of `x` (#2314 and part of #884, @MichaelChirico). * `pipe_return_linter()` for discouraging usage of `return()` inside a {magrittr} pipeline (part of #884, @MichaelChirico). * `one_call_pipe_linter()` for discouraging one-step pipelines like `x |> as.character()` (#2330 and part of #884, @MichaelChirico). * `object_overwrite_linter()` for discouraging re-use of upstream package exports as local variables (#2344, #2346 and part of #884, @MichaelChirico and @AshesITR). diff --git a/R/nrow_subset_linter.R b/R/nrow_subset_linter.R index 2e877ce9f..f34246229 100644 --- a/R/nrow_subset_linter.R +++ b/R/nrow_subset_linter.R @@ -22,19 +22,25 @@ #' #' @evalRd rd_tags("nrow_subset_linter") #' @seealso [linters] for a complete list of linters available in lintr. +#' @include shared_constants.R #' @export nrow_subset_linter <- make_linter_from_function_xpath( function_names = "subset", - xpath = " + xpath = glue(" parent::expr /parent::expr - /parent::expr[expr/SYMBOL_FUNCTION_CALL[text() = 'nrow']] - ", + /parent::expr[ + expr/SYMBOL_FUNCTION_CALL[text() = 'nrow'] + or (self::expr | parent::expr)[ + (PIPE or SPECIAL[{ xp_text_in_table(setdiff(magrittr_pipes, c('%$%', '%<>%'))) }]) + and expr/expr/SYMBOL_FUNCTION_CALL[text() = 'nrow'] + ] + ] + "), lint_message = paste( "Use arithmetic to count the number of rows satisfying a condition,", "rather than fully subsetting the data.frame and counting the resulting rows.", - "For example, replace nrow(subset(x, is_treatment))", - "with sum(x$is_treatment). NB: use na.rm = TRUE if `is_treatment` has", - "missing values." + "For example, replace nrow(subset(x, is_treatment)) with sum(x$is_treatment).", + "NB: use na.rm = TRUE if `is_treatment` has missing values." ) ) diff --git a/tests/testthat/test-nrow_subset_linter.R b/tests/testthat/test-nrow_subset_linter.R index 18a3e45d8..2594f00ef 100644 --- a/tests/testthat/test-nrow_subset_linter.R +++ b/tests/testthat/test-nrow_subset_linter.R @@ -29,3 +29,14 @@ test_that("lints vectorize", { nrow_subset_linter() ) }) + +test_that("linter is pipeline-aware", { + linter <- nrow_subset_linter() + lint_msg <- "Use arithmetic to count the number of rows satisfying a condition" + + expect_lint("x %>% subset(y == z) %>% nrow()", lint_msg, linter) + expect_lint("subset(x) %>% nrow()", lint_msg, linter) + + skip_if_not_r_version("4.1.0") + expect_lint("x |> subset(y == z) |> nrow()", lint_msg, linter) +}) From 24286466bd286de6d269b65a1c3b096e2531c009 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 15 Dec 2023 15:58:29 +0800 Subject: [PATCH 07/11] custom message for top-level printing (#2414) Co-authored-by: Indrajeet Patil --- NEWS.md | 1 + R/implicit_assignment_linter.R | 14 +++++--- .../test-implicit_assignment_linter.R | 32 +++++++++++++++---- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/NEWS.md b/NEWS.md index fa97f51f2..42af1cb3f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -40,6 +40,7 @@ * `string_boundary_linter()` recognizes regular expression calls like `grepl("^abc$", x)` that can be replaced by using `==` instead (#1613, @MichaelChirico). * `unreachable_code_linter()` has an argument `allow_comment_regex` for customizing which "terminal" comments to exclude (#2327, @MichaelChirico). `# nolint end` comments are always excluded, as are {covr} exclusions (e.g. `# nocov end`) by default. * `format()` and `print()` methods for `lint` and `lints` classes get a new option `width` to control the printing width of lint messages (#1884, @MichaelChirico). The default is controlled by a new option `lintr.format_width`; if unset, no wrapping occurs (matching earlier behavior). +* `implicit_assignment_linter()` gets a custom message for the case of using `(` to induce printing like `(x <- foo())`; use an explicit call to `print()` for clarity (#2257, @MichaelChirico). * New function node caching for big efficiency gains to most linters (e.g. overall `lint_package()` improvement of 14-27% and core linting improvement up to 30%; #2357, @AshesITR). Most linters are written around function usage, and XPath performance searching for many functions is poor. The new `xml_find_function_calls()` entry in the `get_source_expressions()` output caches all function call nodes instead. See the vignette on creating linters for more details on how to use it. * `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico). diff --git a/R/implicit_assignment_linter.R b/R/implicit_assignment_linter.R index db7ed5ba9..70dfd3376 100644 --- a/R/implicit_assignment_linter.R +++ b/R/implicit_assignment_linter.R @@ -102,21 +102,25 @@ implicit_assignment_linter <- function(except = c("bquote", "expression", "expr" ) } + implicit_message <- paste( + "Avoid implicit assignments in function calls.", + "For example, instead of `if (x <- 1L) { ... }`, write `x <- 1L; if (x) { ... }`." + ) + + print_message <- "Call print() explicitly instead of relying on implicit printing behavior via '('." + Linter(linter_level = "file", function(source_expression) { # need the full file to also catch usages at the top level xml <- source_expression$full_xml_parsed_content bad_expr <- xml_find_all(xml, xpath) - lint_message <- paste( - "Avoid implicit assignments in function calls.", - "For example, instead of `if (x <- 1L) { ... }`, write `x <- 1L; if (x) { ... }`." - ) + print_only <- !is.na(xml_find_first(bad_expr, "parent::expr[parent::exprlist and *[1][self::OP-LEFT-PAREN]]")) xml_nodes_to_lints( bad_expr, source_expression = source_expression, - lint_message = lint_message, + lint_message = ifelse(print_only, print_message, implicit_message), type = "warning" ) }) diff --git a/tests/testthat/test-implicit_assignment_linter.R b/tests/testthat/test-implicit_assignment_linter.R index d8f5e498a..5457cb441 100644 --- a/tests/testthat/test-implicit_assignment_linter.R +++ b/tests/testthat/test-implicit_assignment_linter.R @@ -347,11 +347,11 @@ test_that("implicit_assignment_linter works as expected with pipes and walrus op }) test_that("parenthetical assignments are caught", { - linter <- implicit_assignment_linter() - lint_message <- rex::rex("Avoid implicit assignments in function calls.") - - expect_lint("(x <- 1:10)", lint_message, linter) - expect_lint("if (A && (B <- foo())) { }", lint_message, linter) + expect_lint( + "if (A && (B <- foo())) { }", + rex::rex("Avoid implicit assignments in function calls."), + implicit_assignment_linter() + ) }) test_that("allow_lazy lets lazy assignments through", { @@ -439,7 +439,6 @@ test_that("allow_scoped skips scoped assignments", { ) # outside of branching, doesn't matter - expect_lint("(idx <- foo()); bar()", lint_message, linter) expect_lint("foo(idx <- bar()); baz()", lint_message, linter) expect_lint("foo(x, idx <- bar()); baz()", lint_message, linter) }) @@ -477,3 +476,24 @@ test_that("interaction of allow_lazy and allow_scoped", { linter ) }) + +test_that("call-less '(' mentions avoiding implicit printing", { + linter <- implicit_assignment_linter() + implicit_msg <- rex::rex("Avoid implicit assignments in function calls.") + print_msg <- rex::rex("Call print() explicitly instead of relying on implicit printing behavior via '('.") + + expect_lint("(a <- foo())", print_msg, linter) + + # only for top-level assignments; withAutoprint() ignored + expect_lint("for (xi in x) (z <- foo(xi))", implicit_msg, linter) + + # mixed messages + expect_lint( + trim_some(" + (a <- foo()) + bar(z <- baz(a)) + "), + list(print_msg, implicit_msg), + linter + ) +}) From 3b933a98e8865f58223403c21cbf48a0f5ccf624 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 15 Dec 2023 16:24:08 +0800 Subject: [PATCH 08/11] Mention saving the output of order() in sort_linter() message (#2407) * mention saving the output of order() * new wording --- R/sort_linter.R | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/R/sort_linter.R b/R/sort_linter.R index 0604b7d23..d4709370d 100644 --- a/R/sort_linter.R +++ b/R/sort_linter.R @@ -125,7 +125,11 @@ sort_linter <- function() { order_lints <- xml_nodes_to_lints( order_expr, source_expression = source_expression, - lint_message = paste0(new_call, " is better than ", orig_call, "."), + lint_message = paste0( + new_call, " is better than ", orig_call, ". ", + "Note that it's always preferable to save the output of order() for the same variable ", + "as a local variable than to re-compute it." + ), type = "warning" ) From c912988753f2b43c31775a62c07094ec8aedb1f1 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 15 Dec 2023 18:03:29 +0800 Subject: [PATCH 09/11] Extend vector_logic_linter() for usage like filter(x, A && B) (#2435) * catch incorrect &&/|| usage in subsets * also catch nested usage inside subsets * examples, new tag, document * explicit |> text * typo --------- Co-authored-by: AshesITR --- NEWS.md | 1 + R/vector_logic_linter.R | 44 +++++++++++-- inst/lintr/linters.csv | 2 +- man/common_mistakes_linters.Rd | 1 + man/default_undesirable_functions.Rd | 4 +- man/linters.Rd | 4 +- man/vector_logic_linter.Rd | 12 +++- tests/testthat/test-vector_logic_linter.R | 78 ++++++++++++++++++++++- 8 files changed, 133 insertions(+), 13 deletions(-) diff --git a/NEWS.md b/NEWS.md index 42af1cb3f..f726569cf 100644 --- a/NEWS.md +++ b/NEWS.md @@ -42,6 +42,7 @@ * `format()` and `print()` methods for `lint` and `lints` classes get a new option `width` to control the printing width of lint messages (#1884, @MichaelChirico). The default is controlled by a new option `lintr.format_width`; if unset, no wrapping occurs (matching earlier behavior). * `implicit_assignment_linter()` gets a custom message for the case of using `(` to induce printing like `(x <- foo())`; use an explicit call to `print()` for clarity (#2257, @MichaelChirico). * New function node caching for big efficiency gains to most linters (e.g. overall `lint_package()` improvement of 14-27% and core linting improvement up to 30%; #2357, @AshesITR). Most linters are written around function usage, and XPath performance searching for many functions is poor. The new `xml_find_function_calls()` entry in the `get_source_expressions()` output caches all function call nodes instead. See the vignette on creating linters for more details on how to use it. +* `vector_logic_linter()` is extended to recognize incorrect usage of scalar operators `&&` and `||` inside subsetting expressions like `dplyr::filter(x, A && B)` (#2166, @MichaelChirico). * `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico). ### New linters diff --git a/R/vector_logic_linter.R b/R/vector_logic_linter.R index 1b18ceda0..2705288cc 100644 --- a/R/vector_logic_linter.R +++ b/R/vector_logic_linter.R @@ -31,6 +31,11 @@ #' linters = vector_logic_linter() #' ) #' +#' lint( +#' text = "filter(x, A && B)", +#' linters = vector_logic_linter() +#' ) +#' #' # okay #' lint( #' text = "if (TRUE && FALSE) 1", @@ -42,6 +47,11 @@ #' linters = vector_logic_linter() #' ) #' +#' lint( +#' text = "filter(x, A & B)", +#' linters = vector_logic_linter() +#' ) +#' #' @evalRd rd_tags("vector_logic_linter") #' @seealso #' - [linters] for a complete list of linters available in lintr. @@ -60,7 +70,7 @@ vector_logic_linter <- function() { # ... # # we _don't_ want to match anything on the second expr, hence this - xpath <- " + condition_xpath <- " (//AND | //OR)[ ancestor::expr[ not(preceding-sibling::OP-RIGHT-PAREN) @@ -81,17 +91,39 @@ vector_logic_linter <- function() { ] " + subset_xpath <- " + parent::expr[not(SYMBOL_PACKAGE[text() = 'stats'])] + /parent::expr + //expr[ + (AND2 or OR2) + and not(preceding-sibling::expr[last()]/SYMBOL_FUNCTION_CALL[not(text() = 'subset' or text() = 'filter')]) + and not(preceding-sibling::OP-LEFT-BRACKET) + and not(preceding-sibling::*[not(self::COMMENT)][2][self::SYMBOL_SUB and text() = 'circular']) + ]/*[2] + " + Linter(linter_level = "expression", function(source_expression) { xml <- source_expression$xml_parsed_content + xml_call <- source_expression$xml_find_function_calls(c("subset", "filter")) - bad_expr <- xml_find_all(xml, xpath) + condition_expr <- xml_find_all(xml, condition_xpath) + condition_op <- xml_text(condition_expr) + condition_lints <- xml_nodes_to_lints( + condition_expr, + source_expression = source_expression, + lint_message = sprintf("Use `%s` in conditional expressions.", strrep(condition_op, 2L)), + type = "warning" + ) - op <- xml_text(bad_expr) - xml_nodes_to_lints( - bad_expr, + subset_expr <- xml_find_all(xml_call, subset_xpath) + subset_op <- xml_text(subset_expr) + subset_lints <- xml_nodes_to_lints( + subset_expr, source_expression = source_expression, - lint_message = sprintf("Use `%s` in conditional expressions.", strrep(op, 2L)), + lint_message = sprintf("Use `%s` in subsetting expressions.", substr(subset_op, 1L, 1L)), type = "warning" ) + + c(condition_lints, subset_lints) }) } diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 9ea93de14..e0805e47d 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -118,7 +118,7 @@ unnecessary_placeholder_linter,readability best_practices unneeded_concatenation_linter,style readability efficiency configurable deprecated unreachable_code_linter,best_practices readability configurable unused_import_linter,best_practices common_mistakes configurable executing -vector_logic_linter,default efficiency best_practices +vector_logic_linter,default efficiency best_practices common_mistakes which_grepl_linter,readability efficiency consistency regex whitespace_linter,style consistency default yoda_test_linter,package_development best_practices readability pkg_testthat diff --git a/man/common_mistakes_linters.Rd b/man/common_mistakes_linters.Rd index d3578e567..47faf43a1 100644 --- a/man/common_mistakes_linters.Rd +++ b/man/common_mistakes_linters.Rd @@ -22,5 +22,6 @@ The following linters are tagged with 'common_mistakes': \item{\code{\link{redundant_equals_linter}}} \item{\code{\link{sprintf_linter}}} \item{\code{\link{unused_import_linter}}} +\item{\code{\link{vector_logic_linter}}} } } diff --git a/man/default_undesirable_functions.Rd b/man/default_undesirable_functions.Rd index 7c5c9ba63..7562e02c8 100644 --- a/man/default_undesirable_functions.Rd +++ b/man/default_undesirable_functions.Rd @@ -28,13 +28,13 @@ Use \code{\link[=modify_defaults]{modify_defaults()}} to produce a custom list. \details{ The following functions are sometimes regarded as undesirable: \itemize{ -\item \code{\link[=.libPaths]{.libPaths()}} As an alternative, use \code{\link[withr:with_libpaths]{withr::with_libpaths()}} for a temporary change instead of permanently modifying the library location. \item \code{\link[=attach]{attach()}} As an alternative, use roxygen2's @importFrom statement in packages, or \code{::} in scripts. \code{\link[=attach]{attach()}} modifies the global search path. \item \code{\link[=browser]{browser()}} As an alternative, remove this likely leftover from debugging. It pauses execution when run. \item \code{\link[=debug]{debug()}} As an alternative, remove this likely leftover from debugging. It traps a function and causes execution to pause when that function is run. \item \code{\link[=debugcall]{debugcall()}} As an alternative, remove this likely leftover from debugging. It traps a function and causes execution to pause when that function is run. \item \code{\link[=debugonce]{debugonce()}} As an alternative, remove this likely leftover from debugging. It traps a function and causes execution to pause when that function is run. \item \code{\link[=detach]{detach()}} As an alternative, avoid modifying the global search path. Detaching environments from the search path is rarely necessary in production code. +\item \code{\link[=.libPaths]{.libPaths()}} As an alternative, use \code{\link[withr:with_libpaths]{withr::with_libpaths()}} for a temporary change instead of permanently modifying the library location. \item \code{\link[=library]{library()}} As an alternative, use roxygen2's @importFrom statement in packages and \code{::} in scripts, instead of modifying the global search path. \item \code{\link[=mapply]{mapply()}} As an alternative, use \code{\link[=Map]{Map()}} to guarantee a list is returned and simplify accordingly. \item \code{\link[=options]{options()}} As an alternative, use \code{\link[withr:with_options]{withr::with_options()}} for a temporary change instead of permanently modifying the session options. @@ -55,7 +55,7 @@ The following functions are sometimes regarded as undesirable: The following operators are sometimes regarded as undesirable: \itemize{ \item \code{\link[base:assignOps]{<<-}} As an alternative, It assigns outside the current environment in a way that can be hard to reason about. Prefer fully-encapsulated functions wherever possible, or, if necessary, assign to a specific environment with \code{\link[=assign]{assign()}}. Recall that you can create an environment at the desired scope with \code{\link[=new.env]{new.env()}}. -\item \code{\link[base:ns-dblcolon]{:::}} As an alternative, It accesses non-exported functions inside packages. Code relying on these is likely to break in future versions of the package because the functions are not part of the public interface and may be changed or removed by the maintainers without notice. Use public functions via \code{::} instead. \item \code{\link[base:assignOps]{<<-}} As an alternative, It assigns outside the current environment in a way that can be hard to reason about. Prefer fully-encapsulated functions wherever possible, or, if necessary, assign to a specific environment with \code{\link[=assign]{assign()}}. Recall that you can create an environment at the desired scope with \code{\link[=new.env]{new.env()}}. +\item \code{\link[base:ns-dblcolon]{:::}} As an alternative, It accesses non-exported functions inside packages. Code relying on these is likely to break in future versions of the package because the functions are not part of the public interface and may be changed or removed by the maintainers without notice. Use public functions via \code{::} instead. } } diff --git a/man/linters.Rd b/man/linters.Rd index e49bd4fca..76cd1d34f 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -18,7 +18,7 @@ see also \code{\link[=available_tags]{available_tags()}}. The following tags exist: \itemize{ \item{\link[=best_practices_linters]{best_practices} (63 linters)} -\item{\link[=common_mistakes_linters]{common_mistakes} (10 linters)} +\item{\link[=common_mistakes_linters]{common_mistakes} (11 linters)} \item{\link[=configurable_linters]{configurable} (42 linters)} \item{\link[=consistency_linters]{consistency} (32 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} @@ -148,7 +148,7 @@ The following linters exist: \item{\code{\link{unnecessary_placeholder_linter}} (tags: best_practices, readability)} \item{\code{\link{unreachable_code_linter}} (tags: best_practices, configurable, readability)} \item{\code{\link{unused_import_linter}} (tags: best_practices, common_mistakes, configurable, executing)} -\item{\code{\link{vector_logic_linter}} (tags: best_practices, default, efficiency)} +\item{\code{\link{vector_logic_linter}} (tags: best_practices, common_mistakes, default, efficiency)} \item{\code{\link{which_grepl_linter}} (tags: consistency, efficiency, readability, regex)} \item{\code{\link{whitespace_linter}} (tags: consistency, default, style)} \item{\code{\link{yoda_test_linter}} (tags: best_practices, package_development, pkg_testthat, readability)} diff --git a/man/vector_logic_linter.Rd b/man/vector_logic_linter.Rd index 0ffe3fa9c..fac73cf22 100644 --- a/man/vector_logic_linter.Rd +++ b/man/vector_logic_linter.Rd @@ -39,6 +39,11 @@ lint( linters = vector_logic_linter() ) +lint( + text = "filter(x, A && B)", + linters = vector_logic_linter() +) + # okay lint( text = "if (TRUE && FALSE) 1", @@ -50,6 +55,11 @@ lint( linters = vector_logic_linter() ) +lint( + text = "filter(x, A & B)", + linters = vector_logic_linter() +) + } \seealso{ \itemize{ @@ -58,5 +68,5 @@ lint( } } \section{Tags}{ -\link[=best_practices_linters]{best_practices}, \link[=default_linters]{default}, \link[=efficiency_linters]{efficiency} +\link[=best_practices_linters]{best_practices}, \link[=common_mistakes_linters]{common_mistakes}, \link[=default_linters]{default}, \link[=efficiency_linters]{efficiency} } diff --git a/tests/testthat/test-vector_logic_linter.R b/tests/testthat/test-vector_logic_linter.R index e7d24db86..6afaafbd4 100644 --- a/tests/testthat/test-vector_logic_linter.R +++ b/tests/testthat/test-vector_logic_linter.R @@ -100,15 +100,91 @@ test_that("vector_logic_linter recognizes some false positves around bitwise &/| expect_lint('if ((mode | "111") != as.hexmode("111")) { }', NULL, linter) }) +test_that("incorrect subset/filter usage is caught", { + linter <- vector_logic_linter() + and_msg <- rex::rex("Use `&` in subsetting expressions") + or_msg <- rex::rex("Use `|` in subsetting expressions") + + expect_lint("filter(x, y && z)", and_msg, linter) + expect_lint("filter(x, y || z)", or_msg, linter) + expect_lint("subset(x, y && z)", and_msg, linter) + expect_lint("subset(x, y || z)", or_msg, linter) + + expect_lint("x %>% filter(y && z)", and_msg, linter) + expect_lint("filter(x, a & b, c | d, e && f)", list(and_msg, column_number = 27L), linter) +}) + +test_that("native pipe usage is caught in subset/filter logic", { + skip_if_not_r_version("4.1.0") + + expect_lint("x |> filter(y && z)", rex::rex("Use `&` in subsetting"), vector_logic_linter()) +}) + +test_that("subsetting logic handles nesting", { + linter <- vector_logic_linter() + and_msg <- rex::rex("Use `&` in subsetting expressions") + or_msg <- rex::rex("Use `|` in subsetting expressions") + + expect_lint("filter(x, a & b || c)", or_msg, linter) + expect_lint("filter(x, a && b | c)", and_msg, linter) + + # but not valid usage + expect_lint("filter(x, y < mean(y, na.rm = AA && BB))", NULL, linter) + expect_lint("subset(x, y < mean(y, na.rm = AA && BB) & y > 0)", NULL, linter) + expect_lint("subset(x, y < x[y > 0, drop = AA && BB, y])", NULL, linter) +}) + +test_that("filter() handling is conservative about stats::filter()", { + linter <- vector_logic_linter() + and_msg <- rex::rex("Use `&` in subsetting expressions") + + # NB: this should be invalid, filter= is a vector argument + expect_lint("stats::filter(x, y && z)", NULL, linter) + # The only logical argument to stats::filter(), exclude by keyword + expect_lint("filter(x, circular = y && z)", NULL, linter) + # But presence of circular= doesn't invalidate lint + expect_lint("filter(x, circular = TRUE, y && z)", and_msg, linter) + expect_lint("filter(x, y && z, circular = TRUE)", and_msg, linter) + expect_lint( + trim_some(" + filter(x, circular # comment + = y && z) + "), + NULL, + linter + ) + expect_lint( + trim_some(" + filter(x, circular = # comment + y && z) + "), + NULL, + linter + ) + expect_lint( + trim_some(" + filter(x, circular # comment + = # comment + y && z) + "), + NULL, + linter + ) +}) + test_that("lints vectorize", { expect_lint( trim_some("{ if (AA & BB) {} if (CC | DD) {} + filter(x, EE && FF) + subset(y, GG || HH) }"), list( list(rex::rex("`&&`"), line_number = 2L), - list(rex::rex("`||`"), line_number = 3L) + list(rex::rex("`||`"), line_number = 3L), + list(rex::rex("`&`"), line_number = 4L), + list(rex::rex("`|`"), line_number = 5L) ), vector_logic_linter() ) From 5b99e6ec7b8036cfe0dc5560d31e54dafa478aec Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 15 Dec 2023 19:10:08 +0800 Subject: [PATCH 10/11] Support pattern-based exclusion in return_linter() (#2433) --- NEWS.md | 1 + R/return_linter.R | 42 +++++++++++++----- man/return_linter.Rd | 8 ++-- tests/testthat/test-return_linter.R | 66 +++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 14 deletions(-) diff --git a/NEWS.md b/NEWS.md index f726569cf..dac147907 100644 --- a/NEWS.md +++ b/NEWS.md @@ -34,6 +34,7 @@ + `allow_implicit_else` (default `TRUE`) which, when `FALSE`, checks that all terminal `if` statements are paired with a corresponding `else` statement (part of #884, @MichaelChirico). + `return_functions` to customize which functions are equivalent to `return()` as "exit" clauses, e.g. `rlang::abort()` can be considered in addition to the default functions like `stop()` and `q()` from base (#2271 and part of #884, @MichaelChirico and @MEO265). + `except` to customize which functions are ignored entirely (i.e., whether they have a return of the specified style is not checked; #2271 and part of #884, @MichaelChirico and @MEO265). Namespace hooks like `.onAttach()` and `.onLoad()` are always ignored. + + `except_regex`, the same purpose as `except=`, but filters functions by pattern. This is motivated by {RUnit}, where test suites are based on unit test functions matched by pattern, e.g. `^Test`, and where explicit return may be awkward (#2335, @MichaelChirico). * `unnecessary_lambda_linter` is extended to encourage vectorized comparisons where possible, e.g. `sapply(x, sum) > 0` instead of `sapply(x, function(x) sum(x) > 0)` (part of #884, @MichaelChirico). Toggle this behavior with argument `allow_comparison`. * `backport_linter()` is slightly faster by moving expensive computations outside the linting function (#2339, #2348, @AshesITR and @MichaelChirico). * `Linter()` has a new argument `linter_level` (default `NA`). This is used by `lint()` to more efficiently check for expression levels than the idiom `if (!is_lint_level(...)) { return(list()) }` (#2351, @AshesITR). diff --git a/R/return_linter.R b/R/return_linter.R index 4094ca755..fd3dd5831 100644 --- a/R/return_linter.R +++ b/R/return_linter.R @@ -14,10 +14,11 @@ #' from base that are always allowed: [stop()], [q()], [quit()], [invokeRestart()], #' `tryInvokeRestart()`, [UseMethod()], [NextMethod()], [standardGeneric()], #' [callNextMethod()], [.C()], [.Call()], [.External()], and [.Fortran()]. -#' @param except Character vector of functions that are not checked when +#' @param except,except_regex Character vector of functions that are not checked when #' `return_style = "explicit"`. These are in addition to namespace hook functions #' that are never checked: `.onLoad()`, `.onUnload()`, `.onAttach()`, `.onDetach()`, -#' `.Last.lib()`, `.First()` and `.Last()`. +#' `.Last.lib()`, `.First()` and `.Last()`. `except` matches function names exactly, +#' while `except_regex` does exclusion by pattern matching with [rex::re_matches()]. #' #' @examples #' # will produce lints @@ -73,16 +74,25 @@ return_linter <- function( return_style = c("implicit", "explicit"), allow_implicit_else = TRUE, return_functions = NULL, - except = NULL) { + except = NULL, + except_regex = NULL) { return_style <- match.arg(return_style) - if (!allow_implicit_else || return_style == "explicit") { - except_xpath <- glue("parent::expr[not( - preceding-sibling::expr/SYMBOL[{ xp_text_in_table(union(special_funs, except)) }] - )]") + check_except <- !allow_implicit_else || return_style == "explicit" + # We defer building the XPath strings in this case since we can't build the + # pattern-based "except" logic directly into the XPath (because of v1.0) + defer_except <- check_except && !is.null(except_regex) + + if (check_except) { + except_xpath_fmt <- "parent::expr[not( + preceding-sibling::expr/SYMBOL[{ xp_text_in_table(except) }] + )]" + except <- union(special_funs, except) + if (!defer_except) except_xpath <- glue(except_xpath_fmt, except = except) } if (return_style == "implicit") { + # nolint next: object_usage. False positive. body_xpath <- "(//FUNCTION | //OP-LAMBDA)/following-sibling::expr[1]" params <- list( implicit = TRUE, @@ -91,8 +101,6 @@ return_linter <- function( lint_message = "Use implicit return behavior; explicit return() is not needed." ) } else { - except <- union(special_funs, except) - base_return_functions <- c( # Normal calls "return", "stop", "q", "quit", @@ -110,11 +118,17 @@ return_linter <- function( return_functions <- union(base_return_functions, return_functions) - body_xpath <- glue(" + body_xpath_fmt <- " (//FUNCTION | //OP-LAMBDA)[{ except_xpath }] /following-sibling::expr[OP-LEFT-BRACE and expr[last()]/@line1 != @line1] /expr[last()] - ") + " + if (defer_except) { + function_name_xpath <- "(//FUNCTION | //OP-LAMBDA)/parent::expr/preceding-sibling::expr/SYMBOL" + } else { + body_xpath <- glue(body_xpath_fmt, except_xpath = except_xpath) + } + params <- list( implicit = FALSE, type = "warning", @@ -130,6 +144,12 @@ return_linter <- function( Linter(linter_level = "expression", function(source_expression) { xml <- source_expression$xml_parsed_content + if (defer_except) { + assigned_functions <- xml_text(xml_find_all(xml, function_name_xpath)) + except <- union(except, assigned_functions[re_matches(assigned_functions, except_regex)]) + except_xpath <- glue(except_xpath_fmt, except = except) + body_xpath <- glue(body_xpath_fmt, except_xpath = except_xpath) + } body_expr <- xml_find_all(xml, body_xpath) diff --git a/man/return_linter.Rd b/man/return_linter.Rd index f18f2d8c1..fc4fdbab0 100644 --- a/man/return_linter.Rd +++ b/man/return_linter.Rd @@ -8,7 +8,8 @@ return_linter( return_style = c("implicit", "explicit"), allow_implicit_else = TRUE, return_functions = NULL, - except = NULL + except = NULL, + except_regex = NULL ) } \arguments{ @@ -27,10 +28,11 @@ from base that are always allowed: \code{\link[=stop]{stop()}}, \code{\link[=q]{ \code{tryInvokeRestart()}, \code{\link[=UseMethod]{UseMethod()}}, \code{\link[=NextMethod]{NextMethod()}}, \code{\link[=standardGeneric]{standardGeneric()}}, \code{\link[=callNextMethod]{callNextMethod()}}, \code{\link[=.C]{.C()}}, \code{\link[=.Call]{.Call()}}, \code{\link[=.External]{.External()}}, and \code{\link[=.Fortran]{.Fortran()}}.} -\item{except}{Character vector of functions that are not checked when +\item{except, except_regex}{Character vector of functions that are not checked when \code{return_style = "explicit"}. These are in addition to namespace hook functions that are never checked: \code{.onLoad()}, \code{.onUnload()}, \code{.onAttach()}, \code{.onDetach()}, -\code{.Last.lib()}, \code{.First()} and \code{.Last()}.} +\code{.Last.lib()}, \code{.First()} and \code{.Last()}. \code{except} matches function names exactly, +while \code{except_regex} does exclusion by pattern matching with \code{\link[rex:re_matches]{rex::re_matches()}}.} } \description{ This linter checks functions' \code{\link[=return]{return()}} expressions. diff --git a/tests/testthat/test-return_linter.R b/tests/testthat/test-return_linter.R index f017b0e08..a284193bd 100644 --- a/tests/testthat/test-return_linter.R +++ b/tests/testthat/test-return_linter.R @@ -698,6 +698,72 @@ test_that("except= argument works", { ) }) +test_that("except_regex= argument works", { + linter <- return_linter(return_style = "explicit", except_regex = "^Test") + + expect_lint( + trim_some(" + TestSummary <- function() { + context <- foo(72643424) + expected <- data.frame(a = 2) + checkEquals(expected, bar(context)) + } + "), + NULL, + linter + ) + + expect_lint( + trim_some(" + TestMyPackage <- function() { + checkMyCustomComparator(x, y) + } + "), + NULL, + linter + ) + + expect_lint( + trim_some(" + TestOuter <- function() { + actual <- lapply( + input, + function(x) { + no_return() + } + ) + TestInner <- function() { + no_return() + } + checkEquals(TestInner(), actual) + } + "), + list(rex::rex("All functions must have an explicit return()."), line_number = 5L), + linter + ) +}) + +test_that("except= and except_regex= combination works", { + expect_lint( + trim_some(" + foo <- function() { + no_return() + } + bar <- function() { + no_return() + } + abaz <- function() { + no_return() + } + bbaz <- function() { + no_return() + } + "), + NULL, + return_linter(return_style = "explicit", except = c("foo", "bar"), except_regex = "baz$") + ) +}) + test_that("return_linter skips brace-wrapped inline functions", { expect_lint("function(x) { sum(x) }", NULL, return_linter(return_style = "explicit")) }) From fdbb9bfa1ea868ad66c6fe9a30b4ae872c03f56e Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 15 Dec 2023 19:37:12 +0800 Subject: [PATCH 11/11] following expr can be other expr-ish nodes (#2454) --- NEWS.md | 1 + R/consecutive_assertion_linter.R | 15 +++++++++------ .../test-consecutive_assertion_linter.R | 18 +++++++++++++++++- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/NEWS.md b/NEWS.md index dac147907..db76f264c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -75,6 +75,7 @@ + ignores calls on the RHS of operators like `lapply(l, function(x) "a" %in% names(x))` (#2310, @MichaelChirico). * `vector_logic_linter()` recognizes some cases where bitwise `&`/`|` are used correctly (#1453, @MichaelChirico). * `expect_comparison_linter()` ignores faulty usage like `expect_true(x, y > z)` (#2083, @MichaelChirico). Note that `y > z` is being passed to the `info=` argument, so this is likely a mistake. +* `consecutive_assertion_linter()` ignores cases where a second asssertion follows assignment with `=` (#2444, @MichaelChirico). ### Lint accuracy fixes: removing false negatives diff --git a/R/consecutive_assertion_linter.R b/R/consecutive_assertion_linter.R index 525edf117..668ed1593 100644 --- a/R/consecutive_assertion_linter.R +++ b/R/consecutive_assertion_linter.R @@ -31,20 +31,23 @@ #' @seealso [linters] for a complete list of linters available in lintr. #' @export consecutive_assertion_linter <- function() { - stopifnot_xpath <- " + # annoying expr-but-not-really nodes + next_expr <- "following-sibling::*[self::expr or self::expr_or_assign_or_help or self::equal_assign][1]" + + stopifnot_xpath <- glue(" parent::expr /parent::expr[ - expr[1]/SYMBOL_FUNCTION_CALL = following-sibling::expr[1]/expr[1]/SYMBOL_FUNCTION_CALL + expr[1]/SYMBOL_FUNCTION_CALL = {next_expr}/expr[1]/SYMBOL_FUNCTION_CALL ] - " - assert_that_xpath <- " + ") + assert_that_xpath <- glue(" parent::expr /parent::expr[ not(SYMBOL_SUB[text() = 'msg']) and not(following-sibling::expr[1]/SYMBOL_SUB[text() = 'msg']) - and expr[1]/SYMBOL_FUNCTION_CALL = following-sibling::expr[1]/expr[1]/SYMBOL_FUNCTION_CALL + and expr[1]/SYMBOL_FUNCTION_CALL = {next_expr}/expr[1]/SYMBOL_FUNCTION_CALL ] - " + ") Linter(linter_level = "file", function(source_expression) { # need the full file to also catch usages at the top level diff --git a/tests/testthat/test-consecutive_assertion_linter.R b/tests/testthat/test-consecutive_assertion_linter.R index f3df750a7..35fe6d3f6 100644 --- a/tests/testthat/test-consecutive_assertion_linter.R +++ b/tests/testthat/test-consecutive_assertion_linter.R @@ -100,7 +100,7 @@ test_that("Mixing test functions is fine", { ) }) -test_that("lints vectorie", { +test_that("lints vectorize", { expect_lint( trim_some("{ stopifnot(A) @@ -127,3 +127,19 @@ test_that("old name consecutive_stopifnot_linter() is deprecated", { expect_lint("stopifnot(x); y; stopifnot(z)", NULL, old_linter) expect_lint("stopifnot(x); stopifnot(y)", "Unify consecutive calls", old_linter) }) + +test_that("interceding = assignments aren't linted", { + expect_lint( + trim_some("{ + stopifnot(A) + x = 1 + stopifnot(B) + + assert_that(C) + z = 3 + assert_that(D) + }"), + NULL, + consecutive_assertion_linter() + ) +})