-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Preserve _parent
Environment in Async Functions for R6 Methods
#64
Open
dereckmezquita
wants to merge
1
commit into
r-lib:main
Choose a base branch
from
dereckmezquita:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains 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
Previously, async functions created via coro::async relied on a lexical binding of _parent to store internal state. When these functions are used as R6 methods, R6 replaces their closure environment, which loses the original _parent binding and causes errors such as: Error in api$getData() : object '_parent' not found This commit injects the captured _parent environment as a default formal parameter (.__coro_env_parent__) in the generator factory (in generator0()). The async function now retrieves the environment via: rlang::env(.__coro_env_parent__) rather than relying on a missing lexical binding. This change is minimal, maintains the original formatting, and ensures that async functions work correctly even when their environments are replaced (e.g., as R6 methods). Fixes the R6 compatibility issue with coro async functions.
I just need some help now on updating the tests and running them: r$> devtools::test()
ℹ Testing coro
✔ | F W S OK | Context
⠇ | 1 38 | async Note: no visible binding for global variable 'type' at generator.R:149
Note: no visible binding for global variable 'state_machine' at generator.R:152
Note: no visible binding for global variable 'state_machine' at generator.R:153
Note: no visible binding for global variable 'fmls' at generator.R:171
Note: no visible binding for global variable 'debugged' at generator.R:205
Note: no visible binding for global variable 'state_machine' at generator.R:269
Note: no visible binding for global variable 'type' at generator.R:279
✖ | 2 44 | async [1.4s]
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Error (test-async.R:235:3): async functions and async generator factories print nicely
Error in `eval(substitute(expr), data, enclos = parent.frame())`: object 'type' not found
Backtrace:
▆
1. └─testthat::expect_snapshot(print(fn, internals = TRUE, reproducible = TRUE)) at test-async.R:235:3
2. └─rlang::cnd_signal(state$error)
Failure (test-async.R:327:3): async functions do not cause CMD check notes (#40)
`invisible(compiler::cmpfun(async(function() NULL), options = list(suppressAll = FALSE)))` produced output.
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
✖ | 2 86 | generator
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Error (test-generator.R:28:3): generator factories print nicely
Error in `eval(substitute(expr), data, enclos = parent.frame())`: object 'type' not found
Backtrace:
▆
1. └─testthat::expect_snapshot(...) at test-generator.R:28:3
2. └─rlang::cnd_signal(state$error)
Failure (test-generator.R:321:3): generators do not cause CMD check notes (#40)
`invisible(compiler::cmpfun(generator(function() NULL), options = list(suppressAll = FALSE)))` produced output.
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
✔ | 3 | iterator-adapt
✔ | 10 | iterator-for
✔ | 3 | iterator [1.0s]
✔ | 15 | parser-block
✔ | 13 | parser-if
✔ | 29 | parser-loop
✔ | 54 | parser
✔ | 19 | step-reduce
✔ | 7 | step
══ Results ════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 5.8 s
── Failed tests ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Error (test-async.R:235:3): async functions and async generator factories print nicely
Error in `eval(substitute(expr), data, enclos = parent.frame())`: object 'type' not found
Backtrace:
▆
1. └─testthat::expect_snapshot(print(fn, internals = TRUE, reproducible = TRUE)) at test-async.R:235:3
2. └─rlang::cnd_signal(state$error)
Failure (test-async.R:327:3): async functions do not cause CMD check notes (#40)
`invisible(compiler::cmpfun(async(function() NULL), options = list(suppressAll = FALSE)))` produced output.
Error (test-generator.R:28:3): generator factories print nicely
Error in `eval(substitute(expr), data, enclos = parent.frame())`: object 'type' not found
Backtrace:
▆
1. └─testthat::expect_snapshot(...) at test-generator.R:28:3
2. └─rlang::cnd_signal(state$error)
Failure (test-generator.R:321:3): generators do not cause CMD check notes (#40)
`invisible(compiler::cmpfun(generator(function() NULL), options = list(suppressAll = FALSE)))` produced output.
[ FAIL 4 | WARN 0 | SKIP 0 | PASS 283 ]
r$>
r$> |
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.
close #63
Background
When an async function created with
coro::async
is used as an R6 method, the function fails with an error:This occurs because R6 replaces the function’s original closure environment (which contains the
_parent
binding used by the coroutine for internal state) with an R6-specific environment that only providesself
andprivate
. Consequently, the lookup for_parent
(viarlang::env("_parent")
) fails, leading to the error.What I Did
I have minimally modified the generator factory function (
generator0()
) in the coro package. The key changes are:Capture and Inject the Environment:
I capture the original environment in
_parent
as before. Then, I inject this environment into the function’s formals as a default argument (named.__coro_env_parent__
). This ensures that even if R6 replaces the closure environment, the necessary environment object is preserved.Reference the Injected Environment:
In the function body, where the original code previously did:
it now uses:
This change guarantees that the coroutine’s internal state is available regardless of any environment substitution by R6.
Minimal Reproducable Example
Below is a minimal script that demonstrates the issue and the subsequent fix:
With updates I get the expected outputs:
This change is backwards compatible and should resolve the compatibility issue between
coro::async
and R6’s environment re-binding.Please review and let me know if any further adjustments are needed.