Add arg to skip with() in object_usage_linter() (#1548)

* Add arg to skip `with()` in `object_usage_linter()`

Closes #1458

* Skip `with()` expressions by default

Closes #941

* address review comments

* Update NEWS.md

* Use character vector instead

Co-authored-by: Michael Chirico <chiricom@google.com>

NEWS.md

@@ -6,6 +6,9 @@
 
 ## Changes to defaults
 
+* `object_usage_linter()` gains `skip_with` argument to skip code in `with()` expressions.
+  To be consistent with `R CMD check`, it defaults to `TRUE` (#941, #1458, @IndrajeetPatil).
+
 * `unused_import_linter()` can detect datasets from imported packages and no longer
   warns when a package is imported only for its datasets (#1545, @IndrajeetPatil).
 
@@ -62,7 +65,7 @@
 
 ## Bug fixes
 
-* `object_length_linter()` does not fail in case there are dependencies with no exports (e.g. data-only packages) (#1509, @IndrajeetPatil).
+* `object_length_linter()` does not fail in case there are dependencies with no exports (e.g. data-only packages) (#1424, #1509, @IndrajeetPatil).
 * `get_source_expressions()` no longer fails on R files that match a knitr pattern (#743, #879, #1406, @AshesITR).
 * Parse error lints now appear with the linter name `"error"` instead of `NA` (#1405, @AshesITR).  
   Also, linting no longer runs if the `source_expressions` contain invalid string data that would cause error messages

R/object_usage_linter.R

@@ -5,11 +5,14 @@
 #'
 #' @param interpret_glue If `TRUE`, interpret [glue::glue()] calls to avoid false positives caused by local variables
 #' which are only used in a glue expression.
+#' @param skip_with A logical. If `TRUE` (default), code in `with()` expressions
+#'   will be skipped. This argument will be passed to `skipWith` argument of
+#'   `codetools::checkUsage()`.
 #'
-#' @evalRd rd_tags("object_usage_linter")
+#' @evalRd rd_linters("package_development")
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
-object_usage_linter <- function(interpret_glue = TRUE) {
+object_usage_linter <- function(interpret_glue = TRUE, skip_with = TRUE) {
   # NB: difference across R versions in how EQ_ASSIGN is represented in the AST
   #   (under <expr_or_assign_or_help> or <equal_assign>)
   # NB: the repeated expr[2][FUNCTION] XPath has no performance impact, so the different direct assignment XPaths are
@@ -69,7 +72,8 @@ object_usage_linter <- function(interpret_glue = TRUE) {
         fun,
         known_used_symbols = known_used_symbols,
         declared_globals = declared_globals,
-        start_line = as.integer(xml2::xml_attr(fun_assignment, "line1"))
+        start_line = as.integer(xml2::xml_attr(fun_assignment, "line1")),
+        skip_with = skip_with
       )
 
       # TODO handle assignment functions properly
@@ -196,8 +200,11 @@ get_assignment_symbols <- function(xml) {
   ))
 }
 
-parse_check_usage <- function(expression, known_used_symbols = character(), declared_globals = character(),
-                              start_line = 1L) {
+parse_check_usage <- function(expression,
+                              known_used_symbols = character(),
+                              declared_globals = character(),
+                              start_line = 1L,
+                              skip_with = TRUE) {
   vals <- list()
 
   report <- function(x) {
@@ -211,7 +218,8 @@ parse_check_usage <- function(expression, known_used_symbols = character(), decl
         expression,
         report = report,
         suppressLocalUnused = known_used_symbols,
-        suppressUndefined = declared_globals
+        suppressUndefined = declared_globals,
+        skipWith = skip_with
       ))
     }
   )

tests/testthat/test-object_usage_linter.R

@@ -288,7 +288,7 @@ test_that("object_usage_linter finds lints spanning multiple lines", {
       }
     "),
     list(message = "unknown_symbol", line_number = 4L, column_number = 5L),
-    object_usage_linter()
+    object_usage_linter(skip_with = FALSE)
   )
 
   # Even ugly names are found
@@ -302,7 +302,7 @@ test_that("object_usage_linter finds lints spanning multiple lines", {
       }
     "),
     list(line_number = 4L, column_number = 5L),
-    object_usage_linter()
+    object_usage_linter(skip_with = FALSE)
   )
 })
 
@@ -480,3 +480,16 @@ test_that("unknown infix operators give good lint metadata", {
     object_usage_linter()
   )
 })
+
+test_that("respects `skip_with` argument for `with()` expressions", {
+  f <- withr::local_tempfile(
+    lines = c(
+      "test_fun <- function(df) {",
+      "  with(df, first_var + second_var)",
+      "}"
+    )
+  )
+
+  expect_length(lint(f, object_usage_linter(skip_with = TRUE)), 0L)
+  expect_length(lint(f, object_usage_linter(skip_with = FALSE)), 2L)
+})