Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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 @@ -47,6 +47,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
)
})