From ac5f4d7ba0ca378e6ce0c5e73cb71fbef623cbcc Mon Sep 17 00:00:00 2001
From: Raymond Chen <oldnewthing@users.noreply.github.com>
Date: Thu, 16 Sep 2021 13:10:16 -0700
Subject: [PATCH] Improve diagnostics when trying to use classic COM without
 <unknwn.h>

Before this change, if you tried to use classic COM without
first including <unknwn.h>, C++/WinRT spit out really confusing
error messages like

```cpp
auto widget = object.as<::IClassicComInterface>();
```

> error: 'unbox_value_type': no matching overloaded function found

or worse: Just ignored you!

```cpp
struct Widget : WidgetT<Widget, ::IPersistStream>
{
    // IPersistStream is simply ignored!
};
```

This change relaxes the rules around `<unknwn.h>` and improves
diagnostics.

1. If you want to use `com_ptr<I>`, `as<I>`, etc.
with classic COM interfaces, you need only include `<unknwn.h>`
at *some point* before using them. Doesn't have to be included
before `winrt/base.h`. (And really, since you need to include
`<unknwn.h>` in order to define *any* classic COM interfaces,
the prerquisite is automatically satisfied in practice.)

2. If you want to use `implements<...>` with classic COM interfaces,
you stlil need to include `<unknwn.h>` before `winrt/base.h`,
but we improved the diagnostics so that if you forget, you get
a compiler error instead of a runtime error:

> To implement classic COM interfaces, you must `#include <unknwn.h>` before including C++/WinRT headers.

Implementation notes
====================

The key step is forward-declaring the missing types
`IUnknown` and `GUID` so that we can talk *about* them
without needing to know what they are. This trick allows us to
remove a large number of `#ifdef WINRT_IUNKNOWN_DEFINED`
tests, which unlocks 80% of this feature.

To avoid repetition, introduced a new type trait
`is_classic_com_interface<T>`, which factors out many
uses of `is_base_of<::IUnknown, T> && !is_implements<T>`.

Having a forward declaration of `IUnknown` lets us detect
that a type is a classic COM interface by seeing if it
derives from `::IUnknown`.

Having a forward declaration of `GUID` lets us reinterpret-cast
them to `guid`, which covers nearly everything.

The only place where we really need to know what's in a `GUID`
is the `constexpr guid(GUID const&)` constructor. For that,
the constructor is templated with a dummy default parameter.
The dummy default parameter delays expansion until the
constructor is invoked. We then forward the `GUID` to
a templated helper as a dependent type `T`, so the compiler
accepts the helper despite not knowing what T's members are.

This change found a bug in the unit tests, because Nested.HierarchyD used
classic COM interface `IReferenceTrackerExtension` without having
first included `unknwn.h`, so it was ignored!

(Amusing note: The implementation of this feature got shorter
the longer I worked on it, as I gradually realized that nearly all
of the places we needed to protect the use of `GUID` didn't need
the duck-typing trick employed in the `guid(GUID const&)` constructor.)
---
 strings/base_implements.h         | 22 ++++++----------------
 strings/base_macros.h             |  4 ++++
 strings/base_meta.h               |  8 +++++---
 strings/base_reference_produce.h  | 10 +---------
 strings/base_types.h              | 23 +++++++++++------------
 strings/base_windows.h            | 14 ++++----------
 test/test/guid.cpp                | 29 ++++++++++++++++++-----------
 test/test_component_derived/pch.h |  1 +
 8 files changed, 50 insertions(+), 61 deletions(-)

diff --git a/strings/base_implements.h b/strings/base_implements.h
index 7bc85220b..b2300edc5 100644
--- a/strings/base_implements.h
+++ b/strings/base_implements.h
@@ -44,17 +44,8 @@ namespace winrt::impl
     template <template <typename> typename Condition, typename T>
     using tuple_if = typename tuple_if_base<Condition, T>::type;
 
-#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
-
     template <typename T>
