fix(state_null): resolve external hypothesis variables via enquo() env, not caller_env() - #22
Merged
joshuamarie merged 2 commits intoAug 7, 2026
Conversation
…v, not caller_env() Reviewing the beyond-null vignette (requested by Joshua) surfaced that Showcase 3 (a hypothesis referencing an external variable, `<= 0.20 * overall_avg`) fails to render: `rlang::caller_env()` inside the S7 method for state_null() depends on the exact call-stack shape and does not reliably land on the user's calling scope once state_null() is invoked from inside a function, local(), or a knitr chunk - which is exactly how every vignette is built. It only "worked" by accident when called at the top level of an interactive script, which is presumably how the numbers currently in the vignette were produced. Fix: reuse the environment `enquo()` already captures correctly for the expression, instead of re-deriving it via a stack-based lookup. Full test suite passes unchanged after the fix. Also aligned the vignette's \VignetteIndexEntry with its YAML title (was raising a title-mismatch warning on render).
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes state_null() failing to resolve externally defined variables referenced in hypothesis expressions (notably when invoked through S7 dispatch from within functions/knitr), which was breaking the beyond-null vignette render.
Changes:
- Switch
state_null()to use the environment captured byrlang::enquo()(rlang::quo_get_env(raw)) instead ofrlang::caller_env(). - Align
beyond-nullvignette\VignetteIndexEntrywith its YAML title to remove render warnings. - Document the bug fix in
NEWS.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
R/hypothesis-core.R |
Fixes hypothesis expression environment resolution for state_null() by using the captured quosure env. |
vignettes/usage/beyond-null.Rmd |
Updates vignette index entry to match the title, avoiding title-mismatch warnings. |
NEWS.md |
Adds a Bug Fixes entry describing the state_null() environment resolution fix and its impact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
26
to
30
| S7::method(state_null, test_lazy) = function(.x, expr, ...) { | ||
| raw = rlang::enquo(expr) | ||
| expr_val = rlang::quo_get_expr(raw) | ||
| env = rlang::caller_env() | ||
| env = rlang::quo_get_env(raw) | ||
| claim = parse_null_claim(rlang::new_quosure(expr_val, env)) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Clarified the purpose of the vignette regarding weighted, non-zero-threshold claims and improved explanations of `state_null()` and `x_by()` functionality.
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.
Summary
@joshuamarie asked the team to review the
beyond-nullvignette (https://s7-stats.github.io/statim/articles/usage/beyond-null.html). The prose/methods checked out against the actual code and re-computed numbers, but rendering the vignette for real surfaced that Showcase 3 doesn't build at all:state_null()fails to resolve an external variable (overall_avg) referenced in the hypothesis expression.Root cause:
R/hypothesis-core.Rre-derives the hypothesis expression's environment viarlang::caller_env()instead of using the environmentrlang::enquo()already captured correctly.caller_env()counts call-stack frames, and the S7 dispatch machinery forstate_null()inserts a variable number of frames depending on context — it happens to resolve correctly when called at the top level of an interactive script, but lands on an internal S7 dispatch environment (confirmed vials(envir = env)) when called from inside a function,local(), or a knitr chunk. Since every vignette renders through knitr, this broke the documented "data-driven threshold" use case in practice, not just as an edge case.Filed as #21 with full repro/root-cause details before opening this PR.
caller_env()forquo_get_env(raw)instate_null()(R/hypothesis-core.R).\VignetteIndexEntrywith its YAML title (was raising a title-mismatch warning on every render).NEWS.mdentry under Bug Fixes.No vignette prose/numbers needed changing — everything cross-checked against re-computed output was already accurate.
Test plan
devtools::test()— full suite passes unchanged with the fixrmarkdown::render("vignettes/usage/beyond-null.Rmd")— now completes with no errors and no title-mismatch warninglocal(), not just in the vignetteCloses #21.