From 7d78f50438c713ecfb180edf025dafe660a158b7 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 30 Jun 2026 13:31:06 +0200 Subject: [PATCH 1/4] Expand `@import` directives to explicit `importFrom()` --- NAMESPACE | 450 +++++++++++++++++++++- NEWS.md | 1 + R/namespace.R | 117 +++++- man/namespace_roclet.Rd | 7 +- tests/testthat/_snaps/namespace.md | 21 + tests/testthat/test-namespace.R | 126 ++++++ tests/testthat/testImports/DESCRIPTION | 7 + tests/testthat/testImports/NAMESPACE | 3 + tests/testthat/testImports/R/exports.R | 3 + tests/testthat/testRawNamespace/NAMESPACE | 7 +- tests/testthat/testRawNamespace/R/a.R | 3 +- vignettes/namespace.Rmd | 7 +- 12 files changed, 733 insertions(+), 19 deletions(-) create mode 100644 tests/testthat/testImports/DESCRIPTION create mode 100644 tests/testthat/testImports/NAMESPACE create mode 100644 tests/testthat/testImports/R/exports.R diff --git a/NAMESPACE b/NAMESPACE index 3bcd46a1..d1d1d629 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -287,13 +287,461 @@ export(tags_metadata) export(update_collate) export(vignette_roclet) export(warn_roxy_tag) -import(rlang) importFrom(R6,R6Class) importFrom(knitr, knit, opts_chunk ) importFrom(lifecycle,deprecated) +importFrom(rlang, + "!!!", + "!!", + "%&&%", + "%<~%", + "%@%", + "%@%<-", + "%|%", + "%||%", + ":=", + "f_env<-", + "f_lhs<-", + "f_rhs<-", + "fn_body<-", + "fn_env<-", + "fn_fmls<-", + "fn_fmls_names<-", + "names2<-", + .data, + .env, + UQ, + UQS, + abort, + are_na, + arg_match, + arg_match0, + as_box, + as_box_if, + as_bytes, + as_character, + as_closure, + as_complex, + as_data_mask, + as_data_pronoun, + as_double, + as_environment, + as_function, + as_integer, + as_label, + as_list, + as_logical, + as_name, + as_quosure, + as_quosures, + as_string, + as_utf8_character, + base_env, + bytes, + call2, + call_args, + call_args_names, + call_fn, + call_inspect, + call_match, + call_modify, + call_name, + call_ns, + call_standardise, + caller_arg, + caller_call, + caller_env, + caller_fn, + calling, + catch_cnd, + check_bool, + check_data_frame, + check_dots_empty, + check_dots_empty0, + check_dots_unnamed, + check_dots_used, + check_exclusive, + check_installed, + check_number_decimal, + check_number_whole, + check_required, + check_string, + child_env, + chr, + chr_unserialise_unicode, + cnd, + cnd_body, + cnd_entrace, + cnd_footer, + cnd_header, + cnd_inherits, + cnd_message, + cnd_muffle, + cnd_signal, + cnd_type, + coerce_class, + coerce_type, + cpl, + ctxt_frame, + current_call, + current_env, + current_fn, + data_sym, + data_syms, + dbl, + done, + dots_list, + dots_n, + dots_splice, + dots_values, + duplicate, + empty_env, + enexpr, + enexprs, + englue, + enquo, + enquo0, + enquos, + enquos0, + ensym, + ensyms, + entrace, + env, + env_bind, + env_bind_active, + env_bind_lazy, + env_binding_are_active, + env_binding_are_lazy, + env_binding_are_locked, + env_binding_lock, + env_binding_unlock, + env_browse, + env_bury, + env_cache, + env_clone, + env_coalesce, + env_depth, + env_get, + env_get_list, + env_has, + env_inherits, + env_is_browsed, + env_is_locked, + env_is_user_facing, + env_label, + env_length, + env_lock, + env_name, + env_names, + env_parent, + env_parents, + env_poke, + env_poke_parent, + env_print, + env_tail, + env_unbind, + env_unlock, + error_call, + error_cnd, + eval_bare, + eval_tidy, + exec, + exiting, + expr, + expr_deparse, + expr_interp, + expr_label, + expr_name, + expr_print, + expr_text, + exprs, + exprs_auto_name, + f_env, + f_label, + f_lhs, + f_name, + f_rhs, + f_text, + ffi_standalone_check_number_1.0.7, + ffi_standalone_is_bool_1.0.7, + flatten, + flatten_chr, + flatten_cpl, + flatten_dbl, + flatten_if, + flatten_int, + flatten_lgl, + flatten_raw, + fn_body, + fn_env, + fn_fmls, + fn_fmls_names, + fn_fmls_syms, + format_error_bullets, + format_error_call, + frame_call, + frame_fn, + friendly_type, + get_env, + get_expr, + global_entrace, + global_env, + global_frame, + global_handle, + global_prompt_install, + has_length, + has_name, + hash, + hash_file, + have_name, + inform, + inherits_all, + inherits_any, + inherits_only, + inject, + int, + interrupt, + invoke, + is_atomic, + is_attached, + is_bare_atomic, + is_bare_bytes, + is_bare_character, + is_bare_complex, + is_bare_double, + is_bare_environment, + is_bare_formula, + is_bare_integer, + is_bare_integerish, + is_bare_list, + is_bare_logical, + is_bare_numeric, + is_bare_raw, + is_bare_string, + is_bare_vector, + is_bool, + is_box, + is_bytes, + is_call, + is_call_simple, + is_callable, + is_character, + is_chr_na, + is_closure, + is_complex, + is_condition, + is_copyable, + is_cpl_na, + is_dbl_na, + is_dictionaryish, + is_done_box, + is_double, + is_empty, + is_environment, + is_error, + is_expression, + is_false, + is_formula, + is_function, + is_installed, + is_int_na, + is_integer, + is_integerish, + is_interactive, + is_lambda, + is_lang, + is_lgl_na, + is_list, + is_logical, + is_message, + is_missing, + is_na, + is_named, + is_named2, + is_namespace, + is_node, + is_node_list, + is_null, + is_pairlist, + is_primitive, + is_primitive_eager, + is_primitive_lazy, + is_quosure, + is_quosures, + is_raw, + is_reference, + is_scalar_atomic, + is_scalar_bytes, + is_scalar_character, + is_scalar_complex, + is_scalar_double, + is_scalar_integer, + is_scalar_integerish, + is_scalar_list, + is_scalar_logical, + is_scalar_raw, + is_scalar_vector, + is_scoped, + is_spliced, + is_spliced_bare, + is_string, + is_symbol, + is_symbolic, + is_syntactic_literal, + is_true, + is_vector, + is_warning, + is_weakref, + is_zap, + lang, + last_error, + last_messages, + last_trace, + last_warnings, + lgl, + list2, + ll, + local_bindings, + local_error_call, + local_interactive, + local_options, + local_use_cli, + locally, + maybe_missing, + message_cnd, + missing_arg, + na_chr, + na_cpl, + na_dbl, + na_int, + na_lgl, + names2, + names_inform_repair, + new_box, + new_call, + new_character, + new_complex, + new_data_mask, + new_double, + new_environment, + new_formula, + new_function, + new_integer, + new_list, + new_logical, + new_node, + new_quosure, + new_quosures, + new_raw, + new_weakref, + node_caar, + node_cadr, + node_car, + node_cdar, + node_cddr, + node_cdr, + node_poke_caar, + node_poke_cadr, + node_poke_car, + node_poke_cdar, + node_poke_cddr, + node_poke_cdr, + node_poke_tag, + node_tag, + ns_env, + ns_env_name, + ns_imports_env, + ns_registry_env, + obj_address, + on_load, + on_package_load, + pairlist2, + parse_bytes, + parse_expr, + parse_exprs, + parse_quo, + parse_quos, + peek_option, + peek_options, + pkg_env, + pkg_env_name, + prim_name, + push_options, + qq_show, + quo, + quo_expr, + quo_get_env, + quo_get_expr, + quo_is_call, + quo_is_missing, + quo_is_null, + quo_is_symbol, + quo_is_symbolic, + quo_label, + quo_name, + quo_set_env, + quo_set_expr, + quo_squash, + quo_text, + quos, + quos_auto_name, + raw_deparse_str, + rep_along, + rep_named, + reset_message_verbosity, + reset_warning_verbosity, + return_from, + run_on_load, + scoped_bindings, + scoped_env, + scoped_interactive, + scoped_options, + search_env, + search_envs, + seq2, + seq2_along, + set_attrs, + set_env, + set_expr, + set_names, + signal, + splice, + squash, + squash_chr, + squash_cpl, + squash_dbl, + squash_if, + squash_int, + squash_lgl, + squash_raw, + stop_input_type, + string, + switch_class, + switch_type, + sym, + syms, + trace_back, + trace_length, + try_fetch, + type_of, + unbox, + vec_poke_n, + vec_poke_range, + warn, + warning_cnd, + with_bindings, + with_env, + with_handlers, + with_interactive, + with_options, + wref_key, + wref_value, + zap, + zap_srcref +) importFrom(stats,setNames) importFrom(utils, URLdecode, diff --git a/NEWS.md b/NEWS.md index 53d76d1c..8e8c34ee 100644 --- a/NEWS.md +++ b/NEWS.md @@ -7,6 +7,7 @@ * `Config/roxygen2/` flag fields in `DESCRIPTION` (like `markdown`) are now parsed case-insensitively, so `true` and `True` work as well as `TRUE`, and an invalid value gives a clear error (#1875). * `@section` titles can now contain code that includes a colon (#1878). * The automatic usage for a data object that is conditional on the `LazyData` option in the `DESCRIPTION` (see below) now correctly detects all ways to specify a true value, e.g. also `yes`, `Yes` or `True` (@jranke, #1881). +* `@import` now expands to an explicit `importFrom()` directive listing every symbol the package currently exports, instead of generating an `import()` directive. This freezes the set of imported symbols at document-time, so a package that later adds exports can't introduce new conflicts into your namespace. If two `@import` directives conflict with each other, roxygen2 now errors at document-time and asks you the maintainer to resolve the conflict with `except`, e.g. `@import dplyr, except = filter`. * `@import` now inserts the directive as is into `NAMESPACE` when it contains a comma, making it possible to use other forms like `@import rlang, except = ":="`. * `@importFrom` now generates a single multiline `importFrom()` directive per package instead of one directive per symbol. This fixes a performance issue with `loadNamespace()` for packages that import many symbols. * `@importFrom`, `@importClassesFrom`, and `@importMethodsFrom` now accept multi-line input, restoring the ability to spread imports across multiple lines for readability; continuation lines must use a hanging indent, so the first flush or blank line ends the tag and content after it (e.g. from a forgotten `@examples`) is no longer silently absorbed into the namespace (#1890). diff --git a/R/namespace.R b/R/namespace.R index e68b962e..f4004006 100644 --- a/R/namespace.R +++ b/R/namespace.R @@ -32,7 +32,12 @@ #' #' # This results in the following lines in `NAMESPACE`: #' # importFrom(magrittr,"%>%") -#' # import(rlang) +#' # importFrom(rlang, ) +#' # +#' # `@import` expands to an `importFrom()` expression so that +#' # the imported set is frozen at document-time. This prevents +#' # load-time conflicts when an updated package now exports new +#' # symbols that happen to conflict with other imported symbols. namespace_roclet <- function() { roclet("namespace") } @@ -148,7 +153,16 @@ block_directives <- function(blocks, env) { directives <- map(blocks, function(block) { map(block$tags, roxy_tag_ns, block = block, env = env) }) - compact(list_c(directives)) + compact(splice_directives(list_c(directives))) +} + +# A `roxy_tag_ns()` method usually returns a single directive, but `@import` +# expands to one `importFrom()` per package, so it returns a bare list of +# directives. `import_from()` objects are themselves lists, so we only splice +# unclassed lists, which leaves those objects (and character directives) +# untouched. +splice_directives <- function(directives) { + list_c(map(directives, \(x) if (is_bare_list(x)) x else list(x))) } # `roxy_tag_ns()` returns either a rendered directive (a character vector) or, @@ -158,19 +172,82 @@ block_directives <- function(blocks, env) { # `importFrom()` directives. ns_format <- function(directives) { is_import <- map_lgl(directives, \(x) inherits(x, "import_from")) + imports <- directives[is_import] + check_import_conflicts(imports) text <- unique(as.character(unlist( directives[!is_import], use.names = FALSE ))) - import_from <- merge_import_from(directives[is_import]) + import_from <- merge_import_from(imports) lines <- c(text, import_from) lines[order_c(lines)] } -import_from <- function(package, funs) { - structure(list(package = package, funs = funs), class = "import_from") +import_from <- function(package, funs, expanded = FALSE) { + structure( + list(package = package, funs = funs, expanded = expanded), + class = "import_from" + ) +} + +# Conflicting `@import` directives are detected at document-time. An error is +# thrown so the user has to resolve the conflict to build the package. +check_import_conflicts <- function(imports) { + syms <- map(imports, \(x) strip_quotes(x$funs)) + imported <- data.frame( + sym = unlist(syms, use.names = FALSE) %||% character(), + pkg = rep(map_chr(imports, \(x) x$package), lengths(syms)), + expanded = rep(map_lgl(imports, \(x) x$expanded %||% FALSE), lengths(syms)) + ) + + # A symbol conflicts when it's imported from more than one package and at + # least one of those imports came from an expanded `@import`. + by_sym <- split(imported, imported$sym) + conflicts <- keep( + by_sym, + \(x) length(unique(x$pkg)) > 1 && any(x$expanded) + ) + + # Re-exports aren't real conflicts: when several packages export the same + # object (e.g. `%>%`), importing it from more than one is harmless. + conflicts <- discard(conflicts, \(x) is_reexport(x$sym[[1]], unique(x$pkg))) + + if (length(conflicts) == 0) { + return(invisible()) + } + + bullets <- map_chr(conflicts, function(x) { + where <- sort_c(unique(x$pkg)) + cli::format_inline("{.code {x$sym[[1]]}} is exported by {.package {where}}") + }) + + conflict <- conflicts[[1]] + example_sym <- conflict$sym[[1]] + example_pkg <- conflict$pkg[conflict$expanded][[1]] + + cli::cli_abort(c( + "Found {length(conflicts)} conflicting import{?s} from {.code @import}.", + set_names(bullets, rep("*", length(bullets))), + i = "Exclude unwanted symbols with e.g. {.code @import {example_pkg}, except = {example_sym}}." + )) +} + +# TRUE when every package exports the identical object for `sym`, so importing +# it from more than one of them doesn't actually clash. Returns FALSE if any +# package's value can't be read (e.g. it isn't installed), since we then can't +# prove they match and would rather flag a false conflict than miss a real one. +is_reexport <- function(sym, pkgs) { + values <- map(pkgs, function(pkg) { + tryCatch(getExportedValue(pkg, sym), error = function(cnd) NULL) + }) + # A failed lookup comes back NULL, meaning we can't prove a match + if (some(values, is.null)) { + return(FALSE) + } + + every(values[-1], \(x) identical(x, values[[1]])) } # Merge the `import_from()` directives by package into one `importFrom()` each. @@ -313,7 +390,7 @@ roxy_tag_parse.roxy_tag_import <- function(x) { #' @export roxy_tag_ns.roxy_tag_import <- function(x, block, env) { ns_verbatim("import", x$val) %||% - one_per_line_ignore_current("import", x$val) + expand_import(x$val) } #' @export @@ -446,16 +523,36 @@ repeat_first <- function(name, x) { paste0(name, "(", auto_quote(x[1]), ",", auto_quote(x[-1]), ")") } -one_per_line_ignore_current <- function(name, x) { +# `import(pkg)` imports everything `pkg` exports. We expand it to an explicit +# `importFrom(pkg, ...)` over every current export so the imported set is frozen +# at document-time. This way a package that later adds new exports doesn't inject +# new conflicts into the namespace at load-time. +expand_import <- function(pkgs) { current <- peek_roxygen_pkg() - # Ignore any occurrence of `current` inside `x` + # Ignore any occurrence of `current` inside `pkgs` if (is_string(current)) { - x <- x[x != current] + pkgs <- pkgs[pkgs != current] } - one_per_line(name, x) + map(pkgs, expand_import_pkg) } + +# Falls back to an unexpanded `import(pkg)` when `pkg` isn't installed, since we +# can't read its exports without it. +expand_import_pkg <- function(pkg) { + if (!requireNamespace(pkg, quietly = TRUE)) { + return(one_per_line("import", pkg)) + } + + exports <- getNamespaceExports(pkg) + if (length(exports) == 0) { + one_per_line("import", pkg) + } else { + import_from(pkg, exports, expanded = TRUE) + } +} + repeat_first_ignore_current <- function(name, x) { current <- peek_roxygen_pkg() diff --git a/man/namespace_roclet.Rd b/man/namespace_roclet.Rd index 90bc055b..d3a1e528 100644 --- a/man/namespace_roclet.Rd +++ b/man/namespace_roclet.Rd @@ -36,7 +36,12 @@ NULL # This results in the following lines in `NAMESPACE`: # importFrom(magrittr,"\%>\%") -# import(rlang) +# importFrom(rlang, ) +# +# `@import` expands to an `importFrom()` expression so that +# the imported set is frozen at document-time. This prevents +# load-time conflicts when an updated package now exports new +# symbols that happen to conflict with other imported symbols. } \seealso{ \link{tags-namespace} for tags that generate \code{NAMESPACE} directives. diff --git a/tests/testthat/_snaps/namespace.md b/tests/testthat/_snaps/namespace.md index d649bfd1..808d68f0 100644 --- a/tests/testthat/_snaps/namespace.md +++ b/tests/testthat/_snaps/namespace.md @@ -59,6 +59,27 @@ x :2: @importFrom must use a hanging indent to span multiple lines. i Continuation lines must be indented; did you forget a tag like `@examples`? +# expanded @import conflicting with another package errors + + Code + check_import_conflicts(imports) + Condition + Error in `check_import_conflicts()`: + ! Found 1 conflicting import from `@import`. + * `foo` is exported by pkgA and pkgB + i Exclude unwanted symbols with e.g. `@import pkgA, except = foo`. + +# each conflicting symbol is reported with its own packages + + Code + check_import_conflicts(imports) + Condition + Error in `check_import_conflicts()`: + ! Found 2 conflicting imports from `@import`. + * `baz` is exported by pkgC and pkgD + * `foo` is exported by pkgA and pkgB + i Exclude unwanted symbols with e.g. `@import pkgC, except = baz`. + # can regenerate NAMESPACE even if its broken Code diff --git a/tests/testthat/test-namespace.R b/tests/testthat/test-namespace.R index 5dca7808..75b0df2e 100644 --- a/tests/testthat/test-namespace.R +++ b/tests/testthat/test-namespace.R @@ -434,6 +434,128 @@ test_that("import doesn't quote if comma present", { expect_equal(sort(out), "import(rlang, except = ':=')") }) +test_that("@import expands to importFrom over all exports", { + out <- roc_proc_text( + namespace_roclet(), + " + #' @import utils + NULL" + ) + + syms <- sort_c(unique(auto_quote(getNamespaceExports("utils")))) + expect_equal(out, format_import_from("utils", syms)) +}) + +test_that("@import falls back to import() when package isn't installed", { + out <- roc_proc_text( + namespace_roclet(), + " + #' @import notarealpkgxyz + NULL" + ) + + expect_equal(out, "import(notarealpkgxyz)") +}) + +test_that("@import expands each package independently", { + out <- roc_proc_text( + namespace_roclet(), + " + #' @import utils stats notarealpkgxyz + NULL" + ) + + # Each installed package expands to its own importFrom() directive, and the + # uninstalled one falls back to import(). + expect_true(any(startsWith(out, "importFrom(utils"))) + expect_true(any(startsWith(out, "importFrom(stats"))) + expect_true("import(notarealpkgxyz)" %in% out) +}) + +test_that("expanded @import conflicting with another package errors", { + imports <- list( + import_from("pkgA", c("foo", "bar"), expanded = TRUE), + import_from("pkgB", "foo") + ) + expect_snapshot(check_import_conflicts(imports), error = TRUE) +}) + +test_that("two expanded @import sharing a symbol errors", { + imports <- list( + import_from("pkgA", "foo", expanded = TRUE), + import_from("pkgB", "foo", expanded = TRUE) + ) + expect_error(check_import_conflicts(imports), "conflicting import") +}) + +test_that("@import conflict detection ignores overlaps without expansion", { + imports <- list( + import_from("pkgA", "foo"), + import_from("pkgB", "foo") + ) + expect_no_error(check_import_conflicts(imports)) +}) + +test_that("@import conflict detection ignores same-package duplicates", { + imports <- list( + import_from("pkgA", "foo", expanded = TRUE), + import_from("pkgA", "foo") + ) + expect_no_error(check_import_conflicts(imports)) +}) + +test_that("@import conflict detection normalises quoted symbols", { + # Expanded imports arrive unquoted from getNamespaceExports(), but an explicit + # @importFrom of a non-syntactic name can be quoted; they must still match. + imports <- list( + import_from("pkgA", "%||%", expanded = TRUE), + import_from("pkgB", "`%||%`") + ) + expect_error(check_import_conflicts(imports), "conflicting import") +}) + +test_that("each conflicting symbol is reported with its own packages", { + # `foo` clashes across one pair of packages, `baz` across a disjoint pair. + imports <- list( + import_from("pkgA", "foo", expanded = TRUE), + import_from("pkgB", "foo"), + import_from("pkgC", "baz", expanded = TRUE), + import_from("pkgD", "baz") + ) + expect_snapshot(check_import_conflicts(imports), error = TRUE) +}) + +test_that("expanding @import errors on a conflict with another import", { + # `stats` expands to include `sd`, which also comes in via the @importFrom. + block <- " + #' @import stats + #' @importFrom anotherpkg sd + NULL" + expect_error( + roc_proc_text(namespace_roclet(), block), + "conflicting import" + ) +}) + +test_that("expanding @import doesn't conflict on re-exports", { + skip_if_not_installed("purrr") + + # `rlang` and `purrr` both re-export the identical `is_logical`, so importing + # it from purrr alongside the expanded rlang import is not a real conflict. + block <- " + #' @import rlang + #' @importFrom purrr is_logical + NULL" + expect_no_error(roc_proc_text(namespace_roclet(), block)) +}) + +test_that("is_reexport compares the exported objects", { + # Same object from each package (here trivially, the same package twice). + expect_true(is_reexport("abort", c("rlang", "rlang"))) + # Can't read a package's value, so we can't prove the objects match. + expect_false(is_reexport("abort", c("rlang", "anotherpkg"))) +}) + test_that("useDynLib imports only selected functions", { out <- roc_proc_text( namespace_roclet(), @@ -513,6 +635,10 @@ test_that("rawNamespace inserted unchanged", { }) test_that("rawNamespace does not break idempotency", { + # `@import testImports` expands using this package's fixed exports. + pkgload::load_all(test_path("testImports"), quiet = TRUE) + withr::defer(pkgload::unload("testImports")) + test_pkg <- local_package_copy(test_path("testRawNamespace")) NAMESPACE <- file.path(test_pkg, "NAMESPACE") diff --git a/tests/testthat/testImports/DESCRIPTION b/tests/testthat/testImports/DESCRIPTION new file mode 100644 index 00000000..fb758574 --- /dev/null +++ b/tests/testthat/testImports/DESCRIPTION @@ -0,0 +1,7 @@ +Package: testImports +Title: Stable export set for @import expansion tests +License: GPL-2 +Description: Exports a fixed set of objects so that @import expansion is + version-independent in tests. +Encoding: UTF-8 +Version: 0.1 diff --git a/tests/testthat/testImports/NAMESPACE b/tests/testthat/testImports/NAMESPACE new file mode 100644 index 00000000..ecb331d3 --- /dev/null +++ b/tests/testthat/testImports/NAMESPACE @@ -0,0 +1,3 @@ +export(import_a) +export(import_b) +export(import_c) diff --git a/tests/testthat/testImports/R/exports.R b/tests/testthat/testImports/R/exports.R new file mode 100644 index 00000000..be0f0891 --- /dev/null +++ b/tests/testthat/testImports/R/exports.R @@ -0,0 +1,3 @@ +import_a <- function() NULL +import_b <- function() NULL +import_c <- function() NULL diff --git a/tests/testthat/testRawNamespace/NAMESPACE b/tests/testthat/testRawNamespace/NAMESPACE index 74ea7af2..9cfc30d1 100644 --- a/tests/testthat/testRawNamespace/NAMESPACE +++ b/tests/testthat/testRawNamespace/NAMESPACE @@ -6,5 +6,8 @@ if (TRUE) { } else { import(methods) } -import(graphics) -import(utils) +importFrom(testImports, + import_a, + import_b, + import_c +) diff --git a/tests/testthat/testRawNamespace/R/a.R b/tests/testthat/testRawNamespace/R/a.R index a44b2fe0..80c485fc 100644 --- a/tests/testthat/testRawNamespace/R/a.R +++ b/tests/testthat/testRawNamespace/R/a.R @@ -1,9 +1,8 @@ -#' @import graphics +#' @import testImports #' @rawNamespace #' if (TRUE) { #' import(grDevices) #' } else { #' import(methods) #' } -#' @import utils NULL diff --git a/vignettes/namespace.Rmd b/vignettes/namespace.Rmd index f34516b5..0412d5f4 100644 --- a/vignettes/namespace.Rmd +++ b/vignettes/namespace.Rmd @@ -97,9 +97,10 @@ NULL Note the use of `NULL` here: you must provide something for roxygen2 to document, so we use `NULL` as placeholder. -It is possible, but not generally recommended to import all functions from a package with `@import package`. -This is risky if you import functions from more than one package, because while it might be ok today, in the future the packages might end up with a function having the same name, and your users will get a warning every time your package is loaded. -Any `@import` specification containing a comma, e.g. `@import rlang, except = ":="` will be inserted as is into the the NAMESPACE, e.g. `import(rlang, except = ":=")`. +It is possible to import all functions from a package with `@import package`. +Be aware that if you bulk-import more than one package, you might need to maintain a list of exceptions in case of conflicts. +Conflicts never arise in a user-visible way at load-time because roxygen builds the imported set at document-time. +When it detects a conflict, roxygen fails with an error that recommends resolving the conflict with an `except =` clause, e.g. `@import package, except = sym`. ### S3 From 6b01e77f6f2611887e90b0a19403ef2bba9982b0 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 3 Jul 2026 09:20:06 +0200 Subject: [PATCH 2/4] Move expansion feature to experimental `@importAllFrom` tag --- NAMESPACE | 2 + NEWS.md | 2 +- R/namespace.R | 145 ++++++++++++++++------- R/roxygen2-package.R | 2 +- inst/roxygen2-tags.yml | 10 +- man/namespace_roclet.Rd | 25 +++- man/tags-namespace.Rd | 5 +- tests/testthat/_snaps/namespace.md | 17 ++- tests/testthat/test-namespace.R | 158 ++++++++++++++++++++++---- tests/testthat/testRawNamespace/R/a.R | 2 +- vignettes/namespace.Rmd | 13 ++- 11 files changed, 300 insertions(+), 81 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index d1d1d629..f853f112 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -129,6 +129,7 @@ S3method(roxy_tag_ns,roxy_tag_exportMethod) S3method(roxy_tag_ns,roxy_tag_exportPattern) S3method(roxy_tag_ns,roxy_tag_exportS3Method) S3method(roxy_tag_ns,roxy_tag_import) +S3method(roxy_tag_ns,roxy_tag_importAllFrom) S3method(roxy_tag_ns,roxy_tag_importClassesFrom) S3method(roxy_tag_ns,roxy_tag_importFrom) S3method(roxy_tag_ns,roxy_tag_importMethodsFrom) @@ -160,6 +161,7 @@ S3method(roxy_tag_parse,roxy_tag_family) S3method(roxy_tag_parse,roxy_tag_field) S3method(roxy_tag_parse,roxy_tag_format) S3method(roxy_tag_parse,roxy_tag_import) +S3method(roxy_tag_parse,roxy_tag_importAllFrom) S3method(roxy_tag_parse,roxy_tag_importClassesFrom) S3method(roxy_tag_parse,roxy_tag_importFrom) S3method(roxy_tag_parse,roxy_tag_importMethodsFrom) diff --git a/NEWS.md b/NEWS.md index 8e8c34ee..8e49d30c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -7,8 +7,8 @@ * `Config/roxygen2/` flag fields in `DESCRIPTION` (like `markdown`) are now parsed case-insensitively, so `true` and `True` work as well as `TRUE`, and an invalid value gives a clear error (#1875). * `@section` titles can now contain code that includes a colon (#1878). * The automatic usage for a data object that is conditional on the `LazyData` option in the `DESCRIPTION` (see below) now correctly detects all ways to specify a true value, e.g. also `yes`, `Yes` or `True` (@jranke, #1881). -* `@import` now expands to an explicit `importFrom()` directive listing every symbol the package currently exports, instead of generating an `import()` directive. This freezes the set of imported symbols at document-time, so a package that later adds exports can't introduce new conflicts into your namespace. If two `@import` directives conflict with each other, roxygen2 now errors at document-time and asks you the maintainer to resolve the conflict with `except`, e.g. `@import dplyr, except = filter`. * `@import` now inserts the directive as is into `NAMESPACE` when it contains a comma, making it possible to use other forms like `@import rlang, except = ":="`. +* New `@importAllFrom` tag expands to an explicit `importFrom()` directive listing every symbol the package currently exports, instead of generating an `import()` directive like `@import` does. This freezes the set of imported symbols at document-time, so a package that later adds exports can't introduce new conflicts into your namespace for your users. If two `@importAllFrom` directives conflict with each other, roxygen2 errors at document-time and asks you the maintainer to resolve the conflict by excluding the unwanted symbol with a `-` prefix, e.g. `@importAllFrom dplyr -filter`. * `@importFrom` now generates a single multiline `importFrom()` directive per package instead of one directive per symbol. This fixes a performance issue with `loadNamespace()` for packages that import many symbols. * `@importFrom`, `@importClassesFrom`, and `@importMethodsFrom` now accept multi-line input, restoring the ability to spread imports across multiple lines for readability; continuation lines must use a hanging indent, so the first flush or blank line ends the tag and content after it (e.g. from a forgotten `@examples`) is no longer silently absorbed into the namespace (#1890). diff --git a/R/namespace.R b/R/namespace.R index f4004006..21eb5404 100644 --- a/R/namespace.R +++ b/R/namespace.R @@ -32,12 +32,25 @@ #' #' # This results in the following lines in `NAMESPACE`: #' # importFrom(magrittr,"%>%") -#' # importFrom(rlang, ) -#' # -#' # `@import` expands to an `importFrom()` expression so that -#' # the imported set is frozen at document-time. This prevents -#' # load-time conflicts when an updated package now exports new -#' # symbols that happen to conflict with other imported symbols. +#' # import(rlang) +#' +#' # There is a new experimental way to bulk-import a package: +#' #' @importAllFrom rlang +#' +#' # This results in the following lines: +#' # importFrom(rlang, +#' # "!!!", +#' # "!!", +#' # "%&&%", +#' # ... +#' # ) +#' +#' # The exported objects are all explicitly imported one by one, which prevents +#' # load-time issues for the users of your package when an update creates an +#' # import conflict. The only time a conflict can arise is when you regenerate +#' # your namespace file, at which point you can manually resolve any conflicts by +#' # excluding a symbol with a `-` prefix: +#' #' @importAllFrom rlang -list2 -`:=` namespace_roclet <- function() { roclet("namespace") } @@ -118,7 +131,12 @@ namespace_imports_blocks <- function(srcref) { comment_refs <- comments(srcref) tokens <- lapply(comment_refs, tokenise_ref) - import_tags <- c(import_directives, "rawNamespace") + # `import_directives` contains the tags that map to literal `NAMESPACE` import + # calls. Two more tags need to be treated here: `importAllFrom` expands to + # multiple `importFrom()` directives rather than appearing literally, and + # `rawNamespace` inserts verbatim text that can itself contain import + # directives. + import_tags <- c(import_directives, "importAllFrom", "rawNamespace") tokens_filtered <- lapply(tokens, function(tokens) { tokens[map_lgl(tokens, \(x) x$tag %in% import_tags)] }) @@ -153,16 +171,7 @@ block_directives <- function(blocks, env) { directives <- map(blocks, function(block) { map(block$tags, roxy_tag_ns, block = block, env = env) }) - compact(splice_directives(list_c(directives))) -} - -# A `roxy_tag_ns()` method usually returns a single directive, but `@import` -# expands to one `importFrom()` per package, so it returns a bare list of -# directives. `import_from()` objects are themselves lists, so we only splice -# unclassed lists, which leaves those objects (and character directives) -# untouched. -splice_directives <- function(directives) { - list_c(map(directives, \(x) if (is_bare_list(x)) x else list(x))) + compact(list_c(directives)) } # `roxy_tag_ns()` returns either a rendered directive (a character vector) or, @@ -192,8 +201,8 @@ import_from <- function(package, funs, expanded = FALSE) { ) } -# Conflicting `@import` directives are detected at document-time. An error is -# thrown so the user has to resolve the conflict to build the package. +# Conflicting `@importAllFrom` directives are detected at document-time. An +# error is thrown so the user has to resolve the conflict to build the package. check_import_conflicts <- function(imports) { syms <- map(imports, \(x) strip_quotes(x$funs)) imported <- data.frame( @@ -203,7 +212,7 @@ check_import_conflicts <- function(imports) { ) # A symbol conflicts when it's imported from more than one package and at - # least one of those imports came from an expanded `@import`. + # least one of those imports came from an `@importAllFrom`. by_sym <- split(imported, imported$sym) conflicts <- keep( by_sym, @@ -224,13 +233,13 @@ check_import_conflicts <- function(imports) { }) conflict <- conflicts[[1]] - example_sym <- conflict$sym[[1]] + example_sym <- auto_quote(conflict$sym[[1]]) example_pkg <- conflict$pkg[conflict$expanded][[1]] cli::cli_abort(c( - "Found {length(conflicts)} conflicting import{?s} from {.code @import}.", + "Found {length(conflicts)} conflicting import{?s} from {.code @importAllFrom}.", set_names(bullets, rep("*", length(bullets))), - i = "Exclude unwanted symbols with e.g. {.code @import {example_pkg}, except = {example_sym}}." + i = "Exclude unwanted symbols with e.g. {.code @importAllFrom {example_pkg} -{example_sym}}." )) } @@ -390,7 +399,16 @@ roxy_tag_parse.roxy_tag_import <- function(x) { #' @export roxy_tag_ns.roxy_tag_import <- function(x, block, env) { ns_verbatim("import", x$val) %||% - expand_import(x$val) + one_per_line_ignore_current("import", x$val) +} + +#' @export +roxy_tag_parse.roxy_tag_importAllFrom <- function(x) { + tag_words(x, min = 1) +} +#' @export +roxy_tag_ns.roxy_tag_importAllFrom <- function(x, block, env) { + expand_import(x) } #' @export @@ -523,36 +541,87 @@ repeat_first <- function(name, x) { paste0(name, "(", auto_quote(x[1]), ",", auto_quote(x[-1]), ")") } -# `import(pkg)` imports everything `pkg` exports. We expand it to an explicit -# `importFrom(pkg, ...)` over every current export so the imported set is frozen -# at document-time. This way a package that later adds new exports doesn't inject -# new conflicts into the namespace at load-time. -expand_import <- function(pkgs) { +one_per_line_ignore_current <- function(name, x) { current <- peek_roxygen_pkg() - # Ignore any occurrence of `current` inside `pkgs` + # Ignore any occurrence of `current` inside `x` if (is_string(current)) { - pkgs <- pkgs[pkgs != current] + x <- x[x != current] } - map(pkgs, expand_import_pkg) + one_per_line(name, x) } -# Falls back to an unexpanded `import(pkg)` when `pkg` isn't installed, since we -# can't read its exports without it. -expand_import_pkg <- function(pkg) { +# `@importAllFrom pkg` expands to explicit `importFrom(pkg, ...)` over every +# object currently exported by `pkg`. This early expansion pins the set of +# imports at document-time and prevents user-visible conflicts at load-time when +# a package update introduces a conflict with other imported symbols. +expand_import <- function(x) { + current <- peek_roxygen_pkg() + spec <- parse_import_all_from(x$val) + pkg <- spec$pkg + excluded <- spec$excluded + + # Ignore an `@importAllFrom` for the package being documented + if (identical(current, pkg)) { + return(character()) + } + if (!requireNamespace(pkg, quietly = TRUE)) { - return(one_per_line("import", pkg)) + cli::cli_abort(c( + "Can't expand {.code @importAllFrom {pkg}}.", + x = "{.package {pkg}} must be installed to enumerate its exports." + )) } - exports <- getNamespaceExports(pkg) + all_exports <- getNamespaceExports(pkg) + + # Warn on exclusions that don't match an export, so a typo like `-improt_b` + # isn't silently dropped and left to resurface as the conflict it was meant to + # resolve. + unknown <- setdiff(excluded, all_exports) + if (length(unknown) > 0) { + warn_roxy_tag( + x, + "Ignoring unknown {cli::qty(length(unknown))} exclusion{?s} for {.package {pkg}}: {.code {unknown}}" + ) + } + + exports <- setdiff(all_exports, excluded) if (length(exports) == 0) { - one_per_line("import", pkg) + # Nothing left to import, either because `pkg` exports nothing or because + # the exclusions removed every export. Import nothing in this case instead + # of falling back to `import(pkg)`. + character() } else { import_from(pkg, exports, expanded = TRUE) } } +# Splits an `@importAllFrom` value into the package to expand and the symbols to +# leave out. An exclusion is a word with a `-` prefix, e.g. `-abort`. Exclusions +# may be quoted: `-"-.Date"`, ``-`-.Date` ``, or `-'-.Date'`. +parse_import_all_from <- function(vals) { + is_excluded <- startsWith(vals, "-") + + pkg <- vals[!is_excluded] + if (length(pkg) == 0) { + cli::cli_abort("{.code @importAllFrom} needs a package to import from.") + } + if (length(pkg) > 1) { + cli::cli_abort( + "Can't import multiple packages with {.code @importAllFrom}." + ) + } + + excluded <- strip_quotes(sub("^-", "", vals[is_excluded])) + + list( + pkg = pkg, + excluded = excluded + ) +} + repeat_first_ignore_current <- function(name, x) { current <- peek_roxygen_pkg() diff --git a/R/roxygen2-package.R b/R/roxygen2-package.R index 1d6aa626..52caf161 100644 --- a/R/roxygen2-package.R +++ b/R/roxygen2-package.R @@ -1,6 +1,6 @@ #' @useDynLib roxygen2, .registration=TRUE #' @keywords internal -#' @import rlang +#' @importAllFrom rlang "_PACKAGE" ## usethis namespace: start diff --git a/inst/roxygen2-tags.yml b/inst/roxygen2-tags.yml index 16a1f719..7949d81b 100644 --- a/inst/roxygen2-tags.yml +++ b/inst/roxygen2-tags.yml @@ -137,7 +137,7 @@ description: > Export an S3 method. Only needed when the method is for a generic from a suggested package. Use `@exportS3Method NULL` to suppress the missing - export warning when it's a false positive, e.g. you're registering the + export warning when it's a false positive, e.g. you're registering the method some other way. template: ' ${1:package}::${2:generic}' vignette: namespace @@ -172,6 +172,14 @@ template: ' ${1:package}' vignette: namespace +- name: importAllFrom + description: > + Import all functions from a package as explicit `importFrom()` calls, so + the imported set is pinned at document-time instead of changing whenever + the package updates. This avoids user-visible conflict warnings. + template: ' ${1:package}' + vignette: namespace + - name: importClassesFrom description: > Import S4 classes from another package. diff --git a/man/namespace_roclet.Rd b/man/namespace_roclet.Rd index d3a1e528..ebe7cd20 100644 --- a/man/namespace_roclet.Rd +++ b/man/namespace_roclet.Rd @@ -36,12 +36,25 @@ NULL # This results in the following lines in `NAMESPACE`: # importFrom(magrittr,"\%>\%") -# importFrom(rlang, ) -# -# `@import` expands to an `importFrom()` expression so that -# the imported set is frozen at document-time. This prevents -# load-time conflicts when an updated package now exports new -# symbols that happen to conflict with other imported symbols. +# import(rlang) + +# There is a new experimental way to bulk-import a package: +#' @importAllFrom rlang + +# This results in the following lines: +# importFrom(rlang, +# "!!!", +# "!!", +# "\%&&\%", +# ... +# ) + +# The exported objects are all explicitly imported one by one, which prevents +# load-time issues for the users of your package when an update creates an +# import conflict. The only time a conflict can arise is when you regenerate +# your namespace file, at which point you can manually resolve any conflicts by +# excluding a symbol with a `-` prefix: +#' @importAllFrom rlang -list2 -`:=` } \seealso{ \link{tags-namespace} for tags that generate \code{NAMESPACE} directives. diff --git a/man/tags-namespace.Rd b/man/tags-namespace.Rd index 54bd0bf6..f95a0b37 100644 --- a/man/tags-namespace.Rd +++ b/man/tags-namespace.Rd @@ -9,6 +9,7 @@ \alias{@exportPattern} \alias{@exportS3Method} \alias{@import} +\alias{@importAllFrom} \alias{@importClassesFrom} \alias{@importFrom} \alias{@importMethodsFrom} @@ -23,6 +24,7 @@ #' @exportPattern ${1:pattern} #' @exportS3Method ${1:package}::${2:generic} #' @import ${1:package} +#' @importAllFrom ${1:package} #' @importClassesFrom ${1:package} ${2:class} #' @importFrom ${1:package} ${2:function} #' @importMethodsFrom ${1:package} ${2:generic} @@ -35,7 +37,7 @@ Learn the full details in \code{vignette('namespace')}. Key tags: \itemize{ \item \verb{@export}: Export this function, method, generic, or class so it's available outside of the package. -\item \verb{@exportS3Method $\{1:package\}::$\{2:generic\}}: Export an S3 method. Only needed when the method is for a generic from a suggested package. Use \verb{@exportS3Method NULL} to suppress the missing export warning when it's a false positive, e.g. you're registering the method some other way. +\item \verb{@exportS3Method $\{1:package\}::$\{2:generic\}}: Export an S3 method. Only needed when the method is for a generic from a suggested package. Use \verb{@exportS3Method NULL} to suppress the missing export warning when it's a false positive, e.g. you're registering the method some other way. \item \verb{@importFrom $\{1:package\} $\{2:function\}}: Import specific functions from a package. \item \verb{@useDynLib $\{1:package\}}: Import compiled code from another package. } @@ -47,6 +49,7 @@ Other less frequently used tags: \item \verb{@exportMethod $\{1:generic\}}: Export S4 methods. For expert use only; in most cases you should use \verb{@export} so roxygen2 can automatically generate the correct directive. \item \verb{@exportPattern $\{1:pattern\}}: Export all objects matching a regular expression. \item \verb{@import $\{1:package\}}: Import all functions from a package. Use with extreme care. +\item \verb{@importAllFrom $\{1:package\}}: Import all functions from a package as explicit \code{importFrom()} calls, so the imported set is pinned at document-time instead of changing whenever the package updates. This avoids user-visible conflict warnings. \item \verb{@importClassesFrom $\{1:package\} $\{2:class\}}: Import S4 classes from another package. \item \verb{@importMethodsFrom $\{1:package\} $\{2:generic\}}: Import S4 methods from a package. \item \verb{@rawNamespace $\{1:namespace directives\}}: Insert literal text directly into the \code{NAMESPACE}. diff --git a/tests/testthat/_snaps/namespace.md b/tests/testthat/_snaps/namespace.md index 808d68f0..3d32660b 100644 --- a/tests/testthat/_snaps/namespace.md +++ b/tests/testthat/_snaps/namespace.md @@ -59,15 +59,22 @@ x :2: @importFrom must use a hanging indent to span multiple lines. i Continuation lines must be indented; did you forget a tag like `@examples`? -# expanded @import conflicting with another package errors +# @importAllFrom warns about an exclusion that isn't an export + + Code + out <- roc_proc_text(namespace_roclet(), block) + Message + x :2: @importAllFrom Ignoring unknown exclusion for testImports: `improt_b`. + +# expanded @importAllFrom conflicting with another package errors Code check_import_conflicts(imports) Condition Error in `check_import_conflicts()`: - ! Found 1 conflicting import from `@import`. + ! Found 1 conflicting import from `@importAllFrom`. * `foo` is exported by pkgA and pkgB - i Exclude unwanted symbols with e.g. `@import pkgA, except = foo`. + i Exclude unwanted symbols with e.g. `@importAllFrom pkgA -foo`. # each conflicting symbol is reported with its own packages @@ -75,10 +82,10 @@ check_import_conflicts(imports) Condition Error in `check_import_conflicts()`: - ! Found 2 conflicting imports from `@import`. + ! Found 2 conflicting imports from `@importAllFrom`. * `baz` is exported by pkgC and pkgD * `foo` is exported by pkgA and pkgB - i Exclude unwanted symbols with e.g. `@import pkgC, except = baz`. + i Exclude unwanted symbols with e.g. `@importAllFrom pkgC -baz`. # can regenerate NAMESPACE even if its broken diff --git a/tests/testthat/test-namespace.R b/tests/testthat/test-namespace.R index 75b0df2e..23037f7a 100644 --- a/tests/testthat/test-namespace.R +++ b/tests/testthat/test-namespace.R @@ -434,7 +434,7 @@ test_that("import doesn't quote if comma present", { expect_equal(sort(out), "import(rlang, except = ':=')") }) -test_that("@import expands to importFrom over all exports", { +test_that("@import never expands, even for an installed package", { out <- roc_proc_text( namespace_roclet(), " @@ -442,37 +442,149 @@ test_that("@import expands to importFrom over all exports", { NULL" ) - syms <- sort_c(unique(auto_quote(getNamespaceExports("utils")))) - expect_equal(out, format_import_from("utils", syms)) + expect_equal(out, "import(utils)") }) -test_that("@import falls back to import() when package isn't installed", { +test_that("@importAllFrom drops symbols excluded with a - prefix", { + pkgload::load_all(test_path("testImports"), quiet = TRUE) + withr::defer(pkgload::unload("testImports")) + out <- roc_proc_text( namespace_roclet(), " - #' @import notarealpkgxyz + #' @importAllFrom testImports -import_b NULL" ) - expect_equal(out, "import(notarealpkgxyz)") + expect_equal(out, "importFrom(testImports,\n import_a,\n import_c\n)") }) -test_that("@import expands each package independently", { +test_that("@importAllFrom accepts several - exclusions", { + pkgload::load_all(test_path("testImports"), quiet = TRUE) + withr::defer(pkgload::unload("testImports")) + out <- roc_proc_text( namespace_roclet(), " - #' @import utils stats notarealpkgxyz + #' @importAllFrom testImports -import_b -import_c + NULL" + ) + + expect_equal(out, "importFrom(testImports,import_a)") +}) + +test_that("@importAllFrom warns about an exclusion that isn't an export", { + pkgload::load_all(test_path("testImports"), quiet = TRUE) + withr::defer(pkgload::unload("testImports")) + + # `-improt_b` is a typo for `-import_b`, so it excludes nothing. + block <- " + #' @importAllFrom testImports -improt_b + NULL" + expect_snapshot(out <- roc_proc_text(namespace_roclet(), block)) + expect_equal( + out, + "importFrom(testImports,\n import_a,\n import_b,\n import_c\n)" + ) +}) + +test_that("parse_import_all_from splits the package from its - exclusions", { + spec <- parse_import_all_from(c("utils", "-head", "-tail")) + expect_equal(spec$pkg, "utils") + expect_equal(spec$excluded, c("head", "tail")) +}) + +test_that("parse_import_all_from unquotes non-syntactic exclusions", { + # A non-syntactic name can itself start with `-` (e.g. the S3 method `-.class`) + # and can be quoted with any of the three styles roxygen accepts. + spec <- parse_import_all_from(c( + "somepkg", + "-\"-.class\"", + "-`%op%`", + "-':='" + )) + expect_equal(spec$pkg, "somepkg") + expect_equal(spec$excluded, c("-.class", "%op%", ":=")) +}) + +test_that("@importAllFrom errors when given more than one package", { + block <- " + #' @importAllFrom utils stats NULL" + expect_error( + roc_proc_text(namespace_roclet(), block), + "Can't import multiple packages" ) +}) - # Each installed package expands to its own importFrom() directive, and the - # uninstalled one falls back to import(). - expect_true(any(startsWith(out, "importFrom(utils"))) - expect_true(any(startsWith(out, "importFrom(stats"))) - expect_true("import(notarealpkgxyz)" %in% out) +test_that("@importAllFrom errors when given no package", { + block <- " + #' @importAllFrom -head + NULL" + expect_error( + roc_proc_text(namespace_roclet(), block), + "needs a package to import from" + ) +}) + +test_that("@importAllFrom imports nothing when exclusions remove every export", { + pkgload::load_all(test_path("testImports"), quiet = TRUE) + withr::defer(pkgload::unload("testImports")) + + out <- roc_proc_text( + namespace_roclet(), + " + #' @importAllFrom testImports -import_a -import_b -import_c + NULL" + ) + + expect_equal(out, character()) +}) + +test_that("excluding a conflicting symbol keeps the rest of @importAllFrom pinned", { + pkgload::load_all(test_path("testImports"), quiet = TRUE) + withr::defer(pkgload::unload("testImports")) + + # `import_a` still conflicts with the @importFrom, since only `import_b` was + # excluded. + block <- " + #' @importAllFrom testImports -import_b + #' @importFrom anotherpkg import_a + NULL" + expect_error(roc_proc_text(namespace_roclet(), block), "conflicting import") + + # Excluding the actual conflicting symbol resolves it without losing the + # pinning for the rest of testImports's exports. + block2 <- " + #' @importAllFrom testImports -import_a + #' @importFrom anotherpkg import_a + NULL" + expect_no_error(roc_proc_text(namespace_roclet(), block2)) +}) + +test_that("@importAllFrom expands to importFrom over all exports", { + out <- roc_proc_text( + namespace_roclet(), + " + #' @importAllFrom utils + NULL" + ) + + syms <- sort_c(unique(auto_quote(getNamespaceExports("utils")))) + expect_equal(out, format_import_from("utils", syms)) +}) + +test_that("@importAllFrom errors when the package isn't installed", { + block <- " + #' @importAllFrom notarealpkgxyz + NULL" + expect_error( + roc_proc_text(namespace_roclet(), block), + "must be installed" + ) }) -test_that("expanded @import conflicting with another package errors", { +test_that("expanded @importAllFrom conflicting with another package errors", { imports <- list( import_from("pkgA", c("foo", "bar"), expanded = TRUE), import_from("pkgB", "foo") @@ -480,7 +592,7 @@ test_that("expanded @import conflicting with another package errors", { expect_snapshot(check_import_conflicts(imports), error = TRUE) }) -test_that("two expanded @import sharing a symbol errors", { +test_that("two expanded @importAllFrom sharing a symbol errors", { imports <- list( import_from("pkgA", "foo", expanded = TRUE), import_from("pkgB", "foo", expanded = TRUE) @@ -488,7 +600,7 @@ test_that("two expanded @import sharing a symbol errors", { expect_error(check_import_conflicts(imports), "conflicting import") }) -test_that("@import conflict detection ignores overlaps without expansion", { +test_that("@importAllFrom conflict detection ignores overlaps without expansion", { imports <- list( import_from("pkgA", "foo"), import_from("pkgB", "foo") @@ -496,7 +608,7 @@ test_that("@import conflict detection ignores overlaps without expansion", { expect_no_error(check_import_conflicts(imports)) }) -test_that("@import conflict detection ignores same-package duplicates", { +test_that("@importAllFrom conflict detection ignores same-package duplicates", { imports <- list( import_from("pkgA", "foo", expanded = TRUE), import_from("pkgA", "foo") @@ -504,7 +616,7 @@ test_that("@import conflict detection ignores same-package duplicates", { expect_no_error(check_import_conflicts(imports)) }) -test_that("@import conflict detection normalises quoted symbols", { +test_that("@importAllFrom conflict detection normalises quoted symbols", { # Expanded imports arrive unquoted from getNamespaceExports(), but an explicit # @importFrom of a non-syntactic name can be quoted; they must still match. imports <- list( @@ -525,10 +637,10 @@ test_that("each conflicting symbol is reported with its own packages", { expect_snapshot(check_import_conflicts(imports), error = TRUE) }) -test_that("expanding @import errors on a conflict with another import", { +test_that("expanding @importAllFrom errors on a conflict with another import", { # `stats` expands to include `sd`, which also comes in via the @importFrom. block <- " - #' @import stats + #' @importAllFrom stats #' @importFrom anotherpkg sd NULL" expect_error( @@ -537,13 +649,13 @@ test_that("expanding @import errors on a conflict with another import", { ) }) -test_that("expanding @import doesn't conflict on re-exports", { +test_that("expanding @importAllFrom doesn't conflict on re-exports", { skip_if_not_installed("purrr") # `rlang` and `purrr` both re-export the identical `is_logical`, so importing # it from purrr alongside the expanded rlang import is not a real conflict. block <- " - #' @import rlang + #' @importAllFrom rlang #' @importFrom purrr is_logical NULL" expect_no_error(roc_proc_text(namespace_roclet(), block)) @@ -635,7 +747,7 @@ test_that("rawNamespace inserted unchanged", { }) test_that("rawNamespace does not break idempotency", { - # `@import testImports` expands using this package's fixed exports. + # `@importAllFrom testImports` expands using this package's fixed exports. pkgload::load_all(test_path("testImports"), quiet = TRUE) withr::defer(pkgload::unload("testImports")) diff --git a/tests/testthat/testRawNamespace/R/a.R b/tests/testthat/testRawNamespace/R/a.R index 80c485fc..135011c8 100644 --- a/tests/testthat/testRawNamespace/R/a.R +++ b/tests/testthat/testRawNamespace/R/a.R @@ -1,4 +1,4 @@ -#' @import testImports +#' @importAllFrom testImports #' @rawNamespace #' if (TRUE) { #' import(grDevices) diff --git a/vignettes/namespace.Rmd b/vignettes/namespace.Rmd index 0412d5f4..c1eff0e0 100644 --- a/vignettes/namespace.Rmd +++ b/vignettes/namespace.Rmd @@ -97,10 +97,15 @@ NULL Note the use of `NULL` here: you must provide something for roxygen2 to document, so we use `NULL` as placeholder. -It is possible to import all functions from a package with `@import package`. -Be aware that if you bulk-import more than one package, you might need to maintain a list of exceptions in case of conflicts. -Conflicts never arise in a user-visible way at load-time because roxygen builds the imported set at document-time. -When it detects a conflict, roxygen fails with an error that recommends resolving the conflict with an `except =` clause, e.g. `@import package, except = sym`. +It is possible, but not generally recommended, to import all functions from a package with `@import package`. +This is risky if you import functions from more than one package, because while it might be ok today, in the future the packages might end up with a function having the same name, and your users will get a warning every time your package is loaded. +Any `@import` specification containing a comma, e.g. `@import rlang, except = ":="` will be inserted as is into the NAMESPACE, e.g. `import(rlang, except = ":=")`. + +If you do want to import everything from a package, `@importAllFrom package` is a safer alternative: it expands to an explicit `importFrom()` listing every symbol the package currently exports, so the imported set is pinned at document-time instead of drifting whenever the package updates. +When two `@importAllFrom` directives end up exporting the same name, an error is thrown to invite you to resolve the conflict. +You can do so by excluding the unwanted symbol from one of the import directives with a `-` prefix, e.g. `@importAllFrom package -sym`. +This drops `sym` from the expansion and keeps the rest of `package`'s exports pinned. +You can quote the excluded name if it contains special characters, e.g. `@importAllFrom package -"-.Date"`. ### S3 From 0b3c7a8bd2209037687d8abd3b8715c0578c4c5d Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 20 Jul 2026 16:15:33 +0200 Subject: [PATCH 3/4] Address code review --- R/namespace.R | 7 +++---- tests/testthat/test-namespace.R | 11 ----------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/R/namespace.R b/R/namespace.R index 21eb5404..c11cdc80 100644 --- a/R/namespace.R +++ b/R/namespace.R @@ -201,8 +201,9 @@ import_from <- function(package, funs, expanded = FALSE) { ) } -# Conflicting `@importAllFrom` directives are detected at document-time. An -# error is thrown so the user has to resolve the conflict to build the package. +# Conflicting `@importAllFrom` directives (either with another `@importAllFrom` +# or a regular `@importFrom`) are detected at document-time. An error is thrown +# so the user has to resolve the conflict to build the package. check_import_conflicts <- function(imports) { syms <- map(imports, \(x) strip_quotes(x$funs)) imported <- data.frame( @@ -222,7 +223,6 @@ check_import_conflicts <- function(imports) { # Re-exports aren't real conflicts: when several packages export the same # object (e.g. `%>%`), importing it from more than one is harmless. conflicts <- discard(conflicts, \(x) is_reexport(x$sym[[1]], unique(x$pkg))) - if (length(conflicts) == 0) { return(invisible()) } @@ -235,7 +235,6 @@ check_import_conflicts <- function(imports) { conflict <- conflicts[[1]] example_sym <- auto_quote(conflict$sym[[1]]) example_pkg <- conflict$pkg[conflict$expanded][[1]] - cli::cli_abort(c( "Found {length(conflicts)} conflicting import{?s} from {.code @importAllFrom}.", set_names(bullets, rep("*", length(bullets))), diff --git a/tests/testthat/test-namespace.R b/tests/testthat/test-namespace.R index 23037f7a..16f077e0 100644 --- a/tests/testthat/test-namespace.R +++ b/tests/testthat/test-namespace.R @@ -434,17 +434,6 @@ test_that("import doesn't quote if comma present", { expect_equal(sort(out), "import(rlang, except = ':=')") }) -test_that("@import never expands, even for an installed package", { - out <- roc_proc_text( - namespace_roclet(), - " - #' @import utils - NULL" - ) - - expect_equal(out, "import(utils)") -}) - test_that("@importAllFrom drops symbols excluded with a - prefix", { pkgload::load_all(test_path("testImports"), quiet = TRUE) withr::defer(pkgload::unload("testImports")) From 3aa1b35ab0c93f50047613a716bb10e4706088c3 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 21 Jul 2026 09:08:26 +0200 Subject: [PATCH 4/4] Use `select_args_text()` to expand `@importAllFrom` arguments --- R/namespace.R | 67 ++++++++++++------------------ R/select-args.R | 7 +++- R/utils-warn.R | 3 +- tests/testthat/_snaps/namespace.md | 13 ++++-- tests/testthat/test-namespace.R | 50 +++++++++++----------- vignettes/namespace.Rmd | 2 +- 6 files changed, 71 insertions(+), 71 deletions(-) diff --git a/R/namespace.R b/R/namespace.R index c11cdc80..30a900ae 100644 --- a/R/namespace.R +++ b/R/namespace.R @@ -403,7 +403,13 @@ roxy_tag_ns.roxy_tag_import <- function(x, block, env) { #' @export roxy_tag_parse.roxy_tag_importAllFrom <- function(x) { - tag_words(x, min = 1) + tag_two_part( + x, + "a package", + "an export selection", + required = FALSE, + markdown = FALSE + ) } #' @export roxy_tag_ns.roxy_tag_importAllFrom <- function(x, block, env) { @@ -555,11 +561,17 @@ one_per_line_ignore_current <- function(name, x) { # object currently exported by `pkg`. This early expansion pins the set of # imports at document-time and prevents user-visible conflicts at load-time when # a package update introduces a conflict with other imported symbols. +# +# Imports are handled by `select_args_text()` with `-sym` / `sym` selection +# syntax (same as `@inheritParams`). expand_import <- function(x) { current <- peek_roxygen_pkg() - spec <- parse_import_all_from(x$val) - pkg <- spec$pkg - excluded <- spec$excluded + pkg <- x$val$name + select <- x$val$description + + if (startsWith(pkg, "-")) { + cli::cli_abort("{.code @importAllFrom} needs a package to import from.") + } # Ignore an `@importAllFrom` for the package being documented if (identical(current, pkg)) { @@ -575,21 +587,20 @@ expand_import <- function(x) { all_exports <- getNamespaceExports(pkg) - # Warn on exclusions that don't match an export, so a typo like `-improt_b` - # isn't silently dropped and left to resurface as the conflict it was meant to - # resolve. - unknown <- setdiff(excluded, all_exports) - if (length(unknown) > 0) { - warn_roxy_tag( - x, - "Ignoring unknown {cli::qty(length(unknown))} exclusion{?s} for {.package {pkg}}: {.code {unknown}}" - ) - } + exports <- tryCatch( + select_args_text(all_exports, select, topic_name = pkg), + roxygen2_select_args_failed = function(cnd) { + cli::cli_abort( + "Can't expand {.code @importAllFrom {pkg}}.", + parent = cnd, + call = NULL + ) + } + ) - exports <- setdiff(all_exports, excluded) if (length(exports) == 0) { # Nothing left to import, either because `pkg` exports nothing or because - # the exclusions removed every export. Import nothing in this case instead + # the selection removed every export. Import nothing in this case instead # of falling back to `import(pkg)`. character() } else { @@ -597,30 +608,6 @@ expand_import <- function(x) { } } -# Splits an `@importAllFrom` value into the package to expand and the symbols to -# leave out. An exclusion is a word with a `-` prefix, e.g. `-abort`. Exclusions -# may be quoted: `-"-.Date"`, ``-`-.Date` ``, or `-'-.Date'`. -parse_import_all_from <- function(vals) { - is_excluded <- startsWith(vals, "-") - - pkg <- vals[!is_excluded] - if (length(pkg) == 0) { - cli::cli_abort("{.code @importAllFrom} needs a package to import from.") - } - if (length(pkg) > 1) { - cli::cli_abort( - "Can't import multiple packages with {.code @importAllFrom}." - ) - } - - excluded <- strip_quotes(sub("^-", "", vals[is_excluded])) - - list( - pkg = pkg, - excluded = excluded - ) -} - repeat_first_ignore_current <- function(name, x) { current <- peek_roxygen_pkg() diff --git a/R/select-args.R b/R/select-args.R index 133091d7..9ef6f4fc 100644 --- a/R/select-args.R +++ b/R/select-args.R @@ -11,7 +11,12 @@ select_args_text <- function(args, select, topic_name) { select_args(args, parsed) }, error = function(e) { - warn_roxy_topic(topic_name, "argument selection failed", parent = e) + warn_roxy_topic( + topic_name, + "argument selection failed", + parent = e, + class = "roxygen2_select_args_failed" + ) character() } ) diff --git a/R/utils-warn.R b/R/utils-warn.R index 9054c2bc..3a874a4c 100644 --- a/R/utils-warn.R +++ b/R/utils-warn.R @@ -63,9 +63,10 @@ warn_roxy_topic <- function( topic, message, parent = NULL, + class = NULL, envir = parent.frame() ) { message[[1]] <- paste0("In topic '", topic, "': ", message[[1]], ".") names(message)[[1]] <- "x" - cli::cli_inform(message, parent = parent, .envir = envir) + cli::cli_inform(message, parent = parent, class = class, .envir = envir) } diff --git a/tests/testthat/_snaps/namespace.md b/tests/testthat/_snaps/namespace.md index 3d32660b..77411e62 100644 --- a/tests/testthat/_snaps/namespace.md +++ b/tests/testthat/_snaps/namespace.md @@ -59,12 +59,17 @@ x :2: @importFrom must use a hanging indent to span multiple lines. i Continuation lines must be indented; did you forget a tag like `@examples`? -# @importAllFrom warns about an exclusion that isn't an export +# @importAllFrom errors on an exclusion that isn't an export Code - out <- roc_proc_text(namespace_roclet(), block) - Message - x :2: @importAllFrom Ignoring unknown exclusion for testImports: `improt_b`. + roc_proc_text(namespace_roclet(), block) + Condition + Error: + ! Can't expand `@importAllFrom testImports`. + Caused by message: + ! x In topic 'testImports': argument selection failed. + Caused by error in `FUN()`: + ! object 'improt_b' not found # expanded @importAllFrom conflicting with another package errors diff --git a/tests/testthat/test-namespace.R b/tests/testthat/test-namespace.R index 16f077e0..5fd5053d 100644 --- a/tests/testthat/test-namespace.R +++ b/tests/testthat/test-namespace.R @@ -462,47 +462,49 @@ test_that("@importAllFrom accepts several - exclusions", { expect_equal(out, "importFrom(testImports,import_a)") }) -test_that("@importAllFrom warns about an exclusion that isn't an export", { +test_that("@importAllFrom errors on an exclusion that isn't an export", { pkgload::load_all(test_path("testImports"), quiet = TRUE) withr::defer(pkgload::unload("testImports")) - # `-improt_b` is a typo for `-import_b`, so it excludes nothing. + # `-improt_b` is a typo for `-import_b`, so it doesn't match any export. block <- " #' @importAllFrom testImports -improt_b NULL" - expect_snapshot(out <- roc_proc_text(namespace_roclet(), block)) - expect_equal( - out, - "importFrom(testImports,\n import_a,\n import_b,\n import_c\n)" - ) + expect_snapshot(roc_proc_text(namespace_roclet(), block), error = TRUE) }) -test_that("parse_import_all_from splits the package from its - exclusions", { - spec <- parse_import_all_from(c("utils", "-head", "-tail")) - expect_equal(spec$pkg, "utils") - expect_equal(spec$excluded, c("head", "tail")) +test_that("@importAllFrom drops a backtick-quoted non-syntactic export", { + out <- roc_proc_text( + namespace_roclet(), + " + #' @importAllFrom rlang -`%||%` + NULL" + ) + + expect_false(any(grepl('"%||%"', out, fixed = TRUE))) }) -test_that("parse_import_all_from unquotes non-syntactic exclusions", { - # A non-syntactic name can itself start with `-` (e.g. the S3 method `-.class`) - # and can be quoted with any of the three styles roxygen accepts. - spec <- parse_import_all_from(c( - "somepkg", - "-\"-.class\"", - "-`%op%`", - "-':='" - )) - expect_equal(spec$pkg, "somepkg") - expect_equal(spec$excluded, c("-.class", "%op%", ":=")) +test_that("@importAllFrom treats a bare word as a positive selection", { + pkgload::load_all(test_path("testImports"), quiet = TRUE) + withr::defer(pkgload::unload("testImports")) + + out <- roc_proc_text( + namespace_roclet(), + " + #' @importAllFrom testImports import_a import_b + NULL" + ) + + expect_equal(out, "importFrom(testImports,\n import_a,\n import_b\n)") }) -test_that("@importAllFrom errors when given more than one package", { +test_that("@importAllFrom errors when a positive selection isn't an export", { block <- " #' @importAllFrom utils stats NULL" expect_error( roc_proc_text(namespace_roclet(), block), - "Can't import multiple packages" + "Can't expand" ) }) diff --git a/vignettes/namespace.Rmd b/vignettes/namespace.Rmd index c1eff0e0..645403bf 100644 --- a/vignettes/namespace.Rmd +++ b/vignettes/namespace.Rmd @@ -105,7 +105,7 @@ If you do want to import everything from a package, `@importAllFrom package` is When two `@importAllFrom` directives end up exporting the same name, an error is thrown to invite you to resolve the conflict. You can do so by excluding the unwanted symbol from one of the import directives with a `-` prefix, e.g. `@importAllFrom package -sym`. This drops `sym` from the expansion and keeps the rest of `package`'s exports pinned. -You can quote the excluded name if it contains special characters, e.g. `@importAllFrom package -"-.Date"`. +Backtick-quote the excluded name if it is non-syntactic, e.g. `` @importAllFrom package -`-.Date` ``. ### S3