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

* non-originating error compiles

* fix with test cases

* and remove ignore

* add runsettings to run tests from visual studio tester, modified readme, make shouldOrigiante a template param.

* change map shouldOriginate to template parameter. Note, could be a breakig change for usages of map that are not default (i.e. custom std::less)
if so, might need to split up templates into separate template, or spin out into separate pr.

changed bool to template parameter in hresult_error, as the char*-to-bool decaying conversion failed the old tests.

also fixed runsettings, added note in readme.

* settle on avoid_originate as naming scheme

* add a printf only to Lookup so we can add SFINAE or something like that.

* weird templating magic required... still not working, has error
1>G:\source\repos\cppwinrt\_build\x64\Debug\winrt\Windows.Foundation.Collections.h(868,51): error C3878: syntax error: unexpected token '>' following 'simple-type-specifier'
1>(compiling source file '/Class.cpp')
1>    G:\source\repos\cppwinrt\_build\x64\Debug\winrt\Windows.Foundation.Collections.h(868,51):
1>    missing one of: '(' '{' ?

* trylookup exists to avoid throwing an error, and let's avoid avoid_originate now.

* remove test.

* cleaned up and correctness.

* undo changes to untouched files

* spacing changes , change do declval.

* added special tag to parameter list to make sure not to break existing TryLookup impls, added test to verify this.

* address avoid_oritinate comment, no need for a different name, resuse existing hresult_cancelled

* change out async to a new name, with it being true by default.
add new unittest to test nullable lookup.

* Address hresult comments by adding tests and removing source_locatoin, special-cased further by checking the typename.

* add unittest without specialization, make sure that the behaviour is opt-in

---------

Co-authored-by: Chris Guzak <[email protected]>

Author: antmor

.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,7 +1873,37 @@ namespace cppwinrt
         }
     }
 
-    static void write_produce_method(writer& w, MethodDef const& method)
+    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, TypeDef const& type)
     {
         std::string_view format;
 
@@ -1902,13 +1932,45 @@ 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);
+        auto name = get_name(method);
+        upcall += name;
 
-        w.write(format,
-            get_abi_name(method),
-            bind<write_produce_params>(signature),
-            bind<write_produce_cleanup>(signature),
-            bind<write_produce_upcall>(upcall, signature));
+        auto typeName = type.TypeName();
+        if (((typeName == "IMapView`2") || (typeName == "IMap`2"))
+            && (name == "Lookup"))
+        {
+            // Special-case IMap*::Lookup to look for a TryLookup here, to avoid extranous throw/originates
+            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)
@@ -1951,7 +2013,7 @@ namespace cppwinrt
                 break;
             }
 
-            w.write_each<write_produce_method>(info.type.MethodList());
+            w.write_each<write_produce_method>(info.type.MethodList(), info.type);
         }
     }
 
@@ -1973,7 +2035,7 @@ namespace cppwinrt
             bind<write_comma_generic_typenames>(generics),
             type,
             type,
-            bind_each<write_produce_method>(type.MethodList()),
+            bind_each<write_produce_method>(type.MethodList(), type),
             bind<write_fast_produce_methods>(type));
     }
 

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) 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,16 @@ 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.