Merge branch 'main' into vl-filter

NEWS.md

@@ -40,6 +40,7 @@
 * `string_boundary_linter()` recognizes regular expression calls like `grepl("^abc$", x)` that can be replaced by using `==` instead (#1613, @MichaelChirico).
 * `unreachable_code_linter()` has an argument `allow_comment_regex` for customizing which "terminal" comments to exclude (#2327, @MichaelChirico). `# nolint end` comments are always excluded, as are {covr} exclusions (e.g. `# nocov end`) by default.
 * `format()` and `print()` methods for `lint` and `lints` classes get a new option `width` to control the printing width of lint messages (#1884, @MichaelChirico). The default is controlled by a new option `lintr.format_width`; if unset, no wrapping occurs (matching earlier behavior).
+* `implicit_assignment_linter()` gets a custom message for the case of using `(` to induce printing like `(x <- foo())`; use an explicit call to `print()` for clarity (#2257, @MichaelChirico).
 * New function node caching for big efficiency gains to most linters (e.g. overall `lint_package()` improvement of 14-27% and core linting improvement up to 30%; #2357, @AshesITR). Most linters are written around function usage, and XPath performance searching for many functions is poor. The new `xml_find_function_calls()` entry in the `get_source_expressions()` output caches all function call nodes instead. See the vignette on creating linters for more details on how to use it.
 * `vector_logic_linter()` is extended to recognize incorrect usage of scalar operators `&&` and `||` inside subsetting expressions like `dplyr::filter(x, A && B)` (#2166, @MichaelChirico).
 * `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico).

R/implicit_assignment_linter.R

@@ -102,21 +102,25 @@ implicit_assignment_linter <- function(except = c("bquote", "expression", "expr"
  )
  }
 
+ implicit_message <- paste(
+ "Avoid implicit assignments in function calls.",
+ "For example, instead of `if (x <- 1L) { ... }`, write `x <- 1L; if (x) { ... }`."
+ )
+
+ print_message <- "Call print() explicitly instead of relying on implicit printing behavior via '('."
+
  Linter(linter_level = "file", function(source_expression) {
  # need the full file to also catch usages at the top level
  xml <- source_expression$full_xml_parsed_content
 
  bad_expr <- xml_find_all(xml, xpath)
 
- lint_message <- paste(
- "Avoid implicit assignments in function calls.",
- "For example, instead of `if (x <- 1L) { ... }`, write `x <- 1L; if (x) { ... }`."
- )
+ print_only <- !is.na(xml_find_first(bad_expr, "parent::expr[parent::exprlist and *[1][self::OP-LEFT-PAREN]]"))
 
  xml_nodes_to_lints(
  bad_expr,
  source_expression = source_expression,
- lint_message = lint_message,
+ lint_message = ifelse(print_only, print_message, implicit_message),
  type = "warning"
  )
  })

R/sort_linter.R

@@ -125,7 +125,11 @@ sort_linter <- function() {
  order_lints <- xml_nodes_to_lints(
  order_expr,
  source_expression = source_expression,
- lint_message = paste0(new_call, " is better than ", orig_call, "."),
+ lint_message = paste0(
+ new_call, " is better than ", orig_call, ". ",
+ "Note that it's always preferable to save the output of order() for the same variable ",
+ "as a local variable than to re-compute it."
+ ),
  type = "warning"
  )
 

tests/testthat/test-implicit_assignment_linter.R

@@ -347,11 +347,11 @@ 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)
+ expect_lint(
+  "if (A && (B <- foo())) { }",
+ rex::rex("Avoid implicit assignments in function calls."),
+  implicit_assignment_linter()
+ )
 })
 
 test_that("allow_lazy lets lazy assignments through", {
@@ -439,7 +439,6 @@ test_that("allow_scoped skips scoped assignments", {
  )
 
  # outside of branching, doesn't matter
- expect_lint("(idx <- foo()); bar()", lint_message, linter)
  expect_lint("foo(idx <- bar()); baz()", lint_message, linter)
  expect_lint("foo(x, idx <- bar()); baz()", lint_message, linter)
 })
@@ -477,3 +476,24 @@ test_that("interaction of allow_lazy and allow_scoped", {
  linter
  )
 })
+
+test_that("call-less '(' mentions avoiding implicit printing", {
+ linter <- implicit_assignment_linter()
+ implicit_msg <- rex::rex("Avoid implicit assignments in function calls.")
+ print_msg <- rex::rex("Call print() explicitly instead of relying on implicit printing behavior via '('.")
+
+ expect_lint("(a <- foo())", print_msg, linter)
+
+ # only for top-level assignments; withAutoprint() ignored
+ expect_lint("for (xi in x) (z <- foo(xi))", implicit_msg, linter)
+
+ # mixed messages
+ expect_lint(
+ trim_some("
+ (a <- foo())
+ bar(z <- baz(a))
+ "),
+ list(print_msg, implicit_msg),
+ linter
+ )
+})