From acee3bb515edc237a3e2447aa77f95fadc2b6972 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 13 Nov 2023 10:24:51 -0800 Subject: [PATCH] New stopifnot_all_linter (#2273) --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 1 + R/stopifnot_all_linter.R | 20 ++++++++++++++++ inst/lintr/linters.csv | 1 + man/best_practices_linters.Rd | 1 + man/linters.Rd | 5 ++-- man/readability_linters.Rd | 1 + man/stopifnot_all_linter.Rd | 18 +++++++++++++++ tests/testthat/test-stopifnot_all_linter.R | 27 ++++++++++++++++++++++ 10 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 R/stopifnot_all_linter.R create mode 100644 man/stopifnot_all_linter.Rd create mode 100644 tests/testthat/test-stopifnot_all_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 6a456f3a9..8b20c69ed 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -166,6 +166,7 @@ Collate: 'spaces_inside_linter.R' 'spaces_left_parentheses_linter.R' 'sprintf_linter.R' + 'stopifnot_all_linter.R' 'string_boundary_linter.R' 'strings_as_factors_linter.R' 'system_file_linter.R' diff --git a/NAMESPACE b/NAMESPACE index 125f27519..f1ea599a2 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -129,6 +129,7 @@ export(sort_linter) export(spaces_inside_linter) export(spaces_left_parentheses_linter) export(sprintf_linter) +export(stopifnot_all_linter) export(string_boundary_linter) export(strings_as_factors_linter) export(system_file_linter) diff --git a/NEWS.md b/NEWS.md index d530ce605..1680bb9b3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -6,6 +6,7 @@ ### New linters +* `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). ### Lint accuracy fixes: removing false positives diff --git a/R/stopifnot_all_linter.R b/R/stopifnot_all_linter.R new file mode 100644 index 000000000..307427061 --- /dev/null +++ b/R/stopifnot_all_linter.R @@ -0,0 +1,20 @@ +#' Block usage of all() within stopifnot() +#' +#' `stopifnot(A)` actually checks `all(A)` "under the hood" if `A` is a vector, +#' and produces a better error message than `stopifnot(all(A))` does. +#' +#' @evalRd rd_tags("stopifnot_all_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +stopifnot_all_linter <- make_linter_from_xpath( + xpath = " + //SYMBOL_FUNCTION_CALL[text() = 'stopifnot'] + /parent::expr + /parent::expr + /expr[expr/SYMBOL_FUNCTION_CALL[text() = 'all']] + ", + lint_message = paste( + "Calling stopifnot(all(x)) is redundant. stopifnot(x) runs all()", + "'under the hood' and provides a better error message in case of failure." + ) +) diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index fbb6847c5..e5e763c4f 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -85,6 +85,7 @@ sort_linter,readability best_practices efficiency spaces_inside_linter,style readability default spaces_left_parentheses_linter,style readability default sprintf_linter,correctness common_mistakes +stopifnot_all_linter,readability best_practices string_boundary_linter,readability efficiency configurable strings_as_factors_linter,robustness system_file_linter,consistency readability best_practices diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index 6016a5f66..7022070d1 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -54,6 +54,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{scalar_in_linter}}} \item{\code{\link{seq_linter}}} \item{\code{\link{sort_linter}}} +\item{\code{\link{stopifnot_all_linter}}} \item{\code{\link{system_file_linter}}} \item{\code{\link{T_and_F_symbol_linter}}} \item{\code{\link{undesirable_function_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index 2ba7b9d97..21947f999 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,7 +17,7 @@ see also \code{\link[=available_tags]{available_tags()}}. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (53 linters)} +\item{\link[=best_practices_linters]{best_practices} (54 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (8 linters)} \item{\link[=configurable_linters]{configurable} (34 linters)} \item{\link[=consistency_linters]{consistency} (23 linters)} @@ -28,7 +28,7 @@ The following tags exist: \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)} -\item{\link[=readability_linters]{readability} (55 linters)} +\item{\link[=readability_linters]{readability} (56 linters)} \item{\link[=robustness_linters]{robustness} (14 linters)} \item{\link[=style_linters]{style} (38 linters)} } @@ -115,6 +115,7 @@ The following linters exist: \item{\code{\link{spaces_inside_linter}} (tags: default, readability, style)} \item{\code{\link{spaces_left_parentheses_linter}} (tags: default, readability, style)} \item{\code{\link{sprintf_linter}} (tags: common_mistakes, correctness)} +\item{\code{\link{stopifnot_all_linter}} (tags: best_practices, readability)} \item{\code{\link{string_boundary_linter}} (tags: configurable, efficiency, readability)} \item{\code{\link{strings_as_factors_linter}} (tags: robustness)} \item{\code{\link{system_file_linter}} (tags: best_practices, consistency, readability)} diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index a60667062..7bb1a809f 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -58,6 +58,7 @@ The following linters are tagged with 'readability': \item{\code{\link{sort_linter}}} \item{\code{\link{spaces_inside_linter}}} \item{\code{\link{spaces_left_parentheses_linter}}} +\item{\code{\link{stopifnot_all_linter}}} \item{\code{\link{string_boundary_linter}}} \item{\code{\link{system_file_linter}}} \item{\code{\link{T_and_F_symbol_linter}}} diff --git a/man/stopifnot_all_linter.Rd b/man/stopifnot_all_linter.Rd new file mode 100644 index 000000000..17e9d8c49 --- /dev/null +++ b/man/stopifnot_all_linter.Rd @@ -0,0 +1,18 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/stopifnot_all_linter.R +\name{stopifnot_all_linter} +\alias{stopifnot_all_linter} +\title{Block usage of all() within stopifnot()} +\usage{ +stopifnot_all_linter() +} +\description{ +\code{stopifnot(A)} actually checks \code{all(A)} "under the hood" if \code{A} is a vector, +and produces a better error message than \code{stopifnot(all(A))} does. +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=readability_linters]{readability} +} diff --git a/tests/testthat/test-stopifnot_all_linter.R b/tests/testthat/test-stopifnot_all_linter.R new file mode 100644 index 000000000..ded0fb4b3 --- /dev/null +++ b/tests/testthat/test-stopifnot_all_linter.R @@ -0,0 +1,27 @@ +test_that("stopifnot_all_linter skips allowed usages", { + expect_lint("stopifnot(all(x) || any(y))", NULL, stopifnot_all_linter()) +}) + +test_that("stopifnot_all_linter blocks simple disallowed usages", { + linter <- stopifnot_all_linter() + lint_msg <- rex::rex("stopifnot(x) runs all() 'under the hood'") + + expect_lint("stopifnot(all(A))", list(lint_msg, column_number = 11L), linter) + expect_lint("stopifnot(x, y, all(z))", list(lint_msg, column_number = 17L), linter) + + expect_lint( + trim_some("{ + stopifnot(all(x), all(y), + all(z) + ) + stopifnot(a > 0, b < 0, all(c == 0)) + }"), + list( + list(lint_msg, line_number = 2L, column_number = 13L), + list(lint_msg, line_number = 2L, column_number = 21L), + list(lint_msg, line_number = 3L, column_number = 5L), + list(lint_msg, line_number = 5L, column_number = 27L) + ), + linter + ) +})