fix(migrations): recover legacy arguments against the old signature by krlmlr · Pull Request #2812 · igraph/rigraph

krlmlr · 2026-08-05T21:20:12Z

Supersedes #2808 and #2810 — one commit, based on main, nothing stacked. Prepared with Claude Code.

Fixes the four argument is missing, with no default revdep failures from #2646, by removing the runtime argument matcher rather than patching it.

The bug

A call may leave an argument slot empty — a trailing comma (f(x, mode = "out", )) or a skipped positional, which is what magrittr writes for x %>% f(., , directed = FALSE). Under the old signatures those slots matched a formal by position and left it missing, so the formal's default applied.

Now that the optional arguments sit behind ... (#2757–#2778), the empty slot lands in ..., where the generated block's list(...) forces it:

Error in igraph::graph_from_adjacency_matrix(adjmatrix = x, mode = mode, weighted = TRUE, ) :
  argument is missing, with no default
Calls: ... -> <Anonymous> -> migrate_recover_args

Four revdeps are affected: tna (31 test failures, plus examples and vignettes), lagdynamics, modelbpp, NetSci.

The fix

migrate_recover_args() re-implemented base R's argument matching over list(...) and seven parallel configuration vectors. But that matching is something R already does, and did do before the migration — so the block declares the old signature and lets R match against it:

# fmt: skip
if (...length() > 0L) {
  # Pre-3.0.0 signature: migration_fixture(graph, n, weight, kind, directed)
  .old_signature <- function(weight, kind, directed, ...) {
    if (...length() > 0L) {
      .arg_extra <- base::names(base::substitute(...()))
      .arg_extra <- .arg_extra[base::nzchar(.arg_extra)]
      if (base::length(.arg_extra) == 0L) cli::cli_abort("Too many arguments passed to {.fn migration_fixture}.", call = base::parent.frame())
      cli::cli_abort(base::c("Unexpected argument passed to {.fn migration_fixture}: {.arg {(.arg_extra)}}.", i = "Arguments after {.arg ...} must be spelled out in full."), call = base::parent.frame())
    }
    base::c(
      if (!base::missing(weight)) base::list(weights = weight),
      if (!base::missing(kind)) base::list(type = kind),
      if (!base::missing(directed)) base::list(directed = directed)
    )
  }
  .arg_handle <- .old_signature(...)
  if (base::length(.arg_handle) > 0L) {
    .arg_names <- base::names(.arg_handle)
    .arg_conflict <- base::intersect(.arg_names, base::c(
      if (!base::missing(weights)) "weights",
      if (!base::missing(type)) "type",
      if (!base::missing(directed)) "directed"
    ))
    if (base::length(.arg_conflict) > 0L) cli::cli_abort(base::c("Argument {.arg {(.arg_conflict)}} of {.fn migration_fixture} was supplied more than once.", i = "Pass it exactly once, by its new name {.arg {(.arg_conflict)}}."))
    base::list2env(.arg_handle, base::environment())
    lifecycle::deprecate_soft(
      "3.0.0",
      what = base::I("Calling `migration_fixture()` with positional or abbreviated arguments"),
      details = base::c(
        i = base::paste0("Detected call:  migration_fixture(", base::paste(base::c("graph", "n", base::c(weights = "weight", type = "kind", directed = "directed")[.arg_names]), collapse = ", "), ")"),
        i = base::paste0("Use instead:    migration_fixture(", base::paste(base::c("graph", "n", base::paste0(.arg_names, " = ")), collapse = ", "), ")")
      )
    )
  }
}

#2646 falls out of it. ... is never materialised as a list, so an empty slot passes straight through into .old_signature(), leaves its formal missing, and still consumes its position — precisely the pre-migration behaviour. f(g, 5, , "in", TRUE) lands on the second and third recoverable arguments without anything having to arrange for it, and rlang::pairlist2() (which #2810 needed) is not involved.

Everything is unrolled, one line per argument: if (!base::missing(<old>)) picks the value up, and if (!base::missing(<new>)) reports it as already supplied. Asking missing() in the host frame answers "was this passed" directly, so the block needs neither the defaults nor a comparison against them.

No runtime layer is left. The guards and messages are inline too, so R/migrate-args.R is deleted; lifecycle::deprecate_soft() still sits in the host frame and resolves user_env to the user on its own.

Every call is namespace-qualified. The block is spliced into someone else's function body, where a formal may be named names or c — and a missing formal is worse than a shadowing one, since R forces the promise while looking for a function to call and reports argument "names" is missing. Qualifying unconditionally keeps every block the same shape instead of making it depend on the signature. Only operators and ...length() are left bare — a formal cannot shadow those. The measured cost of base:: on the hot ...length() guard is ~80 ns per call.

Behaviour is unchanged, down to the message text

The diff to the existing snapshots is empty; the new entries are additions. Two things were needed for that:

.old_signature() takes a trailing ... of its own, so an unknown or surplus argument lands there instead of tripping base R's "unused argument"; it is reported with the message the matcher produced. The tags come from substitute(...()), which forces nothing — a surplus slot may itself be empty.
Ambiguous abbreviations are rejected up front. .old_signature() only knows the old names, so it would silently resolve some abbreviations the matcher rejected — at for as_adjacency_matrix(attr =), which the migration renamed to weights while a deprecated attr formal remains. The generator enumerates those tags (48 functions) and the block tests the dot tags against them first.

One case is deliberately stricter: an argument passed explicitly at its own default value, together with a positional recovering into it, is now a conflict. migration_fixture(g, 5, weights = NULL, 1:3) supplies weights twice and errors, where comparing against the default could not tell the two apart and let the positional win silently.

Tests

A third fixture, migration_fixture_shadow(graph, ..., weights, names, c, attr), covers the two hazards a block faces in a real function body — both taken from as_adjacency_matrix(), which had no direct coverage: formals named names and c that shadow the block's own calls, and attr renamed to weights while a deprecated attr formal keeps the name. Every path through it is asserted deterministically and snapshotted: recovery with the shadowing formals missing (the dangerous case), recovery of a shadowing formal by abbreviation and by position, attr = binding the retained formal, and the ambiguous, unknown, surplus and supplied-twice errors.

The empty-slot behaviour is snapshotted alongside its deterministic tests, because the Detected call line is the readable form of the position bookkeeping:

> migration_fixture("g", 5, , "in", TRUE)
Warning:
Calling `migration_fixture()` with positional or abbreviated arguments was deprecated in igraph 3.0.0.
i Detected call: migration_fixture(graph, n, kind, directed)
i Use instead: migration_fixture(graph, n, type = , directed = )

One more edge is pinned: a slot past the last recoverable one is surplus whether or not it is empty, so migration_fixture("g", 5, 1, 2, 3, ) errors — the old signature had nothing to match a sixth argument against either.

Cost

The generated blocks grow by ~2000 lines across all 227 — 21 lines per block before against ~30, and 60 for the widest (layout_with_fr(), 17 recoverable arguments). The blocks open with # fmt: skip so air leaves the layout to the generator: that buys some of it back, and it removed the code that had to predict how air would wrap.

Verification

Full testthat::test_local() green apart from two failures that need the environment rather than the code: test-foreign.R:69 downloads from github.com/igraph/graphsdb (no network here) and test-other.R:20 starts a callr subprocess that needs igraph installed rather than loaded via pkgload.
Both Revdep Failures #2646 reproducers, plus graph_from_data_frame(d, , ) (all slots empty — defaults kept, silent), behave as they do on CRAN 2.3.1.
Generator output is untouched by air format ..

By submitting this pull request, I assign the copyright of my contribution to The igraph development team.

krlmlr · 2026-08-05T21:26:39Z

Another idea for fixing: inlining and unrolling. Generated code doesn't really need static helper functions, can do everything from first principles. Work incoming.

github-actions · 2026-08-05T21:44:46Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if c186c6b is merged into main:

✔️as_adjacency_matrix: 640ms -> 638ms [-3.07%, +2.46%]
✔️as_biadjacency_matrix: 573ms -> 576ms [-1.71%, +2.76%]
✔️as_data_frame_both: 1.31ms -> 1.31ms [-2.65%, +2.9%]
✔️as_long_data_frame: 3.36ms -> 3.38ms [-1.58%, +2.75%]
✔️es_attr_filter: 2.33ms -> 2.39ms [-0.78%, +5.2%]
✔️graph_from_adjacency_matrix: 107ms -> 106ms [-9.47%, +6.89%]
✔️graph_from_data_frame: 2.87ms -> 2.93ms [-0.08%, +4.34%]
✔️vs_attr_filter: 1.22ms -> 1.22ms [-0.98%, +1.93%]
✔️vs_by_name: 816µs -> 838µs [-2.69%, +8.02%]
Further explanation regarding interpretation and methodology can be found in the documentation.

krlmlr · 2026-08-05T21:46:43Z

Can it really be as simple as this? My favorite so far. A bit longer, but wins by a margin when debugging. Need to iterate a bit, let me know what you think.

github-actions · 2026-08-06T08:53:48Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 4440b1c is merged into main:

🚀as_adjacency_matrix: 772ms -> 751ms [-3.4%, -1.83%]
✔️as_biadjacency_matrix: 733ms -> 732ms [-1.47%, +1.28%]
❗🐌as_data_frame_both: 1.51ms -> 1.56ms [+2.12%, +4.26%]
✔️as_long_data_frame: 4.01ms -> 4.02ms [-1.7%, +2.17%]
✔️es_attr_filter: 2.69ms -> 2.75ms [-0.02%, +4.5%]
✔️graph_from_adjacency_matrix: 120ms -> 120ms [-1%, +1.53%]
❗🐌graph_from_data_frame: 3.56ms -> 3.76ms [+1.42%, +9.86%]
✔️vs_attr_filter: 1.48ms -> 1.47ms [-1.44%, +0.24%]
✔️vs_by_name: 961µs -> 952µs [-2.07%, +0.21%]
Further explanation regarding interpretation and methodology can be found in the documentation.

github-actions · 2026-08-06T09:23:36Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 632c4b5 is merged into main:

🚀as_adjacency_matrix: 772ms -> 749ms [-4.11%, -1.84%]
✔️as_biadjacency_matrix: 761ms -> 767ms [-0.89%, +2.63%]
❗🐌as_data_frame_both: 1.51ms -> 1.59ms [+3.67%, +6.34%]
✔️as_long_data_frame: 4.02ms -> 4.07ms [-0.3%, +2.95%]
✔️es_attr_filter: 2.75ms -> 2.73ms [-3.45%, +1.84%]
✔️graph_from_adjacency_matrix: 119ms -> 120ms [-0.14%, +2.05%]
❗🐌graph_from_data_frame: 3.53ms -> 3.76ms [+4.65%, +8.03%]
✔️vs_attr_filter: 1.59ms -> 1.62ms [-1.95%, +5.04%]
✔️vs_by_name: 1.05ms -> 1.04ms [-3.93%, +2.25%]
Further explanation regarding interpretation and methodology can be found in the documentation.

krlmlr · 2026-08-06T09:33:09Z

One more change (new snapshot tests), then ready for review.

…2646) Supersedes #2808 and #2810. `migrate_recover_args()` re-implemented base R's argument matching over a `list(...)` and seven parallel configuration vectors. It is replaced here by matching `...` against the old signature itself. Each generated block carries `.old_signature()`, a local function whose formals are the pre-migration tail arguments, plus a trailing `...` of its own. Base R then does the recovery -- positionally, by partial name, and leaving an empty argument slot missing while it still consumes its position. That last part is the #2646 fix: a trailing comma (`f(x, mode = "out", )`) or a skipped positional (magrittr's `x %>% f(., , directed = FALSE)`) used to be forced by `list(...)` and error with "argument is missing, with no default"; now `...` is never materialised as a list at all. Everything is unrolled, one line per argument: `if (!base::missing(<old>)) base::list(<new> = <old>)` picks the value up, and `if (!base::missing(<new>)) "<new>"` reports it as already supplied. Asking `missing()` in the host frame answers "was this passed" directly, so the block needs neither the defaults nor a comparison against them. The guards and messages are inline too, so R/migrate-args.R goes away entirely and `lifecycle::deprecate_soft()` keeps resolving `user_env` to the user's frame on its own. Every call in a generated block is namespace-qualified. The block is spliced into someone else's function body, where a formal may be named `names` or `c` -- and a *missing* formal is worse than a shadowing one, since R forces the promise while looking for a function to call and reports `argument "names" is missing`. Qualifying unconditionally keeps every block the same shape instead of making it depend on the signature. Behaviour is unchanged, down to the message text -- the diff to the existing snapshots is empty: * `.old_signature()`'s own `...` catches unknown and surplus arguments, which keep the messages the matcher produced instead of R's "unused argument". * Abbreviations that were ambiguous between an old and a new name are rejected up front, since `.old_signature()` only knows the old names and would otherwise resolve some of them silently -- `at` for `as_adjacency_matrix(attr =)`, renamed to `weights`, while a deprecated `attr` formal remains. One case is now stricter: an argument passed explicitly at its own default value is a conflict with a positional recovering into it, where comparing against the default could not tell the two apart. `migration_fixture_shadow()` is a third fixture for the two hazards a block faces in a real function body, both taken from `as_adjacency_matrix()`: formals named `names` and `c` that shadow the block's own calls, and a name renamed away while a deprecated formal keeps it. Snapshots now cover the empty-slot messages, where the "Detected call" line is the readable form of the position bookkeeping, and every path through the new fixture. The blocks open with `# fmt: skip`, so `air` leaves the layout to the generator: that buys back some of the vertical space the unrolling costs and drops the code that had to predict how `air` would wrap. Co-Authored-By: David Schoch <david@schochastics.net> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSnzx8x7uh1MjSPNHqjkWC

