From 521ee3596a78f950a305e05f994fb7bdee2c2137 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 9 Oct 2024 18:06:24 -0700 Subject: [PATCH 1/5] Don't emit default ctor for non activatable types --- .../WinRT.SourceGenerator/WinRTTypeWriter.cs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/Authoring/WinRT.SourceGenerator/WinRTTypeWriter.cs b/src/Authoring/WinRT.SourceGenerator/WinRTTypeWriter.cs index 8e7520001..c6f74e16c 100644 --- a/src/Authoring/WinRT.SourceGenerator/WinRTTypeWriter.cs +++ b/src/Authoring/WinRT.SourceGenerator/WinRTTypeWriter.cs @@ -1932,12 +1932,25 @@ void AddComponentType(INamedTypeSymbol type, Action visitTypeDeclaration = null) bool isInterface = type.TypeKind == TypeKind.Interface; bool hasConstructor = false; + bool hasAtLeastOneNonPublicConstructor = false; bool hasDefaultConstructor = false; foreach (var member in type.GetMembers()) { if (!IsPublic(member) || typeDeclaration.CustomMappedSymbols.Contains(member)) { Logger.Log(member.Kind + " member skipped " + member.Name); + + // We want to track whether a given public class has at least one non-public constructor. + // In this case, the class is still exposed to WinRT, but it's not activatable. This is + // different than a class with no explicit constructor, where we do want to generate that + // in the .winmd, and make the class activatable. But we want to avoid always emitting a + // default constructor for classes that only have non-public ones. + if (type.TypeKind == TypeKind.Class && + member is IMethodSymbol { MethodKind: MethodKind.Constructor }) + { + hasAtLeastOneNonPublicConstructor = true; + } + continue; } @@ -1988,8 +2001,8 @@ void AddComponentType(INamedTypeSymbol type, Action visitTypeDeclaration = null) CheckAndMarkSymbolForAttributes(member); } - // implicit constructor if none defined - if (!hasConstructor && type.TypeKind == TypeKind.Class && !type.IsStatic) + // Implicit constructor if none defined, but only if the type doesn't already have some non-public constructor + if (!hasConstructor && !hasAtLeastOneNonPublicConstructor && type.TypeKind == TypeKind.Class && !type.IsStatic) { string constructorMethodName = ".ctor"; var methodDefinitionHandle = AddMethodDefinition( From 8add531cbf4587db342b034416db15e6e173f61e Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 9 Oct 2024 19:08:38 -0700 Subject: [PATCH 2/5] Add test for non activatable type --- .../AuthoringConsumptionTest.exe.manifest | 6 +++++- src/Tests/AuthoringConsumptionTest/test.cpp | 18 ++++++++++++++++++ src/Tests/AuthoringTest/Program.cs | 12 ++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest b/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest index 7e408a3cb..f9c66fd5e 100644 --- a/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest +++ b/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest @@ -82,9 +82,13 @@ name="AuthoringTest.CustomXamlMetadataProvider" threadingModel="both" xmlns="urn:schemas-microsoft-com:winrt.v1" /> - + diff --git a/src/Tests/AuthoringConsumptionTest/test.cpp b/src/Tests/AuthoringConsumptionTest/test.cpp index 7d8da8f26..68729a844 100644 --- a/src/Tests/AuthoringConsumptionTest/test.cpp +++ b/src/Tests/AuthoringConsumptionTest/test.cpp @@ -735,4 +735,22 @@ TEST(AuthoringTest, CustomInterfaceGuid) check_hresult(customInterfaceClassUnknown->QueryInterface(customInterfaceIid, reinterpret_cast(winrt::put_abi(customInterface)))); EXPECT_EQ(customInterface.HelloWorld(), L"Hello World!"); +} + +TEST(AuthoringTest, NonActivatableType) +{ + bool hasFailed = false; + + try + { + NonActivatableType nonActivatableType; + + nonActivatableType.Dummy(); + } + catch (...) + { + hasFailed = true; + } + + ASSERT_TRUE(hasFailed); } \ No newline at end of file diff --git a/src/Tests/AuthoringTest/Program.cs b/src/Tests/AuthoringTest/Program.cs index f1c715b0d..462ca5a3c 100644 --- a/src/Tests/AuthoringTest/Program.cs +++ b/src/Tests/AuthoringTest/Program.cs @@ -1865,6 +1865,18 @@ public sealed class CustomInterfaceGuidClass : ICustomInterfaceGuid { public string HelloWorld() => "Hello World!"; } + + public sealed class NonActivatableType + { + // This should not be referenced by the generated activation factory + internal NonActivatableType(string dummyParameter) + { + } + + public void Dummy() + { + } + } } namespace ABI.AuthoringTest From 5c8fbec0ff0a50b530f21cc50070a50dc61167c0 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 9 Oct 2024 19:27:34 -0700 Subject: [PATCH 3/5] Add more unit tests --- .../AuthoringConsumptionTest.exe.manifest | 4 ++++ src/Tests/AuthoringConsumptionTest/test.cpp | 7 ++++++- src/Tests/AuthoringTest/Program.cs | 16 ++++++++++++++-- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest b/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest index f9c66fd5e..33b5a607b 100644 --- a/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest +++ b/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest @@ -90,5 +90,9 @@ name="AuthoringTest.NonActivatableType" threadingModel="both" xmlns="urn:schemas-microsoft-com:winrt.v1" /> + diff --git a/src/Tests/AuthoringConsumptionTest/test.cpp b/src/Tests/AuthoringConsumptionTest/test.cpp index 68729a844..55d4bbafe 100644 --- a/src/Tests/AuthoringConsumptionTest/test.cpp +++ b/src/Tests/AuthoringConsumptionTest/test.cpp @@ -745,7 +745,7 @@ TEST(AuthoringTest, NonActivatableType) { NonActivatableType nonActivatableType; - nonActivatableType.Dummy(); + nonActivatableType.GetText(); } catch (...) { @@ -753,4 +753,9 @@ TEST(AuthoringTest, NonActivatableType) } ASSERT_TRUE(hasFailed); +} + +TEST(AuthoringTest, NonActivatableFactory) +{ + EXPECT_EQ(NonActivatableFactory::Create().GetText(), L"Test123"); } \ No newline at end of file diff --git a/src/Tests/AuthoringTest/Program.cs b/src/Tests/AuthoringTest/Program.cs index 462ca5a3c..005641418 100644 --- a/src/Tests/AuthoringTest/Program.cs +++ b/src/Tests/AuthoringTest/Program.cs @@ -1868,13 +1868,25 @@ public sealed class CustomInterfaceGuidClass : ICustomInterfaceGuid public sealed class NonActivatableType { + private readonly string _text; + // This should not be referenced by the generated activation factory - internal NonActivatableType(string dummyParameter) + internal NonActivatableType(string text) { + _text = text; } - public void Dummy() + public string GetText() + { + return _text; + } + } + + public static class NonActivatableFactory + { + public static NonActivatableType Create() { + return new("Test123"); } } } From bea716956a176bd329075a64874fcfc8f10a0119 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 9 Oct 2024 20:07:44 -0700 Subject: [PATCH 4/5] Remove 'NonActivatableType' test --- src/Tests/AuthoringConsumptionTest/test.cpp | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/Tests/AuthoringConsumptionTest/test.cpp b/src/Tests/AuthoringConsumptionTest/test.cpp index 55d4bbafe..21213fb00 100644 --- a/src/Tests/AuthoringConsumptionTest/test.cpp +++ b/src/Tests/AuthoringConsumptionTest/test.cpp @@ -737,24 +737,6 @@ TEST(AuthoringTest, CustomInterfaceGuid) EXPECT_EQ(customInterface.HelloWorld(), L"Hello World!"); } -TEST(AuthoringTest, NonActivatableType) -{ - bool hasFailed = false; - - try - { - NonActivatableType nonActivatableType; - - nonActivatableType.GetText(); - } - catch (...) - { - hasFailed = true; - } - - ASSERT_TRUE(hasFailed); -} - TEST(AuthoringTest, NonActivatableFactory) { EXPECT_EQ(NonActivatableFactory::Create().GetText(), L"Test123"); From 67832b5a4968789d0be18a9b00bf83fe5cf8534b Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 10 Oct 2024 00:09:29 -0700 Subject: [PATCH 5/5] Add one more test case --- .../AuthoringConsumptionTest.exe.manifest | 4 ++++ src/Tests/AuthoringConsumptionTest/test.cpp | 5 +++++ src/Tests/AuthoringTest/Program.cs | 20 +++++++++++++++++++ 3 files changed, 29 insertions(+) diff --git a/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest b/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest index 33b5a607b..bf43d8acd 100644 --- a/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest +++ b/src/Tests/AuthoringConsumptionTest/AuthoringConsumptionTest.exe.manifest @@ -94,5 +94,9 @@ name="AuthoringTest.NonActivatableFactory" threadingModel="both" xmlns="urn:schemas-microsoft-com:winrt.v1" /> + diff --git a/src/Tests/AuthoringConsumptionTest/test.cpp b/src/Tests/AuthoringConsumptionTest/test.cpp index 21213fb00..a7c5db33a 100644 --- a/src/Tests/AuthoringConsumptionTest/test.cpp +++ b/src/Tests/AuthoringConsumptionTest/test.cpp @@ -740,4 +740,9 @@ TEST(AuthoringTest, CustomInterfaceGuid) TEST(AuthoringTest, NonActivatableFactory) { EXPECT_EQ(NonActivatableFactory::Create().GetText(), L"Test123"); +} + +TEST(AuthoringTest, TypeOnlyActivatableViaItsOwnFactory) +{ + EXPECT_EQ(TypeOnlyActivatableViaItsOwnFactory::Create().GetText(), L"Hello!"); } \ No newline at end of file diff --git a/src/Tests/AuthoringTest/Program.cs b/src/Tests/AuthoringTest/Program.cs index 005641418..db0eb5963 100644 --- a/src/Tests/AuthoringTest/Program.cs +++ b/src/Tests/AuthoringTest/Program.cs @@ -1889,6 +1889,26 @@ public static NonActivatableType Create() return new("Test123"); } } + + public sealed class TypeOnlyActivatableViaItsOwnFactory + { + private readonly string _text; + + private TypeOnlyActivatableViaItsOwnFactory(string text) + { + _text = text; + } + + public static TypeOnlyActivatableViaItsOwnFactory Create() + { + return new("Hello!"); + } + + public string GetText() + { + return _text; + } + } } namespace ABI.AuthoringTest