New stopifnot_all_linter (#2273)

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'

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)

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

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."
+  )
+)

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

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
+  )
+})