Implement external classes#660
Conversation
# Conflicts: # NEWS.md # R/class-spec.R # R/external-generic.R # _pkgdown.yml # tests/testthat/_snaps/property.md
hadley
requested review from
lawremi and
t-kalinowski
and removed request for
t-kalinowski
|
@t-kalinowski @lawremi I think this is now ready for your feedback. |
lawremi
left a comment
lawremi
left a comment
There was a problem hiding this comment.
This is a very useful feature. Implementation looks good.
Add regression tests for method registration outside packages, external classes nested in union signatures, and unresolved external class introspection.
Cover resolving external classes whose S7 name differs from their namespace binding, preserving generic sentinels for deferred foreign methods, and rejecting unprovable property overrides involving unresolved external classes.
Resolve external classes by S7 class name, preserve generic sentinels when deferring methods for foreign generics with external-class signatures, and avoid treating unresolved external classes as an empty inheritance hierarchy.
There was a problem hiding this comment.
I pushed tests and fixes for some edge cases. Can you please take a look and see if the approach makes sense?
An open question is what behavior we want for property override checks when an external class is from a package that is not loaded yet. The current fix avoids treating an unresolved external class as matching every child type, but it's possible we want to take a different approach here (e.g., defer until the package is loaded, or load the package eagerly)
| } | ||
| } | ||
|
|
||
| # Also consider cases where the constructor isn't named the same as the class |
There was a problem hiding this comment.
Are we sure that this is a case that we want to support? Why do we want to encourage people to have a mismatch between constructor and class name?
| #' Make sure to call [S7_on_load()] in your package's `.onLoad()` so that | ||
| #' deferred method registrations fire when the relevant package is loaded. | ||
| #' | ||
| #' External classes can not currently be used as parents in [new_class()]. |
t-kalinowski
left a comment
t-kalinowski
left a comment
There was a problem hiding this comment.
I opened a PR with some proposed changes to this branch: #710
Most of the changes address various edge cases raised by automated review.
One change I want to call out is that I'm proposing that external S7 classes must be exported by a package using the same symbol name as the class's @name. This makes resolution much simpler and faster. It also makes the code easier to reason about, both internally in S7 and in user code, and avoids introducing a bunch of complexity. In particular, we don't need to go on a fishing expedition looking for an exact match, which could potentially force a bunch of lazy bindings.
I also tightened up a few edge cases around this:
- external class resolution now has separate errors for missing packages, too-old package versions, and binding mismatches
- external classes work in union arms, including self-referential and mutually recursive class definitions
- child properties can narrow an external parent class to a loaded subclass
- subclassing against an unloaded external class now fails at the point where we need to resolve it
- method introspection and
method_explain()now use the same required-resolution behavior - unload cleanup handles the case where the external class package has already unloaded before the downstream package unloads
I also changed one of the tests from using local_mocked_bindings() to local_package(), to avoid needing to define getNamespaceVersion in the package namespace just to mock it during tests -- that seemed to me like test scaffolding leaking into production code.
* Address external class review findings
* Fix deferred external class method unregistration
* Resolve external class signatures on unload
* Fix external class validation and cleanup
* Fix external-class method hook cleanup
* Fix external class dispatch handling
* Fix deferred external class registration
* Resolve external signatures on method unregister
* Check external class versions before string match
* Remove stale S7 hooks after failed loads
* Reject package-less external class matches
* Normalize external generic sentinels
* Register runtime external-class methods
* Fix deferred external class method registration
* Fix external generic hook replacement
* Short-circuit union class checks
* Address external method registration reviews
* Fix external class union handling
* Remove stale deferred external methods
* Fix external method unregister review cases
* Fix deferred union registration
* Preserve versioned external generic on unregister
* Allow external classes to narrow S7_object
* Simplify external class matching
* Simplify external class cleanup
* Simplify external class registration
* Trim external class changes
* Trim external class cleanup
* Simplify external class resolution
* Simplify external dependency checks
* Use closure for external class validation
* Remove external S7_object special case
* Document generic sentinel invariant
* Trim-external-class-tests
* Clarify-external-package-vignette
* Clarify external class resolution errors
* Restore external class docs example
* Restore external dependency flattening helper
* Simplify external class error construction
* Snapshot external class binding mismatches
* Restore external class default snapshot
* Reduce external class snapshot churn
* Fix external class review findings
[P2] Allow subclasses of external parent classes — /Users/tomasz/github/RConsortium/S7/R/class-spec.R:372-375: Valid narrowing is rejected when a child class is a real S7 subclass of an external parent class. The branch returns FALSE unless the child has the exact same package/name as the external placeholder, so cases like dep::Sub extending dep::Base cannot override a parent property typed as new_external_class("dep", "Base") even though the inheritance chain would make it valid.
[P2] Remove methods when an external class unloads first — /Users/tomasz/github/RConsortium/S7/R/hooks.R:89-90: If the external class package unloads before the downstream package, external_deps_resolvable(deps) is false and unload uses the placeholder signature. Since the method was registered with resolved class objects, unregister_own_S7_method() builds a non-identical method and leaves the downstream method installed in the upstream generic after S7_on_unload().
* Rename `S7_class` attribute to `_S7_class` (#689)
This keeps the name as similar as possible, but makes it clear that it's in the namespace S7 reserves for its own usage. Includes backward compatibility fallback that we can remove in a future version.
Closes #677
* Clarify external class resolution errors
* avoid binding `getNamespaceVersion` in package namespace
* Refactor test helpers to use code blocks (#714)
---------
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
3 participants
I think the use of external is a little bit misleading here, because these are really "delayed" classes. But external matches external generic, which exist for a similar reason.
Fixes #250. Fixes #573.
Note this does not solve #317, since currently creating a subclass will force loading of the package via
class_constructor(). But I think can fix that after #694 by using...to call constructors from other packages. That will decouple the superclass arguments from the subclass in a way that won't causeR CMD checkto fail with a codoc mismatch if the parent args change.