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

GeneratedComInterfaceAttribute can cause CS0108 error with two derived COM interfaces declaring methods with same name and signature #101240

Closed
smourier opened this issue Apr 18, 2024 · 6 comments · Fixed by #101577
Assignees
Labels
area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature
Milestone

Comments

@smourier
Copy link

smourier commented Apr 18, 2024

Description

When using .NET8's GeneratedComInterfaceAttribute, we cannot define a derived interface with a method that has the same name and signature (parameters) as a method (unrelated in vtable) on a base interface, it raises CS0108 "'member1' hides inherited member 'member2'. Use the new keyword if hiding was intended."

Reproduction Steps

Consider this simple .NET 8 project:

<Project Sdk="Microsoft.NET.Sdk">

	<PropertyGroup>
		<OutputType>Exe</OutputType>
		<TargetFramework>net8.0</TargetFramework>
		<Nullable>enable</Nullable>
		<PublishAot>true</PublishAot>
		<InvariantGlobalization>true</InvariantGlobalization>
		<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
	</PropertyGroup>

</Project>

Add these interfaces definitions:

    [GeneratedComInterface, Guid("e4817011-64d7-4626-a2ed-ac0cf6ad76e7")]
    public partial interface IFace1
    {
        void Blah(nint p);
    }

    [GeneratedComInterface, Guid("5fdae1bd-24db-43cd-88e5-3825fac0f827")]
    public partial interface IFace2 : IFace1
    {
        void Blah2(nint p);

        void Blah(nint p); // this is where the problem lies (same name and same parameters as in IFace1)
    }

Expected behavior

I think it should just compile fine.

Actual behavior

Building creates these errors (all related):

1> c:\temp\ConsoleApp6.Program.IFace2.cs(31,45,31,49): warning CS0473: Explicit interface implementation 'InterfaceImplementation.Program.IFace2.Blah(nint)' matches more than one interface member. Which interface member is actually chosen is implementation-dependent. Consider using a non-explicit implementation instead.
1> c:\temp\ConsoleApp6.Program.IFace2.cs(46,45,46,49): warning CS0473: Explicit interface implementation 'InterfaceImplementation.Program.IFace2.Blah(nint)' matches more than one interface member. Which interface member is actually chosen is implementation-dependent. Consider using a non-explicit implementation instead.
1> c:\temp\ConsoleApp6.Program.IFace2.cs(12,31,12,54): error CS8646: 'Program.IFace2.Blah(nint)' is explicitly implemented more than once.
1> c:\temp\ConsoleApp6.Program.IFace2.cs(46,45,46,49): error CS0111: Type 'InterfaceImplementation' already defines a member called 'global::ConsoleApp6.Program.IFace2.Blah' with the same parameter types
1>E:\smo\work\ConsoleApp33\ConsoleApp6\Program.cs(51,18,51,22): warning CS0108: 'Program.IFace2.Blah(nint)' hides inherited member 'Program.IFace1.Blah(nint)'. Use the new keyword if hiding was intended.
1> c:\temp\ConsoleApp6.Program.IFace2.cs(144,22,144,26): error CS0111: Type 'Program.IFace2' already defines a member called 'Blah' with the same parameter types
1>Done building project "ConsoleApp6.csproj" -- FAILED.

Regression?

I don't think so since GeneratedComInterfaceAttribute is pretty new.

Known Workarounds

We can just rename IFace2's Blah with something else that doesn't conflict as COM names are not technically important but this raises lots of other issues, especially with .NET where names are.

Note that if if define IFace2's like this:

    [GeneratedComInterface, Guid("5fdae1bd-24db-43cd-88e5-3825fac0f827")]
    public partial interface IFace2 : IFace1
    {
        void Blah2(nint p);

        new void Blah(nint p);
    }

There are other errors, but the most interesting is this:

1>c:\temp\ConsoleApp6.Program.IFace2.cs(31,49,31,53): error CS0106: The modifier 'new' is not valid for this item

which is kinda strange, this typically works with .NET default marshaling.

Configuration

.NET 8.0.4 (tested on x64 but I don't think it's relevant here).

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 18, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Apr 18, 2024
@vcsjones vcsjones removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 19, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT added area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature and removed area-Interop-coreclr labels Apr 19, 2024
@AaronRobinsonMSFT
Copy link
Member

@smourier My gut reaction here is, this is by design. If the interface is inheriting from a base interface what is the purpose of redeclaring a function with the same name? What is the relevant COM scenario that is being attempted here? Note that the new COM source generator is specifically designed to follow and enforce COM rules.

@AaronRobinsonMSFT AaronRobinsonMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 19, 2024
@smourier
Copy link
Author

smourier commented Apr 19, 2024

@smourier My gut reaction here is, this is by design. If the interface is inheriting from a base interface what is the purpose of redeclaring a function with the same name? What is the relevant COM scenario that is being attempted here? Note that the new COM source generator is specifically designed to follow and enforce COM rules.

