Remove throwing/originating errors in expected scenarios (Lookup/TryLookup and Cancel)

.runsettings

@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="utf-8"?>
+<RunSettings>
+  <RunConfiguration>
+    <ResultsDirectory>.\TestResults</ResultsDirectory><!-- Path relative to solution directory -->
+    <TestSessionTimeout>60000</TestSessionTimeout><!-- Milliseconds -->
+    <ForceListContent>true</ForceListContent>
+  </RunConfiguration>
+
+  <!-- Adapter Specific sections -->
+
+  <!-- Catch2 adapter -->
+  <Catch2Adapter>
+    <ExecutionMode>Single</ExecutionMode>
+    <FilenameFilter>(?i:Test)</FilenameFilter><!-- Regex filter -->
+    <ForceListContent>true</ForceListContent>
+    <DebugBreak>on</DebugBreak>
+    <IncludeHidden>true</IncludeHidden>
+    <Logging>Verbose</Logging>
+    <MessageFormat>AdditionalInfo</MessageFormat>
+    <StackTraceFormat>ShortInfo</StackTraceFormat>
+    <StackTracePointReplacement>,</StackTracePointReplacement>
+    <TestCaseTimeout>20000</TestCaseTimeout><!-- 20s in Milliseconds -->
+  </Catch2Adapter>
+</RunSettings>

README.md

@@ -34,3 +34,8 @@ a dev command prompt at the root of the repo _after_ following the above build i
 * Run `build_prior_projection.cmd` in the dev command prompt as well
 * Run `prepare_versionless_diffs.cmd` which removes version stamps on both current and prior projection
 * Use a directory-level differencing tool to compare `_build\$(arch)\$(flavor)\winrt` and `_reference\$(arch)\$(flavor)\winrt`
+
+## Testing
+This repository uses the [Catch2](https://github.com/catchorg/Catch2) testing framework.
+- From a Visual Studio command line, you should run `build_tests_all.cmd` to build and run the tests. To Debug the tests, you can debug the associated `_build\$(arch)\$(flavor)\<test>.exe` under the debugger of your choice.
+- Optionally, you can install the [Catch2Adapter](https://marketplace.visualstudio.com/items?itemName=JohnnyHendriks.ext01) to run the tests from Visual Studio.

cppwinrt/code_writers.h

@@ -1873,6 +1873,36 @@ namespace cppwinrt
         }
     }
 
+    static void write_produce_upcall_TryLookup(writer& w, std::string_view const& upcall, method_signature const& method_signature)
+    {
+        auto name = method_signature.return_param_name();
+
+        w.write("auto out_param_val = %(%, trylookup_from_abi);",
+            upcall,
+            bind<write_produce_args>(method_signature));
+        w.write(R"(
+                if (out_param_val.has_value()) 
+                {
+                    *% = detach_from<%>(std::move(*out_param_val));
+                }
+                else 
+                {
+                    return impl::error_out_of_bounds; 
+                }
+)", 
+            name, method_signature.return_signature());
+
+        for (auto&& [param, param_signature] : method_signature.params())
+        {
+            if (param.Flags().Out() && !param_signature->Type().is_szarray() && is_object(param_signature->Type()))
+            {
+                auto param_name = param.Name();
+
+                w.write("\n            if (%) *% = detach_abi(winrt_impl_%);", param_name, param_name, param_name);
+            }
+        }
+    }
+
     static void write_produce_method(writer& w, MethodDef const& method)
     {
         std::string_view format;
@@ -1902,13 +1932,44 @@ namespace cppwinrt
         method_signature signature{ method };
         auto async_types_guard = w.push_async_types(signature.is_async());
         std::string upcall = "this->shim().";
-        upcall += get_name(method);
-
-        w.write(format,
-            get_abi_name(method),
-            bind<write_produce_params>(signature),
-            bind<write_produce_cleanup>(signature),
-            bind<write_produce_upcall>(upcall, signature));
+        auto name = get_name(method);
+        upcall += name;
+        // if (type_name == "Windows.Foundation.Collections.IMapView`2")
+        if (name == "Lookup")
+        {
+            // Special-case Lookup to look for a TryLookup here, to avoid a throw/originate
+            // and add a small performance improvement.
+            std::string tryLookupUpCall = "this->shim().TryLookup";
+            format = R"(        int32_t __stdcall %(%) noexcept final try
+        {
+%            typename D::abi_guard guard(this->shim());
+            if constexpr (has_TryLookup_v<D, K>)
+            {
+                %
+            }
+            else
+            {
+                %
+            }
+            return 0;
+        }
+        catch (...) { return to_hresult(); }
+)";
+            w.write(format,
+                get_abi_name(method),
+                bind<write_produce_params>(signature),
+                bind<write_produce_cleanup>(signature), // clear_abi
+                bind<write_produce_upcall_TryLookup>(tryLookupUpCall, signature),
+                bind<write_produce_upcall>(upcall, signature));
+        }
+        else
+        {
+            w.write(format,
+                get_abi_name(method),
+                bind<write_produce_params>(signature),
+                bind<write_produce_cleanup>(signature),
+                bind<write_produce_upcall>(upcall, signature));
+        }
     }
 
     static void write_fast_produce_methods(writer& w, TypeDef const& default_interface)

