Correct class names for KeysView, ValuesView and ItemsView in bind_map #4353

aimir · 2022-11-23T00:39:01Z

Description

Wrap KeysView, ValuesView and ItemsView with correct typing information in mappings wrapped via bind_map.

For example, for a mapping from strings to doubles, its .keys() function should have return type KeysView[str].

Currently, things have been wrapped like KeysView[MappingWrapperName], which is both misleading and inconsistent with the parallel python classes (especially for ItemsView, which usually has two type arguments, but here has only one).

This also means that we get differently-typed KeysView objects for different mappings with the same key type, even though they are functionally identical types.

This is solved by making KeysView etc. an abstract class that's a template on the relevant types only, rather than the entire map, so that maps with similar keys will have KeysView with the same type. The actual implementation is provided later, when wrapping the .keys() function of the mapping class, and similarly for values and items.

Closes #3986.

Suggested changelog entry:

* Fixed typing of ``KeysView``, ``ValuesView`` and ``ItemsView`` in ``bind_map``

rwgk · 2022-11-23T17:47:25Z

I glanced through: this looks great at first sight but isn't easy to review for me (I don't know much about this code).

@bmerry is there a chance that you could help reviewing this code?

I ran this with ASAN, MSAN, UBSAN using the Google toolchain: everything passes.

I spot-checked the CI failures. It looks like a C++11 and C++14 incompatibility, a static variable at class scope that only works with C++17. I think it's this:

pybind11::detail::type_caster<double, void>::name

With C++11 and C++14 this can only be used as a constexpr, but e.g. printing the name doesn't work. I never understood this fully. I didn't look enough to see what triggers the error, I hope this helps you at least a little bit.

Please tag me here if you need a button click to trigger the CI.

aimir · 2022-11-23T18:41:22Z

Thanks! I see I've still got quite a lot of issues, so I moved this to to be a draft for a while.

My last commits should hopefully resolve most of the issues, except for:

incorrect type strings for view of python-wrapped c++ classes - this should hopefully be easy to fix by doing something similar to pybind11.h here:

pybind11/include/pybind11/pybind11.h

Line 432 in 9c18a74

const std::type_info *t = types[type_index++];
The macos-latest failures are a bit weirder, I don't really understand what the issue is - any insights?

rwgk · 2022-11-25T23:20:08Z

The macos-latest failures are a bit weirder, I don't really understand what the issue is - any insights?

It's gone :-)

The only one CI failure is definitely unrelated. (PGI key signing issue)

include/pybind11/stl_bind.h

tests/test_stl_binders.py

aimir · 2022-12-02T05:38:57Z

I'm not sure what's with that failing check on pypy-3.7 windows-2022 x64...

But assuming this is just a random failiure, I think this is ready to be merged, isn't it?

rwgk · 2022-12-02T15:25:55Z

But assuming this is just a random failiure

Yes, that's just one of the annoying flakes we have.

Do you know someone with a good background to review this PR?

rwgk

After you make this change, and pending completion of updating the smart_holder branch (hopefully very soon), I will run this through Google-internal global testing, which includes testing with a large number of 3rd-party (OSS) projects.

include/pybind11/stl_bind.h

aimir · 2022-12-03T15:17:06Z

@rwgk Did you happen to run the Google-internal tests?

I can't really think of anything else to do, and if those pass it would probably be a pretty strong indication that things work as expected.

rwgk · 2022-12-03T15:57:02Z

@rwgk Did you happen to run the Google-internal tests?

I tried last night but it was too early after the smart_holder update (there is a few hours lag for the batched global testing that I was trying to use).

I tried again just now and it accepted the job. It will probably take 6+ hours to finish.

I did run the pybind11 unit tests with all sanitizers interactively and that passed.

Skylion007 · 2022-12-03T22:57:54Z

Do we have any testing for PyBInd11 without RTTI. I know that was supported before this PR, but I am wondering if the virtual methods will break it.

aimir · 2022-12-03T23:55:24Z

I don't really see how any of the current code would work without RTTI. There's already typeid everywhere, applied to both types and variable names. Examples:

pybind11/include/pybind11/detail/type_caster_base.h

Line 894 in e133c33

type = src ? &typeid(*src) : nullptr;

pybind11/include/pybind11/detail/typeid.h

Line 62 in e133c33

return detail::clean_type_id(typeid(T).name());

Perhaps I misunderstand? I just tried to compile with -fno-rtti and saw many failures unrelated to this PR.

Edit: see also #526, I think the discussion there implies that pybind11 without RTTI is not supported ever since d7efa4f

rwgk · 2022-12-04T10:00:55Z

