New expect_not_linter (#945)

DESCRIPTION

@@ -64,6 +64,7 @@ Collate:
     'equals_na_linter.R'
     'exclude.R'
     'expect_lint.R'
+    'expect_not_linter.R'
     'expect_null_linter.R'
     'expect_type_linter.R'
     'extract.R'

NAMESPACE

@@ -31,6 +31,7 @@ export(duplicate_argument_linter)
 export(equals_na_linter)
 export(expect_lint)
 export(expect_lint_free)
+export(expect_not_linter)
 export(expect_null_linter)
 export(expect_type_linter)
 export(extraction_operator_linter)

NEWS.md

@@ -89,6 +89,7 @@ function calls. (#850, #851, @renkun-ken)
 * `lintr` is adopting a new set of linters provided as part of Google's extension to the tidyverse style guide (#884, @michaelchirico)
    + `expect_null_linter()` Require usage of `expect_null(x)` over `expect_equal(x, NULL)` and similar
    + `expect_type_linter()` Require usage of `expect_type(x, t)` over `expect_equal(typeof(x), t)` and similar
+   + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_.
 * `assignment_linter()` now lints right assignment (`->` and `->>`) and gains two arguments. `allow_cascading_assign` (`TRUE` by default) toggles whether to lint `<<-` and `->>`; `allow_right_assign` toggles whether to lint `->` and `->>` (#915, @michaelchirico)
 
 # lintr 2.0.1

R/expect_not_linter.R

@@ -0,0 +1,35 @@
+#' Require usage of expect_false(.) over expect_true(!.)
+#'
+#' [testthat::expect_false()] exists specifically for testing that an output is
+#'   `FALSE`. [testthat::expect_true()] can also be used for such tests by
+#'   negating the output, but it is better to use the tailored function instead.
+#'   The reverse is also true -- use `expect_false(A)` instead of
+#'   `expect_true(!A)`.
+#'
+#' @evalRd rd_tags("expect_not_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+expect_not_linter <- function() {
+  Linter(function(source_file) {
+    if (length(source_file$parsed_content) == 0L) {
+      return(list())
+    }
+
+    xml <- source_file$xml_parsed_content
+
+    xpath <- "//expr[
+      SYMBOL_FUNCTION_CALL[text() = 'expect_true' or text() = 'expect_false']
+      and following-sibling::expr[1][OP-EXCLAMATION]
+    ]"
+
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+
+    return(lapply(
+      bad_expr,
+      xml_nodes_to_lint,
+      source_file = source_file,
+      message = "expect_false(x) is better than expect_true(!x), and vice versa.",
+      type = "warning"
+    ))
+  })
+}

inst/lintr/linters.csv

@@ -9,6 +9,7 @@ commented_code_linter,style readability best_practices default
 cyclocomp_linter,style readability best_practices default configurable
 duplicate_argument_linter,correctness common_mistakes configurable
 equals_na_linter,robustness correctness common_mistakes default
+expect_not_linter,package_development best_practices readability
 expect_null_linter,package_development best_practices
 expect_type_linter,package_development best_practices
 extraction_operator_linter,style best_practices

tests/testthat/test-expect_not_linter.R

@@ -0,0 +1,37 @@
+test_that("expect_not_linter skips allowed usages", {
+  expect_lint("expect_true(x)", NULL, expect_not_linter())
+  # NB: also applies to tinytest, but it's sufficient to test testthat
+  expect_lint("testthat::expect_true(x)", NULL, expect_not_linter())
+  expect_lint("expect_false(x)", NULL, expect_not_linter())
+  expect_lint("testthat::expect_false(x)", NULL, expect_not_linter())
+
+  # not a strict ban on !
+  ##  (expect_false(x && y) is the same, but it's not clear which to prefer)
+  expect_lint("expect_true(!x || !y)", NULL, expect_not_linter())
+})
+
+test_that("expect_not_linter blocks simple disallowed usages", {
+  expect_lint(
+    "expect_true(!x)",
+    "expect_false\\(x\\) is better than expect_true\\(!x\\)",
+    expect_not_linter()
+  )
+
+  expect_lint(
+    "testthat::expect_true(!x)",
+    "expect_false\\(x\\) is better than expect_true\\(!x\\)",
+    expect_not_linter()
+  )
+
+  expect_lint(
+    "expect_false(!foo(x))",
+    "expect_false\\(x\\) is better than expect_true\\(!x\\)",
+    expect_not_linter()
+  )
+
+  expect_lint(
+    "testthat::expect_true(!(x && y))",
+    "expect_false\\(x\\) is better than expect_true\\(!x\\)",
+    expect_not_linter()
+  )
+})