strings/base_collections_base.h

@@ -506,6 +506,20 @@ WINRT_EXPORT namespace winrt
     template <typename D, typename K, typename V, typename Version = impl::no_collection_version>
     struct map_view_base : iterable_base<D, Windows::Foundation::Collections::IKeyValuePair<K, V>, Version>
     {
+        // specialization of Lookup that avoids throwing the hresult
+        std::optional<V> TryLookup(K const& key, trylookup_from_abi_t) const
+        {
+            [[maybe_unused]] auto guard = static_cast<D const&>(*this).acquire_shared();
+            auto pair = static_cast<D const&>(*this).get_container().find(static_cast<D const&>(*this).wrap_value(key));
+
+            if (pair == static_cast<D const&>(*this).get_container().end())
+            {
+                return { std::nullopt };
+            }
+
+            return { static_cast<D const&>(*this).unwrap_value(pair->second) };
+        }
+
         V Lookup(K const& key) const
         {
             [[maybe_unused]] auto guard = static_cast<D const&>(*this).acquire_shared();
@@ -536,6 +550,7 @@ WINRT_EXPORT namespace winrt
             first = nullptr;
             second = nullptr;
         }
+
     };
 
     template <typename D, typename K, typename V>

strings/base_coroutine_foundation.h

@@ -351,6 +351,10 @@ namespace winrt::impl
             return m_promise->enable_cancellation_propagation(value);
         }
 
+        bool originate_on_cancel(bool value = true) const noexcept
+        {
+            return m_promise->originate_on_cancel(value);
+        }
     private:
 
         Promise* m_promise;
@@ -484,7 +488,14 @@ namespace winrt::impl
                 if (m_status.load(std::memory_order_relaxed) == AsyncStatus::Started)
                 {
                     m_status.store(AsyncStatus::Canceled, std::memory_order_relaxed);
-                    m_exception = std::make_exception_ptr(hresult_canceled());
+                    if (cancellable_promise::originate_on_cancel())
+                    {
+                        m_exception = std::make_exception_ptr(hresult_canceled());
+                    }
+                    else
+                    {
+                        m_exception = std::make_exception_ptr(hresult_canceled(hresult_error::no_originate));
+                    }
                     cancel = std::move(m_cancel);
                 }
             }
@@ -628,7 +639,14 @@ namespace winrt::impl
         {
             if (Status() == AsyncStatus::Canceled)
             {
-                throw winrt::hresult_canceled();
+                if (cancellable_promise::originate_on_cancel())
+                {
+                    throw winrt::hresult_canceled();
+                }
+                else
+                {
+                    throw winrt::hresult_canceled(hresult_error::no_originate);
+                }
             }
 
             return std::forward<Expression>(expression);

strings/base_coroutine_threadpool.h

@@ -180,12 +180,23 @@ WINRT_EXPORT namespace winrt
             return m_propagate_cancellation;
         }
 
