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

Proposal: allow exception marshallers via '[return: MarshalUsing]' on generated COM interfaces methods without '[PreserveSig]' #109522

Open
Sergio0694 opened this issue Nov 4, 2024 · 14 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Milestone

Comments

@Sergio0694
Copy link
Contributor

Sergio0694 commented Nov 4, 2024

Overview

This came up in microsoft/CsWinRT#1858. We'd like for the COM generator to support [return: MarshalUsing] to use custom marshallers for exceptions, so that CsWinRT could correctly allow using the necessary error propagation logic from generated COM interfaces. Currently, trying the code below doesn't do anything (but also it doesn't emit any warnings, it just silently does nothing).

API proposal

class GeneratedComInterfaceAttribute
{
+     public Type? ExceptionToUnmanagedMarshaller { get; set; }
}

Use case example

Users will be able to implement COM interfaces like this, implemented on WinRT-exposed types:

[Guid("6234C2F7-9917-469F-BDB4-3E8C630598AF")]
[GeneratedComInterface(
    Options = ComInterfaceOptions.ManagedObjectWrapper,
    ExceptionToUnmanagedMarshaller = typeof(ExceptionHelpersHResultMarshaller))]
public partial interface IFoo
{
    void Bar();
}

With ExceptionHelpersHResultMarshaller being a WinRT-compatible marshaller type we'll include in CsWinRT.

Risks

None that I can think of.

Alternatives

Developers can work around this by manually writing the entire vtable backend (eg. here).
This is extremely clunky and error prone.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Nov 4, 2024
Copy link
Contributor

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

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Nov 4, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the Future milestone Nov 4, 2024
@jkotas
Copy link
Member

jkotas commented Nov 4, 2024

What does this look like for methods with return value?

@Sergio0694
Copy link
Contributor Author

Sergio0694 commented Nov 4, 2024

My understanding is there would be no difference, because at the ABI level, the return is always an HRESULT anyway. So the generated ABI method will still always return an int __retVal local. This proposal is simply to allow overriding the default ExceptionAsHResultMarshaller<> type. Basically whenever that would be used by default by the COM generator, users should be able to indicate a custom equivalent type. Then the way the COM generator uses it would be identical to however ExceptionAsHResultMarshaller<> is already used today.

Eg. example codegen for:

[Guid("6234C2F7-9917-469F-BDB4-3E8C630598AF")]
[GeneratedComInterface(Options = ComInterfaceOptions.ManagedObjectWrapper)]
public partial interface IFoo
{
    double Bar();
}

You get:

file unsafe partial interface InterfaceImplementation
{
    [global::System.Runtime.InteropServices.UnmanagedCallersOnlyAttribute(CallConvs = new[] { typeof(global::System.Runtime.CompilerServices.CallConvMemberFunction) })]
    internal static int ABI_Bar(global::System.Runtime.InteropServices.ComWrappers.ComInterfaceDispatch* __this_native, double* __invokeRetValUnmanaged__param)
    {
        global::IFoo @this = default;
        ref double __invokeRetValUnmanaged = ref *__invokeRetValUnmanaged__param;
        double __invokeRetVal = default;
        int __retVal = default;
        try
        {
            // Unmarshal - Convert native data to managed data.
            @this = global::System.Runtime.InteropServices.ComWrappers.ComInterfaceDispatch.GetInstance<global::IFoo>(__this_native);
            __invokeRetVal = @this.Bar();
            // NotifyForSuccessfulInvoke - Keep alive any managed objects that need to stay alive across the call.
            __retVal = 0; // S_OK
            // Marshal - Convert managed data to native data.
            __invokeRetValUnmanaged = __invokeRetVal;
        }
        catch (global::System.Exception __exception)
        {
            __retVal = global::System.Runtime.InteropServices.Marshalling.ExceptionAsHResultMarshaller<int>.ConvertToUnmanaged(__exception);
        }

        return __retVal;
    }
}

We want to override that

__retVal = global::System.Runtime.InteropServices.Marshalling.ExceptionAsHResultMarshaller<int>.ConvertToUnmanaged(__exception);

@jkotas
Copy link
Member

jkotas commented Nov 4, 2024

What about properties that return strings or other types that need marshalling? I can write this today:

public partial interface IFoo
{
    [return: MarshalUsing(typeof(BStrStringMarshaller))]
    string Bar();
}

If I want to override the exception marshaling too, do I specify two return marshalling directives? Is it always going to non-ambiguous how the two marshalling directives should be applied?

@Sergio0694
Copy link
Contributor Author

Sergio0694 commented Nov 4, 2024

I see what you mean. Right, that syntax was meant to be an example, but the real thing would likely need some other attribute targeting methods that would allow to indicate an exception marshaller. I just don't know which exact API surface it would require. If we want to brainstorm what an actual API proposal to bring to review would look like, I'd imagine a starting point could maybe be:

