Extend conjunct_expectation_linter to include stopifnot(), rename to …

…conjunct_test_linter (#1011)

* New stopifnot_conjunct_linter

* rename

* fix inst/linters entry

* merge with conjunct_expectation_linter --> conjunct_test_linter

* tidy TODO

* fix db entry

DESCRIPTION

@@ -60,7 +60,7 @@ Collate:
     'commas_linter.R'
     'comment_linters.R'
     'comments.R'
-    'conjunct_expectation_linter.R'
+    'conjunct_test_linter.R'
     'consecutive_stopifnot_linter.R'
     'cyclocomp_linter.R'
     'declared_functions.R'

NAMESPACE

@@ -24,7 +24,7 @@ export(clear_cache)
 export(closed_curly_linter)
 export(commas_linter)
 export(commented_code_linter)
-export(conjunct_expectation_linter)
+export(conjunct_test_linter)
 export(consecutive_stopifnot_linter)
 export(cyclocomp_linter)
 export(default_linters)

NEWS.md

@@ -99,7 +99,7 @@ function calls. (#850, #851, @renkun-ken)
    + `expect_type_linter()` Require usage of `expect_type(x, t)` over `expect_equal(typeof(x), t)` and similar
    + `expect_s3_class_linter()` Require usage of `expect_s3_class(x, k)` over `expect_equal(class(x), k)` and similar
    + `expect_s4_class_linter()` Require usage of `expect_s4_class(x, k)` over `expect_true(methods::is(x, k))`
-   + `conjunct_expectation_linter()` Require usage of `expect_true(x); expect_true(y)` over `expect_true(x && y)` and similar
+   + `conjunct_test_linter()` Require usage of `expect_true(x); expect_true(y)` over `expect_true(x && y)` and similar
    + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_.
    + `expect_true_false_linter()` Require usage of `expect_true(x)` over `expect_equal(x, TRUE)` and similar
    + `expect_named_linter()` Require usage of `expect_named(x, n)` over `expect_equal(names(x), n)` and similar

R/conjunct_expectation_linter.R → R/conjunct_test_linter.R

@@ -5,10 +5,10 @@
 #'   `expect_true(A); expect_true(B)` is better than `expect_true(A && B)`, and
 #'   `expect_false(A); expect_false(B)` is better than `expect_false(A || B)`.
 #'
-#' @evalRd rd_tags("expect_true_false_and_condition_linter")
+#' @evalRd rd_tags("conjunct_test_linter")
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
-conjunct_expectation_linter <- function() {
+conjunct_test_linter <- function() {
   Linter(function(source_file) {
     # need the full file to also catch usages at the top level
     if (length(source_file$full_parsed_content) == 0L) {
@@ -17,9 +17,11 @@ conjunct_expectation_linter <- function() {
 
     xml <- source_file$full_xml_parsed_content
 
+    # TODO(#1016): include assert_that() for consideration here too
+    # TODO(#1017): address keyword arguments in R>=4.0 for stopifnot(). lint optionally (on by default)?
     xpath <- "//expr[
       (
-        expr[SYMBOL_FUNCTION_CALL[text() = 'expect_true']]
+        expr[SYMBOL_FUNCTION_CALL[text() = 'expect_true' or text() = 'stopifnot']]
         and expr[2][AND2]
       ) or (
         expr[SYMBOL_FUNCTION_CALL[text() = 'expect_false']]
@@ -35,10 +37,14 @@ conjunct_expectation_linter <- function() {
       source_file,
       lint_message = function(expr) {
         matched_fun <- xml2::xml_text(xml2::xml_find_first(expr, "expr/SYMBOL_FUNCTION_CALL"))
-        op <- if (matched_fun == "expect_true") "&&" else "||"
-        message <-
-          sprintf("Instead of %1$s(A %2$s B), write multiple expectations like %1$s(A) and %1$s(B)", matched_fun, op)
-        paste(message, "The latter will produce better error messages in the case of failure.")
+        op <- xml2::xml_text(xml2::xml_find_first(expr, "expr/*[self::AND2 or self::OR2]"))
+        instead_of <- sprintf("Instead of %s(A %s B),", matched_fun, op)
+        if (matched_fun %in% c("expect_true", "expect_false")) {
+          replacement <- sprintf("write multiple expectations like %1$s(A) and %1$s(B)", matched_fun)
+        } else {
+          replacement <- "write multiple conditions like stopifnot(A, B)."
+        }
+        paste(instead_of, replacement, "The latter will produce better error messages in the case of failure.")
       },
       type = "warning",
       global = TRUE

inst/lintr/linters.csv

@@ -8,7 +8,7 @@ class_equals_linter,best_practices robustness consistency
 closed_curly_linter,style readability default configurable
 commas_linter,style readability default
 commented_code_linter,style readability best_practices default
-conjunct_expectation_linter,package_development best_practices readability
+conjunct_test_linter,package_development best_practices readability
 consecutive_stopifnot_linter,style readability consistency
 cyclocomp_linter,style readability best_practices default configurable
 duplicate_argument_linter,correctness common_mistakes configurable

tests/testthat/test-conjunct_test_linter.R

@@ -0,0 +1,84 @@
+test_that("conjunct_test_linter skips allowed usages of expect_true", {
+  expect_lint("expect_true(x)", NULL, conjunct_test_linter())
+  expect_lint("testthat::expect_true(x, y, z)", NULL, conjunct_test_linter())
+
+  # more complicated expression
+  expect_lint("expect_true(x || (y && z))", NULL, conjunct_test_linter())
+  # the same by operator precedence, though not obvious a priori
+  expect_lint("expect_true(x || y && z)", NULL, conjunct_test_linter())
+  expect_lint("expect_true(x && y || z)", NULL, conjunct_test_linter())
+})
+
+test_that("conjunct_test_linter skips allowed usages of expect_true", {
+  expect_lint("expect_false(x)", NULL, conjunct_test_linter())
+  expect_lint("testthat::expect_false(x, y, z)", NULL, conjunct_test_linter())
+
+  # more complicated expression
+  # (NB: xx && yy || zz and xx || yy && zz both parse with || first)
+  expect_lint("expect_false(x && (y || z))", NULL, conjunct_test_linter())
+})
+
+test_that("conjunct_test_linter blocks && conditions with expect_true()", {
+  expect_lint(
+    "expect_true(x && y)",
+    rex::rex("Instead of expect_true(A && B), write multiple expectations"),
+    conjunct_test_linter()
+  )
+
+  expect_lint(
+    "expect_true(x && y && z)",
+    rex::rex("Instead of expect_true(A && B), write multiple expectations"),
+    conjunct_test_linter()
+  )
+})
+
+test_that("conjunct_test_linter blocks || conditions with expect_false()", {
+  expect_lint(
+    "expect_false(x || y)",
+    rex::rex("Instead of expect_false(A || B), write multiple expectations"),
+    conjunct_test_linter()
+  )
+
+  expect_lint(
+    "expect_false(x || y || z)",
+    rex::rex("Instead of expect_false(A || B), write multiple expectations"),
+    conjunct_test_linter()
+  )
+
+  # these lint because `||` is always outer by operator precedence
+  expect_lint(
+    "expect_false(x || y && z)",
+    rex::rex("Instead of expect_false(A || B), write multiple expectations"),
+    conjunct_test_linter()
+  )
+  expect_lint(
+    "expect_false(x && y || z)",
+    rex::rex("Instead of expect_false(A || B), write multiple expectations"),
+    conjunct_test_linter()
+  )
+})
+
+test_that("conjunct_test_linter skips allowed usages", {
+  expect_lint("stopifnot(x)", NULL, conjunct_test_linter())
+  expect_lint("stopifnot(x, y, z)", NULL, conjunct_test_linter())
+
+  # more complicated expression
+  expect_lint("stopifnot(x || (y && z))", NULL, conjunct_test_linter())
+  # the same by operator precedence, though not obvious a priori
+  expect_lint("stopifnot(x || y && z)", NULL, conjunct_test_linter())
+  expect_lint("stopifnot(x && y || z)", NULL, conjunct_test_linter())
+})
+
+test_that("conjunct_stopifnot_linter blocks simple disallowed usages", {
+  expect_lint(
+    "stopifnot(x && y)",
+    rex::rex("Instead of stopifnot(A && B), write multiple conditions"),
+    conjunct_test_linter()
+  )
+
+  expect_lint(
+    "stopifnot(x && y && z)",
+    rex::rex("Instead of stopifnot(A && B), write multiple conditions"),
+    conjunct_test_linter()
+  )
+})