pybind11
pybind11 copied to clipboard
[BUG]: stl_bind generates invalid type signature for Sphinx/Pylance
Required prerequisites
- [X] Make sure you've read the documentation. Your issue may be addressed there.
- [X] Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
- [X] Consider asking first in the Gitter chat room or in a Discussion.
Problem description
STL_bind creates subclasses that try to mimic keyview. However, our subclasses have no explicit relation with typing.collections.KeyView or collection.abc.KeyView etc... https://github.com/pybind/pybind11/pull/3985 attempts to fix it by removing the [], but this seems like a cludge and we really should let the typing system represent these as the correct type. I am wondering if there is a way we can still have Sphinx / Pylance recognize these KeyView, ItemViews etc as what they are instead of just an opaque class. Maybe just having them inherit from / mixin collections.abc.KeyViews would be appropiate?
Would appreciate thoughts on this front @rwgk @henryiii?
Reproducible example code
No response
@bmerry Since you created the original PR for this feature, any thoughts on if there is a clever way to solve this issue?
@bmerry Since you created the original PR for this feature, any thoughts on if there is a clever way to solve this issue?
No, no clever ideas. I don't think I've touched pybind11 since I wrote that code, and I think the square bracket convention predated the changes I made.
The current naming with e.g. KeysView[BindedMap] also prevents formatting the stubs with black, because it stumbles on the IMO incorrect Python syntax. Is there a reason against a name with valid syntax such as KeysViewBindedMap?
I guess the reason to inherit the class from e.g. collections.abc.KeysView would be to allow checking its capability with something like this:
isinstance(keys_view_binded_map, collections.abc.KeysView)
If so, could this be done with creating a Python class like https://github.com/pybind/pybind11/issues/1193#issuecomment-429451094 or specifically indicating the base class like https://github.com/pybind/pybind11/issues/1193#issuecomment-660192531 ?
I actually figured that there is a typing.KeysView etc.. that we can use. However, we need to specify the keytype, value_type.
Whenever I insert the following line into stl_bind.h, it gives a linker error though for Clang:
constexpr const char *keyname = detail::make_caster<KeyType>::name.text;
error:
ld: illegal text reloc in '__ZN8pybind118bind_mapINSt3__113unordered_mapINS1_12basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE9LocalBaseILi0EENS1_4hashIS8_EENS1_8equal_toIS8_EENS6_INS1_4pairIKS8_SA_EEEEEENS1_10unique_ptrISJ_NS1_14default_deleteISJ_EEEEJEEENS_6class_IT_JT0_EEENS_6handleERSG_DpOT1_' to '__ZN8pybind116detail13string_casterINSt3__112basic_stringIcNS2_11char_traitsIcEENS2_9allocatorIcEEEELb0EE4nameE' for architecture x86_64
Thoughts @rwgk @henryiii on how to fix this? It seems like the culprit its converting the descr compiletime string into a runtime one.
I found a workaround to the linker error, but if I fix the typing system, we get an error because we define the same classname multiple times:
diff:
diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h
index 22a29b47..bc1e0312 100644
--- a/include/pybind11/stl_bind.h
+++ b/include/pybind11/stl_bind.h
@@ -651,6 +651,15 @@ struct items_view {
Map ↦
};
+/*
+// Helper function needed to prevent linker error
+template <typename Value>
+const char* get_caster_name(){
+ static constexpr auto value_name = detail::make_caster<Value>::name;
+ return value_name.text;
+}
+*/
+
PYBIND11_NAMESPACE_END(detail)
template <typename Map, typename holder_type = std::unique_ptr<Map>, typename... Args>
@@ -662,6 +671,12 @@ class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args &&
using ItemsView = detail::items_view<Map>;
using Class_ = class_<Map, holder_type>;
+ // Need to do an explicit copy from static to local to prevent linker errors
+ static constexpr auto key_conv_name = detail::make_caster<KeyType>::name;
+ static constexpr auto value_conv_name = detail::make_caster<MappedType>::name;
+ const char *key_name = key_conv_name.text;
+ const char *value_name = value_conv_name.text;
+
// If either type is a non-module-local bound type then make the map binding non-local as well;
// otherwise (e.g. both types are either module-local or converting) the map will be
// module-local.
@@ -674,11 +689,11 @@ class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args &&
Class_ cl(scope, name.c_str(), pybind11::module_local(local), std::forward<Args>(args)...);
class_<KeysView> keys_view(
- scope, ("KeysView[" + name + "]").c_str(), pybind11::module_local(local));
+ scope, (std::string("KeysView[") + key_name + ']').c_str(), pybind11::module_local(local));
class_<ValuesView> values_view(
- scope, ("ValuesView[" + name + "]").c_str(), pybind11::module_local(local));
+ scope, (std::string("ValuesView[") + value_name + ']').c_str(), pybind11::module_local(local));
class_<ItemsView> items_view(
- scope, ("ItemsView[" + name + "]").c_str(), pybind11::module_local(local));
+ scope, (std::string("ItemsView[") + key_name + ',' + value_name + ']').c_str(), pybind11::module_local(local));
cl.def(init<>());
Error:
import pybind11_tests
E ImportError: generic_type: cannot initialize type "KeysView[str]": an object with that name is already defined
Is the name used for anything other than rendering documentation? If not, then I don't see why we can't allow duplicates. If so, then add the ability to specify both an 'actual' internal name and a doc-only name used for rendering docs.