From db390e6511f3601f60b8e37cd2533844714ccfa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Mon, 29 Jun 2026 14:37:04 +0200 Subject: [PATCH 1/7] docs: improve `max_cliques()` documentation --- R/cliques.R | 25 +++++++++++++++---------- man/cliques.Rd | 25 ++++++++++++++++--------- man/maximal.cliques.Rd | 16 +++++++++------- man/maximal.cliques.count.Rd | 5 +++-- 4 files changed, 43 insertions(+), 28 deletions(-) diff --git a/R/cliques.R b/R/cliques.R index 9005ce8f8cc..4ff9b3fc142 100644 --- a/R/cliques.R +++ b/R/cliques.R @@ -221,8 +221,11 @@ clique.number <- function(graph) { #' vectors of vertex IDs. Each list element is a clique, i.e. a vertex sequence #' of class [igraph.vs][V]. #' -#' `max_cliques()` returns `NULL`, invisibly, if its `file` -#' argument is not `NULL`. The output is written to the specified file in +#' `max_cliques()` returns: +#' - When `file` is `NULL` and `callback` is `NULL`, a list of vertex sequences. +#' - When `callback` is not `NULL`, the callback function is called for each click, +#' then `max_cliques()` returns `NULL` invisibly. +#' - When `file` is not `NULL`, the output is written to the specified file in #' this case. #' #' `clique_num()` and `count_max_cliques()` return an integer @@ -286,16 +289,18 @@ largest_cliques <- function(graph) { } #' @rdname cliques -#' @param subset If not `NULL`, then it must be a vector of vertex IDs, +#' @param subset A vector of vertex IDs, #' numeric or symbolic if the graph is named. The algorithm is run from these #' vertices only, so only a subset of all maximal cliques is returned. See the #' Eppstein paper for details. This argument makes it possible to easily -#' parallelize the finding of maximal cliques. -#' @param file If not `NULL`, then it must be a file name, i.e. a -#' character scalar. The output of the algorithm is written to this file. (If -#' it exists, then it will be overwritten.) Each clique will be a separate line -#' in the file, given with the numeric IDs of its vertices, separated by -#' whitespace. +#' parallelize the finding of maximal cliques. Default: `NULL`, the algorithm +#' is run from all vertices. +#' @param file A file name, i.e. a character scalar. +#' The output of the algorithm is written to this file. +#' (If it exists, then it will be overwritten.) +#' Each clique will be a separate line in the file, +#' given with the numeric IDs of its vertices, separated by whitespace. +#' Default: `NULL`, the function returns a list of vertex sequences. #' @export max_cliques <- function( graph, @@ -323,7 +328,7 @@ max_cliques <- function( tmpfile <- FALSE } on.exit(.Call(Rx_igraph_finalizer)) - res <- .Call( + .Call( Rx_igraph_maximal_cliques_file, graph, subset, diff --git a/man/cliques.Rd b/man/cliques.Rd index a9d7497d016..da026562d45 100644 --- a/man/cliques.Rd +++ b/man/cliques.Rd @@ -61,17 +61,19 @@ so will cause R to crash due to reentrancy issues. Extract any needed graph information before calling the function with a callback, or use collector mode (the default) and process results afterward.} -\item{subset}{If not \code{NULL}, then it must be a vector of vertex IDs, +\item{subset}{A vector of vertex IDs, numeric or symbolic if the graph is named. The algorithm is run from these vertices only, so only a subset of all maximal cliques is returned. See the Eppstein paper for details. This argument makes it possible to easily -parallelize the finding of maximal cliques.} +parallelize the finding of maximal cliques. Default: \code{NULL}, the algorithm +is run from all vertices.} -\item{file}{If not \code{NULL}, then it must be a file name, i.e. a -character scalar. The output of the algorithm is written to this file. (If -it exists, then it will be overwritten.) Each clique will be a separate line -in the file, given with the numeric IDs of its vertices, separated by -whitespace.} +\item{file}{A file name, i.e. a character scalar. +The output of the algorithm is written to this file. +(If it exists, then it will be overwritten.) +Each clique will be a separate line in the file, +given with the numeric IDs of its vertices, separated by whitespace. +Default: \code{NULL}, the function returns a list of vertex sequences.} \item{vertex.weights}{Vertex weight vector. If the graph has a \code{weight} vertex attribute, then this is used by default. If the graph does not have a @@ -95,9 +97,14 @@ of class \link[=V]{igraph.vs}. If \code{callback} is provided, returns \code{NUL vectors of vertex IDs. Each list element is a clique, i.e. a vertex sequence of class \link[=V]{igraph.vs}. -\code{max_cliques()} returns \code{NULL}, invisibly, if its \code{file} -argument is not \code{NULL}. The output is written to the specified file in +\code{max_cliques()} returns: +\itemize{ +\item When \code{file} is \code{NULL} and \code{callback} is \code{NULL}, a list of vertex sequences. +\item When \code{callback} is not \code{NULL}, the callback function is called for each click, +then \code{max_cliques()} returns \code{NULL} invisibly. +\item When \code{file} is not \code{NULL}, the output is written to the specified file in this case. +} \code{clique_num()} and \code{count_max_cliques()} return an integer scalar. diff --git a/man/maximal.cliques.Rd b/man/maximal.cliques.Rd index 0570f2408ac..ae209406885 100644 --- a/man/maximal.cliques.Rd +++ b/man/maximal.cliques.Rd @@ -15,17 +15,19 @@ maximal.cliques(graph, min = NULL, max = NULL, subset = NULL, file = NULL) \item{max}{Numeric constant, upper limit on the size of the cliques to find. \code{NULL} means no limit.} -\item{subset}{If not \code{NULL}, then it must be a vector of vertex IDs, +\item{subset}{A vector of vertex IDs, numeric or symbolic if the graph is named. The algorithm is run from these vertices only, so only a subset of all maximal cliques is returned. See the Eppstein paper for details. This argument makes it possible to easily -parallelize the finding of maximal cliques.} +parallelize the finding of maximal cliques. Default: \code{NULL}, the algorithm +is run from all vertices.} -\item{file}{If not \code{NULL}, then it must be a file name, i.e. a -character scalar. The output of the algorithm is written to this file. (If -it exists, then it will be overwritten.) Each clique will be a separate line -in the file, given with the numeric IDs of its vertices, separated by -whitespace.} +\item{file}{A file name, i.e. a character scalar. +The output of the algorithm is written to this file. +(If it exists, then it will be overwritten.) +Each clique will be a separate line in the file, +given with the numeric IDs of its vertices, separated by whitespace. +Default: \code{NULL}, the function returns a list of vertex sequences.} } \description{ \ifelse{html}{\href{https://lifecycle.r-lib.org/articles/stages.html#deprecated}{\figure{lifecycle-deprecated.svg}{options: alt='[Deprecated]'}}}{\strong{[Deprecated]}} diff --git a/man/maximal.cliques.count.Rd b/man/maximal.cliques.count.Rd index 5dad067399f..954024b1908 100644 --- a/man/maximal.cliques.count.Rd +++ b/man/maximal.cliques.count.Rd @@ -15,11 +15,12 @@ maximal.cliques.count(graph, min = NULL, max = NULL, subset = NULL) \item{max}{Numeric constant, upper limit on the size of the cliques to find. \code{NULL} means no limit.} -\item{subset}{If not \code{NULL}, then it must be a vector of vertex IDs, +\item{subset}{A vector of vertex IDs, numeric or symbolic if the graph is named. The algorithm is run from these vertices only, so only a subset of all maximal cliques is returned. See the Eppstein paper for details. This argument makes it possible to easily -parallelize the finding of maximal cliques.} +parallelize the finding of maximal cliques. Default: \code{NULL}, the algorithm +is run from all vertices.} } \description{ \ifelse{html}{\href{https://lifecycle.r-lib.org/articles/stages.html#deprecated}{\figure{lifecycle-deprecated.svg}{options: alt='[Deprecated]'}}}{\strong{[Deprecated]}} From 857c3fe1141d9baf1ad61980cd38752e57433af5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Mon, 29 Jun 2026 14:55:54 +0200 Subject: [PATCH 2/7] chore!: simplify `max_cliques()` code --- R/cliques.R | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/R/cliques.R b/R/cliques.R index 4ff9b3fc142..83ebe81f351 100644 --- a/R/cliques.R +++ b/R/cliques.R @@ -316,30 +316,21 @@ max_cliques <- function( # Handle file and subset modes (original functionality) if (!is.null(file)) { - if ( - !is.character(file) || - length(grep("://", file, fixed = TRUE)) > 0 || - length(grep("~", file, fixed = TRUE)) > 0 - ) { - tmpfile <- TRUE - origfile <- file - file <- tempfile() - } else { - tmpfile <- FALSE + if (!is.character(file)) { + cli::cli_abort("{.arg file} must be a file path, not a connection.") + } + if (grepl("://", file, fixed = TRUE)) { + cli::cli_abort("{.arg file} must be a file path, not a URL.") } on.exit(.Call(Rx_igraph_finalizer)) .Call( Rx_igraph_maximal_cliques_file, graph, subset, - file, + path.expand(file), as.numeric(min %||% 0), as.numeric(max %||% 0) ) - if (tmpfile) { - buffer <- read.graph.toraw(file) - write.graph.fromraw(buffer, origfile) - } return(invisible(NULL)) } From caabe13a4452cd7598b32ea10e914051aed8fac9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Mon, 29 Jun 2026 14:56:28 +0200 Subject: [PATCH 3/7] chore: remove unused C code --- src/rinterface_extra.c | 31 +++++++------------------------ 1 file changed, 7 insertions(+), 24 deletions(-) diff --git a/src/rinterface_extra.c b/src/rinterface_extra.c index b9699995125..b64abbaa27a 100644 --- a/src/rinterface_extra.c +++ b/src/rinterface_extra.c @@ -5682,15 +5682,10 @@ SEXP Rx_igraph_maximal_cliques(SEXP graph, SEXP psubset, SEXP Rx_igraph_maximal_cliques_file(SEXP graph, SEXP psubset, SEXP file, SEXP pminsize, SEXP pmaxsize) { igraph_t g; - igraph_integer_t minsize=(igraph_integer_t) REAL(pminsize)[0]; - igraph_integer_t maxsize=(igraph_integer_t) REAL(pmaxsize)[0]; + igraph_integer_t minsize = (igraph_integer_t) REAL(pminsize)[0]; + igraph_integer_t maxsize = (igraph_integer_t) REAL(pmaxsize)[0]; igraph_vector_int_t subset; - SEXP result; FILE *stream; -#if HAVE_OPEN_MEMSTREAM == 1 - char *bp; - size_t size; -#endif Rz_SEXP_to_igraph(graph, &g); if (!Rf_isNull(psubset)) { @@ -5699,29 +5694,17 @@ SEXP Rx_igraph_maximal_cliques_file(SEXP graph, SEXP psubset, SEXP file, IGRAPH_R_CHECK(igraph_vector_int_init(&subset, 0)); } IGRAPH_FINALLY_PV(igraph_vector_int_destroy, &subset); -#if HAVE_OPEN_MEMSTREAM == 1 - stream=open_memstream(&bp, &size); -#else - stream=fopen(CHAR(STRING_ELT(file, 0)), "w"); -#endif - if (stream==0) { igraph_error("Cannot write cliques", __FILE__, - __LINE__, IGRAPH_EFILE); } + stream = fopen(CHAR(STRING_ELT(file, 0)), "w"); + if (stream == 0) { + igraph_error("Cannot write cliques", __FILE__, __LINE__, IGRAPH_EFILE); + } igraph_maximal_cliques_subset(&g, Rf_isNull(psubset) ? 0 : &subset, /*ptr=*/ 0, /*no=*/ 0, /*file=*/ stream, minsize, maxsize); fclose(stream); igraph_vector_int_destroy(&subset); IGRAPH_FINALLY_CLEAN(1); -#if HAVE_OPEN_MEMSTREAM == 1 - PROTECT(result=Rf_allocVector(RAWSXP, size)); - memcpy(RAW(result), bp, sizeof(char)*size); - free(bp); -#else - PROTECT(result=NEW_NUMERIC(0)); -#endif - - UNPROTECT(1); - return result; + return R_NilValue; } /* TOP-LEVEL: called from R via .Call; must use IGRAPH_R_CHECK */ From 5fadb442e8ba3c7d8b61400d176ed37dc0b20767 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Jul 2026 14:30:41 +0200 Subject: [PATCH 4/7] oops --- R/cliques.R | 2 +- man/cliques.Rd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/cliques.R b/R/cliques.R index 83ebe81f351..a1f3eef7056 100644 --- a/R/cliques.R +++ b/R/cliques.R @@ -223,7 +223,7 @@ clique.number <- function(graph) { #' #' `max_cliques()` returns: #' - When `file` is `NULL` and `callback` is `NULL`, a list of vertex sequences. -#' - When `callback` is not `NULL`, the callback function is called for each click, +#' - When `callback` is not `NULL`, the callback function is called for each clique, #' then `max_cliques()` returns `NULL` invisibly. #' - When `file` is not `NULL`, the output is written to the specified file in #' this case. diff --git a/man/cliques.Rd b/man/cliques.Rd index da026562d45..b70733c828d 100644 --- a/man/cliques.Rd +++ b/man/cliques.Rd @@ -100,7 +100,7 @@ of class \link[=V]{igraph.vs}. \code{max_cliques()} returns: \itemize{ \item When \code{file} is \code{NULL} and \code{callback} is \code{NULL}, a list of vertex sequences. -\item When \code{callback} is not \code{NULL}, the callback function is called for each click, +\item When \code{callback} is not \code{NULL}, the callback function is called for each clique, then \code{max_cliques()} returns \code{NULL} invisibly. \item When \code{file} is not \code{NULL}, the output is written to the specified file in this case. From 13af5a9f8917c1af930e7d41e8f733730dd70b73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Thu, 9 Jul 2026 14:03:56 +0200 Subject: [PATCH 5/7] c changes --- src/rinterface_extra.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/rinterface_extra.c b/src/rinterface_extra.c index b64abbaa27a..c9d1834a5c4 100644 --- a/src/rinterface_extra.c +++ b/src/rinterface_extra.c @@ -4787,7 +4787,9 @@ SEXP Rx_igraph_read_graph_edgelist(SEXP pvfile, SEXP pn, SEXP pdirected) { #endif if (file==0) { igraph_error("Cannot read edgelist", __FILE__, __LINE__, IGRAPH_EFILE); } + IGRAPH_FINALLY(fclose, file); IGRAPH_R_CHECK(igraph_read_graph_edgelist(&g, file, n, directed)); + IGRAPH_FINALLY_CLEAN(1); fclose(file); PROTECT(result=Ry_igraph_to_SEXP(&g)); IGRAPH_I_DESTROY(&g); @@ -4815,11 +4817,13 @@ SEXP Rx_igraph_read_graph_ncol(SEXP pvfile, SEXP ppredef, #endif if (file==0) { igraph_error("Cannot read edgelist", __FILE__, __LINE__, IGRAPH_EFILE); } + IGRAPH_FINALLY(fclose, file); if (Rf_xlength(ppredef)>0) { Rx_igraph_SEXP_to_strvector(ppredef, &predef); predefptr=&predef; } IGRAPH_R_CHECK(igraph_read_graph_ncol(&g, file, predefptr, names, weights, directed)); + IGRAPH_FINALLY_CLEAN(1); fclose(file); PROTECT(result=Ry_igraph_to_SEXP(&g)); IGRAPH_I_DESTROY(&g); @@ -4859,7 +4863,9 @@ SEXP Rx_igraph_write_graph_ncol(SEXP graph, SEXP file, SEXP pnames, #endif if (stream==0) { igraph_error("Cannot write .ncol file", __FILE__, __LINE__, IGRAPH_EFILE); } + IGRAPH_FINALLY(fclose, stream); IGRAPH_R_CHECK(igraph_write_graph_ncol(&g, stream, names, weights)); + IGRAPH_FINALLY_CLEAN(1); fclose(stream); #if HAVE_OPEN_MEMSTREAM == 1 PROTECT(result=Rf_allocVector(RAWSXP, size)); @@ -4889,7 +4895,9 @@ SEXP Rx_igraph_read_graph_lgl(SEXP pvfile, SEXP pnames, SEXP pweights, SEXP pdir #endif if (file==0) { igraph_error("Cannot read edgelist", __FILE__, __LINE__, IGRAPH_EFILE); } + IGRAPH_FINALLY(fclose, file); IGRAPH_R_CHECK(igraph_read_graph_lgl(&g, file, names, weights, directed)); + IGRAPH_FINALLY_CLEAN(1); fclose(file); PROTECT(result=Ry_igraph_to_SEXP(&g)); IGRAPH_I_DESTROY(&g); @@ -4928,7 +4936,11 @@ SEXP Rx_igraph_write_graph_lgl(SEXP graph, SEXP file, SEXP pnames, #else stream=fopen(CHAR(STRING_ELT(file, 0)), "w"); #endif + if (stream==0) { igraph_error("Cannot write LGL file", __FILE__, __LINE__, + IGRAPH_EFILE); } + IGRAPH_FINALLY(fclose, stream); IGRAPH_R_CHECK(igraph_write_graph_lgl(&g, stream, names, weights, isolates)); + IGRAPH_FINALLY_CLEAN(1); fclose(stream); #if HAVE_OPEN_MEMSTREAM == 1 PROTECT(result=Rf_allocVector(RAWSXP, size)); @@ -5375,10 +5387,12 @@ SEXP Rx_igraph_read_graph_dimacs(SEXP pvfile, SEXP pdirected) { if (file==0) { igraph_error("Cannot read edgelist", __FILE__, __LINE__, IGRAPH_EFILE); } + IGRAPH_FINALLY(fclose, file); igraph_vector_int_init(&label, 0); igraph_strvector_init(&problem, 0); igraph_vector_init(&cap, 0); IGRAPH_R_CHECK(igraph_read_graph_dimacs_flow(&g, file, &problem, &label, &source, &target, &cap, directed)); + IGRAPH_FINALLY_CLEAN(1); fclose(file); if (!strcmp(igraph_strvector_get(&problem, 0), "max")) { PROTECT(result=NEW_LIST(5)); px++; @@ -5437,7 +5451,9 @@ SEXP Rx_igraph_write_graph_dimacs(SEXP graph, SEXP file, if (stream==0) { igraph_error("Cannot write edgelist", __FILE__, __LINE__, IGRAPH_EFILE); } + IGRAPH_FINALLY(fclose, stream); IGRAPH_R_CHECK(igraph_write_graph_dimacs_flow(&g, stream, source, target, &cap)); + IGRAPH_FINALLY_CLEAN(1); fclose(stream); #if HAVE_OPEN_MEMSTREAM == 1 PROTECT(result=Rf_allocVector(RAWSXP, size)); @@ -5698,9 +5714,11 @@ SEXP Rx_igraph_maximal_cliques_file(SEXP graph, SEXP psubset, SEXP file, if (stream == 0) { igraph_error("Cannot write cliques", __FILE__, __LINE__, IGRAPH_EFILE); } + IGRAPH_FINALLY(fclose, stream); igraph_maximal_cliques_subset(&g, Rf_isNull(psubset) ? 0 : &subset, /*ptr=*/ 0, /*no=*/ 0, /*file=*/ stream, minsize, maxsize); + IGRAPH_FINALLY_CLEAN(1); fclose(stream); igraph_vector_int_destroy(&subset); IGRAPH_FINALLY_CLEAN(1); From 9657f6d86ef929302f816c5742b3f139eddb0467 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Thu, 9 Jul 2026 14:04:22 +0200 Subject: [PATCH 6/7] change argument order --- R/cliques.R | 2 +- man/cliques.Rd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/cliques.R b/R/cliques.R index a1f3eef7056..78d35b82201 100644 --- a/R/cliques.R +++ b/R/cliques.R @@ -307,8 +307,8 @@ max_cliques <- function( min = NULL, max = NULL, subset = NULL, - file = NULL, ..., + file = NULL, callback = NULL ) { ensure_igraph(graph) diff --git a/man/cliques.Rd b/man/cliques.Rd index b70733c828d..80cbde32e0d 100644 --- a/man/cliques.Rd +++ b/man/cliques.Rd @@ -21,8 +21,8 @@ max_cliques( min = NULL, max = NULL, subset = NULL, - file = NULL, ..., + file = NULL, callback = NULL ) From f938e16a200b7154a229bc2eccc9a70467eaab1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Wed, 12 Aug 2026 13:38:15 +0200 Subject: [PATCH 7/7] Claude's answer to Kirill's comments --- R/cliques.R | 23 ++- tests/testthat/test-migration-fixture.R | 145 +++++++++++++++++++ tools/generate-migrations.R | 179 ++++++++++++++++++++++-- tools/migrations.R | 27 ++++ 4 files changed, 362 insertions(+), 12 deletions(-) diff --git a/R/cliques.R b/R/cliques.R index 78d35b82201..da2aa669092 100644 --- a/R/cliques.R +++ b/R/cliques.R @@ -312,7 +312,28 @@ max_cliques <- function( callback = NULL ) { ensure_igraph(graph) - check_dots_empty() + + # BEGIN GENERATED ARG_HANDLE: max_cliques, do not edit, see tools/generate-migrations.R + if (...length() > 0L) { + .arg_handle <- migrate_recover_args( + list(...), + current = list(file = file, callback = callback), + recover_new = c("file"), + recover_old = c("file"), + match_names = c("file", "callback"), + match_to = c("file", "callback"), + defaults = list(file = NULL, callback = NULL), + head_args = c("graph", "min", "max", "subset"), + fn_name = "max_cliques" + ) + list2env(.arg_handle$values, environment()) + lifecycle::deprecate_soft( + "3.0.0", + what = I(.arg_handle$what), + details = .arg_handle$details + ) + } + # END GENERATED ARG_HANDLE # Handle file and subset modes (original functionality) if (!is.null(file)) { diff --git a/tests/testthat/test-migration-fixture.R b/tests/testthat/test-migration-fixture.R index 4e952681631..dc466f7f60f 100644 --- a/tests/testthat/test-migration-fixture.R +++ b/tests/testthat/test-migration-fixture.R @@ -247,3 +247,148 @@ test_that("render_call_arg() wraps long arguments the way air formats them", { ) expect_true(all(nchar(wrapped) + 2L <= 80L)) }) + +test_that("`...` in `old` marks a name-only boundary, not a positional slot", { + # max_cliques() is migrated a second time: `file` moves from positional to + # keyword-only, past `...` -- and `old` documents the signature as it stood + # just before that, which already had `...` (from an earlier, unregistered + # change that added `callback` as keyword-only). `callback` must land past + # `old`'s own `...` and stay name-only-recoverable, never folded into the + # positional `recover_old`/`recover_new` pair. + generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") + skip_if_not(file.exists(generator)) + gen_env <- new.env() + sys.source(generator, envir = gen_env) + + entry <- gen_env$normalise_migration( + "fn", + list( + old = function(graph, min, max, subset, file, ..., callback) {}, + new = function( + graph, + min = NULL, + max = NULL, + subset = NULL, + ..., + file = NULL, + callback = NULL + ) {} + ) + ) + + expect_identical(entry$old, c("graph", "min", "max", "subset", "file")) + expect_identical(entry$recover_old, "file") + expect_identical(entry$recover_new, "file") + # `callback` is matchable by name (it is already in `new`'s tail) but does + # not appear a second time as a positionally-recoverable slot. + expect_identical(entry$match_names, c("file", "callback")) + expect_identical(entry$match_to, c("file", "callback")) +}) + +test_that("inject_block() splices a fresh block at the top of a bare function", { + generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") + skip_if_not(file.exists(generator)) + gen_env <- new.env() + sys.source(generator, envir = gen_env) + + entry <- gen_env$normalise_migration( + "fn", + list( + old = function(graph, n, weights, directed) {}, + new = function(graph, n, ..., weights = NULL, directed = FALSE) {} + ) + ) + + lines <- c( + "#' @noRd", + "fn <- function(", + " graph,", + " n,", + " ...,", + " weights = NULL,", + " directed = FALSE", + ") {", + " do_the_thing(graph)", + "}" + ) + + out <- gen_env$inject_block(lines, "fn", entry) + + expect_identical(out[seq_len(8)], lines[seq_len(8)]) + expect_true(any(grepl("BEGIN GENERATED ARG_HANDLE: fn", out))) + expect_true(any(grepl("END GENERATED ARG_HANDLE", out))) + # Injected before the pre-existing body, i.e. at the very beginning. + begin_at <- which(grepl("BEGIN GENERATED ARG_HANDLE", out)) + body_at <- which(grepl("do_the_thing", out)) + expect_lt(begin_at, body_at) + expect_identical(out[[length(out)]], "}") + + # Idempotent once spliced: re-running the normal marker-based path leaves it + # unchanged. + spliced <- gen_env$splice_blocks(out, list(fn = entry)) + expect_identical(spliced$lines, out) + expect_identical(spliced$filled, "fn") +}) + +test_that("inject_block() returns NULL when the function isn't in `lines`", { + generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R") + skip_if_not(file.exists(generator)) + gen_env <- new.env() + sys.source(generator, envir = gen_env) + + entry <- gen_env$normalise_migration( + "fn", + list( + old = function(graph, n, weights) {}, + new = function(graph, n, ..., weights = NULL) {} + ) + ) + expect_null(gen_env$inject_block( + c("other_fn <- function(graph) {", "}"), + "fn", + entry + )) +}) + +test_that("generate_migrations() injects a missing block and warns, then stays quiet", { + 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_dir <- withr::local_tempdir() + registry <- file.path(registry_dir, "migrations.R") + writeLines( + c( + "migrations <- list(", + " fn = list(", + " old = function(graph, n, weights) {},", + " new = function(graph, n, ..., weights = NULL) {}", + " )", + ")" + ), + registry + ) + + src_dir <- withr::local_tempdir() + fn_file <- file.path(src_dir, "fn.R") + writeLines( + c( + "fn <- function(graph, n, ..., weights = NULL) {", + " do_the_thing(graph)", + "}" + ), + fn_file + ) + + expect_warning( + gen_env$generate_migrations(registry, src_dir), + "injected" + ) + lines <- readLines(fn_file, warn = FALSE) + expect_true(any(grepl("BEGIN GENERATED ARG_HANDLE: fn", lines))) + + # Re-running now finds the marker and is a silent no-op. + expect_no_warning(gen_env$generate_migrations(registry, src_dir)) + expect_identical(readLines(fn_file, warn = FALSE), lines) +}) diff --git a/tools/generate-migrations.R b/tools/generate-migrations.R index a478ff68582..3dc7808c0e7 100644 --- a/tools/generate-migrations.R +++ b/tools/generate-migrations.R @@ -17,6 +17,11 @@ # Usage: # Rscript tools/generate-migrations.R # +# If a registry entry has no marker pair anywhere yet, one is injected at the +# very beginning of the function's body (with a warning) instead of leaving +# the migration unwired -- move it by hand afterwards if a later placement +# reads better, then re-run the generator. +# # 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 @@ -85,7 +90,21 @@ normalise_migration <- function(fn, entry) { old_fmls <- formals(entry$old) new_fmls <- formals(entry$new) - entry$old <- names(old_fmls) + + old_names_raw <- names(old_fmls) + old_dots_idx <- which(old_names_raw == "...") + # `old` may itself carry `...` -- this migration is not the function's + # first: an earlier migration already made the names past it keyword-only, + # so they were never positionally callable and must stay recoverable by + # (partial) name only, never folded into the positional recovery below. + if (length(old_dots_idx) == 1L) { + old_positional <- old_names_raw[seq_len(old_dots_idx - 1L)] + old_keyword_only <- old_names_raw[(old_dots_idx + 1L):length(old_names_raw)] + } else { + old_positional <- old_names_raw + old_keyword_only <- character(0) + } + entry$old <- old_positional entry$new <- names(new_fmls) if (sum(entry$new == "...") != 1L) { @@ -99,7 +118,7 @@ normalise_migration <- function(fn, entry) { # Renames: an old formal whose default is a bare symbol points at its new name. renames <- character(0) - for (nm in entry$old) { + for (nm in c(old_positional, old_keyword_only)) { if (!nzchar(default_expr(old_fmls, nm))) { next } # no default -> no rename @@ -155,7 +174,18 @@ normalise_migration <- function(fn, entry) { USE.NAMES = FALSE ) - bad <- setdiff(entry$recover_new, entry$tail) + # Old names already past `old`'s own `...` map through the same renames, but + # -- unlike `recover_old`/`recover_new` -- never drive positional recovery. + keyword_only_new <- vapply( + old_keyword_only, + function(nm) { + if (nm %in% names(entry$renames)) entry$renames[[nm]] else nm + }, + character(1), + USE.NAMES = FALSE + ) + + bad <- setdiff(c(entry$recover_new, keyword_only_new), entry$tail) if (length(bad)) { stop( "Migration `", @@ -171,8 +201,17 @@ normalise_migration <- function(fn, entry) { # 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) + keyword_renamed <- old_keyword_only != keyword_only_new + entry$match_names <- c( + entry$recover_old[renamed], + old_keyword_only[keyword_renamed], + entry$tail + ) + entry$match_to <- c( + entry$recover_new[renamed], + keyword_only_new[keyword_renamed], + entry$tail + ) entry } @@ -322,6 +361,91 @@ splice_blocks <- function(lines, by_fn) { list(lines = out, filled = filled) } +# ---- injecting a block when no marker exists ------------------------------- + +# Escape a function name for literal use inside a regex. Registry names are +# always identifiers (`[A-Za-z0-9._]+`, per `begin_re` above), so `.` is the +# only metacharacter that can occur. +regex_escape <- function(x) { + gsub(".", "\\.", x, fixed = TRUE) +} + +# A registry entry with no `# BEGIN GENERATED ARG_HANDLE` marker anywhere +# would otherwise stay silently unwired -- easy to miss when adding a new +# migration, since the generator would just warn and move on. Instead, locate +# the function's definition (` <- function(`) and inject a freshly +# rendered block right after the opening `{` of its body: the safest default, +# since it then runs before anything else touches the (possibly still-legacy) +# arguments. It is very likely not the *best* spot -- an early argument-check +# such as `ensure_igraph()` usually reads better before it -- so the caller +# always pairs this with a warning asking for a manual look. Returns NULL +# (added nothing) if `fn`'s definition is not in `lines`. +inject_block <- function(lines, fn, entry) { + def_re <- paste0("^", regex_escape(fn), "\\s*<-\\s*function\\s*\\(") + idx <- which(grepl(def_re, lines)) + if (length(idx) == 0L) { + return(NULL) + } + i <- idx[[1]] + + # Walk forward from the definition, tracking paren depth, to the line where + # the signature's closing `)` brings it back to zero -- this spans however + # many lines the (possibly multi-line, `air`-formatted) signature takes. + depth <- 0L + j <- i + repeat { + ch <- strsplit(lines[[j]], "", fixed = TRUE)[[1]] + depth <- depth + sum(ch == "(") - sum(ch == ")") + if (depth <= 0L) { + break + } + j <- j + 1L + if (j > length(lines)) { + stop( + "Could not find the end of `", + fn, + "`'s signature while injecting an ARG_HANDLE block.", + call. = FALSE + ) + } + } + + # The body's opening `{` is usually on the same line as that closing `)` + # (`air`'s style); scan forward otherwise. + while (!grepl("{", lines[[j]], fixed = TRUE)) { + j <- j + 1L + if (j > length(lines)) { + stop( + "Could not find the opening `{` of `", + fn, + "`'s body while injecting an ARG_HANDLE block.", + call. = FALSE + ) + } + } + brace_pos <- regexpr("{", lines[[j]], fixed = TRUE)[[1]] + before <- substring(lines[[j]], 1L, brace_pos) + after <- substring(lines[[j]], brace_pos + 1L) + + block <- c( + paste0( + " # BEGIN GENERATED ARG_HANDLE: ", + fn, + ", do not edit, see tools/generate-migrations.R" + ), + paste0(" ", render_arg_handle(entry)), + " # END GENERATED ARG_HANDLE" + ) + + c( + lines[seq_len(j - 1L)], + before, + block, + if (nzchar(after)) after, + if (j < length(lines)) lines[(j + 1L):length(lines)] + ) +} + # ---- driver ---------------------------------------------------------------- generate_migrations <- function(registry_path, src_dir) { @@ -332,25 +456,58 @@ generate_migrations <- function(registry_path, src_dir) { ) files <- list.files(src_dir, pattern = "\\.R$", full.names = TRUE) + orig_lines <- stats::setNames(lapply(files, readLines, warn = FALSE), files) + file_lines <- orig_lines filled <- character() + for (f in files) { - lines <- readLines(f, warn = FALSE) + lines <- file_lines[[f]] if (!any(grepl(begin_re, lines))) { next } res <- splice_blocks(lines, by_fn) + file_lines[[f]] <- res$lines filled <- c(filled, res$filled) - if (!identical(res$lines, lines)) { - writeLines(res$lines, f) + } + + # Entries that never had a marker: inject one rather than leave them unwired. + missing <- setdiff(names(by_fn), filled) + for (fn in missing) { + for (f in files) { + new_lines <- inject_block(file_lines[[f]], fn, by_fn[[fn]]) + if (is.null(new_lines)) { + next + } + file_lines[[f]] <- new_lines + filled <- c(filled, fn) + warning( + paste0( + "No `# BEGIN GENERATED ARG_HANDLE` marker found for `", + fn, + "`; injected one at the very beginning of its body in ", + f, + ". An early argument-checking guard usually belongs before it -- ", + "review the placement, then re-run the generator." + ), + call. = FALSE + ) + break + } + } + + for (f in files) { + if (!identical(file_lines[[f]], orig_lines[[f]])) { + writeLines(file_lines[[f]], f) message("updated ", f) } } - missing <- setdiff(names(by_fn), filled) - if (length(missing)) { + still_missing <- setdiff(names(by_fn), filled) + if (length(still_missing)) { warning( "No `# BEGIN GENERATED ARG_HANDLE` marker found for: ", - paste(missing, collapse = ", "), + paste(still_missing, collapse = ", "), + ", and no matching ` <- function(` definition to inject one into.", call. = FALSE ) } diff --git a/tools/migrations.R b/tools/migrations.R index e18c7283b99..3a88237748e 100644 --- a/tools/migrations.R +++ b/tools/migrations.R @@ -30,6 +30,13 @@ # 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` may itself contain `...`, when the function was already +# migrated once before and this entry captures a *later* migration on +# top of that (e.g. reordering an argument that already sits after +# `...`). Names before it are still positional; names after it were +# already keyword-only and stay recoverable by (partial) name only, +# never by position. +# # new The post-migration signature. Must contain exactly one `...`. The # non-`...` formals are the new-API arguments, in order; their defaults # become the function's defaults and the values the conflict check @@ -95,6 +102,26 @@ migrations <- list( when = "3.0.0" ), + # `file` moves from a positional slot to keyword-only, past `...`, so a call + # that used to pass it as the 5th positional argument still needs recovering. + # `old` documents the signature as it stood just before this reordering -- + # which already had `...` in it (a previous, unregistered change had added + # `callback` as keyword-only) -- so `callback` sits after `old`'s own `...` + # and is recovered by name only, exactly as it already was. + max_cliques = list( + old = function(graph, min, max, subset, file, ..., callback) {}, + new = function( + graph, + min = NULL, + max = NULL, + subset = NULL, + ..., + file = NULL, + callback = NULL + ) {}, + when = "3.0.0" + ), + # --- test fixture -------------------------------------------------------- # Exercises the generator end-to-end without migrating a real function. The # arg names are chosen to cover every recovery path: two renames (`weight ->