diff --git a/DESCRIPTION b/DESCRIPTION index 4e44760c6..3833a2482 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -66,6 +66,7 @@ Collate: 'expect_lint.R' 'expect_not_linter.R' 'expect_null_linter.R' + 'expect_s3_class_linter.R' 'expect_type_linter.R' 'extract.R' 'extraction_operator_linter.R' diff --git a/NAMESPACE b/NAMESPACE index d563fed50..3f1fbd1db 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -33,6 +33,8 @@ export(expect_lint) export(expect_lint_free) export(expect_not_linter) export(expect_null_linter) +export(expect_s3_class_linter) +export(expect_s4_class_linter) export(expect_type_linter) export(extraction_operator_linter) export(function_left_parentheses_linter) diff --git a/NEWS.md b/NEWS.md index 64ae49a9f..ad2291b76 100644 --- a/NEWS.md +++ b/NEWS.md @@ -89,6 +89,8 @@ 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_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))` + `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) diff --git a/R/expect_s3_class_linter.R b/R/expect_s3_class_linter.R new file mode 100644 index 000000000..7a7e893d0 --- /dev/null +++ b/R/expect_s3_class_linter.R @@ -0,0 +1,108 @@ +#' Require usage of expect_s3_class() +#' +#' [testthat::expect_s3_class()] exists specifically for testing the class +#' of S3 objects. [testthat::expect_equal()], [testthat::expect_identical()], +#' and [testthat::expect_true()] can also be used for such tests, +#' but it is better to use the tailored function instead. +#' +#' @evalRd rd_tags("expect_s3_class_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +expect_s3_class_linter <- function() { + Linter(function(source_file) { + if (length(source_file$parsed_content) == 0L) { + return(list()) + } + + xml <- source_file$xml_parsed_content + + # (1) expect_{equal,identical}(class(x), C) + # (2) expect_true(is.(x)) and expect_true(inherits(x, C)) + is_class_call <- xp_text_in_table(c(is_s3_class_calls, "inherits")) # nolint: object_usage_linter. TODO(#942): fix this. + xpath <- glue::glue("//expr[ + ( + SYMBOL_FUNCTION_CALL[text() = 'expect_equal' or text() = 'expect_identical'] + and following-sibling::expr[ + expr[SYMBOL_FUNCTION_CALL[text() = 'class']] + and (position() = 1 or preceding-sibling::expr[STR_CONST]) + ] + ) or ( + SYMBOL_FUNCTION_CALL[text() = 'expect_true'] + and following-sibling::expr[1][expr[SYMBOL_FUNCTION_CALL[ {is_class_call} ]]] + ) + ]") + + bad_expr <- xml2::xml_find_all(xml, xpath) + return(lapply(bad_expr, gen_expect_s3_class_lint, source_file)) + }) +} + +# NB: there is no easy way to make an exhaustive list of places where an +# is. call can be replaced by expect_s3_class(); this list was manually +# populated from the default R packages by inspection. For example, +# is.matrix(x) cannot be replaced by expect_s3_class(x, "matrix") because +# it is not actually an S3 class (is.object(x) is not TRUE since there +# is no class set for a matrix [I am not sure if this changes in R 4]. +# Further, there are functions named is. that have nothing to do with +# object type, e.g. is.finite(), is.nan(), or is.R(). +is_s3_class_calls <- paste0("is.", c( + # base + "data.frame", "factor", "numeric_version", + "ordered", "package_version", "qr", "table", + # utils grDevices tcltk tcltk grid grid + "relistable", "raster", "tclObj", "tkwin", "grob", "unit", + # stats + "mts", "stepfun", "ts", "tskernel" +)) + +gen_expect_s3_class_lint <- function(expr, source_file) { + matched_function <- xml2::xml_text(xml2::xml_find_first(expr, "SYMBOL_FUNCTION_CALL")) + if (matched_function %in% c("expect_equal", "expect_identical")) { + lint_msg <- sprintf("expect_s3_class(x, k) is better than %s(class(x), k).", matched_function) + } else { + lint_msg <- "expect_s3_class(x, k) is better than expect_true(is.(x)) or expect_true(inherits(x, k))." + } + lint_msg <- paste(lint_msg, "Note also expect_s4_class() available for testing S4 objects.") + xml_nodes_to_lint(expr, source_file, lint_msg, type = "warning") +} + +#' Require usage of expect_s4_class(x, k) over expect_true(is(x, k)) +#' +#' [testthat::expect_s4_class()] exists specifically for testing the class +#' of S4 objects. [testthat::expect_true()] can also be used for such tests, +#' but it is better to use the tailored function instead. +#' +#' @evalRd rd_tags("expect_s3_class_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +expect_s4_class_linter <- function() { + Linter(function(source_file) { + if (length(source_file$parsed_content) == 0L) { + return(list()) + } + + xml <- source_file$xml_parsed_content + + # TODO(michaelchirico): also catch expect_{equal,identical}(methods::is(x), k). + # there are no hits for this on google3 as of now. + + # require 2 expressions because methods::is(x) alone is a valid call, even + # though the character output wouldn't make any sense for expect_true(). + xpath <- "//expr[ + SYMBOL_FUNCTION_CALL[text() = 'expect_true'] + and following-sibling::expr[1][count(expr) = 3 and expr[SYMBOL_FUNCTION_CALL[text() = 'is']]] + ]" + + bad_expr <- xml2::xml_find_all(xml, xpath) + return(lapply( + bad_expr, + xml_nodes_to_lint, + source_file = source_file, + message = paste( + "expect_s4_class(x, k) is better than expect_true(is(x, k)).", + "Note also expect_s3_class() available for testing S3 objects." + ), + type = "warning" + )) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index ce0dcbf02..6a11db3de 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -11,6 +11,8 @@ 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_s3_class_linter,package_development best_practices +expect_s4_class_linter,package_development best_practices expect_type_linter,package_development best_practices extraction_operator_linter,style best_practices function_left_parentheses_linter,style readability default diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index b84a09504..a94e0a28b 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -17,6 +17,8 @@ The following linters are tagged with 'best_practices': \item{\code{\link{cyclocomp_linter}}} \item{\code{\link{expect_not_linter}}} \item{\code{\link{expect_null_linter}}} +\item{\code{\link{expect_s3_class_linter}}} +\item{\code{\link{expect_s4_class_linter}}} \item{\code{\link{expect_type_linter}}} \item{\code{\link{extraction_operator_linter}}} \item{\code{\link{implicit_integer_linter}}} diff --git a/man/expect_s3_class_linter.Rd b/man/expect_s3_class_linter.Rd new file mode 100644 index 000000000..daf0dcd4f --- /dev/null +++ b/man/expect_s3_class_linter.Rd @@ -0,0 +1,20 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/expect_s3_class_linter.R +\name{expect_s3_class_linter} +\alias{expect_s3_class_linter} +\title{Require usage of expect_s3_class()} +\usage{ +expect_s3_class_linter() +} +\description{ +\code{\link[testthat:inheritance-expectations]{testthat::expect_s3_class()}} exists specifically for testing the class +of S3 objects. \code{\link[testthat:equality-expectations]{testthat::expect_equal()}}, \code{\link[testthat:equality-expectations]{testthat::expect_identical()}}, +and \code{\link[testthat:logical-expectations]{testthat::expect_true()}} can also be used for such tests, +but it is better to use the tailored function instead. +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=package_development_linters]{package_development} +} diff --git a/man/expect_s4_class_linter.Rd b/man/expect_s4_class_linter.Rd new file mode 100644 index 000000000..2a49c5853 --- /dev/null +++ b/man/expect_s4_class_linter.Rd @@ -0,0 +1,19 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/expect_s3_class_linter.R +\name{expect_s4_class_linter} +\alias{expect_s4_class_linter} +\title{Require usage of expect_s4_class(x, k) over expect_true(is(x, k))} +\usage{ +expect_s4_class_linter() +} +\description{ +\code{\link[testthat:inheritance-expectations]{testthat::expect_s4_class()}} exists specifically for testing the class +of S4 objects. \code{\link[testthat:logical-expectations]{testthat::expect_true()}} can also be used for such tests, +but it is better to use the tailored function instead. +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=package_development_linters]{package_development} +} diff --git a/man/linters.Rd b/man/linters.Rd index 6c3353f97..c68cf9ec5 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,6 +17,7 @@ Documentation for linters is structured into tags to allow for easier discovery. \section{Tags}{ The following tags exist: \itemize{ +\item{\link[=best_practices_linters]{best_practices} (15 linters)} \item{\link[=best_practices_linters]{best_practices} (13 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (5 linters)} \item{\link[=configurable_linters]{configurable} (16 linters)} @@ -24,7 +25,7 @@ The following tags exist: \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (25 linters)} \item{\link[=efficiency_linters]{efficiency} (4 linters)} -\item{\link[=package_development_linters]{package_development} (5 linters)} +\item{\link[=package_development_linters]{package_development} (7 linters)} \item{\link[=readability_linters]{readability} (22 linters)} \item{\link[=robustness_linters]{robustness} (10 linters)} \item{\link[=style_linters]{style} (32 linters)} @@ -45,6 +46,8 @@ The following linters exist: \item{\code{\link{equals_na_linter}} (tags: common_mistakes, correctness, default, robustness)} \item{\code{\link{expect_not_linter}} (tags: best_practices, package_development, readability)} \item{\code{\link{expect_null_linter}} (tags: best_practices, package_development)} +\item{\code{\link{expect_s3_class_linter}} (tags: best_practices, package_development)} +\item{\code{\link{expect_s4_class_linter}} (tags: best_practices, package_development)} \item{\code{\link{expect_type_linter}} (tags: best_practices, package_development)} \item{\code{\link{extraction_operator_linter}} (tags: best_practices, style)} \item{\code{\link{function_left_parentheses_linter}} (tags: default, readability, style)} diff --git a/man/package_development_linters.Rd b/man/package_development_linters.Rd index 6fbc2e23b..b92f6a6a5 100644 --- a/man/package_development_linters.Rd +++ b/man/package_development_linters.Rd @@ -15,6 +15,8 @@ The following linters are tagged with 'package_development': \item{\code{\link{backport_linter}}} \item{\code{\link{expect_not_linter}}} \item{\code{\link{expect_null_linter}}} +\item{\code{\link{expect_s3_class_linter}}} +\item{\code{\link{expect_s4_class_linter}}} \item{\code{\link{expect_type_linter}}} \item{\code{\link{package_hooks_linter}}} } diff --git a/tests/testthat/test-expect_s3_class_linter.R b/tests/testthat/test-expect_s3_class_linter.R new file mode 100644 index 000000000..0ec35af7c --- /dev/null +++ b/tests/testthat/test-expect_s3_class_linter.R @@ -0,0 +1,104 @@ +test_that("expect_s3_class_linter skips allowed usages", { + # expect_s3_class doesn't have an inverted version + expect_lint("expect_true(!inherits(x, 'class'))", NULL, expect_s3_class_linter()) + # NB: also applies to tinytest, but it's sufficient to test testthat + expect_lint("testthat::expect_true(!inherits(x, 'class'))", NULL, expect_s3_class_linter()) + + # other is. calls are not suitable for expect_s3_class in particular + expect_lint("expect_true(is.na(x))", NULL, expect_s3_class_linter()) + + # case where expect_s3_class() *could* be used but we don't enforce + expect_lint("expect_true(is.data.table(x))", NULL, expect_s3_class_linter()) +}) + +test_that("expect_s3_class_linter blocks simple disallowed usages", { + expect_lint( + "expect_equal(class(x), 'data.frame')", + rex::rex("expect_s3_class(x, k) is better than expect_equal(class(x), k)"), + expect_s3_class_linter() + ) + + # works when testing against a sequence of classes too + expect_lint( + "expect_equal(class(x), c('data.table', 'data.frame'))", + rex::rex("expect_s3_class(x, k) is better than expect_equal(class(x), k)"), + expect_s3_class_linter() + ) + + # expect_identical is treated the same as expect_equal + expect_lint( + "testthat::expect_identical(class(x), 'lm')", + rex::rex("expect_s3_class(x, k) is better than expect_identical(class(x), k)"), + expect_s3_class_linter() + ) + + # yoda test with string literal in first arg also caught + expect_lint( + "expect_equal('data.frame', class(x))", + rex::rex("expect_s3_class(x, k) is better than expect_equal(class(x), k)"), + expect_s3_class_linter() + ) + + # different equivalent usages + expect_lint( + "expect_true(is.table(foo(x)))", + rex::rex("expect_s3_class(x, k) is better than expect_true(is.(x))"), + expect_s3_class_linter() + ) + expect_lint( + "expect_true(inherits(x, 'table'))", + rex::rex("expect_s3_class(x, k) is better than expect_true(is.(x))"), + expect_s3_class_linter() + ) + + # TODO(michaelchirico): consider more carefully which sorts of class(x) %in% . and + # . %in% class(x) calls should be linted + # expect_lint( + # "expect_true('lm' %in% class(x))", + # "expect_s3_class\\(x, k\\) is better than expect_equal\\(class\\(x\\), k", + # expect_s3_class_linter + # ) +}) + +test_that("expect_s4_class_linter skips allowed usages", { + # expect_s4_class doesn't have an inverted version + expect_lint("expect_true(!is(x, 'class'))", NULL, expect_s4_class_linter()) + # NB: also applies to tinytest, but it's sufficient to test testthat + expect_lint("testthat::expect_s3_class(!is(x, 'class'))", NULL, expect_s4_class_linter()) +}) + +test_that("expect_s4_class blocks simple disallowed usages", { + expect_lint( + "expect_true(is(x, 'data.frame'))", + rex::rex("expect_s4_class(x, k) is better than expect_true(is(x, k))"), + expect_s4_class_linter() + ) + + # namespace qualification is irrelevant + expect_lint( + "testthat::expect_true(methods::is(x, 'SpatialPolygonsDataFrame'))", + rex::rex("expect_s4_class(x, k) is better than expect_true(is(x, k))"), + expect_s4_class_linter() + ) +}) + +skip_if_not_installed("patrick") +local({ + # test for lint errors appropriately raised for all is. calls + is_classes <- c( + "data.frame", "factor", "numeric_version", + "ordered", "package_version", "qr", "table", + "relistable", "raster", "tclObj", "tkwin", "grob", "unit", + "mts", "stepfun", "ts", "tskernel" + ) + patrick::with_parameters_test_that( + "expect_true(is.) is caught", + expect_lint( + sprintf("expect_true(is.%s(x))", is_class), + rex::rex("expect_s3_class(x, k) is better than expect_true(is.(x))"), + expect_s3_class_linter() + ), + .test_name = is_classes, + is_class = is_classes + ) +}) diff --git a/tests/testthat/test-methods.R b/tests/testthat/test-methods.R index fc929bfcf..140dd1372 100644 --- a/tests/testthat/test-methods.R +++ b/tests/testthat/test-methods.R @@ -80,7 +80,7 @@ test_that("summary.lints() works (no lints)", { "x <- 1\n", linters = assignment_linter()) no_lint_summary <- summary(no_lints) - expect_true(is.data.frame(no_lint_summary)) + expect_s3_class(no_lint_summary, "data.frame") expect_equal(nrow(no_lint_summary), 0) }) @@ -89,7 +89,7 @@ test_that("summary.lints() works (lints found)", { "x = 1\n", linters = assignment_linter()) has_lint_summary <- summary(has_lints) - expect_true(is.data.frame(has_lint_summary)) + expect_s3_class(has_lint_summary, "data.frame") expect_equal(nrow(has_lint_summary), 1) expect_true(has_lint_summary$style > 0) expect_equal(has_lint_summary$warning, 0)