Do we have any testing for PyBInd11 without RTTI. I know that was supported before this PR,

@Skylion007

What made you think that any part of pybind11 works without RTTI?
What would be the motivation for avoiding RTTI?

I systematically looked at all files that mention "rtti" (case-insensitive):

include/pybind11/attr.h
include/pybind11/detail/type_caster_base.h
docs/changelog.rst
docs/advanced/classes.rst

I don't see any mention of pybind11 working without RTTI.

Then I did a small quick experiment, creating this file:

$ cat include_stl_bind_h.cpp
#include "pybind11/stl_bind.h"

Then:

$ clang++ -c -std=c++17 -Iinclude -I/usr/include/python3.10 include_stl_bind_h.cpp

Succeeds without error.

clang++ -c -std=c++17 -fno-rtti -Iinclude -I/usr/include/python3.10 include_stl_bind_h.cpp

See output below.

Thinking about it, it occurred to me maybe pytypes.h could work without RTTI, so I tried that, too:

$ clang++ -c -std=c++17 -Iinclude -I/usr/include/python3.10 include_pytypes_h.cpp

Succeeds without error.

$ clang++ -c -std=c++17 -fno-rtti -Iinclude -I/usr/include/python3.10 include_pytypes_h.cpp
In file included from include_pytypes_h.cpp:1:
include/pybind11/pytypes.h:741:44: error: use of typeid requires -frtti
    return detail::isinstance_generic(obj, typeid(T));
                                           ^
1 error generated.

In file included from include_stl_bind_h.cpp:1:
In file included from include/pybind11/stl_bind.h:13:
In file included from include/pybind11/operators.h:12:
In file included from include/pybind11/pybind11.h:13:
In file included from include/pybind11/detail/class.h:12:
In file included from include/pybind11/detail/../attr.h:14:
In file included from include/pybind11/cast.h:14:
include/pybind11/detail/descr.h:40:19: error: use of typeid requires -frtti
        return {{&typeid(Ts)..., nullptr}};
                  ^
In file included from include_stl_bind_h.cpp:1:
In file included from include/pybind11/stl_bind.h:13:
In file included from include/pybind11/operators.h:12:
In file included from include/pybind11/pybind11.h:13:
In file included from include/pybind11/detail/class.h:12:
In file included from include/pybind11/detail/../attr.h:14:
In file included from include/pybind11/cast.h:15:
In file included from include/pybind11/detail/type_caster_base.h:12:
include/pybind11/detail/../pytypes.h:741:44: error: use of typeid requires -frtti
    return detail::isinstance_generic(obj, typeid(T));
                                           ^
