From 892e10f2cf5f569742d0f1516ccd5c00fecafba7 Mon Sep 17 00:00:00 2001 From: David Schoch Date: Mon, 1 Jun 2026 16:31:51 +0200 Subject: [PATCH 01/12] add arg migration --- .github/workflows/check-migrations.yaml | 42 ++++ R/handle-args-migration_fixture.R | 82 ++++++++ air.toml | 2 +- tests/testthat/test-handle-args.R | 115 +++++++++++ tools/README.md | 66 +++++++ tools/generate-migrations.R | 248 ++++++++++++++++++++++++ tools/migrations.R | 88 +++++++++ 7 files changed, 642 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/check-migrations.yaml create mode 100644 R/handle-args-migration_fixture.R create mode 100644 tests/testthat/test-handle-args.R create mode 100644 tools/generate-migrations.R create mode 100644 tools/migrations.R diff --git a/.github/workflows/check-migrations.yaml b/.github/workflows/check-migrations.yaml new file mode 100644 index 00000000000..2b6a9642774 --- /dev/null +++ b/.github/workflows/check-migrations.yaml @@ -0,0 +1,42 @@ +# Verifies that the generated argument-migration helpers in R/handle-args-*.R +# are in sync with the registry in tools/migrations.R. If someone edits a +# generated file by hand, or changes the registry without regenerating, this +# fails. Regenerate locally with: Rscript tools/generate-migrations.R +# +# See tools/README.md ("Argument-migration helpers") for the design. +name: check-migrations + +on: + push: + branches: [main, master] + pull_request: + branches: [main, master] + merge_group: + types: [checks_requested] + workflow_dispatch: + +permissions: + contents: read + +jobs: + check-migrations: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + + - uses: r-lib/actions/setup-r@v2 + with: + # The generator is base-R only; no package dependencies are needed. + use-public-rspm: true + + - name: Regenerate migration helpers + run: Rscript tools/generate-migrations.R + + - name: Fail if generated files drift from the registry + run: | + if ! git diff --exit-code -- 'R/handle-args-*.R'; then + echo "::error::R/handle-args-*.R is out of sync with tools/migrations.R." + echo "Run 'Rscript tools/generate-migrations.R' and commit the result." + exit 1 + fi + shell: bash diff --git a/R/handle-args-migration_fixture.R b/R/handle-args-migration_fixture.R new file mode 100644 index 00000000000..5ef68ed36c0 --- /dev/null +++ b/R/handle-args-migration_fixture.R @@ -0,0 +1,82 @@ +# Generated by tools/generate-migrations.R from tools/migrations.R — do not edit by hand. +# Regenerate with: Rscript tools/generate-migrations.R +# (Excluded from `air` formatting via air.toml so this layout stays canonical.) +# +# `migration_fixture()`'s pre-3.0.0 signature took its arguments +# positionally. After `...` was inserted, a legacy positional call lands the +# old arguments in `...`. This helper recovers them, soft-deprecates the +# positional form, and returns the new-API bindings as a named list. +# +# `.user_env` is threaded into `lifecycle::deprecate_soft()` so the warning is +# attributed to the user's call instead of being suppressed as +# package-internal. Public callers must pass `.user_env = rlang::caller_env()`. + +#' @noRd +handle_args_migration_fixture <- function( + a, + b, + ..., + c_renamed = NULL, + d = NULL, + .user_env = rlang::caller_env() +) { + bindings <- list(a = a, b = b, c_renamed = c_renamed, d = d) + + dots <- list(...) + if (length(dots) == 0L) { + return(bindings) + } + + # Any *named* dot is an unknown keyword (future extension or a typo); enforce + # the strict contract. Only a purely positional `...` is treated as a legacy + # call and recovered below. + if (any(nzchar(rlang::names2(dots)))) { + rlang::check_dots_empty(call = .user_env) + } + + recover_new <- c("c_renamed", "d") + recover_old <- c("c", "d") + defaults <- list(c_renamed = NULL, d = NULL) + + if (length(dots) > length(recover_new)) { + cli::cli_abort("Too many arguments passed to {.fn migration_fixture}.", call = .user_env) + } + + for (i in seq_along(dots)) { + new_name <- recover_new[[i]] + # Conflict: the same logical slot was supplied positionally *and* by its + # new keyword (i.e. the keyword differs from its formal default). + if (!identical(bindings[[new_name]], defaults[[new_name]])) { + cli::cli_abort( + c( + "Can't supply {.arg {recover_old[[i]]}} both positionally and as {.arg {new_name}}.", + i = "Drop the positional value and keep {.code {new_name} = }." + ), + call = .user_env + ) + } + bindings[[new_name]] <- dots[[i]] + } + + # One consolidated soft-deprecation per call. Key the message on the first + # renamed slot if there is one, else the first recovered slot. + rebound <- seq_along(dots) + renamed <- rebound[recover_old[rebound] != recover_new[rebound]] + primary <- if (length(renamed)) renamed[[1L]] else 1L + lifecycle::deprecate_soft( + when = "3.0.0", + what = sprintf("migration_fixture(%s)", recover_old[[primary]]), + with = if (recover_old[[primary]] != recover_new[[primary]]) sprintf("migration_fixture(%s)", recover_new[[primary]]), + details = c( + "Positional use of the pre-3.0.0 signature of `migration_fixture()` is deprecated.", + i = paste0( + "Name the argument(s) instead: ", + toString(sprintf("`%s = `", recover_new[rebound])), "." + ) + ), + user_env = .user_env + ) + + bindings +} + diff --git a/air.toml b/air.toml index 58d5e7dcd80..c56a6915daa 100644 --- a/air.toml +++ b/air.toml @@ -5,5 +5,5 @@ indent-style = "space" line-ending = "auto" persistent-line-breaks = true default-exclude = true -exclude = ["R/utils-s3.R"] +exclude = ["R/utils-s3.R", "R/handle-args-*.R"] skip = ["tribble", "graph_from_literal", "matrix", "c", "make_graph", "edges"] diff --git a/tests/testthat/test-handle-args.R b/tests/testthat/test-handle-args.R new file mode 100644 index 00000000000..8438fdcab81 --- /dev/null +++ b/tests/testthat/test-handle-args.R @@ -0,0 +1,115 @@ +# Tests for the generated argument-migration helpers (tools/generate-migrations.R). +# +# These exercise the test fixture `handle_args_migration_fixture()`, generated +# from the `migration_fixture` entry in tools/migrations.R. Old signature: +# f(a, b, c, d); new signature: f(a, b, ..., c_renamed, d) with c -> c_renamed. + +test_that("pure new-API call returns the bindings unchanged", { + expect_equal( + handle_args_migration_fixture(a = 1, b = 2, c_renamed = 3, d = 4), + list(a = 1, b = 2, c_renamed = 3, d = 4) + ) +}) + +test_that("empty dots fall back to the formal defaults", { + expect_equal( + handle_args_migration_fixture(a = 1, b = 2), + list(a = 1, b = 2, c_renamed = NULL, d = NULL) + ) +}) + +test_that("a named extension in `...` errors via check_dots_empty()", { + expect_error( + handle_args_migration_fixture(1, 2, c_renamed = 3, d = 4, foo = 5), + class = "rlib_error_dots_nonempty" + ) +}) + +test_that("a legacy positional call is recovered with one soft-deprecation", { + rlang::local_options(lifecycle_verbosity = "warning") + + lifecycle::expect_deprecated( + res <- handle_args_migration_fixture(1, 2, 3, 4), + "migration_fixture" + ) + expect_equal(res, list(a = 1, b = 2, c_renamed = 3, d = 4)) +}) + +test_that("a legacy positional call recovering a single slot still works", { + rlang::local_options(lifecycle_verbosity = "warning") + + lifecycle::expect_deprecated( + res <- handle_args_migration_fixture(1, 2, 99) + ) + expect_equal(res, list(a = 1, b = 2, c_renamed = 99, d = NULL)) +}) + +test_that("recovery only fires one deprecation warning, not one per slot", { + rlang::local_options(lifecycle_verbosity = "warning") + + warnings <- character() + withCallingHandlers( + handle_args_migration_fixture(1, 2, 3, 4), + lifecycle_warning_deprecated = function(w) { + warnings <<- c(warnings, conditionMessage(w)) + invokeRestart("muffleWarning") + } + ) + expect_length(warnings, 1L) +}) + +test_that("supplying a slot both positionally and by its new keyword errors", { + expect_error( + handle_args_migration_fixture(1, 2, 3, c_renamed = "kw"), + "both positionally and as" + ) +}) + +test_that("too many positional arguments errors", { + expect_error( + handle_args_migration_fixture(1, 2, 3, 4, 5), + "Too many arguments" + ) +}) + +test_that(".user_env controls whether the soft-deprecation surfaces", { + # Gotcha: lifecycle::deprecate_soft() only signals when is_direct(user_env) is + # TRUE -- a package-internal `user_env` is swallowed *even under* + # lifecycle_verbosity = "warning" (it gates on `direct` before honouring the + # verbosity override). This is why the generated helper threads `.user_env`, + # and why public callers must pass `.user_env = rlang::caller_env()` so it + # resolves to the *user's* frame rather than the package namespace. + rlang::local_options(lifecycle_verbosity = "warning") + + # A direct user env (the default here is this test's frame) -> warning fires. + lifecycle::expect_deprecated( + res <- handle_args_migration_fixture(1, 2, 3, 4) + ) + expect_equal(res, list(a = 1, b = 2, c_renamed = 3, d = 4)) + + # A package namespace user env -> suppressed, but recovery still happens. + expect_no_warning( + res <- handle_args_migration_fixture(1, 2, 3, 4, .user_env = asNamespace("igraph")) + ) + expect_equal(res, list(a = 1, b = 2, c_renamed = 3, d = 4)) +}) + +test_that("the generated helper is in sync with the registry", { + # tools/ is excluded from the built package (.Rbuildignore), so this only runs + # from a source checkout (local dev + CI). CI also guards drift via + # .github/workflows/check-migrations.yaml. + generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") + skip_if_not(file.exists(generator)) + + gen_env <- new.env() + sys.source(generator, envir = gen_env) + registry <- testthat::test_path("..", "..", "tools", "migrations.R") + entries <- gen_env$load_migrations(registry) + fixture <- Filter(function(e) e$fn == "migration_fixture", entries)[[1]] + + expected <- gen_env$render_migration(fixture) + committed <- readLines( + testthat::test_path("..", "..", "R", "handle-args-migration_fixture.R") + ) + expect_identical(committed, expected) +}) diff --git a/tools/README.md b/tools/README.md index b8fd8b3a2ef..66403f91cfd 100644 --- a/tools/README.md +++ b/tools/README.md @@ -145,6 +145,72 @@ git subrepo status tools/py-stimulus This shows the remote URL, branch, and commit hashes. +## Argument-migration helpers + +When a public function's signature is migrated by inserting `...` and renaming +or repositioning arguments, pre-existing **positional** calls would otherwise +break (the old positional values land in `...` and hit `check_dots_empty()`). +To soften that, each migrated function gets an internal `handle_args_()` +helper that recovers a legacy positional call, soft-deprecates the positional +form, and returns the new-API bindings as a named list. + +These helpers are **generated**, not written by hand: + +- **`tools/migrations.R`** is the single source of truth. It declares one entry + per migrated function (old/new signatures, renames, defaults, lifecycle + `when`). The entry schema is documented at the top of that file. +- **`tools/generate-migrations.R`** reads the registry and emits one + `R/handle-args-.R` per entry. Output is deterministic (entries sorted by + function name) and idempotent (running twice produces no diff). + +Regenerate after editing the registry: + +```sh +Rscript tools/generate-migrations.R +``` + +Do **not** edit `R/handle-args-*.R` by hand — `.github/workflows/check-migrations.yaml` +regenerates them in CI and fails on drift. + +### Call-site usage + +A migrated public function delegates argument resolution to its generated +helper, passing `.user_env = rlang::caller_env()`: + +```r +as_biadjacency_matrix <- function(graph, types, ..., weights = NULL, + attr = lifecycle::deprecated(), + names = TRUE, sparse = FALSE) { + args <- handle_args_as_biadjacency_matrix( + graph, types, ..., weights = weights, attr = attr, + names = names, sparse = sparse, + .user_env = rlang::caller_env() + ) + graph <- args$graph; types <- args$types; weights <- args$weights + # ... rest of the body +} +``` + +The `.user_env` argument is essential. `lifecycle::deprecate_soft()` only signals +when `is_direct(user_env)` is `TRUE`; a package-internal `user_env` is swallowed +**even under** `lifecycle_verbosity = "warning"`. Threading the *caller's* env +through `.user_env` is what keeps the deprecation visible to users. + +### Known limitations + +The generator handles **pure renames and insertions** only: + +- **Removed args** — deleting an old positional slot (rather than renaming it) + makes a legacy positional call silently rebind to the wrong new slot. Not + detected. +- **Reordered args** — the new signature must preserve the relative order of + surviving old args. Reordering breaks the position math. +- **Partial matching** — pre-`...` R allowed `f(g, att = "x")` to partial-match + `attrs`. Post-insertion, partial names fall into `...` and are treated as + unknown keywords (the strict `check_dots_empty()` path), never recovered. + +These are acceptable because the migrations planned for 3.0.0 are pure renames. + ## Additional Documentation For detailed information on implementing R wrappers for callback functions, see [`AGENTS.md`](AGENTS.md) in this directory. diff --git a/tools/generate-migrations.R b/tools/generate-migrations.R new file mode 100644 index 00000000000..141d923b2ca --- /dev/null +++ b/tools/generate-migrations.R @@ -0,0 +1,248 @@ +#!/usr/bin/env Rscript +# +# Generator for per-function argument-migration helpers. +# +# Reads the declarative registry in tools/migrations.R and, for each entry, +# emits one self-contained internal helper `handle_args_()` at +# R/handle-args-.R. The generated helpers recover legacy positional calls +# made against a function's pre-migration signature after `...` was inserted. +# +# Usage: +# Rscript tools/generate-migrations.R +# +# The output is deterministic (entries sorted alphabetically by `fn`) and +# idempotent (running twice produces no diff). A CI check regenerates and fails +# on drift; see .github/workflows/check-migrations.yaml. +# +# Base R only -- no package needs to be loaded to run this. + +# ---- locate paths relative to this script -------------------------------- + +migration_paths <- function() { + # When run via `Rscript tools/generate-migrations.R`, the working directory is + # the package root. + root <- getwd() + list( + registry = file.path(root, "tools", "migrations.R"), + out_dir = file.path(root, "R") + ) +} + +# ---- registry loading + validation ---------------------------------------- + +load_migrations <- function(registry_path) { + env <- new.env(parent = baseenv()) + sys.source(registry_path, envir = env, keep.source = FALSE) + if (!exists("migrations", envir = env, inherits = FALSE)) { + stop("`", registry_path, "` must define a `migrations` list.", call. = FALSE) + } + migrations <- get("migrations", envir = env) + lapply(migrations, validate_migration) +} + +validate_migration <- function(entry) { + req <- c("fn", "old", "new") + missing <- setdiff(req, names(entry)) + if (length(missing)) { + stop("Migration entry is missing field(s): ", + paste(missing, collapse = ", "), call. = FALSE) + } + entry$renames <- entry$renames %||% character(0) + entry$defaults <- entry$defaults %||% list() + entry$when <- entry$when %||% "3.0.0" + + if (sum(entry$new == "...") != 1L) { + stop("Migration `", entry$fn, + "`: `new` must contain exactly one \"...\" marker.", call. = FALSE) + } + + dots_idx <- which(entry$new == "...") + entry$head <- entry$new[seq_len(dots_idx - 1L)] + entry$tail <- if (dots_idx < length(entry$new)) { + entry$new[(dots_idx + 1L):length(entry$new)] + } else { + character(0) + } + entry$new_args <- c(entry$head, entry$tail) + + if (length(entry$old) < length(entry$head)) { + stop("Migration `", entry$fn, + "`: `old` is shorter than the head args of `new`.", call. = FALSE) + } + # Old positional slots beyond the head are the recoverable ones. + entry$recover_old <- if (length(entry$old) > length(entry$head)) { + entry$old[(length(entry$head) + 1L):length(entry$old)] + } else { + character(0) + } + entry$recover_new <- vapply(entry$recover_old, function(nm) { + if (nm %in% names(entry$renames)) unname(entry$renames[[nm]]) else nm + }, character(1)) + + bad <- setdiff(entry$recover_new, entry$tail) + if (length(bad)) { + stop("Migration `", entry$fn, "`: recovered slot(s) ", + paste(bad, collapse = ", "), + " do not appear after `...` in `new`.", call. = FALSE) + } + entry +} + +`%||%` <- function(x, y) if (is.null(x)) y else x + +# ---- rendering ------------------------------------------------------------- + +# Build the formals list of the generated helper as a single string. +render_formals <- function(entry) { + head_fmls <- entry$head + tail_fmls <- vapply(entry$tail, function(nm) { + if (nm %in% names(entry$defaults)) { + paste0(nm, " = ", entry$defaults[[nm]]) + } else { + nm + } + }, character(1)) + c(head_fmls, "...", tail_fmls, ".user_env = rlang::caller_env()") +} + +# Render a character vector literal, e.g. c("a", "b"). +render_chr_vec <- function(x) { + if (length(x) == 0L) return("character(0)") + paste0("c(", paste0('"', x, '"', collapse = ", "), ")") +} + +# Render the `defaults` list literal restricted to recoverable slots, reusing +# the registry's default *expressions* so the runtime identity check compares +# against the true formal default. +render_defaults_list <- function(entry) { + if (length(entry$recover_new) == 0L) return("list()") + items <- vapply(entry$recover_new, function(nm) { + expr <- if (nm %in% names(entry$defaults)) entry$defaults[[nm]] else "NULL" + paste0(nm, " = ", expr) + }, character(1)) + paste0("list(", paste(items, collapse = ", "), ")") +} + +render_migration <- function(entry) { + fn <- entry$fn + bindings_items <- paste0(entry$new_args, " = ", entry$new_args) + + header <- c( + "# Generated by tools/generate-migrations.R from tools/migrations.R — do not edit by hand.", + "# Regenerate with: Rscript tools/generate-migrations.R", + "# (Excluded from `air` formatting via air.toml so this layout stays canonical.)", + "#", + paste0("# `", fn, "()`'s pre-", entry$when, " signature took its arguments"), + "# positionally. After `...` was inserted, a legacy positional call lands the", + "# old arguments in `...`. This helper recovers them, soft-deprecates the", + "# positional form, and returns the new-API bindings as a named list.", + "#", + "# `.user_env` is threaded into `lifecycle::deprecate_soft()` so the warning is", + "# attributed to the user's call instead of being suppressed as", + "# package-internal. Public callers must pass `.user_env = rlang::caller_env()`." + ) + + # One formal per line, matching rigraph's hand-written signatures. The file is + # excluded from `air` (see air.toml) so this layout is canonical. + fmls <- render_formals(entry) + fmls_lines <- paste0(" ", fmls) + fmls_lines[-length(fmls_lines)] <- paste0(fmls_lines[-length(fmls_lines)], ",") + signature <- c( + paste0("handle_args_", fn, " <- function("), + fmls_lines, + ") {" + ) + + body <- c( + paste0(" bindings <- list(", paste(bindings_items, collapse = ", "), ")"), + "", + " dots <- list(...)", + " if (length(dots) == 0L) {", + " return(bindings)", + " }", + "", + " # Any *named* dot is an unknown keyword (future extension or a typo); enforce", + " # the strict contract. Only a purely positional `...` is treated as a legacy", + " # call and recovered below.", + " if (any(nzchar(rlang::names2(dots)))) {", + " rlang::check_dots_empty(call = .user_env)", + " }", + "", + paste0(" recover_new <- ", render_chr_vec(entry$recover_new)), + paste0(" recover_old <- ", render_chr_vec(entry$recover_old)), + paste0(" defaults <- ", render_defaults_list(entry)), + "", + " if (length(dots) > length(recover_new)) {", + paste0(" cli::cli_abort(\"Too many arguments passed to {.fn ", fn, "}.\", call = .user_env)"), + " }", + "", + " for (i in seq_along(dots)) {", + " new_name <- recover_new[[i]]", + " # Conflict: the same logical slot was supplied positionally *and* by its", + " # new keyword (i.e. the keyword differs from its formal default).", + " if (!identical(bindings[[new_name]], defaults[[new_name]])) {", + " cli::cli_abort(", + " c(", + " \"Can't supply {.arg {recover_old[[i]]}} both positionally and as {.arg {new_name}}.\",", + " i = \"Drop the positional value and keep {.code {new_name} = }.\"", + " ),", + " call = .user_env", + " )", + " }", + " bindings[[new_name]] <- dots[[i]]", + " }", + "", + " # One consolidated soft-deprecation per call. Key the message on the first", + " # renamed slot if there is one, else the first recovered slot.", + " rebound <- seq_along(dots)", + " renamed <- rebound[recover_old[rebound] != recover_new[rebound]]", + " primary <- if (length(renamed)) renamed[[1L]] else 1L", + " lifecycle::deprecate_soft(", + paste0(" when = \"", entry$when, "\","), + paste0(" what = sprintf(\"", fn, "(%s)\", recover_old[[primary]]),"), + paste0(" with = if (recover_old[[primary]] != recover_new[[primary]]) sprintf(\"", fn, "(%s)\", recover_new[[primary]]),"), + " details = c(", + paste0(" \"Positional use of the pre-", entry$when, " signature of `", fn, "()` is deprecated.\","), + " i = paste0(", + " \"Name the argument(s) instead: \",", + " toString(sprintf(\"`%s = `\", recover_new[rebound])), \".\"", + " )", + " ),", + " user_env = .user_env", + " )", + "", + " bindings", + "}" + ) + + c(header, "", "#' @noRd", signature, body, "") +} + +# ---- driver ---------------------------------------------------------------- + +generate_migrations <- function(registry_path, out_dir) { + migrations <- load_migrations(registry_path) + + # Remove stale generated files first so deleted registry entries don't leave + # orphaned helpers behind. + stale <- list.files(out_dir, pattern = "^handle-args-.*\\.R$", full.names = TRUE) + if (length(stale)) file.remove(stale) + + fns <- vapply(migrations, function(e) e$fn, character(1)) + migrations <- migrations[order(fns)] + + for (entry in migrations) { + lines <- render_migration(entry) + out_path <- file.path(out_dir, paste0("handle-args-", entry$fn, ".R")) + writeLines(lines, out_path) + message("wrote ", out_path) + } + message("generated ", length(migrations), " migration helper(s)") + invisible(vapply(migrations, function(e) e$fn, character(1))) +} + +# Run when invoked as a script (not when sourced for its functions). +if (sys.nframe() == 0L && !interactive()) { + paths <- migration_paths() + generate_migrations(paths$registry, paths$out_dir) +} diff --git a/tools/migrations.R b/tools/migrations.R new file mode 100644 index 00000000000..742b2a13a7e --- /dev/null +++ b/tools/migrations.R @@ -0,0 +1,88 @@ +# Migration registry: the single source of truth for argument-signature +# migrations that insert `...` and soft-deprecate the old positional form. +# +# This file is read by tools/generate-migrations.R, which turns each entry into +# an internal `handle_args_()` helper at R/handle-args-.R. Do NOT edit +# the generated R/handle-args-*.R files by hand -- edit this registry and +# regenerate: +# +# Rscript tools/generate-migrations.R +# +# --------------------------------------------------------------------------- +# Entry schema (a named list per migration): +# +# fn Character scalar. Name of the public function being migrated. +# The generated helper is `handle_args_()`. +# +# old Character vector. The positional argument order *before* the +# migration, e.g. c("graph", "types", "attr", "names", "sparse"). +# +# new Character vector. The positional order *after* the migration, +# including the literal "..." marker showing where dots were +# inserted, e.g. +# c("graph", "types", "...", "weights", "attr", "names", "sparse"). +# The non-"..." names are the new-API formals, in order. +# +# renames Named character vector mapping old name -> new name, for the +# arguments that were renamed. Args that kept their name are omitted. +# e.g. c(attr = "weights"). Default: character(0) (pure reorder/none). +# +# defaults Named list mapping new-API arg name -> a *string* holding the R +# expression used as that arg's formal default. Args present here get +# `= ` in the generated signature; args absent are required +# (no default). The default value is also what the helper compares +# against (via identical()) to decide whether the user explicitly +# passed a keyword that would conflict with a recovered positional. +# e.g. list(weights = "NULL", attr = "lifecycle::deprecated()"). +# +# when Character scalar. The version string passed to +# `lifecycle::deprecate_soft(when = )`, e.g. "3.0.0". +# +# --------------------------------------------------------------------------- +# Known limitations (documented, not handled -- see tools/README.md): +# +# * Removed args: if a migration deletes an old positional slot rather than +# renaming it, a legacy positional call silently rebinds to the wrong new +# slot. The generator assumes every old slot beyond the head survives. +# * Reordered args: the generator assumes the new signature preserves the +# relative order of surviving old args (insertions and renames only). +# * Partial matching: pre-`...` R let `f(g, att = "x")` partial-match `attrs`. +# Post-insertion, partial names fall into `...` and are treated as unknown +# named extras (the strict `check_dots_empty()` path), never recovered. +# +# These are acceptable because the migrations planned for 3.0.0 are pure +# renames with no removals or reorderings. +# --------------------------------------------------------------------------- + +migrations <- list( + # No real package function is migrated yet. Real entries are added by the + # branch that consumes this infrastructure (see tools/README.md). Example of + # the shape a real entry will take: + # + # list( + # fn = "as_biadjacency_matrix", + # old = c("graph", "types", "attr", "names", "sparse"), + # new = c("graph", "types", "...", "weights", "attr", "names", "sparse"), + # renames = c(attr = "weights"), + # defaults = list( + # weights = "NULL", + # attr = "lifecycle::deprecated()", + # names = "TRUE", + # sparse = "FALSE" + # ), + # when = "3.0.0" + # ), + + # --- test fixture -------------------------------------------------------- + # Exercises the generator end-to-end without migrating a real function. + # Old signature: f(a, b, c, d); new: f(a, b, ..., c_renamed, d) with c renamed + # to c_renamed. Consumed by tests/testthat/test-handle-args.R. + list( + fn = "migration_fixture", + old = c("a", "b", "c", "d"), + new = c("a", "b", "...", "c_renamed", "d"), + renames = c(c = "c_renamed"), + defaults = list(c_renamed = "NULL", d = "NULL"), + when = "3.0.0" + ) +) From e83a58d46ddef2d86cd5adad01457dea7591d990 Mon Sep 17 00:00:00 2001 From: schochastics Date: Mon, 1 Jun 2026 14:44:54 +0000 Subject: [PATCH 02/12] chore: Auto-update from GitHub Actions Run: https://github.com/igraph/rigraph/actions/runs/26761697414 --- tests/testthat/test-handle-args.R | 8 ++- tools/generate-migrations.R | 114 +++++++++++++++++++++--------- tools/migrations.R | 10 +-- 3 files changed, 94 insertions(+), 38 deletions(-) diff --git a/tests/testthat/test-handle-args.R b/tests/testthat/test-handle-args.R index 8438fdcab81..46142241833 100644 --- a/tests/testthat/test-handle-args.R +++ b/tests/testthat/test-handle-args.R @@ -89,7 +89,13 @@ test_that(".user_env controls whether the soft-deprecation surfaces", { # A package namespace user env -> suppressed, but recovery still happens. expect_no_warning( - res <- handle_args_migration_fixture(1, 2, 3, 4, .user_env = asNamespace("igraph")) + res <- handle_args_migration_fixture( + 1, + 2, + 3, + 4, + .user_env = asNamespace("igraph") + ) ) expect_equal(res, list(a = 1, b = 2, c_renamed = 3, d = 4)) }) diff --git a/tools/generate-migrations.R b/tools/generate-migrations.R index 141d923b2ca..c401add1f42 100644 --- a/tools/generate-migrations.R +++ b/tools/generate-migrations.R @@ -24,7 +24,7 @@ migration_paths <- function() { root <- getwd() list( registry = file.path(root, "tools", "migrations.R"), - out_dir = file.path(root, "R") + out_dir = file.path(root, "R") ) } @@ -34,7 +34,12 @@ load_migrations <- function(registry_path) { env <- new.env(parent = baseenv()) sys.source(registry_path, envir = env, keep.source = FALSE) if (!exists("migrations", envir = env, inherits = FALSE)) { - stop("`", registry_path, "` must define a `migrations` list.", call. = FALSE) + stop( + "`", + registry_path, + "` must define a `migrations` list.", + call. = FALSE + ) } migrations <- get("migrations", envir = env) lapply(migrations, validate_migration) @@ -44,16 +49,23 @@ validate_migration <- function(entry) { req <- c("fn", "old", "new") missing <- setdiff(req, names(entry)) if (length(missing)) { - stop("Migration entry is missing field(s): ", - paste(missing, collapse = ", "), call. = FALSE) + stop( + "Migration entry is missing field(s): ", + paste(missing, collapse = ", "), + call. = FALSE + ) } - entry$renames <- entry$renames %||% character(0) + entry$renames <- entry$renames %||% character(0) entry$defaults <- entry$defaults %||% list() - entry$when <- entry$when %||% "3.0.0" + entry$when <- entry$when %||% "3.0.0" if (sum(entry$new == "...") != 1L) { - stop("Migration `", entry$fn, - "`: `new` must contain exactly one \"...\" marker.", call. = FALSE) + stop( + "Migration `", + entry$fn, + "`: `new` must contain exactly one \"...\" marker.", + call. = FALSE + ) } dots_idx <- which(entry$new == "...") @@ -66,8 +78,12 @@ validate_migration <- function(entry) { entry$new_args <- c(entry$head, entry$tail) if (length(entry$old) < length(entry$head)) { - stop("Migration `", entry$fn, - "`: `old` is shorter than the head args of `new`.", call. = FALSE) + stop( + "Migration `", + entry$fn, + "`: `old` is shorter than the head args of `new`.", + call. = FALSE + ) } # Old positional slots beyond the head are the recoverable ones. entry$recover_old <- if (length(entry$old) > length(entry$head)) { @@ -75,15 +91,24 @@ validate_migration <- function(entry) { } else { character(0) } - entry$recover_new <- vapply(entry$recover_old, function(nm) { - if (nm %in% names(entry$renames)) unname(entry$renames[[nm]]) else nm - }, character(1)) + entry$recover_new <- vapply( + entry$recover_old, + function(nm) { + if (nm %in% names(entry$renames)) unname(entry$renames[[nm]]) else nm + }, + character(1) + ) bad <- setdiff(entry$recover_new, entry$tail) if (length(bad)) { - stop("Migration `", entry$fn, "`: recovered slot(s) ", - paste(bad, collapse = ", "), - " do not appear after `...` in `new`.", call. = FALSE) + stop( + "Migration `", + entry$fn, + "`: recovered slot(s) ", + paste(bad, collapse = ", "), + " do not appear after `...` in `new`.", + call. = FALSE + ) } entry } @@ -95,19 +120,25 @@ validate_migration <- function(entry) { # Build the formals list of the generated helper as a single string. render_formals <- function(entry) { head_fmls <- entry$head - tail_fmls <- vapply(entry$tail, function(nm) { - if (nm %in% names(entry$defaults)) { - paste0(nm, " = ", entry$defaults[[nm]]) - } else { - nm - } - }, character(1)) + tail_fmls <- vapply( + entry$tail, + function(nm) { + if (nm %in% names(entry$defaults)) { + paste0(nm, " = ", entry$defaults[[nm]]) + } else { + nm + } + }, + character(1) + ) c(head_fmls, "...", tail_fmls, ".user_env = rlang::caller_env()") } # Render a character vector literal, e.g. c("a", "b"). render_chr_vec <- function(x) { - if (length(x) == 0L) return("character(0)") + if (length(x) == 0L) { + return("character(0)") + } paste0("c(", paste0('"', x, '"', collapse = ", "), ")") } @@ -115,11 +146,21 @@ render_chr_vec <- function(x) { # the registry's default *expressions* so the runtime identity check compares # against the true formal default. render_defaults_list <- function(entry) { - if (length(entry$recover_new) == 0L) return("list()") - items <- vapply(entry$recover_new, function(nm) { - expr <- if (nm %in% names(entry$defaults)) entry$defaults[[nm]] else "NULL" - paste0(nm, " = ", expr) - }, character(1)) + if (length(entry$recover_new) == 0L) { + return("list()") + } + items <- vapply( + entry$recover_new, + function(nm) { + expr <- if (nm %in% names(entry$defaults)) { + entry$defaults[[nm]] + } else { + "NULL" + } + paste0(nm, " = ", expr) + }, + character(1) + ) paste0("list(", paste(items, collapse = ", "), ")") } @@ -146,7 +187,10 @@ render_migration <- function(entry) { # excluded from `air` (see air.toml) so this layout is canonical. fmls <- render_formals(entry) fmls_lines <- paste0(" ", fmls) - fmls_lines[-length(fmls_lines)] <- paste0(fmls_lines[-length(fmls_lines)], ",") + fmls_lines[-length(fmls_lines)] <- paste0( + fmls_lines[-length(fmls_lines)], + "," + ) signature <- c( paste0("handle_args_", fn, " <- function("), fmls_lines, @@ -225,8 +269,14 @@ generate_migrations <- function(registry_path, out_dir) { # Remove stale generated files first so deleted registry entries don't leave # orphaned helpers behind. - stale <- list.files(out_dir, pattern = "^handle-args-.*\\.R$", full.names = TRUE) - if (length(stale)) file.remove(stale) + stale <- list.files( + out_dir, + pattern = "^handle-args-.*\\.R$", + full.names = TRUE + ) + if (length(stale)) { + file.remove(stale) + } fns <- vapply(migrations, function(e) e$fn, character(1)) migrations <- migrations[order(fns)] diff --git a/tools/migrations.R b/tools/migrations.R index 742b2a13a7e..e366319a59f 100644 --- a/tools/migrations.R +++ b/tools/migrations.R @@ -78,11 +78,11 @@ migrations <- list( # Old signature: f(a, b, c, d); new: f(a, b, ..., c_renamed, d) with c renamed # to c_renamed. Consumed by tests/testthat/test-handle-args.R. list( - fn = "migration_fixture", - old = c("a", "b", "c", "d"), - new = c("a", "b", "...", "c_renamed", "d"), - renames = c(c = "c_renamed"), + fn = "migration_fixture", + old = c("a", "b", "c", "d"), + new = c("a", "b", "...", "c_renamed", "d"), + renames = c(c = "c_renamed"), defaults = list(c_renamed = "NULL", d = "NULL"), - when = "3.0.0" + when = "3.0.0" ) ) From 8eb66699bc14ccf587de0bb310481adab2b89c5e Mon Sep 17 00:00:00 2001 From: David Schoch Date: Mon, 1 Jun 2026 22:12:01 +0200 Subject: [PATCH 03/12] feat: declare migrations as function signatures; recover named args Rework the migration tooling per review on #2684. - Registry entries now declare `old`/`new` as literal function signatures; renames are inferred from bare-symbol defaults (`c = c_renamed`) and defaults are read off the `new` formals. Less typing, no parallel renames/defaults bookkeeping. - Generated helpers now recover legacy calls by position *and* by (partial) name: a renamed-away old name (`attr =`) or an abbreviation of a new arg (`weig =`) is matched via charmatch(); ambiguous prefixes error. - Deprecation message shows the detected vs. requested signature side by side and drops the "pre-3.0.0" phrasing; build it with paste0(). Co-Authored-By: Claude Opus 4.8 --- R/handle-args-migration_fixture.R | 112 +++++++++----- tests/testthat/test-handle-args.R | 80 ++++++++-- tools/generate-migrations.R | 239 +++++++++++++++++++++--------- tools/migrations.R | 97 ++++++------ 4 files changed, 355 insertions(+), 173 deletions(-) diff --git a/R/handle-args-migration_fixture.R b/R/handle-args-migration_fixture.R index 5ef68ed36c0..75da9d710a2 100644 --- a/R/handle-args-migration_fixture.R +++ b/R/handle-args-migration_fixture.R @@ -2,10 +2,10 @@ # Regenerate with: Rscript tools/generate-migrations.R # (Excluded from `air` formatting via air.toml so this layout stays canonical.) # -# `migration_fixture()`'s pre-3.0.0 signature took its arguments -# positionally. After `...` was inserted, a legacy positional call lands the -# old arguments in `...`. This helper recovers them, soft-deprecates the -# positional form, and returns the new-API bindings as a named list. +# `migration_fixture()` gained a `...` in its signature. A call written +# against the old signature lands its arguments in `...`; this helper +# recovers them by position or by (partial) name, soft-deprecates the old +# form, and returns the new-API bindings as a named list. # # `.user_env` is threaded into `lifecycle::deprecate_soft()` so the warning is # attributed to the user's call instead of being suppressed as @@ -27,52 +27,96 @@ handle_args_migration_fixture <- function( return(bindings) } - # Any *named* dot is an unknown keyword (future extension or a typo); enforce - # the strict contract. Only a purely positional `...` is treated as a legacy - # call and recovered below. - if (any(nzchar(rlang::names2(dots)))) { - rlang::check_dots_empty(call = .user_env) - } - + # Maps derived from the old/new signatures. recover_new <- c("c_renamed", "d") recover_old <- c("c", "d") + match_names <- c("c", "c_renamed", "d") + match_to <- c("c_renamed", "c_renamed", "d") defaults <- list(c_renamed = NULL, d = NULL) + head_args <- c("a", "b") + fn_name <- "migration_fixture" - if (length(dots) > length(recover_new)) { - cli::cli_abort("Too many arguments passed to {.fn migration_fixture}.", call = .user_env) - } + dot_names <- rlang::names2(dots) + rebound_old <- character() # old labels, in the order encountered + rebound_new <- character() # resolved new names, same order + unknown <- character() + pos <- 0L - for (i in seq_along(dots)) { - new_name <- recover_new[[i]] - # Conflict: the same logical slot was supplied positionally *and* by its - # new keyword (i.e. the keyword differs from its formal default). - if (!identical(bindings[[new_name]], defaults[[new_name]])) { + for (k in seq_along(dots)) { + nm <- dot_names[[k]] + if (nzchar(nm)) { + # Named (possibly abbreviated): partial-match against recoverable names. + j <- charmatch(nm, match_names) + if (is.na(j)) { + unknown <- c(unknown, nm) + next + } + if (j == 0L) { + cli::cli_abort( + "Argument {.arg {nm}} matches multiple arguments of {.fn {fn_name}}.", + call = .user_env + ) + } + new_name <- match_to[[j]] + old_label <- match_names[[j]] + } else { + # Unnamed: recover by position into the next old slot beyond the head. + pos <- pos + 1L + if (pos > length(recover_new)) { + cli::cli_abort( + "Too many arguments passed to {.fn {fn_name}}.", + call = .user_env + ) + } + new_name <- recover_new[[pos]] + old_label <- recover_old[[pos]] + } + + # Conflict: the same new arg was already rebound, or was also supplied by + # its new keyword (i.e. differs from its formal default). + if (new_name %in% rebound_new || + (new_name %in% names(defaults) && + !identical(bindings[[new_name]], defaults[[new_name]]))) { cli::cli_abort( c( - "Can't supply {.arg {recover_old[[i]]}} both positionally and as {.arg {new_name}}.", - i = "Drop the positional value and keep {.code {new_name} = }." + "Argument {.arg {new_name}} of {.fn {fn_name}} was supplied more than once.", + i = "Pass it exactly once, by its new name {.arg {new_name}}." ), call = .user_env ) } - bindings[[new_name]] <- dots[[i]] + + bindings[[new_name]] <- dots[[k]] + rebound_old <- c(rebound_old, old_label) + rebound_new <- c(rebound_new, new_name) } - # One consolidated soft-deprecation per call. Key the message on the first - # renamed slot if there is one, else the first recovered slot. - rebound <- seq_along(dots) - renamed <- rebound[recover_old[rebound] != recover_new[rebound]] - primary <- if (length(renamed)) renamed[[1L]] else 1L + if (length(unknown)) { + cli::cli_abort( + c( + "Unexpected argument(s) passed to {.fn {fn_name}}: {.arg {unknown}}.", + i = "Arguments after {.arg ...} must be spelled out in full." + ), + call = .user_env + ) + } + + # One consolidated soft-deprecation per call, showing the detected legacy + # call next to the recommended keyword form. + detected <- paste0( + fn_name, "(", paste(c(head_args, rebound_old), collapse = ", "), ")" + ) + requested <- paste0( + fn_name, "(", + paste(c(head_args, paste0(rebound_new, " = ")), collapse = ", "), + ")" + ) lifecycle::deprecate_soft( when = "3.0.0", - what = sprintf("migration_fixture(%s)", recover_old[[primary]]), - with = if (recover_old[[primary]] != recover_new[[primary]]) sprintf("migration_fixture(%s)", recover_new[[primary]]), + what = I(paste0("Calling `", fn_name, "()` with positional or abbreviated arguments")), details = c( - "Positional use of the pre-3.0.0 signature of `migration_fixture()` is deprecated.", - i = paste0( - "Name the argument(s) instead: ", - toString(sprintf("`%s = `", recover_new[rebound])), "." - ) + i = paste0("Detected call: ", detected), + i = paste0("Use instead: ", requested) ), user_env = .user_env ) diff --git a/tests/testthat/test-handle-args.R b/tests/testthat/test-handle-args.R index 46142241833..cba67904eef 100644 --- a/tests/testthat/test-handle-args.R +++ b/tests/testthat/test-handle-args.R @@ -18,13 +18,6 @@ test_that("empty dots fall back to the formal defaults", { ) }) -test_that("a named extension in `...` errors via check_dots_empty()", { - expect_error( - handle_args_migration_fixture(1, 2, c_renamed = 3, d = 4, foo = 5), - class = "rlib_error_dots_nonempty" - ) -}) - test_that("a legacy positional call is recovered with one soft-deprecation", { rlang::local_options(lifecycle_verbosity = "warning") @@ -44,7 +37,25 @@ test_that("a legacy positional call recovering a single slot still works", { expect_equal(res, list(a = 1, b = 2, c_renamed = 99, d = NULL)) }) -test_that("recovery only fires one deprecation warning, not one per slot", { +test_that("a renamed-away old name is recovered by name", { + rlang::local_options(lifecycle_verbosity = "warning") + + lifecycle::expect_deprecated( + res <- handle_args_migration_fixture(1, 2, c = 99) + ) + expect_equal(res, list(a = 1, b = 2, c_renamed = 99, d = NULL)) +}) + +test_that("an abbreviation of a new arg is recovered by partial match", { + rlang::local_options(lifecycle_verbosity = "warning") + + lifecycle::expect_deprecated( + res <- handle_args_migration_fixture(1, 2, c_r = 42) + ) + expect_equal(res, list(a = 1, b = 2, c_renamed = 42, d = NULL)) +}) + +test_that("recovery emits a single deprecation warning, not one per slot", { rlang::local_options(lifecycle_verbosity = "warning") warnings <- character() @@ -58,10 +69,24 @@ test_that("recovery only fires one deprecation warning, not one per slot", { expect_length(warnings, 1L) }) +test_that("an unknown named argument errors", { + expect_error( + handle_args_migration_fixture(1, 2, foo = 5), + "Unexpected argument" + ) +}) + test_that("supplying a slot both positionally and by its new keyword errors", { expect_error( handle_args_migration_fixture(1, 2, 3, c_renamed = "kw"), - "both positionally and as" + "supplied more than once" + ) +}) + +test_that("supplying a slot by two different names errors", { + expect_error( + handle_args_migration_fixture(1, 2, c = 1, c_renamed = 2), + "supplied more than once" ) }) @@ -100,10 +125,12 @@ test_that(".user_env controls whether the soft-deprecation surfaces", { expect_equal(res, list(a = 1, b = 2, c_renamed = 3, d = 4)) }) +# ---- generator-level tests (source checkout only) -------------------------- + test_that("the generated helper is in sync with the registry", { # tools/ is excluded from the built package (.Rbuildignore), so this only runs - # from a source checkout (local dev + CI). CI also guards drift via - # .github/workflows/check-migrations.yaml. + # from a source checkout (local dev + CI). The rcc workflow guards drift with + # its own `git diff` step. generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") skip_if_not(file.exists(generator)) @@ -119,3 +146,34 @@ test_that("the generated helper is in sync with the registry", { ) expect_identical(committed, expected) }) + +test_that("an abbreviation matching multiple arguments errors", { + # Built from an ad-hoc registry so we don't ship a second fixture helper. + generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") + skip_if_not(file.exists(generator)) + + gen_env <- new.env() + sys.source(generator, envir = gen_env) + + tmp_reg <- withr::local_tempfile(fileext = ".R") + writeLines( + c( + "migrations <- list(", + " amb_fixture = list(", + " old = function(x, alpha, alphabet) {},", + " new = function(x, ..., alpha = NULL, alphabet = NULL) {},", + " when = \"3.0.0\"", + " )", + ")" + ), + tmp_reg + ) + out <- withr::local_tempdir() + suppressMessages(gen_env$generate_migrations(tmp_reg, out)) + sys.source(file.path(out, "handle-args-amb_fixture.R"), envir = environment()) + + expect_error( + handle_args_amb_fixture(1, alph = 2), + "matches multiple" + ) +}) diff --git a/tools/generate-migrations.R b/tools/generate-migrations.R index c401add1f42..0b6689897fc 100644 --- a/tools/generate-migrations.R +++ b/tools/generate-migrations.R @@ -4,15 +4,16 @@ # # Reads the declarative registry in tools/migrations.R and, for each entry, # emits one self-contained internal helper `handle_args_()` at -# R/handle-args-.R. The generated helpers recover legacy positional calls -# made against a function's pre-migration signature after `...` was inserted. +# R/handle-args-.R. The generated helpers recover legacy calls made against +# a function's pre-migration signature after `...` was inserted -- both +# positional values and (partially) named ones. # # Usage: # Rscript tools/generate-migrations.R # -# The output is deterministic (entries sorted alphabetically by `fn`) and -# idempotent (running twice produces no diff). A CI check regenerates and fails -# on drift; see .github/workflows/check-migrations.yaml. +# The output is deterministic (entries sorted alphabetically by function name) +# and idempotent (running twice produces no diff). A testthat helper regenerates +# automatically when the registry is newer; CI fails on any uncommitted drift. # # Base R only -- no package needs to be loaded to run this. @@ -28,7 +29,7 @@ migration_paths <- function() { ) } -# ---- registry loading + validation ---------------------------------------- +# ---- registry loading + normalisation -------------------------------------- load_migrations <- function(registry_path) { env <- new.env(parent = baseenv()) @@ -42,32 +43,79 @@ load_migrations <- function(registry_path) { ) } migrations <- get("migrations", envir = env) - lapply(migrations, validate_migration) + fns <- names(migrations) + if (is.null(fns) || any(!nzchar(fns))) { + stop("`migrations` must be a list named by function.", call. = FALSE) + } + Map(normalise_migration, fns, migrations) } -validate_migration <- function(entry) { - req <- c("fn", "old", "new") - missing <- setdiff(req, names(entry)) - if (length(missing)) { - stop( - "Migration entry is missing field(s): ", - paste(missing, collapse = ", "), - call. = FALSE - ) +# Deparse a formal's default, force-safely. A formal with no default holds R's +# "missing argument" sentinel, which errors the moment it is forced (e.g. by +# `is.symbol()`); `deparse()` tolerates it and yields "". So we deparse first to +# detect missingness, and only inspect the value once we know it is present. +default_expr <- function(fmls, nm) { + paste(deparse(fmls[[nm]], width.cutoff = 500L), collapse = " ") +} + +# Turn one registry entry (with `old`/`new` as function objects) into the flat +# structure the renderer consumes. +normalise_migration <- function(fn, entry) { + for (field in c("old", "new")) { + if (!is.function(entry[[field]])) { + stop( + "Migration `", + fn, + "`: `", + field, + "` must be a function.", + call. = FALSE + ) + } } - entry$renames <- entry$renames %||% character(0) - entry$defaults <- entry$defaults %||% list() + entry$fn <- fn entry$when <- entry$when %||% "3.0.0" + old_fmls <- formals(entry$old) + new_fmls <- formals(entry$new) + entry$old <- names(old_fmls) + entry$new <- names(new_fmls) + if (sum(entry$new == "...") != 1L) { stop( "Migration `", - entry$fn, - "`: `new` must contain exactly one \"...\" marker.", + fn, + "`: `new` must contain exactly one `...`.", call. = FALSE ) } + # Renames: an old formal whose default is a bare symbol points at its new name. + renames <- character(0) + for (nm in entry$old) { + if (!nzchar(default_expr(old_fmls, nm))) { + next + } # no default -> no rename + default <- old_fmls[[nm]] # safe to force now that we know it is present + if (is.symbol(default)) { + renames[[nm]] <- as.character(default) + } + } + entry$renames <- renames + + # Defaults of the new-API args, deparsed back to expressions. + defaults <- list() + for (nm in entry$new) { + if (nm == "...") { + next + } + expr <- default_expr(new_fmls, nm) + if (nzchar(expr)) { + defaults[[nm]] <- expr + } + } + entry$defaults <- defaults + dots_idx <- which(entry$new == "...") entry$head <- entry$new[seq_len(dots_idx - 1L)] entry$tail <- if (dots_idx < length(entry$new)) { @@ -80,12 +128,12 @@ validate_migration <- function(entry) { if (length(entry$old) < length(entry$head)) { stop( "Migration `", - entry$fn, + fn, "`: `old` is shorter than the head args of `new`.", call. = FALSE ) } - # Old positional slots beyond the head are the recoverable ones. + # Old positional slots beyond the head are recovered by position. entry$recover_old <- if (length(entry$old) > length(entry$head)) { entry$old[(length(entry$head) + 1L):length(entry$old)] } else { @@ -94,22 +142,31 @@ validate_migration <- function(entry) { entry$recover_new <- vapply( entry$recover_old, function(nm) { - if (nm %in% names(entry$renames)) unname(entry$renames[[nm]]) else nm + if (nm %in% names(entry$renames)) entry$renames[[nm]] else nm }, - character(1) + character(1), + USE.NAMES = FALSE ) bad <- setdiff(entry$recover_new, entry$tail) if (length(bad)) { stop( "Migration `", - entry$fn, + fn, "`: recovered slot(s) ", paste(bad, collapse = ", "), " do not appear after `...` in `new`.", call. = FALSE ) } + + # Names recoverable from `...`: renamed-away old names (no longer formals) plus + # the new tail names (so abbreviations of the new args are matched too). Each + # entry records where it resolves in the new API. + renamed <- entry$recover_old != entry$recover_new + entry$match_names <- c(entry$recover_old[renamed], entry$tail) + entry$match_to <- c(entry$recover_new[renamed], entry$tail) + entry } @@ -117,9 +174,7 @@ validate_migration <- function(entry) { # ---- rendering ------------------------------------------------------------- -# Build the formals list of the generated helper as a single string. render_formals <- function(entry) { - head_fmls <- entry$head tail_fmls <- vapply( entry$tail, function(nm) { @@ -131,10 +186,9 @@ render_formals <- function(entry) { }, character(1) ) - c(head_fmls, "...", tail_fmls, ".user_env = rlang::caller_env()") + c(entry$head, "...", tail_fmls, ".user_env = rlang::caller_env()") } -# Render a character vector literal, e.g. c("a", "b"). render_chr_vec <- function(x) { if (length(x) == 0L) { return("character(0)") @@ -142,22 +196,15 @@ render_chr_vec <- function(x) { paste0("c(", paste0('"', x, '"', collapse = ", "), ")") } -# Render the `defaults` list literal restricted to recoverable slots, reusing -# the registry's default *expressions* so the runtime identity check compares -# against the true formal default. render_defaults_list <- function(entry) { - if (length(entry$recover_new) == 0L) { + keep <- intersect(entry$tail, names(entry$defaults)) + if (length(keep) == 0L) { return("list()") } items <- vapply( - entry$recover_new, + keep, function(nm) { - expr <- if (nm %in% names(entry$defaults)) { - entry$defaults[[nm]] - } else { - "NULL" - } - paste0(nm, " = ", expr) + paste0(nm, " = ", entry$defaults[[nm]]) }, character(1) ) @@ -173,10 +220,10 @@ render_migration <- function(entry) { "# Regenerate with: Rscript tools/generate-migrations.R", "# (Excluded from `air` formatting via air.toml so this layout stays canonical.)", "#", - paste0("# `", fn, "()`'s pre-", entry$when, " signature took its arguments"), - "# positionally. After `...` was inserted, a legacy positional call lands the", - "# old arguments in `...`. This helper recovers them, soft-deprecates the", - "# positional form, and returns the new-API bindings as a named list.", + paste0("# `", fn, "()` gained a `...` in its signature. A call written"), + "# against the old signature lands its arguments in `...`; this helper", + "# recovers them by position or by (partial) name, soft-deprecates the old", + "# form, and returns the new-API bindings as a named list.", "#", "# `.user_env` is threaded into `lifecycle::deprecate_soft()` so the warning is", "# attributed to the user's call instead of being suppressed as", @@ -205,52 +252,96 @@ render_migration <- function(entry) { " return(bindings)", " }", "", - " # Any *named* dot is an unknown keyword (future extension or a typo); enforce", - " # the strict contract. Only a purely positional `...` is treated as a legacy", - " # call and recovered below.", - " if (any(nzchar(rlang::names2(dots)))) {", - " rlang::check_dots_empty(call = .user_env)", - " }", - "", + " # Maps derived from the old/new signatures.", paste0(" recover_new <- ", render_chr_vec(entry$recover_new)), paste0(" recover_old <- ", render_chr_vec(entry$recover_old)), + paste0(" match_names <- ", render_chr_vec(entry$match_names)), + paste0(" match_to <- ", render_chr_vec(entry$match_to)), paste0(" defaults <- ", render_defaults_list(entry)), + paste0(" head_args <- ", render_chr_vec(entry$head)), + paste0(" fn_name <- \"", fn, "\""), "", - " if (length(dots) > length(recover_new)) {", - paste0(" cli::cli_abort(\"Too many arguments passed to {.fn ", fn, "}.\", call = .user_env)"), - " }", + " dot_names <- rlang::names2(dots)", + " rebound_old <- character() # old labels, in the order encountered", + " rebound_new <- character() # resolved new names, same order", + " unknown <- character()", + " pos <- 0L", + "", + " for (k in seq_along(dots)) {", + " nm <- dot_names[[k]]", + " if (nzchar(nm)) {", + " # Named (possibly abbreviated): partial-match against recoverable names.", + " j <- charmatch(nm, match_names)", + " if (is.na(j)) {", + " unknown <- c(unknown, nm)", + " next", + " }", + " if (j == 0L) {", + " cli::cli_abort(", + " \"Argument {.arg {nm}} matches multiple arguments of {.fn {fn_name}}.\",", + " call = .user_env", + " )", + " }", + " new_name <- match_to[[j]]", + " old_label <- match_names[[j]]", + " } else {", + " # Unnamed: recover by position into the next old slot beyond the head.", + " pos <- pos + 1L", + " if (pos > length(recover_new)) {", + " cli::cli_abort(", + " \"Too many arguments passed to {.fn {fn_name}}.\",", + " call = .user_env", + " )", + " }", + " new_name <- recover_new[[pos]]", + " old_label <- recover_old[[pos]]", + " }", "", - " for (i in seq_along(dots)) {", - " new_name <- recover_new[[i]]", - " # Conflict: the same logical slot was supplied positionally *and* by its", - " # new keyword (i.e. the keyword differs from its formal default).", - " if (!identical(bindings[[new_name]], defaults[[new_name]])) {", + " # Conflict: the same new arg was already rebound, or was also supplied by", + " # its new keyword (i.e. differs from its formal default).", + " if (new_name %in% rebound_new ||", + " (new_name %in% names(defaults) &&", + " !identical(bindings[[new_name]], defaults[[new_name]]))) {", " cli::cli_abort(", " c(", - " \"Can't supply {.arg {recover_old[[i]]}} both positionally and as {.arg {new_name}}.\",", - " i = \"Drop the positional value and keep {.code {new_name} = }.\"", + " \"Argument {.arg {new_name}} of {.fn {fn_name}} was supplied more than once.\",", + " i = \"Pass it exactly once, by its new name {.arg {new_name}}.\"", " ),", " call = .user_env", " )", " }", - " bindings[[new_name]] <- dots[[i]]", + "", + " bindings[[new_name]] <- dots[[k]]", + " rebound_old <- c(rebound_old, old_label)", + " rebound_new <- c(rebound_new, new_name)", + " }", + "", + " if (length(unknown)) {", + " cli::cli_abort(", + " c(", + " \"Unexpected argument(s) passed to {.fn {fn_name}}: {.arg {unknown}}.\",", + " i = \"Arguments after {.arg ...} must be spelled out in full.\"", + " ),", + " call = .user_env", + " )", " }", "", - " # One consolidated soft-deprecation per call. Key the message on the first", - " # renamed slot if there is one, else the first recovered slot.", - " rebound <- seq_along(dots)", - " renamed <- rebound[recover_old[rebound] != recover_new[rebound]]", - " primary <- if (length(renamed)) renamed[[1L]] else 1L", + " # One consolidated soft-deprecation per call, showing the detected legacy", + " # call next to the recommended keyword form.", + " detected <- paste0(", + " fn_name, \"(\", paste(c(head_args, rebound_old), collapse = \", \"), \")\"", + " )", + " requested <- paste0(", + " fn_name, \"(\",", + " paste(c(head_args, paste0(rebound_new, \" = \")), collapse = \", \"),", + " \")\"", + " )", " lifecycle::deprecate_soft(", paste0(" when = \"", entry$when, "\","), - paste0(" what = sprintf(\"", fn, "(%s)\", recover_old[[primary]]),"), - paste0(" with = if (recover_old[[primary]] != recover_new[[primary]]) sprintf(\"", fn, "(%s)\", recover_new[[primary]]),"), + paste0(" what = I(paste0(\"Calling `\", fn_name, \"()` with positional or abbreviated arguments\")),"), " details = c(", - paste0(" \"Positional use of the pre-", entry$when, " signature of `", fn, "()` is deprecated.\","), - " i = paste0(", - " \"Name the argument(s) instead: \",", - " toString(sprintf(\"`%s = `\", recover_new[rebound])), \".\"", - " )", + " i = paste0(\"Detected call: \", detected),", + " i = paste0(\"Use instead: \", requested)", " ),", " user_env = .user_env", " )", diff --git a/tools/migrations.R b/tools/migrations.R index e366319a59f..b00eac9c835 100644 --- a/tools/migrations.R +++ b/tools/migrations.R @@ -8,81 +8,70 @@ # # Rscript tools/generate-migrations.R # -# --------------------------------------------------------------------------- -# Entry schema (a named list per migration): +# (A testthat helper also regenerates them automatically when this file is newer +# than the generated helpers; see tests/testthat/helper-migrations.R.) # -# fn Character scalar. Name of the public function being migrated. -# The generated helper is `handle_args_()`. +# --------------------------------------------------------------------------- +# Entry schema # -# old Character vector. The positional argument order *before* the -# migration, e.g. c("graph", "types", "attr", "names", "sparse"). +# `migrations` is a list named by function. Each entry declares the function's +# signature *before* and *after* the migration as literal R function objects -- +# renames and defaults are read straight off their formals: # -# new Character vector. The positional order *after* the migration, -# including the literal "..." marker showing where dots were -# inserted, e.g. -# c("graph", "types", "...", "weights", "attr", "names", "sparse"). -# The non-"..." names are the new-API formals, in order. +# = list( +# old = function() {}, +# new = function() {}, +# when = "" +# ) # -# renames Named character vector mapping old name -> new name, for the -# arguments that were renamed. Args that kept their name are omitted. -# e.g. c(attr = "weights"). Default: character(0) (pure reorder/none). +# old The pre-migration signature. The *order* of its formals is the old +# positional order. A formal whose default is a **bare symbol** declares +# a rename to that name, e.g. `c = c_renamed` means the old `c` argument +# is the new `c_renamed`. Formals without a symbol default keep their +# name. # -# defaults Named list mapping new-API arg name -> a *string* holding the R -# expression used as that arg's formal default. Args present here get -# `= ` in the generated signature; args absent are required -# (no default). The default value is also what the helper compares -# against (via identical()) to decide whether the user explicitly -# passed a keyword that would conflict with a recovered positional. -# e.g. list(weights = "NULL", attr = "lifecycle::deprecated()"). +# new The post-migration signature. Must contain exactly one `...`. The +# non-`...` formals are the new-API arguments, in order; their defaults +# become the generated helper's defaults (and the values the conflict +# check compares against). # -# when Character scalar. The version string passed to -# `lifecycle::deprecate_soft(when = )`, e.g. "3.0.0". +# when The version string passed to `lifecycle::deprecate_soft(when = )`. # # --------------------------------------------------------------------------- -# Known limitations (documented, not handled -- see tools/README.md): +# How recovery maps old calls onto the new signature # -# * Removed args: if a migration deletes an old positional slot rather than -# renaming it, a legacy positional call silently rebinds to the wrong new -# slot. The generator assumes every old slot beyond the head survives. -# * Reordered args: the generator assumes the new signature preserves the -# relative order of surviving old args (insertions and renames only). -# * Partial matching: pre-`...` R let `f(g, att = "x")` partial-match `attrs`. -# Post-insertion, partial names fall into `...` and are treated as unknown -# named extras (the strict `check_dots_empty()` path), never recovered. +# * Arguments *before* `...` in `new` ("head" args) bind positionally as +# before; they must keep their relative order and names. +# * Old positional slots *beyond* the head are recovered from `...`: an +# unnamed value is mapped by position, a (possibly abbreviated) name by +# partial match. Renamed-away old names and abbreviations of the new names +# are both recovered, all under one soft-deprecation. # -# These are acceptable because the migrations planned for 3.0.0 are pure -# renames with no removals or reorderings. +# To reorder or drop an argument, place it *after* `...` in `new`. It then +# becomes keyword-only and is recovered by (partial) name rather than by +# position, side-stepping the position math entirely. # --------------------------------------------------------------------------- migrations <- list( # No real package function is migrated yet. Real entries are added by the # branch that consumes this infrastructure (see tools/README.md). Example of - # the shape a real entry will take: + # the shape a real entry will take (old `attr` renamed to `weights`): # - # list( - # fn = "as_biadjacency_matrix", - # old = c("graph", "types", "attr", "names", "sparse"), - # new = c("graph", "types", "...", "weights", "attr", "names", "sparse"), - # renames = c(attr = "weights"), - # defaults = list( - # weights = "NULL", - # attr = "lifecycle::deprecated()", - # names = "TRUE", - # sparse = "FALSE" - # ), - # when = "3.0.0" + # as_biadjacency_matrix = list( + # old = function(graph, types, attr = weights, names, sparse) {}, + # new = function(graph, types, ..., weights = NULL, + # attr = lifecycle::deprecated(), names = TRUE, + # sparse = FALSE) {}, + # when = "3.0.0" # ), # --- test fixture -------------------------------------------------------- # Exercises the generator end-to-end without migrating a real function. - # Old signature: f(a, b, c, d); new: f(a, b, ..., c_renamed, d) with c renamed + # Old signature f(a, b, c, d); new f(a, b, ..., c_renamed, d) with c renamed # to c_renamed. Consumed by tests/testthat/test-handle-args.R. - list( - fn = "migration_fixture", - old = c("a", "b", "c", "d"), - new = c("a", "b", "...", "c_renamed", "d"), - renames = c(c = "c_renamed"), - defaults = list(c_renamed = "NULL", d = "NULL"), + migration_fixture = list( + old = function(a, b, c = c_renamed, d) {}, + new = function(a, b, ..., c_renamed = NULL, d = NULL) {}, when = "3.0.0" ) ) From 64a82ca4e5a9b4853cce9dd8b770c8276539a517 Mon Sep 17 00:00:00 2001 From: David Schoch Date: Mon, 1 Jun 2026 22:12:01 +0200 Subject: [PATCH 04/12] ci: auto-regenerate migration helpers and guard drift in rcc Replace the standalone check-migrations workflow with two cheaper hooks: - A testthat helper regenerates R/handle-args-*.R whenever tools/migrations.R (or the generator) is newer, so editing the registry and running the tests refreshes the helpers automatically. - The rcc smoke job regenerates from the source checkout and fails on any resulting git diff, before the auto-commit steps. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/R-CMD-check.yaml | 14 +++++++++ .github/workflows/check-migrations.yaml | 42 ------------------------- tests/testthat/helper-migrations.R | 32 +++++++++++++++++++ 3 files changed, 46 insertions(+), 42 deletions(-) delete mode 100644 .github/workflows/check-migrations.yaml create mode 100644 tests/testthat/helper-migrations.R diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index e39bee746a9..38bcf42bd98 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -132,6 +132,20 @@ jobs: _R_SHLIB_STRIP_=true R CMD INSTALL . shell: bash + # Regenerate the argument-migration helpers from tools/migrations.R and + # fail if they drift. Runs from the source checkout (tools/ is excluded + # from the built package) and before the auto-commit steps below, so a + # contributor who forgot to regenerate gets a hard failure. + - name: Check migration helpers are in sync with the registry + run: | + Rscript tools/generate-migrations.R + if ! git diff --exit-code -- 'R/handle-args-*.R'; then + echo "::error::R/handle-args-*.R is out of sync with tools/migrations.R." + echo "Run 'Rscript tools/generate-migrations.R' and commit the result." + exit 1 + fi + shell: bash + - id: versions-matrix # Only run for: # - pull requests if the base repo is different from the head repo diff --git a/.github/workflows/check-migrations.yaml b/.github/workflows/check-migrations.yaml deleted file mode 100644 index 2b6a9642774..00000000000 --- a/.github/workflows/check-migrations.yaml +++ /dev/null @@ -1,42 +0,0 @@ -# Verifies that the generated argument-migration helpers in R/handle-args-*.R -# are in sync with the registry in tools/migrations.R. If someone edits a -# generated file by hand, or changes the registry without regenerating, this -# fails. Regenerate locally with: Rscript tools/generate-migrations.R -# -# See tools/README.md ("Argument-migration helpers") for the design. -name: check-migrations - -on: - push: - branches: [main, master] - pull_request: - branches: [main, master] - merge_group: - types: [checks_requested] - workflow_dispatch: - -permissions: - contents: read - -jobs: - check-migrations: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v6 - - - uses: r-lib/actions/setup-r@v2 - with: - # The generator is base-R only; no package dependencies are needed. - use-public-rspm: true - - - name: Regenerate migration helpers - run: Rscript tools/generate-migrations.R - - - name: Fail if generated files drift from the registry - run: | - if ! git diff --exit-code -- 'R/handle-args-*.R'; then - echo "::error::R/handle-args-*.R is out of sync with tools/migrations.R." - echo "Run 'Rscript tools/generate-migrations.R' and commit the result." - exit 1 - fi - shell: bash diff --git a/tests/testthat/helper-migrations.R b/tests/testthat/helper-migrations.R new file mode 100644 index 00000000000..d8611770940 --- /dev/null +++ b/tests/testthat/helper-migrations.R @@ -0,0 +1,32 @@ +# Keep the generated argument-migration helpers (R/handle-args-*.R) in sync with +# the registry during development: when tools/migrations.R or the generator is +# newer than the generated files, regenerate them before the tests run. +# +# tools/ is excluded from the built package (.Rbuildignore), so this is a no-op +# under `R CMD check` of the tarball; it only fires from a source checkout +# (devtools::test(), covr). CI additionally guards against committed drift with +# a `git diff` step in the rcc workflow. +local({ + registry <- testthat::test_path("..", "..", "tools", "migrations.R") + generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") + if (!file.exists(registry) || !file.exists(generator)) { + return(invisible()) + } + + out_dir <- testthat::test_path("..", "..", "R") + generated <- list.files( + out_dir, + pattern = "^handle-args-.*\\.R$", + full.names = TRUE + ) + + sources_mtime <- max(file.mtime(c(registry, generator))) + fresh <- length(generated) > 0 && all(file.mtime(generated) >= sources_mtime) + if (fresh) { + return(invisible()) + } + + gen_env <- new.env() + sys.source(generator, envir = gen_env) + suppressMessages(gen_env$generate_migrations(registry, out_dir)) +}) From abed336f218adf83120178d1d70a5c42ceeb435c Mon Sep 17 00:00:00 2001 From: David Schoch Date: Mon, 1 Jun 2026 22:12:01 +0200 Subject: [PATCH 05/12] docs: clarify migration reordering/removal and .user_env - Reframe "removed/reordered args" as a supported workaround: place the affected argument after `...` so it is recovered by name, not position. - Explain why `.user_env` is threaded: deprecate_soft() gates on is_direct(user_env) before the verbosity override, so the helper's extra call frame would otherwise suppress the warning. Co-Authored-By: Claude Opus 4.8 --- tools/README.md | 67 ++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 29 deletions(-) diff --git a/tools/README.md b/tools/README.md index 66403f91cfd..ef656ceb64a 100644 --- a/tools/README.md +++ b/tools/README.md @@ -148,17 +148,19 @@ This shows the remote URL, branch, and commit hashes. ## Argument-migration helpers When a public function's signature is migrated by inserting `...` and renaming -or repositioning arguments, pre-existing **positional** calls would otherwise -break (the old positional values land in `...` and hit `check_dots_empty()`). -To soften that, each migrated function gets an internal `handle_args_()` -helper that recovers a legacy positional call, soft-deprecates the positional -form, and returns the new-API bindings as a named list. +arguments, pre-existing calls would otherwise break: the old positional (or +partially-named) values land in `...` and hit `check_dots_empty()`. To soften +that, each migrated function gets an internal `handle_args_()` helper that +recovers the legacy call — by position **or by (partial) name** — soft-deprecates +the old form, and returns the new-API bindings as a named list. These helpers are **generated**, not written by hand: -- **`tools/migrations.R`** is the single source of truth. It declares one entry - per migrated function (old/new signatures, renames, defaults, lifecycle - `when`). The entry schema is documented at the top of that file. +- **`tools/migrations.R`** is the single source of truth. Each entry declares the + function's `old` and `new` signatures as literal R functions; renames and + defaults are read straight off their formals (a bare-symbol default in `old`, + e.g. `c = c_renamed`, means a rename). The schema is documented at the top of + that file. - **`tools/generate-migrations.R`** reads the registry and emits one `R/handle-args-.R` per entry. Output is deterministic (entries sorted by function name) and idempotent (running twice produces no diff). @@ -169,8 +171,11 @@ Regenerate after editing the registry: Rscript tools/generate-migrations.R ``` -Do **not** edit `R/handle-args-*.R` by hand — `.github/workflows/check-migrations.yaml` -regenerates them in CI and fails on drift. +This usually happens for you: a testthat helper +([`tests/testthat/helper-migrations.R`](../tests/testthat/helper-migrations.R)) +regenerates the helpers whenever the registry is newer, and the `rcc` CI +workflow runs the generator and fails if the committed files have drifted. Do +**not** edit `R/handle-args-*.R` by hand. ### Call-site usage @@ -191,25 +196,29 @@ as_biadjacency_matrix <- function(graph, types, ..., weights = NULL, } ``` -The `.user_env` argument is essential. `lifecycle::deprecate_soft()` only signals -when `is_direct(user_env)` is `TRUE`; a package-internal `user_env` is swallowed -**even under** `lifecycle_verbosity = "warning"`. Threading the *caller's* env -through `.user_env` is what keeps the deprecation visible to users. - -### Known limitations - -The generator handles **pure renames and insertions** only: - -- **Removed args** — deleting an old positional slot (rather than renaming it) - makes a legacy positional call silently rebind to the wrong new slot. Not - detected. -- **Reordered args** — the new signature must preserve the relative order of - surviving old args. Reordering breaks the position math. -- **Partial matching** — pre-`...` R allowed `f(g, att = "x")` to partial-match - `attrs`. Post-insertion, partial names fall into `...` and are treated as - unknown keywords (the strict `check_dots_empty()` path), never recovered. - -These are acceptable because the migrations planned for 3.0.0 are pure renames. +The `.user_env` argument matters because the helper interposes an extra call +frame. `lifecycle::deprecate_soft()` only signals when `is_direct(user_env)` is +`TRUE` — that is, when `topenv(user_env)` is the global env (or under testthat). +Its auto-detected `user_env` would resolve to the helper's caller, the **igraph +namespace**, so the warning is swallowed (this gate runs *before* the +`lifecycle_verbosity = "warning"` override, so forcing verbosity does not bring +it back). Threading the public function's `rlang::caller_env()` through +`.user_env` makes the *user's* frame the reference: real user calls warn, while +genuinely internal igraph callers stay correctly silent — the point of a *soft* +deprecation. + +### Reordering or removing an argument + +Positional recovery only covers old slots whose order is preserved. To reorder +an argument, or to drop a positional slot without breaking old calls, **place +the affected argument after `...`** in the `new` signature. Arguments past the +ellipsis are keyword-only and are recovered by (partial) name rather than by +position, so they side-step the position math entirely. + +The one case the generator cannot rescue is a positional slot that is removed +*and* whose old position is reused by a different surviving argument — a legacy +positional call would then rebind to the wrong slot. No migration planned for +3.0.0 does this (they are pure renames). ## Additional Documentation From 71e7431e5de0a78419ff6f3abd01970ea3768bcd Mon Sep 17 00:00:00 2001 From: David Schoch Date: Tue, 2 Jun 2026 18:39:03 +0200 Subject: [PATCH 06/12] refactor: generate argument migration in place, drop handler + .user_env Per review on #2684, pivot from generated handler functions to in-place codegen. The generator now rewrites the code between `# BEGIN/END GENERATED ARG_HANDLE` markers inside each migrated function body instead of emitting a standalone `handle_args_()`. - The recovery runs inline: an immediately-invoked function does the pure matching (temps stay scoped, no environment access), the host frame reassigns its own locals, and a single `lifecycle::deprecate_soft()` is emitted directly in the function. Its default `user_env` (caller_env(2)) resolves to the user, so the `.user_env` parameter and all the env threading are gone. - Output is laid out exactly as `air` formats it, so the host files stay clean (no air.toml exclusion needed; the old one is dropped). - Registry: document that old-side default values are ignored (default changes live in `new`); note the bare-symbol-default edge. - Fixture moves to R/migration-fixture.R (a marker-carrying function); the old R/handle-args-migration_fixture.R is removed. Co-Authored-By: Claude Opus 4.8 --- R/handle-args-migration_fixture.R | 126 ------------- R/migration-fixture.R | 107 +++++++++++ air.toml | 2 +- tests/testthat/test-handle-args.R | 114 ++++++----- tools/generate-migrations.R | 303 ++++++++++++++++-------------- tools/migrations.R | 21 ++- 6 files changed, 343 insertions(+), 330 deletions(-) delete mode 100644 R/handle-args-migration_fixture.R create mode 100644 R/migration-fixture.R diff --git a/R/handle-args-migration_fixture.R b/R/handle-args-migration_fixture.R deleted file mode 100644 index 75da9d710a2..00000000000 --- a/R/handle-args-migration_fixture.R +++ /dev/null @@ -1,126 +0,0 @@ -# Generated by tools/generate-migrations.R from tools/migrations.R — do not edit by hand. -# Regenerate with: Rscript tools/generate-migrations.R -# (Excluded from `air` formatting via air.toml so this layout stays canonical.) -# -# `migration_fixture()` gained a `...` in its signature. A call written -# against the old signature lands its arguments in `...`; this helper -# recovers them by position or by (partial) name, soft-deprecates the old -# form, and returns the new-API bindings as a named list. -# -# `.user_env` is threaded into `lifecycle::deprecate_soft()` so the warning is -# attributed to the user's call instead of being suppressed as -# package-internal. Public callers must pass `.user_env = rlang::caller_env()`. - -#' @noRd -handle_args_migration_fixture <- function( - a, - b, - ..., - c_renamed = NULL, - d = NULL, - .user_env = rlang::caller_env() -) { - bindings <- list(a = a, b = b, c_renamed = c_renamed, d = d) - - dots <- list(...) - if (length(dots) == 0L) { - return(bindings) - } - - # Maps derived from the old/new signatures. - recover_new <- c("c_renamed", "d") - recover_old <- c("c", "d") - match_names <- c("c", "c_renamed", "d") - match_to <- c("c_renamed", "c_renamed", "d") - defaults <- list(c_renamed = NULL, d = NULL) - head_args <- c("a", "b") - fn_name <- "migration_fixture" - - dot_names <- rlang::names2(dots) - rebound_old <- character() # old labels, in the order encountered - rebound_new <- character() # resolved new names, same order - unknown <- character() - pos <- 0L - - for (k in seq_along(dots)) { - nm <- dot_names[[k]] - if (nzchar(nm)) { - # Named (possibly abbreviated): partial-match against recoverable names. - j <- charmatch(nm, match_names) - if (is.na(j)) { - unknown <- c(unknown, nm) - next - } - if (j == 0L) { - cli::cli_abort( - "Argument {.arg {nm}} matches multiple arguments of {.fn {fn_name}}.", - call = .user_env - ) - } - new_name <- match_to[[j]] - old_label <- match_names[[j]] - } else { - # Unnamed: recover by position into the next old slot beyond the head. - pos <- pos + 1L - if (pos > length(recover_new)) { - cli::cli_abort( - "Too many arguments passed to {.fn {fn_name}}.", - call = .user_env - ) - } - new_name <- recover_new[[pos]] - old_label <- recover_old[[pos]] - } - - # Conflict: the same new arg was already rebound, or was also supplied by - # its new keyword (i.e. differs from its formal default). - if (new_name %in% rebound_new || - (new_name %in% names(defaults) && - !identical(bindings[[new_name]], defaults[[new_name]]))) { - cli::cli_abort( - c( - "Argument {.arg {new_name}} of {.fn {fn_name}} was supplied more than once.", - i = "Pass it exactly once, by its new name {.arg {new_name}}." - ), - call = .user_env - ) - } - - bindings[[new_name]] <- dots[[k]] - rebound_old <- c(rebound_old, old_label) - rebound_new <- c(rebound_new, new_name) - } - - if (length(unknown)) { - cli::cli_abort( - c( - "Unexpected argument(s) passed to {.fn {fn_name}}: {.arg {unknown}}.", - i = "Arguments after {.arg ...} must be spelled out in full." - ), - call = .user_env - ) - } - - # One consolidated soft-deprecation per call, showing the detected legacy - # call next to the recommended keyword form. - detected <- paste0( - fn_name, "(", paste(c(head_args, rebound_old), collapse = ", "), ")" - ) - requested <- paste0( - fn_name, "(", - paste(c(head_args, paste0(rebound_new, " = ")), collapse = ", "), - ")" - ) - lifecycle::deprecate_soft( - when = "3.0.0", - what = I(paste0("Calling `", fn_name, "()` with positional or abbreviated arguments")), - details = c( - i = paste0("Detected call: ", detected), - i = paste0("Use instead: ", requested) - ), - user_env = .user_env - ) - - bindings -} - diff --git a/R/migration-fixture.R b/R/migration-fixture.R new file mode 100644 index 00000000000..66bf7440cce --- /dev/null +++ b/R/migration-fixture.R @@ -0,0 +1,107 @@ +# Test fixture for the in-place argument-migration generator (tools/migrations.R, +# tools/generate-migrations.R). `migration_fixture()` carries a generated +# ARG_HANDLE block that recovers a legacy call to its pre-3.0.0 signature +# f(a, b, c, d) -- now f(a, b, ..., c_renamed, d) with c renamed to c_renamed. +# It exists only to exercise the generator end-to-end; see +# tests/testthat/test-handle-args.R. + +#' @noRd +migration_fixture <- function(a, b, ..., c_renamed = NULL, d = NULL) { + # BEGIN GENERATED ARG_HANDLE: migration_fixture + # Generated from tools/migrations.R by tools/generate-migrations.R -- do not edit. + # Recovers a legacy call to the pre-migration signature; regenerate with + # `Rscript tools/generate-migrations.R`. + .arg_handle <- (function(dots, current) { + if (length(dots) == 0L) { + return(NULL) + } + recover_new <- c("c_renamed", "d") + recover_old <- c("c", "d") + match_names <- c("c", "c_renamed", "d") + match_to <- c("c_renamed", "c_renamed", "d") + defaults <- list(c_renamed = NULL, d = NULL) + head_args <- c("a", "b") + fn_name <- "migration_fixture" + dot_names <- rlang::names2(dots) + values <- list() + rebound_old <- character() + rebound_new <- character() + pos <- 0L + for (k in seq_along(dots)) { + nm <- dot_names[[k]] + if (nzchar(nm)) { + # Named (possibly abbreviated): partial-match recoverable names. + j <- charmatch(nm, match_names) + if (is.na(j)) { + cli::cli_abort(c( + "Unexpected argument passed to {.fn {fn_name}}: {.arg {nm}}.", + i = "Arguments after {.arg ...} must be spelled out in full." + )) + } + if (j == 0L) { + cli::cli_abort( + "Argument {.arg {nm}} matches multiple arguments of {.fn {fn_name}}." + ) + } + new_name <- match_to[[j]] + old_label <- match_names[[j]] + } else { + # Unnamed: recover by position into the next old slot past the head. + pos <- pos + 1L + if (pos > length(recover_new)) { + cli::cli_abort("Too many arguments passed to {.fn {fn_name}}.") + } + new_name <- recover_new[[pos]] + old_label <- recover_old[[pos]] + } + duplicated <- new_name %in% rebound_new + has_default <- new_name %in% names(defaults) + reassigned <- has_default && + !identical(current[[new_name]], defaults[[new_name]]) + if (duplicated || reassigned) { + cli::cli_abort(c( + "Argument {.arg {new_name}} of {.fn {fn_name}} was supplied more than once.", + i = "Pass it exactly once, by its new name {.arg {new_name}}." + )) + } + values[[new_name]] <- dots[[k]] + rebound_old <- c(rebound_old, old_label) + rebound_new <- c(rebound_new, new_name) + } + detected <- paste0( + fn_name, + "(", + paste(c(head_args, rebound_old), collapse = ", "), + ")" + ) + requested <- paste0( + fn_name, + "(", + paste(c(head_args, paste0(rebound_new, " = ")), collapse = ", "), + ")" + ) + list( + values = values, + what = paste0( + "Calling `", + fn_name, + "()` with positional or abbreviated arguments" + ), + details = c( + i = paste0("Detected call: ", detected), + i = paste0("Use instead: ", requested) + ) + ) + })(list(...), list(c_renamed = c_renamed, d = d)) + if (!is.null(.arg_handle)) { + list2env(.arg_handle$values, environment()) + lifecycle::deprecate_soft( + "3.0.0", + what = I(.arg_handle$what), + details = .arg_handle$details + ) + } + # END GENERATED ARG_HANDLE + + list(a = a, b = b, c_renamed = c_renamed, d = d) +} diff --git a/air.toml b/air.toml index c56a6915daa..58d5e7dcd80 100644 --- a/air.toml +++ b/air.toml @@ -5,5 +5,5 @@ indent-style = "space" line-ending = "auto" persistent-line-breaks = true default-exclude = true -exclude = ["R/utils-s3.R", "R/handle-args-*.R"] +exclude = ["R/utils-s3.R"] skip = ["tribble", "graph_from_literal", "matrix", "c", "make_graph", "edges"] diff --git a/tests/testthat/test-handle-args.R b/tests/testthat/test-handle-args.R index cba67904eef..c81e86bce0a 100644 --- a/tests/testthat/test-handle-args.R +++ b/tests/testthat/test-handle-args.R @@ -1,19 +1,19 @@ -# Tests for the generated argument-migration helpers (tools/generate-migrations.R). +# Tests for the in-place argument-migration generator (tools/generate-migrations.R). # -# These exercise the test fixture `handle_args_migration_fixture()`, generated -# from the `migration_fixture` entry in tools/migrations.R. Old signature: -# f(a, b, c, d); new signature: f(a, b, ..., c_renamed, d) with c -> c_renamed. +# These exercise the test fixture `migration_fixture()` (R/migration-fixture.R), +# whose body carries a generated ARG_HANDLE block. Old signature f(a, b, c, d); +# new signature f(a, b, ..., c_renamed, d) with c -> c_renamed. test_that("pure new-API call returns the bindings unchanged", { expect_equal( - handle_args_migration_fixture(a = 1, b = 2, c_renamed = 3, d = 4), + migration_fixture(a = 1, b = 2, c_renamed = 3, d = 4), list(a = 1, b = 2, c_renamed = 3, d = 4) ) }) test_that("empty dots fall back to the formal defaults", { expect_equal( - handle_args_migration_fixture(a = 1, b = 2), + migration_fixture(a = 1, b = 2), list(a = 1, b = 2, c_renamed = NULL, d = NULL) ) }) @@ -22,7 +22,7 @@ test_that("a legacy positional call is recovered with one soft-deprecation", { rlang::local_options(lifecycle_verbosity = "warning") lifecycle::expect_deprecated( - res <- handle_args_migration_fixture(1, 2, 3, 4), + res <- migration_fixture(1, 2, 3, 4), "migration_fixture" ) expect_equal(res, list(a = 1, b = 2, c_renamed = 3, d = 4)) @@ -32,7 +32,7 @@ test_that("a legacy positional call recovering a single slot still works", { rlang::local_options(lifecycle_verbosity = "warning") lifecycle::expect_deprecated( - res <- handle_args_migration_fixture(1, 2, 99) + res <- migration_fixture(1, 2, 99) ) expect_equal(res, list(a = 1, b = 2, c_renamed = 99, d = NULL)) }) @@ -41,7 +41,7 @@ test_that("a renamed-away old name is recovered by name", { rlang::local_options(lifecycle_verbosity = "warning") lifecycle::expect_deprecated( - res <- handle_args_migration_fixture(1, 2, c = 99) + res <- migration_fixture(1, 2, c = 99) ) expect_equal(res, list(a = 1, b = 2, c_renamed = 99, d = NULL)) }) @@ -50,7 +50,7 @@ test_that("an abbreviation of a new arg is recovered by partial match", { rlang::local_options(lifecycle_verbosity = "warning") lifecycle::expect_deprecated( - res <- handle_args_migration_fixture(1, 2, c_r = 42) + res <- migration_fixture(1, 2, c_r = 42) ) expect_equal(res, list(a = 1, b = 2, c_renamed = 42, d = NULL)) }) @@ -60,7 +60,7 @@ test_that("recovery emits a single deprecation warning, not one per slot", { warnings <- character() withCallingHandlers( - handle_args_migration_fixture(1, 2, 3, 4), + migration_fixture(1, 2, 3, 4), lifecycle_warning_deprecated = function(w) { warnings <<- c(warnings, conditionMessage(w)) invokeRestart("muffleWarning") @@ -69,93 +69,76 @@ test_that("recovery emits a single deprecation warning, not one per slot", { expect_length(warnings, 1L) }) +test_that("the inline deprecation fires for a direct user call", { + # The spliced `deprecate_soft()` runs in the function's own frame, so its + # default `user_env` (caller_env(2)) resolves to the caller -- here the test + # frame, which lifecycle treats as a direct user call. No `.user_env` plumbing. + rlang::local_options(lifecycle_verbosity = "warning") + lifecycle::expect_deprecated(migration_fixture(1, 2, 3, 4)) +}) + test_that("an unknown named argument errors", { expect_error( - handle_args_migration_fixture(1, 2, foo = 5), + migration_fixture(1, 2, foo = 5), "Unexpected argument" ) }) test_that("supplying a slot both positionally and by its new keyword errors", { expect_error( - handle_args_migration_fixture(1, 2, 3, c_renamed = "kw"), + migration_fixture(1, 2, 3, c_renamed = "kw"), "supplied more than once" ) }) test_that("supplying a slot by two different names errors", { expect_error( - handle_args_migration_fixture(1, 2, c = 1, c_renamed = 2), + migration_fixture(1, 2, c = 1, c_renamed = 2), "supplied more than once" ) }) test_that("too many positional arguments errors", { expect_error( - handle_args_migration_fixture(1, 2, 3, 4, 5), + migration_fixture(1, 2, 3, 4, 5), "Too many arguments" ) }) -test_that(".user_env controls whether the soft-deprecation surfaces", { - # Gotcha: lifecycle::deprecate_soft() only signals when is_direct(user_env) is - # TRUE -- a package-internal `user_env` is swallowed *even under* - # lifecycle_verbosity = "warning" (it gates on `direct` before honouring the - # verbosity override). This is why the generated helper threads `.user_env`, - # and why public callers must pass `.user_env = rlang::caller_env()` so it - # resolves to the *user's* frame rather than the package namespace. - rlang::local_options(lifecycle_verbosity = "warning") - - # A direct user env (the default here is this test's frame) -> warning fires. - lifecycle::expect_deprecated( - res <- handle_args_migration_fixture(1, 2, 3, 4) - ) - expect_equal(res, list(a = 1, b = 2, c_renamed = 3, d = 4)) - - # A package namespace user env -> suppressed, but recovery still happens. - expect_no_warning( - res <- handle_args_migration_fixture( - 1, - 2, - 3, - 4, - .user_env = asNamespace("igraph") - ) - ) - expect_equal(res, list(a = 1, b = 2, c_renamed = 3, d = 4)) -}) - # ---- generator-level tests (source checkout only) -------------------------- -test_that("the generated helper is in sync with the registry", { +test_that("the spliced block is in sync with the registry", { # tools/ is excluded from the built package (.Rbuildignore), so this only runs - # from a source checkout (local dev + CI). The rcc workflow guards drift with - # its own `git diff` step. + # from a source checkout (local dev + CI). The before-install action guards + # drift in CI. generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") skip_if_not(file.exists(generator)) gen_env <- new.env() sys.source(generator, envir = gen_env) registry <- testthat::test_path("..", "..", "tools", "migrations.R") - entries <- gen_env$load_migrations(registry) - fixture <- Filter(function(e) e$fn == "migration_fixture", entries)[[1]] - - expected <- gen_env$render_migration(fixture) - committed <- readLines( - testthat::test_path("..", "..", "R", "handle-args-migration_fixture.R") + migrations <- gen_env$load_migrations(registry) + by_fn <- stats::setNames( + migrations, + vapply(migrations, function(e) e$fn, character(1)) ) - expect_identical(committed, expected) + + fixture <- testthat::test_path("..", "..", "R", "migration-fixture.R") + lines <- readLines(fixture, warn = FALSE) + spliced <- gen_env$splice_blocks(lines, by_fn) + expect_identical(spliced$lines, lines) }) test_that("an abbreviation matching multiple arguments errors", { - # Built from an ad-hoc registry so we don't ship a second fixture helper. + # Built from an ad-hoc registry + source file so we don't ship a second + # fixture. generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") skip_if_not(file.exists(generator)) gen_env <- new.env() sys.source(generator, envir = gen_env) - tmp_reg <- withr::local_tempfile(fileext = ".R") + dir <- withr::local_tempdir() writeLines( c( "migrations <- list(", @@ -166,14 +149,25 @@ test_that("an abbreviation matching multiple arguments errors", { " )", ")" ), - tmp_reg + file.path(dir, "migrations.R") + ) + writeLines( + c( + "amb_fixture <- function(x, ..., alpha = NULL, alphabet = NULL) {", + " # BEGIN GENERATED ARG_HANDLE: amb_fixture", + " # END GENERATED ARG_HANDLE", + " list(x = x, alpha = alpha, alphabet = alphabet)", + "}" + ), + file.path(dir, "amb.R") + ) + suppressMessages( + gen_env$generate_migrations(file.path(dir, "migrations.R"), dir) ) - out <- withr::local_tempdir() - suppressMessages(gen_env$generate_migrations(tmp_reg, out)) - sys.source(file.path(out, "handle-args-amb_fixture.R"), envir = environment()) + sys.source(file.path(dir, "amb.R"), envir = environment()) expect_error( - handle_args_amb_fixture(1, alph = 2), + amb_fixture(1, alph = 2), "matches multiple" ) }) diff --git a/tools/generate-migrations.R b/tools/generate-migrations.R index 0b6689897fc..2d8318b38b6 100644 --- a/tools/generate-migrations.R +++ b/tools/generate-migrations.R @@ -1,19 +1,26 @@ #!/usr/bin/env Rscript # -# Generator for per-function argument-migration helpers. +# Generator for in-place argument-migration code. # -# Reads the declarative registry in tools/migrations.R and, for each entry, -# emits one self-contained internal helper `handle_args_()` at -# R/handle-args-.R. The generated helpers recover legacy calls made against -# a function's pre-migration signature after `...` was inserted -- both -# positional values and (partially) named ones. +# Reads the declarative registry in tools/migrations.R and splices recovery code +# directly into each migrated function body, between the markers +# +# # BEGIN GENERATED ARG_HANDLE: +# # END GENERATED ARG_HANDLE +# +# The spliced block recovers a legacy call to the pre-migration signature (after +# `...` was inserted) -- both positional values and (partially) named ones -- and +# emits a single soft-deprecation. There is no handler function and no caller-env +# threading: the `lifecycle::deprecate_soft()` call sits directly in the host +# function, so its default `user_env` already resolves to the user's frame. # # Usage: # Rscript tools/generate-migrations.R # -# The output is deterministic (entries sorted alphabetically by function name) -# and idempotent (running twice produces no diff). A testthat helper regenerates -# automatically when the registry is newer; CI fails on any uncommitted drift. +# Output is deterministic and idempotent (running twice produces no diff) and is +# laid out exactly as `air` formats it, so the host files stay clean. A testthat +# helper regenerates automatically when the registry is newer; CI fails on any +# uncommitted drift. # # Base R only -- no package needs to be loaded to run this. @@ -174,21 +181,6 @@ normalise_migration <- function(fn, entry) { # ---- rendering ------------------------------------------------------------- -render_formals <- function(entry) { - tail_fmls <- vapply( - entry$tail, - function(nm) { - if (nm %in% names(entry$defaults)) { - paste0(nm, " = ", entry$defaults[[nm]]) - } else { - nm - } - }, - character(1) - ) - c(entry$head, "...", tail_fmls, ".user_env = rlang::caller_env()") -} - render_chr_vec <- function(x) { if (length(x) == 0L) { return("character(0)") @@ -211,48 +203,39 @@ render_defaults_list <- function(entry) { paste0("list(", paste(items, collapse = ", "), ")") } -render_migration <- function(entry) { - fn <- entry$fn - bindings_items <- paste0(entry$new_args, " = ", entry$new_args) - - header <- c( - "# Generated by tools/generate-migrations.R from tools/migrations.R — do not edit by hand.", - "# Regenerate with: Rscript tools/generate-migrations.R", - "# (Excluded from `air` formatting via air.toml so this layout stays canonical.)", - "#", - paste0("# `", fn, "()` gained a `...` in its signature. A call written"), - "# against the old signature lands its arguments in `...`; this helper", - "# recovers them by position or by (partial) name, soft-deprecates the old", - "# form, and returns the new-API bindings as a named list.", - "#", - "# `.user_env` is threaded into `lifecycle::deprecate_soft()` so the warning is", - "# attributed to the user's call instead of being suppressed as", - "# package-internal. Public callers must pass `.user_env = rlang::caller_env()`." - ) - - # One formal per line, matching rigraph's hand-written signatures. The file is - # excluded from `air` (see air.toml) so this layout is canonical. - fmls <- render_formals(entry) - fmls_lines <- paste0(" ", fmls) - fmls_lines[-length(fmls_lines)] <- paste0( - fmls_lines[-length(fmls_lines)], - "," - ) - signature <- c( - paste0("handle_args_", fn, " <- function("), - fmls_lines, - ") {" - ) +# The new-API args (with defaults) whose current value we pass to the matcher for +# conflict detection -- i.e. the recoverable args that have a formal default. +render_current_list <- function(entry) { + keep <- intersect(entry$tail, names(entry$defaults)) + if (length(keep) == 0L) { + return("list()") + } + paste0("list(", paste0(keep, " = ", keep, collapse = ", "), ")") +} - body <- c( - paste0(" bindings <- list(", paste(bindings_items, collapse = ", "), ")"), - "", - " dots <- list(...)", +# Render the inline ARG_HANDLE block spliced into a function body between the +# markers. It is laid out exactly as `air` formats it so regeneration leaves no +# drift in the (hand-written) host file. +# +# Shape: an immediately-invoked function does the pure matching (its temporaries +# stay scoped, never leaking into the host frame and never touching environments) +# and returns either NULL or the recovered values plus message parts. The host +# frame then assigns the recovered values over its own locals and emits a single +# `lifecycle::deprecate_soft()`. Because that call sits directly in the host +# function, its default `user_env` (caller_env(2)) resolves to the user's frame +# -- no `.user_env` threading needed. +render_arg_handle <- function(entry) { + fn <- entry$fn + c( + paste0( + "# Generated from tools/migrations.R by tools/generate-migrations.R -- do not edit." + ), + "# Recovers a legacy call to the pre-migration signature; regenerate with", + "# `Rscript tools/generate-migrations.R`.", + ".arg_handle <- (function(dots, current) {", " if (length(dots) == 0L) {", - " return(bindings)", + " return(NULL)", " }", - "", - " # Maps derived from the old/new signatures.", paste0(" recover_new <- ", render_chr_vec(entry$recover_new)), paste0(" recover_old <- ", render_chr_vec(entry$recover_old)), paste0(" match_names <- ", render_chr_vec(entry$match_names)), @@ -260,126 +243,174 @@ render_migration <- function(entry) { paste0(" defaults <- ", render_defaults_list(entry)), paste0(" head_args <- ", render_chr_vec(entry$head)), paste0(" fn_name <- \"", fn, "\""), - "", " dot_names <- rlang::names2(dots)", - " rebound_old <- character() # old labels, in the order encountered", - " rebound_new <- character() # resolved new names, same order", - " unknown <- character()", + " values <- list()", + " rebound_old <- character()", + " rebound_new <- character()", " pos <- 0L", - "", " for (k in seq_along(dots)) {", " nm <- dot_names[[k]]", " if (nzchar(nm)) {", - " # Named (possibly abbreviated): partial-match against recoverable names.", + " # Named (possibly abbreviated): partial-match recoverable names.", " j <- charmatch(nm, match_names)", " if (is.na(j)) {", - " unknown <- c(unknown, nm)", - " next", + " cli::cli_abort(c(", + " \"Unexpected argument passed to {.fn {fn_name}}: {.arg {nm}}.\",", + " i = \"Arguments after {.arg ...} must be spelled out in full.\"", + " ))", " }", " if (j == 0L) {", " cli::cli_abort(", - " \"Argument {.arg {nm}} matches multiple arguments of {.fn {fn_name}}.\",", - " call = .user_env", + " \"Argument {.arg {nm}} matches multiple arguments of {.fn {fn_name}}.\"", " )", " }", " new_name <- match_to[[j]]", " old_label <- match_names[[j]]", " } else {", - " # Unnamed: recover by position into the next old slot beyond the head.", + " # Unnamed: recover by position into the next old slot past the head.", " pos <- pos + 1L", " if (pos > length(recover_new)) {", - " cli::cli_abort(", - " \"Too many arguments passed to {.fn {fn_name}}.\",", - " call = .user_env", - " )", + " cli::cli_abort(\"Too many arguments passed to {.fn {fn_name}}.\")", " }", " new_name <- recover_new[[pos]]", " old_label <- recover_old[[pos]]", " }", - "", - " # Conflict: the same new arg was already rebound, or was also supplied by", - " # its new keyword (i.e. differs from its formal default).", - " if (new_name %in% rebound_new ||", - " (new_name %in% names(defaults) &&", - " !identical(bindings[[new_name]], defaults[[new_name]]))) {", - " cli::cli_abort(", - " c(", - " \"Argument {.arg {new_name}} of {.fn {fn_name}} was supplied more than once.\",", - " i = \"Pass it exactly once, by its new name {.arg {new_name}}.\"", - " ),", - " call = .user_env", - " )", + " duplicated <- new_name %in% rebound_new", + " has_default <- new_name %in% names(defaults)", + " reassigned <- has_default &&", + " !identical(current[[new_name]], defaults[[new_name]])", + " if (duplicated || reassigned) {", + " cli::cli_abort(c(", + " \"Argument {.arg {new_name}} of {.fn {fn_name}} was supplied more than once.\",", + " i = \"Pass it exactly once, by its new name {.arg {new_name}}.\"", + " ))", " }", - "", - " bindings[[new_name]] <- dots[[k]]", + " values[[new_name]] <- dots[[k]]", " rebound_old <- c(rebound_old, old_label)", " rebound_new <- c(rebound_new, new_name)", " }", - "", - " if (length(unknown)) {", - " cli::cli_abort(", - " c(", - " \"Unexpected argument(s) passed to {.fn {fn_name}}: {.arg {unknown}}.\",", - " i = \"Arguments after {.arg ...} must be spelled out in full.\"", - " ),", - " call = .user_env", - " )", - " }", - "", - " # One consolidated soft-deprecation per call, showing the detected legacy", - " # call next to the recommended keyword form.", " detected <- paste0(", - " fn_name, \"(\", paste(c(head_args, rebound_old), collapse = \", \"), \")\"", + " fn_name,", + " \"(\",", + " paste(c(head_args, rebound_old), collapse = \", \"),", + " \")\"", " )", " requested <- paste0(", - " fn_name, \"(\",", + " fn_name,", + " \"(\",", " paste(c(head_args, paste0(rebound_new, \" = \")), collapse = \", \"),", " \")\"", " )", - " lifecycle::deprecate_soft(", - paste0(" when = \"", entry$when, "\","), - paste0(" what = I(paste0(\"Calling `\", fn_name, \"()` with positional or abbreviated arguments\")),"), + " list(", + " values = values,", + " what = paste0(", + " \"Calling `\",", + " fn_name,", + " \"()` with positional or abbreviated arguments\"", + " ),", " details = c(", " i = paste0(\"Detected call: \", detected),", " i = paste0(\"Use instead: \", requested)", - " ),", - " user_env = .user_env", + " )", + " )", + paste0("})(list(...), ", render_current_list(entry), ")"), + "if (!is.null(.arg_handle)) {", + " list2env(.arg_handle$values, environment())", + " lifecycle::deprecate_soft(", + paste0(" \"", entry$when, "\","), + " what = I(.arg_handle$what),", + " details = .arg_handle$details", " )", - "", - " bindings", "}" ) +} - c(header, "", "#' @noRd", signature, body, "") +# ---- splicing into source files -------------------------------------------- + +begin_re <- "^([\t ]*)#\\s*BEGIN GENERATED ARG_HANDLE:\\s*([A-Za-z0-9._]+)\\s*$" +end_re <- "^[\t ]*#\\s*END GENERATED ARG_HANDLE\\s*$" + +# Replace the content between each ARG_HANDLE marker pair with freshly rendered +# code, preserving the markers and the indentation of the BEGIN marker. Returns +# the new line vector and the function names whose blocks were filled. +splice_blocks <- function(lines, by_fn) { + out <- character() + filled <- character() + i <- 1L + n <- length(lines) + while (i <= n) { + m <- regmatches(lines[[i]], regexec(begin_re, lines[[i]]))[[1]] + if (length(m) == 0L) { + out <- c(out, lines[[i]]) + i <- i + 1L + next + } + indent <- m[[2]] + fn <- m[[3]] + # find the matching END + j <- i + 1L + while (j <= n && !grepl(end_re, lines[[j]])) { + j <- j + 1L + } + if (j > n) { + stop( + "Unterminated `# BEGIN GENERATED ARG_HANDLE: ", + fn, + "` block.", + call. = FALSE + ) + } + if (is.null(by_fn[[fn]])) { + stop( + "ARG_HANDLE marker for `", + fn, + "` has no entry in the migration registry.", + call. = FALSE + ) + } + block <- render_arg_handle(by_fn[[fn]]) + block <- ifelse(nzchar(block), paste0(indent, block), block) + out <- c(out, lines[[i]], block, lines[[j]]) + filled <- c(filled, fn) + i <- j + 1L + } + list(lines = out, filled = filled) } # ---- driver ---------------------------------------------------------------- -generate_migrations <- function(registry_path, out_dir) { +generate_migrations <- function(registry_path, src_dir) { migrations <- load_migrations(registry_path) - - # Remove stale generated files first so deleted registry entries don't leave - # orphaned helpers behind. - stale <- list.files( - out_dir, - pattern = "^handle-args-.*\\.R$", - full.names = TRUE + by_fn <- stats::setNames( + migrations, + vapply(migrations, function(e) e$fn, character(1)) ) - if (length(stale)) { - file.remove(stale) - } - fns <- vapply(migrations, function(e) e$fn, character(1)) - migrations <- migrations[order(fns)] + files <- list.files(src_dir, pattern = "\\.R$", full.names = TRUE) + filled <- character() + for (f in files) { + lines <- readLines(f, warn = FALSE) + if (!any(grepl(begin_re, lines))) { + next + } + res <- splice_blocks(lines, by_fn) + filled <- c(filled, res$filled) + if (!identical(res$lines, lines)) { + writeLines(res$lines, f) + message("updated ", f) + } + } - for (entry in migrations) { - lines <- render_migration(entry) - out_path <- file.path(out_dir, paste0("handle-args-", entry$fn, ".R")) - writeLines(lines, out_path) - message("wrote ", out_path) + missing <- setdiff(names(by_fn), filled) + if (length(missing)) { + warning( + "No `# BEGIN GENERATED ARG_HANDLE` marker found for: ", + paste(missing, collapse = ", "), + call. = FALSE + ) } - message("generated ", length(migrations), " migration helper(s)") - invisible(vapply(migrations, function(e) e$fn, character(1))) + message("filled ", length(filled), " ARG_HANDLE block(s)") + invisible(filled) } # Run when invoked as a script (not when sourced for its functions). diff --git a/tools/migrations.R b/tools/migrations.R index b00eac9c835..b59e715d805 100644 --- a/tools/migrations.R +++ b/tools/migrations.R @@ -24,19 +24,26 @@ # when = "" # ) # -# old The pre-migration signature. The *order* of its formals is the old -# positional order. A formal whose default is a **bare symbol** declares -# a rename to that name, e.g. `c = c_renamed` means the old `c` argument -# is the new `c_renamed`. Formals without a symbol default keep their -# name. +# old The pre-migration signature. Only its formal *names* and *order* are +# read -- old default *values* are ignored. A formal whose default is a +# **bare symbol** is the one exception: it declares a rename to that +# name, e.g. `c = c_renamed` means the old `c` argument is the new +# `c_renamed`. Formals without a symbol default keep their name. # # new The post-migration signature. Must contain exactly one `...`. The # non-`...` formals are the new-API arguments, in order; their defaults -# become the generated helper's defaults (and the values the conflict -# check compares against). +# become the function's defaults and the values the conflict check +# compares against. # # when The version string passed to `lifecycle::deprecate_soft(when = )`. # +# Changing a default as part of a migration is fine: the new default lives in +# `new` and is what the recovery uses; the old default is never consulted. The +# only caveat is the bare-symbol convention above -- an old argument whose +# genuine default is itself a bare symbol would be misread as a rename (rare; +# wrap it, e.g. `(sym)`, or reintroduce an explicit rename field if it ever +# bites). +# # --------------------------------------------------------------------------- # How recovery maps old calls onto the new signature # From 829681fe9f483f4912ae23c003f5909bd6478b6e Mon Sep 17 00:00:00 2001 From: David Schoch Date: Tue, 2 Jun 2026 18:39:12 +0200 Subject: [PATCH 07/12] ci: regenerate ARG_HANDLE blocks in after-install, drop rcc step Move the regenerate + `git diff` drift guard out of the rcc smoke job into the `custom/after-install` composite action (R is available there, and it is the natural place per review). The testthat helper still regenerates locally when the registry is newer. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/R-CMD-check.yaml | 14 ---------- .../workflows/custom/after-install/action.yml | 15 +++++++++++ tests/testthat/helper-migrations.R | 27 ++++++------------- 3 files changed, 23 insertions(+), 33 deletions(-) diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index 38bcf42bd98..e39bee746a9 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -132,20 +132,6 @@ jobs: _R_SHLIB_STRIP_=true R CMD INSTALL . shell: bash - # Regenerate the argument-migration helpers from tools/migrations.R and - # fail if they drift. Runs from the source checkout (tools/ is excluded - # from the built package) and before the auto-commit steps below, so a - # contributor who forgot to regenerate gets a hard failure. - - name: Check migration helpers are in sync with the registry - run: | - Rscript tools/generate-migrations.R - if ! git diff --exit-code -- 'R/handle-args-*.R'; then - echo "::error::R/handle-args-*.R is out of sync with tools/migrations.R." - echo "Run 'Rscript tools/generate-migrations.R' and commit the result." - exit 1 - fi - shell: bash - - id: versions-matrix # Only run for: # - pull requests if the base repo is different from the head repo diff --git a/.github/workflows/custom/after-install/action.yml b/.github/workflows/custom/after-install/action.yml index 9d910822d79..a0c839c4ac8 100644 --- a/.github/workflows/custom/after-install/action.yml +++ b/.github/workflows/custom/after-install/action.yml @@ -8,3 +8,18 @@ runs: run: | echo -e 'CPPFLAGS = -I/opt/homebrew/include\nLDFLAGS = -L/opt/homebrew/lib' | tee ~/.R/Makevars shell: bash + + # Regenerate the in-place ARG_HANDLE blocks from tools/migrations.R and fail + # if they have drifted. Runs from the source checkout (R is available after + # install; tools/ is excluded from the built package) so a contributor who + # forgot to regenerate gets a hard failure. Local dev refreshes them + # automatically via tests/testthat/helper-migrations.R. + - name: Regenerate argument-migration blocks and check for drift + run: | + Rscript tools/generate-migrations.R + if ! git diff --exit-code -- R; then + echo "::error::Generated ARG_HANDLE blocks are out of sync with tools/migrations.R." + echo "Run 'Rscript tools/generate-migrations.R' and commit the result." + exit 1 + fi + shell: bash diff --git a/tests/testthat/helper-migrations.R b/tests/testthat/helper-migrations.R index d8611770940..4771697c580 100644 --- a/tests/testthat/helper-migrations.R +++ b/tests/testthat/helper-migrations.R @@ -1,32 +1,21 @@ -# Keep the generated argument-migration helpers (R/handle-args-*.R) in sync with -# the registry during development: when tools/migrations.R or the generator is -# newer than the generated files, regenerate them before the tests run. +# Keep the in-place ARG_HANDLE blocks in sync with the registry during +# development: regenerate them before the tests run so editing tools/migrations.R +# and re-running the tests refreshes the spliced code. # # tools/ is excluded from the built package (.Rbuildignore), so this is a no-op # under `R CMD check` of the tarball; it only fires from a source checkout -# (devtools::test(), covr). CI additionally guards against committed drift with -# a `git diff` step in the rcc workflow. +# (devtools::test(), covr). The generator only rewrites a file when its block +# actually changes, so a clean tree stays clean. CI additionally guards against +# committed drift (see .github/workflows/custom/before-install). local({ registry <- testthat::test_path("..", "..", "tools", "migrations.R") generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") if (!file.exists(registry) || !file.exists(generator)) { return(invisible()) } - - out_dir <- testthat::test_path("..", "..", "R") - generated <- list.files( - out_dir, - pattern = "^handle-args-.*\\.R$", - full.names = TRUE - ) - - sources_mtime <- max(file.mtime(c(registry, generator))) - fresh <- length(generated) > 0 && all(file.mtime(generated) >= sources_mtime) - if (fresh) { - return(invisible()) - } + src_dir <- testthat::test_path("..", "..", "R") gen_env <- new.env() sys.source(generator, envir = gen_env) - suppressMessages(gen_env$generate_migrations(registry, out_dir)) + suppressMessages(gen_env$generate_migrations(registry, src_dir)) }) From 3e6c9ea29702a72782cbbe4c79908e4db358e72d Mon Sep 17 00:00:00 2001 From: David Schoch Date: Tue, 2 Jun 2026 18:39:12 +0200 Subject: [PATCH 08/12] docs: describe in-place ARG_HANDLE blocks; drop .user_env rationale Co-Authored-By: Claude Opus 4.8 --- tools/README.md | 56 +++++++++++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/tools/README.md b/tools/README.md index ef656ceb64a..7a78fa74c2e 100644 --- a/tools/README.md +++ b/tools/README.md @@ -145,25 +145,26 @@ git subrepo status tools/py-stimulus This shows the remote URL, branch, and commit hashes. -## Argument-migration helpers +## Argument-migration blocks When a public function's signature is migrated by inserting `...` and renaming arguments, pre-existing calls would otherwise break: the old positional (or partially-named) values land in `...` and hit `check_dots_empty()`. To soften -that, each migrated function gets an internal `handle_args_()` helper that -recovers the legacy call — by position **or by (partial) name** — soft-deprecates -the old form, and returns the new-API bindings as a named list. +that, the migrated function carries a small **generated block** in its body that +recovers the legacy call — by position **or by (partial) name** — and emits one +soft-deprecation. -These helpers are **generated**, not written by hand: +The block is **generated in place**, not written by hand: - **`tools/migrations.R`** is the single source of truth. Each entry declares the function's `old` and `new` signatures as literal R functions; renames and defaults are read straight off their formals (a bare-symbol default in `old`, e.g. `c = c_renamed`, means a rename). The schema is documented at the top of that file. -- **`tools/generate-migrations.R`** reads the registry and emits one - `R/handle-args-.R` per entry. Output is deterministic (entries sorted by - function name) and idempotent (running twice produces no diff). +- **`tools/generate-migrations.R`** reads the registry and rewrites the code + between the `# BEGIN GENERATED ARG_HANDLE: ` / `# END GENERATED ARG_HANDLE` + markers inside each function. Output is idempotent (running twice produces no + diff) and laid out exactly as `air` formats it, so the host files stay clean. Regenerate after editing the registry: @@ -173,39 +174,34 @@ Rscript tools/generate-migrations.R This usually happens for you: a testthat helper ([`tests/testthat/helper-migrations.R`](../tests/testthat/helper-migrations.R)) -regenerates the helpers whenever the registry is newer, and the `rcc` CI -workflow runs the generator and fails if the committed files have drifted. Do -**not** edit `R/handle-args-*.R` by hand. +regenerates the blocks whenever the registry is newer, and CI re-runs the +generator (in `custom/after-install`) and fails if the committed code has +drifted. Do **not** edit between the markers by hand. ### Call-site usage -A migrated public function delegates argument resolution to its generated -helper, passing `.user_env = rlang::caller_env()`: +A migrated function gets the marker pair at the top of its body; the generator +fills it in: ```r as_biadjacency_matrix <- function(graph, types, ..., weights = NULL, attr = lifecycle::deprecated(), names = TRUE, sparse = FALSE) { - args <- handle_args_as_biadjacency_matrix( - graph, types, ..., weights = weights, attr = attr, - names = names, sparse = sparse, - .user_env = rlang::caller_env() - ) - graph <- args$graph; types <- args$types; weights <- args$weights - # ... rest of the body + # BEGIN GENERATED ARG_HANDLE: as_biadjacency_matrix + # ... generated: recover dots, reassign weights/attr/..., deprecate_soft() ... + # END GENERATED ARG_HANDLE + + # ... rest of the body, using the (possibly reassigned) locals } ``` -The `.user_env` argument matters because the helper interposes an extra call -frame. `lifecycle::deprecate_soft()` only signals when `is_direct(user_env)` is -`TRUE` — that is, when `topenv(user_env)` is the global env (or under testthat). -Its auto-detected `user_env` would resolve to the helper's caller, the **igraph -namespace**, so the warning is swallowed (this gate runs *before* the -`lifecycle_verbosity = "warning"` override, so forcing verbosity does not bring -it back). Threading the public function's `rlang::caller_env()` through -`.user_env` makes the *user's* frame the reference: real user calls warn, while -genuinely internal igraph callers stay correctly silent — the point of a *soft* -deprecation. +The recovery runs inline, so there is no handler function and no `.user_env` +plumbing. The generated `lifecycle::deprecate_soft()` sits directly in the +function body, so its default `user_env` (`caller_env(2)`) already resolves to +the *user's* frame: real user calls warn, while genuinely internal igraph callers +stay correctly silent — the point of a *soft* deprecation. (The pure matching is +done by an immediately-invoked function whose temporaries stay scoped and never +touch an environment, so nothing leaks into the host frame.) ### Reordering or removing an argument From bb6bc251828f18106382743f8a7b6e238fc3064b Mon Sep 17 00:00:00 2001 From: David Schoch Date: Tue, 2 Jun 2026 21:49:24 +0200 Subject: [PATCH 09/12] refactor: replace inline closure with migrate_recover_args() helper Address review on #2684: - Drop the inline IIFE in the generated ARG_HANDLE block in favour of a plain, named, hand-written helper `migrate_recover_args()` (R/migrate-args.R) that takes the per-function maps and returns the recovered values plus the deprecation message parts, or NULL. It is pure (no environment access) and easy to step through in a debugger. The generated block now just supplies the configuration, assigns the results, and emits the inline deprecate_soft(). - Collapse the three-line generated header comment onto the BEGIN marker, which may now carry a trailing note; the generator's marker regex captures the function name up to the first non-identifier character. Co-Authored-By: Claude Opus 4.8 --- R/migrate-args.R | 114 ++++++++++++++++++++++++++++++ R/migration-fixture.R | 98 ++++--------------------- tests/testthat/test-handle-args.R | 96 ++++++++++++++++--------- tools/README.md | 9 ++- tools/generate-migrations.R | 112 +++++------------------------ 5 files changed, 213 insertions(+), 216 deletions(-) create mode 100644 R/migrate-args.R diff --git a/R/migrate-args.R b/R/migrate-args.R new file mode 100644 index 00000000000..dae8d1e4d60 --- /dev/null +++ b/R/migrate-args.R @@ -0,0 +1,114 @@ +# Runtime helper behind the generated `# ... ARG_HANDLE` blocks (see +# tools/migrations.R, tools/generate-migrations.R). Hand-written and tested +# directly -- the generated blocks only carry the per-function configuration and +# call this. Kept a plain function (not an inline closure) so it is easy to step +# through in a debugger. +# +# Pure: it inspects `dots` against the supplied maps and returns the recovered +# values plus the deprecation message parts, or NULL when there is nothing to +# recover. It never touches an environment and never emits the deprecation +# itself -- the caller assigns the values into its own frame and calls +# `lifecycle::deprecate_soft()` inline, so the warning is attributed correctly +# without any caller-env plumbing. + +#' @noRd +migrate_recover_args <- function( + dots, + current, + recover_new, + recover_old, + match_names, + match_to, + defaults, + head_args, + fn_name, + call = rlang::caller_env() +) { + if (length(dots) == 0L) { + return(NULL) + } + + dot_names <- rlang::names2(dots) + values <- list() + rebound_old <- character() + rebound_new <- character() + pos <- 0L + for (k in seq_along(dots)) { + nm <- dot_names[[k]] + if (nzchar(nm)) { + # Named (possibly abbreviated): partial-match the recoverable names. + j <- charmatch(nm, match_names) + if (is.na(j)) { + cli::cli_abort( + c( + "Unexpected argument passed to {.fn {fn_name}}: {.arg {nm}}.", + i = "Arguments after {.arg ...} must be spelled out in full." + ), + call = call + ) + } + if (j == 0L) { + cli::cli_abort( + "Argument {.arg {nm}} matches multiple arguments of {.fn {fn_name}}.", + call = call + ) + } + new_name <- match_to[[j]] + old_label <- match_names[[j]] + } else { + # Unnamed: recover by position into the next old slot past the head. + pos <- pos + 1L + if (pos > length(recover_new)) { + cli::cli_abort( + "Too many arguments passed to {.fn {fn_name}}.", + call = call + ) + } + new_name <- recover_new[[pos]] + old_label <- recover_old[[pos]] + } + + duplicated <- new_name %in% rebound_new + has_default <- new_name %in% names(defaults) + reassigned <- has_default && + !identical(current[[new_name]], defaults[[new_name]]) + if (duplicated || reassigned) { + cli::cli_abort( + c( + "Argument {.arg {new_name}} of {.fn {fn_name}} was supplied more than once.", + i = "Pass it exactly once, by its new name {.arg {new_name}}." + ), + call = call + ) + } + + values[[new_name]] <- dots[[k]] + rebound_old <- c(rebound_old, old_label) + rebound_new <- c(rebound_new, new_name) + } + + detected <- paste0( + fn_name, + "(", + paste(c(head_args, rebound_old), collapse = ", "), + ")" + ) + requested <- paste0( + fn_name, + "(", + paste(c(head_args, paste0(rebound_new, " = ")), collapse = ", "), + ")" + ) + list( + values = values, + what = paste0( + "Calling `", + fn_name, + "()` with positional or abbreviated arguments" + ), + details = c( + i = paste0("Detected call: ", detected), + i = paste0("Use instead: ", requested) + ) + ) +} diff --git a/R/migration-fixture.R b/R/migration-fixture.R index 66bf7440cce..b8dd54656d7 100644 --- a/R/migration-fixture.R +++ b/R/migration-fixture.R @@ -7,92 +7,18 @@ #' @noRd migration_fixture <- function(a, b, ..., c_renamed = NULL, d = NULL) { - # BEGIN GENERATED ARG_HANDLE: migration_fixture - # Generated from tools/migrations.R by tools/generate-migrations.R -- do not edit. - # Recovers a legacy call to the pre-migration signature; regenerate with - # `Rscript tools/generate-migrations.R`. - .arg_handle <- (function(dots, current) { - if (length(dots) == 0L) { - return(NULL) - } - recover_new <- c("c_renamed", "d") - recover_old <- c("c", "d") - match_names <- c("c", "c_renamed", "d") - match_to <- c("c_renamed", "c_renamed", "d") - defaults <- list(c_renamed = NULL, d = NULL) - head_args <- c("a", "b") - fn_name <- "migration_fixture" - dot_names <- rlang::names2(dots) - values <- list() - rebound_old <- character() - rebound_new <- character() - pos <- 0L - for (k in seq_along(dots)) { - nm <- dot_names[[k]] - if (nzchar(nm)) { - # Named (possibly abbreviated): partial-match recoverable names. - j <- charmatch(nm, match_names) - if (is.na(j)) { - cli::cli_abort(c( - "Unexpected argument passed to {.fn {fn_name}}: {.arg {nm}}.", - i = "Arguments after {.arg ...} must be spelled out in full." - )) - } - if (j == 0L) { - cli::cli_abort( - "Argument {.arg {nm}} matches multiple arguments of {.fn {fn_name}}." - ) - } - new_name <- match_to[[j]] - old_label <- match_names[[j]] - } else { - # Unnamed: recover by position into the next old slot past the head. - pos <- pos + 1L - if (pos > length(recover_new)) { - cli::cli_abort("Too many arguments passed to {.fn {fn_name}}.") - } - new_name <- recover_new[[pos]] - old_label <- recover_old[[pos]] - } - duplicated <- new_name %in% rebound_new - has_default <- new_name %in% names(defaults) - reassigned <- has_default && - !identical(current[[new_name]], defaults[[new_name]]) - if (duplicated || reassigned) { - cli::cli_abort(c( - "Argument {.arg {new_name}} of {.fn {fn_name}} was supplied more than once.", - i = "Pass it exactly once, by its new name {.arg {new_name}}." - )) - } - values[[new_name]] <- dots[[k]] - rebound_old <- c(rebound_old, old_label) - rebound_new <- c(rebound_new, new_name) - } - detected <- paste0( - fn_name, - "(", - paste(c(head_args, rebound_old), collapse = ", "), - ")" - ) - requested <- paste0( - fn_name, - "(", - paste(c(head_args, paste0(rebound_new, " = ")), collapse = ", "), - ")" - ) - list( - values = values, - what = paste0( - "Calling `", - fn_name, - "()` with positional or abbreviated arguments" - ), - details = c( - i = paste0("Detected call: ", detected), - i = paste0("Use instead: ", requested) - ) - ) - })(list(...), list(c_renamed = c_renamed, d = d)) + # BEGIN GENERATED ARG_HANDLE: migration_fixture, do not edit, see tools/generate-migrations.R + .arg_handle <- migrate_recover_args( + list(...), + current = list(c_renamed = c_renamed, d = d), + recover_new = c("c_renamed", "d"), + recover_old = c("c", "d"), + match_names = c("c", "c_renamed", "d"), + match_to = c("c_renamed", "c_renamed", "d"), + defaults = list(c_renamed = NULL, d = NULL), + head_args = c("a", "b"), + fn_name = "migration_fixture" + ) if (!is.null(.arg_handle)) { list2env(.arg_handle$values, environment()) lifecycle::deprecate_soft( diff --git a/tests/testthat/test-handle-args.R b/tests/testthat/test-handle-args.R index c81e86bce0a..77ddd56dd22 100644 --- a/tests/testthat/test-handle-args.R +++ b/tests/testthat/test-handle-args.R @@ -129,45 +129,73 @@ test_that("the spliced block is in sync with the registry", { expect_identical(spliced$lines, lines) }) -test_that("an abbreviation matching multiple arguments errors", { - # Built from an ad-hoc registry + source file so we don't ship a second - # fixture. - generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") - skip_if_not(file.exists(generator)) +# ---- migrate_recover_args() helper (the engine behind the blocks) ---------- + +# Config equivalent to the fixture, for exercising the helper directly. +fixture_args <- function(dots, current = list(c_renamed = NULL, d = NULL)) { + migrate_recover_args( + dots, + current = current, + recover_new = c("c_renamed", "d"), + recover_old = c("c", "d"), + match_names = c("c", "c_renamed", "d"), + match_to = c("c_renamed", "c_renamed", "d"), + defaults = list(c_renamed = NULL, d = NULL), + head_args = c("a", "b"), + fn_name = "migration_fixture" + ) +} + +test_that("migrate_recover_args() returns NULL when there is nothing to recover", { + expect_null(fixture_args(list())) +}) - gen_env <- new.env() - sys.source(generator, envir = gen_env) +test_that("migrate_recover_args() returns recovered values and message parts", { + res <- fixture_args(list(3, 4)) + expect_equal(res$values, list(c_renamed = 3, d = 4)) + expect_match(res$what, "positional or abbreviated") + expect_match(res$details[[1]], "migration_fixture\\(a, b, c, d\\)") + expect_match(res$details[[2]], "c_renamed = , d = ") +}) - dir <- withr::local_tempdir() - writeLines( - c( - "migrations <- list(", - " amb_fixture = list(", - " old = function(x, alpha, alphabet) {},", - " new = function(x, ..., alpha = NULL, alphabet = NULL) {},", - " when = \"3.0.0\"", - " )", - ")" - ), - file.path(dir, "migrations.R") - ) - writeLines( - c( - "amb_fixture <- function(x, ..., alpha = NULL, alphabet = NULL) {", - " # BEGIN GENERATED ARG_HANDLE: amb_fixture", - " # END GENERATED ARG_HANDLE", - " list(x = x, alpha = alpha, alphabet = alphabet)", - "}" +test_that("migrate_recover_args() errors on an ambiguous abbreviation", { + expect_error( + migrate_recover_args( + list(alph = 2), + current = list(alpha = NULL, alphabet = NULL), + recover_new = c("alpha", "alphabet"), + recover_old = c("alpha", "alphabet"), + match_names = c("alpha", "alphabet"), + match_to = c("alpha", "alphabet"), + defaults = list(alpha = NULL, alphabet = NULL), + head_args = "x", + fn_name = "amb_fixture" ), - file.path(dir, "amb.R") - ) - suppressMessages( - gen_env$generate_migrations(file.path(dir, "migrations.R"), dir) + "matches multiple" ) - sys.source(file.path(dir, "amb.R"), envir = environment()) +}) +test_that("migrate_recover_args() errors on unknown, conflict, and overflow", { + expect_error(fixture_args(list(foo = 5)), "Unexpected argument") expect_error( - amb_fixture(1, alph = 2), - "matches multiple" + fixture_args(list(3), current = list(c_renamed = "kw", d = NULL)), + "supplied more than once" ) + expect_error(fixture_args(list(1, 2, 3)), "Too many arguments") +}) + +test_that("the BEGIN marker may carry a trailing note", { + generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") + skip_if_not(file.exists(generator)) + gen_env <- new.env() + sys.source(generator, envir = gen_env) + + m <- regmatches( + " # BEGIN GENERATED ARG_HANDLE: foo, do not edit, see x", + regexec( + gen_env$begin_re, + " # BEGIN GENERATED ARG_HANDLE: foo, do not edit, see x" + ) + )[[1]] + expect_identical(m[[3]], "foo") }) diff --git a/tools/README.md b/tools/README.md index 7a78fa74c2e..3769e9c30f0 100644 --- a/tools/README.md +++ b/tools/README.md @@ -199,9 +199,12 @@ The recovery runs inline, so there is no handler function and no `.user_env` plumbing. The generated `lifecycle::deprecate_soft()` sits directly in the function body, so its default `user_env` (`caller_env(2)`) already resolves to the *user's* frame: real user calls warn, while genuinely internal igraph callers -stay correctly silent — the point of a *soft* deprecation. (The pure matching is -done by an immediately-invoked function whose temporaries stay scoped and never -touch an environment, so nothing leaks into the host frame.) +stay correctly silent — the point of a *soft* deprecation. The matching itself is +delegated to a small hand-written helper, `migrate_recover_args()` +([`R/migrate-args.R`](../R/migrate-args.R)) — a plain, debuggable function that +takes the per-function maps and returns the recovered values plus message parts +(or `NULL`); the generated block just supplies the configuration and assigns the +results. ### Reordering or removing an argument diff --git a/tools/generate-migrations.R b/tools/generate-migrations.R index 2d8318b38b6..7ba37ce134a 100644 --- a/tools/generate-migrations.R +++ b/tools/generate-migrations.R @@ -217,103 +217,26 @@ render_current_list <- function(entry) { # markers. It is laid out exactly as `air` formats it so regeneration leaves no # drift in the (hand-written) host file. # -# Shape: an immediately-invoked function does the pure matching (its temporaries -# stay scoped, never leaking into the host frame and never touching environments) -# and returns either NULL or the recovered values plus message parts. The host -# frame then assigns the recovered values over its own locals and emits a single +# Shape: the per-function configuration is passed to `migrate_recover_args()` +# (a hand-written, debuggable helper) which returns either NULL or the recovered +# values plus the deprecation message parts. The host frame then assigns the +# recovered values over its own locals and emits a single # `lifecycle::deprecate_soft()`. Because that call sits directly in the host # function, its default `user_env` (caller_env(2)) resolves to the user's frame # -- no `.user_env` threading needed. render_arg_handle <- function(entry) { - fn <- entry$fn c( - paste0( - "# Generated from tools/migrations.R by tools/generate-migrations.R -- do not edit." - ), - "# Recovers a legacy call to the pre-migration signature; regenerate with", - "# `Rscript tools/generate-migrations.R`.", - ".arg_handle <- (function(dots, current) {", - " if (length(dots) == 0L) {", - " return(NULL)", - " }", - paste0(" recover_new <- ", render_chr_vec(entry$recover_new)), - paste0(" recover_old <- ", render_chr_vec(entry$recover_old)), - paste0(" match_names <- ", render_chr_vec(entry$match_names)), - paste0(" match_to <- ", render_chr_vec(entry$match_to)), - paste0(" defaults <- ", render_defaults_list(entry)), - paste0(" head_args <- ", render_chr_vec(entry$head)), - paste0(" fn_name <- \"", fn, "\""), - " dot_names <- rlang::names2(dots)", - " values <- list()", - " rebound_old <- character()", - " rebound_new <- character()", - " pos <- 0L", - " for (k in seq_along(dots)) {", - " nm <- dot_names[[k]]", - " if (nzchar(nm)) {", - " # Named (possibly abbreviated): partial-match recoverable names.", - " j <- charmatch(nm, match_names)", - " if (is.na(j)) {", - " cli::cli_abort(c(", - " \"Unexpected argument passed to {.fn {fn_name}}: {.arg {nm}}.\",", - " i = \"Arguments after {.arg ...} must be spelled out in full.\"", - " ))", - " }", - " if (j == 0L) {", - " cli::cli_abort(", - " \"Argument {.arg {nm}} matches multiple arguments of {.fn {fn_name}}.\"", - " )", - " }", - " new_name <- match_to[[j]]", - " old_label <- match_names[[j]]", - " } else {", - " # Unnamed: recover by position into the next old slot past the head.", - " pos <- pos + 1L", - " if (pos > length(recover_new)) {", - " cli::cli_abort(\"Too many arguments passed to {.fn {fn_name}}.\")", - " }", - " new_name <- recover_new[[pos]]", - " old_label <- recover_old[[pos]]", - " }", - " duplicated <- new_name %in% rebound_new", - " has_default <- new_name %in% names(defaults)", - " reassigned <- has_default &&", - " !identical(current[[new_name]], defaults[[new_name]])", - " if (duplicated || reassigned) {", - " cli::cli_abort(c(", - " \"Argument {.arg {new_name}} of {.fn {fn_name}} was supplied more than once.\",", - " i = \"Pass it exactly once, by its new name {.arg {new_name}}.\"", - " ))", - " }", - " values[[new_name]] <- dots[[k]]", - " rebound_old <- c(rebound_old, old_label)", - " rebound_new <- c(rebound_new, new_name)", - " }", - " detected <- paste0(", - " fn_name,", - " \"(\",", - " paste(c(head_args, rebound_old), collapse = \", \"),", - " \")\"", - " )", - " requested <- paste0(", - " fn_name,", - " \"(\",", - " paste(c(head_args, paste0(rebound_new, \" = \")), collapse = \", \"),", - " \")\"", - " )", - " list(", - " values = values,", - " what = paste0(", - " \"Calling `\",", - " fn_name,", - " \"()` with positional or abbreviated arguments\"", - " ),", - " details = c(", - " i = paste0(\"Detected call: \", detected),", - " i = paste0(\"Use instead: \", requested)", - " )", - " )", - paste0("})(list(...), ", render_current_list(entry), ")"), + ".arg_handle <- migrate_recover_args(", + " list(...),", + paste0(" current = ", render_current_list(entry), ","), + paste0(" recover_new = ", render_chr_vec(entry$recover_new), ","), + paste0(" recover_old = ", render_chr_vec(entry$recover_old), ","), + paste0(" match_names = ", render_chr_vec(entry$match_names), ","), + paste0(" match_to = ", render_chr_vec(entry$match_to), ","), + paste0(" defaults = ", render_defaults_list(entry), ","), + paste0(" head_args = ", render_chr_vec(entry$head), ","), + paste0(" fn_name = \"", entry$fn, "\""), + ")", "if (!is.null(.arg_handle)) {", " list2env(.arg_handle$values, environment())", " lifecycle::deprecate_soft(", @@ -327,7 +250,10 @@ render_arg_handle <- function(entry) { # ---- splicing into source files -------------------------------------------- -begin_re <- "^([\t ]*)#\\s*BEGIN GENERATED ARG_HANDLE:\\s*([A-Za-z0-9._]+)\\s*$" +# The function name is captured up to the first non-identifier char, so the +# BEGIN marker may carry a trailing note, e.g. +# # BEGIN GENERATED ARG_HANDLE: foo, do not edit, see tools/generate-migrations.R +begin_re <- "^([\t ]*)#\\s*BEGIN GENERATED ARG_HANDLE:\\s*([A-Za-z0-9._]+)" end_re <- "^[\t ]*#\\s*END GENERATED ARG_HANDLE\\s*$" # Replace the content between each ARG_HANDLE marker pair with freshly rendered From 159c0e089cccd0d4fe9ff16483f9682ef7a8ed77 Mon Sep 17 00:00:00 2001 From: David Schoch Date: Wed, 3 Jun 2026 07:53:51 +0200 Subject: [PATCH 10/12] perf: guard generated block with ...length(); extend fixture Address review on #2684: - Wrap the generated ARG_HANDLE block in `if (...length() > 0L)` so a correct new-API call (nothing in `...`) skips the `migrate_recover_args()` call entirely; the now-redundant `if (!is.null())` check is dropped. - Extend the test fixture to `migration_fixture(graph, n, weight = weights, type, directed)` so it exercises a rename (`weight -> weights`), unique abbreviations (`ty`, `dir`) and an ambiguous one (`weig` matches both `weight` and `weights`). Co-Authored-By: Claude Opus 4.8 --- R/migration-fixture.R | 49 ++++++++++++++++++++++++------------- tools/generate-migrations.R | 38 ++++++++++++++-------------- tools/migrations.R | 19 ++++++++++---- 3 files changed, 66 insertions(+), 40 deletions(-) diff --git a/R/migration-fixture.R b/R/migration-fixture.R index b8dd54656d7..dc0d458538f 100644 --- a/R/migration-fixture.R +++ b/R/migration-fixture.R @@ -1,25 +1,34 @@ # Test fixture for the in-place argument-migration generator (tools/migrations.R, # tools/generate-migrations.R). `migration_fixture()` carries a generated # ARG_HANDLE block that recovers a legacy call to its pre-3.0.0 signature -# f(a, b, c, d) -- now f(a, b, ..., c_renamed, d) with c renamed to c_renamed. -# It exists only to exercise the generator end-to-end; see -# tests/testthat/test-handle-args.R. +# f(graph, n, weight, type, directed) -- now +# f(graph, n, ..., weights, type, directed) with weight renamed to weights. The +# names are chosen so the tests can exercise renames, unique abbreviations and an +# ambiguous one. It exists only to exercise the generator end-to-end; see +# tests/testthat/test-migration-fixture.R. #' @noRd -migration_fixture <- function(a, b, ..., c_renamed = NULL, d = NULL) { +migration_fixture <- function( + graph, + n, + ..., + weights = NULL, + type = "out", + directed = FALSE +) { # BEGIN GENERATED ARG_HANDLE: migration_fixture, do not edit, see tools/generate-migrations.R - .arg_handle <- migrate_recover_args( - list(...), - current = list(c_renamed = c_renamed, d = d), - recover_new = c("c_renamed", "d"), - recover_old = c("c", "d"), - match_names = c("c", "c_renamed", "d"), - match_to = c("c_renamed", "c_renamed", "d"), - defaults = list(c_renamed = NULL, d = NULL), - head_args = c("a", "b"), - fn_name = "migration_fixture" - ) - if (!is.null(.arg_handle)) { + if (...length() > 0L) { + .arg_handle <- migrate_recover_args( + list(...), + current = list(weights = weights, type = type, directed = directed), + recover_new = c("weights", "type", "directed"), + recover_old = c("weight", "type", "directed"), + match_names = c("weight", "weights", "type", "directed"), + match_to = c("weights", "weights", "type", "directed"), + defaults = list(weights = NULL, type = "out", directed = FALSE), + head_args = c("graph", "n"), + fn_name = "migration_fixture" + ) list2env(.arg_handle$values, environment()) lifecycle::deprecate_soft( "3.0.0", @@ -29,5 +38,11 @@ migration_fixture <- function(a, b, ..., c_renamed = NULL, d = NULL) { } # END GENERATED ARG_HANDLE - list(a = a, b = b, c_renamed = c_renamed, d = d) + list( + graph = graph, + n = n, + weights = weights, + type = type, + directed = directed + ) } diff --git a/tools/generate-migrations.R b/tools/generate-migrations.R index 7ba37ce134a..e66cec7676e 100644 --- a/tools/generate-migrations.R +++ b/tools/generate-migrations.R @@ -218,26 +218,28 @@ render_current_list <- function(entry) { # drift in the (hand-written) host file. # # Shape: the per-function configuration is passed to `migrate_recover_args()` -# (a hand-written, debuggable helper) which returns either NULL or the recovered -# values plus the deprecation message parts. The host frame then assigns the -# recovered values over its own locals and emits a single -# `lifecycle::deprecate_soft()`. Because that call sits directly in the host -# function, its default `user_env` (caller_env(2)) resolves to the user's frame -# -- no `.user_env` threading needed. +# (a hand-written, debuggable helper) which returns the recovered values plus the +# deprecation message parts. The host frame then assigns the recovered values +# over its own locals and emits a single `lifecycle::deprecate_soft()`. Because +# that call sits directly in the host function, its default `user_env` +# (caller_env(2)) resolves to the user's frame -- no `.user_env` threading needed. +# +# The whole thing is guarded by `...length() > 0L` so the common path (a correct +# new-API call with nothing in `...`) skips the helper call entirely. render_arg_handle <- function(entry) { c( - ".arg_handle <- migrate_recover_args(", - " list(...),", - paste0(" current = ", render_current_list(entry), ","), - paste0(" recover_new = ", render_chr_vec(entry$recover_new), ","), - paste0(" recover_old = ", render_chr_vec(entry$recover_old), ","), - paste0(" match_names = ", render_chr_vec(entry$match_names), ","), - paste0(" match_to = ", render_chr_vec(entry$match_to), ","), - paste0(" defaults = ", render_defaults_list(entry), ","), - paste0(" head_args = ", render_chr_vec(entry$head), ","), - paste0(" fn_name = \"", entry$fn, "\""), - ")", - "if (!is.null(.arg_handle)) {", + "if (...length() > 0L) {", + " .arg_handle <- migrate_recover_args(", + " list(...),", + paste0(" current = ", render_current_list(entry), ","), + paste0(" recover_new = ", render_chr_vec(entry$recover_new), ","), + paste0(" recover_old = ", render_chr_vec(entry$recover_old), ","), + paste0(" match_names = ", render_chr_vec(entry$match_names), ","), + paste0(" match_to = ", render_chr_vec(entry$match_to), ","), + paste0(" defaults = ", render_defaults_list(entry), ","), + paste0(" head_args = ", render_chr_vec(entry$head), ","), + paste0(" fn_name = \"", entry$fn, "\""), + " )", " list2env(.arg_handle$values, environment())", " lifecycle::deprecate_soft(", paste0(" \"", entry$when, "\","), diff --git a/tools/migrations.R b/tools/migrations.R index b59e715d805..1a23eabb81c 100644 --- a/tools/migrations.R +++ b/tools/migrations.R @@ -73,12 +73,21 @@ migrations <- list( # ), # --- test fixture -------------------------------------------------------- - # Exercises the generator end-to-end without migrating a real function. - # Old signature f(a, b, c, d); new f(a, b, ..., c_renamed, d) with c renamed - # to c_renamed. Consumed by tests/testthat/test-handle-args.R. + # Exercises the generator end-to-end without migrating a real function. The + # arg names are chosen to cover every recovery path: a rename (`weight -> + # weights`), unique abbreviations (`ty`, `dir`) and an ambiguous one (`weig` + # matches both `weight` and `weights`). Consumed by + # tests/testthat/test-migration-fixture.R. migration_fixture = list( - old = function(a, b, c = c_renamed, d) {}, - new = function(a, b, ..., c_renamed = NULL, d = NULL) {}, + old = function(graph, n, weight = weights, type, directed) {}, + new = function( + graph, + n, + ..., + weights = NULL, + type = "out", + directed = FALSE + ) {}, when = "3.0.0" ) ) From 7ba782ec94439582bccb685fa0502ff258153120 Mon Sep 17 00:00:00 2001 From: David Schoch Date: Wed, 3 Jun 2026 07:53:51 +0200 Subject: [PATCH 11/12] test: snapshot the migration fixture's deprecation and error messages Rename test-handle-args.R to test-migration-fixture.R and add snapshot tests (run under lifecycle_verbosity = "warning") covering positional recovery, the renamed-away old name, unique/ambiguous abbreviations, and the unknown / conflict / too-many error messages, alongside the migrate_recover_args() unit tests and the generator in-sync checks. Co-Authored-By: Claude Opus 4.8 --- tests/testthat/_snaps/migration-fixture.md | 84 +++++++++ tests/testthat/test-handle-args.R | 201 --------------------- tests/testthat/test-migration-fixture.R | 165 +++++++++++++++++ 3 files changed, 249 insertions(+), 201 deletions(-) create mode 100644 tests/testthat/_snaps/migration-fixture.md delete mode 100644 tests/testthat/test-handle-args.R create mode 100644 tests/testthat/test-migration-fixture.R diff --git a/tests/testthat/_snaps/migration-fixture.md b/tests/testthat/_snaps/migration-fixture.md new file mode 100644 index 00000000000..3c2930143ac --- /dev/null +++ b/tests/testthat/_snaps/migration-fixture.md @@ -0,0 +1,84 @@ +# recovery deprecation messages + + Code + x <- migration_fixture("g", 5, 1:3, "in", TRUE) + Condition + Warning: + Calling `migration_fixture()` with positional or abbreviated arguments was deprecated in igraph 3.0.0. + i Detected call: migration_fixture(graph, n, weight, type, directed) + i Use instead: migration_fixture(graph, n, weights = , type = , directed = ) + +--- + + Code + x <- migration_fixture("g", 5, 1:3) + Condition + Warning: + Calling `migration_fixture()` with positional or abbreviated arguments was deprecated in igraph 3.0.0. + i Detected call: migration_fixture(graph, n, weight) + i Use instead: migration_fixture(graph, n, weights = ) + +--- + + Code + x <- migration_fixture("g", 5, weight = 1:3) + Condition + Warning: + Calling `migration_fixture()` with positional or abbreviated arguments was deprecated in igraph 3.0.0. + i Detected call: migration_fixture(graph, n, weight) + i Use instead: migration_fixture(graph, n, weights = ) + +--- + + Code + x <- migration_fixture("g", 5, ty = "in") + Condition + Warning: + Calling `migration_fixture()` with positional or abbreviated arguments was deprecated in igraph 3.0.0. + i Detected call: migration_fixture(graph, n, type) + i Use instead: migration_fixture(graph, n, type = ) + +--- + + Code + x <- migration_fixture("g", 5, dir = TRUE) + Condition + Warning: + Calling `migration_fixture()` with positional or abbreviated arguments was deprecated in igraph 3.0.0. + i Detected call: migration_fixture(graph, n, directed) + i Use instead: migration_fixture(graph, n, directed = ) + +# error message snapshots + + Code + migration_fixture("g", 5, weig = 1) + Condition + Error in `migration_fixture()`: + ! Argument `weig` matches multiple arguments of `migration_fixture()`. + +--- + + Code + migration_fixture("g", 5, foo = 1) + Condition + Error in `migration_fixture()`: + ! Unexpected argument passed to `migration_fixture()`: `foo`. + i Arguments after `...` must be spelled out in full. + +--- + + Code + migration_fixture("g", 5, 1:3, weights = 9) + Condition + Error in `migration_fixture()`: + ! Argument `weights` of `migration_fixture()` was supplied more than once. + i Pass it exactly once, by its new name `weights`. + +--- + + Code + migration_fixture("g", 5, 1, 2, 3, 4) + Condition + Error in `migration_fixture()`: + ! Too many arguments passed to `migration_fixture()`. + diff --git a/tests/testthat/test-handle-args.R b/tests/testthat/test-handle-args.R deleted file mode 100644 index 77ddd56dd22..00000000000 --- a/tests/testthat/test-handle-args.R +++ /dev/null @@ -1,201 +0,0 @@ -# Tests for the in-place argument-migration generator (tools/generate-migrations.R). -# -# These exercise the test fixture `migration_fixture()` (R/migration-fixture.R), -# whose body carries a generated ARG_HANDLE block. Old signature f(a, b, c, d); -# new signature f(a, b, ..., c_renamed, d) with c -> c_renamed. - -test_that("pure new-API call returns the bindings unchanged", { - expect_equal( - migration_fixture(a = 1, b = 2, c_renamed = 3, d = 4), - list(a = 1, b = 2, c_renamed = 3, d = 4) - ) -}) - -test_that("empty dots fall back to the formal defaults", { - expect_equal( - migration_fixture(a = 1, b = 2), - list(a = 1, b = 2, c_renamed = NULL, d = NULL) - ) -}) - -test_that("a legacy positional call is recovered with one soft-deprecation", { - rlang::local_options(lifecycle_verbosity = "warning") - - lifecycle::expect_deprecated( - res <- migration_fixture(1, 2, 3, 4), - "migration_fixture" - ) - expect_equal(res, list(a = 1, b = 2, c_renamed = 3, d = 4)) -}) - -test_that("a legacy positional call recovering a single slot still works", { - rlang::local_options(lifecycle_verbosity = "warning") - - lifecycle::expect_deprecated( - res <- migration_fixture(1, 2, 99) - ) - expect_equal(res, list(a = 1, b = 2, c_renamed = 99, d = NULL)) -}) - -test_that("a renamed-away old name is recovered by name", { - rlang::local_options(lifecycle_verbosity = "warning") - - lifecycle::expect_deprecated( - res <- migration_fixture(1, 2, c = 99) - ) - expect_equal(res, list(a = 1, b = 2, c_renamed = 99, d = NULL)) -}) - -test_that("an abbreviation of a new arg is recovered by partial match", { - rlang::local_options(lifecycle_verbosity = "warning") - - lifecycle::expect_deprecated( - res <- migration_fixture(1, 2, c_r = 42) - ) - expect_equal(res, list(a = 1, b = 2, c_renamed = 42, d = NULL)) -}) - -test_that("recovery emits a single deprecation warning, not one per slot", { - rlang::local_options(lifecycle_verbosity = "warning") - - warnings <- character() - withCallingHandlers( - migration_fixture(1, 2, 3, 4), - lifecycle_warning_deprecated = function(w) { - warnings <<- c(warnings, conditionMessage(w)) - invokeRestart("muffleWarning") - } - ) - expect_length(warnings, 1L) -}) - -test_that("the inline deprecation fires for a direct user call", { - # The spliced `deprecate_soft()` runs in the function's own frame, so its - # default `user_env` (caller_env(2)) resolves to the caller -- here the test - # frame, which lifecycle treats as a direct user call. No `.user_env` plumbing. - rlang::local_options(lifecycle_verbosity = "warning") - lifecycle::expect_deprecated(migration_fixture(1, 2, 3, 4)) -}) - -test_that("an unknown named argument errors", { - expect_error( - migration_fixture(1, 2, foo = 5), - "Unexpected argument" - ) -}) - -test_that("supplying a slot both positionally and by its new keyword errors", { - expect_error( - migration_fixture(1, 2, 3, c_renamed = "kw"), - "supplied more than once" - ) -}) - -test_that("supplying a slot by two different names errors", { - expect_error( - migration_fixture(1, 2, c = 1, c_renamed = 2), - "supplied more than once" - ) -}) - -test_that("too many positional arguments errors", { - expect_error( - migration_fixture(1, 2, 3, 4, 5), - "Too many arguments" - ) -}) - -# ---- generator-level tests (source checkout only) -------------------------- - -test_that("the spliced block is in sync with the registry", { - # tools/ is excluded from the built package (.Rbuildignore), so this only runs - # from a source checkout (local dev + CI). The before-install action guards - # drift in CI. - generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") - skip_if_not(file.exists(generator)) - - gen_env <- new.env() - sys.source(generator, envir = gen_env) - registry <- testthat::test_path("..", "..", "tools", "migrations.R") - migrations <- gen_env$load_migrations(registry) - by_fn <- stats::setNames( - migrations, - vapply(migrations, function(e) e$fn, character(1)) - ) - - fixture <- testthat::test_path("..", "..", "R", "migration-fixture.R") - lines <- readLines(fixture, warn = FALSE) - spliced <- gen_env$splice_blocks(lines, by_fn) - expect_identical(spliced$lines, lines) -}) - -# ---- migrate_recover_args() helper (the engine behind the blocks) ---------- - -# Config equivalent to the fixture, for exercising the helper directly. -fixture_args <- function(dots, current = list(c_renamed = NULL, d = NULL)) { - migrate_recover_args( - dots, - current = current, - recover_new = c("c_renamed", "d"), - recover_old = c("c", "d"), - match_names = c("c", "c_renamed", "d"), - match_to = c("c_renamed", "c_renamed", "d"), - defaults = list(c_renamed = NULL, d = NULL), - head_args = c("a", "b"), - fn_name = "migration_fixture" - ) -} - -test_that("migrate_recover_args() returns NULL when there is nothing to recover", { - expect_null(fixture_args(list())) -}) - -test_that("migrate_recover_args() returns recovered values and message parts", { - res <- fixture_args(list(3, 4)) - expect_equal(res$values, list(c_renamed = 3, d = 4)) - expect_match(res$what, "positional or abbreviated") - expect_match(res$details[[1]], "migration_fixture\\(a, b, c, d\\)") - expect_match(res$details[[2]], "c_renamed = , d = ") -}) - -test_that("migrate_recover_args() errors on an ambiguous abbreviation", { - expect_error( - migrate_recover_args( - list(alph = 2), - current = list(alpha = NULL, alphabet = NULL), - recover_new = c("alpha", "alphabet"), - recover_old = c("alpha", "alphabet"), - match_names = c("alpha", "alphabet"), - match_to = c("alpha", "alphabet"), - defaults = list(alpha = NULL, alphabet = NULL), - head_args = "x", - fn_name = "amb_fixture" - ), - "matches multiple" - ) -}) - -test_that("migrate_recover_args() errors on unknown, conflict, and overflow", { - expect_error(fixture_args(list(foo = 5)), "Unexpected argument") - expect_error( - fixture_args(list(3), current = list(c_renamed = "kw", d = NULL)), - "supplied more than once" - ) - expect_error(fixture_args(list(1, 2, 3)), "Too many arguments") -}) - -test_that("the BEGIN marker may carry a trailing note", { - generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") - skip_if_not(file.exists(generator)) - gen_env <- new.env() - sys.source(generator, envir = gen_env) - - m <- regmatches( - " # BEGIN GENERATED ARG_HANDLE: foo, do not edit, see x", - regexec( - gen_env$begin_re, - " # BEGIN GENERATED ARG_HANDLE: foo, do not edit, see x" - ) - )[[1]] - expect_identical(m[[3]], "foo") -}) diff --git a/tests/testthat/test-migration-fixture.R b/tests/testthat/test-migration-fixture.R new file mode 100644 index 00000000000..fc473bf3696 --- /dev/null +++ b/tests/testthat/test-migration-fixture.R @@ -0,0 +1,165 @@ +# Tests for the in-place argument-migration generator (tools/generate-migrations.R) +# via the fixture `migration_fixture()` (R/migration-fixture.R). Old signature +# f(graph, n, weight, type, directed); new f(graph, n, ..., weights, type, +# directed) with weight -> weights. + +# ---- behaviour -------------------------------------------------------------- + +test_that("a correct new-API call is returned unchanged (and is silent)", { + rlang::local_options(lifecycle_verbosity = "warning") + expect_no_warning( + res <- migration_fixture( + "g", + 5, + weights = 1:3, + type = "in", + directed = TRUE + ) + ) + expect_equal( + res, + list(graph = "g", n = 5, weights = 1:3, type = "in", directed = TRUE) + ) +}) + +test_that("empty dots fall back to the formal defaults", { + expect_equal( + migration_fixture("g", 5), + list(graph = "g", n = 5, weights = NULL, type = "out", directed = FALSE) + ) +}) + +test_that("a legacy positional call is recovered", { + rlang::local_options(lifecycle_verbosity = "warning") + lifecycle::expect_deprecated( + res <- migration_fixture("g", 5, 1:3, "in", TRUE) + ) + expect_equal( + res, + list(graph = "g", n = 5, weights = 1:3, type = "in", directed = TRUE) + ) +}) + +test_that("a renamed-away old name and abbreviations are recovered by name", { + rlang::local_options(lifecycle_verbosity = "warning") + lifecycle::expect_deprecated(res <- migration_fixture("g", 5, weight = 1:3)) + expect_equal(res$weights, 1:3) + lifecycle::expect_deprecated(res <- migration_fixture("g", 5, ty = "in")) + expect_equal(res$type, "in") + lifecycle::expect_deprecated(res <- migration_fixture("g", 5, dir = TRUE)) + expect_true(res$directed) +}) + +test_that("recovery emits a single deprecation warning, not one per slot", { + rlang::local_options(lifecycle_verbosity = "warning") + warnings <- character() + withCallingHandlers( + migration_fixture("g", 5, 1:3, "in", TRUE), + lifecycle_warning_deprecated = function(w) { + warnings <<- c(warnings, conditionMessage(w)) + invokeRestart("muffleWarning") + } + ) + expect_length(warnings, 1L) +}) + +# ---- deprecation message snapshots ----------------------------------------- + +test_that("recovery deprecation messages", { + rlang::local_options(lifecycle_verbosity = "warning") + expect_snapshot(x <- migration_fixture("g", 5, 1:3, "in", TRUE)) + expect_snapshot(x <- migration_fixture("g", 5, 1:3)) + expect_snapshot(x <- migration_fixture("g", 5, weight = 1:3)) + expect_snapshot(x <- migration_fixture("g", 5, ty = "in")) + expect_snapshot(x <- migration_fixture("g", 5, dir = TRUE)) +}) + +test_that("error message snapshots", { + expect_snapshot(migration_fixture("g", 5, weig = 1), error = TRUE) + expect_snapshot(migration_fixture("g", 5, foo = 1), error = TRUE) + expect_snapshot(migration_fixture("g", 5, 1:3, weights = 9), error = TRUE) + expect_snapshot(migration_fixture("g", 5, 1, 2, 3, 4), error = TRUE) +}) + +# ---- migrate_recover_args() helper (the engine behind the blocks) ---------- + +# Config equivalent to the fixture, for exercising the helper directly. +fixture_args <- function( + dots, + current = list(weights = NULL, type = "out", directed = FALSE) +) { + migrate_recover_args( + dots, + current = current, + recover_new = c("weights", "type", "directed"), + recover_old = c("weight", "type", "directed"), + match_names = c("weight", "weights", "type", "directed"), + match_to = c("weights", "weights", "type", "directed"), + defaults = list(weights = NULL, type = "out", directed = FALSE), + head_args = c("graph", "n"), + fn_name = "migration_fixture" + ) +} + +test_that("migrate_recover_args() returns NULL when there is nothing to recover", { + expect_null(fixture_args(list())) +}) + +test_that("migrate_recover_args() returns recovered values and message parts", { + res <- fixture_args(list(1:3, "in", TRUE)) + expect_equal(res$values, list(weights = 1:3, type = "in", directed = TRUE)) + expect_match(res$what, "positional or abbreviated") + expect_match( + res$details[[1]], + "migration_fixture\\(graph, n, weight, type, directed\\)" + ) + expect_match(res$details[[2]], "weights = , type = , directed = ") +}) + +test_that("migrate_recover_args() errors on unknown, ambiguous, conflict, overflow", { + expect_error(fixture_args(list(foo = 5)), "Unexpected argument") + expect_error(fixture_args(list(weig = 1)), "matches multiple") + expect_error( + fixture_args( + list(1:3), + current = list(weights = 9, type = "out", directed = FALSE) + ), + "supplied more than once" + ) + expect_error(fixture_args(list(1, 2, 3, 4)), "Too many arguments") +}) + +# ---- generator-level tests (source checkout only) -------------------------- + +test_that("the generated block is in sync with the registry", { + # tools/ is excluded from the built package (.Rbuildignore), so this only runs + # from a source checkout (local dev + CI). The after-install action guards + # drift in CI. + generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") + skip_if_not(file.exists(generator)) + + gen_env <- new.env() + sys.source(generator, envir = gen_env) + registry <- testthat::test_path("..", "..", "tools", "migrations.R") + migrations <- gen_env$load_migrations(registry) + by_fn <- stats::setNames( + migrations, + vapply(migrations, function(e) e$fn, character(1)) + ) + + fixture <- testthat::test_path("..", "..", "R", "migration-fixture.R") + lines <- readLines(fixture, warn = FALSE) + spliced <- gen_env$splice_blocks(lines, by_fn) + expect_identical(spliced$lines, lines) +}) + +test_that("the BEGIN marker may carry a trailing note", { + generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") + skip_if_not(file.exists(generator)) + gen_env <- new.env() + sys.source(generator, envir = gen_env) + + marker <- " # BEGIN GENERATED ARG_HANDLE: foo, do not edit, see x" + m <- regmatches(marker, regexec(gen_env$begin_re, marker))[[1]] + expect_identical(m[[3]], "foo") +}) From 01f19067ac8f6f7c87b45be289fad59b89bb0c96 Mon Sep 17 00:00:00 2001 From: David Schoch Date: Wed, 3 Jun 2026 08:06:16 +0200 Subject: [PATCH 12/12] test: cover mixed positional + named argument recovery Add a fixture case where a positional value and a named abbreviation are supplied together, pinning down the interleaved recovery and the positional-then-named ordering in the "Detected call" message. Co-Authored-By: Claude Opus 4.8 --- tests/testthat/_snaps/migration-fixture.md | 10 ++++++++++ tests/testthat/test-migration-fixture.R | 12 ++++++++++++ 2 files changed, 22 insertions(+) diff --git a/tests/testthat/_snaps/migration-fixture.md b/tests/testthat/_snaps/migration-fixture.md index 3c2930143ac..411a082cd7b 100644 --- a/tests/testthat/_snaps/migration-fixture.md +++ b/tests/testthat/_snaps/migration-fixture.md @@ -48,6 +48,16 @@ i Detected call: migration_fixture(graph, n, directed) i Use instead: migration_fixture(graph, n, directed = ) +--- + + Code + x <- migration_fixture("g", 5, 1:3, dir = TRUE) + Condition + Warning: + Calling `migration_fixture()` with positional or abbreviated arguments was deprecated in igraph 3.0.0. + i Detected call: migration_fixture(graph, n, weight, directed) + i Use instead: migration_fixture(graph, n, weights = , directed = ) + # error message snapshots Code diff --git a/tests/testthat/test-migration-fixture.R b/tests/testthat/test-migration-fixture.R index fc473bf3696..d43b86da913 100644 --- a/tests/testthat/test-migration-fixture.R +++ b/tests/testthat/test-migration-fixture.R @@ -50,6 +50,16 @@ test_that("a renamed-away old name and abbreviations are recovered by name", { expect_true(res$directed) }) +test_that("positional and named recovery can be mixed in one call", { + rlang::local_options(lifecycle_verbosity = "warning") + lifecycle::expect_deprecated( + res <- migration_fixture("g", 5, 1:3, dir = TRUE) + ) + expect_equal(res$weights, 1:3) + expect_true(res$directed) + expect_equal(res$type, "out") +}) + test_that("recovery emits a single deprecation warning, not one per slot", { rlang::local_options(lifecycle_verbosity = "warning") warnings <- character() @@ -72,6 +82,8 @@ test_that("recovery deprecation messages", { expect_snapshot(x <- migration_fixture("g", 5, weight = 1:3)) expect_snapshot(x <- migration_fixture("g", 5, ty = "in")) expect_snapshot(x <- migration_fixture("g", 5, dir = TRUE)) + # mixed: a positional value and a named abbreviation in the same call + expect_snapshot(x <- migration_fixture("g", 5, 1:3, dir = TRUE)) }) test_that("error message snapshots", {