-    struct is_interface : std::disjunction<std::is_base_of<Windows::Foundation::IInspectable, T>, std::conjunction<std::is_base_of<::IUnknown, T>, std::negation<is_implements<T>>>> {};
-
-#else
-
-    template <typename T>
-    struct is_interface : std::is_base_of<Windows::Foundation::IInspectable, T> {};
-
-#endif
+    struct is_interface : std::disjunction<std::is_base_of<Windows::Foundation::IInspectable, T>, is_classic_com_interface<T>> {};
 
     template <typename T>
     struct is_marker : std::disjunction<std::is_base_of<marker, T>, std::is_void<T>> {};
@@ -485,20 +476,19 @@ namespace winrt::impl
         }
     };
 
-#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
-
     template <typename D, typename I>
-    struct producer<D, I, std::enable_if_t<std::is_base_of_v< ::IUnknown, I> && !is_implements_v<I>>> : I
+    struct producer<D, I, std::enable_if_t<is_classic_com_interface<I>::value>> : I
     {
+#ifndef WINRT_IMPL_IUNKNOWN_DEFINED
+        static_assert(std::is_void_v<I> /* dependent_false */, "To implement classic COM interfaces, you must #include <unknwn.h> before including C++/WinRT headers.");
+#endif
     };
 
     template <typename D, typename I>
-    struct producer_convert<D, I, std::enable_if_t<std::is_base_of_v< ::IUnknown, I> && !is_implements_v<I>>> : producer<D, I>
+    struct producer_convert<D, I, std::enable_if_t<is_classic_com_interface<I>::value>> : producer<D, I>
     {
     };
 
-#endif
-
     struct INonDelegatingInspectable : Windows::Foundation::IUnknown
     {
         INonDelegatingInspectable(std::nullptr_t = nullptr) noexcept {}
diff --git a/strings/base_macros.h b/strings/base_macros.h
index 92f746699..1b96e240c 100644
--- a/strings/base_macros.h
+++ b/strings/base_macros.h
@@ -55,4 +55,8 @@
 
 #ifdef __IUnknown_INTERFACE_DEFINED__
 #define WINRT_IMPL_IUNKNOWN_DEFINED
+#else
+// Forward declare so we can talk about it.
+struct IUnknown;
+typedef struct _GUID GUID;
 #endif
diff --git a/strings/base_meta.h b/strings/base_meta.h
index 9c18a84f0..f951c4db0 100644
--- a/strings/base_meta.h
+++ b/strings/base_meta.h
@@ -118,12 +118,14 @@ namespace winrt::impl
     };
 
     template <typename T>
-#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
-#ifdef __clang__
+#if defined(__clang__)
+#if __has_declspec_attribute(uuid)
     inline const guid guid_v{ __uuidof(T) };
 #else
-    inline constexpr guid guid_v{ __uuidof(T) };
+    inline constexpr guid guid_v{};
 #endif
+#elif defined(_MSC_VER)
+    inline constexpr guid guid_v{ __uuidof(T) };
 #else
     inline constexpr guid guid_v{};
 #endif
diff --git a/strings/base_reference_produce.h b/strings/base_reference_produce.h
index 8228cbc4c..8ea2de5b9 100644
--- a/strings/base_reference_produce.h
+++ b/strings/base_reference_produce.h
@@ -212,14 +212,12 @@ namespace winrt::impl
         using itf = Windows::Foundation::IReference<guid>;
     };
 
-#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
     template <>
     struct reference_traits<GUID>
     {
-        static auto make(GUID const& value) { return Windows::Foundation::PropertyValue::CreateGuid(value); }
+        static auto make(GUID const& value) { return Windows::Foundation::PropertyValue::CreateGuid(reinterpret_cast<guid const&>(value)); }
         using itf = Windows::Foundation::IReference<guid>;
     };
-#endif
 
     template <>
     struct reference_traits<Windows::Foundation::DateTime>
@@ -354,14 +352,12 @@ namespace winrt::impl
         using itf = Windows::Foundation::IReferenceArray<guid>;
     };
 
