diff --git a/NEWS.md b/NEWS.md index be1c847cd..f066bcba9 100644 --- a/NEWS.md +++ b/NEWS.md @@ -34,6 +34,7 @@ * `seq_linter()` recommends `rev()` in the lint message for lints like `nrow(x):1` (#1542, @MichaelChirico). * New `xp_call_name()` helper to facilitate writing custom linters (#2023, @MichaelChirico). This helper converts a matched XPath to the R function to which it corresponds. This is useful for including the "offending" function in the lint's message. * `function_argument_linter()` detects usage of `missing()` for the linted argument (#1546, @MichaelChirico). The simplest fix for `function_argument_linter()` lints is typically to set that argument to `NULL` by default, in which case it's usually preferable to update function logic checking `missing()` to check `is.null()` instead. +* `equals_na_linter()` checks for `x %in% NA`, which is a more convoluted form of `is.na(x)` (#2088, @MichaelChirico). * `commas_linter()` gains an option `allow_trailing` (default `FALSE`) to allow trailing commas while indexing. (#2104, @MEO265) ### New linters diff --git a/R/equals_na_linter.R b/R/equals_na_linter.R index 16af7e1b9..6d352b47b 100644 --- a/R/equals_na_linter.R +++ b/R/equals_na_linter.R @@ -1,6 +1,6 @@ #' Equality check with NA linter #' -#' Check for `x == NA` and `x != NA`. Such usage is almost surely incorrect -- +#' Check for `x == NA`, `x != NA` and `x %in% NA`. Such usage is almost surely incorrect -- #' checks for missing values should be done with [is.na()]. #' #' @examples @@ -15,6 +15,11 @@ #' linters = equals_na_linter() #' ) #' +#' lint( +#' text = "x %in% NA", +#' linters = equals_na_linter() +#' ) +#' #' # okay #' lint( #' text = "is.na(x)", @@ -36,6 +41,9 @@ equals_na_linter <- function() { //NUM_CONST[ {na_table} ] /parent::expr /parent::expr[EQ or NE] + | + //SPECIAL[text() = '%in%' and following-sibling::expr/NUM_CONST[ {na_table} ]] + /parent::expr ") Linter(function(source_expression) { @@ -50,7 +58,7 @@ equals_na_linter <- function() { xml_nodes_to_lints( bad_expr, source_expression, - lint_message = "Use is.na for comparisons to NA (not == or !=)", + lint_message = "Use is.na for comparisons to NA (not == or != or %in%)", type = "warning" ) }) diff --git a/man/equals_na_linter.Rd b/man/equals_na_linter.Rd index 274d0521a..34b21f94f 100644 --- a/man/equals_na_linter.Rd +++ b/man/equals_na_linter.Rd @@ -7,7 +7,7 @@ equals_na_linter() } \description{ -Check for \code{x == NA} and \code{x != NA}. Such usage is almost surely incorrect -- +Check for \code{x == NA}, \code{x != NA} and \code{x \%in\% NA}. Such usage is almost surely incorrect -- checks for missing values should be done with \code{\link[=is.na]{is.na()}}. } \examples{ @@ -22,6 +22,11 @@ lint( linters = equals_na_linter() ) +lint( + text = "x \%in\% NA", + linters = equals_na_linter() +) + # okay lint( text = "is.na(x)", diff --git a/tests/testthat/test-equals_na_linter.R b/tests/testthat/test-equals_na_linter.R index ba4c64365..34f9e6f9c 100644 --- a/tests/testthat/test-equals_na_linter.R +++ b/tests/testthat/test-equals_na_linter.R @@ -17,6 +17,9 @@ test_that("equals_na_linter skips allowed usages", { # nested NAs are okay expect_lint("x==f(1, ignore = NA)", NULL, linter) + + # this should be covered by any_is_na_linter() + expect_lint("NA %in% x", NULL, linter) }) skip_if_not_installed("tibble") @@ -24,27 +27,32 @@ patrick::with_parameters_test_that( "equals_na_linter blocks disallowed usages for all combinations of operators and types of NAs", expect_lint( paste("x", operation, type_na), - rex::rex("Use is.na for comparisons to NA (not == or !=)"), + rex::rex("Use is.na for comparisons to NA (not == or != or %in%)"), equals_na_linter() ), .cases = tibble::tribble( - ~.test_name, ~operation, ~type_na, - "equality, logical NA", "==", "NA", - "equality, integer NA", "==", "NA_integer_", - "equality, real NA", "==", "NA_real_", - "equality, complex NA", "==", "NA_complex_", - "equality, character NA", "==", "NA_character_", - "inequality, logical NA", "!=", "NA", - "inequality, integer NA", "!=", "NA_integer_", - "inequality, real NA", "!=", "NA_real_", - "inequality, complex NA", "!=", "NA_complex_", - "inequality, character NA", "!=", "NA_character_" + ~.test_name, ~operation, ~type_na, + "equality, logical NA", "==", "NA", + "equality, integer NA", "==", "NA_integer_", + "equality, real NA", "==", "NA_real_", + "equality, complex NA", "==", "NA_complex_", + "equality, character NA", "==", "NA_character_", + "containment, logical NA", "%in%", "NA", + "containment, integer NA", "%in%", "NA_integer_", + "containment, real NA", "%in%", "NA_real_", + "containment, complex NA", "%in%", "NA_complex_", + "containment, character NA", "%in%", "NA_character_", + "inequality, logical NA", "!=", "NA", + "inequality, integer NA", "!=", "NA_integer_", + "inequality, real NA", "!=", "NA_real_", + "inequality, complex NA", "!=", "NA_complex_", + "inequality, character NA", "!=", "NA_character_" ) ) test_that("equals_na_linter blocks disallowed usages in edge cases", { linter <- equals_na_linter() - lint_msg <- rex::rex("Use is.na for comparisons to NA (not == or !=)") + lint_msg <- rex::rex("Use is.na for comparisons to NA (not == or != or %in%)") # missing spaces around operators expect_lint("x==NA", list(message = lint_msg, line_number = 1L, column_number = 1L), linter)