Make S7 generics and methods traceable - #695
Merged
Merged
Conversation
This is pure claude, and appears to fix the problem, but if it's really about class returning a vector instead of a single string, it's likely we should upstream this to R itself. Fixes #584
Register generic and method properties on their S4 old classes and keep the initializer workaround for R versions before 4.7. Share native normalization across dispatch and method lookup while preserving the ordinary dispatch path. Support the data slot read by R tracing and import the S4 initializer for namespace loading. Add public API regressions and benchmarks for dispatch and trace setup.
Register the representation version slot and prototype for S7 generics and methods so tracing preserves the marker introduced on main. Cover the marker before and during tracing through the public API.
Register S7's S4 classes and tracing methods when methods loads, using a writable registry exposed through namespace class bindings. Preserve registration metadata and R's tracing caches across S7 unloads and reloads. Use base S3 group definitions directly so ordinary S7 use does not load methods. Cover both namespace load orders, reloads, and tracing restoration.
Import methods and register S7 S4 classes directly during package load. Remove the deferred hook, separate registry, active bindings, and custom cache cleanup, and restore group-generic discovery through methods. Keep the pre-R 4.7 tracing initializer and representation version slots. Update base-only loading and reload tests for the imported dependency.
Restore static declarations for the two base S4 classes and register each S7 function class when it is defined. Remove the separate registration hook and environment arguments left over from deferred methods loading. Consolidate the base-only loading, reload, and unload checks into one subprocess test while retaining the focused tracing and metadata tests.
Member
Author
|
Could you give us a brief (AI generated) summary of why all this additional machinery is necessary? |
Member
|
Member
Author
|
I can't approve my own PR but I think this is good to go. |
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.
Fixes #584.
Allow
trace()anduntrace()to work with S7 generics and methods, including tracing both during multiple dispatch. For example,trace("my_class", browser, where = my_generic@methods)sets a breakpoint in the method formy_class. Traced functions retain their S7 properties and representation metadata, so method lookup, property access, and printing continue to work.untrace()restores the original function.Internally, this declares the S7 generic and method properties as S4 slots, exposes the
.Dataslot needed by R's tracing initializer, and recognizes traced generics during native dispatch and method lookup. It movesmethodsfrom Suggests to Imports so registration can happen directly during package loading.R versions before 4.7 use a compatibility initializer that preserves S7 metadata. On R 4.7 and later, S7 uses R's initializer, incorporating the upstream fixes discussed in r-devel/r-svn#262. Those fixes complement the slot declarations and dispatch changes here.
Local measurements on macOS with R 4.6.1, comparing this PR with
main(245aaf46):methodsalready loaded (main loads 4.3× faster). This adds about 35 ms once per fresh session for the S4 registrations.The dispatch and tracing measurements can be reproduced with
bench/method-trace.Ragainst separately installed builds.