In file included from include_stl_bind_h.cpp:1:
In file included from include/pybind11/stl_bind.h:13:
In file included from include/pybind11/operators.h:12:
In file included from include/pybind11/pybind11.h:13:
In file included from include/pybind11/detail/class.h:12:
In file included from include/pybind11/detail/../attr.h:14:
In file included from include/pybind11/cast.h:15:
In file included from include/pybind11/detail/type_caster_base.h:15:
include/pybind11/detail/internals.h:361:31: error: use of dynamic_cast requires -frtti
        if (const auto *nep = dynamic_cast<const std::nested_exception *>(std::addressof(e))) {
                              ^
In file included from include_stl_bind_h.cpp:1:
In file included from include/pybind11/stl_bind.h:13:
In file included from include/pybind11/operators.h:12:
In file included from include/pybind11/pybind11.h:13:
In file included from include/pybind11/detail/class.h:12:
In file included from include/pybind11/detail/../attr.h:14:
In file included from include/pybind11/cast.h:15:
In file included from include/pybind11/detail/type_caster_base.h:16:
include/pybind11/detail/typeid.h:62:34: error: use of typeid requires -frtti
    return detail::clean_type_id(typeid(T).name());
                                 ^
In file included from include_stl_bind_h.cpp:1:
In file included from include/pybind11/stl_bind.h:13:
In file included from include/pybind11/operators.h:12:
In file included from include/pybind11/pybind11.h:13:
In file included from include/pybind11/detail/class.h:12:
In file included from include/pybind11/detail/../attr.h:14:
In file included from include/pybind11/cast.h:15:
include/pybind11/detail/type_caster_base.h:894:23: error: use of typeid requires -frtti
        type = src ? &typeid(*src) : nullptr;
                      ^
include/pybind11/detail/type_caster_base.h:911:43: error: use of typeid requires -frtti
    type_caster_base() : type_caster_base(typeid(type)) {}
                                          ^
include/pybind11/detail/type_caster_base.h:930:33: error: use of typeid requires -frtti
        const auto &cast_type = typeid(itype);
                                ^
In file included from include_stl_bind_h.cpp:1:
In file included from include/pybind11/stl_bind.h:13:
In file included from include/pybind11/operators.h:12:
In file included from include/pybind11/pybind11.h:13:
In file included from include/pybind11/detail/class.h:12:
In file included from include/pybind11/detail/../attr.h:14:
include/pybind11/cast.h:1650:36: error: use of typeid requires -frtti
    return detail::get_type_handle(typeid(T), true);
                                   ^
In file included from include_stl_bind_h.cpp:1:
In file included from include/pybind11/stl_bind.h:13:
In file included from include/pybind11/operators.h:12:
In file included from include/pybind11/pybind11.h:13:
In file included from include/pybind11/detail/class.h:12:
include/pybind11/detail/../attr.h:551:69: error: use of typeid requires -frtti
    static void init(const base<T> &, type_record *r) { r->add_base(typeid(T), nullptr); }
                                                                    ^
In file included from include_stl_bind_h.cpp:1:
In file included from include/pybind11/stl_bind.h:13:
In file included from include/pybind11/operators.h:12:
include/pybind11/pybind11.h:305:70: error: use of typeid requires -frtti
                = const_cast<void *>(reinterpret_cast<const void *>(&typeid(FunctionType)));
                                                                     ^
include/pybind11/pybind11.h:1530:24: error: use of typeid requires -frtti
        record.type = &typeid(type);
                       ^
include/pybind11/pybind11.h:1551:39: error: use of typeid requires -frtti
            instances[std::type_index(typeid(type_alias))]
                                      ^
include/pybind11/pybind11.h:1552:45: error: use of typeid requires -frtti
                = instances[std::type_index(typeid(type))];
                                            ^
include/pybind11/pybind11.h:1558:22: error: use of typeid requires -frtti
        rec.add_base(typeid(Base), [](void *src) -> void * {
                     ^
include/pybind11/pybind11.h:1854:69: error: use of typeid requires -frtti
        auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type)));
                                                                    ^
