Allow module-local classes to be loaded by external modules #1007
Conversation
|
Just a quick comment for now. The search for an external loader looks really complicated. Instead of the multimap in |
|
That simplifies it; I've pushed a couple of commits to stash things in an attribute of the python type. The second commit is simpler, but increases the |
|
I had a go at simplifying this just a bit more: reduced the reliance on caster state (recursion flag, typeinfo state change) because these will need to go anyway with the planned new-style |
|
Looks great! |
dean0x7d
force-pushed
the
external-loading
branch
from
e1bc5d0 to
7b8b40a
Compare
|
I think we should include this in 2.2 -- I noticed you also put it in the upgrade doc, so I'm guessing you agree. |
|
Yes, definitely. It makes |
|
The current build is just for the comment change, right? (i.e. safe to not wait for the ci tests to finish?) |
|
Yeah. |
include/pybind11/cast.h
Outdated
|
|
||
| type_info *foreign_typeinfo = reinterpret_borrow<capsule>(getattr(pytype, local_key)); | ||
| if (foreign_typeinfo == typeinfo) | ||
| return false; // Only consider this foreign loader if actually foreign |
There was a problem hiding this comment.
I think there's a slight problem here: if the type has a masked global registration, typeinfo will have been changed to the global one, in which case the foreign_typeinfo == typeinfo isn't going to be true, so we could end recursing.
There was a problem hiding this comment.
Do you mean a local binding masking a global one? That should work and is covered by the tests. (Not the cpptype mismatch mentioned below.)
Edit: meant local masking global.
There was a problem hiding this comment.
I'm thinking:
- the local type masks the global type, is the correct type_info for the given argument, but the load fails anyway.
- the global type gets checked, which also fails (the global type is not right).
- foreign loaders get checked, but because
typeinfohas been changed to the global one, we don't short-circuit intry_load_foreignand so end up recursing.
It's not immediately obvious to me that the first step is impossible; though it's not immediately obvious to me how I can trigger it in a test, either.
There was a problem hiding this comment.
I think 225caf3 ought to solve this: when using a proper function (rather than a lambda) we can just test whether that function is foreign rather than relying on typeinfo.
|
There's another problem here that I'm looking into: We aren't checking that the foreign loader is actually a loader of the right type. |
| @@ -655,8 +665,6 @@ class type_caster_generic { | |||
| return inst.release(); | |||
| } | |||
|
|
|||
| protected: | |||
There was a problem hiding this comment.
Was this removal intentional?
There was a problem hiding this comment.
Yes. Direct access to value is needed in the module_local_load lambda. And seeing as detail::type_caster_generic is already an implementation detail, there isn't really much of a need to protect access.
Good catch. |
The main point of `py::module_local` is to make the C++ -> Python cast unique so that returning/casting a C++ instance is well-defined. Unfortunately it also makes loading unique, but this isn't particularly desirable: when an instance contains `Type` instance there's no reason it shouldn't be possible to pass that instance to a bound function taking a `Type` parameter, even if that function is in another module. This commit solves the issue by allowing foreign module (and global) type loaders have a chance to load the value if the local module loader fails. The implementation here does this by storing a module-local loading function in a capsule in the python type, which we can then call if the local (and possibly global, if the local type is masking a global type) version doesn't work.
jagerman
force-pushed
the
external-loading
branch
from
253c31a to
13b4264
Compare
The main point of
py::module_localis to make the C++ -> Python cast unique so that returning/casting a C++ instance is well-defined. Unfortunately it also makes loading unique, but this isn't particularly desirable or necessary: when an instance containsTypeinstance there's no reason it shouldn't be possible to pass that instance to a bound function taking aTypeparameter, even if that function is in another module.This commit solves the issue by storing the module-local
&type_caster_generic::loadin a global multimap so that we can call into local modules'load()methods formodule_localtypes. For module-local types that override a globally-registered types a global type conversion is also attempted.Fixes #919.
It increases
.sosize slightly (a little over 4KB, not counting the added tests), but as the additions are entirely in non-templated code, that number shouldn't change with the number of bound types.