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

[API Proposal]: Define parameter marshalling of interfaces #85797

Closed
jkoritzinsky opened this issue May 4, 2023 · 5 comments · Fixed by #86177
Closed

[API Proposal]: Define parameter marshalling of interfaces #85797

jkoritzinsky opened this issue May 4, 2023 · 5 comments · Fixed by #86177
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature
Milestone

Comments

@jkoritzinsky
Copy link
Member

jkoritzinsky commented May 4, 2023

Background and motivation

Today, we allow developers to specify the default marshaller for a type in source-generated marshalling with the NativeMarshalling attribute. We should allow this attribute on interface types, so interface types in source-generated interop signatures can have default marshallers. This wasn't common before, but with source-generated COM coming into the mix, this scenario is much more popular.

When NativeMarshalling is applied to an interface, the source-generator will only respect it when the static type in a signature is the interface type (same rules we use for all other types). We don't do any inheritance walking, we only look on the type itself.

API Proposal

namespace System.Runtime.InteropServices.Marshalling;

- [AttributeUsage(AttributeTargets.Struct | AttributeTargets.Class | AttributeTargets.Enum | AttributeTargets.Delegate)]
+ [AttributeUsage(AttributeTargets.Struct | AttributeTargets.Class | AttributeTargets.Enum | AttributeTargets.Interface | AttributeTargets.Delegate)]
public sealed class NativeMarshallingAttribute : Attribute
{
}

+/// <summary>
+/// COM interface marshaller using a StrategyBasedComWrappers instance
+/// </summary>
+/// <remarks>
+/// This marshaller will always pass the <see cref="CreateObjectFlags.Unwrap"/> flag
+/// to <see cref="ComWrappers.GetOrCreateObjectForComInstance(IntPtr, CreateObjectFlags)"/>.
+/// </remarks>
+[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.Default, typeof(ComInterfaceMarshaller<>)]
+public static class ComInterfaceMarshaller<T>
+{
+    public static void* ConvertToUnmanaged(T managed);
+    public static T ConvertToManaged(void* unmanaged);
+}

+/// <summary>
+/// COM interface marshaller using a StrategyBasedComWrappers instance
+/// that will only create unique native object wrappers (RCW).
+/// </summary>
+/// <remarks>
+/// This marshaller will always pass the <see cref="CreateObjectFlags.Unwrap"/> and <see cref="CreateObjectFlags.UniqueInstance"/> flags
+/// to <see cref="ComWrappers.GetOrCreateObjectForComInstance(IntPtr, CreateObjectFlags)"/>.
+/// </remarks>
+[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.Default, typeof(UniqueComInterfaceMarshaller<>)]
+public static class UniqueComInterfaceMarshaller<T>
+{
+    public static void* ConvertToUnmanaged(T managed);
+    public static T ConvertToManaged(void* unmanaged);
+}

+/// <summary>
+/// COM interface marshaller using a user defined ComWrappers type.
+/// </summary>
+/// <remarks>
+/// This marshaller will always pass the <see cref="CreateObjectFlags.Unwrap"/> flag
+/// to <see cref="ComWrappers.GetOrCreateObjectForComInstance(IntPtr, CreateObjectFlags)"/>.
+/// </remarks>
+[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.Default, typeof(ComInterfaceMarshaller<,>)]
+public static class ComInterfaceMarshaller<T, U> where U : ComWrappers
+{
+    public static void* ConvertToUnmanaged(T managed);
+    public static T ConvertToManaged(void* unmanaged);
+}

API Usage

[NativeMarshalling(typeof(ComInterfaceMarshaller<IMyComInterface>))]
interface IMyComInterface
{
    public void Method([MarshalUsing(typeof(UniqueComInterfaceMarshaller<IMyComInterface>))] IMyComInterface itf);
}

Alternative Designs

None

Risks

This proposal does not attempt to provide first-class support for more complicated scenarios such as using a specific ComWrappers instance or specifying the IID of an object in a method signature. Rather than add complexity to the source generator, users can handle such scenarios manually by explicitly marshalling using their ComWrappers instance and/or defining different interfaces.

@jkoritzinsky jkoritzinsky added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature labels May 4, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 4, 2023
@ghost
Copy link

ghost commented May 4, 2023

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Today, we allow developers to specify the default marshaller for a type in source-generated marshalling with the NativeMarshalling attribute. We should allow this attribute on interface types, so interface types in source-generated interop signatures can have default marshallers. This wasn't common before, but with source-generated COM coming into the mix, this scenario is much more popular.

When NativeMarshalling is applied to an interface, the source-generator will only respect it when the static type in a signature is the interface type (same rules we use for all other types). We don't do any inheritance walking, we only look on the type itself.

API Proposal

namespace System.Runtime.InteropServices.Marshalling;

- [AttributeUsage(AttributeTargets.Struct | AttributeTargets.Class | AttributeTargets.Enum | AttributeTargets.Delegate)]
+ [AttributeUsage(AttributeTargets.Struct | AttributeTargets.Class | AttributeTargets.Enum | AttributeTargets.Interface | AttributeTargets.Delegate)]
public sealed class NativeMarshallingAttribute : Attribute
{
}