github-actions · 2026-08-06T09:54:11Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 5f23064 is merged into main:

🚀as_adjacency_matrix: 784ms -> 764ms [-4.32%, -0.97%]
❗🐌as_biadjacency_matrix: 757ms -> 770ms [+0.04%, +3.36%]
❗🐌as_data_frame_both: 1.56ms -> 1.66ms [+3.92%, +8.94%]
❗🐌as_long_data_frame: 3.99ms -> 4.1ms [+1.25%, +3.97%]
✔️es_attr_filter: 2.88ms -> 2.86ms [-5.08%, +3.8%]
✔️graph_from_adjacency_matrix: 121ms -> 122ms [-0.03%, +1.88%]
❗🐌graph_from_data_frame: 3.62ms -> 3.87ms [+4.03%, +9.78%]
✔️vs_attr_filter: 1.57ms -> 1.56ms [-3.01%, +1.52%]
✔️vs_by_name: 997µs -> 992µs [-2.32%, +1.5%]
Further explanation regarding interpretation and methodology can be found in the documentation.

krlmlr · 2026-08-06T10:02:28Z

Ready for review.

Revdeps running: https://github.com/igraph/rigraph/actions/runs/31091591505

github-actions · 2026-08-06T10:13:14Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 59a192e is merged into main:

🚀as_adjacency_matrix: 578ms -> 565ms [-3.57%, -1.18%]
✔️as_biadjacency_matrix: 567ms -> 569ms [-0.63%, +1.28%]
❗🐌as_data_frame_both: 1.25ms -> 1.32ms [+4.3%, +6.87%]
❗🐌as_long_data_frame: 3.27ms -> 3.39ms [+2.35%, +4.75%]
✔️es_attr_filter: 2.33ms -> 2.32ms [-1.42%, +1.04%]
✔️graph_from_adjacency_matrix: 103ms -> 103ms [-2.9%, +2.65%]
❗🐌graph_from_data_frame: 2.9ms -> 3.06ms [+1.63%, +9.5%]
✔️vs_attr_filter: 1.21ms -> 1.21ms [-1.03%, +0.36%]
❗🐌vs_by_name: 777µs -> 786µs [+0.06%, +2.03%]
Further explanation regarding interpretation and methodology can be found in the documentation.

