Improved diagnostics for detecting direct allocations in C++/WinRT (m…

…icrosoft#391)

src/tool/cppwinrt/strings/base_implements.h

@@ -810,7 +810,6 @@ namespace winrt::impl
 
         void abi_enter() const noexcept {}
         void abi_exit() const noexcept {}
-        static void final_release(std::unique_ptr<D>) noexcept {}
 
         int32_t query_interface_tearoff(guid const&, void**) const noexcept
         {
@@ -905,7 +904,14 @@ namespace winrt::impl
                 // This ensures destruction has a stable value during destruction.
                 m_references = 1;
 
-                D::final_release(std::unique_ptr<D>(static_cast<D*>(this)));
+                if constexpr (has_final_release::value)
+                {
+                    D::final_release(std::unique_ptr<D>(static_cast<D*>(this)));
+                }
+                else
+                {
+                    delete this;
+                }
             }
 
             return target;
@@ -1036,6 +1042,16 @@ namespace winrt::impl
 
     private:
 
+        class has_final_release
+        {
+            template <typename U, typename = decltype(std::declval<U>().final_release(0))> 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);
+        };
+
         using is_agile = std::negation<std::disjunction<std::is_same<non_agile, I>...>>;
         using is_inspectable = std::disjunction<std::is_base_of<Windows::Foundation::IInspectable, I>...>;
         using is_weak_ref_source = std::conjunction<is_inspectable, std::negation<is_factory>, std::negation<std::disjunction<std::is_same<no_weak_ref, I>...>>>;
@@ -1174,17 +1190,22 @@ namespace winrt::impl
         friend struct impl::produce;
     };
 
+#if defined(WINRT_NO_MAKE_DETECTION)
+    template <typename T>
+    using heap_implements = T;
+#else
     template <typename T>
     struct heap_implements final : T
     {
         using T::T;
 
-#if defined(_DEBUG) && !defined(WINRT_NO_MAKE_DETECTION)
+#if defined(_DEBUG)
         void use_make_function_to_create_this_object() final
         {
         }
 #endif
     };
+#endif
 
     template <typename D>
     auto make_factory() -> typename impl::implements_default_interface<D>::type
@@ -1234,6 +1255,12 @@ namespace winrt
     template <typename D, typename... Args>
     auto make(Args&&... args)
     {
+#if !defined(WINRT_NO_MAKE_DETECTION)
+        // Note: https://aka.ms/cppwinrt/detect_direct_allocations
+        static_assert(std::is_destructible_v<D>, "C++/WinRT implementation types must have a public destructor");
+        static_assert(!std::is_final_v<D>, "C++/WinRT implementation types must not be final");
+#endif
+
         using I = typename impl::implements_default_interface<D>::type;
 
         if constexpr (std::is_same_v<I, Windows::Foundation::IActivationFactory>)
@@ -1260,6 +1287,12 @@ namespace winrt
     template <typename D, typename... Args>
     com_ptr<D> make_self(Args&&... args)
     {
+#if !defined(WINRT_NO_MAKE_DETECTION)
+        // Note: https://aka.ms/cppwinrt/detect_direct_allocations
+        static_assert(std::is_destructible_v<D>, "C++/WinRT implementation types must have a public destructor");
+        static_assert(!std::is_final_v<D>, "C++/WinRT implementation types must not be final");
+#endif
+
         return { new impl::heap_implements<D>(std::forward<Args>(args)...), take_ownership_from_abi };
     }
 

src/tool/cppwinrt/test/no_make_detection.cpp

@@ -0,0 +1,48 @@
+// This test validates that defining WINRT_NO_MAKE_DETECTION actually
+// allows an implementation to be final and have a private destructor.
+// This is *not* recommended as there are no safeguards for direct and
+// invalid allocations, but is provided for compatibility.
+
+#define WINRT_NO_MAKE_DETECTION
+#include "catch.hpp"
+#include "winrt/Windows.Foundation.h"
+
+using namespace winrt;
+using namespace Windows::Foundation;
+
+namespace
+{
+    struct Stringable final : implements<Stringable, IStringable>
+    {
+        hstring ToString()
+        {
+            return L"Stringable";
+        }
+
+        inline static bool Destroyed{};
+
+    private:
+
+        ~Stringable()
+        {
+            Destroyed = true;
+        }
+    };
+}
+
+TEST_CASE("no_make_detection")
+{
+    {
+        IStringable stringable{ (new Stringable())->get_abi<IStringable>(), take_ownership_from_abi };
+        REQUIRE(!Stringable::Destroyed);
+        stringable = nullptr;
+        REQUIRE(Stringable::Destroyed);
+    }
+    {
+        Stringable::Destroyed = false;
+        IStringable stringable = make<Stringable>();
+        REQUIRE(!Stringable::Destroyed);
+        stringable = nullptr;
+        REQUIRE(Stringable::Destroyed);
+    }
+}

src/tool/cppwinrt/test/test.vcxproj

@@ -253,6 +253,12 @@
     </ClCompile>
     <ClCompile Include="names.cpp" />
     <ClCompile Include="noexcept.cpp" />
+    <ClCompile Include="no_make_detection.cpp">
+      <PrecompiledHeader Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">NotUsing</PrecompiledHeader>
+      <PrecompiledHeader Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">NotUsing</PrecompiledHeader>
+      <PrecompiledHeader Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">NotUsing</PrecompiledHeader>
+      <PrecompiledHeader Condition="'$(Configuration)|$(Platform)'=='Release|x64'">NotUsing</PrecompiledHeader>
+    </ClCompile>
     <ClCompile Include="out_params.cpp" />
     <ClCompile Include="out_params_abi.cpp" />
     <ClCompile Include="out_params_bad.cpp" />