fix: support NULL as the canonical root namespace for session$destroy(), etc#4395
Draft
cpsievert wants to merge 14 commits into
Draft
fix: support NULL as the canonical root namespace for session$destroy(), etc#4395cpsievert wants to merge 14 commits into
NULL as the canonical root namespace for session$destroy(), etc#4395cpsievert wants to merge 14 commits into
Conversation
Pairing-session work with colleague: make character(0) the canonical user-facing value for the root scope (replacing ""), rename the destroy `id` argument to `namespace`, fold id validation into invokeDestroyCallbacks via an `allowRoot` flag, and drop the standalone validateDestroyId helper. Committed verbatim as a record of the pairing session before applying fixes.
- Real ShinySession scope `destroy()` recursed infinitely: the renamed `namespace` parameter shadowed the scope's own captured namespace, and the self-destroy branch was replaced by a recursive makeScope() call that never reached invokeDestroyCallbacks(). Restore the self-vs-child branch and capture the scope's namespace as `selfNamespace`. - MockShinySession was left on the old nzchar()/"" logic, so it still crashed with "argument is of length zero" on callModule(server, character(0)) (the teal failure path via testServer). Mirror the character(0) handling so mock and real sessions behave identically. - Validate the user-supplied destroy `namespace` at the public boundary (before makeScope) via validateDestroyNamespace(), instead of inside invokeDestroyCallbacks() which only runs after makeScope() has already choked on bad input. - Restore the reserved "..root" namespace guard in real makeScope(), and map "" to root in destroyNsKey()/getOrCreateDestroyCallbacks() (fastmap can't key on an empty string). Full test suite passes (0 failures).
Replace the private ShinySession$destroyNsRoot field and the hardcoded "..root" literals in MockShinySession with a single package-level `destroyNsRoot` constant, documenting that it must be a non-zero-length string because it is used as a fastmap key (fastmap disallows ""/NA keys).
cpsievert
commented
Jun 1, 2026
cpsievert
commented
Jun 1, 2026
cpsievert
changed the title
character(0) as the canonical root namespace for session$destroy()
- validateDestroyNamespace(): quote/coerce the offending value in the error (clear output for "", numeric, NA, and multi-element inputs). - MockShinySession$onDestroy(): use getOrCreateDestroyCallbacks() instead of hand-managing the callback map, keeping root-key handling consistent. - Align the mock's root-teardown error messages with ShinySession (direct callers to close(), add call. = FALSE). - Restore the original two-line formatting of the exportTestValues guard.
Drop the `|| !nzchar(ns)` folding I had added to destroyNsKey() and the mock's getOrCreateDestroyCallbacks(). It made registration treat "" as root while invokeDestroyCallbacks() (length-0) did not, so an ""-namespaced scope would register onDestroy callbacks under the root key that teardown could never find. character(0) is now the sole root spelling everywhere.
Addresses Copilot feedback: with character(0) as the sole root spelling,
an empty-string `id` (e.g. callModule(server, "")) would otherwise fall
through to an empty-string fastmap key and error cryptically. Reject ""
early with a clear message in both the real and mock makeScope(); root
(character(0)) is still accepted. NS("") also yields a stray "-" prefix,
so "" was never a valid namespace.
Widen the makeScope() guard (real + mock) to reject NA as well as "", since both are unusable as fastmap keys. Document the contract in the destroy() docstrings: a module namespace must be a non-empty, non-NA string, and the root scope is identified by character(0).
Default the public destroy()/onDestroy() entry points and the internal invokeDestroyCallbacks() to NULL instead of character(0), and lead with NULL in the docstrings and guard messages. NULL is the idiomatic R 'absence' value and was the original #4372 public default. character(0) remains accepted (both are length 0, which is how root is detected everywhere), so callers like teal that pass character(0) are unaffected; '' and NA are still rejected.
cpsievert
changed the title
character(0) as the canonical root namespace for session$destroy()NULL/character(0) as the canonical root namespace for session$destroy()
The reserved-sentinel and empty/NA guards were duplicated byte-for-byte in ShinySession$makeScope() and MockShinySession$makeScope(). Pull them into a package-level validateScopeNamespace(), mirroring the existing validateDestroyNamespace() helper. Behavior-preserving.
validateScopeNamespace and validateDestroyNamespace differed only in whether the root (length-0) namespace is allowed -- the apparent leniency of the scope validator was illusory (numeric/multi-element namespaces crashed cryptically downstream rather than being permitted). Collapse them into a single validateNamespace(namespace, allow_root) helper: makeScope() passes allow_root = TRUE, the destroy paths use the default. As a side benefit, malformed makeScope() namespaces now get the clean validation error instead of a cryptic fastmap/if-length crash.
R6 method docstrings are string literals (unlike #' roxygen comments), so the em-dashes I used tripped 'checking code files for non-ASCII characters', which CI runs with error_on = 'warning'. Replace them with '--'.
cpsievert
changed the title
NULL/character(0) as the canonical root namespace for session$destroy()NULL as the canonical root namespace for session$destroy(), etc
With character(0)/NULL now a valid root namespace, the bookmark/restore helpers' scalar-string assumptions broke for root scopes: filterNamespace() treated the prefix as "-" (dropping all root-level names), and the save/restore dir handling did file.path(dir, character(0)) -> dir.exists(character(0)) -> "argument is of length zero". Treat a length-0 namespace as the root: all names in scope, identity unNamespace(), and share the top-level bookmark dir (no subdir). Pre-existing in v1.13.0; re-exposed by re-enabling root scopes. Verified the root vs named behavior in isolation; full test suite passes.
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 refines the unreleased
session$destroy()work (#4372) so that an empty module id —callModule(server, character(0)), the documentedNS()length-0 idiom that packages like teal use to run a module server in the parent namespace — is treated as the root scope throughout the destroy machinery.The first commit records a pairing-session refactor (making
character(0)the canonical root value instead of"", renaming the destroyidargument tonamespace). The second commit fixes several issues found while reviewing it:Real
ShinySession$destroy()no longer recurses infinitely. The renamednamespaceparameter shadowed the scope’s own captured namespace and the self-destroy branch had been replaced by a recursivemakeScope()call that never reachedinvokeDestroyCallbacks(). The self-vs-child branch is restored, with the scope’s namespace captured asselfNamespace.MockShinySessionnow matches the real session. It had been left on the oldnzchar()/""logic, so it still errored with "argument is of length zero" oncallModule(server, character(0))(the teal failure path viatestServer()). Thecharacter(0)handling is mirrored so mock and real behave identically.Destroy
namespaceis validated at the public boundary, beforemakeScope()runs, via a smallvalidateDestroyNamespace()helper.Restored safeguards: the reserved
"..root"namespace guard inmakeScope(), and mapping""to root in the destroy-key logic (fastmap can’t key on an empty string).Tests
Full suite passes (0 failures). New coverage: empty-namespace
makeScope()/callModule()acceptance, un-prefixed ids (scope$ns("x") == "x"), and non-recursive teardown.Caveat for reviewers: the recursion fix is verified through
MockShinySession(now line-for-line equivalent to the real session) plus static reasoning — the test harness can’t instantiate a liveShinySession.Caught by reverse-dependency checks against teal. Supersedes the minimal-guard approach in #4394 (one of the two should be chosen).