Merge branch 'main' into f-2386-use-cli

.Rbuildignore

@@ -8,7 +8,7 @@
 ^\.Rproj\.user$
 ^\.idea$
 ^\.dev$
-^\.devcontainers$
+^\.devcontainer$
 ^\.lintr$
 ^\.lintr_new$
 ^wercker\.yml$

.dev/roxygen_test.R

@@ -0,0 +1,54 @@
+# Test to ensure roxygenize() has been run on the current PR
+library(tools)
+library(roxygen2)
+
+old_dir <- file.path(tempdir(), "man")
+if (dir.exists(old_dir)) unlink(old_dir, recursive = TRUE)
+file.copy("man", tempdir(), recursive = TRUE)
+old_files <- list.files(old_dir, pattern = "\\.Rd$")
+new_dir <- "man"
+.Last <- function() unlink(old_dir, recursive = TRUE)
+
+# Rd2txt() prints to its out= argument, so we'd have to compare file contents;
+#   plain parse_Rd() keeps srcref info that encodes the file path, which as.character() strips.
+normalize_rd <- function(rd_file) as.character(parse_Rd(rd_file))
+
+rd_equal <- function(f1, f2) isTRUE(all.equal(normalize_rd(f1), normalize_rd(f2)))
+
+check_roxygenize_idempotent <- function(LOCALE) {
+  Sys.setlocale("LC_COLLATE", LOCALE)
+  roxygenize()
+
+  new_files <- list.files(new_dir, pattern = "\\.Rd$")
+
+  old_not_new <- setdiff(old_files, new_files)
+  if (length(old_not_new) > 0L) {
+    stop("Found saved .Rd files gone from a fresh run of roxygenize(): ", toString(old_not_new))
+  }
+
+  new_not_old <- setdiff(new_files, old_files)
+  if (length(new_not_old) > 0L) {
+    stop("Found new .Rd files from a fresh run of roxygenize(): ", toString(new_not_old))
+  }
+
+  for (file in new_files) {
+    old_file <- file.path(old_dir, file)
+    new_file <- file.path(new_dir, file)
+    if (rd_equal(old_file, new_file)) {
+      next
+    }
+    cat(sprintf("roxygenize() output differs from saved output for %s.\n", file))
+    cat("Here's the 'diff' comparison of the two files:\n")
+    cat("  [---]: saved output in man/ directory\n")
+    cat("  [+++]: roxygenize() output of R/ sources\n")
+    system2("diff", c("--unified", old_file, new_file))
+    stop("Failed in LOCALE=", LOCALE, ".", call. = FALSE)
+  }
+}
+
+# Run the check in a few locales to ensure there's no idempotency issues w.r.t. sorting, too
+for (LOCALE in c("C", "en_US", "hu_HU", "ja_JP")) {
+  check_roxygenize_idempotent(LOCALE)
+}
+
+unlink(old_dir, recursive = TRUE)

.devcontainer/Dockerfile

@@ -0,0 +1,14 @@
+FROM rocker/r-base
+
+RUN apt-get -qq update && \
+  apt-get install -y --no-install-recommends git libxml2-dev
+
+COPY DESCRIPTION .
+
+RUN Rscript -e '                           \
+  install.packages("remotes");             \
+  remotes::install_deps(dependencies = c(  \
+    "Imports",                             \
+    "Config/needs/development"             \
+  ))                                       \
+'

.devcontainer/devcontainer.json

@@ -0,0 +1,3 @@
+{
+    "build": { "dockerfile": "Dockerfile", "context": ".."}
+}

.github/workflows/ensure-metadata-tests.yaml → .github/workflows/repo-meta-tests.yaml

@@ -1,14 +1,14 @@
-# Ensure lint metadata is tested
+# Various repo-level tests for code quality
 on:
   push:
     branches: [main]
   pull_request:
     branches: [main]
 
-name: ensure-metadata-tests
+name: repo-meta-tests
 
 jobs:
-  ensure-metadata-tests:
+  repo-meta-tests:
     runs-on: ubuntu-latest
     env:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
@@ -28,3 +28,8 @@ jobs:
           options(crayon.enabled = TRUE)
           callr::rscript(".dev/lint_metadata_test.R")
         shell: Rscript {0}
+
+      - name: Ensure roxygen content matches man directory
+        run: |
+          callr::rscript(".dev/roxygen_test.R")
+        shell: Rscript {0}

.lintr