schochastics

lgtm

github-actions · 2026-08-07T13:02:19Z

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 7b3402d is merged into main:

🚀as_adjacency_matrix: 835ms -> 798ms [-5.32%, -3.48%]
✔️as_biadjacency_matrix: 800ms -> 807ms [0%, +1.87%]
❗🐌as_data_frame_both: 1.66ms -> 1.72ms [+0.05%, +6.72%]
❗🐌as_long_data_frame: 4.07ms -> 4.34ms [+3.87%, +9.01%]
✔️es_attr_filter: 2.98ms -> 2.95ms [-5.59%, +3.59%]
✔️graph_from_adjacency_matrix: 133ms -> 133ms [-1.71%, +1.25%]
❗🐌graph_from_data_frame: 3.62ms -> 3.9ms [+5.68%, +9.97%]
🚀vs_attr_filter: 1.75ms -> 1.66ms [-10.21%, -0.08%]
✔️vs_by_name: 1.12ms -> 1.09ms [-6.7%, +0.85%]
Further explanation regarding interpretation and methodology can be found in the documentation.

Eight packages tracked in #2646 had sections in the report that the revdep2 runs dropped. Splitting them by package makes it visible which is which. Kept, restored from the last report that carried them: * `SEMgraph` and `scistreer` were never checked. Both halves stopped at `checking package dependencies` in three and two seconds -- `RBGL`, `Rgraphviz`, `graphite`, `AnnotationDbi` for one, `ggtree` for the other -- agreed, and were scored `ok`. * `Boptbd` and `ggm` were checked and are still broken, identically under both versions, which is `ok` for this workflow and not for a reader. * `lagdynamics` and `migraph` are not in the revdep set at all at `depth: 1`: both reach igraph through another package (`tna` and `manynet`), so they are level 2. Dropped, because the runs checked them and found them clean: * `NetSci`, 35 s, no errors either side. * `tna`, 215 s, no errors either side. Both were fixed by #2812. `* : <UNKNOWN>` in the `ggm` and `SEMgraph` sections becomes the CRAN mirror link the collector emits now. Their content is otherwise the historical text, unedited. The collector would have deleted four of the six again on the next run, so `keeps_committed()` gains the `ok`-but-broken case: a run that reproduces breakage on both sides has not shown that a committed section is stale. It declines to delete, and adds nothing -- `problems.md` is the newly-broken list by design. `newly_broken` is excluded deliberately: archeofrag went 1E to 2E in run 31930350338 and that section is this run's to rewrite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D1xpHRV7yVfgtJg4vp9P7z