-#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
     template <>
     struct reference_traits<com_array<GUID>>
     {
         static auto make(array_view<GUID const> const& value) { return Windows::Foundation::PropertyValue::CreateGuidArray(reinterpret_cast<array_view<guid const> const&>(value)); }
         using itf = Windows::Foundation::IReferenceArray<guid>;
     };
-#endif
 
     template <>
     struct reference_traits<com_array<Windows::Foundation::DateTime>>
@@ -444,14 +440,12 @@ namespace winrt::impl
                 return static_cast<T>(value.template as<Windows::Foundation::IReference<std::underlying_type_t<T>>>().Value());
             }
         }
-#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
         else if constexpr (std::is_same_v<T, com_array<GUID>>)
         {
             T result;
             reinterpret_cast<com_array<guid>&>(result) = value.template as<typename impl::reference_traits<T>::itf>().Value();
             return result;
         }
-#endif
         else
         {
             return value.template as<typename impl::reference_traits<T>::itf>().Value();
@@ -473,7 +467,6 @@ namespace winrt::impl
                 return static_cast<T>(temp.Value());
             }
         }
-#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
         else if constexpr (std::is_same_v<T, com_array<GUID>>)
         {
             if (auto temp = value.template try_as<typename impl::reference_traits<T>::itf>())
@@ -483,7 +476,6 @@ namespace winrt::impl
                 return result;
             }
         }
-#endif
         else
         {
             if (auto temp = value.template try_as<typename impl::reference_traits<T>::itf>())
diff --git a/strings/base_types.h b/strings/base_types.h
index 7dbb32165..afff3db35 100644
--- a/strings/base_types.h
+++ b/strings/base_types.h
@@ -141,24 +141,14 @@ WINRT_EXPORT namespace winrt
         {
         }
 
-#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
-
-        constexpr guid(GUID const& value) noexcept :
-            Data1(value.Data1),
-            Data2(value.Data2),
-            Data3(value.Data3),
-            Data4{ value.Data4[0], value.Data4[1], value.Data4[2], value.Data4[3], value.Data4[4], value.Data4[5], value.Data4[6], value.Data4[7] }
-        {
-
-        }
+        template<bool dummy = true>
+        constexpr guid(GUID const& value) noexcept : guid(convert<dummy>(value)) { }
 
         operator GUID const&() const noexcept
         {
             return reinterpret_cast<GUID const&>(*this);
         }
 
-#endif
-
         constexpr explicit guid(std::string_view const value) :
             guid(parse(value))
         {
@@ -168,6 +158,15 @@ WINRT_EXPORT namespace winrt
             guid(parse(value))
         {
         }
+
+    private:
+        template<bool, typename T>
+        constexpr static guid convert(T const& value) noexcept
+        {
+            return { value.Data1, value.Data2, value.Data3,
+                { value.Data4[0], value.Data4[1], value.Data4[2], value.Data4[3], value.Data4[4], value.Data4[5], value.Data4[6], value.Data4[7] }
+            };
+        }
     };
 
     inline bool operator==(guid const& left, guid const& right) noexcept
diff --git a/strings/base_windows.h b/strings/base_windows.h
index 3800af1b1..41030d368 100644
--- a/strings/base_windows.h
+++ b/strings/base_windows.h
@@ -81,13 +81,11 @@ namespace winrt::impl
         return { result, take_ownership_from_abi };
     }
 
-#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
-    template <typename T>
-    struct is_com_interface : std::disjunction<std::is_base_of<Windows::Foundation::IUnknown, T>, std::is_base_of<unknown_abi, T>, is_implements<T>, std::is_base_of<::IUnknown, T>> {};
-#else
+    template<typename T>
+    struct is_classic_com_interface : std::conjunction<std::is_base_of<::IUnknown, T>, std::negation<is_implements<T>>> {};
+
     template <typename T>
-    struct is_com_interface : std::disjunction<std::is_base_of<Windows::Foundation::IUnknown, T>, std::is_base_of<unknown_abi, T>, is_implements<T>> {};
-#endif
+    struct is_com_interface : std::disjunction<std::is_base_of<Windows::Foundation::IUnknown, T>, std::is_base_of<unknown_abi, T>, is_implements<T>, is_classic_com_interface<T>> {};
 
     template <typename T>
     inline constexpr bool is_com_interface_v = is_com_interface<T>::value;
