diff --git a/DESCRIPTION b/DESCRIPTION index 8d60cc417..9e6b2ec05 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -126,6 +126,7 @@ Collate: 'path_linters.R' 'pipe_call_linter.R' 'pipe_continuation_linter.R' + 'redundant_equals_linter.R' 'redundant_ifelse_linter.R' 'regex_subset_linter.R' 'semicolon_linter.R' diff --git a/NAMESPACE b/NAMESPACE index 991b17c78..cf3751932 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -92,6 +92,7 @@ export(paren_brace_linter) export(paste_linter) export(pipe_call_linter) export(pipe_continuation_linter) +export(redundant_equals_linter) export(redundant_ifelse_linter) export(regex_subset_linter) export(sarif_output) diff --git a/NEWS.md b/NEWS.md index 310ef9f8e..6c8802dd6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -22,6 +22,10 @@ * `object_usage_linter()` improves lint metadata when detecting undefined infix operators, e.g. `%>%` or `:=` (#1497, @MichaelChirico) +### New linters + +* `redundant_equals_linter()` for redundant comparisons to `TRUE` or `FALSE` like `is_treatment == TRUE` (#1500, @MichaelChirico) + # lintr 3.0.1 * Skip multi-byte tests in non UTF-8 locales (#1504) diff --git a/R/redundant_equals_linter.R b/R/redundant_equals_linter.R new file mode 100644 index 000000000..8c4b6105b --- /dev/null +++ b/R/redundant_equals_linter.R @@ -0,0 +1,38 @@ +#' Block usage of `==`, `!=` on logical vectors +#' +#' Testing `x == TRUE` is redundant if `x` is a logical vector. Wherever this is +#' used to improve readability, the solution should instead be to improve the +#' naming of the object to better indicate that its contents are logical. This +#' can be done using prefixes (is, has, can, etc.). For example, `is_child`, +#' `has_parent_supervision`, `can_watch_horror_movie` clarify their logical +#' nature, while `child`, `parent_supervision`, `watch_horror_movie` don't. +#' @export +redundant_equals_linter <- function() { + xpath <- paste0( + c("//EQ", "//NE"), + "/parent::expr/expr[NUM_CONST[text() = 'TRUE' or text() = 'FALSE']]/parent::expr", + collapse = " | " + ) + + Linter(function(source_expression) { + if (!is_lint_level(source_expression, "expression")) { + return(list()) + } + + xml <- source_expression$xml_parsed_content + + bad_expr <- xml2::xml_find_all(xml, xpath) + op <- xml2::xml_text(xml2::xml_find_first(bad_expr, "*[2]")) + + xml_nodes_to_lints( + bad_expr, + source_expression = source_expression, + lint_message = paste( + "Using", op, "on a logical vector is redundant.", + "Well-named logical vectors can be used directly in filtering.", + "For data.table's `i` argument, wrap the column name in (), like `DT[(is_treatment)]`." + ), + type = "warning" + ) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 1e4da3698..62549994a 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -53,6 +53,7 @@ paren_brace_linter,style readability deprecated paste_linter,best_practices consistency pipe_call_linter,style readability pipe_continuation_linter,style readability default +redundant_equals_linter,best_practices readability efficiency common_mistakes redundant_ifelse_linter,best_practices efficiency consistency regex_subset_linter,best_practices efficiency semicolon_linter,style readability default configurable diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index 567d46f6c..8bcaf1797 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -38,6 +38,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{nonportable_path_linter}}} \item{\code{\link{outer_negation_linter}}} \item{\code{\link{paste_linter}}} +\item{\code{\link{redundant_equals_linter}}} \item{\code{\link{redundant_ifelse_linter}}} \item{\code{\link{regex_subset_linter}}} \item{\code{\link{seq_linter}}} diff --git a/man/common_mistakes_linters.Rd b/man/common_mistakes_linters.Rd index c0cba5bd2..3163d311c 100644 --- a/man/common_mistakes_linters.Rd +++ b/man/common_mistakes_linters.Rd @@ -16,6 +16,7 @@ The following linters are tagged with 'common_mistakes': \item{\code{\link{equals_na_linter}}} \item{\code{\link{missing_argument_linter}}} \item{\code{\link{missing_package_linter}}} +\item{\code{\link{redundant_equals_linter}}} \item{\code{\link{sprintf_linter}}} \item{\code{\link{unused_import_linter}}} } diff --git a/man/efficiency_linters.Rd b/man/efficiency_linters.Rd index ef507fecf..f1219b709 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{literal_coercion_linter}}} \item{\code{\link{nested_ifelse_linter}}} \item{\code{\link{outer_negation_linter}}} +\item{\code{\link{redundant_equals_linter}}} \item{\code{\link{redundant_ifelse_linter}}} \item{\code{\link{regex_subset_linter}}} \item{\code{\link{seq_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index 6ceee755b..f32bc4ac6 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,17 +17,17 @@ see also \code{\link[=available_tags]{available_tags()}}. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (37 linters)} -\item{\link[=common_mistakes_linters]{common_mistakes} (6 linters)} +\item{\link[=best_practices_linters]{best_practices} (38 linters)} +\item{\link[=common_mistakes_linters]{common_mistakes} (7 linters)} \item{\link[=configurable_linters]{configurable} (20 linters)} \item{\link[=consistency_linters]{consistency} (17 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (24 linters)} \item{\link[=deprecated_linters]{deprecated} (4 linters)} -\item{\link[=efficiency_linters]{efficiency} (16 linters)} +\item{\link[=efficiency_linters]{efficiency} (17 linters)} \item{\link[=executing_linters]{executing} (5 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} -\item{\link[=readability_linters]{readability} (37 linters)} +\item{\link[=readability_linters]{readability} (38 linters)} \item{\link[=robustness_linters]{robustness} (12 linters)} \item{\link[=style_linters]{style} (36 linters)} } @@ -89,6 +89,7 @@ The following linters exist: \item{\code{\link{paste_linter}} (tags: best_practices, consistency)} \item{\code{\link{pipe_call_linter}} (tags: readability, style)} \item{\code{\link{pipe_continuation_linter}} (tags: default, readability, style)} +\item{\code{\link{redundant_equals_linter}} (tags: best_practices, common_mistakes, efficiency, readability)} \item{\code{\link{redundant_ifelse_linter}} (tags: best_practices, consistency, efficiency)} \item{\code{\link{regex_subset_linter}} (tags: best_practices, efficiency)} \item{\code{\link{semicolon_linter}} (tags: configurable, default, readability, style)} diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index b14012864..b3d6951dd 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -38,6 +38,7 @@ The following linters are tagged with 'readability': \item{\code{\link{paren_brace_linter}}} \item{\code{\link{pipe_call_linter}}} \item{\code{\link{pipe_continuation_linter}}} +\item{\code{\link{redundant_equals_linter}}} \item{\code{\link{semicolon_linter}}} \item{\code{\link{semicolon_terminator_linter}}} \item{\code{\link{single_quotes_linter}}} diff --git a/man/redundant_equals_linter.Rd b/man/redundant_equals_linter.Rd new file mode 100644 index 000000000..235fe9c10 --- /dev/null +++ b/man/redundant_equals_linter.Rd @@ -0,0 +1,16 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/redundant_equals_linter.R +\name{redundant_equals_linter} +\alias{redundant_equals_linter} +\title{Block usage of \code{==}, \code{!=} on logical vectors} +\usage{ +redundant_equals_linter() +} +\description{ +Testing \code{x == TRUE} is redundant if \code{x} is a logical vector. Wherever this is +used to improve readability, the solution should instead be to improve the +naming of the object to better indicate that its contents are logical. This +can be done using prefixes (is, has, can, etc.). For example, \code{is_child}, +\code{has_parent_supervision}, \code{can_watch_horror_movie} clarify their logical +nature, while \code{child}, \code{parent_supervision}, \code{watch_horror_movie} don't. +} diff --git a/tests/testthat/test-redundant_equals_linter.R b/tests/testthat/test-redundant_equals_linter.R new file mode 100644 index 000000000..1eec725d4 --- /dev/null +++ b/tests/testthat/test-redundant_equals_linter.R @@ -0,0 +1,39 @@ +test_that("redundant_equals_linter skips allowed usages", { + # comparisons to non-logical constants + expect_lint("x == 1", NULL, redundant_equals_linter()) + # comparison to TRUE as a string + expect_lint("x != 'TRUE'", NULL, redundant_equals_linter()) +}) + +test_that("mutliple lints return correct custom messages", { + expect_lint( + "list(x == TRUE, y != TRUE)", + list( + "Using == on a logical vector", + "Using != on a logical vector" + ), + redundant_equals_linter() + ) +}) + +test_that("Order doesn't matter", { + expect_lint("TRUE == x", rex::rex("Using == on a logical vector is redundant."), redundant_equals_linter()) +}) + +skip_if_not_installed("patrick") + +patrick::with_parameters_test_that( + "redundant_equals_linter blocks simple disallowed usages", + { + msg <- rex::rex(paste("Using", op, "on a logical vector is redundant.")) + code <- paste("x", op, bool) + expect_lint(code, msg, redundant_equals_linter()) + }, + .cases = tibble::tribble( + ~.test_name, ~op, ~bool, + "==, TRUE", "==", "TRUE", + "==, FALSE", "==", "FALSE", + "!=, TRUE", "!=", "TRUE", + "!=, FALSE", "!=", "FALSE" + ) +)