Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with properties implemented across multiple static interfaces #1415

Merged
merged 2 commits into from
Dec 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Tests/TestComponentCSharp/TestComponentCSharp.idl
Original file line number Diff line number Diff line change
Expand Up @@ -505,11 +505,13 @@ namespace TestComponentCSharp
static void Method();
static Int32 Property;
static event Windows.Foundation.EventHandler<Int32> Event;
static Int32 ReadWriteProperty{ get; };
[contract(Windows.Foundation.UniversalApiContract, 10)]
{
static void WarningMethod();
static Int32 WarningProperty;
static event Windows.Foundation.EventHandler<Int32> WarningEvent;
static Int32 ReadWriteProperty{ set; };
}
}

Expand Down
7 changes: 7 additions & 0 deletions src/Tests/TestComponentCSharp/WarningStatic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,11 @@ namespace winrt::TestComponentCSharp::implementation
void WarningStatic::WarningEvent(winrt::event_token const& token) noexcept
{
}
int32_t WarningStatic::ReadWriteProperty()
{
return 0;
}
void WarningStatic::ReadWriteProperty(int32_t value)
{
}
}
2 changes: 2 additions & 0 deletions src/Tests/TestComponentCSharp/WarningStatic.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ namespace winrt::TestComponentCSharp::implementation
static void WarningProperty(int32_t value);
static winrt::event_token WarningEvent(Windows::Foundation::EventHandler<int32_t> const& handler);
static void WarningEvent(winrt::event_token const& token) noexcept;
static int32_t ReadWriteProperty();
static void ReadWriteProperty(int32_t value);
};
}
namespace winrt::TestComponentCSharp::factory_implementation
Expand Down
8 changes: 8 additions & 0 deletions src/Tests/UnitTest/TestComponentCSharp_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2660,6 +2660,14 @@ public void TestSetPropertyAcrossProjections()
Assert.Equal(4, property.ReadWriteProperty);
}

[Fact]
public void TestStaticPropertyImplementedAcrossInterfaces()
{
// Testing call doesn't fail.
WarningStatic.ReadWriteProperty = 4; // expected warning CA1416
_ = WarningStatic.ReadWriteProperty;
}

// Test scenario where type reported by runtimeclass name is not a valid type (i.e. internal type).
[Fact]
public void TestNonProjectedRuntimeClass()
Expand Down
108 changes: 85 additions & 23 deletions src/cswinrt/code_writers.h
Original file line number Diff line number Diff line change
Expand Up @@ -2404,17 +2404,6 @@ Marshal.Release(inner);
std::nullopt);
}

void write_static_property(writer& w, Property const& prop, std::string_view prop_target, std::string_view platform_attribute = ""sv)
{
auto [getter, setter] = get_property_methods(prop);
auto getter_target = getter ? prop_target : "";
auto setter_target = setter ? prop_target : "";
write_property(w, prop.Name(), prop.Name(), write_prop_type(w, prop),
getter_target, setter_target, "public "sv, "static "sv, platform_attribute, platform_attribute,
!getter ? std::nullopt : std::optional(std::pair(prop.Parent(), prop)),
!setter ? std::nullopt : std::optional(std::pair(prop.Parent(), prop)));
}

void write_static_factory_event(writer& w, Event const& event, std::string_view event_target, std::string_view platform_attribute = ""sv)
{
write_event(w, event.Name(), event, event_target, "public "sv, ""sv, platform_attribute, std::nullopt);
Expand All @@ -2425,18 +2414,72 @@ Marshal.Release(inner);
write_event(w, event.Name(), event, event_target, "public "sv, "static "sv, platform_attribute, std::optional(std::tuple(event.Parent(), event, true)));
}

