From f61967bcc0b7acc5a3211f2193d3f62efc8a8d45 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 18 Nov 2023 11:30:20 -0800 Subject: [PATCH 1/3] Fully deprecate anything marked deprecated since 3.0.0 (#2284) * Fully deprecate anything marked deprecated since 3.0.0 * NEWS * delint * remove absent parameter from docmentation * Correct test name * remove redundant test * restore linters, mark as defunct * always exclude defunct tag from available_linters; fix some tests * exclude defunct from test * correct NEWS * correct NEWS * parallel wording * guess fix, debug print * guess correct; remove debug+delint * remove obsolete descriptions * document absence of "defunct" linters * typo except_tags->exclude_tags * consistency --------- Co-authored-by: Indrajeet Patil --- NEWS.md | 11 ++ R/deprecated.R | 6 +- R/ids_with_token.R | 7 +- R/lint.R | 39 +++--- R/linter_tag_docs.R | 2 +- R/linter_tags.R | 12 +- R/lintr-deprecated.R | 148 ++-------------------- R/with.R | 9 +- R/with_id.R | 7 +- inst/lintr/linters.csv | 8 +- man/available_linters.Rd | 5 +- man/deprecated_linters.Rd | 6 +- man/linters.Rd | 2 +- man/linters_with_defaults.Rd | 5 +- man/linters_with_tags.Rd | 5 +- man/lintr-deprecated.Rd | 27 ++-- tests/testthat/test-closed_curly_linter.R | 78 ------------ tests/testthat/test-ids_with_token.R | 7 +- tests/testthat/test-lint.R | 20 ++- tests/testthat/test-linter_tags.R | 7 +- tests/testthat/test-methods.R | 2 +- tests/testthat/test-open_curly_linter.R | 109 ---------------- tests/testthat/test-paren_brace_linter.R | 50 -------- tests/testthat/test-semicolon_linter.R | 43 ------- tests/testthat/test-with.R | 20 +-- tests/testthat/test-with_id.R | 13 +- 26 files changed, 112 insertions(+), 536 deletions(-) delete mode 100644 tests/testthat/test-closed_curly_linter.R delete mode 100644 tests/testthat/test-open_curly_linter.R delete mode 100644 tests/testthat/test-paren_brace_linter.R diff --git a/NEWS.md b/NEWS.md index b3a90e6b3..9fe5f6b71 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,16 @@ # lintr (development version) +## Deprecations & breaking changes + +* Various things marked deprecated since {lintr} 3.0.0 have been fully deprecated. They will be completely removed in the subsequent release. + + `source_file=` argument to `ids_with_token()` and `with_id()`. + + Passing linters by name or as non-`"linter"`-classed functions. + + `linter=` argument of `Lint()`. + + Linters `closed_curly_linter()`, `open_curly_linter()`, `paren_brace_linter()`, and `semicolon_terminator_linter()`.. + + `with_defaults()`. + + Linters `closed_curly_linter()`, `open_curly_linter()`, `paren_brace_linter()`, and `semicolon_terminator_linter()`. + + Helper `with_defaults()`. + ## Bug fixes * `object_name_linter()` no longer errors when user-supplied `regexes=` have capture groups (#2188, @MichaelChirico). diff --git a/R/deprecated.R b/R/deprecated.R index bbbbb98c7..777974513 100644 --- a/R/deprecated.R +++ b/R/deprecated.R @@ -6,7 +6,9 @@ NULL lintr_deprecated <- function(old, new = NULL, version = NULL, - type = "Function") { + type = "Function", signal = c("warning", "stop")) { + signal <- match.arg(signal) + signal <- match.fun(signal) msg <- c( c(type, " ", old, " was deprecated"), if (length(version) > 0L) { @@ -18,5 +20,5 @@ lintr_deprecated <- function(old, new = NULL, version = NULL, } ) msg <- paste0(msg, collapse = "") - warning(msg, call. = FALSE, domain = NA) + signal(msg, call. = FALSE, domain = NA) } diff --git a/R/ids_with_token.R b/R/ids_with_token.R index 4d5df40f1..ed20bb06a 100644 --- a/R/ids_with_token.R +++ b/R/ids_with_token.R @@ -29,8 +29,11 @@ #' @export ids_with_token <- function(source_expression, value, fun = `==`, source_file = NULL) { if (!missing(source_file)) { - lintr_deprecated(old = "source_file", new = "source_expression", version = "3.0.0", type = "Argument") - source_expression <- source_file + lintr_deprecated( + old = "source_file", new = "source_expression", + version = "3.0.0", type = "Argument", + signal = "stop" + ) } if (!is_lint_level(source_expression, "expression")) { return(integer()) diff --git a/R/lint.R b/R/lint.R index 1c75d5742..82b718e8f 100644 --- a/R/lint.R +++ b/R/lint.R @@ -336,31 +336,27 @@ define_linters <- function(linters = NULL) { } validate_linter_object <- function(linter, name) { - if (!is_linter(linter) && is.function(linter)) { - if (is_linter_factory(linter)) { - lintr_deprecated( - old = "Passing linters as variables", - new = "a call to the linters (see ?linters)", - version = "3.0.0", - type = "" - ) - linter <- linter() - } else { - lintr_deprecated( - old = "The use of linters of class 'function'", - new = "linters classed as 'linter' (see ?Linter)", - version = "3.0.0", - type = "" - ) - linter <- Linter(linter, name = name) - } - } else if (!is.function(linter)) { + if (is_linter(linter)) { + return(linter) + } + if (!is.function(linter)) { stop(gettextf( "Expected '%s' to be a function of class 'linter', not a %s of class '%s'", name, typeof(linter), class(linter)[[1L]] )) } - linter + if (is_linter_factory(linter)) { + old <- "Passing linters as variables" + new <- "a call to the linters (see ?linters)" + } else { + old <- "The use of linters of class 'function'" + new <- "linters classed as 'linter' (see ?Linter)" + } + lintr_deprecated( + old = old, new = new, version = "3.0.0", + type = "", + signal = "stop" + ) } is_linter_factory <- function(fun) { @@ -404,7 +400,8 @@ Lint <- function(filename, line_number = 1L, column_number = 1L, # nolint: objec lintr_deprecated( old = "Using the `linter` argument of `Lint()`", version = "3.0.0", - type = "" + type = "", + signal = "stop" ) } diff --git a/R/linter_tag_docs.R b/R/linter_tag_docs.R index 453cea4da..c28048772 100644 --- a/R/linter_tag_docs.R +++ b/R/linter_tag_docs.R @@ -85,7 +85,7 @@ NULL #' @name deprecated_linters #' @description #' Linters that are deprecated and provided for backwards compatibility only. -#' These linters will be excluded from `linters_with_tags()` by default. +#' These linters will be excluded from [linters_with_tags()] by default. #' @evalRd rd_linters("deprecated") #' @seealso [linters] for a complete list of linters available in lintr. NULL diff --git a/R/linter_tags.R b/R/linter_tags.R index 65eae8dcd..12bf39ca1 100644 --- a/R/linter_tags.R +++ b/R/linter_tags.R @@ -4,11 +4,12 @@ #' #' @param packages A character vector of packages to search for linters. #' @param tags Optional character vector of tags to search. Only linters with at least one matching tag will be -#' returned. If `tags` is `NULL`, all linters will be returned. See `available_tags("lintr")` to find out what -#' tags are already used by lintr. +#' returned. If `tags` is `NULL`, all linters will be returned. See `available_tags("lintr")` to find out what +#' tags are already used by lintr. #' @param exclude_tags Tags to exclude from the results. Linters with at least one matching tag will not be returned. -#' If `except_tags` is `NULL`, no linters will be excluded. Note that `tags` takes priority, meaning that any -#' tag found in both `tags` and `exclude_tags` will be included, not excluded. +#' If `exclude_tags` is `NULL`, no linters will be excluded. Note that `tags` takes priority, meaning that any +#' tag found in both `tags` and `exclude_tags` will be included, not excluded. Note that linters with tag `"defunct"` +#' (which do not work and can no longer be run) cannot be queried directly. See [lintr-deprecated] instead. #' #' @section Package Authors: #' @@ -58,7 +59,8 @@ available_linters <- function(packages = "lintr", tags = NULL, exclude_tags = "d } # any tags specified explicitly will not be excluded (#1959) - exclude_tags <- setdiff(exclude_tags, tags) + # never include defunct linters, which don't work / error on instantiation (#2284). + exclude_tags <- unique(c(setdiff(exclude_tags, tags), "defunct")) # Handle multiple packages if (length(packages) > 1L) { diff --git a/R/lintr-deprecated.R b/R/lintr-deprecated.R index 89a660a1c..ceb9065eb 100644 --- a/R/lintr-deprecated.R +++ b/R/lintr-deprecated.R @@ -5,26 +5,15 @@ #' #' These functions have been deprecated from lintr. #' -#' - `open_curly_linter()` and `closed_curly_linter()` check that open and closed curly braces -#' are on their own line unless they follow an else, a comma, or a closing bracket. -#' Deprecated in favor of `brace_linter()`. +#' - `open_curly_linter()` (use [brace_linter()]) +#' - `closed_curly_linter()` (use `brace_linter()`) +#' - `paren_brace_linter()` (use `brace_linter()`) +#' - `semicolon_terminator_linter()` (use [semicolon_linter()]) #' -#' - `paren_brace_linter()` checks that there is a space between right parentheses and an opening -#' curly brace. E.g., `function(){}` doesn't have a space, while `function() {}` does. -#' Deprecated in favor of `brace_linter()`. -#' -#' - `semicolon_terminator_linter()` checks that no semicolons terminate expressions. -#' Deprecated in favor of `semicolon_linter()`. -#' -#' @param allow_single_line if `TRUE`, allow an open and closed curly pair on the same line. -#' @param semicolon A character vector defining which semicolons to report: -#' \describe{ -#' \item{compound}{Semicolons that separate two statements on the same line.} -#' \item{trailing}{Semicolons following the last statement on the line.} -#' } +#' @param allow_single_line,semicolon Irrelevant parameters to defunct linters. #' #' @seealso [linters] for a complete list of linters available in lintr. -#' @evalRd rd_tags("closed_curly_linter") +#' @evalRd rd_tags("single_quotes_linter") #' @keywords internal NULL @@ -32,133 +21,21 @@ NULL #' @rdname lintr-deprecated #' @export closed_curly_linter <- function(allow_single_line = FALSE) { - lintr_deprecated("closed_curly_linter", new = "brace_linter", version = "3.0.0", type = "Linter") - xp_cond_closed <- xp_and(c( - # matching { is on same line - if (isTRUE(allow_single_line)) { - "(@line1 != preceding-sibling::OP-LEFT-BRACE/@line1)" - }, - # immediately followed by ",", "]" or ")" - "not( - @line1 = ancestor::expr/following-sibling::*[1][ - self::OP-COMMA or self::OP-RIGHT-BRACKET or self::OP-RIGHT-PAREN - ]/@line1 - )", - # double curly - "not( - (@line1 = parent::expr/following-sibling::OP-RIGHT-BRACE/@line1) or - (@line1 = preceding-sibling::expr/OP-RIGHT-BRACE/@line1) - )" - )) - - xpath <- glue("//OP-RIGHT-BRACE[ - { xp_cond_closed } and ( - (@line1 = preceding-sibling::*[1]/@line2) or - (@line1 = parent::expr/following-sibling::*[1][not(self::ELSE)]/@line1) - ) - ]") - - Linter(function(source_expression) { - if (!is_lint_level(source_expression, "expression")) { - return(list()) - } - - xml_nodes_to_lints( - xml_find_all(source_expression$xml_parsed_content, xpath), - source_expression = source_expression, - lint_message = "Closing curly-braces should always be on their own line, unless they are followed by an else." - ) - }) + lintr_deprecated("closed_curly_linter", new = "brace_linter", version = "3.0.0", type = "Linter", signal = "stop") } #' Open curly linter #' @rdname lintr-deprecated #' @export open_curly_linter <- function(allow_single_line = FALSE) { - lintr_deprecated("open_curly_linter", new = "brace_linter", version = "3.0.0", type = "Linter") - - xpath_before <- "//OP-LEFT-BRACE[ - not(following-sibling::expr[1][OP-LEFT-BRACE]) - and not(parent::expr/preceding-sibling::*[1][OP-LEFT-BRACE]) - and @line1 != parent::expr/preceding-sibling::*[1][not(self::ELSE)]/@line2 - ]" - if (allow_single_line) { - xpath_after <- "//OP-LEFT-BRACE[ - not(following-sibling::expr[1][OP-LEFT-BRACE]) - and not(parent::expr/preceding-sibling::OP-LEFT-BRACE) - and not(@line2 = following-sibling::OP-RIGHT-BRACE/@line1) - and @line2 = following-sibling::expr[position() = 1 and not(OP-LEFT-BRACE)]/@line1 - ]" - message_after <- paste( - "Opening curly braces should always be followed by a new line", - "unless the paired closing brace is on the same line." - ) - } else { - xpath_after <- "//OP-LEFT-BRACE[ - not(following-sibling::expr[1][OP-LEFT-BRACE]) - and not(parent::expr/preceding-sibling::OP-LEFT-BRACE) - and @line2 = following-sibling::expr[1]/@line1 - ]" - message_after <- "Opening curly braces should always be followed by a new line." - } - - Linter(function(source_expression) { - if (!is_lint_level(source_expression, "expression")) { - return(list()) - } - - xml <- source_expression$xml_parsed_content - - expr_before <- xml_find_all(xml, xpath_before) - lints_before <- xml_nodes_to_lints( - expr_before, - source_expression = source_expression, - lint_message = "Opening curly braces should never go on their own line.", - type = "style" - ) - - expr_after <- xml_find_all(xml, xpath_after) - lints_after <- xml_nodes_to_lints( - expr_after, - source_expression = source_expression, - lint_message = message_after, - type = "style" - ) - - return(c(lints_before, lints_after)) - }) + lintr_deprecated("open_curly_linter", new = "brace_linter", version = "3.0.0", type = "Linter", signal = "stop") } #' Parentheses before brace linter #' @rdname lintr-deprecated #' @export paren_brace_linter <- function() { - lintr_deprecated("paren_brace_linter", new = "brace_linter", version = "3.0.0", type = "Linter") - - xpath <- paste( - "//OP-LEFT-BRACE[", - "@line1 = parent::expr/preceding-sibling::OP-RIGHT-PAREN/@line1", - "and", - "@col1 = parent::expr/preceding-sibling::OP-RIGHT-PAREN/@col1 + 1", - "]" - ) - - Linter(function(source_expression) { - if (!is_lint_level(source_expression, "expression")) { - return(list()) - } - - xml <- source_expression$xml_parsed_content - - match_exprs <- xml_find_all(xml, xpath) - - xml_nodes_to_lints( - match_exprs, - source_expression = source_expression, - lint_message = "There should be a space between right parenthesis and an opening curly brace.", - type = "style" - ) - }) + lintr_deprecated("paren_brace_linter", new = "brace_linter", version = "3.0.0", type = "Linter", signal = "stop") } #' Semicolon linter @@ -169,12 +46,9 @@ semicolon_terminator_linter <- function(semicolon = c("compound", "trailing")) { old = "semicolon_terminator_linter", new = "semicolon_linter", version = "3.0.0", - type = "Linter" + type = "Linter", + signal = "stop" ) - semicolon <- match.arg(semicolon, several.ok = TRUE) - allow_compound <- !"compound" %in% semicolon - allow_trailing <- !"trailing" %in% semicolon - semicolon_linter(allow_compound, allow_trailing) } #' Unnecessary concatenation linter diff --git a/R/with.R b/R/with.R index ca637cffc..2d35e4961 100644 --- a/R/with.R +++ b/R/with.R @@ -145,7 +145,7 @@ all_linters <- function(packages = "lintr", ...) { #' The result of this function is meant to be passed to the `linters` argument of `lint()`, #' or to be put in your configuration file. #' -#' @param defaults,default Default list of linters to modify. Must be named. +#' @param defaults Default list of linters to modify. Must be named. #' @inheritParams linters_with_tags #' @examplesIf requireNamespace("withr", quietly = TRUE) #' # When using interactively you will usually pass the result onto `lint` or `lint_package()` @@ -194,13 +194,10 @@ linters_with_defaults <- function(..., defaults = default_linters) { modify_defaults(..., defaults = defaults) } -#' @rdname linters_with_defaults +#' @rdname lintr-deprecated #' @export with_defaults <- function(..., default = default_linters) { - lintr_deprecated("with_defaults", "linters_with_defaults or modify_defaults", "3.0.0") - # to ease the burden of transition -- default = NULL used to behave like defaults = list() now does - if (is.null(default)) default <- list() - linters_with_defaults(..., defaults = default) + lintr_deprecated("with_defaults", "linters_with_defaults or modify_defaults", "3.0.0", signal = "stop") } #' @keywords internal diff --git a/R/with_id.R b/R/with_id.R index 97e40b894..5dc21dd53 100644 --- a/R/with_id.R +++ b/R/with_id.R @@ -11,8 +11,11 @@ #' @export with_id <- function(source_expression, id, source_file) { if (!missing(source_file)) { - lintr_deprecated(old = "source_file", new = "source_expression", version = "3.0.0", type = "Argument") - source_expression <- source_file + lintr_deprecated( + old = "source_file", new = "source_expression", version = "3.0.0", + type = "Argument", + signal = "stop" + ) } if (!is_lint_level(source_expression, "expression")) { return(data.frame()) diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 334f6167d..0308fa3f9 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -7,7 +7,7 @@ backport_linter,robustness configurable package_development boolean_arithmetic_linter,efficiency best_practices readability brace_linter,style readability default configurable class_equals_linter,best_practices robustness consistency -closed_curly_linter,style readability deprecated configurable +closed_curly_linter,defunct commas_linter,style readability default configurable commented_code_linter,style readability best_practices default comparison_negation_linter,readability consistency @@ -61,11 +61,11 @@ numeric_leading_zero_linter,style consistency readability object_length_linter,style readability default configurable executing object_name_linter,style consistency default configurable executing object_usage_linter,style readability correctness default executing configurable -open_curly_linter,style readability deprecated configurable +open_curly_linter,defunct outer_negation_linter,readability efficiency best_practices package_hooks_linter,style correctness package_development paren_body_linter,style readability default -paren_brace_linter,style readability deprecated +paren_brace_linter,defunct paste_linter,best_practices consistency configurable pipe_call_linter,style readability pipe_consistency_linter,style readability configurable @@ -80,7 +80,7 @@ routine_registration_linter,best_practices efficiency robustness sample_int_linter,efficiency readability robustness scalar_in_linter,readability consistency best_practices efficiency semicolon_linter,style readability default configurable -semicolon_terminator_linter,style readability deprecated configurable +semicolon_terminator_linter,defunct seq_linter,robustness efficiency consistency best_practices default single_quotes_linter,style consistency readability deprecated sort_linter,readability best_practices efficiency diff --git a/man/available_linters.Rd b/man/available_linters.Rd index ce75e764c..9aa4516bf 100644 --- a/man/available_linters.Rd +++ b/man/available_linters.Rd @@ -17,8 +17,9 @@ returned. If \code{tags} is \code{NULL}, all linters will be returned. See \code tags are already used by lintr.} \item{exclude_tags}{Tags to exclude from the results. Linters with at least one matching tag will not be returned. -If \code{except_tags} is \code{NULL}, no linters will be excluded. Note that \code{tags} takes priority, meaning that any -tag found in both \code{tags} and \code{exclude_tags} will be included, not excluded.} +If \code{exclude_tags} is \code{NULL}, no linters will be excluded. Note that \code{tags} takes priority, meaning that any +tag found in both \code{tags} and \code{exclude_tags} will be included, not excluded. Note that linters with tag \code{"defunct"} +(which do not work and can no longer be run) cannot be queried directly. See \link{lintr-deprecated} instead.} } \value{ \code{available_linters} returns a data frame with columns 'linter', 'package' and 'tags': diff --git a/man/deprecated_linters.Rd b/man/deprecated_linters.Rd index 8c30c6d73..08de6c677 100644 --- a/man/deprecated_linters.Rd +++ b/man/deprecated_linters.Rd @@ -5,7 +5,7 @@ \title{Deprecated linters} \description{ Linters that are deprecated and provided for backwards compatibility only. -These linters will be excluded from \code{linters_with_tags()} by default. +These linters will be excluded from \code{\link[=linters_with_tags]{linters_with_tags()}} by default. } \seealso{ \link{linters} for a complete list of linters available in lintr. @@ -13,12 +13,8 @@ These linters will be excluded from \code{linters_with_tags()} by default. \section{Linters}{ The following linters are tagged with 'deprecated': \itemize{ -\item{\code{\link{closed_curly_linter}}} \item{\code{\link{consecutive_stopifnot_linter}}} \item{\code{\link{no_tab_linter}}} -\item{\code{\link{open_curly_linter}}} -\item{\code{\link{paren_brace_linter}}} -\item{\code{\link{semicolon_terminator_linter}}} \item{\code{\link{single_quotes_linter}}} \item{\code{\link{unneeded_concatenation_linter}}} } diff --git a/man/linters.Rd b/man/linters.Rd index 91d035bb0..1c64865bc 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -23,7 +23,7 @@ The following tags exist: \item{\link[=consistency_linters]{consistency} (24 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (25 linters)} -\item{\link[=deprecated_linters]{deprecated} (8 linters)} +\item{\link[=deprecated_linters]{deprecated} (4 linters)} \item{\link[=efficiency_linters]{efficiency} (26 linters)} \item{\link[=executing_linters]{executing} (5 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} diff --git a/man/linters_with_defaults.Rd b/man/linters_with_defaults.Rd index 0af77a1eb..183986a67 100644 --- a/man/linters_with_defaults.Rd +++ b/man/linters_with_defaults.Rd @@ -2,19 +2,16 @@ % Please edit documentation in R/with.R \name{linters_with_defaults} \alias{linters_with_defaults} -\alias{with_defaults} \title{Create a linter configuration based on defaults} \usage{ linters_with_defaults(..., defaults = default_linters) - -with_defaults(..., default = default_linters) } \arguments{ \item{...}{Arguments of elements to change. If unnamed, the argument is automatically named. If the named argument already exists in the list of linters, it is replaced by the new element. If it does not exist, it is added. If the value is \code{NULL}, the linter is removed.} -\item{defaults, default}{Default list of linters to modify. Must be named.} +\item{defaults}{Default list of linters to modify. Must be named.} } \description{ Make a new list based on \pkg{lintr}'s default linters. diff --git a/man/linters_with_tags.Rd b/man/linters_with_tags.Rd index eb8ed740e..d7cdbb096 100644 --- a/man/linters_with_tags.Rd +++ b/man/linters_with_tags.Rd @@ -18,8 +18,9 @@ If it does not exist, it is added. If the value is \code{NULL}, the linter is re \item{packages}{A character vector of packages to search for linters.} \item{exclude_tags}{Tags to exclude from the results. Linters with at least one matching tag will not be returned. -If \code{except_tags} is \code{NULL}, no linters will be excluded. Note that \code{tags} takes priority, meaning that any -tag found in both \code{tags} and \code{exclude_tags} will be included, not excluded.} +If \code{exclude_tags} is \code{NULL}, no linters will be excluded. Note that \code{tags} takes priority, meaning that any +tag found in both \code{tags} and \code{exclude_tags} will be included, not excluded. Note that linters with tag \code{"defunct"} +(which do not work and can no longer be run) cannot be queried directly. See \link{lintr-deprecated} instead.} } \value{ A modified list of linters. diff --git a/man/lintr-deprecated.Rd b/man/lintr-deprecated.Rd index bf71d4981..73b3f62f5 100644 --- a/man/lintr-deprecated.Rd +++ b/man/lintr-deprecated.Rd @@ -1,5 +1,5 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/lintr-deprecated.R +% Please edit documentation in R/lintr-deprecated.R, R/with.R \name{lintr-deprecated} \alias{lintr-deprecated} \alias{closed_curly_linter} @@ -10,6 +10,7 @@ \alias{single_quotes_linter} \alias{consecutive_stopifnot_linter} \alias{no_tab_linter} +\alias{with_defaults} \title{Deprecated functions in lintr} \usage{ closed_curly_linter(allow_single_line = FALSE) @@ -27,27 +28,19 @@ single_quotes_linter() consecutive_stopifnot_linter() no_tab_linter() + +with_defaults(..., default = default_linters) } \arguments{ -\item{allow_single_line}{if \code{TRUE}, allow an open and closed curly pair on the same line.} - -\item{semicolon}{A character vector defining which semicolons to report: -\describe{ -\item{compound}{Semicolons that separate two statements on the same line.} -\item{trailing}{Semicolons following the last statement on the line.} -}} +\item{allow_single_line, semicolon}{Irrelevant parameters to defunct linters.} } \description{ These functions have been deprecated from lintr. \itemize{ -\item \code{open_curly_linter()} and \code{closed_curly_linter()} check that open and closed curly braces -are on their own line unless they follow an else, a comma, or a closing bracket. -Deprecated in favor of \code{brace_linter()}. -\item \code{paren_brace_linter()} checks that there is a space between right parentheses and an opening -curly brace. E.g., \code{function(){}} doesn't have a space, while \code{function() {}} does. -Deprecated in favor of \code{brace_linter()}. -\item \code{semicolon_terminator_linter()} checks that no semicolons terminate expressions. -Deprecated in favor of \code{semicolon_linter()}. +\item \code{open_curly_linter()} (use \code{\link[=brace_linter]{brace_linter()}}) +\item \code{closed_curly_linter()} (use \code{brace_linter()}) +\item \code{paren_brace_linter()} (use \code{brace_linter()}) +\item \code{semicolon_terminator_linter()} (use \code{\link[=semicolon_linter]{semicolon_linter()}}) } } \seealso{ @@ -55,5 +48,5 @@ Deprecated in favor of \code{semicolon_linter()}. } \keyword{internal} \section{Tags}{ -\link[=configurable_linters]{configurable}, \link[=deprecated_linters]{deprecated}, \link[=readability_linters]{readability}, \link[=style_linters]{style} +\link[=consistency_linters]{consistency}, \link[=deprecated_linters]{deprecated}, \link[=readability_linters]{readability}, \link[=style_linters]{style} } diff --git a/tests/testthat/test-closed_curly_linter.R b/tests/testthat/test-closed_curly_linter.R deleted file mode 100644 index b0ca2af70..000000000 --- a/tests/testthat/test-closed_curly_linter.R +++ /dev/null @@ -1,78 +0,0 @@ -test_that("returns the correct linting", { - closed_curly_message_regex <- rex::rex( - paste( - "Closing curly-braces should always be on their own line,", - "unless they are followed by an else." - ) - ) - - expect_warning( - { - linter <- closed_curly_linter() - }, - "Linter closed_curly_linter was deprecated", - fixed = TRUE - ) - - expect_lint("blah", NULL, linter) - expect_lint("a <- function() {\n}", NULL, linter) - - expect_lint( - "a <- function() { 1 }", - closed_curly_message_regex, - linter - ) - - expect_lint( - "a <- function() { 1 }", - closed_curly_message_regex, - linter - ) - - expect_lint( - "a <- function() { 1 }", - NULL, - suppressWarnings(closed_curly_linter(allow_single_line = TRUE)) - ) - - expect_lint( - "a <- if(1) {\n 1} else {\n 2\n}", - closed_curly_message_regex, - linter - ) - - expect_lint( - "a <- if(1) {\n 1\n} else {\n 2}", - closed_curly_message_regex, - linter - ) - - expect_lint( - "a <- if(1) {\n 1} else {\n 2}", - list( - closed_curly_message_regex, - closed_curly_message_regex - ), - linter - ) - - expect_lint( - "eval(bquote({...}))", - NULL, - linter - ) - - expect_lint( - "fun({\n statements\n}, param)", - NULL, - linter - ) - - expect_lint( - "out <- lapply(stuff, function(i) {\n do_something(i)\n}) %>% unlist", - NULL, - linter - ) - - expect_lint("{{x}}", NULL, linter) -}) diff --git a/tests/testthat/test-ids_with_token.R b/tests/testthat/test-ids_with_token.R index 7b4365545..39af0db6e 100644 --- a/tests/testthat/test-ids_with_token.R +++ b/tests/testthat/test-ids_with_token.R @@ -5,11 +5,8 @@ test_that("ids_with_token works as expected", { expect_identical(source_expression$parsed_content$token[ref], rep_len("expr", length(ref))) # deprecated argument - expect_warning( - { - old_arg <- ids_with_token(source_file = source_expression, value = "expr") - }, + expect_error( + ids_with_token(source_file = source_expression, value = "expr"), "Argument source_file was deprecated" ) - expect_identical(old_arg, ref) }) diff --git a/tests/testthat/test-lint.R b/tests/testthat/test-lint.R index 9295c14ac..0df64dcd1 100644 --- a/tests/testthat/test-lint.R +++ b/tests/testthat/test-lint.R @@ -150,8 +150,8 @@ test_that("exclusions work with custom linter names", { ) }) -test_that("compatibility warnings work", { - expect_warning( +test_that("old compatibility usage errors", { + expect_error( expect_lint( "a == NA", "Use is.na", @@ -161,7 +161,7 @@ test_that("compatibility warnings work", { fixed = TRUE ) - expect_warning( + expect_error( expect_lint( "a = 42", "Use <-", @@ -172,7 +172,7 @@ test_that("compatibility warnings work", { ) # Also within `linters_with_defaults()` (#1725) - expect_warning( + expect_error( expect_lint( "a = 42", "Use <-", @@ -182,7 +182,7 @@ test_that("compatibility warnings work", { fixed = TRUE ) - expect_warning( + expect_error( expect_lint( "a == NA", "Use is.na", @@ -193,7 +193,7 @@ test_that("compatibility warnings work", { ) # Trigger compatibility in auto_names() - expect_warning( + expect_error( expect_lint( "a == NA", "Use is.na", @@ -204,12 +204,8 @@ test_that("compatibility warnings work", { ) expect_error( - expect_warning( - lint("a <- 1\n", linters = function(two, arguments) NULL), - regexp = "The use of linters of class 'function'", - fixed = TRUE - ), - regexp = "`fun` must be a function taking exactly one argument", + lint("a <- 1\n", linters = function(two, arguments) NULL), + regexp = "The use of linters of class 'function'", fixed = TRUE ) diff --git a/tests/testthat/test-linter_tags.R b/tests/testthat/test-linter_tags.R index ca86a7d2f..218c813ee 100644 --- a/tests/testthat/test-linter_tags.R +++ b/tests/testthat/test-linter_tags.R @@ -79,11 +79,14 @@ test_that("warnings occur only for deprecated linters", { test_that("available_linters matches the set of linters available from lintr", { lintr_db <- available_linters(exclude_tags = NULL) - linters_in_namespace <- setdiff(ls(asNamespace("lintr"), pattern = "_linter$"), "is_linter") + defunct_linters <- + subset(read.csv(system.file("lintr", "linters.csv", package = "lintr"), as.is = TRUE), tags == "defunct")$linter + ignore <- c("is_linter", defunct_linters) + linters_in_namespace <- setdiff(ls(asNamespace("lintr"), pattern = "_linter$"), ignore) # ensure that the contents of inst/lintr/linters.csv covers all _linter objects in our namespace expect_identical(sort(lintr_db$linter), sort(linters_in_namespace)) # ensure that all _linter objects in our namespace are also exported - exported_linters <- setdiff(grep("_linter$", getNamespaceExports("lintr"), value = TRUE), "is_linter") + exported_linters <- setdiff(grep("_linter$", getNamespaceExports("lintr"), value = TRUE), ignore) expect_identical(sort(linters_in_namespace), sort(exported_linters)) }) diff --git a/tests/testthat/test-methods.R b/tests/testthat/test-methods.R index f9def5807..f58c1747f 100644 --- a/tests/testthat/test-methods.R +++ b/tests/testthat/test-methods.R @@ -43,7 +43,7 @@ test_that("as.data.frame.lints", { ) expect_s3_class(l2, "lint") - expect_warning( + expect_error( Lint("dummy.R", linter = "deprecated"), regexp = "deprecated", fixed = TRUE diff --git a/tests/testthat/test-open_curly_linter.R b/tests/testthat/test-open_curly_linter.R deleted file mode 100644 index c64846bc9..000000000 --- a/tests/testthat/test-open_curly_linter.R +++ /dev/null @@ -1,109 +0,0 @@ -test_that("returns the correct linting", { - expect_warning( - { - linter <- open_curly_linter() - }, - "Linter open_curly_linter was deprecated", - fixed = TRUE - ) - - expect_lint("blah", NULL, linter) - - expect_lint( - trim_some(" - a <- function() { - } - "), - NULL, - linter - ) - - expect_lint( - trim_some(' - pkg_name <- function(path = find_package()) { - if (is.null(path)) { - return(NULL) - } else { - read.dcf(file.path(path, "DESCRIPTION"), fields = "Package")[1] - } - } - '), - NULL, - linter - ) - - expect_lint( - trim_some(" - a <- function() - { - 1 - } - "), - rex::rex("Opening curly braces should never go on their own line."), - linter - ) - - expect_lint( - trim_some(" - a <- function() - { - 1 - } - "), - rex::rex("Opening curly braces should never go on their own line."), - linter - ) - - expect_lint( - trim_some(" - a <- function() - \t{ - 1 - } - "), - rex::rex("Opening curly braces should never go on their own line"), - linter - ) - - # trailing whitespace _doesn't_ trigger a lint (it used to; leave that to trailing_whitespace_linter now) - expect_lint("a <- function() { \n}", NULL, linter) - - expect_lint( - "a <- function() { 1 }", - rex::rex("Opening curly braces should always be followed by a new line"), - linter - ) - - expect_lint( - trim_some(' - if ("P" != "NP") { # what most people expect - print("Cryptomania is possible") - } - '), - NULL, - linter - ) - - expect_lint("{{x}}", NULL, linter) -}) - -test_that("allow_single_line=TRUE works", { - expect_warning( - { - linter <- open_curly_linter(allow_single_line = TRUE) - }, - "Linter open_curly_linter was deprecated", - fixed = TRUE - ) - - expect_lint("a <- function() { 1 }", NULL, linter) - - expect_lint( - trim_some(" - a <- function() { 1 - 2 } - "), - rex::rex("Opening curly braces should always be followed by a new line unless"), - linter - ) -}) diff --git a/tests/testthat/test-paren_brace_linter.R b/tests/testthat/test-paren_brace_linter.R deleted file mode 100644 index 44a9f0785..000000000 --- a/tests/testthat/test-paren_brace_linter.R +++ /dev/null @@ -1,50 +0,0 @@ -test_that("returns the correct linting", { - expect_warning( - { - linter <- paren_brace_linter() - }, - "Linter paren_brace_linter was deprecated", - fixed = TRUE - ) - lint_msg <- rex::rex("There should be a space between right parenthesis and an opening curly brace.") - - expect_lint("blah", NULL, linter) - expect_lint("blah <- function() {}", NULL, linter) - expect_lint("blah <- function() {\n}", NULL, linter) - - expect_lint( - "blah <- function(){}", - list( - message = lint_msg, - column_number = 19L - ), - linter - ) - - expect_lint( - "\nblah <- function(){\n\n\n}", - list( - message = lint_msg, - column_number = 19L - ), - linter - ) - - # paren_brace_linter should ignore strings and comments, as in regexes: - expect_lint("grepl('(iss){2}', 'Mississippi')", NULL, linter) - expect_lint( - "x <- 123 # don't flag (paren){brace} if inside a comment", - NULL, - linter - ) - # paren-brace lint should not be thrown when the brace lies on subsequent line - expect_lint( - paste( - "x <- function()", - " {2}", - sep = "\n" - ), - NULL, - linter - ) -}) diff --git a/tests/testthat/test-semicolon_linter.R b/tests/testthat/test-semicolon_linter.R index e1c6c5ef5..ea809df37 100644 --- a/tests/testthat/test-semicolon_linter.R +++ b/tests/testthat/test-semicolon_linter.R @@ -103,46 +103,3 @@ test_that("Compound semicolons only", { fixed = TRUE ) }) - -test_that("deprecation notices for semicolon_terminator_linter succeed, and the deprecated version works", { - expect_warning( - { - linter <- semicolon_terminator_linter() - }, - "Linter semicolon_terminator_linter was deprecated", - fixed = TRUE - ) - expect_lint("a <- 1", NULL, linter) - expect_lint("a <- 1;", rex::rex("Trailing semicolons are not needed."), linter) - expect_lint("a <- 1; b <- 2", rex::rex("Compound semicolons are discouraged."), linter) - - # old string argument gets translated to new boolean arguments - expect_warning( - { - linter <- semicolon_terminator_linter("compound") - }, - "Linter semicolon_terminator_linter was deprecated", - fixed = TRUE - ) - expect_lint("a <- 1", NULL, linter) - expect_lint("a <- 1;", NULL, linter) - expect_lint("a <- 1; b <- 2", rex::rex("Compound semicolons are discouraged."), linter) - - expect_warning( - { - linter <- semicolon_terminator_linter("trailing") - }, - "Linter semicolon_terminator_linter was deprecated", - fixed = TRUE - ) - expect_lint("a <- 1", NULL, linter) - expect_lint("a <- 1;", rex::rex("Trailing semicolons are not needed."), linter) - expect_lint("a <- 1; b <- 2", NULL, linter) - - # linters_with_defaults warns about now-absent semicolon_terminator_linter - expect_warning( - expect_true("semicolon_linter" %in% names(linters_with_defaults(semicolon_terminator_linter = NULL))), - # regex because the message uses sQuote() --> fancy quotes - rex::rex("Trying to remove", anything, "semicolon_terminator_linter", anything, ", which is not in `defaults`.") - ) -}) diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index 03b4259d6..6b4fc00a8 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -68,25 +68,11 @@ test_that("can instantiate all linters without arguments", { expect_length(really_all_linters, nrow(available_linters(exclude_tags = NULL))) }) -test_that("with_defaults is supported with a deprecation warning", { - defaults <- linters_with_defaults() - expect_warning( - { - old_defaults <- with_defaults() - }, - rex::rex("Use linters_with_defaults or modify_defaults instead.") - ) - expect_identical(defaults, old_defaults) - - # linters_with_defaults only accepts `defaults = list()` to start from blank - defaults <- linters_with_defaults(defaults = list(), whitespace_linter()) - expect_warning( - { - old_defaults <- with_defaults(default = NULL, whitespace_linter()) - }, +test_that("with_defaults is fully deprecated", { + expect_error( + with_defaults(), rex::rex("Use linters_with_defaults or modify_defaults instead.") ) - expect_identical(defaults, old_defaults) }) test_that("modify_defaults works", { diff --git a/tests/testthat/test-with_id.R b/tests/testthat/test-with_id.R index d6ef6762b..f8a418985 100644 --- a/tests/testthat/test-with_id.R +++ b/tests/testthat/test-with_id.R @@ -8,14 +8,11 @@ test_that("with_id works as expected", { expect_identical(ref$token, rep_len("expr", nrow(ref))) # deprecated argument - expect_warning( - { - old_arg <- with_id( - source_file = source_expression, - id = ids_with_token(source_expression = source_expression, value = "expr") - ) - }, + expect_error( + with_id( + source_file = source_expression, + id = ids_with_token(source_expression = source_expression, value = "expr") + ), "Argument source_file was deprecated" ) - expect_identical(old_arg, ref) }) From 79702600aa82f7ebad1bfac9d0ff7c4e38c5c227 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 18 Nov 2023 12:09:01 -0800 Subject: [PATCH 2/3] New list_comparison_linter (#2293) --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 1 + R/list_comparison_linter.R | 70 ++++++++++++++++++++ inst/lintr/linters.csv | 1 + man/best_practices_linters.Rd | 1 + man/common_mistakes_linters.Rd | 1 + man/efficiency_linters.Rd | 1 + man/linters.Rd | 7 +- man/list_comparison_linter.Rd | 33 +++++++++ tests/testthat/test-list_comparison_linter.R | 34 ++++++++++ 11 files changed, 148 insertions(+), 3 deletions(-) create mode 100644 R/list_comparison_linter.R create mode 100644 man/list_comparison_linter.Rd create mode 100644 tests/testthat/test-list_comparison_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 7e0f6250d..183ce9867 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -128,6 +128,7 @@ Collate: 'linter_tags.R' 'lintr-deprecated.R' 'lintr-package.R' + 'list_comparison_linter.R' 'literal_coercion_linter.R' 'make_linter_from_regex.R' 'matrix_apply_linter.R' diff --git a/NAMESPACE b/NAMESPACE index 8be33824d..a240beb6a 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -90,6 +90,7 @@ export(lint_dir) export(lint_package) export(linters_with_defaults) export(linters_with_tags) +export(list_comparison_linter) export(literal_coercion_linter) export(make_linter_from_xpath) export(matrix_apply_linter) diff --git a/NEWS.md b/NEWS.md index 9fe5f6b71..e5ca597dd 100644 --- a/NEWS.md +++ b/NEWS.md @@ -26,6 +26,7 @@ * `stopifnot_all_linter()` discourages tests with `all()` like `stopifnot(all(x > 0))`; `stopifnot()` runs `all()` itself, and uses a better error message (part of #884, @MichaelChirico). * `comparison_negation_linter()` for discouraging negated comparisons when a direct negation is preferable, e.g. `!(x == y)` could be `x != y` (part of #884, @MichaelChirico). * `terminal_close_linter()` for discouraging using `close()` to end functions (part of #884, @MichaelChirico). Such usages are not robust to errors, where `close()` will not be run as intended. Put `close()` in an `on.exit()` hook, or use {withr} to manage connections with proper cleanup. +* `list_comparison_linter()` for discouraging comparisons on the output of `lapply()`, e.g. `lapply(x, sum) > 10` (part of #884, @MichaelChirico). * `print_linter()` for discouraging usage of `print()` on string literals like `print("Reached here")` or `print(paste("Found", nrow(DF), "rows."))` (#1894, @MichaelChirico). ### Lint accuracy fixes: removing false positives diff --git a/R/list_comparison_linter.R b/R/list_comparison_linter.R new file mode 100644 index 000000000..60d5c03eb --- /dev/null +++ b/R/list_comparison_linter.R @@ -0,0 +1,70 @@ +#' Block usage of comparison operators with known-list() functions like lapply +#' +#' Usage like `lapply(x, sum) > 10` is awkward because the list must first +#' be coerced to a vector for comparison. A function like [vapply()] +#' should be preferred. +#' +#' @examples +#' # will produce lints +#' lint( +#' text = "lapply(x, sum) > 10", +#' linters = list_comparison_linter() +#' ) +#' +#' # okay +#' lint( +#' text = "unlist(lapply(x, sum)) > 10", +#' linters = list_comparison_linter() +#' ) +#' +#' @evalRd rd_tags("list_comparison_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +list_comparison_linter <- function() { + # TODO(michaelchirico): extend to cases where using simplify=FALSE implies a + # list output, e.g. with sapply, replicate, mapply. + list_mapper_alternatives <- c( + lapply = "vapply(x, FUN, character(1L))", + map = "map_chr(x, FUN)", + Map = "mapply()", + .mapply = "mapply()" + ) + + # NB: anchor to the comparison expr so that we can easily include the comparator + # in the lint message. + xpath <- glue(" + //SYMBOL_FUNCTION_CALL[{ xp_text_in_table(names(list_mapper_alternatives)) }] + /parent::expr + /parent::expr + /parent::expr[{ xp_or(infix_metadata$xml_tag[infix_metadata$comparator]) }] + ") + + Linter(function(source_expression) { + if (!is_lint_level(source_expression, "expression")) { + return(list()) + } + + xml <- source_expression$xml_parsed_content + + bad_expr <- xml_find_all(xml, xpath) + + list_mapper <- xp_call_name(bad_expr, depth = 2L) + + vector_mapper <- list_mapper_alternatives[list_mapper] + # we are at `x ? y` in which the comparator ? comes 2nd + comparator <- xml_find_chr(bad_expr, "string(*[2])") + + lint_message <- as.character(glue( + "The output of {list_mapper}(), a list(), is being ", + "coerced for comparison by `{comparator}`. ", + "Instead, use a mapper that generates a vector with the correct type ", + "directly, for example {vector_mapper} if the output is a string." + )) + xml_nodes_to_lints( + bad_expr, + source_expression = source_expression, + lint_message = lint_message, + type = "warning" + ) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 0308fa3f9..b1416540c 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -49,6 +49,7 @@ length_test_linter,common_mistakes efficiency lengths_linter,efficiency readability best_practices library_call_linter,style best_practices readability configurable line_length_linter,style readability default configurable +list_comparison_linter,best_practices common_mistakes efficiency literal_coercion_linter,best_practices consistency efficiency matrix_apply_linter,readability efficiency missing_argument_linter,correctness common_mistakes configurable diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index 4e8cc2ead..c57c9b6f3 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -43,6 +43,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{length_levels_linter}}} \item{\code{\link{lengths_linter}}} \item{\code{\link{library_call_linter}}} +\item{\code{\link{list_comparison_linter}}} \item{\code{\link{literal_coercion_linter}}} \item{\code{\link{nonportable_path_linter}}} \item{\code{\link{outer_negation_linter}}} diff --git a/man/common_mistakes_linters.Rd b/man/common_mistakes_linters.Rd index 73767f1f5..b21480c12 100644 --- a/man/common_mistakes_linters.Rd +++ b/man/common_mistakes_linters.Rd @@ -15,6 +15,7 @@ The following linters are tagged with 'common_mistakes': \item{\code{\link{duplicate_argument_linter}}} \item{\code{\link{equals_na_linter}}} \item{\code{\link{length_test_linter}}} +\item{\code{\link{list_comparison_linter}}} \item{\code{\link{missing_argument_linter}}} \item{\code{\link{missing_package_linter}}} \item{\code{\link{redundant_equals_linter}}} diff --git a/man/efficiency_linters.Rd b/man/efficiency_linters.Rd index 146c0be54..334c4573f 100644 --- a/man/efficiency_linters.Rd +++ b/man/efficiency_linters.Rd @@ -20,6 +20,7 @@ The following linters are tagged with 'efficiency': \item{\code{\link{inner_combine_linter}}} \item{\code{\link{length_test_linter}}} \item{\code{\link{lengths_linter}}} +\item{\code{\link{list_comparison_linter}}} \item{\code{\link{literal_coercion_linter}}} \item{\code{\link{matrix_apply_linter}}} \item{\code{\link{nested_ifelse_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index 1c64865bc..ac13ed35f 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,14 +17,14 @@ see also \code{\link[=available_tags]{available_tags()}}. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (56 linters)} -\item{\link[=common_mistakes_linters]{common_mistakes} (8 linters)} +\item{\link[=best_practices_linters]{best_practices} (57 linters)} +\item{\link[=common_mistakes_linters]{common_mistakes} (9 linters)} \item{\link[=configurable_linters]{configurable} (34 linters)} \item{\link[=consistency_linters]{consistency} (24 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (25 linters)} \item{\link[=deprecated_linters]{deprecated} (4 linters)} -\item{\link[=efficiency_linters]{efficiency} (26 linters)} +\item{\link[=efficiency_linters]{efficiency} (27 linters)} \item{\link[=executing_linters]{executing} (5 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} \item{\link[=pkg_testthat_linters]{pkg_testthat} (12 linters)} @@ -84,6 +84,7 @@ The following linters exist: \item{\code{\link{lengths_linter}} (tags: best_practices, efficiency, readability)} \item{\code{\link{library_call_linter}} (tags: best_practices, configurable, readability, style)} \item{\code{\link{line_length_linter}} (tags: configurable, default, readability, style)} +\item{\code{\link{list_comparison_linter}} (tags: best_practices, common_mistakes, efficiency)} \item{\code{\link{literal_coercion_linter}} (tags: best_practices, consistency, efficiency)} \item{\code{\link{matrix_apply_linter}} (tags: efficiency, readability)} \item{\code{\link{missing_argument_linter}} (tags: common_mistakes, configurable, correctness)} diff --git a/man/list_comparison_linter.Rd b/man/list_comparison_linter.Rd new file mode 100644 index 000000000..9a939a686 --- /dev/null +++ b/man/list_comparison_linter.Rd @@ -0,0 +1,33 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/list_comparison_linter.R +\name{list_comparison_linter} +\alias{list_comparison_linter} +\title{Block usage of comparison operators with known-list() functions like lapply} +\usage{ +list_comparison_linter() +} +\description{ +Usage like \code{lapply(x, sum) > 10} is awkward because the list must first +be coerced to a vector for comparison. A function like \code{\link[=vapply]{vapply()}} +should be preferred. +} +\examples{ +# will produce lints +lint( + text = "lapply(x, sum) > 10", + linters = list_comparison_linter() +) + +# okay +lint( + text = "unlist(lapply(x, sum)) > 10", + linters = list_comparison_linter() +) + +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=common_mistakes_linters]{common_mistakes}, \link[=efficiency_linters]{efficiency} +} diff --git a/tests/testthat/test-list_comparison_linter.R b/tests/testthat/test-list_comparison_linter.R new file mode 100644 index 000000000..6177caed1 --- /dev/null +++ b/tests/testthat/test-list_comparison_linter.R @@ -0,0 +1,34 @@ +test_that("list_comparison_linter skips allowed usages", { + expect_lint("sapply(x, sum) > 10", NULL, list_comparison_linter()) +}) + +local({ + linter <- list_comparison_linter() + lint_msg <- rex::rex("a list(), is being coerced for comparison") + + cases <- expand.grid( + list_mapper = c("lapply", "map", "Map", ".mapply"), + comparator = c("==", "!=", ">=", "<=", ">", "<") + ) + cases$.test_name <- with(cases, paste(list_mapper, comparator)) + patrick::with_parameters_test_that( + "list_comparison_linter blocks simple disallowed usages", + expect_lint(sprintf("%s(x, sum) %s 10", list_mapper, comparator), lint_msg, linter), + .cases = cases + ) +}) + +test_that("list_comparison_linter vectorizes", { + expect_lint( + trim_some("{ + sapply(x, sum) > 10 + .mapply(`+`, list(1:10, 1:10), NULL) == 2 + lapply(x, sum) < 5 + }"), + list( + list(rex::rex(".mapply()", anything, "`==`"), line_number = 3L), + list(rex::rex("lapply()", anything, "`<`"), line_number = 4L) + ), + list_comparison_linter() + ) +}) From 1d84caeac81a821e429d3f83ea822ba055f2e1aa Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 18 Nov 2023 13:01:38 -0800 Subject: [PATCH 3/3] New nzchar_linter (#2275) * New nzchar_linter * tag NEWS * use infix_metadata * shorter lint message * examples * more metadata testing --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 1 + R/expect_comparison_linter.R | 2 +- R/nzchar_linter.R | 128 ++++++++++++++++++++++++++++ inst/lintr/linters.csv | 1 + man/best_practices_linters.Rd | 1 + man/consistency_linters.Rd | 1 + man/efficiency_linters.Rd | 1 + man/linters.Rd | 7 +- man/nzchar_linter.Rd | 50 +++++++++++ tests/testthat/test-nzchar_linter.R | 74 ++++++++++++++++ 12 files changed, 264 insertions(+), 4 deletions(-) create mode 100644 R/nzchar_linter.R create mode 100644 man/nzchar_linter.Rd create mode 100644 tests/testthat/test-nzchar_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 183ce9867..3b06ad275 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -140,6 +140,7 @@ Collate: 'nested_ifelse_linter.R' 'nonportable_path_linter.R' 'numeric_leading_zero_linter.R' + 'nzchar_linter.R' 'object_length_linter.R' 'object_name_linter.R' 'object_usage_linter.R' diff --git a/NAMESPACE b/NAMESPACE index a240beb6a..e0a399379 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -102,6 +102,7 @@ export(nested_ifelse_linter) export(no_tab_linter) export(nonportable_path_linter) export(numeric_leading_zero_linter) +export(nzchar_linter) export(object_length_linter) export(object_name_linter) export(object_usage_linter) diff --git a/NEWS.md b/NEWS.md index e5ca597dd..5af6e63a6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -25,6 +25,7 @@ * `sample_int_linter()` for encouraging `sample.int(n, ...)` over equivalents like `sample(1:n, ...)` (part of #884, @MichaelChirico). * `stopifnot_all_linter()` discourages tests with `all()` like `stopifnot(all(x > 0))`; `stopifnot()` runs `all()` itself, and uses a better error message (part of #884, @MichaelChirico). * `comparison_negation_linter()` for discouraging negated comparisons when a direct negation is preferable, e.g. `!(x == y)` could be `x != y` (part of #884, @MichaelChirico). +* `nzchar_linter()` for encouraging `nzchar()` to test for empty strings, e.g. `nchar(x) > 0` can be `nzchar(x)` (part of #884, @MichaelChirico). * `terminal_close_linter()` for discouraging using `close()` to end functions (part of #884, @MichaelChirico). Such usages are not robust to errors, where `close()` will not be run as intended. Put `close()` in an `on.exit()` hook, or use {withr} to manage connections with proper cleanup. * `list_comparison_linter()` for discouraging comparisons on the output of `lapply()`, e.g. `lapply(x, sum) > 10` (part of #884, @MichaelChirico). * `print_linter()` for discouraging usage of `print()` on string literals like `print("Reached here")` or `print(paste("Found", nrow(DF), "rows."))` (#1894, @MichaelChirico). diff --git a/R/expect_comparison_linter.R b/R/expect_comparison_linter.R index e9801bf35..761882002 100644 --- a/R/expect_comparison_linter.R +++ b/R/expect_comparison_linter.R @@ -49,7 +49,7 @@ #' @export expect_comparison_linter <- function() { # != doesn't have a clean replacement - comparator_nodes <- setdiff(as.list(infix_metadata$xml_tag[infix_metadata$comparator]), "NE") + comparator_nodes <- setdiff(infix_metadata$xml_tag[infix_metadata$comparator], "NE") xpath <- glue(" //SYMBOL_FUNCTION_CALL[text() = 'expect_true'] /parent::expr diff --git a/R/nzchar_linter.R b/R/nzchar_linter.R new file mode 100644 index 000000000..db95d179b --- /dev/null +++ b/R/nzchar_linter.R @@ -0,0 +1,128 @@ +#' Require usage of nzchar where appropriate +#' +#' [nzchar()] efficiently determines which of a vector of strings are empty +#' (i.e., are `""`). It should in most cases be used instead of +#' constructions like `string == ""` or `nchar(string) == 0`. +#' +#' One crucial difference is in the default handling of `NA_character_`, i.e., +#' missing strings. `nzchar(NA_character_)` is `TRUE`, while `NA_character_ == ""` +#' and `nchar(NA_character_) == 0` are both `NA`. +#' +#' @examples +#' # will produce lints +#' lint( +#' text = "x[x == '']", +#' linters = nzchar_linter() +#' ) +#' +#' lint( +#' text = "x[nchar(x) > 0]", +#' linters = nzchar_linter() +#' ) +#' +#' # okay +#' lint( +#' text = "x[nchar(x) > 1]", +#' linters = nzchar_linter() +#' ) +#' +#' # nzchar()'s primary benefit is for vector input; +#' # for guaranteed-scalar cases like if() conditions, comparing to "" is OK. +#' lint( +#' text = "if (x == '') y", +#' linters = nzchar_linter() +#' ) +#' +#' @evalRd rd_tags("nzchar_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +nzchar_linter <- function() { + comparator_nodes <- infix_metadata$xml_tag[infix_metadata$comparator] + + # use string-length to capture both "" and '' + # if (any(x == "")) is not treated like it's part of if(), but + # any(if (x == "") y else z) _is_ treated so. this condition looks for the + # expr to be inside a call that's _not_ above an IF/WHILE. + comparison_xpath <- glue(" + //STR_CONST[string-length(text()) = 2] + /parent::expr + /parent::expr[ + ({ xp_or(comparator_nodes) }) + and ( + not(ancestor-or-self::expr[ + preceding-sibling::IF + or preceding-sibling::WHILE + ]) + or ancestor-or-self::expr[ + ( + preceding-sibling::expr[SYMBOL_FUNCTION_CALL] + or preceding-sibling::OP-LEFT-BRACKET + ) and not( + descendant-or-self::expr[IF or WHILE] + ) + ] + ) + ] + ") + + # nchar(., type="width") not strictly compatible with nzchar + # unsure allowNA compatible, so allow it just in case (see TODO in tests) + nchar_xpath <- glue(" + //SYMBOL_FUNCTION_CALL[text() = 'nchar'] + /parent::expr + /parent::expr + /parent::expr[ + ({ xp_or(comparator_nodes) }) + and not(expr[SYMBOL_SUB[ + ( + text() = 'type' + and following-sibling::expr[1][STR_CONST[contains(text(), 'width')]] + ) or ( + text() = 'allowNA' + and following-sibling::expr[1][NUM_CONST[text() = 'TRUE']] + ) + ]]) + and expr[NUM_CONST[text() = '0' or text() = '0L' or text() = '0.0']] + ] + ") + + keepna_note <- paste( + "Whenever missing data is possible,", + "please take care to use nzchar(., keepNA = TRUE);", + "nzchar(NA) is TRUE by default." + ) + + Linter(function(source_expression) { + if (!is_lint_level(source_expression, "expression")) { + return(list()) + } + + xml <- source_expression$xml_parsed_content + + comparison_expr <- xml_find_all(xml, comparison_xpath) + comparison_lints <- xml_nodes_to_lints( + comparison_expr, + source_expression = source_expression, + lint_message = paste( + 'Instead of comparing strings to "", use nzchar().', + "Note that if x is a factor, you'll have use ", + 'as.character() to replicate an implicit conversion that happens in x == "".', + keepna_note + ), + type = "warning" + ) + + nchar_expr <- xml_find_all(xml, nchar_xpath) + nchar_lints <- xml_nodes_to_lints( + nchar_expr, + source_expression = source_expression, + lint_message = paste( + "Instead of comparing nchar(x) to 0, use nzchar().", + keepna_note + ), + type = "warning" + ) + + c(comparison_lints, nchar_lints) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index b1416540c..ccea2ead7 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -59,6 +59,7 @@ nested_ifelse_linter,efficiency readability no_tab_linter,style consistency deprecated nonportable_path_linter,robustness best_practices configurable numeric_leading_zero_linter,style consistency readability +nzchar_linter,efficiency best_practices consistency object_length_linter,style readability default configurable executing object_name_linter,style consistency default configurable executing object_usage_linter,style readability correctness default executing configurable diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index c57c9b6f3..821acc0c2 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -46,6 +46,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{list_comparison_linter}}} \item{\code{\link{literal_coercion_linter}}} \item{\code{\link{nonportable_path_linter}}} +\item{\code{\link{nzchar_linter}}} \item{\code{\link{outer_negation_linter}}} \item{\code{\link{paste_linter}}} \item{\code{\link{print_linter}}} diff --git a/man/consistency_linters.Rd b/man/consistency_linters.Rd index 7d8a609c6..f8894aa29 100644 --- a/man/consistency_linters.Rd +++ b/man/consistency_linters.Rd @@ -27,6 +27,7 @@ The following linters are tagged with 'consistency': \item{\code{\link{length_levels_linter}}} \item{\code{\link{literal_coercion_linter}}} \item{\code{\link{numeric_leading_zero_linter}}} +\item{\code{\link{nzchar_linter}}} \item{\code{\link{object_name_linter}}} \item{\code{\link{paste_linter}}} \item{\code{\link{print_linter}}} diff --git a/man/efficiency_linters.Rd b/man/efficiency_linters.Rd index 334c4573f..fdeaff567 100644 --- a/man/efficiency_linters.Rd +++ b/man/efficiency_linters.Rd @@ -24,6 +24,7 @@ The following linters are tagged with 'efficiency': \item{\code{\link{literal_coercion_linter}}} \item{\code{\link{matrix_apply_linter}}} \item{\code{\link{nested_ifelse_linter}}} +\item{\code{\link{nzchar_linter}}} \item{\code{\link{outer_negation_linter}}} \item{\code{\link{redundant_equals_linter}}} \item{\code{\link{redundant_ifelse_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index ac13ed35f..e3a1cb7b8 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,14 +17,14 @@ see also \code{\link[=available_tags]{available_tags()}}. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (57 linters)} +\item{\link[=best_practices_linters]{best_practices} (58 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (9 linters)} \item{\link[=configurable_linters]{configurable} (34 linters)} -\item{\link[=consistency_linters]{consistency} (24 linters)} +\item{\link[=consistency_linters]{consistency} (25 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (25 linters)} \item{\link[=deprecated_linters]{deprecated} (4 linters)} -\item{\link[=efficiency_linters]{efficiency} (27 linters)} +\item{\link[=efficiency_linters]{efficiency} (28 linters)} \item{\link[=executing_linters]{executing} (5 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} \item{\link[=pkg_testthat_linters]{pkg_testthat} (12 linters)} @@ -93,6 +93,7 @@ The following linters exist: \item{\code{\link{nested_ifelse_linter}} (tags: efficiency, readability)} \item{\code{\link{nonportable_path_linter}} (tags: best_practices, configurable, robustness)} \item{\code{\link{numeric_leading_zero_linter}} (tags: consistency, readability, style)} +\item{\code{\link{nzchar_linter}} (tags: best_practices, consistency, efficiency)} \item{\code{\link{object_length_linter}} (tags: configurable, default, executing, readability, style)} \item{\code{\link{object_name_linter}} (tags: configurable, consistency, default, executing, style)} \item{\code{\link{object_usage_linter}} (tags: configurable, correctness, default, executing, readability, style)} diff --git a/man/nzchar_linter.Rd b/man/nzchar_linter.Rd new file mode 100644 index 000000000..d729bd63d --- /dev/null +++ b/man/nzchar_linter.Rd @@ -0,0 +1,50 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/nzchar_linter.R +\name{nzchar_linter} +\alias{nzchar_linter} +\title{Require usage of nzchar where appropriate} +\usage{ +nzchar_linter() +} +\description{ +\code{\link[=nzchar]{nzchar()}} efficiently determines which of a vector of strings are empty +(i.e., are \code{""}). It should in most cases be used instead of +constructions like \code{string == ""} or \code{nchar(string) == 0}. +} +\details{ +One crucial difference is in the default handling of \code{NA_character_}, i.e., +missing strings. \code{nzchar(NA_character_)} is \code{TRUE}, while \code{NA_character_ == ""} +and \code{nchar(NA_character_) == 0} are both \code{NA}. +} +\examples{ +# will produce lints +lint( + text = "x[x == '']", + linters = nzchar_linter() +) + +lint( + text = "x[nchar(x) > 0]", + linters = nzchar_linter() +) + +# okay +lint( + text = "x[nchar(x) > 1]", + linters = nzchar_linter() +) + +# nzchar()'s primary benefit is for vector input; +# for guaranteed-scalar cases like if() conditions, comparing to "" is OK. +lint( + text = "if (x == '') y", + linters = nzchar_linter() +) + +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=consistency_linters]{consistency}, \link[=efficiency_linters]{efficiency} +} diff --git a/tests/testthat/test-nzchar_linter.R b/tests/testthat/test-nzchar_linter.R new file mode 100644 index 000000000..4a3c65dca --- /dev/null +++ b/tests/testthat/test-nzchar_linter.R @@ -0,0 +1,74 @@ +test_that("nzchar_linter skips allowed usages", { + linter <- nzchar_linter() + + expect_lint("if (any(nzchar(x))) TRUE", NULL, linter) + + expect_lint("letters == 'a'", NULL, linter) + + expect_lint("which(nchar(x) == 4)", NULL, linter) + expect_lint("which(nchar(x) != 2)", NULL, linter) +}) + +test_that("nzchar_linter skips as appropriate for other nchar args", { + linter <- nzchar_linter() + + # using type="width" can lead to 0-width strings that are counted as + # nzchar, c.f. nchar("\u200b", type="width"), so don't lint this. + # type="bytes" should be >= the value for the default (type="chars") + expect_lint("nchar(x, type='width') == 0L", NULL, linter) + + # TODO(michaelchirico): check compatibility of nchar(., allowNA=TRUE). + # there are no examples in ?nchar, nor any relevant usages on StackOverflow. + # just assume they are incompatible now to be conservative. + expect_lint("nchar(x, allowNA=TRUE) == 0L", NULL, linter) + + # nzchar also has keepNA argument so a drop-in switch is easy + expect_lint( + "nchar(x, keepNA=TRUE) == 0", + rex::rex("Instead of comparing nchar(x) to 0"), + linter + ) +}) + +test_that("nzchar_linter blocks simple disallowed usages", { + linter <- nzchar_linter() + lint_msg_quote <- rex::rex('Instead of comparing strings to "", use nzchar()') + lint_msg_nchar <- rex::rex("Instead of comparing nchar(x) to 0") + + expect_lint("which(x == '')", lint_msg_quote, linter) + expect_lint("any(nchar(x) >= 0)", lint_msg_nchar, linter) + expect_lint("all(nchar(x) == 0L)", lint_msg_nchar, linter) + expect_lint("sum(0.0 < nchar(x))", lint_msg_nchar, linter) +}) + +test_that("nzchar_linter skips comparison to '' in if/while statements", { + linter <- nzchar_linter() + lint_msg_quote <- rex::rex('Instead of comparing strings to "", use nzchar()') + lint_msg_nchar <- rex::rex("Instead of comparing nchar(x) to 0") + + # still lint nchar() comparisons + expect_lint("if (nchar(x) > 0) TRUE", lint_msg_nchar, linter) + expect_lint('if (x == "") TRUE', NULL, linter) + expect_lint('while (x == "") TRUE', NULL, linter) + + # nested versions, a la nesting issues with vector_logic_linter + expect_lint('if (TRUE || (x == "" && FALSE)) TRUE', NULL, linter) + expect_lint('if (TRUE && x == "" && FALSE) TRUE', NULL, linter) + expect_lint('if (any(x == "")) TRUE', lint_msg_quote, linter) + expect_lint('if (TRUE || any(x == "" | FALSE)) TRUE', lint_msg_quote, linter) + expect_lint('foo(if (x == "") y else z)', NULL, linter) +}) + +test_that("multiple lints are generated correctly", { + expect_lint( + trim_some("{ + a == '' + nchar(b) != 0 + }"), + list( + list(rex::rex('Instead of comparing strings to ""'), line_number = 2L), + list(rex::rex("Instead of comparing nchar(x) to 0"), line_number = 3L) + ), + nzchar_linter() + ) +})