detect missing() usage in function_argument_linter (#2095)

* detect missing() usage in function_argument_linter

* test for inline case

* Update NEWS.md

* die NEWS item! DIE! 🧟✝

* wrong castle

NAMESPACE

@@ -166,6 +166,7 @@ importFrom(xml2,xml_attr)
 importFrom(xml2,xml_find_all)
 importFrom(xml2,xml_find_chr)
 importFrom(xml2,xml_find_first)
+importFrom(xml2,xml_find_lgl)
 importFrom(xml2,xml_find_num)
 importFrom(xml2,xml_name)
 importFrom(xml2,xml_text)

NEWS.md

@@ -32,6 +32,7 @@
 * `paste_linter()` gains detection for file paths that are better constructed with `file.path()`, e.g. `paste0(dir, "/", file)` would be better as `file.path(dir, file)` (part of #884, @MichaelChirico).
 * `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.
 
 ### New linters
 

R/function_argument_linter.R

@@ -55,6 +55,10 @@ function_argument_linter <- function() {
     ]
   "
 
+  used_in_missing_xpath <- "
+    text() = following-sibling::expr[last()]//expr[expr/SYMBOL_FUNCTION_CALL[text() = 'missing']]/expr[2]/SYMBOL/text()
+  "
+
   Linter(function(source_expression) {
     if (!is_lint_level(source_expression, "expression")) {
       return(list())
@@ -64,10 +68,15 @@ function_argument_linter <- function() {
 
     bad_expr <- xml_find_all(xml, xpath)
 
+    uses_missing <- xml_find_lgl(bad_expr, used_in_missing_xpath)
+
+    missing_note <-
+      ifelse(uses_missing, " Consider setting the default to NULL and using is.null() instead of using missing()", "")
+
     xml_nodes_to_lints(
       bad_expr,
       source_expression = source_expression,
-      lint_message = "Arguments without defaults should come before arguments with defaults.",
+      lint_message = paste0("Arguments without defaults should come before arguments with defaults.", missing_note),
       type = "style"
     )
   })

R/lintr-package.R

@@ -13,7 +13,8 @@
 #' @importFrom rex rex regex re_matches re_substitutes character_class
 #' @importFrom stats na.omit
 #' @importFrom utils capture.output getParseData globalVariables head relist tail
-#' @importFrom xml2 xml_attr xml_find_all xml_find_chr xml_find_num xml_find_first xml_name xml_text as_list
+#' @importFrom xml2 as_list
+#'   xml_attr xml_find_all xml_find_chr xml_find_lgl xml_find_num xml_find_first xml_name xml_text
 #' @rawNamespace
 #' if (getRversion() >= "4.0.0") {
 #'   importFrom(tools, R_user_dir)

tests/testthat/test-function_argument_linter.R

@@ -63,3 +63,94 @@ test_that("function_argument_linter also lints lambda expressions", {
   expect_lint("\\(x, y = 1, z = 2) {}", NULL, linter)
   expect_lint("\\(x, y, z = 1) {}", NULL, linter)
 })
+
+test_that("Use of missing() is reported in the lint message", {
+  linter <- function_argument_linter()
+  simple_msg <- "Arguments without defaults should come before arguments with defaults."
+
+  expect_lint(
+    trim_some("
+      function(x, y = 1, z) {
+        if (missing(z)) {
+          z <- 2
+        }
+      }
+    "),
+    rex::rex(simple_msg, anything, "missing()"),
+    linter
+  )
+
+  expect_lint(
+    trim_some("
+      function(x, y = 1, z) {
+        if (y > 1) {
+          if (missing(z)) {
+            z <- 2
+          }
+        }
+      }
+    "),
+    rex::rex(simple_msg, anything, "missing()"),
+    linter
+  )
+
+  # inline function
+  expect_lint(
+    "function(x = 2, y) if (missing(y)) x else y",
+    rex::rex(simple_msg, anything, "missing()"),
+    linter
+  )
+
+  # missing() used for a different argument
+  expect_lint(
+    trim_some("
+      function(x, y = 1, z) {
+        if (missing(x)) {
+          z <- 2
+        }
+      }
+    "),
+    rex::rex(simple_msg, end),
+    linter
+  )
+
+  # missing() used in the signature (not quite the same, and easier to spot)
+  expect_lint(
+    trim_some("
+      function(x = 1, y, z = missing(y)) {
+        x
+      }
+    "),
+    rex::rex(simple_msg, end),
+    linter
+  )
+})
+
+test_that("multiple lints give correct message", {
+  simple_msg <- "Arguments without defaults should come before arguments with defaults."
+
+  expect_lint(
+    trim_some("{
+      function(x, y = 1, z) {
+        x
+      }
+      function(x, y = 1, z) {
+        if (missing(z)) {
+          z <- 2
+        }
+      }
+      function(x, y = 1, z, w) {
+        if (missing(z)) {
+          z <- 2
+        }
+      }
+    }"),
+    list(
+      list(rex::rex(simple_msg, end), line_number = 2L),
+      list(rex::rex(simple_msg, anything, "missing()"), line_number = 5L),
+      list(rex::rex(simple_msg, anything, "missing()"), line_number = 10L, column_number = 22L),
+      list(rex::rex(simple_msg, end), line_number = 10L, column_number = 25L)
+    ),
+    function_argument_linter()
+  )
+})