[BUG]: stl_bind generates invalid type signature for Sphinx/Pylance #3986

Activity

1. Skylion007

   added

   bug

   signaturesIssue about static signaturesIssue about static signatures

   on Jun 2, 2022
2. Skylion007 commented on Jun 2, 2022

   

   Skylion007

   on Jun 2, 2022

   CollaboratorAuthor

   More actions

   @bmerry Since you created the original PR for this feature, any thoughts on if there is a clever way to solve this issue?
3. bmerry commented on Jun 2, 2022

   

   bmerry

   on Jun 2, 2022

   Contributor

   More actions

   @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.
4. odel4y commented on Oct 21, 2022

   

   odel4y

   on Oct 21, 2022

   More actions

   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 #1193 (comment) or specifically indicating the base class like #1193 (comment) ?
5. Skylion007 commented on Oct 23, 2022

   

   Skylion007

   on Oct 23, 2022

   CollaboratorAuthor

   More actions

   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.
6. Skylion007 commented on Oct 23, 2022

   

   Skylion007

   on Oct 23, 2022

   CollaboratorAuthor

   More actions

   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
   
7. virtuald commented on Oct 25, 2022

   

   virtuald

   on Oct 25, 2022

   Contributor

   More actions

   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.
8. rwgk

   closed this as completedin #4353on Dec 9, 2022