@@ -363,14 +361,10 @@ WINRT_EXPORT namespace winrt
         }
     }
 
-#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
-
     inline ::IUnknown* get_unknown(Windows::Foundation::IUnknown const& object) noexcept
     {
         return static_cast<::IUnknown*>(get_abi(object));
     }
-
-#endif
 }
 
 WINRT_EXPORT namespace winrt::Windows::Foundation
diff --git a/test/test/guid.cpp b/test/test/guid.cpp
index b3edbc570..3dff63088 100644
--- a/test/test/guid.cpp
+++ b/test/test/guid.cpp
@@ -1,20 +1,23 @@
 #include "pch.h"
 
+#define STATIC_REQUIRE_GUID_EQUAL(left, right) \
+    STATIC_REQUIRE(left.Data1 == right.Data1); \
+    STATIC_REQUIRE(left.Data2 == right.Data2); \
+    STATIC_REQUIRE(left.Data3 == right.Data3); \
+    STATIC_REQUIRE(left.Data4[0] == right.Data4[0]); \
+    STATIC_REQUIRE(left.Data4[1] == right.Data4[1]); \
+    STATIC_REQUIRE(left.Data4[2] == right.Data4[2]); \
+    STATIC_REQUIRE(left.Data4[3] == right.Data4[3]); \
+    STATIC_REQUIRE(left.Data4[4] == right.Data4[4]); \
+    STATIC_REQUIRE(left.Data4[5] == right.Data4[5]); \
+    STATIC_REQUIRE(left.Data4[6] == right.Data4[6]); \
+    STATIC_REQUIRE(left.Data4[7] == right.Data4[7]);
+
 TEST_CASE("guid")
 {
     constexpr winrt::guid expected{ 0x00112233, 0x4455, 0x6677, { 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff } };
 
-    STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data1 == expected.Data1);
-    STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data2 == expected.Data2);
-    STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data3 == expected.Data3);
-    STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[0] == expected.Data4[0]);
-    STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[1] == expected.Data4[1]);
-    STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[2] == expected.Data4[2]);
-    STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[3] == expected.Data4[3]);
-    STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[4] == expected.Data4[4]);
-    STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[5] == expected.Data4[5]);
-    STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[6] == expected.Data4[6]);
-    STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[7] == expected.Data4[7]);
+    STATIC_REQUIRE_GUID_EQUAL(winrt::guid("00112233-4455-6677-8899-aabbccddeeff"), expected);
 
     REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff") == expected);
     REQUIRE(winrt::guid({ "{00112233-4455-6677-8899-aabbccddeeff}" + 1, 36 }) == expected);
@@ -26,4 +29,8 @@ TEST_CASE("guid")
     REQUIRE_THROWS_AS(winrt::guid("00112233-4455-6677-8899-aabbccddeeff with extra"), std::invalid_argument);
     REQUIRE_THROWS_AS(winrt::guid("xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"), std::invalid_argument);
     REQUIRE_THROWS_AS(winrt::guid("{00112233-4455-6677-8899-aabbccddeeff}"), std::invalid_argument);
+
+    // Verify that you can constexpr-construct a guid from a GUID.
+    constexpr winrt::guid from_abi_guid = GUID{ 0x00112233, 0x4455, 0x6677, { 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff } };
+    STATIC_REQUIRE_GUID_EQUAL(from_abi_guid, expected);
 }
\ No newline at end of file
diff --git a/test/test_component_derived/pch.h b/test/test_component_derived/pch.h
index 6f70f09be..6365abede 100644
--- a/test/test_component_derived/pch.h
+++ b/test/test_component_derived/pch.h
@@ -1 +1,2 @@
 #pragma once
+#include <unknwn.h> // Nested.HierarchyD uses classic COM interface IReferenceTrackerExtension
\ No newline at end of file