API Usage

[NativeMarshalling(typeof(Marshaller))]
interface IMyComInterface
{}

Alternative Designs

None

Risks

N/A

Author: jkoritzinsky
Assignees: -
Labels:

api-suggestion, area-System.Runtime.InteropServices, source-generator

Milestone: -

@AaronRobinsonMSFT AaronRobinsonMSFT added api-ready-for-review API is ready for review, it is NOT ready for implementation api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 4, 2023
@AaronRobinsonMSFT
Copy link
Member

We might want to add in a built-in marshaller type too.

public static class ComInterfaceMarshaller<T>
{
    private static StrategyBasedComWrappers s_wrappers = new();
    public static void* ConvertToUnmanaged(T managed);
    public static T ConvertToManaged(void* unmanaged);
}

// Perhaps nice to have?
public static class ComInterfaceMarshaller<T, U> where U : ComWrappers
{
    private static U s_wrappers = new();
    public static void* ConvertToUnmanaged(T managed);
    public static T ConvertToManaged(void* unmanaged);
}

@AaronRobinsonMSFT
Copy link
Member

How would this integrate with LibraryImport? Is the assumption here the logic would "just work" since the defined marshaller could be used and so APIs like CreateStreamOnHGlobal() could be defined as:

[LibraryImport("Ole32")]
private static partial int CreateStreamOnHGlobal(
    void* hGlobal,
    bool fDeleteOnRelease,
    out IStream ppstm);

[NativeMarshalling(typeof(Marshaller))]
private interface IStream
{ }

Do we have any concerns with down level support if the above is true? The LibraryImport API degrades to DllImport, how would the above scenario work in that case?

@AaronRobinsonMSFT AaronRobinsonMSFT changed the title [API Proposal]: Allow System.Runtime.InteropServices.Marshalling.NativeMarshalling on interfaces [API Proposal]: Define parameter marshalling of interfaces May 5, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 5, 2023
@jkoritzinsky
Copy link
Member Author

For LibraryImport, these marshallers would work like any other user-defined marshaller. They don't work downlevel below .NET 7. If we decide to make the ComInterfaceMarshaller<T> type the default marshaller for [MarshalAs(UnmanagedType.Interface)] (which it sounds like we want), then we'd only recognize UnmanagedType.Interface on .NET 8 and newer.

@jkoritzinsky jkoritzinsky added this to the 8.0.0 milestone May 5, 2023
@bartonjs
Copy link
Member

bartonjs commented May 9, 2023

Video

  • We removed ComInterfaceMarshaller<T, U> in lieu of answering whether or not the generic arity overloading was obvious for the scenario.
  • We raised the question of whether or not UniqueComInterfaceMarshaller needed "Instance" in the name, and decided it was OK as-is because of a low number of users (who are expected to be domain semi-experts) and that the name was already long enough.
namespace System.Runtime.InteropServices.Marshalling;

- [AttributeUsage(AttributeTargets.Struct | AttributeTargets.Class | AttributeTargets.Enum | AttributeTargets.Delegate)]
+ [AttributeUsage(AttributeTargets.Struct | AttributeTargets.Class | AttributeTargets.Enum | AttributeTargets.Interface | AttributeTargets.Delegate)]
public sealed class NativeMarshallingAttribute : Attribute
{
}
namespace System.Runtime.InteropServices.Marshalling;

/// <summary>
/// COM interface marshaller using a StrategyBasedComWrappers instance
/// </summary>
/// <remarks>
/// This marshaller will always pass the <see cref="CreateObjectFlags.Unwrap"/> flag
/// to <see cref="ComWrappers.GetOrCreateObjectForComInstance(IntPtr, CreateObjectFlags)"/>.
/// </remarks>
[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.Default, typeof(ComInterfaceMarshaller<>))]
public static class ComInterfaceMarshaller<T>
{
    public static void* ConvertToUnmanaged(T managed);
    public static T ConvertToManaged(void* unmanaged);
}

/// <summary>
/// COM interface marshaller using a StrategyBasedComWrappers instance
/// that will only create unique native object wrappers (RCW).
/// </summary>
/// <remarks>
/// This marshaller will always pass the <see cref="CreateObjectFlags.Unwrap"/> and <see cref="CreateObjectFlags.UniqueInstance"/> flags
/// to <see cref="ComWrappers.GetOrCreateObjectForComInstance(IntPtr, CreateObjectFlags)"/>.
/// </remarks>
[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.Default, typeof(UniqueComInterfaceMarshaller<>))]
public static class UniqueComInterfaceMarshaller<T>
{
    public static void* ConvertToUnmanaged(T managed);
    public static T ConvertToManaged(void* unmanaged);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 9, 2023
@jkoritzinsky jkoritzinsky self-assigned this May 11, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 12, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 20, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants