New unnecessary_placeholder_linter (#1656)

* New unnecessary_placeholder_linter

* add examples, links

* remove TODO (posted as issue comment)

Co-authored-by: Indrajeet Patil <[email protected]>

Author: MichaelChirico

DESCRIPTION

@@ -152,6 +152,7 @@ Collate:
     'undesirable_function_linter.R'
     'undesirable_operator_linter.R'
     'unnecessary_lambda_linter.R'
+    'unnecessary_placeholder_linter.R'
     'unneeded_concatenation_linter.R'
     'unreachable_code_linter.R'
     'unused_import_linter.R'

NAMESPACE

@@ -118,6 +118,7 @@ export(trailing_whitespace_linter)
 export(undesirable_function_linter)
 export(undesirable_operator_linter)
 export(unnecessary_lambda_linter)
+export(unnecessary_placeholder_linter)
 export(unneeded_concatenation_linter)
 export(unreachable_code_linter)
 export(unused_import_linter)

NEWS.md

@@ -61,6 +61,8 @@
 
 * `empty_assignment_linter()` for identifying empty assignments like `x = {}` that are more clearly written as `x = NULL` (@MichaelChirico)
 
+* `unnecessary_placeholder_linter()` for identifying where usage of the {magrittr} placeholder `.` could be omitted (@MichaelChirico)
+
 ## Notes
 
 * `lint()` continues to support Rmarkdown documents. For users of custom .Rmd engines, e.g.

R/unnecessary_placeholder_linter.R

@@ -0,0 +1,69 @@
+#' Block usage of pipeline placeholders if unnecessary
+#'
+#' The argument placeholder `.` in magrittr pipelines is unnecessary if
+#'   passed as the first positional argument; using it can cause confusion
+#'   and impacts readability.
+#'
+#' @examples
+#' # will produce lints
+#' lint(
+#'   text = "x %>% sum(., na.rm = TRUE)",
+#'   linters = unnecessary_placeholder_linter()
+#' )
+#'
+#' # okay
+#' lint(
+#'   text = "x %>% sum(na.rm = TRUE)",
+#'   linters = unnecessary_placeholder_linter()
+#' )
+#'
+#' lint(
+#'   text = "x %>% lm(data = ., y ~ z)",
+#'   linters = unnecessary_placeholder_linter()
+#' )
+#'
+#' lint(
+#'   text = "x %>% outer(., .)",
+#'   linters = unnecessary_placeholder_linter()
+#' )
+#'
+#' @evalRd rd_tags("unnecessary_placeholder_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+unnecessary_placeholder_linter <- function() {
+  # TODO(michaelchirico): handle R4.2.0 native placeholder _ as well
+  xpath <- "
+  //SPECIAL[text() = '%>%']
+    /following-sibling::expr[
+      expr/SYMBOL_FUNCTION_CALL
+      and not(expr[
+        position() > 2
+        and descendant-or-self::expr/SYMBOL[text() = '.']
+      ])
+    ]
+    /expr[2][
+      SYMBOL[text() = '.']
+      and not(preceding-sibling::*[1][self::EQ_SUB])
+    ]
+  "
+
+  Linter(function(source_expression) {
+    if (!is_lint_level(source_expression, "expression")) {
+      return(list())
+    }
+
+    xml <- source_expression$xml_parsed_content
+
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+
+    xml_nodes_to_lints(
+      bad_expr,
+      source_expression = source_expression,
+      lint_message = paste(
+        "Don't use the placeholder (`.`) when it's not needed,",
+        "i.e., when it's only used as the first positional argument in a pipeline step."
+      ),
+      type = "warning"
+    )
+  })
+}

inst/lintr/linters.csv

@@ -79,6 +79,7 @@ trailing_whitespace_linter,style default
 undesirable_function_linter,style efficiency configurable robustness best_practices
 undesirable_operator_linter,style efficiency configurable robustness best_practices
 unnecessary_lambda_linter,best_practices efficiency readability
+unnecessary_placeholder_linter,readability best_practices
 unneeded_concatenation_linter,style readability efficiency configurable
 unreachable_code_linter,best_practices readability
 unused_import_linter,best_practices common_mistakes configurable executing

man/best_practices_linters.Rd

@@ -0,0 +1,25 @@
+test_that("unnecessary_placeholder_linter skips allowed usages", {
+  # . used in position other than first --> ok
+  expect_lint("x %>% foo(y, .)", NULL, unnecessary_placeholder_linter())
+  # ditto for nested usage
+  expect_lint("x %>% foo(y, bar(.))", NULL, unnecessary_placeholder_linter())
+  # . used twice --> ok
+  expect_lint("x %>% foo(., .)", NULL, unnecessary_placeholder_linter())
+  # . used as a keyword argument --> ok
+  expect_lint("x %>% foo(arg = .)", NULL, unnecessary_placeholder_linter())
+})
+
+test_that("unnecessary_placeholder_linter blocks simple disallowed usages", {
+  expect_lint(
+    "x %>% sum(.)",
+    rex::rex("Don't use the placeholder (`.`) when it's not needed"),
+    unnecessary_placeholder_linter()
+  )
+
+  # can come anywhere in the pipeline
+  expect_lint(
+    "x %>% y(.) %>% sum()",
+    rex::rex("Don't use the placeholder (`.`) when it's not needed"),
+    unnecessary_placeholder_linter()
+  )
+})