A sweep of the pre-revdep2 report -- 32 problems and 7 failures, the curated record #2646 was written from -- found ten of its packages with no section left. Six were restored in the previous commit; three more here. * `c3net` was checked and is still broken, identically under both versions (1E, 21 s), which is `ok` for this workflow and not for a reader. Same shape as `Boptbd` and `ggm`. * `ggpicrust2` likewise, 1E on both sides in 85 s. * `MiscMetabar` was never checked: both halves stopped in four seconds and agreed. Dropped, and not restored: * `modelbpp` was checked clean in 73 s (fixed by #2812), as were `blavaan` (800 s) and `multinma` (1253 s). * `deepSTRAPP` and `invasible` are no longer reverse dependencies at all -- neither lists igraph in any dependency field on CRAN today. `invasible`'s section was empty besides its heading in any case. The revdep2-era reports name about 130 further packages that have no section now, but those come from run 31879790285 (78 problems, 76 of them the baseline-versus-`parse_check` parser artefact) and run 31893156685 (56 `donttest` timeout failures). Both are known-bad runs and their findings were not real, so those names are correctly absent. Twelve of them are nonetheless genuinely broken or unchecked; they were never in the curated record, so nothing is added for them here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D1xpHRV7yVfgtJg4vp9P7z

krlmlr requested a review from schochastics August 5, 2026 21:25