include/pybind11/pybind11.h:2362:32: error: use of typeid requires -frtti
    if (!detail::get_type_info(typeid(state), false)) {
                               ^
include/pybind11/pybind11.h:2491:45: error: use of typeid requires -frtti
    if (auto *tinfo = detail::get_type_info(typeid(OutputType))) {
                                            ^
include/pybind11/pybind11.h:2761:41: error: use of typeid requires -frtti
    auto *tinfo = detail::get_type_info(typeid(T));
                                        ^
In file included from include_stl_bind_h.cpp:1:
include/pybind11/stl_bind.h:489:46: error: use of typeid requires -frtti
    auto *vtype_info = detail::get_type_info(typeid(vtype));
                                             ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.

rwgk · 2022-12-04T10:06:22Z

The global testing passed.

But I need to find a little bit of time to better understand what this PR is doing.

rwgk

The approach with the virtual functions looks really smart to me, and economical.

include/pybind11/stl_bind.h

tests/test_stl_binders.py

rwgk

Very nice!

I'm working on getting our CI healthy again under PR #4382. I already confirmed that the sledge-hammer method of globally changing ubuntu-latest to ubuntu-20.04 works. I'm still working on more targeted (limited) changes. My thinking: let's get that settled & merged first (hopefully today), then rebase this PR to confirm that we're all green again here, too.

include/pybind11/stl_bind.h

rwgk · 2022-12-05T04:44:40Z

let's get that settled & merged first (hopefully today), then rebase this PR to confirm that we're all green again here, too.

All done & green now!

aimir · 2022-12-06T08:39:04Z

Amazing! So is it ready to merge? 😄

rwgk · 2022-12-06T18:07:20Z

@Skylion007 could you please comment or approve?

Skylion007

Looks good to me.

Skylion007 · 2022-12-09T05:10:58Z

include/pybind11/stl_bind.h

+        if (it == map.end()) {
+            return false;
+        }
+        return true;


Suggested change

if (it == map.end()) {

return false;

}

return true;

return it != map.end();

Applied with commit 4525c5d

After that it was staring me in the face that map.find(k) != map.end() is even better.

And then clang-format turned the entire function into a one-liner.

… and implement them on-the-fly when wrapping any specific map type

…and const double, for example, as both wrappers will be named ValuesView[float]

…ypes for similarly-typed maps

…ght have different types

…ing the 'override' keyword when overriding functions

…he struct being defined inside a module, which was also needlessly ugly anyway

…ness they are specified to be static

…ues - I personally think this looks uglier, but it's probably worth it for clang-tidy to be happy

…c constexpr to make sure they are known before linking

Co-authored-by: Aaron Gokaslan <[email protected]>

…rence separately

…ames + stricter asserts

rwgk · 2022-12-09T06:47:23Z

Similar to #2768, I went ahead with a rebase & applying the review suggestions, to get this into the next smart_holder update.

Waiting for the CI.

@Skylion007

pybind#4353) * Create templated abstract classes KeysView, ValuesView and ItemsView, and implement them on-the-fly when wrapping any specific map type * We don't want to wrap different ValuesView objects for double values and const double, for example, as both wrappers will be named ValuesView[float] * Fallback to C++ names if key or values types are not wrapped * Added a test for .keys(), .values() and .items() returning the same types for similarly-typed maps * Fixed wrong use of auto in a declarator list: the two descriptions might have different types * Fixes for clang-tidy issues: explicit single-argument constructor, using the 'override' keyword when overriding functions * Bugfix for old versions of clang++, which seem to have trouble with the struct being defined inside a module, which was also needlessly ugly anyway * Bugfix for clang++, which doesn't have some of the names in runtime uness they are specified to be static * A fix for clang-tidy performance-inefficient-string-concatenation issues - I personally think this looks uglier, but it's probably worth it for clang-tidy to be happy * Possible fix for clang++ linking issues - make the descriptions static constexpr to make sure they are known before linking * Correct names for previously-wrapped types as keys/values of maps * Bugfix - typo in type info names which caused things to segfault * Apply suggestions from code review Co-authored-by: Aaron Gokaslan <[email protected]> * Use detail::remove_cvref_t instead of doing remove_cv and remove_reference separately * Avoid names with double underscore, as they are reserved * Improved testing for KeysView, ValuesView and ItemsView: check type names + stricter asserts * Moved description logic to helper function in type_caster_base.h * style: pre-commit fixes * Fix a clang-tidy issue: do not use 'else' after 'return' * Apply suggestion by @Skylion007, with additional trivial simplification. Co-authored-by: Amir <aimir@local> Co-authored-by: aimir <aimir@localhost> Co-authored-by: Aaron Gokaslan <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>

aimir marked this pull request as draft November 23, 2022 16:36

Skylion007 reviewed Nov 26, 2022

View reviewed changes

aimir marked this pull request as ready for review November 27, 2022 00:12

rwgk reviewed Dec 2, 2022

View reviewed changes

include/pybind11/stl_bind.h Outdated Show resolved Hide resolved

rwgk reviewed Dec 4, 2022

View reviewed changes

include/pybind11/stl_bind.h Show resolved Hide resolved

tests/test_stl_binders.py Show resolved Hide resolved

rwgk approved these changes Dec 4, 2022

View reviewed changes

include/pybind11/stl_bind.h Show resolved Hide resolved

rwgk force-pushed the map-view-types branch from 8c66127 to 67bcb1f Compare December 5, 2022 03:49

Skylion007 approved these changes Dec 9, 2022

View reviewed changes

Skylion007 reviewed Dec 9, 2022

View reviewed changes

Amir and others added 7 commits December 8, 2022 22:36

Create templated abstract classes KeysView, ValuesView and ItemsView,…

db09d94

… and implement them on-the-fly when wrapping any specific map type

We don't want to wrap different ValuesView objects for double values …

6bbde12

…and const double, for example, as both wrappers will be named ValuesView[float]

Fallback to C++ names if key or values types are not wrapped

f405e11

Added a test for .keys(), .values() and .items() returning the same t…

72a645e

…ypes for similarly-typed maps

Fixed wrong use of auto in a declarator list: the two descriptions mi…

3e1b653

…ght have different types

Fixes for clang-tidy issues: explicit single-argument constructor, us…

c2846b4

…ing the 'override' keyword when overriding functions

Bugfix for old versions of clang++, which seem to have trouble with t…

dfc3ee1

…he struct being defined inside a module, which was also needlessly ugly anyway

aimir and others added 13 commits December 8, 2022 22:36

Bugfix for clang++, which doesn't have some of the names in runtime u…

42d8694

…ness they are specified to be static

A fix for clang-tidy performance-inefficient-string-concatenation iss…

c558073

…ues - I personally think this looks uglier, but it's probably worth it for clang-tidy to be happy

Possible fix for clang++ linking issues - make the descriptions stati…

f0294f1

…c constexpr to make sure they are known before linking

Correct names for previously-wrapped types as keys/values of maps

ebf874d

Bugfix - typo in type info names which caused things to segfault

24019ae

Apply suggestions from code review

ddcc4e2

Co-authored-by: Aaron Gokaslan <[email protected]>

Use detail::remove_cvref_t instead of doing remove_cv and remove_refe…

21ec3ec

…rence separately

Avoid names with double underscore, as they are reserved

8abc0ae

Improved testing for KeysView, ValuesView and ItemsView: check type n…

60d754f

…ames + stricter asserts

Moved description logic to helper function in type_caster_base.h

e054752

style: pre-commit fixes

b4041b8

Fix a clang-tidy issue: do not use 'else' after 'return'

3a3b9f2

Apply suggestion by @Skylion007, with additional trivial simplification.

4525c5d

rwgk force-pushed the map-view-types branch from 67bcb1f to 4525c5d Compare December 9, 2022 06:43

rwgk merged commit 9db9880 into pybind:master Dec 9, 2022

github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 9, 2022

rwgk mentioned this pull request Dec 11, 2022

[KNOWN TRAP]: segfault with stl.h wrapper for std::map (and no optimization enabled) #4395

Closed

henryiii removed the needs changelog Possibly needs a changelog entry label Dec 20, 2022

nicholasjng mentioned this pull request Jan 6, 2023

Add nb::bind_map wjakob/nanobind#114

Merged

rwgk mentioned this pull request Feb 11, 2023

FWD pybind11 google/pybind11clif#4353

Closed

h-larsson mentioned this pull request Apr 8, 2023

ImportError for newer pybind11 versions block-hczhai/block2-preview#36

Closed

mika-fischer mentioned this pull request Apr 25, 2023

[BUG]: Binding two maps with two different integer key types failues due to duplicate KeyViews #4529

Closed

3 tasks

themarpe mentioned this pull request Jun 7, 2023

STL map bind naming fix for Sphinx and Pylance #3985

Closed

This was referenced Dec 14, 2023

bugfix: ensure different KeysView/ValuesView type names for int16_t/int32_t or float/double. Fix #4529 #4972

Closed

bugfix: KeysView/ValuesView/ItemsView using Python types. Fix #4529 #4983

Closed

rwgk mentioned this pull request Dec 21, 2023

fix: removing typing and duplicate class_ for KeysView/ValuesView/ItemsView. #4985

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct class names for KeysView, ValuesView and ItemsView in bind_map #4353

Correct class names for KeysView, ValuesView and ItemsView in bind_map #4353

aimir commented Nov 23, 2022 •

edited by henryiii

Loading

rwgk commented Nov 23, 2022

aimir commented Nov 23, 2022

rwgk commented Nov 25, 2022

aimir commented Dec 2, 2022

rwgk commented Dec 2, 2022

rwgk left a comment

aimir commented Dec 3, 2022

rwgk commented Dec 3, 2022

Skylion007 commented Dec 3, 2022

aimir commented Dec 3, 2022 •

edited

Loading

rwgk commented Dec 4, 2022

rwgk commented Dec 4, 2022

rwgk left a comment

rwgk left a comment

rwgk commented Dec 5, 2022

aimir commented Dec 6, 2022

rwgk commented Dec 6, 2022

Skylion007 left a comment

Skylion007 Dec 9, 2022

rwgk Dec 9, 2022

rwgk commented Dec 9, 2022

Correct class names for KeysView, ValuesView and ItemsView in bind_map #4353

Correct class names for KeysView, ValuesView and ItemsView in bind_map #4353

Conversation

aimir commented Nov 23, 2022 • edited by henryiii Loading

Description

Suggested changelog entry:

rwgk commented Nov 23, 2022

aimir commented Nov 23, 2022

rwgk commented Nov 25, 2022

aimir commented Dec 2, 2022

rwgk commented Dec 2, 2022

rwgk left a comment

Choose a reason for hiding this comment

aimir commented Dec 3, 2022

rwgk commented Dec 3, 2022

Skylion007 commented Dec 3, 2022

aimir commented Dec 3, 2022 • edited Loading

rwgk commented Dec 4, 2022

rwgk commented Dec 4, 2022

rwgk left a comment

Choose a reason for hiding this comment

rwgk left a comment

Choose a reason for hiding this comment

rwgk commented Dec 5, 2022

aimir commented Dec 6, 2022

rwgk commented Dec 6, 2022

Skylion007 left a comment

Choose a reason for hiding this comment

Skylion007 Dec 9, 2022

Choose a reason for hiding this comment

rwgk Dec 9, 2022

Choose a reason for hiding this comment

rwgk commented Dec 9, 2022

aimir commented Nov 23, 2022 •

edited by henryiii

Loading

aimir commented Dec 3, 2022 •

edited

Loading