namespace System.Runtime.InteropServices.Marshalling
{
    [AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
    public sealed class ExceptionMarshallerAttribute : Attribute
    {
        public ExceptionMarshallerAttribute(MarshalMode marshalMode, Type marshallerType);

        public MarshalMode MarshalMode { get; }
        public Type MarshallerType { get; }
    }
}

And then you'd use it as follows?

public partial interface IFoo
{
    [ExceptionMarshaller(MarshalMode.ManagedToUnmanagedIn, typeof(ExceptionHelpersHResultMarshaller<int>))]
    void Bar();
}

cc. @jkoritzinsky for thoughts, since you also mentioned we'd need a new API for this likely 🙂

EDIT: not entirely sure if we need MarshalMode, since it seems implied by the context? Maybe this is sufficient?

namespace System.Runtime.InteropServices.Marshalling
{
    [AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
    public sealed class ExceptionMarshallerAttribute : Attribute
    {
        public ExceptionMarshallerAttribute(Type marshallerType);

        public Type MarshallerType { get; }
    }
}

And then you just do:

public partial interface IFoo
{
    [ExceptionMarshaller(typeof(ExceptionHelpersHResultMarshaller<int>))]
    void Bar();
}

@dongle-the-gadget
Copy link

I believe we should have a default exception handler for IErrorInfo, as well, since it looks like generated COM doesn't support that yet.

@jkoritzinsky
Copy link
Member

jkoritzinsky commented Jan 28, 2025

We've talked about this on the team and here's the API shape we thought of:

class GeneratedComInterfaceAttribute
{
+     public Type? ExceptionToUnmanagedMarshaller { get; set; }
}

This marshaller would be used to marshal an Exception to the unmanaged return value. We want to put it on the interface as a whole as it's very likely that a given interface wants to handle errors consistently, and we feel that COM interfaces that use PreserveSig with a non-HRESULT return type is rare enough that it wouldn't overlap with the scenarios for this property, at least for V1.

A marshaller type that is assigned to ExceptionToUnmanagedMarshaller would need to have the UnmanagedToManagedOut marshal mode, with a managed type of Exception and an unmanaged type of the unmanaged return of the methods on the interface (so int 99% of the time).

If we feel that the non-int scenarios are common enough, we can revise this API shape and add a member-level option.

I believe we should have a default exception handler for IErrorInfo, as well, since it looks like generated COM doesn't support that yet.

Source-Generated COM uses the built-in IErrorInfo support, so on CoreCLR we do set the IErrorInfo when you're not using trimming. We don't set it on NativeAOT today. Do you have a scenario where you'd like us to set it in these cases?

@jkoritzinsky jkoritzinsky added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 28, 2025
@AaronRobinsonMSFT
Copy link
Member

Should be Type?, right?

@jkoritzinsky
Copy link
Member

Yep, updating now.

@Sergio0694
Copy link
Contributor Author

This looks good to me, thank you for taking a look! We can add our own marshaller type for this for WinRT scenarios in CsWinRT, and then direct people to using that. We could even add an analyzer to warn if someone uses a generated COM interface on a WinRT type without specifying this marshaller, to help with discoverability. Basically the scenario from this issue would be like:

[Guid("6234C2F7-9917-469F-BDB4-3E8C630598AF")]
[GeneratedComInterface(
    Options = ComInterfaceOptions.ManagedObjectWrapper,
    ExceptionToUnmanagedMarshaller = typeof(ExceptionHelpersHResultMarshaller))]
public partial interface IFoo
{
    void Bar();
}

Where ExceptionHelpersHResultMarshaller would be our own marshaller somewhere in WinRT.*.

@jkoritzinsky what else are we missing to make this proposal ready for review? 🙂

@jkoritzinsky
Copy link
Member

I think we just need to update the top post into the API review format and then we can mark this as api-ready-for-review API is ready for review, it is NOT ready for implementation

@AaronRobinsonMSFT AaronRobinsonMSFT added 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 Jan 29, 2025
@AaronRobinsonMSFT AaronRobinsonMSFT modified the milestones: Future, 10.0.0 Jan 29, 2025
@AaronRobinsonMSFT
Copy link
Member

@jkoritzinsky what else are we missing to make this proposal ready for review? 🙂

Stakeholder sign off :)

@Sergio0694
Copy link
Contributor Author

Thank you! I've updated the first post with the updated API proposal 😄

@bartonjs
Copy link
Member

bartonjs commented Feb 4, 2025

Video

  • Looks good as proposed.
  • For a reference example of what one of these types would be, see ExceptionAsHResultMarshaller<T>
namespace System.Runtime.InteropServices.Marshalling;

public partial class GeneratedComInterfaceAttribute
{
    public Type? ExceptionToUnmanagedMarshaller { get; set; }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Projects
Status: No status
Development

No branches or pull requests

6 participants