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]: Expose options to only generate one direction for generated COM interfaces #86360

Closed
jkoritzinsky opened this issue May 16, 2023 · 10 comments · Fixed by #87831
Closed
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 16, 2023

Background and motivation

Currently, users must provide support in custom marshallers for [GeneratedComInterface] APIs for both managed-to-unmanaged and unmanaged-to-managed. Many COM interfaces are meant to be consumed from managed code either by calling into a COM component implemented elsewhere or implementing a COM component that will be called from non-.NET code. This proposal adds members on the [GeneratedComInterface] attribute to support turning off either the managed->unmanaged or the unmanaged->managed direction of code-gen.

API Proposal

namespace System.Runtime.InteropServices.Marshalling;

+ [Flags]
+public enum ComInterfaceOptions
+ {
+     ManagedObjectWrapper = 0x1,
+     ComObjectWrapper = 0x2,
+ }

[AttributeUsage(AttributeTargets.Interface)]
public sealed class GeneratedComInterfaceAttribute
{
+    public ComInterfaceOptions Options { get; } = ComInterfaceOptions.ManagedObjectWrapper | ComInterfaceOptions.ComObjectWrapper;
}

API Usage

[GeneratedComInterface(Options = ComInterfaceOptions.ManagedObjectWrapper)]
[Guid("e7ba7579-49ec-4d6a-905b-87fb4e476e36")]
interface IFoo
{
     void Bar();
}

Alternative Designs

  • Continue to require developers to handle marshallers going in both directions.
  • Use two bool properties instead of an enum.

Risks

No response

@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 16, 2023
@jkoritzinsky jkoritzinsky added this to the 8.0.0 milestone May 16, 2023
@ghost
Copy link

ghost commented May 16, 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

Currently, users must provide support in custom marshallers for [GeneratedComInterface] APIs for both managed-to-unmanaged and unmanaged-to-managed. Many COM interfaces are meant to be consumed from managed code either by calling into a COM component implemented elsewhere or implementing a COM component that will be called from non-.NET code. This proposal adds members on the [GeneratedComInterface] attribute to support turning off either the managed->unmanaged or the unmanaged->managed direction of code-gen.

API Proposal

namespace System.Runtime.InteropServices.Marshalling;

[AttributeUsage(AttributeTargets.Interface)]
public class GeneratedComInterfaceAttribute
{
+    public bool GenerateManagedObjectWrapper { get; } = true;

+    public bool GenerateComObjectWrapper { get; } = true;
}

API Usage

[GeneratedComInterface(GenerateManagedObjectWrapper = false)]
[Guid("e7ba7579-49ec-4d6a-905b-87fb4e476e36")]
interface IFoo
{
     void Bar();
}

Alternative Designs

Continue to require developers to handle marshallers going in both directions.

Risks

No response

Author: jkoritzinsky
Assignees: -
Labels:

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

Milestone: 8.0.0

@Sergio0694
Copy link
Contributor

Love this 😄

Just out of curiosity, would there be any benefits other than binary size reduction from turning off either marshalling side, or would it be just for that? Not that it'd be a bad thing, just wondering if disabling either of those properties would also being any wins for perf or other things 🙂

@jkoritzinsky
Copy link
Member Author

No it’s really just to generate less code. There’s no other improvements this would provide.

@AaronRobinsonMSFT
Copy link
Member

I'm fine with two bool values, but is there any argument for an enum?

@jtschuster
Copy link
Member

jtschuster commented May 18, 2023

I generally would prefer enums, my only thought is to avoid "ManagedToUnmanaged" and "UnmanagedToManaged" enum values. We usually mean that in the caller to callee, but with COM object, a runtime object passed to COM is passed from "Managed to Unmanaged", but then is called from Unmanaged to Managed, so that could be confusing IMO.

@jkoritzinsky
Copy link
Member Author

I've updated the proposal to use an enum. Let me know what you think of the naming/shape!

@Sergio0694
Copy link
Contributor

I like it! Nit: I feel the "Generated" prefix on the enum isn't strictly necessary, and removing it might make the type potentially more general, which might or might not be potentially useful in other scenarios in the future? Other than that LGTM 😄

@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 Jun 1, 2023
@bartonjs
Copy link
Member

bartonjs commented Jun 15, 2023

Video

namespace System.Runtime.InteropServices.Marshalling;

[Flags]
public enum ComInterfaceOptions
{
     None = 0,
     ManagedObjectWrapper = 0x1,
     ComObjectWrapper = 0x2,
}

[AttributeUsage(AttributeTargets.Interface)]
public sealed partial class GeneratedComInterfaceAttribute
{
    public ComInterfaceOptions Options { get; set; } = ComInterfaceOptions.ManagedObjectWrapper | ComInterfaceOptions.ComObjectWrapper;
}

@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 Jun 15, 2023
@Sergio0694
Copy link
Contributor

Just watched the API review — shouldn't the setter be init?

@jkoritzinsky
Copy link
Member Author

It looks like for attributes, we usually just do get; set;, so the setter is correct.

@jkoritzinsky jkoritzinsky self-assigned this Jun 19, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 20, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 21, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 22, 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.

5 participants