DM-19582: Add cross-language GenericMap unit tests

include/lsst/afw/typehandling/GenericMap.h

@@ -26,7 +26,6 @@
 #define LSST_AFW_TYPEHANDLING_GENERICMAP_H
 
 #include <algorithm>
-#include <cstdint>
 #include <functional>
 #include <ostream>
 #include <memory>
@@ -238,7 +237,11 @@ class GenericMap {
     /**
      * Return a reference to the mapped value of the element with key equal to `key`.
      *
-     * @tparam T the type of the element mapped to `key`
+     * @tparam T the type of the element mapped to `key`. It may be the exact
+     *           type of the element, if known, or any type to which its
+     *           references or pointers can be implicitly converted (e.g.,
+     *           a superclass). For example, references to built-in types are
+     *           not convertible, so you can't retrieve an `int` with `T=long`.
      * @param key the key of the element to find
      *
      * @return a reference to the `T` mapped to `key`, if one exists
@@ -409,8 +412,9 @@ class GenericMap {
      * type (which may be `void`). Through any combination of overloading or
      * templates, the visitor must accept values of the following types:
      *      * either `bool` or `bool const&`
-     *      * either `std::int32_t` or `std::int32_t const&`
-     *      * either `std::int64_t` or `std::int64_t const&`
+     *      * either `int` or `int const&`
+     *      * either `long` or `long const&`
+     *      * either `long long` or `long long const&`
      *      * either `float` or `float const&`
      *      * either `double` or `double const&`
      *      * `std::string const&`
@@ -499,7 +503,9 @@ class GenericMap {
      *
      * Keys of any subclass of Storable are implemented using PolymorphicValue to preserve type.
      */
-    using StorableType = boost::variant<bool, std::int32_t, std::int64_t, float, double, std::string,
+    // Use int, long, long long instead of int32_t, int64_t because C++ doesn't
+    // consider signed integers of the same length but different names equivalent
+    using StorableType = boost::variant<bool, int, long, long long, float, double, std::string,
                                         PolymorphicValue, std::shared_ptr<Storable const>>;
 
     /**

include/lsst/afw/typehandling/python.h

@@ -49,6 +49,8 @@ namespace typehandling {
 template <class Base = Storable>
 class StorableHelper : public Base {
 public:
+    using Base::Base;
+
     // Can't wrap clone(); PYBIND11_OVERLOAD chokes on unique_ptr return type
 
     std::string toString() const override {
@@ -59,7 +61,6 @@ class StorableHelper : public Base {
         PYBIND11_OVERLOAD_NAME(std::size_t, Base, "__hash__", hash_value, );
     }
 
-protected:
     bool equals(Base const& other) const noexcept override {
         PYBIND11_OVERLOAD_NAME(bool, Base, "__eq__", equals, other);
     }

include/lsst/afw/typehandling/test.h

@@ -256,8 +256,9 @@ BOOST_TEST_CASE_TEMPLATE_FUNCTION(TestConstVisitor, GenericMapFactory) {
 
         // Local classes can't have method templates
         void operator()(int, bool value) { results.push_back(universalToString(value)); }
-        void operator()(int, std::int32_t const& value) { results.push_back(universalToString(value)); }
-        void operator()(int, std::int64_t value) { results.push_back(universalToString(value)); }
+        void operator()(int, int const& value) { results.push_back(universalToString(value)); }
+        void operator()(int, long value) { results.push_back(universalToString(value)); }
+        void operator()(int, long long value) { results.push_back(universalToString(value)); }
         void operator()(int, float const& value) { results.push_back(universalToString(value)); }
         void operator()(int, double value) { results.push_back(universalToString(value)); }
         void operator()(int, std::string const& value) { results.push_back(universalToString(value)); }
@@ -293,8 +294,9 @@ BOOST_TEST_CASE_TEMPLATE_FUNCTION(TestModifyingVoidVisitor, GenericMapFactory) {
     public:
         // Local classes can't have method templates
         void operator()(int, bool& value) { value = !value; }
-        void operator()(int, std::int32_t& value) { value *= 2; }
-        void operator()(int, std::int64_t& value) { value *= 2; }
+        void operator()(int, int& value) { value *= 2; }
+        void operator()(int, long& value) { value *= 2; }
+        void operator()(int, long long& value) { value *= 2; }
         void operator()(int, float& value) { value *= 2; }
         void operator()(int, double& value) { value *= 2; }
         void operator()(int, std::string& value) { value += "Appendix"; }
@@ -333,11 +335,15 @@ BOOST_TEST_CASE_TEMPLATE_FUNCTION(TestModifyingReturningVisitor, GenericMapFacto
             value = !value;
             return key;
         }
-        int operator()(int key, std::int32_t& value) {
+        int operator()(int key, int& value) {
             value *= 2;
             return key;
         }
-        int operator()(int key, std::int64_t& value) {
+        int operator()(int key, long& value) {
+            value *= 2;
+            return key;
+        }
+        int operator()(int key, long long& value) {
             value *= 2;
             return key;
         }

python/lsst/afw/typehandling/_GenericMap.cc

@@ -74,7 +74,18 @@ template <typename K>
 py::object get(GenericMap<K>& self, K const& key) {
     auto callable = static_cast<typename Publicist<K>::ConstValueReference (GenericMap<K>::*)(K) const>(
             &Publicist<K>::unsafeLookup);
-    return py::cast((self.*callable)(key));
+    auto variant = (self.*callable)(key);
+
+    // py::cast can't convert PolymorphicValue to Storable; do it by hand
+    PolymorphicValue const* storableHolder = boost::get<PolymorphicValue const&>(&variant);
+    if (storableHolder) {
+        Storable const& value = *storableHolder;
+        // Prevent segfaults when assigning a Key<Storable> to Python variable, then deleting from map
+        // No existing code depends on being able to modify an item stored by value
+        return py::cast(value.cloneStorable());
+    } else {
+        return py::cast(variant);
+    }
 };
 
 template <typename K>
@@ -109,9 +120,7 @@ void declareGenericMap(utils::python::WrapperCollection& wrappers, std::string c
                         std::throw_with_nested(py::key_error(buffer.str()));
                     }
                 },
-                // Prevent segfaults when assigning a key<Storable> to Python variable, then deleting from map
-                // No existing code depends on being able to modify an item stored by value
-                "key"_a, py::return_value_policy::copy);
+                "key"_a);
         cls.def("get",
                 [](Class& self, K const& key, py::object const& def) {
                     try {
@@ -167,12 +176,13 @@ void declareMutableGenericMap(utils::python::WrapperCollection& wrappers, std::s
     wrappers.wrapType(PyClass(wrappers.module, className.c_str(), docstring.c_str()),
                       [](auto& mod, auto& cls) {
                           // Don't rewrap members of GenericMap
-                          declareMutableGenericMapTypedMethods<std::shared_ptr<Storable>>(cls);
+                          declareMutableGenericMapTypedMethods<std::shared_ptr<Storable const>>(cls);
                           declareMutableGenericMapTypedMethods<bool>(cls);
+                          // TODO: int32 and float are suppressed for now, see DM-21268
+                          declareMutableGenericMapTypedMethods<std::int64_t>(cls);  // chosen for builtins.int
                           declareMutableGenericMapTypedMethods<std::int32_t>(cls);
-                          declareMutableGenericMapTypedMethods<std::int64_t>(cls);
+                          declareMutableGenericMapTypedMethods<double>(cls);  // chosen for builtins.float
                           declareMutableGenericMapTypedMethods<float>(cls);
-                          declareMutableGenericMapTypedMethods<double>(cls);
                           declareMutableGenericMapTypedMethods<std::string>(cls);
                           cls.def("__delitem__",
                                   [](Class& self, K const& key) {

tests/SConscript

@@ -1,7 +1,7 @@
 # -*- python -*-
 from lsst.sconsUtils import scripts, env, targets
 
-pybind11_test_modules = ['testTableArchivesLib']
+pybind11_test_modules = ['testTableArchivesLib', 'testGenericMapLib']
 
 scripts.BasicSConscript.pybind11(pybind11_test_modules, addUnderscore=False)
 
@@ -11,7 +11,6 @@ if afwdataDir:
     env["ENV"]["AFWDATA_DIR"] = afwdataDir
 
 scripts.BasicSConscript.tests(pyList=[],
-                              noBuildList=[name + '.cc' for name in pybind11_test_modules],
-                              ignoreList=[name + '.py' for name in pybind11_test_modules])
+                              noBuildList=[name + '.cc' for name in pybind11_test_modules])
 
 env.Clean(targets["tests"], "#testTable.fits")