@@ -2,6 +2,18 @@ linters: all_linters(
     backport_linter("3.6.0", except = c("R_user_dir", "deparse1", "...names")),
     line_length_linter(120L),
     object_overwrite_linter(allow_names = c("line", "lines", "pipe", "symbols")),
+    todo_comment_linter(
+      except_regex = rex::rex(
+        "TODO(",
+        group(or(
+          # GitHub issue number #1234, possibly from another repo org/repo#5678
+          list(maybe(one_or_more(alnum, "-"), "/", one_or_more(alnum, ".", "-", "_")), "#", one_or_more(digit)),
+          # GitHub user. TODO(#2450): remove this temporary immunity
+          one_or_more(alnum, "-")
+        )),
+        ")"
+      )
+    ),
     undesirable_function_linter(modify_defaults(
       defaults = default_undesirable_functions,
       library = NULL,

DESCRIPTION

@@ -52,6 +52,7 @@ Enhances:
 VignetteBuilder: 
     knitr
 Config/Needs/website: tidyverse/tidytemplate
+Config/Needs/development: pkgload, cli, testthat, patrick
 Config/testthat/edition: 3
 Encoding: UTF-8
 Roxygen: list(markdown = TRUE)
@@ -192,7 +193,6 @@ Collate:
     'undesirable_operator_linter.R'
     'unnecessary_concatenation_linter.R'
     'unnecessary_lambda_linter.R'
-    'unnecessary_nested_if_linter.R'
     'unnecessary_nesting_linter.R'
     'unnecessary_placeholder_linter.R'
     'unreachable_code_linter.R'

NEWS.md

@@ -13,6 +13,7 @@
 * `all_linters()` has signature `all_linters(..., packages)` rather than `all_linters(packages, ...)` (#2332, @MichaelChirico). This forces `packages=` to be supplied by name and will break users who rely on supplying `packages=` positionally, of which we found none searching GitHub.
 * Adjusted various lint messages for consistency in readability (#1330, @MichaelChirico). In general, we favor lint messages to be phrased like "Action, reason" to but the "what" piece of the message front-and-center. This may be a breaking change for code that tests the specific phrasing of lints.
 * `extraction_operator_linter()` is deprecated. Although switching from `$` to `[[` has some robustness benefits for package code, it can lead to non-idiomatic code in many contexts (e.g. R6 classes, Shiny applications, etc.) (#2409, @IndrajeetPatil). To enable the detection of the `$` operator for extraction through partial matching, use `options(warnPartialMatchDollar = TRUE)`.
+* `unnecessary_nested_if_linter()` is deprecated and subsumed into the new/more general `unnecessary_nesting_linter()`.
 
 ## Bug fixes
 
@@ -21,7 +22,7 @@
 
 ## Changes to default linters
 
-* New default linter `return_linter()` for the style guide rule that terminal returns should be left implicit (#1100, #2354, and #2356, @MEO265 and @MichaelChirico).
+* New default linter `return_linter()` for the style guide rule that terminal returns should be left implicit (#1100, #2343, #2354, and #2356, @MEO265 and @MichaelChirico).
 
 ## New and improved features
 
@@ -43,6 +44,7 @@
 * `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.
+* `todo_comment_linter()` has a new argument `except_regex` for setting _valid_ TODO comments, e.g. for forcing TODO comments to be linked to GitHub issues like `TODO(#154)` (#2047, @MichaelChirico).
 * `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).
 
@@ -58,11 +60,11 @@
 * `which_grepl_linter()` for discouraging `which(grepl(ptn, x))` in favor of directly using `grep(ptn, x)` (part of #884, @MichaelChirico).
 * `list_comparison_linter()` for discouraging comparisons on the output of `lapply()`, e.g. `lapply(x, sum) > 10` (part of #884, @MichaelChirico).
 * `print_linter()` for discouraging usage of `print()` on string literals like `print("Reached here")` or `print(paste("Found", nrow(DF), "rows."))` (#1894, @MichaelChirico).
-* `unnecessary_nesting_linter()` for discouraging overly-nested code where an early return or eliminated sub-expression (inside '{') is preferable (part of #884, @MichaelChirico).
+* `unnecessary_nesting_linter()` for discouraging overly-nested code where an early return or eliminated sub-expression (inside '{') is preferable (#2317 and part of #884, @MichaelChirico).
 * `consecutive_mutate_linter()` for encouraging consecutive calls to `dplyr::mutate()` to be combined (part of #884, @MichaelChirico).
 * `if_switch_linter()` for encouraging `switch()` over repeated `if`/`else` tests (part of #884, @MichaelChirico).
 * `nested_pipe_linter()` for discouraging pipes within pipes, e.g. `df1 %>% inner_join(df2 %>% select(a, b))` (part of #884, @MichaelChirico).
-* `nrow_subset_linter()` for discouraging usage like `nrow(subset(x, conditions))` in favor of something like `with(x, sum(conditions))` which doesn't require a full subset of `x` (#2314 and part of #884, @MichaelChirico).
+* `nrow_subset_linter()` for discouraging usage like `nrow(subset(x, conditions))` in favor of something like `with(x, sum(conditions))` which doesn't require a full subset of `x` (#2313, #2314 and part of #884, @MichaelChirico).
 * `pipe_return_linter()` for discouraging usage of `return()` inside a {magrittr} pipeline (part of #884, @MichaelChirico).
 * `one_call_pipe_linter()` for discouraging one-step pipelines like `x |> as.character()` (#2330 and part of #884, @MichaelChirico).
 * `object_overwrite_linter()` for discouraging re-use of upstream package exports as local variables (#2344, #2346 and part of #884, @MichaelChirico and @AshesITR).

R/brace_linter.R

@@ -74,7 +74,7 @@ brace_linter <- function(allow_single_line = FALSE) {
     )")
   ))
 
-  # TODO (AshesITR): if c_style_braces is TRUE, invert the preceding-sibling condition
+  # TODO(#1103): if c_style_braces is TRUE, invert the preceding-sibling condition
   xp_open_curly <- glue("//OP-LEFT-BRACE[
     { xp_cond_open }
     and (
@@ -109,7 +109,7 @@ brace_linter <- function(allow_single_line = FALSE) {
     )"
   ))
 
-  # TODO (AshesITR): if c_style_braces is TRUE, skip the not(ELSE) condition
+  # TODO(#1103): if c_style_braces is TRUE, skip the not(ELSE) condition
   xp_closed_curly <- glue("//OP-RIGHT-BRACE[
     { xp_cond_closed }
     and (
@@ -121,7 +121,7 @@ brace_linter <- function(allow_single_line = FALSE) {
   xp_else_closed_curly <- "preceding-sibling::IF/following-sibling::expr[2]/OP-RIGHT-BRACE"
   # need to (?) repeat previous_curly_path since != will return true if there is
   #   no such node. ditto for approach with not(@line1 = ...).
-  # TODO (AshesITR): if c_style_braces is TRUE, this needs to be @line2 + 1
+  # TODO(#1103): if c_style_braces is TRUE, this needs to be @line2 + 1
   xp_else_same_line <- glue("//ELSE[{xp_else_closed_curly} and @line1 != {xp_else_closed_curly}/@line2]")
 
   xp_function_brace <- "(//FUNCTION | //OP-LAMBDA)/parent::expr[@line1 != @line2 and not(expr[OP-LEFT-BRACE])]"

R/get_source_expressions.R

@@ -640,18 +640,7 @@ fix_eq_assigns <- function(pc) {
 
   for (i in seq_len(n_expr)) {
     start_loc <- true_locs[i]
-
-    # TODO(michaelchirico): vectorize this loop away. the tricky part is,
-    #   this loop doesn't execute on most R versions (we tried 3.6.3 and 4.2.0).
-    #   so it likely requires some GHA print debugging -- tedious :)
     end_loc <- true_locs[i]
-    j <- end_loc + 1L
-    # nocov start: only runs on certain R versions
-    while (j <= length(expr_locs) && !expr_locs[j]) {
-      end_loc <- j
-      j <- j + 1L
-    }
-    # nocov end
 
     prev_loc <- prev_locs[start_loc]
     next_loc <- next_locs[end_loc]

R/lint.R

@@ -24,8 +24,6 @@
 #' @param text Optional argument for supplying a string or lines directly, e.g. if the file is already in memory or
 #'   linting is being done ad hoc.
 #'
-#' @aliases lint_file
-# TODO(next release after 3.0.0): remove the alias
 #' @return An object of class `c("lints", "list")`, each element of which is a `"list"` object.
 #'
 #' @examplesIf requireNamespace("withr", quietly = TRUE)
@@ -36,7 +34,9 @@
 #'
 #' @export
 lint <- function(filename, linters = NULL, ..., cache = FALSE, parse_settings = TRUE, text = NULL) {
-  check_dots(...names(), c("exclude", "parse_exclusions"))
+  # TODO(#2502): Remove this workaround.
+  dot_names <- if (getRversion() %in% c("4.1.1", "4.1.2")) names(list(...)) else ...names()
+  check_dots(dot_names, c("exclude", "parse_exclusions"))
 
   needs_tempfile <- missing(filename) || re_matches(filename, rex(newline))
   inline_data <- !is.null(text) || needs_tempfile
@@ -139,7 +139,9 @@ lint_dir <- function(path = ".", ...,
                      pattern = "(?i)[.](r|rmd|qmd|rnw|rhtml|rrst|rtex|rtxt)$",
                      parse_settings = TRUE,
                      show_progress = NULL) {
-  check_dots(...names(), c("lint", "exclude", "parse_exclusions"))
+  # TODO(#2502): Remove this workaround.
+  dot_names <- if (getRversion() %in% c("4.1.1", "4.1.2")) names(list(...)) else ...names()
+  check_dots(dot_names, c("lint", "exclude", "parse_exclusions"))
 
   if (isTRUE(parse_settings)) {
     read_settings(path)

R/lintr-deprecated.R

@@ -161,3 +161,41 @@ extraction_operator_linter <- function() {
     )
   })
 }
+
+#' Unnecessary nested if linter
+#' @rdname lintr-deprecated
+#' @export
+unnecessary_nested_if_linter <- function() {
+  lintr_deprecated(
+    what = "unnecessary_nested_if_linter",
+    alternative = "unnecessary_nesting_linter",
+    version = "3.2.0",
+    type = "Linter",
+    signal = "warning"
+  )
+
+  xpath <- paste0(
+    "//IF/parent::expr[not(ELSE)]/OP-RIGHT-PAREN/",
+    c(
+      "following-sibling::expr[IF and not(ELSE)]", # catch if (cond) if (other_cond) { ... }
+      "following-sibling::expr[OP-LEFT-BRACE and count(expr) = 1]
+         /expr[IF and not(ELSE)]" # catch if (cond) { if (other_cond) { ... } }
+    ),
+    collapse = " | "
+  )
+
+  Linter(linter_level = "expression", function(source_expression) {
+    xml <- source_expression$xml_parsed_content
+
+    bad_exprs <- xml_find_all(xml, xpath)
+    xml_nodes_to_lints(
+      bad_exprs,
+      source_expression = source_expression,
+      lint_message = paste(
+        "Don't use nested `if` statements,",
+        "where a single `if` with the combined conditional expression will do.",
+        "For example, instead of `if (x) { if (y) { ... }}`, use `if (x && y) { ... }`."
+      )
+    )
+  })
+}

R/missing_argument_linter.R

@@ -58,7 +58,7 @@ missing_argument_linter <- function(except = c("alist", "quote", "switch"), allo
     named_idx <- xml_name(missing_args) == "EQ_SUB"
     arg_id <- character(length(missing_args))
     arg_id[named_idx] <- sQuote(xml_find_chr(missing_args[named_idx], "string(preceding-sibling::SYMBOL_SUB[1])"), "'")
-    # TODO(r-lib/xml2#412-->CRAN): use xml_find_int() instead
+    # TODO(#2452): use xml_find_int() instead
     arg_id[!named_idx] <- xml_find_num(missing_args[!named_idx], "count(preceding-sibling::OP-COMMA)") + 1.0
 
     xml_nodes_to_lints(

R/nrow_subset_linter.R

@@ -14,6 +14,16 @@
 #'   linters = nrow_subset_linter()
 #' )
 #'
+#' lint(
+#'   text = "nrow(filter(x, is_treatment))",
+#'   linters = nrow_subset_linter()
+#' )
+#'
+#' lint(
+#'   text = "x %>% filter(x, is_treatment) %>% nrow()",
+#'   linters = nrow_subset_linter()
+#' )
+#'
 #' # okay
 #' lint(
 #'   text = "with(x, sum(is_treatment, na.rm = TRUE))",
@@ -25,7 +35,7 @@
 #' @include shared_constants.R
 #' @export
 nrow_subset_linter <- make_linter_from_function_xpath(
-  function_names = "subset",
+  function_names = c("subset", "filter"),
   xpath = glue("
   parent::expr
     /parent::expr