krlmlr force-pushed the claude/rigraph-pr-2808-review-3xlqcq-unroll branch from 718a640 to c186c6b Compare August 5, 2026 21:31

krlmlr force-pushed the claude/rigraph-pr-2808-review-3xlqcq-unroll branch from c186c6b to 4440b1c Compare August 6, 2026 08:39

krlmlr changed the title ~~refactor(migrations): inline and loop-unroll the argument recovery~~ fix(migrations): recover legacy arguments against the old signature Aug 6, 2026

krlmlr marked this pull request as ready for review August 6, 2026 08:40

krlmlr force-pushed the claude/rigraph-pr-2808-review-3xlqcq-unroll branch from 4440b1c to 632c4b5 Compare August 6, 2026 09:09

krlmlr mentioned this pull request Aug 6, 2026

fix(migrations): ignore empty argument slots in ... #2810

Closed

1 task

krlmlr force-pushed the claude/rigraph-pr-2808-review-3xlqcq-unroll branch from 632c4b5 to 5f23064 Compare August 6, 2026 09:40

Merge branch 'main' into claude/rigraph-pr-2808-review-3xlqcq-unroll

59a192e

schochastics approved these changes Aug 6, 2026

View reviewed changes

Merge branch 'main' into claude/rigraph-pr-2808-review-3xlqcq-unroll

7b3402d

krlmlr merged commit 0604e40 into main Aug 7, 2026
9 checks passed

krlmlr deleted the claude/rigraph-pr-2808-review-3xlqcq-unroll branch August 7, 2026 15:31

This was referenced Aug 16, 2026

fix: An empty argument slot with a name is not an ambiguous argument #2850

Open

Revdep Failures #2646

Open

Navigation Menu

Uh oh!

Uh oh!

fix(migrations): recover legacy arguments against the old signature - #2812

fix(migrations): recover legacy arguments against the old signature#2812
krlmlr merged 3 commits into
mainfrom
claude/rigraph-pr-2808-review-3xlqcq-unroll

krlmlr commented Aug 5, 2026 •

edited

Loading

Uh oh!

krlmlr commented Aug 5, 2026

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

krlmlr commented Aug 5, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

krlmlr commented Aug 6, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

krlmlr commented Aug 6, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

schochastics left a comment

Uh oh!

github-actions Bot commented Aug 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

krlmlr commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bug

The fix

Behaviour is unchanged, down to the message text

Tests

Cost

Verification

Uh oh!

krlmlr commented Aug 5, 2026

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

krlmlr commented Aug 5, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

krlmlr commented Aug 6, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

krlmlr commented Aug 6, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

schochastics left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

krlmlr commented Aug 5, 2026 •

edited

Loading