From ea47fa8324abf3f8db87ff07feee5552e759ac71 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 25 Sep 2022 03:43:31 +0000 Subject: [PATCH 1/8] New redundant_equals_linter --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 4 +++ R/redundant_equals_linter.R | 36 +++++++++++++++++++ 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 | 9 ++--- man/readability_linters.Rd | 1 + man/redundant_equals_linter.Rd | 13 +++++++ tests/testthat/test-redundant_equals_linter.R | 30 ++++++++++++++++ 12 files changed, 95 insertions(+), 4 deletions(-) create mode 100644 R/redundant_equals_linter.R create mode 100644 man/redundant_equals_linter.Rd create mode 100644 tests/testthat/test-redundant_equals_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 989d3546f..8e83d7a82 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -127,6 +127,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 aedb8948e..6cc64022e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -16,6 +16,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..5859c5522 --- /dev/null +++ b/R/redundant_equals_linter.R @@ -0,0 +1,36 @@ +#' 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. +#' +#' @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..1f1ab02cd --- /dev/null +++ b/man/redundant_equals_linter.Rd @@ -0,0 +1,13 @@ +% 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 ==, != 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. +} diff --git a/tests/testthat/test-redundant_equals_linter.R b/tests/testthat/test-redundant_equals_linter.R new file mode 100644 index 000000000..06a6badd5 --- /dev/null +++ b/tests/testthat/test-redundant_equals_linter.R @@ -0,0 +1,30 @@ +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("redundant_equals_linter blocks simple disallowed usages", { + linter <- redundant_equals_linter() + eq_msg <- rex::rex("Using == on a logical vector is redundant.") + ne_msg <- rex::rex("Using != on a logical vector is redundant.") + expect_lint("x == TRUE", eq_msg, linter) + expect_lint("x != TRUE", ne_msg, linter) + expect_lint("x == FALSE", eq_msg, linter) + expect_lint("x != FALSE", ne_msg, linter) + + # order doesn't matter + expect_lint("TRUE == x", eq_msg, 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() + ) +}) From 371efb8f1f5e51918a4c512d4fd6aca31085dec7 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 24 Sep 2022 22:58:41 -0700 Subject: [PATCH 2/8] Update R/redundant_equals_linter.R Co-authored-by: Indrajeet Patil --- R/redundant_equals_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/redundant_equals_linter.R b/R/redundant_equals_linter.R index 5859c5522..1322b5c04 100644 --- a/R/redundant_equals_linter.R +++ b/R/redundant_equals_linter.R @@ -28,7 +28,7 @@ redundant_equals_linter <- function() { 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)]." + "For data.table's `i` argument, wrap the column name in (), like `DT[(is_treatment)]`." ), type = "warning" ) From 65435c533b76fd274bf6450fc2cb59b964f49d37 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 24 Sep 2022 22:59:40 -0700 Subject: [PATCH 3/8] Update R/redundant_equals_linter.R Co-authored-by: Indrajeet Patil --- R/redundant_equals_linter.R | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/R/redundant_equals_linter.R b/R/redundant_equals_linter.R index 1322b5c04..4a7aa5182 100644 --- a/R/redundant_equals_linter.R +++ b/R/redundant_equals_linter.R @@ -3,7 +3,9 @@ #' 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( From f4f75314f99fc817ef6ebe8e53d607fa63d9e40b Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 25 Sep 2022 10:43:36 -0700 Subject: [PATCH 4/8] Update R/redundant_equals_linter.R Co-authored-by: Indrajeet Patil --- R/redundant_equals_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/redundant_equals_linter.R b/R/redundant_equals_linter.R index 4a7aa5182..44f650221 100644 --- a/R/redundant_equals_linter.R +++ b/R/redundant_equals_linter.R @@ -1,4 +1,4 @@ -#' Block usage of ==, != on logical vectors +#' 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 From 9a69accae86546c114da0692a9f8144454203a9b Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 25 Sep 2022 10:55:35 -0700 Subject: [PATCH 5/8] use patrick to test 4 combinations of cases --- tests/testthat/test-redundant_equals_linter.R | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/tests/testthat/test-redundant_equals_linter.R b/tests/testthat/test-redundant_equals_linter.R index 06a6badd5..2e05118c8 100644 --- a/tests/testthat/test-redundant_equals_linter.R +++ b/tests/testthat/test-redundant_equals_linter.R @@ -5,19 +5,6 @@ test_that("redundant_equals_linter skips allowed usages", { expect_lint("x != 'TRUE'", NULL, redundant_equals_linter()) }) -test_that("redundant_equals_linter blocks simple disallowed usages", { - linter <- redundant_equals_linter() - eq_msg <- rex::rex("Using == on a logical vector is redundant.") - ne_msg <- rex::rex("Using != on a logical vector is redundant.") - expect_lint("x == TRUE", eq_msg, linter) - expect_lint("x != TRUE", ne_msg, linter) - expect_lint("x == FALSE", eq_msg, linter) - expect_lint("x != FALSE", ne_msg, linter) - - # order doesn't matter - expect_lint("TRUE == x", eq_msg, linter) -}) - test_that("mutliple lints return correct custom messages", { expect_lint( "list(x == TRUE, y != TRUE)", @@ -28,3 +15,25 @@ test_that("mutliple lints return correct custom messages", { 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") + +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" + ) +) From 54676690cec0f8eb9a84ec4491af7a28f798351d Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 28 Sep 2022 00:28:02 +0000 Subject: [PATCH 6/8] namespace-qualify; patrick not loaded --- tests/testthat/test-redundant_equals_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-redundant_equals_linter.R b/tests/testthat/test-redundant_equals_linter.R index 2e05118c8..1eec725d4 100644 --- a/tests/testthat/test-redundant_equals_linter.R +++ b/tests/testthat/test-redundant_equals_linter.R @@ -22,7 +22,7 @@ test_that("Order doesn't matter", { skip_if_not_installed("patrick") -with_parameters_test_that( +patrick::with_parameters_test_that( "redundant_equals_linter blocks simple disallowed usages", { msg <- rex::rex(paste("Using", op, "on a logical vector is redundant.")) From ff3e8d2896c4383be67a0c2fa7729862199bb439 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Wed, 28 Sep 2022 07:25:02 +0200 Subject: [PATCH 7/8] style and re-document --- R/redundant_equals_linter.R | 7 ++++--- man/redundant_equals_linter.Rd | 6 +++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/R/redundant_equals_linter.R b/R/redundant_equals_linter.R index 44f650221..a5b40d082 100644 --- a/R/redundant_equals_linter.R +++ b/R/redundant_equals_linter.R @@ -3,9 +3,10 @@ #' 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. +#' 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( diff --git a/man/redundant_equals_linter.Rd b/man/redundant_equals_linter.Rd index 1f1ab02cd..5b9da9ccd 100644 --- a/man/redundant_equals_linter.Rd +++ b/man/redundant_equals_linter.Rd @@ -2,7 +2,7 @@ % Please edit documentation in R/redundant_equals_linter.R \name{redundant_equals_linter} \alias{redundant_equals_linter} -\title{Block usage of ==, != on logical vectors} +\title{Block usage of \code{==}, \code{!=} on logical vectors} \usage{ redundant_equals_linter() } @@ -10,4 +10,8 @@ redundant_equals_linter() 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. } From 5030cd555a111756530360f2c51d79ea192dbc4a Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Wed, 28 Sep 2022 07:27:42 +0200 Subject: [PATCH 8/8] reformat one more time --- R/redundant_equals_linter.R | 13 ++++++------- man/redundant_equals_linter.Rd | 13 ++++++------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/R/redundant_equals_linter.R b/R/redundant_equals_linter.R index a5b40d082..8c4b6105b 100644 --- a/R/redundant_equals_linter.R +++ b/R/redundant_equals_linter.R @@ -1,12 +1,11 @@ #' 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. +#' 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( diff --git a/man/redundant_equals_linter.Rd b/man/redundant_equals_linter.Rd index 5b9da9ccd..235fe9c10 100644 --- a/man/redundant_equals_linter.Rd +++ b/man/redundant_equals_linter.Rd @@ -7,11 +7,10 @@ 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. +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. }