fix: call a function passed as an argument from the caller's environment - #2845
Open
krlmlr wants to merge 5 commits into
Open
fix: call a function passed as an argument from the caller's environment#2845krlmlr wants to merge 5 commits into
krlmlr wants to merge 5 commits into
Conversation
igraph functions that take another function as an argument call it themselves. lifecycle attributes a deprecation to the caller of the deprecated function, and that caller is an igraph frame: `deprecate_soft()` says nothing at all, and `deprecate_warn()` blames igraph and asks the user to report a bug against it. Either way `plot(g, layout = layout.circle)` never tells the user that the layout function they chose is on its way out. `bfs()`, `dfs()`, `arpack()` and `cluster_leading_eigen()` already evaluate their callback in the environment they were called from, through their `rho` and `env` arguments, and are attributed correctly because of it. `call_user_callback()` and `as_user_callback()` extend that to the remaining function arguments, without an argument to pass in and thread through: the function is called from a closure enclosed in the environment igraph was called from, which is the one lifecycle asks about. Nothing is inspected and nothing is replayed, so this covers any deprecation the function may signal, including one that depends on how it was called. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJx1uH2GvN5DcqdVzDLvwW
`layout.spring()`, `layout.svd()` and `layout.fruchterman.reingold.grid()` are deprecated with a warning again, and lay out with `layout_with_fr()` as before. They are layout callbacks, and their hard deprecation -- made after 2.3.3 and never released -- would break `plot(layout = )` for users who have never seen the warning, because igraph, not they, called the function. Their documentation already describes this behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJx1uH2GvN5DcqdVzDLvwW
Contributor
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 7e6a3be is merged into main:
|
The hard deprecation of `layout.spring()`, `layout.svd()` and `layout.fruchterman.reingold.grid()` stands: a defunct layout function passed to `plot()` now fails against the call that chose it, which is what un-deprecating them was for. This reverts commit 7e6a3be and the documentation the roxygen run derived from it. The tests that assert on message output are snapshot tests now. One of them records what a deprecation looked like before and after in the same place, which says more than a regular expression can. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJx1uH2GvN5DcqdVzDLvwW
krlmlr
marked this pull request as ready for review
August 16, 2026 14:13
Contributor
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 3bb7d95 is merged into main:
|
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR was prepared with Claude Code (Claude Opus).
Supersedes #2844, which closed the same gap by inspecting the function object instead.
The problem
igraph functions that take another function as an argument call it themselves. lifecycle attributes a deprecation to the caller of the deprecated function, and that caller is an igraph frame, not the user. So
plot(g, layout = layout.circle)was silent, whilelayout.circle(g)warned:deprecate_soft()stays quiet entirely, because it does not report a package's own use of a deprecated function;deprecate_warn()does warn, but appends "The deprecated feature was likely used in the igraph package. Please report the issue at …", pointing the user at our tracker for something they wrote themselves.Either way the user is not told that the function they passed is on its way out, until the deprecation becomes hard and the call fails outright.
setup-lifecycle.Ralready flags this class of deprecation in our own test suite (#2782); this closes the same gap for users.The fix
Nothing is inspected and nothing is replayed. The call moves, so that lifecycle's own attribution lands where it belongs.
igraph already does this in four places:
bfs(),dfs(),arpack()andcluster_leading_eigen()evaluate their callback in the environment they were called from, through theirrhoandenvarguments, and are attributed correctly because of it.call_user_callback()andas_user_callback()generalize that to the remaining function arguments, without an argument to pass in and thread through: the function runs from a closure enclosed in the environment igraph was called from, which is the one lifecycle consults viatopenv().Because there is no inspection, this covers any deprecation the function may signal, including one that depends on how it was called, one whose message is assembled at run time, and one in a function from another package entirely.
Applied to plotting parameters (which covers
plot(),tkplot(),rglplot(), thelayoutgraph attribute and function-valuedigraph_options()),layout_nicely(),layout_components(),tk_reshape(), vertex shapes, attribute combinations,local_scan(), printer callbacks, and the search callbacks that do not already take an environment.What this changes for a caller
add_shape()the warning arrives at the firstplot()that uses the shape, not at registration.parent.frame()inside a callback is now the environment igraph was called from, for every callback rather than only deprecated ones. That is the mechanism; it is a visible change for a callback that inspects its caller.Testing
New
tests/testthat/test-utils-user-callbacks.R, with the assertions on message output as snapshots.Getting these to mean anything took some care: tests run in an environment that belongs to igraph, and lifecycle exempts the test suite of the deprecating package from its "is this the user's doing?" question, so a naive test passes with or without the change. They call through an
as_user()helper — an environment that belongs to no package, withTESTTHAT_PKGcleared. The first snapshot records both halves of the change in one place:Full suite passes; the two failures seen locally are environmental (a blocked download in
test-foreign.R, and acallrsubprocess intest-other.Rthat needs an installed igraph).Related
Signalling a deprecation from inside a callback that igraph invokes from C used to crash, because lifecycle reaches
vcount()through the backtrace it assembles and the nested call unwound the running algorithm's structures. #2851 fixes that; it is independent of this PR, but the two meet whenever a deprecated function is used as a search callback.🤖 Generated with Claude Code
https://claude.ai/code/session_01WJx1uH2GvN5DcqdVzDLvwW