Skip to content

Conversation

@manodasanW
Copy link
Member

This PR adds a new attribute WinRTExposedType which can be put on C# implemented types that are passed across the ABI. This attribute takes an array of WinRT interface types which are entries on the CCW vtable for that type. Previously we were using DynamicallyAccessedMemberTypes.Interfaces along with reflection, but given this attribute was placed on a function several levels down and one of which is of type object rather than the concrete type, it didn't work as intended when all assemblies were trimmed. With this new attribute, we are also able to skip some of the WinRT type checks along with covariance type generation at runtime along with not needing to keep all the interfaces on the type that aren't WinRT interfaces if they aren't needed.

This attribute can be added by libraries and apps on their C# types that they use with CsWinRT projected APIs if desired. But this PR also adds a source generator which will analyze the sources and determine if there are any WinRT interfaces implemented by the class and if so adds the attribute to the class if it is declared partial. CsWinRT also puts that attribute on unsealed classes so that any internal interfaces implemented by such projected classes in override / protected scenarios are also on the vtable if they are extended by another C# class.

[global::WinRT.WinRTExposedType(typeof(TestComponentCSharp.IProperties1), typeof(TestComponentCSharp.IUriHandler))]
partial class ManagedProperties
{
}

Upcoming todo:

  • Currently the source generator only handles classes but in a future PR will also handle structs and delegates to add the IReference interface. Custom type mapped delegates like System.EventHandler<int> may need to be handled differently.
  • Diagnostics will also be added in a future PR to do a code fix to add partial if the class isn't already marked as such.
  • CsWinRT Authored WinRT components will also be updated to have this attribute.

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just leaving some initial thoughts 🙂

I assume the analyzer you mentioned would be to warn users if their types are not partial, because the generator wouldn't be able to add the necessary attributes to it?

@Sergio0694
Copy link
Member

For authored WinRT components, will it be an issue that the new attributes takes an array of types? Array of types aren't allowed as attribute parameters in WinRT components as far as I know, and I remember we specifically hit this issue in the Microsoft Store as well. Unless that restriction was just due to .NET Native specifically and not something about WinRT components per se? 🤔

@manodasanW
Copy link
Member Author

I assume the analyzer you mentioned would be to warn users if their types are not partial, because the generator wouldn't be able to add the necessary attributes to it?

Right, I believe the type would need to be marked partial for the source generator to add the attribute. There is always the option for the developer themselves to put the attribute if they want and I plan on updating the source generator to detect that and not put it itself.

@manodasanW
Copy link
Member Author

manodasanW commented Aug 31, 2023

For authored WinRT components, will it be an issue that the new attributes takes an array of types? Array of types aren't allowed as attribute parameters in WinRT components as far as I know, and I remember we specifically hit this issue in the Microsoft Store as well. Unless that restriction was just due to .NET Native specifically and not something about WinRT components per se? 🤔

Based on this from the docs "The types of a custom attribute's parameters and fields are limited to the WinRT fundamental types, enums, and references to other WinRT types. No other parameter or field type is allowed.", it sounds like array of types might not be allowed on an WinRT attribute. But in this case, the attribute we are putting will not be emitted to the WinMD to project, it will just be on the actual implementation type as a C# attribute (not a WinRT attribute).

@manodasanW
Copy link
Member Author

This should possibly fix #1270, will need to validate.

Co-authored-by: Sergio Pedri <sergio0694@live.com>
@manodasanW
Copy link
Member Author

Merging these changes for now to unblock my other PRs, but I will probably have an upcoming PR that will slightly change the design here to the below. This will allow to eliminate some of the remaining reflection to get the vftbl ptr and also be a design that we can eventually easily move to static interfaces from C# 11 when we can.

[global::WinRT.WinRTExposedType(typeof(ManagedPropertiesCCW))]
partial class ManagedProperties
{
}

public ManagedPropertiesCCW : IWinRTExposedTypeDetails
{
...
}

@manodasanW manodasanW merged commit 2005e9e into staging/AOT Sep 9, 2023
@manodasanW manodasanW deleted the manodasanw/fixaotvtable branch September 9, 2023 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants