Add nb::bind_map

[nicholasjng]

Adds a port of pybind11's bind_map to nanobind, including tests.

For now, all additions have been made inline in the new header file, nanobind/stl/bind_map.h. This file opts the user into the standard library headers <string> (due to its use of string concatenation for constructing dynamic keys/values view names) and <memory> (for dynamically allocating keys/values/items views.)

---

This is my best effort of porting over py::bind_map, so far. Since I am not writing C++ that often, please be relentless so that I can learn and do better :)

The use of <string> and <memory> are only necessary because I ported over pybind/pybind11#4353, which pulls them in for dynamically constructing string reprentations for keys/value/items views. Please give guidance on whether this is ok, or whether that makes bind_map incompatible with nanobind's design goals.

I would like guidance on the following questions that came up over the course of developing (I also left some TODOs which are essentially questions that I asked myself while implementing):

• How should I structure the added using defs (I assume they are detail-namespace-wide)?
• Is the polymorphism in KeysViewImpl et al. compatible with nanobind? (this ties into the above)
• I was confused at having to use NB_MAKE_OPAQUE in the tests (they are essentially ported verbatim from pybind), to avoid a static assertion about a missing type caster, where pybind does apparently not. What am I doing wrong there?

Thanks in advance for guidance and comments.

[wjakob]

Hi Nicholas,

this looks like a great start. A few comments:

nanobind uses C++17, which brought one huge simplification: if constexpr. All the ugly declaring of various functions with SFINAE and std::enable_if can go. You can just have a single function with a corresponding if constexpr (condition) { ...} block containing logic that must be disabled in some settings. This feature contributed significantly to the simplicity of nanobind compared to pybind11, and it would be good to also use it here.

That way, you can e.g. merge the various __setitem__ bindings and have an if constexpr adaptation inside. It will also significantly cut down on redundant type alias declarations like using KeyType = ...

Minor: Again, due to C++17, type trait-derived values like std::is_copy_constructible<typename Map::mapped_type>::value can be turned into the shorter std::is_copy_constructible_v<typename Map::mapped_type>.

The use of polymorphism seems overkill to me (it might have been more relevant for pybind11, where creating bindings for things in general has a larger cost). Let's instead use map-specific variants (e.g. replacing ItemsView by ItemsViewImpl, etc) without virtual destructor, etc. This means that the calls to nb_type_lookup can disappear.

You can probably even declare the helper view types inline within the main binding function.

The unique_ptr isn't needed -- you can directly return new ItemsView(..) etc., and nanobind will properly take ownership of the pointer.

Likewise, the string bindings also doesn't seem to be needed. You can install the views within the scope of their parent container (since there is now a 1:1 mapping), and then their relation ship will be clarified.

What was the static assertion you got?

[nicholasjng]

Thanks for the suggestions! I read up on if constexpr and used it to refactor the __setitem__ operator. I integrated your suggestions in 38c254d, let me know if I understood everything correctly.

If so, I can also redesign the tests later. Regarding the compile-time assertion, this is the CMake output I'm getting without NB_MAKE_OPAQUE (which I just pushed):

output.txt

[wjakob]

A few minor comments. I think the most important missing part are testcases -- we will want to cover all the functions and also account for special cases (for example non-copy-assignable types that are mentioned in one place.)

My suggestion would be that you simply reuse the existing tests by porting them from pybind11.

[wjakob]

Can you remove stl/map.h and stl/unordered_map.h from this file? This is likely the source of your error message regarding NB_OPAQUE (you imported the default STL map type caster and tried to declare bindings for a specific STL map).

[nicholasjng]

No, removing them makes the compiler complain (no template 'map' in namespace 'std').

[wjakob]

You can include map from the STL:

#include <map>
#include <unordered_map>

The nanobind header pulls in type casters that are unwanted in this context.

[nicholasjng]

Thanks, that works. Should I do the same with <string> instead of <nanobind/stl/string.h>?

[wjakob]

right. You only need nanobind/stl/.. to have automated conversion of the associated type between Python and C++ (and those automated converters work by always performing a copy).

This means that you would only need nanobind/stl/string.h if there is a testcase which returns a string to Python (or takes one as an input).

[nicholasjng]

Included <string> now instead of nanobind/stl/string.h, but the tests are still failing (the maps with non-copyable values). I don't know what is going wrong there, the branch seems correct - I will look at it later with a fresh mind.

[nicholasjng]

