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]: Add StringMarshalling property to GeneratedComInterfaceAttribute #84662

Closed
3 of 4 tasks
jtschuster opened this issue Apr 11, 2023 · 8 comments · Fixed by #87065
Closed
3 of 4 tasks

[API Proposal]: Add StringMarshalling property to GeneratedComInterfaceAttribute #84662

jtschuster opened this issue Apr 11, 2023 · 8 comments · Fixed by #87065
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Milestone

Comments

@jtschuster
Copy link
Member

jtschuster commented Apr 11, 2023

Background and motivation

GeneratedComInterface does not support string marshalling with a StringMarshalling enum. To match the functionality of LibraryImport, GeneratedComInterface should also allow specifying a StringMarshalling value and an accompanying StringMarshallingCustomType. This would apply for all methods in the interface, as well as methods on interfaces that inherit from it if the inheriting interfaces do not specify their own StringMarshalling.

API Proposal

namespace System.Runtime.InteropServices.Marshalling;

[AttributeUsage(AttributeTargets.Interface)]
public class GeneratedComInterfaceAttribute : Attribute
{
    public StringMarshalling StringMarshalling { get; set; }
    public Type? StringMarshallingCustomType { get; set; }
}

API Usage

Example from crossgen2:

    [GeneratedComInterface(StringMarshalling = StringMarshalling.Utf16)]
    [Guid("B029E51B-4C55-4fe2-B993-9F7BC1F10DB4")]
    internal partial interface ISymNGenWriter2 : ISymNGenWriter
    {
        void OpenModW(
                      string wszModule,
                      string wszObjFile,
                      out UIntPtr ppmod);

        void CloseMod(UIntPtr pmod);

        void ModAddSymbols(UIntPtr pmod, [MarshalAs(UnmanagedType.LPArray)] byte[] pbSym, int cb);

        void ModAddSecContribEx(
            UIntPtr pmod,
            ushort isect,
            int off,
            int cb,
            uint dwCharacteristics,
            uint dwDataCrc,
            uint dwRelocCrc);

        void QueryPDBNameExW(
            [MarshalUsing(CountElementName = nameof(cchMax))]
            char[] pdb,
            IntPtr cchMax);
    }

Alternative Designs

We could do a per-method string marshalling attribute, or continue using per-parameter MarshalAs / MarshaUsing attributes.

Risks

The ComInterfaceGenerator is new, so there aren't risks of regressions or breaking changes. No other risks come to mind.

Steps

@jtschuster jtschuster added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices labels Apr 11, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 11, 2023
@ghost
Copy link

ghost commented Apr 11, 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

GeneratedComInterface does not support string marshalling with a StringMarshalling enum. To match the functionality of LibraryImport, GeneratedComInterface should also allow specifying a StringMarshalling value. This would apply for all methods in the interface, as well as methods on interfaces that inherit from it if the inheriting interfaces do not specify their own StringMarshalling.

API Proposal

namespace System.Runtime.InteropServices.Marshalling;

[AttributeUsage(AttributeTargets.Interface)]
public class GeneratedComInterfaceAttribute : Attribute
{
    public StringMarshalling StringMarshalling { get; set; }
}

API Usage

Example from crossgen2:

    [GeneratedComInterface(StringMarshalling = StringMarshalling.Utf16)]
    [Guid("B029E51B-4C55-4fe2-B993-9F7BC1F10DB4")]
    internal partial interface ISymNGenWriter2 : ISymNGenWriter
    {
        void OpenModW(
                      string wszModule,
                      string wszObjFile,
                      out UIntPtr ppmod);

        void CloseMod(UIntPtr pmod);

        void ModAddSymbols(UIntPtr pmod, [MarshalAs(UnmanagedType.LPArray)] byte[] pbSym, int cb);

        void ModAddSecContribEx(
            UIntPtr pmod,
            ushort isect,
            int off,
            int cb,
            uint dwCharacteristics,
            uint dwDataCrc,
            uint dwRelocCrc);

        void QueryPDBNameExW(
            [MarshalUsing(CountElementName = nameof(cchMax))]
            char[] pdb,
            IntPtr cchMax);
    }

Alternative Designs

We could do a per-method string marshalling attribute, or continue using per-parameter MarshalAs / MarshaUsing attributes.

Risks

The ComInterfaceGenerator is new, so there aren't risks of regressions or breaking changes. No other risks come to mind.

Author: jtschuster
Assignees: -
Labels:

api-suggestion, area-System.Runtime.InteropServices

Milestone: -

@jtschuster jtschuster removed the untriaged New issue has not been triaged by the area owner label Apr 11, 2023
@jkoritzinsky

This comment was marked as resolved.

@jkoritzinsky
Copy link
Member

Also, I'm unsure if we want to propagate the setting across inheritance boundaries. While it's likely correct, it goes more towards the "spooky-action-at-a-distance" model that we've been trying to actively avoid with source-generated interop. Also, in the past we've discussed introducing an assembly-level attribute to specify the string marshalling rules, which would then conflict with the inheritance-based model if the base type was in a different assembly.

I'd be interested to hear from the rest of the team as well as I might be in the minority here.

@jtschuster
Copy link
Member Author

Those are good points, and I think also it could be confusing if we generate shadowing methods for the base interfaces. Maybe it would be best to force StringMarshalling to be the same as the base interface? And if anyone needs different string marshalling than the base, they can use MarshalUsing/MarshalAs.

@AaronRobinsonMSFT
Copy link
Member

While it's likely correct, it goes more towards the "spooky-action-at-a-distance" model that we've been trying to actively avoid with source-generated interop

Maybe it would be best to force StringMarshalling to be the same as the base interface?

Agreed, and we can easily detect that in the analyzer and force propagation of the setting throughout the heirarchy. The gist is specify it once, it must be specified, and match, for everything below. Anything above should be "okay" because we require it if strings are involved so we should be set. It is sane and matches our expectations for being explicit about string encoding.

@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 8.0.0 milestone Apr 12, 2023
@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 Apr 12, 2023
@bartonjs
Copy link
Member

bartonjs commented Apr 27, 2023

Video

Looks good as proposed

namespace System.Runtime.InteropServices.Marshalling;

[AttributeUsage(AttributeTargets.Interface)]
public partial class GeneratedComInterfaceAttribute : Attribute
{
    public StringMarshalling StringMarshalling { get; set; }
    public Type? StringMarshallingCustomType { 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 Apr 27, 2023
@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Apr 27, 2023 via email

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Apr 27, 2023 via email

@jkoritzinsky jkoritzinsky added the in-pr There is an active PR which will close this issue when it is merged label Jun 13, 2023
jtschuster added a commit that referenced this issue Jun 14, 2023
…shallingCustomType (#87065)

The generated code is in file scoped classes, so the interface and StringMarshallingCustomType need to be at least internal visibility. These changes warn if that condition isn't met.

Originally, I expected to report a diagnostic, but still generate code for the interface. This required some changes to DiagnosticOr<T> to allow it to hold both a diagnostic and a value if the diagnostic didn't cause failure. In the end those changes weren't necessary, but I left them because I can see them being valuable at some point in the future.

Completes #84662
@jkoritzinsky jkoritzinsky removed the in-pr There is an active PR which will close this issue when it is merged label Jun 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 14, 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
Projects
None yet
4 participants