The first reason is this is valid in COM. Nothing prevents you from doing this. Now, here is a real use case, consider this COM interface IDWriteTextLayout3 https://learn.microsoft.com/en-us/windows/win32/api/dwrite_3/nn-dwrite_3-idwritetextlayout3

It derives (along the line) from IDWriteTextLayout. IDWriteTextLayout has this function GetLineMetrics defined like this in the SDK:

HRESULT GetLineMetrics([out, optional] DWRITE_LINE_METRICS *lineMetrics, UINT32 maxLineCount, [out]UINT32 *actualLineCount);

IDWriteTextLayout3 defines another GetLineMetrics like this:

HRESULT GetLineMetrics([out] DWRITE_LINE_METRICS1 *lineMetrics, UINT32 maxLineCount, [out] UINT32 *actualLineCount);

The two first parameters are not of the same type in native world (DWRITE_LINE_METRICS*vs DWRITE_LINE_METRICS1*), but in .NET world, if you want to keep them optional (ok the second is not defined in the header as optional but that doesn't change the discussion, and I'm pretty confident it is optional too), it would be logical to define both lineMetrics parameters as IntPtrs, which the current source generator doesn't support (if I don't want to use unsafe raw * pointers)

More generally you don't always have a way to discriminate between two methods with same name in .NET like you can do with the native definition, it's not always a 1-1 mapping.

@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 19, 2024
@AaronRobinsonMSFT
Copy link
Member

The first reason is this is valid in COM.

I didn't realize that the same name was valid across inheritance. I think the issue I'm thinking of is function overloading on the same interface.

As a general note, the DirectDraw interfaces and just about everything in the graphics domain are not technically COM compliant. This is easy to see with the number of times they violate the core requirement of returning an HRESULT. This is violated all over the place and therefore they really aren't COM, but rather IUnknown ABI compliant.

Regardless of the above, the ComWrappers API, despite its name, should really have been called IUnknownWrappers, so I accept the request here. I'd hesitant to do anything that blocks users from providing wrappers for these interfaces so let's see what we can do.

/cc @jkoritzinsky and @jtschuster

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Apr 19, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT modified the milestones: 9.0.0, Future Apr 19, 2024
@AaronRobinsonMSFT
Copy link
Member

which is kinda strange, this typically works with .NET default marshaling.

A quick note on this statement. This assumption has no relation to the new COM source generator. We have intentionally broke with the built-in system because of the complexity and issues it created. This new approach definitely takes influence from the built-in system and endevours to provide similar utility, but it will deviate when the decision introduces confusion or oddities.

@smourier
Copy link
Author

As a general note, the DirectDraw interfaces and just about everything in the graphics domain are not technically COM compliant. This is easy to see with the number of times they violate the core requirement of returning an HRESULT. This is violated all over the place and therefore they really aren't COM, but rather IUnknown ABI compliant.

Well, I won't fight that battle myself, but the necessity of HRESULT is very related to the remotability of the COM interface in question and in the graphics domain, they are usually not. If you look into old COM specs, it's not expressed as strict as a "core requirement". In fact, you're (still) supposed to be ok w/o HRESULT if you use midl's [local] attribute which is the case for example with ID3D11Device in d3d11.idl.

Does that mean ID3D11Device is not a sanctified COM interface? or worst, that you shouldn't support it when generating .NET code? If you want to get rid of the legacy built-in system (I do! hence this issue) you should support many existing weird constructs, IMHO :-)

PS: I must admin the DWrite guys are the real rebels: no idl and funky .h headers :-) and ITextServices is the weirdest one: it's was not even defined with stdcall methods...

@AaronRobinsonMSFT
Copy link
Member

If you want to get rid of the legacy built-in system (I do! hence this issue) you should support many existing weird constructs, IMHO :-)

Yes, we do and that is the long term plan. What we are currently discussing here is how to support the weird constructs. A simple workaround, as you noted, would be to rename the method to Blah2, or some other name. This is how overloading in COM works when done on the same interface. When done through inheritance it seems this is permissable but the definition in C++ of the IDWriteTextLayout and IDWriteTextLayout3 types shows where this is leaky from the perspective of the MIDL compiler. The DirectDraw team went out of their way to do something odd and it pains me to codify this sort of nonsense in the source generator tool.

That being said, I think @jkoritzinsky and @jtschuster are clever enough to come up with some affordance to make this possible. For example, since the signatures are different in this case, I could see an argument being made that we enforce that in C# as well. Doesn't address all issues but since there is another workaround with renaming that seems reasonable to me. I'm going to look to them for how we address this though.

PS: I must admin the DWrite guys are the real rebels: no idl and funky .h headers :-) and ITextServices is the weirdest one: it's was not even defined with stdcall methods...

Exactly. Many of these interfaces are not COM compliant, which created subtle and incredibly bad design decisions in the built-in COM interop system. We are not going to be repeating these decisions, but will ensure there are reasonable and hopefully intuitive workarounds.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants