Hide protected and overridable members from public projections (#1319)

README.md

@@ -15,6 +15,16 @@ If you really want to build it yourself, the simplest way to do so is to run the
 
 * Open a dev command prompt pointing at the root of the repo.
 * Open the `cppwinrt.sln` solution.
-* Build the x64 Release configuration of the `prebuild` and `cppwinrt` projects only. Do not attempt to build anything else just yet.
+* Build the x64 Release configuration of the `cppwinrt` project only. Do not attempt to build anything else just yet.
 * Run `build_projection.cmd` in the dev command prompt.
 * Switch to the x64 Debug configuration in Visual Studio and build all projects as needed.
+
+## Comparing Outputs
+
+Comparing the output of the prior release and your current changes will help show the impact of any updates. Starting from
+a dev command prompt at the root of the repo _after_ following the above build instructions:
+
+* Run `build_projection.cmd` in the dev command prompt
+* 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`

build_prior_projection.cmd

@@ -0,0 +1,50 @@
+@echo off
+
+setlocal ENABLEDELAYEDEXPANSION
+
+set target_platform=%1
+set target_configuration=%2
+if "%target_platform%"=="" set target_platform=x64
+
+if /I "%target_platform%" equ "all" (
+  if "%target_configuration%"=="" (
+    set target_configuration=all
+  )
+  call %0 x86 !target_configuration!
+  call %0 x64 !target_configuration!
+  call %0 arm !target_configuration!
+  call %0 arm64 !target_configuration!
+  goto :eof
+)
+
+if /I "%target_configuration%" equ "all" (
+  call %0 %target_platform% Debug
+  call %0 %target_platform% Release
+  goto :eof
+)
+
+if "%target_configuration%"=="" (
+ set target_configuration=Debug
+)
+
+set reference_output=%~p0\_reference\%target_platform%\%target_configuration%
+if exist "%reference_output%" (
+ echo Removing existing reference projections
+ rmdir /s /q "%reference_output%"
+)
+
+if not exist ".\.nuget" mkdir ".\.nuget"
+if not exist ".\.nuget\nuget.exe" powershell -Command "$ProgressPreference = 'SilentlyContinue' ; Invoke-WebRequest https://dist.nuget.org/win-x86-commandline/latest/nuget.exe -OutFile .\.nuget\nuget.exe"
+
+mkdir %reference_output%\package
+.\.nuget\nuget.exe install Microsoft.Windows.CppWinRT -o %reference_output%\package
+set reference_cppwinrt=
+for /F "delims=" %%a in ('dir /s /b %reference_output%\package\cppwinrt.exe') DO set reference_cppwinrt=%%a
+if "%reference_cppwinrt%"=="" (
+ echo Could not find the reference cppwinrt.exe under %reference_output%\package
+ goto :EOF
+)
+
+echo Generating reference projection from %reference_cppwinrt% to %reference_output%\cppwinrt
+%reference_cppwinrt% -in local -out %reference_output% -verbose
+echo.

build_projection.cmd

@@ -27,6 +27,13 @@ if "%target_configuration%"=="" (
  set target_configuration=Debug
 )
 
+set cppwinrt_exe=%~p0\_build\x64\Release\cppwinrt.exe
+
+if not exist "%cppwinrt_exe%" (
+ echo Remember to build the "prebuild" and then "cppwinrt" projects for Release x64 first
+ goto :eof
+)
+
 echo Building projection into %target_platform% %target_configuration%
-%~p0\_build\x64\Release\cppwinrt.exe -in local -out %~p0\_build\%target_platform%\%target_configuration% -verbose
+%cppwinrt_exe% -in local -out %~p0\_build\%target_platform%\%target_configuration% -verbose
 echo.

build_test_all.cmd

@@ -10,7 +10,7 @@ if "%target_configuration%"=="" set target_configuration=Release
 if "%target_version%"=="" set target_version=1.2.3.4
 
 if not exist ".\.nuget" mkdir ".\.nuget"
-if not exist ".\.nuget\nuget.exe" powershell -Command "Invoke-WebRequest https://dist.nuget.org/win-x86-commandline/latest/nuget.exe -OutFile .\.nuget\nuget.exe"
+if not exist ".\.nuget\nuget.exe" powershell -Command "$ProgressPreference = 'SilentlyContinue' ; Invoke-WebRequest https://dist.nuget.org/win-x86-commandline/latest/nuget.exe -OutFile .\.nuget\nuget.exe"
 
 call .nuget\nuget.exe restore cppwinrt.sln"
 call .nuget\nuget.exe restore natvis\cppwinrtvisualizer.sln

cppwinrt/code_writers.h

@@ -2035,13 +2035,39 @@ struct WINRT_IMPL_EMPTY_BASES produce_dispatch_to_overridable<T, D, %>
     {
         for (auto&& [name, info] : interfaces)
         {
-            if (!info.overridable)
+            if (!info.overridable && !info.is_protected)
             {
                 w.write(", %", name);
             }
         }
     }
 
+    static void write_class_override_protected_requires(writer& w, get_interfaces_t const& interfaces)
+    {
+        bool first = true;
+
+        for (auto&& [name, info] : interfaces)
+        {
+            if (info.is_protected)
+            {
+                if (first)
+                {
+                    first = false;
+                    w.write(",\n        protected impl::require<D, %", name);
+                }
+                else
+                {
+                    w.write(", %", name);
+                }
+            }
+        }
+
+        if (!first)
+        {
+            w.write('>');
+        }
+    }
+
     static void write_class_override_defaults(writer& w, get_interfaces_t const& interfaces)
     {
         bool first = true;
@@ -2073,6 +2099,18 @@ struct WINRT_IMPL_EMPTY_BASES produce_dispatch_to_overridable<T, D, %>
         }
     }
 
+    static void write_class_override_friends(writer& w, get_interfaces_t const& interfaces)
+    {
+        for (auto&& [name, info] : interfaces)
+        {
+            if (info.is_protected)
+            {
+                w.write("\n        friend impl::consume_t<D, %>;", name);
+                w.write("\n        friend impl::require_one<D, %>;", name);
+            }
+        }
+    }
+
     static void write_call_factory(writer& w, TypeDef const& type, TypeDef const& factory)
     {
         std::string factory_name;
@@ -2243,10 +2281,10 @@ struct WINRT_IMPL_EMPTY_BASES produce_dispatch_to_overridable<T, D, %>
         auto format = R"(    template <typename D, typename... Interfaces>
     struct %T :
         implements<D%, composing, Interfaces...>,
-        impl::require<D%>,
+        impl::require<D%>%,
         impl::base<D, %%>%
     {
-        using composable = %;
+        using composable = %;%
     protected:
 %%    };
 )";
@@ -2258,10 +2296,12 @@ struct WINRT_IMPL_EMPTY_BASES produce_dispatch_to_overridable<T, D, %>
             type_name,
             bind<write_class_override_implements>(interfaces),
             bind<write_class_override_requires>(interfaces),
+            bind<write_class_override_protected_requires>(interfaces),
             type_name,
             bind<write_class_override_bases>(type),
             bind<write_class_override_defaults>(interfaces),
             type_name,
+            bind<write_class_override_friends>(interfaces),
             bind<write_class_override_constructors>(type, factories),
             bind<write_class_override_usings>(interfaces));
     }
@@ -2326,18 +2366,21 @@ struct WINRT_IMPL_EMPTY_BASES produce_dispatch_to_overridable<T, D, %>
 
         for (auto&& [interface_name, info] : get_interfaces(w, type))
         {
-            if (info.defaulted && !info.base)
+            if (!info.is_protected && !info.overridable)
             {
-                for (auto&& method : info.type.MethodList())
+                if (info.defaulted && !info.base)
                 {
-                    method_usage[get_name(method)].insert(default_interface_name);
+                    for (auto&& method : info.type.MethodList())
+                    {
+                        method_usage[get_name(method)].insert(default_interface_name);
+                    }
                 }
-            }
-            else
-            {
-                for (auto&& method : info.type.MethodList())
+                else
                 {
-                    method_usage[get_name(method)].insert(interface_name);
+                    for (auto&& method : info.type.MethodList())
+                    {
+                        method_usage[get_name(method)].insert(interface_name);
+                    }
                 }
             }
         }
@@ -2804,7 +2847,7 @@ struct WINRT_IMPL_EMPTY_BASES produce_dispatch_to_overridable<T, D, %>
 
         for (auto&& [interface_name, info] : get_interfaces(w, type))
         {
-            if (!info.defaulted || info.base)
+            if ((!info.defaulted || info.base) && (!info.is_protected && !info.overridable))
             {
                 if (first)
                 {

cppwinrt/component_writers.h

@@ -743,13 +743,13 @@ catch (...) { return winrt::to_hresult(); }
             auto format = R"(namespace winrt::@::implementation
 {
     template <typename D%, typename... I>
-    struct WINRT_IMPL_EMPTY_BASES %_base : implements<D, @::%%%, %I...>%%%
+    struct WINRT_IMPL_EMPTY_BASES %_base : implements<D, @::%%%, %I...>%%%%
     {
         using base_type = %_base;
         using class_type = @::%;
         using implements_type = typename %_base::implements_type;
         using implements_type::implements_type;
-        %
+        %%
         hstring GetRuntimeClassName() const
         {
             return L"%.%";
@@ -764,6 +764,8 @@ catch (...) { return winrt::to_hresult(); }
             std::string base_type_argument;
             std::string no_module_lock;
             std::string external_requires;
+            std::string external_protected_requires;
+            std::string friends;
 
             if (base_type)
             {
@@ -774,7 +776,9 @@ catch (...) { return winrt::to_hresult(); }
                     composable_base_name = w.write_temp("using composable_base = %;", base_type);
                     auto base_interfaces = get_interfaces(w, base_type);
                     uint32_t base_interfaces_count{};
+                    uint32_t protected_base_interfaces_count{};
                     external_requires = ",\n        impl::require<D";
+                    external_protected_requires = ",\n        protected impl::require<D";
 
                     for (auto&&[name, info] : base_interfaces)
                     {
@@ -783,9 +787,25 @@ catch (...) { return winrt::to_hresult(); }
                             continue;
                         }
 
-                        ++base_interfaces_count;
-                        external_requires += ", ";
-                        external_requires += name;
+                        if (info.is_protected || info.overridable)
+                        {
+                            ++protected_base_interfaces_count;
+                            external_protected_requires += ", ";
+                            external_protected_requires += name;
+
+                            friends += "\n        friend impl::consume_t<D, ";
+                            friends += name;
+                            friends += ">;";
+                            friends += "\n        friend impl::require_one<D, ";
+                            friends += name;
+                            friends += ">;";
+                        }
+                        else
+                        {
+                            ++base_interfaces_count;
+                            external_requires += ", ";
+                            external_requires += name;
+                        }
                     }
 
                     if (base_interfaces_count)
@@ -796,6 +816,15 @@ catch (...) { return winrt::to_hresult(); }
                     {
                         external_requires.clear();
                     }
+
+                    if (protected_base_interfaces_count)
+                    {
+                        external_protected_requires += '>';
+                    }
+                    else
+                    {
+                        external_protected_requires.clear();
+                    }
                 }
                 else
                 {
@@ -816,6 +845,7 @@ catch (...) { return winrt::to_hresult(); }
                 base_type_argument,
                 no_module_lock,
                 external_requires,
+                external_protected_requires,
                 bind<write_component_class_base>(type),
                 bind<write_component_override_defaults>(type),
                 type_name,
@@ -823,6 +853,7 @@ catch (...) { return winrt::to_hresult(); }
                 type_name,
                 type_name,
                 composable_base_name,
+                friends,
                 type_namespace,
                 type_name,
                 bind<write_component_class_override_constructors>(type),

cppwinrt/helpers.h

@@ -527,6 +527,7 @@ namespace cppwinrt
     {
         TypeDef type;
         bool is_default{};
+        bool is_protected{};
         bool defaulted{};
         bool overridable{};
         bool base{};
@@ -577,6 +578,7 @@ namespace cppwinrt
             auto type = impl.Interface();
             auto name = w.write_temp("%", type);
             info.is_default = has_attribute(impl, "Windows.Foundation.Metadata", "DefaultAttribute");
+            info.is_protected = has_attribute(impl, "Windows.Foundation.Metadata", "ProtectedAttribute");
             info.defaulted = !base && (defaulted || info.is_default);
 
             {

prepare_versionless_diffs.cmd

@@ -0,0 +1,41 @@
+@echo off
+
+setlocal ENABLEDELAYEDEXPANSION
+
+set target_platform=%1
+set target_configuration=%2
+if "%target_platform%"=="" set target_platform=x64
+
+if /I "%target_platform%" equ "all" (
+  if "%target_configuration%"=="" (
+    set target_configuration=all
+  )
+  call %0 x86 !target_configuration!
+  call %0 x64 !target_configuration!
+  call %0 arm !target_configuration!
+  call %0 arm64 !target_configuration!
+  goto :eof
+)
+
+if /I "%target_configuration%" equ "all" (
+  call %0 %target_platform% Debug
+  call %0 %target_platform% Release
+  goto :eof
+)
+
+if "%target_configuration%"=="" (
+ set target_configuration=Debug
+)
+
+set reference_output=%~p0\_reference\%target_platform%\%target_configuration%
+set build_output=%~p0\_build\%target_platform%\%target_configuration%
+
+echo Removing version stamps from %reference_output%\winrt
+pushd %reference_output%\winrt
+powershell -Command "gci -r -include *.h,*.ixx | %%{ (get-content $_) -replace 'was generated by.*|CPPWINRT_VERSION.*','' | set-content $_ }"
+popd
+
+echo Removing version stamps from %build_output%\winrt
+pushd %build_output%\winrt
+powershell -Command "gci -r -include *.h,*.ixx | %%{ (get-content $_) -replace 'was generated by.*|CPPWINRT_VERSION.*','' | set-content $_ }"
+popd

test/old_tests/Composable/Base.cpp

@@ -41,6 +41,11 @@ namespace winrt::Composable::implementation
         return 42;
     }
 
+    int32_t Base::ProtectedMethod()
+    {
+        return 0xDEADBEEF;
+    }
+
     hstring Base::Name() const
     {
         return m_name;

test/old_tests/Composable/Base.h

@@ -18,6 +18,7 @@ namespace winrt::Composable::implementation
         hstring OverridableMethod() ;
         virtual hstring OverridableVirtualMethod();
         int32_t OverridableNoexceptMethod() noexcept;
+        int32_t ProtectedMethod();
 
         hstring Name() const;
 

test/old_tests/Composable/Composable.idl

@@ -39,10 +39,17 @@ namespace Composable
         HRESULT OverridableVirtualMethod([out, retval] HSTRING* value);
         [noexcept2] HRESULT OverridableNoexceptMethod([out, retval] int* value);
     };
+
+    [version(1.0), uuid(6EA77EAE-56BC-419D-AE70-211C1A631496), exclusiveto(Base)]
+    interface IBaseProtected : IInspectable
+    {
+        HRESULT ProtectedMethod([out, retval] int* value);
+    };
     
     [version(1.0), uuid(5f3996e1-3cf7-4716-9a3d-11eb5d32caff), exclusiveto(Derived)]
     interface IDerived : IInspectable
     {
+        HRESULT CallProtectedMethod([out, retval] int* value);
     };
 
     [version(1.0), uuid(56dc2c28-edd1-4fa3-91e5-f63c3db47070), exclusiveto(Derived)]
@@ -60,6 +67,7 @@ namespace Composable
     {
         [default] interface IBase;
         [overridable] interface Composable.IBaseOverrides;
+        [protected] interface Composable.IBaseProtected;
     };
 
     [composable(Composable.IDerivedFactory, public, 1.0)]

test/old_tests/Composable/Derived.cpp

@@ -12,4 +12,9 @@ namespace winrt::Composable::implementation
     {
         return L"Derived::OverridableVirtualMethod";
     }
+
+    int32_t Derived::CallProtectedMethod()
+    {
+        return ProtectedMethod();
+    }
 }

test/old_tests/Composable/Derived.h

@@ -14,6 +14,7 @@ namespace winrt::Composable::implementation
 
         hstring VirtualMethod() override;
         hstring OverridableVirtualMethod() override;
+        int32_t CallProtectedMethod();
     };
 }
 

test/old_tests/UnitTests/Composable.cpp

@@ -14,6 +14,7 @@ namespace
     constexpr auto Base_OverridableMethod{ L"Base::OverridableMethod"sv };
     constexpr auto Base_OverridableVirtualMethod{ L"Base::OverridableVirtualMethod"sv };
     constexpr auto Base_OverridableNoexceptMethod{ 42 };
+    constexpr auto Base_ProtectedMethod{ 0xDEADBEEF };
 
     constexpr auto Derived_VirtualMethod{ L"Derived::VirtualMethod"sv };
     constexpr auto Derived_OverridableVirtualMethod{ L"Derived::OverridableVirtualMethod"sv };
@@ -61,12 +62,20 @@ TEST_CASE("Composable.OverriddenBase")
             {
                 return OverriddenBase_OverridableNoexceptMethod;
             }
+
+            int32_t CallProtectedMethod()
+            {
+                return ProtectedMethod();
+            }
         };
-        auto object = make<OverriddenBase>();
+
+        auto object_self = make_self<OverriddenBase>();
+        auto object = object_self.as<Base>();
         REQUIRE(object.VirtualMethod() == Base_VirtualMethod);
         REQUIRE(object.CallOverridableMethod() == OverriddenBase_OverridableMethod);
         REQUIRE(object.CallOverridableVirtualMethod() == OverriddenBase_OverridableVirtualMethod);
         REQUIRE(object.CallOverridableNoexceptMethod() == OverriddenBase_OverridableNoexceptMethod);
+        REQUIRE(object_self->CallProtectedMethod() == Base_ProtectedMethod);
     }
     {
         const std::wstring OverridableMethodResult = std::wstring(OverriddenBase_OverridableMethod) + L"=>" + Base_OverridableMethod.data();
@@ -106,6 +115,7 @@ TEST_CASE("Composable.Derived")
     REQUIRE(obj.CallOverridableMethod() == Base_OverridableMethod);
     REQUIRE(obj.CallOverridableVirtualMethod() == Derived_OverridableVirtualMethod);
     REQUIRE(obj.CallOverridableNoexceptMethod() == Base_OverridableNoexceptMethod);
+    REQUIRE(obj.CallProtectedMethod() == Base_ProtectedMethod);
 }
 
 namespace
@@ -133,6 +143,24 @@ namespace
         CallIDerived(obj);
         CallDerived(obj);
     }
+
+    template <typename T, typename = void>
+    struct has_ProtectedMember : std::false_type { };
+
+    template <typename T>
+    struct has_ProtectedMember<T, std::enable_if_t<std::is_member_function_pointer_v<decltype(&T::ProtectedMember)>>> : std::true_type { };
+
+    // make sure we can't access protected members directly
+    static_assert(!has_ProtectedMember<Composable::Base>::value);
+    static_assert(!has_ProtectedMember<Composable::Derived>::value);
+    static_assert(!has_ProtectedMember<Foo>::value);
+    static_assert(!has_ProtectedMember<Bar>::value);
+
+    // make sure we can't implicitly convert to IBaseProtected
+    static_assert(!std::is_convertible_v<Composable::Base, Composable::IBaseProtected>);
+    static_assert(!std::is_convertible_v<Composable::Derived, Composable::IBaseProtected>);
+    static_assert(!std::is_convertible_v<Foo, Composable::IBaseProtected>);
+    static_assert(!std::is_convertible_v<Bar, Composable::IBaseProtected>);
 }
 
 TEST_CASE("Composable conversions")

test/test_component_base/HierarchyA.cpp

@@ -7,6 +7,10 @@ namespace winrt::test_component_base::implementation
     {
         throw hresult_not_implemented();
     }
+    HierarchyA::HierarchyA(int32_t dummy, hstring const& name)
+    {
+        throw hresult_not_implemented();
+    }
     void HierarchyA::HierarchyA_Method()
     {
         //test_component_base::HierarchyA a = *this;
@@ -15,4 +19,10 @@ namespace winrt::test_component_base::implementation
         //test_component_base::IHierarchyA ia = *this;
         //assert(a);
     }
+    int HierarchyA::HierarchyA_Protected()
+    {
+        return 42;
+    }
+
+    static_assert(!std::is_constructible_v<test_component_base::HierarchyA, int32_t, hstring const&>);
 }

test/test_component_base/HierarchyA.h

@@ -8,7 +8,9 @@ namespace winrt::test_component_base::implementation
         HierarchyA() = default;
 
         HierarchyA(hstring const& name);
+        HierarchyA(int32_t dummy, hstring const& name);
         void HierarchyA_Method();
+        int HierarchyA_Protected();
     };
 }
 namespace winrt::test_component_base::factory_implementation

test/test_component_base/HierarchyB.cpp

@@ -7,8 +7,16 @@ namespace winrt::test_component_base::implementation
     {
         throw hresult_not_implemented();
     }
+    HierarchyB::HierarchyB(int32_t dummy, hstring const& name) : HierarchyB_base(dummy, name)
+    {
+        throw hresult_not_implemented();
+    }
     void HierarchyB::HierarchyB_Method()
     {
         throw hresult_not_implemented();
     }
+    void HierarchyB::HierarchyB_TestInnerProtected()
+    {
+        assert(HierarchyA_Protected() == 42);
+    }
 }

test/test_component_base/HierarchyB.h

@@ -9,7 +9,9 @@ namespace winrt::test_component_base::implementation
         HierarchyB() = default;
 
         HierarchyB(hstring const& name);
+        HierarchyB(int32_t dummy, hstring const& name);
         void HierarchyB_Method();
+        void HierarchyB_TestInnerProtected();
     };
 }
 namespace winrt::test_component_base::factory_implementation

test/test_component_base/test_component_base.idl

@@ -6,15 +6,19 @@ namespace test_component_base
     {
         HierarchyA();
         HierarchyA(String name);
+        protected HierarchyA(Int32 dummy, String name);
 
         void HierarchyA_Method();
+        protected Int32 HierarchyA_Protected();
     }
 
     unsealed runtimeclass HierarchyB : HierarchyA
     {
         HierarchyB();
         HierarchyB(String name);
+        protected HierarchyB(Int32 dummy, String name);
 
         void HierarchyB_Method();
+        void HierarchyB_TestInnerProtected();
     }
 }

test/test_component_derived/Nested.HierarchyC.cpp

@@ -3,7 +3,7 @@
 
 namespace winrt::test_component_derived::Nested::implementation
 {
-    HierarchyC::HierarchyC(hstring const& name)
+    HierarchyC::HierarchyC(hstring const& name) : HierarchyC_base(10, name)
     {
         throw hresult_not_implemented();
     }
@@ -11,4 +11,7 @@ namespace winrt::test_component_derived::Nested::implementation
     {
         throw hresult_not_implemented();
     }
+
+    static_assert(!std::is_convertible_v<HierarchyC, winrt::test_component_base::IHierarchyAProtected>);
+    static_assert(!std::is_constructible_v<winrt::test_component_base::HierarchyA, int32_t, hstring const&>);
 }

test/test_component_derived/Nested.HierarchyD.cpp

@@ -32,5 +32,8 @@ namespace winrt::test_component_derived::Nested::implementation
 
         test_component_base::IHierarchyA ia = *this;
         assert(ia);
+
+        assert(HierarchyA_Protected() == 42);
+        HierarchyB_TestInnerProtected();
     }
 }