Fix ABI conformance of IMap::Remove, add TryRemove (#655)

* Fix ABI conformance of IMap::Remove, add TryRemove

`single_threaded_map()`'s IMap::Remove did not throw
`hresult_out_of_range` on attempts to remove a nonexistent
key. Now it throws.

Failure to remove a nonexistent key does not invalidate
iterators, because nothing actually changed. This brings
the map implementation in line with the implementations
in other projections.

Note that this is a breaking change. Code that assumed
nonexistent objects could be harmlessly removed will
encounter exceptions when run against C++/WinRT implementations.
This was, however, a pre-existing bug, because implementations
from other projections (C#, C++/CX) always threw under those
conditions.

Added a TryRemove() method for people who wanted the
nonthrowing version.

Note that fixing the ABI conformance is required in order
for TryRemove to work, because TryRemove relies on the
call to Remove failing if the key doesn't exist.

(JavaScript doesn't project objects as maps, so there
is nothing to validate there.)

Tightened the behavior of TryLookup and TryRemove so
they propagate RPC failures. Because the inability to
remove the item could be due to the server being unavailable,
and that's not the same as the item not existing in
the collection. Previous code treated loss of server
the same as "The item doesn't exist", which is not true:
The item could exist, we just were unable to contact
the server to find out.

* TryLookup and TryRemove should not be noexcept

because they can throw on other errors.

Added unit test to verify that errors other than "key not found" are
propagated.

Co-authored-by: Kenny Kerr <[email protected]>

Author: oldnewthing

cppwinrt/code_writers.h

@@ -1305,20 +1305,20 @@ namespace cppwinrt
         else if (type_name == "Windows.Foundation.Collections.IMapView`2")
         {
             w.write(R"(
-        auto TryLookup(param_type<K> const& key) const noexcept
+        auto TryLookup(param_type<K> const& key) const
         {
             if constexpr (std::is_base_of_v<Windows::Foundation::IUnknown, V>)
             {
                 V result{ nullptr };
-                WINRT_IMPL_SHIM(Windows::Foundation::Collections::IMapView<K, V>)->Lookup(get_abi(key), put_abi(result));
+                impl::check_hresult_allow_bounds(WINRT_IMPL_SHIM(Windows::Foundation::Collections::IMapView<K, V>)->Lookup(get_abi(key), put_abi(result)));
                 return result;
             }
             else
             {
                 std::optional<V> result;
                 V value{ empty_value<V>() };
 
-                if (0 == WINRT_IMPL_SHIM(Windows::Foundation::Collections::IMapView<K, V>)->Lookup(get_abi(key), put_abi(value)))
+                if (0 == impl::check_hresult_allow_bounds(WINRT_IMPL_SHIM(Windows::Foundation::Collections::IMapView<K, V>)->Lookup(get_abi(key), put_abi(value))))
                 {
                     result = std::move(value);
                 }
@@ -1331,27 +1331,32 @@ namespace cppwinrt
         else if (type_name == "Windows.Foundation.Collections.IMap`2")
         {
             w.write(R"(
-        auto TryLookup(param_type<K> const& key) const noexcept
+        auto TryLookup(param_type<K> const& key) const
         {
             if constexpr (std::is_base_of_v<Windows::Foundation::IUnknown, V>)
             {
                 V result{ nullptr };
-                WINRT_IMPL_SHIM(Windows::Foundation::Collections::IMap<K, V>)->Lookup(get_abi(key), put_abi(result));
+                impl::check_hresult_allow_bounds(WINRT_IMPL_SHIM(Windows::Foundation::Collections::IMap<K, V>)->Lookup(get_abi(key), put_abi(result)));
                 return result;
             }
             else
             {
                 std::optional<V> result;
                 V value{ empty_value<V>() };
 
-                if (0 == WINRT_IMPL_SHIM(Windows::Foundation::Collections::IMap<K, V>)->Lookup(get_abi(key), put_abi(value)))
+                if (0 == impl::check_hresult_allow_bounds(WINRT_IMPL_SHIM(Windows::Foundation::Collections::IMap<K, V>)->Lookup(get_abi(key), put_abi(value))))
                 {
                     result = std::move(value);
                 }
 
                 return result;
             }
         }
+
+        auto TryRemove(param_type<K> const& key) const
+        {
+            return 0 == impl::check_hresult_allow_bounds(WINRT_IMPL_SHIM(Windows::Foundation::Collections::IMap<K, V>)->Remove(get_abi(key)));
+        }
 )");
         }
         else if (type_name == "Windows.Foundation.IAsyncAction")

strings/base_collections_base.h

@@ -1,4 +1,3 @@
-
 WINRT_EXPORT namespace winrt
 {
     template <typename D, typename T, typename Version = impl::no_collection_version>
@@ -415,8 +414,14 @@ WINRT_EXPORT namespace winrt
 
         void Remove(K const& key)
         {
+            auto& container = static_cast<D&>(*this).get_container();
+            auto found = container.find(static_cast<D const&>(*this).wrap_value(key));
+            if (found == container.end())
+            {
+                throw hresult_out_of_bounds();
+            }
             this->increment_version();
-            static_cast<D&>(*this).get_container().erase(static_cast<D const&>(*this).wrap_value(key));
+            container.erase(found);
         }
 
         void Clear() noexcept

strings/base_error.h

@@ -579,3 +579,15 @@ WINRT_EXPORT namespace winrt
         abort();
     }
 }
+
+namespace winrt::impl
+{
+    inline hresult check_hresult_allow_bounds(hresult const result)
+    {
+        if (result != impl::error_out_of_bounds)
+        {
+            check_hresult(result);
+        }
+        return result;
+    }
+}

test/old_tests/UnitTests/TryLookup.cpp

@@ -90,3 +90,41 @@ TEST_CASE("TryLookup")
         REQUIRE(map.TryLookup(123).value() == 456);
     }
 }
+
+TEST_CASE("TryRemove")
+{
+    auto map = single_threaded_map<int, IStringable>(std::map<int, IStringable>{
+            { 123, nullptr },
+            { 124, make<stringable>(L"remove") },
+            { 125, make<stringable>(L"keep") },
+        });
+
+    REQUIRE(map.TryRemove(122) == false);
+    REQUIRE(map.TryRemove(123) == true);
+    REQUIRE(map.TryRemove(124) == true);
+
+    // Should still have one item left.
+    REQUIRE(map.Size() == 1);
+    REQUIRE(map.Lookup(125).ToString() == L"keep");
+}
+
+TEST_CASE("TryLookup TryRemove error")
+{
+    // Simulate a non-agile map that is being accessed from the wrong thread.
+    // "Try" operations should throw rather than erroneously report "not found".
+    // Because they didn't even try. The operation never got off the ground.
+    struct incorrectly_used_non_agile_map : implements<incorrectly_used_non_agile_map, IMap<int, int>>
+    {
+        int Lookup(int) { throw hresult_wrong_thread(); }
+        int32_t Size() { throw hresult_wrong_thread(); }
+        bool HasKey(int) { throw hresult_wrong_thread(); }
+        IMapView<int, int> GetView() { throw hresult_wrong_thread(); }
+        bool Insert(int, int) { throw hresult_wrong_thread(); }
+        void Remove(int) { throw hresult_wrong_thread(); }
+        void Clear() { throw hresult_wrong_thread(); }
+    };
+    
+    auto map = make<incorrectly_used_non_agile_map>();
+    REQUIRE_THROWS_AS(map.TryLookup(123), hresult_wrong_thread);
+    REQUIRE_THROWS_AS(map.TryRemove(123), hresult_wrong_thread);
+}

test/old_tests/UnitTests/produce_map.cpp

@@ -93,7 +93,7 @@ TEST_CASE("produce_IMap_int32_t_hstring")
     REQUIRE(m.Size() == 2);
     m.Remove(1); // existing
     REQUIRE(m.Size() == 1);
-    m.Remove(3); // not existing
+    REQUIRE_THROWS_AS(m.Remove(3), hresult_out_of_bounds); // not existing
     REQUIRE(m.Size() == 1);
 
     m.Clear();
@@ -177,7 +177,8 @@ TEST_CASE("produce_IMap_hstring_int32_t")
     REQUIRE(m.Size() == 2);
     m.Remove(L"one"); // existing
     REQUIRE(m.Size() == 1);
-    m.Remove(L"three"); // not existing
+    REQUIRE_THROWS_AS(m.Remove(L"three"), hresult_out_of_bounds); // not existing
+    REQUIRE(!m.TryRemove(L"three")); // not existing
     REQUIRE(m.Size() == 1);
 
     m.Clear();

test/old_tests/UnitTests/single_threaded_map.cpp

@@ -28,6 +28,7 @@ namespace
         values.Insert(2,20);
         values.Insert(3,30);
         IIterator<IKeyValuePair<int, int>> first = values.First();
+        REQUIRE(!values.TryRemove(999)); // failed removal does not invalidate
         REQUIRE(first.HasCurrent());
         [[maybe_unused]] auto pair = first.Current();
         REQUIRE(first.MoveNext());
@@ -52,7 +53,8 @@ namespace
         REQUIRE(!values.Insert(2, 20));
         compare(values, { { 1,100 }, {2,20} });
 
-        values.Remove(3);
+        REQUIRE_THROWS_AS(values.Remove(3), hresult_out_of_bounds);
+        REQUIRE(!values.TryRemove(3));
         compare(values, { { 1,100 },{ 2,20 } });
         values.Remove(2);
         compare(values, { { 1,100 } });
@@ -65,7 +67,7 @@ namespace
         compare(values, {});
 
         test_invalidation(values, [&] { values.Clear(); });
-        test_invalidation(values, [&] { values.Remove(10); });
+        test_invalidation(values, [&] { values.Remove(1); });
         test_invalidation(values, [&] { values.Insert(1,10); });
     }
 }