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

.runsettings

@@ -0,0 +1,25 @@
+<?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>
+    <CombinedTimeout>30000</CombinedTimeout><!-- 30s in Milliseconds -->
+    <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 = %(%);",
+            upcall,
+            bind<write_produce_args>(method_signature));
+        w.write(R"(
+            if (out_param_val) 
+            {
+                *% = 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,43 @@ 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 (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_try_lookup_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) 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>
@@ -561,7 +576,7 @@ WINRT_EXPORT namespace winrt
 
             return !added;
         }
-
+        // todo also create a tryremove?
         void Remove(K const& key)
         {
             typename impl::container_type_t<D>::node_type removedNode;

strings/base_coroutine_foundation.h

@@ -351,6 +351,10 @@ namespace winrt::impl
             return m_promise->enable_cancellation_propagation(value);
         }
 
+        bool avoid_cancel_origination(bool value = true) const noexcept
+        {
+            return m_promise->enable_avoid_cancel_origination(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::avoid_cancel_origination_enabled())
+                    {
+                        m_exception = std::make_exception_ptr(non_originating_hresult_canceled());
+                    }
+                    else
+                    {
+                        m_exception = std::make_exception_ptr(hresult_canceled());
+                    }
                     cancel = std::move(m_cancel);
                 }
             }
@@ -617,6 +628,10 @@ namespace winrt::impl
             {
                 m_status.store(AsyncStatus::Canceled, std::memory_order_relaxed);
             }
+            catch (non_originating_hresult_canceled const&)
+            {
+                m_status.store(AsyncStatus::Canceled, std::memory_order_relaxed);
+            }
             catch (...)
             {
                 m_status.store(AsyncStatus::Error, std::memory_order_relaxed);
@@ -628,7 +643,14 @@ namespace winrt::impl
         {
             if (Status() == AsyncStatus::Canceled)
             {
-                throw winrt::hresult_canceled();
+                if (cancellable_promise::avoid_cancel_origination_enabled())
+                {
+                    throw winrt::non_originating_hresult_canceled();
+                }
+                else
+                {
+                    throw winrt::hresult_canceled();
+                }
             }
 
             return std::forward<Expression>(expression);

strings/base_coroutine_threadpool.h

@@ -180,12 +180,23 @@ WINRT_EXPORT namespace winrt
             return m_propagate_cancellation;
         }
 
+        bool enable_avoid_cancel_origination(bool value) noexcept
+        {
+            return std::exchange(m_avoid_cancel_origination, value);
+        }
+
+        bool avoid_cancel_origination_enabled() const noexcept
+        {
+            return m_avoid_cancel_origination;
+        }
+
     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_avoid_cancel_origination{ false };
     };
 
     template <typename Derived>

strings/base_error.h

@@ -84,6 +84,9 @@ WINRT_EXPORT namespace winrt
 {
     struct hresult_error
     {
+        struct avoid_originate_t {};
+        static constexpr avoid_originate_t avoid_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, avoid_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);
@@ -329,6 +336,14 @@ WINRT_EXPORT namespace winrt
         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) {}
     };
 
+    struct non_originating_hresult_canceled : hresult_error
+    {
+        non_originating_hresult_canceled(winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, hresult_error::avoid_originate, sourceInformation) {}
+        non_originating_hresult_canceled(param::hstring const& message, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) = delete;
+        non_originating_hresult_canceled(take_ownership_from_abi_t , winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) = delete;
+
+    };
+
     [[noreturn]] inline WINRT_IMPL_NOINLINE void throw_hresult(hresult const result, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current())
     {
         if (winrt_throw_hresult_handler)

strings/base_meta.h

@@ -298,4 +298,18 @@ namespace winrt::impl
             return (func(Types{}) || ...);
         }
     };
+
+    template <typename D, typename K>
+    struct has_try_lookup
+    {
+        template <typename U, typename = decltype(std::declval<U>().TryLookup(K{}))> 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_try_lookup_v = has_try_lookup<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.