Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
14 changes: 13 additions & 1 deletion R/implicit_assignment_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -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")) {
Expand Down
11 changes: 10 additions & 1 deletion man/implicit_assignment_linter.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 31 additions & 0 deletions tests/testthat/test-implicit_assignment_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
})