From 90513463776b73326047f8fa0a91984b50a50723 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 30 Jul 2025 16:43:39 +0000 Subject: [PATCH 01/10] dont warn if cyclocomp is missing --- R/with.R | 8 +++++++- tests/testthat/test-with.R | 9 +++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/R/with.R b/R/with.R index e4087a42c..0eb76fa3c 100644 --- a/R/with.R +++ b/R/with.R @@ -97,10 +97,16 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep } tagged_linters <- list() + dots <- list(...) + exclude_idx <- vapply(dots, is.null, logical(1L)) + excluded_linters <- names(dots)[exclude_idx] + dots <- dots[!exclude_idx] + for (package in packages) { pkg_ns <- loadNamespace(package) ns_exports <- getNamespaceExports(pkg_ns) available <- available_linters(packages = package, tags = tags, exclude_tags = exclude_tags) + available <- available[!available$linter %in% excluded_linters, ] if (nrow(available) > 0L) { if (!all(available$linter %in% ns_exports)) { missing_linters <- setdiff(available$linter, ns_exports) # nolint: object_usage_linter. TODO(#2252). @@ -120,7 +126,7 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep } } - modify_defaults(..., defaults = tagged_linters) + do.call(modify_defaults, c(list(defaults = tagged_linters), dots)) } #' Create a linter configuration based on all available linters diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index dc0e19494..3bb2b90c2 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -118,3 +118,12 @@ test_that("all_linters respects ellipsis argument", { all_linters(packages = "lintr", implicit_integer_linter = NULL) ) }) + +test_that("Excluding cyclocomp linter avoids a warning", { + local_mocked_bindings( + requireNamespace = function(pkg, ...) pkg != "cyclocomp" || base::requireNamespace(pkg, ...) + ) + + expect_silent(all_linters(cyclocomp_linter = NULL)) + expect_silent(linters_with_tags("configurable", cyclocomp_linter = NULL)) +}) From d89d06fb03238cb541309ce5db8deff9d3745d23 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 30 Jul 2025 18:25:40 +0000 Subject: [PATCH 02/10] revert --- R/with.R | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/R/with.R b/R/with.R index 0eb76fa3c..e4087a42c 100644 --- a/R/with.R +++ b/R/with.R @@ -97,16 +97,10 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep } tagged_linters <- list() - dots <- list(...) - exclude_idx <- vapply(dots, is.null, logical(1L)) - excluded_linters <- names(dots)[exclude_idx] - dots <- dots[!exclude_idx] - for (package in packages) { pkg_ns <- loadNamespace(package) ns_exports <- getNamespaceExports(pkg_ns) available <- available_linters(packages = package, tags = tags, exclude_tags = exclude_tags) - available <- available[!available$linter %in% excluded_linters, ] if (nrow(available) > 0L) { if (!all(available$linter %in% ns_exports)) { missing_linters <- setdiff(available$linter, ns_exports) # nolint: object_usage_linter. TODO(#2252). @@ -126,7 +120,7 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep } } - do.call(modify_defaults, c(list(defaults = tagged_linters), dots)) + modify_defaults(..., defaults = tagged_linters) } #' Create a linter configuration based on all available linters From d69d7a00bf170ed70726e7900a64b1d4bab2e651 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 30 Jul 2025 20:15:12 +0000 Subject: [PATCH 03/10] Delay evaluation of linter factories until modify_defaults --- R/with.R | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/R/with.R b/R/with.R index e4087a42c..9157b9dbd 100644 --- a/R/with.R +++ b/R/with.R @@ -34,8 +34,8 @@ modify_defaults <- function(defaults, ...) { if (missing(defaults)) { cli_abort("{.arg defaults} is a required argument, but is missing.") } - if (!is.list(defaults) || !all(nzchar(names2(defaults)))) { - cli_abort("{.arg defaults} must be a named list, not {.obj_type_friendly {defaults}}.") + if (!(is.list(defaults) || is.environment(defaults)) || !all(nzchar(names2(defaults)))) { + cli_abort("{.arg defaults} must be a named list or environment, not {.obj_type_friendly {defaults}}.") } vals <- list(...) nms <- names2(vals) @@ -53,7 +53,8 @@ modify_defaults <- function(defaults, ...) { } is.na(vals) <- nms == vals - defaults[nms] <- vals + for (ii in seq_along(nms)) defaults[[nms[ii]]] <- vals[[ii]] + if (is.environment(defaults)) defaults <- as.list(defaults) res <- defaults[!vapply(defaults, is.null, logical(1L))] res <- res[platform_independent_order(names(res))] @@ -95,7 +96,7 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep if (!is.character(tags) && !is.null(tags)) { cli_abort("{.arg tags} must be a character vector, or {.code NULL}, not {.obj_type_friendly {tags}}.") } - tagged_linters <- list() + tagged_linter_env <- new.env() for (package in packages) { pkg_ns <- loadNamespace(package) @@ -109,18 +110,14 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep i = "These are advertised by {.fn available_linters}, but are not exported by package {.pkg {package}}." )) } - linter_factories <- mget(available$linter, envir = pkg_ns) - linters <- Map( - call_linter_factory, - linter_factory = linter_factories, - linter_name = names(linter_factories), - MoreArgs = list(package = package) - ) - tagged_linters <- c(tagged_linters, linters) + for (linter in available$linter) { + linter_factory <- get(linter, envir = pkg_ns, inherits = FALSE) + delayedAssign(linter, call_linter_factory(linter_factory, linter, package), assign.env = tagged_linter_env) + } } } - modify_defaults(..., defaults = tagged_linters) + modify_defaults(..., defaults = tagged_linter_env) } #' Create a linter configuration based on all available linters From b90b3c5f419422996c426a521172b2bc1149f0c2 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 1 Dec 2025 15:30:06 +0000 Subject: [PATCH 04/10] fix delayedAssign issue --- R/with.R | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/R/with.R b/R/with.R index 35c4fa67c..724318763 100644 --- a/R/with.R +++ b/R/with.R @@ -114,8 +114,13 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep )) } for (linter in available$linter) { - linter_factory <- get(linter, envir = pkg_ns, inherits = FALSE) - delayedAssign(linter, call_linter_factory(linter_factory, linter, package), assign.env = tagged_linter_env) + # need an environment in each iteration, otherwise the lazy evaluation will use + # the value of 'linter' left over after the loop, i.e., tail(available$linter, 1) + local({ + linter_factory <- get(linter, envir = pkg_ns, inherits = FALSE) + linter <- linter # force a local copy to be found by delayedAssign + delayedAssign(linter, call_linter_factory(linter_factory, linter, package), assign.env = tagged_linter_env) + }) } } } From 1051124e26e5d08a27a2276a7090a5bc5edb7e3e Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 1 Dec 2025 15:53:04 +0000 Subject: [PATCH 05/10] simplify by using a helper over local() --- R/with.R | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/R/with.R b/R/with.R index 724318763..743ab9b08 100644 --- a/R/with.R +++ b/R/with.R @@ -102,8 +102,7 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep tagged_linter_env <- new.env() for (package in packages) { - pkg_ns <- loadNamespace(package) - ns_exports <- getNamespaceExports(pkg_ns) + ns_exports <- getNamespaceExports(package) available <- available_linters(packages = package, tags = tags, exclude_tags = exclude_tags) if (nrow(available) > 0L) { if (!all(available$linter %in% ns_exports)) { @@ -113,21 +112,29 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep i = "These are advertised by {.fn available_linters}, but are not exported by package {.pkg {package}}." )) } - for (linter in available$linter) { - # need an environment in each iteration, otherwise the lazy evaluation will use - # the value of 'linter' left over after the loop, i.e., tail(available$linter, 1) - local({ - linter_factory <- get(linter, envir = pkg_ns, inherits = FALSE) - linter <- linter # force a local copy to be found by delayedAssign - delayedAssign(linter, call_linter_factory(linter_factory, linter, package), assign.env = tagged_linter_env) - }) - } + for (linter in available$linter) lazily_assign_linter(linter, package, tagged_linter_env) } } modify_defaults(..., defaults = tagged_linter_env) } +#' Avoid call_linter_factory up-front to delay displaying warnings until needed. +#' This e.g. allows modify_defaults() to skip warnings from linters that aren't needed. +#' NB: this helper has the very subtle effect of ensuring 'linter' is associated correctly; +#' an earlier attempt had this logic directly in a loop in linters_with_tags, but +#' that results in the value of 'linter' matching that of the loop index after the loop +#' completes, rather than what its value was when 'delayedAssign()' is called. local({}) +#' _can_ work, but requires the befuddling line 'linter <- linter' to ensure that the +#' local() environment retains a copy of that variable; the formals of a helper +#' have the same effect. +#' @noRd +lazily_assign_linter <- function(linter, package, env) { + linter_factory <- get(linter, envir = getNamespace(package), inherits = FALSE) + delayedAssign(linter, call_linter_factory(linter_factory, linter, package), assign.env = env) +} + + #' Create a linter configuration based on all available linters #' #' @inheritParams linters_with_tags From 433f6e1f3d14cc600ace7f4ccb196a0d4e73037b Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 1 Dec 2025 16:02:21 +0000 Subject: [PATCH 06/10] append _ to avoid regex for linter$ --- R/with.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/with.R b/R/with.R index 743ab9b08..7b9aa999d 100644 --- a/R/with.R +++ b/R/with.R @@ -112,7 +112,7 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep i = "These are advertised by {.fn available_linters}, but are not exported by package {.pkg {package}}." )) } - for (linter in available$linter) lazily_assign_linter(linter, package, tagged_linter_env) + for (linter in available$linter) lazily_assign_linter_(linter, package, tagged_linter_env) } } @@ -129,7 +129,7 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep #' local() environment retains a copy of that variable; the formals of a helper #' have the same effect. #' @noRd -lazily_assign_linter <- function(linter, package, env) { +lazily_assign_linter_ <- function(linter, package, env) { linter_factory <- get(linter, envir = getNamespace(package), inherits = FALSE) delayedAssign(linter, call_linter_factory(linter_factory, linter, package), assign.env = env) } From 659c6e1c38aea86641b7741b763ce9a4b8be95ec Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 1 Dec 2025 16:34:08 +0000 Subject: [PATCH 07/10] add a case generating the expected warning --- tests/testthat/test-with.R | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index 24b22569f..ee85cc6cb 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -125,4 +125,5 @@ test_that("Excluding cyclocomp linter avoids a warning", { expect_silent(all_linters(cyclocomp_linter = NULL)) expect_silent(linters_with_tags("configurable", cyclocomp_linter = NULL)) + expect_warning(linters_with_tags("configurable"), "cyclocomp::cyclocomp") }) From 589b9a9bc5cf44855ef41bf7d2366870b874cdc8 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 1 Dec 2025 16:35:17 +0000 Subject: [PATCH 08/10] remove blank line --- R/with.R | 1 - 1 file changed, 1 deletion(-) diff --git a/R/with.R b/R/with.R index 7b9aa999d..8abbed4f8 100644 --- a/R/with.R +++ b/R/with.R @@ -134,7 +134,6 @@ lazily_assign_linter_ <- function(linter, package, env) { delayedAssign(linter, call_linter_factory(linter_factory, linter, package), assign.env = env) } - #' Create a linter configuration based on all available linters #' #' @inheritParams linters_with_tags From fce809c0fe3d83892ec1f9fe470a1215510b2387 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 1 Dec 2025 16:37:28 +0000 Subject: [PATCH 09/10] NEWS --- NEWS.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/NEWS.md b/NEWS.md index 5eddfba41..94973eb9c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,6 +4,10 @@ * Six linters fully deprecated in the previous release are now removed: `consecutive_stopifnot_linter()`, `extraction_operator_linter()`, `no_tab_linter()`, `single_quotes_linter()`, `unnecessary_nested_if_linter()`, and `unneeded_concatenation_linter()`. +## Bug fixes + +* Excluding `cyclocomp_linter()` in `available_linters()` or `linters_with_tags()`, which requires the weak dependency {cyclocomp}, no longer emits a warning (#2909, @MichaelChirico). + ## Notes * {lintr} now requires R 4.1.0 From fda7119ebe99bd323c209b1f5ec49d52a8954950 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 1 Dec 2025 20:15:08 +0000 Subject: [PATCH 10/10] fix mocking to get warning if cyclocomp is installed --- tests/testthat/test-with.R | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index ee85cc6cb..822c4df0b 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -125,5 +125,12 @@ test_that("Excluding cyclocomp linter avoids a warning", { expect_silent(all_linters(cyclocomp_linter = NULL)) expect_silent(linters_with_tags("configurable", cyclocomp_linter = NULL)) +}) + +test_that("cyclocomp_linter does warn as intended", { + local_mocked_bindings( + requireNamespace = function(pkg, ...) pkg != "cyclocomp" && base::requireNamespace(pkg, ...) + ) + expect_warning(linters_with_tags("configurable"), "cyclocomp::cyclocomp") })