allow_lazy argument for implicit_assignment_linter to skip lazy assignment (#2140)

* simplify implicit_assignment_linter logic, and catch false negatives

* narrow line

* add a simpler test of lints in all call args

* catch parenthetical assignments

* allow_lazy argument for implicit_assignment_linter to skip lazy assignments

* + not * for bullet (consistency)

---------

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

Author: MichaelChirico

NEWS.md

@@ -48,6 +48,7 @@
 * `implicit_assignment_linter()`
   + finds assignments in call arguments besides the first one (#2136, @MichaelChirico).
   + finds assignments in parenthetical expressions like `if (A && (B <- foo(A))) { }` (#2138, @MichaelChirico).
+  + gains an argument `allow_lazy` (default `FALSE`) that allows optionally skipping lazy assignments like `A && (B <- foo(A))` (#2016, @MichaelChirico).
 * `implicit_assignment_linter()` finds assignments in call arguments besides the first one (#2136, @MichaelChirico).
 * `inner_combine_linter()` no longer throws on length-1 calls to `c()` like `c(exp(2))` or `c(log(3))` (#2017, @MichaelChirico). Such usage is discouraged by `unnecessary_concatenation_linter()`, but `inner_combine_linter()` _per se_ does not apply.
 * `condition_message_linter()` ignores usages of extracted calls like `env$stop(paste(a, b))` (#1455, @MichaelChirico).

R/implicit_assignment_linter.R

@@ -4,6 +4,8 @@
 #' be avoided, except for functions that capture side-effects (e.g. [capture.output()]).
 #'
 #' @param except A character vector of functions to be excluded from linting.
+#' @param allow_lazy logical, default `FALSE`. If `TRUE`, assignments that only
+#'   trigger conditionally (e.g. in the RHS of `&&` or `||` expressions) are skipped.
 #'
 #' @examples
 #' # will produce lints
@@ -30,13 +32,19 @@
 #'   linters = implicit_assignment_linter()
 #' )
 #'
+#' lint(
+#'   text = "A && (B <- foo(A))",
+#'   linters = implicit_assignment_linter(allow_lazy = TRUE)
+#' )
+#'
 #' @evalRd rd_tags("implicit_assignment_linter")
 #' @seealso
 #' - [linters] for a complete list of linters available in lintr.
 #' - <https://style.tidyverse.org/syntax.html#assignment>
 #'
 #' @export
-implicit_assignment_linter <- function(except = c("bquote", "expression", "expr", "quo", "quos", "quote")) {
+implicit_assignment_linter <- function(except = c("bquote", "expression", "expr", "quo", "quos", "quote"),
+                                       allow_lazy = FALSE) {
   stopifnot(is.null(except) || is.character(except))
 
   if (length(except) > 0L) {
@@ -64,6 +72,10 @@ implicit_assignment_linter <- function(except = c("bquote", "expression", "expr"
       ]
   ")
 
+  if (allow_lazy) {
+    xpath <- paste0(xpath, "[not(ancestor::expr/preceding-sibling::*[self::AND2 or self::OR2])]")
+  }
+
   Linter(function(source_expression) {
     # need the full file to also catch usages at the top level
     if (!is_lint_level(source_expression, "file")) {

man/implicit_assignment_linter.Rd

@@ -349,6 +349,37 @@ test_that("implicit_assignment_linter works as expected with pipes and walrus op
 test_that("parenthetical assignments are caught", {
   linter <- implicit_assignment_linter()
   lint_message <- rex::rex("Avoid implicit assignments in function calls.")
+
   expect_lint("(x <- 1:10)", lint_message, linter)
   expect_lint("if (A && (B <- foo())) { }", lint_message, linter)
 })
+
+test_that("allow_lazy lets lazy assignments through", {
+  linter <- implicit_assignment_linter(allow_lazy = TRUE)
+  lint_message <- rex::rex("Avoid implicit assignments in function calls.")
+
+  expect_lint("A && (B <- foo(A))", NULL, linter)
+  # || also admits laziness
+  expect_lint("A || (B <- foo(A))", NULL, linter)
+  # & and |, however, do not
+  expect_lint("A & (B <- foo(A))", lint_message, linter)
+  expect_lint("A | (B <- foo(A))", lint_message, linter)
+  expect_lint("A && foo(bar(idx <- baz()))", NULL, linter)
+  # LHS _is_ linted
+  expect_lint("(A <- foo()) && B", lint_message, linter)
+  # however we skip on _any_ RHS (even if it's later an LHS)
+  # test on all &&/|| combinations to stress test operator precedence
+  expect_lint("A && (B <- foo(A)) && C", NULL, linter)
+  expect_lint("A && (B <- foo(A)) || C", NULL, linter)
+  expect_lint("A || (B <- foo(A)) && C", NULL, linter)
+  expect_lint("A || (B <- foo(A)) || C", NULL, linter)
+  # &&/|| elsewhere in the tree don't matter
+  expect_lint(
+    trim_some("
+      A && B
+      foo(C <- bar())
+    "),
+    lint_message,
+    linter
+  )
+})