void write_static_members(writer& w, TypeDef const& static_type, TypeDef const& class_type)
void write_static_members(writer& w, TypeDef const& class_type)
{
auto vftblType = settings.netstandard_compat ?
w.write_temp("%.Vftbl", bind<write_type_name>(static_type, typedef_name_type::ABI, true)) :
"IUnknownVftbl";
write_static_objref_definition(w, vftblType, static_type, class_type);
auto cache_object = w.write_temp("%", bind<write_objref_type_name>(static_type));
std::map<std::string, std::tuple<std::string, std::string, std::string, std::string, std::string, std::optional<std::pair<TypeDef, Property>>, std::optional<std::pair<TypeDef, Property>>>> properties;

for (auto&& [interface_name, factory] : get_attributed_types(w, class_type))
{
if (factory.statics)
{
auto vftblType = settings.netstandard_compat ?
w.write_temp("%.Vftbl", bind<write_type_name>(factory.type, typedef_name_type::ABI, true)) :
"IUnknownVftbl";
write_static_objref_definition(w, vftblType, factory.type, class_type);
auto cache_object = w.write_temp("%", bind<write_objref_type_name>(factory.type));

auto platform_attribute = write_platform_attribute_temp(w, factory.type);
w.write_each<write_static_method>(factory.type.MethodList(), cache_object, platform_attribute);
w.write_each<write_static_event>(factory.type.EventList(), cache_object, platform_attribute);

// Merge property getters/setters, since such may be defined across interfaces
for (auto&& prop : factory.type.PropertyList())
{
auto [getter, setter] = get_property_methods(prop);
auto prop_type = write_prop_type(w, prop);

auto [prop_targets, inserted] = properties.try_emplace(std::string(prop.Name()),
prop_type,
getter ? cache_object : "",
getter ? platform_attribute : "",
setter ? cache_object : "",
setter ? platform_attribute : "",
!getter ? std::nullopt : std::optional(std::pair(prop.Parent(), prop)),
!setter ? std::nullopt : std::optional(std::pair(prop.Parent(), prop))
);
if (!inserted)
{
auto& [property_type, getter_target, getter_platform, setter_target, setter_platform, getter_prop, setter_prop] = prop_targets->second;
XLANG_ASSERT(property_type == prop_type);
if (getter)
{
XLANG_ASSERT(getter_target.empty());
getter_target = cache_object;
getter_platform = platform_attribute;
getter_prop = std::optional(std::pair(prop.Parent(), prop));
}
if (setter)
{
XLANG_ASSERT(setter_target.empty());
setter_target = cache_object;
setter_platform = platform_attribute;
setter_prop = std::optional(std::pair(prop.Parent(), prop));
}
XLANG_ASSERT(!getter_target.empty() || !setter_target.empty());
}
}
}
}

auto platform_attribute = write_platform_attribute_temp(w, static_type);
w.write_each<write_static_method>(static_type.MethodList(), cache_object, platform_attribute);
w.write_each<write_static_property>(static_type.PropertyList(), cache_object, platform_attribute);
w.write_each<write_static_event>(static_type.EventList(), cache_object, platform_attribute);
// Write properties with merged accessors
for (auto& [prop_name, prop_data] : properties)
{
auto& [prop_type, getter_target, getter_platform, setter_target, setter_platform, getter_prop, setter_prop] = prop_data;
write_property(w, prop_name, prop_name, prop_type,
getter_target, setter_target, "public "sv, "static "sv, getter_platform, setter_platform,
getter_prop,
setter_prop);
}
}

void write_attributed_types(writer& w, TypeDef const& type)
Expand Down Expand Up @@ -2489,10 +2532,10 @@ public static %I As<I>() => ActivationFactory.Get("%.%").AsInterface<I>();
type.TypeNamespace(),
type.TypeName());
}

write_static_members(w, factory.type, type);
}
}

write_static_members(w, type);
}

void write_nongeneric_enumerable_members(writer& w, std::string_view target)
Expand Down Expand Up @@ -3098,6 +3141,20 @@ remove => %.ErrorsChanged -= value;
return false;
};

std::function<bool(TypeDef const&)> search_interfaces_from_attributes = [&](TypeDef const& type)
{
for (auto&& [interface_name, factory] : get_attributed_types(w, type))
{
if (factory.statics && factory.type && (search_interface(factory.type) || search_interfaces(factory.type)))
{
return true;
}
}

return false;
};


// first search base interfaces for property getter
if (search_interfaces(setter_iface))
{
Expand All @@ -3116,6 +3173,11 @@ remove => %.ErrorsChanged -= value;
{
return { getter_iface, false };
}

if (search_interfaces_from_attributes(exclusive_to_type))
{
return { getter_iface, false };
}
}

throw_invalid("Could not find property getter interface");
Expand Down