Thanks for the review, I just ported over the relevant tests from pybind. I am still getting compile errors, since nanobind attempts to bind a copy constructor to the map here for the non-copyable value class E_nc:

nanobind/include/nanobind/nb_class.h

Lines 277 to 279 in f4d5e12

	if constexpr (!std::is_trivially_copy_constructible_v<T>) {
	d.flags |= (uint32_t) detail::type_flags::has_copy;
	d.copy = detail::wrap_copy<T>;

I think this might be an error in my if constexpr branch, and I need to check for std::is_trivially_copy_constructible_v, too.

[nicholasjng]

I went into pybind's codebase to check why it works there, and they are using SFINAE to overload their own version of std::is_copy_constructible_v which returns false for containers with non-copyable values (same with is_copy_assignable:

https://github.com/pybind/pybind11/blob/a34596bfe1947b4a6b0bcc4218e1f72d0c2e9b4c/include/pybind11/detail/type_caster_base.h#L825-L841

Is there an obvious way to implement this as a constexpr bool to use C++17 style syntax?

[wjakob]

How about

if contexpr(std::is_copy_constructible_v<typename Container::value_type>) { .. }

?

[nicholasjng]

How about

if contexpr(std::is_copy_constructible_v<typename Container::value_type>) { .. }

?

That works for non-copyable value types, but fails for nested maps with non-copyable values (since std::is_copy_constructible_v<std::map<key, noncopyable_value>> = true), for example in L69:

test_stl_bind_map.cpp: L69
nb::bind_map<std::map<int, std::map<int, E_nc>>>(m, "MapMapENC");

[wjakob]

Ah, I understand now. The logic in pybind11 actually looks good to me, and implementing this using partial overloads is probably still the best way to go. You can basically copy those definitions of is_copy_constructible and then query them with if constexpr from your code (replacing std::is_copy_constructible with detail::is_copy_constructible).

[wjakob]

Can you add these type traits to a new file include/nanobind/stl/detail/type_traits.h? This is all very specific to STL bindings and should not be added to nanobind by default.

[nicholasjng]

Will do, thanks for the clarification!

[wjakob]

Ah wait -- if I understand correctly, the if constexpr (!std::is_trivially_copy_constructible_v<T>) part in nanobind is broken, correct?

[nicholasjng]

Yes, that's due to the same phenomenon - a map with a non-copyable value is copyable, and if I understand correctly, the default copy constructor of a map tries to call the copy constructor of its value (or rather, the std::pair<key, value>, whose copy constructor is deleted if one of the two types has a deleted CC), which is an error here.

EDIT: Related pybind PR: pybind/pybind11#1886

[wjakob]

I pushed a fix, see 94bdb05

[nicholasjng]

Thanks, looks good to me. Should I add aliases for the values (detail::is_copy_constructible_v), and do I need to do the same for std::is_copy_assignable? (It was there in pybind/pybind11#1886, so I assume that needs to go in too)

[wjakob]

Thanks, looks good to me. Should I add aliases for the values (detail::is_copy_constructible_v), and do I need to do the same for std::is_copy_assignable? (It was there in pybind/pybind11#1886, so I assume that needs to go in too)

Sure, let's do that. The goal of my commit was just to fix the parts affecting core parts of nanobind without adding the complicated templates there by default.

[nicholasjng]

Thanks, just pushed the ported templates and values. I'm confident that that fixed the __setitem__ assignment - tests are still failing though, because the created ValuesView iterator impl ends up calling the wrong overload, which becomes aninsert, creating a copy of the map value. Is there a missing reference somewhere? Seems like we should not be copying on iteration here.

[wjakob]

Hi @nicholasjng -- I made a simplification pass over your code, please check that it still okay to you. I had to make a change to nanobind on master for this to work (8916f51) and I then rebased your code & force-pushed to your branch. 😇

[nicholasjng]

Nice, thank you for the fixes! Could you briefly clarify why it needs to be const as well in 8916f51?

[wjakob]

The const may not be needed, I added it to accommodate every possible iterator. nanobind will in any case cast away the const part internally since such a concept isn't easily mapped onto Python.

[wjakob]

Shall I merge this then?

[nicholasjng]

Yep, looks good to me!

[wjakob]

Thanks!

[nicholasjng]

Thanks for the help and the review! See you again soon for nb::bind_vector, I guess :)

[wjakob]

That one should be refreshingly easy in comparison.