+        bool originate_on_cancel(bool value = true) noexcept
+        {
+            return std::exchange(m_originate_on_cancel, value);
+        }
+
+        bool should_originate_on_cancel() const noexcept
+        {
+            return m_originate_on_cancel;
+        }
+
     private:
         static inline auto const cancelling_ptr = reinterpret_cast<canceller_t>(1);
 
         std::atomic<canceller_t> m_canceller{ nullptr };
         void* m_context{ nullptr };
         bool m_propagate_cancellation{ false };
+        bool m_originate_on_cancel{ true }; // By default, will call RoOriginateError before throwing a cancel error code.
     };
 
     template <typename Derived>

strings/base_error.h

@@ -84,6 +84,9 @@ WINRT_EXPORT namespace winrt
 {
     struct hresult_error
     {
+        struct no_originate_t {};
+        static constexpr no_originate_t no_originate{};
+
         using from_abi_t = take_ownership_from_abi_t;
         static constexpr auto from_abi{ take_ownership_from_abi };
 
@@ -109,6 +112,10 @@ WINRT_EXPORT namespace winrt
             originate(code, nullptr, sourceInformation);
         }
 
+        explicit hresult_error(hresult const code, no_originate_t, [[maybe_unused]] winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : m_code(verify_error(code))
+        {
+        }
+
         hresult_error(hresult const code, param::hstring const& message, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : m_code(verify_error(code))
         {
             originate(code, get_abi(message), sourceInformation);
@@ -325,6 +332,7 @@ WINRT_EXPORT namespace winrt
     struct hresult_canceled : hresult_error
     {
         hresult_canceled(winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, sourceInformation) {}
+        hresult_canceled(hresult_error::no_originate_t) noexcept : hresult_error(impl::error_canceled, hresult_error::no_originate) {}
         hresult_canceled(param::hstring const& message, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, message, sourceInformation) {}
         hresult_canceled(take_ownership_from_abi_t, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, take_ownership_from_abi, sourceInformation) {}
     };

strings/base_meta.h

@@ -10,6 +10,10 @@ WINRT_EXPORT namespace winrt
     struct take_ownership_from_abi_t {};
     inline constexpr take_ownership_from_abi_t take_ownership_from_abi{};
 
+    // Map implementations can implement TryLookup with trylookup_from_abi_t as an optimization
+    struct trylookup_from_abi_t {};
+    inline constexpr trylookup_from_abi_t trylookup_from_abi{};
+
     template <typename T>
     struct com_ptr;
 
@@ -298,4 +302,18 @@ namespace winrt::impl
             return (func(Types{}) || ...);
         }
     };
+
+    template <typename D, typename K>
+    struct has_TryLookup
+    {
+        template <typename U, typename = decltype(std::declval<U>().TryLookup(std::declval<K>(), trylookup_from_abi))> static constexpr bool get_value(int) { return true; }
+        template <typename> static constexpr bool get_value(...) { return false; }
+
+    public:
+
+        static constexpr bool value = get_value<D>(0);
+    };
+
+    template <typename D, typename K>
+    inline constexpr bool has_TryLookup_v = has_TryLookup<D, K>::value;
 }

test/old_tests/UnitTests/Errors.cpp

@@ -233,6 +233,7 @@ TEST_CASE("Errors")
 
     // Make sure trimming works.
     hresult_error e(E_FAIL, L":) is \u263A \n \t ");
+    auto x = e.message();
     REQUIRE(e.message() == L":) is \u263A");
 
     // Make sure delegates propagate correctly.

test/old_tests/UnitTests/TryLookup.cpp

@@ -143,4 +143,103 @@ TEST_CASE("TryLookup TryRemove error")
     REQUIRE(!map.TryLookup(123));
     REQUIRE(!map.TryRemove(123));
 
-}
+}
+
+TEST_CASE("trylookup_from_abi specialization")
+{
+    // A map that throws a specific error, used to verify various edge cases.
+    // and implements tryLookup, to take advantage of an optimization to avoid a throw. 
+    struct map_with_try_lookup : implements<map_with_try_lookup, IMapView<int, int>>
+    {
+        hresult codeToThrow{ S_OK };
+        bool shouldThrowOnTryLookup{ false };
+        std::optional<int> TryLookup(int, trylookup_from_abi_t)
+        {
+            if (shouldThrowOnTryLookup)
+            { 
+                throw_hresult(codeToThrow); 
+            }
+            else
+            {
+                return { std::nullopt };
+            }
+        }
+        int Lookup(int) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
+        int32_t Size() { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
+        bool HasKey(int) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
+        void Split(IMapView<int, int>&, IMapView<int, int>&) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
+    };
+
+    auto self = make_self<map_with_try_lookup>();
+    IMapView<int, int> map = *self;
+
+    // Make sure that we use the TryLookup specialization, and don't throw an unexpected exception.
+    self->shouldThrowOnTryLookup = false;
+    REQUIRE(!map.TryLookup(123));
+    // make sure regular lookup stll throws bounds
+    REQUIRE_THROWS_AS(map.Lookup(123), hresult_out_of_bounds);
+
+    // 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.
+    self->shouldThrowOnTryLookup = true;
+    self->codeToThrow = RPC_E_WRONG_THREAD;
+    REQUIRE_THROWS_AS(map.TryLookup(123), hresult_wrong_thread);
+    // regular lookup should throw the same error
+    REQUIRE_THROWS_AS(map.Lookup(123), hresult_wrong_thread);
+}
+
+TEST_CASE("trylookup_from_abi specialization with IInspectable")
+{
+    // A map that throws a specific error, used to verify various edge cases.
+    // and implements tryLookup, to take advantage of an optimization to avoid a throw. 
+    struct map_with_try_lookup : implements<map_with_try_lookup, IMapView<int, IInspectable>>
+    {
+        hresult codeToThrow{ S_OK };
+        bool shouldThrowOnTryLookup{ false };
+        bool returnNullptr{ false };
+        std::optional<IInspectable> TryLookup(int, trylookup_from_abi_t)
+        {
+            if (returnNullptr)
+            {
+                return { nullptr };
+            }
+            else if (shouldThrowOnTryLookup)
+            { 
+                throw_hresult(codeToThrow); 
+            }
+            else
+            {
+                return { std::nullopt };
+            }
+        }
+        IInspectable Lookup(int) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
+        int32_t Size() { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
+        bool HasKey(int) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
+        void Split(IMapView<int, IInspectable>&, IMapView<int, IInspectable>&) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
+    };
+
+    auto self = make_self<map_with_try_lookup>();
+    IMapView<int, IInspectable> map = *self;
+
+    // Ensure that we return a value on nullptr, a nullptr is a valid IInspectable in the Map
+    self->returnNullptr = true;
+    REQUIRE(map.TryLookup(123) == IInspectable{nullptr});
+    REQUIRE(map.Lookup(123) == IInspectable{nullptr});
+
+    // Make sure that we use the TryLookup specialization, and don't throw an unexpected exception.
+    self->shouldThrowOnTryLookup = false;
+    self->returnNullptr = false;
+    REQUIRE(map.TryLookup(123) == IInspectable{nullptr});
+    // make sure regular lookup stll throws bounds
+    REQUIRE_THROWS_AS(map.Lookup(123), hresult_out_of_bounds);
+
+    // 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.
+    self->shouldThrowOnTryLookup = true;
+    self->codeToThrow = RPC_E_WRONG_THREAD;
+    REQUIRE_THROWS_AS(map.TryLookup(123), hresult_wrong_thread);
+    // regular lookup should throw the same error
+    REQUIRE_THROWS_AS(map.Lookup(123), hresult_wrong_thread);
+}