COM interfaces are not declared with the `partial` keyword

[jnm2]

0.1.506-beta

I wanted to try something out by writing this:

namespace Windows.Win32.UI.Shell
{
    [CoClass(typeof(ShellLink))]
    internal partial interface IShellLinkW { }
}

But the IShellLinkW.g.cs declaration does not use the partial keyword:

[Guid("000214F9-0000-0000-C000-000000000046"),InterfaceType(ComInterfaceType.InterfaceIsIUnknown),ComImport()]internal interface IShellLinkW
{

[AArnott]

Using partial on a COM interface would lead to undefined member ordering inside the interface, which would be disastrous. I get it in your case you just want to add attributes, but that's a very narrow case for using partial and IMO the risk that folks might use it for other purposes and end up corrupting the vtable would be huge.
Let's keep your other issue to track adding the attributes you want on the interface within the generated code.

[jnm2]

I think the entire risk could be mitigated with a new source generator error if any user-defined parts contain (non-type) members. But maybe the use case of adding attributes is too niche to be worth it.

[AArnott]

Honestly, a C# compiler-level warning when methods are added to two parts of a partial interface that includes interop attributes would probably be appropriate, IMO. If the compiler emits one (maybe it already does), then I wouldn't object to this suggestion. Can you test whether it already does, and if it doesn't, maybe file a bug at https://github.com/dotnet/csharplang?

[jnm2]

There is no warning (sharplab).

Since this affects existing C#, I expect they will only want to add this in a warning wave. Things like this tend to be designed at dotnet/roslyn rather than dotnet/csharplang, which is good news because only a few things that require a spec change ever ship and it's usually years out.

[jnm2]

I checked on the repo contribution Discord channel and Fred confirmed that this would probably be a feature request for the Roslyn repo. https://github.com/dotnet/roslyn/issues/55006

[elachlan]

@kant2002 does your COM Wrapper work in winforms need a partial interface at all? I believe winforms is going to use cswin32 without marshalling, so the interfaces will not be generated? Just making sure this doesn't end up blocking your work.

[kant2002]

I’m was not able to properly check current state of CsWin32 , but in general I never use partials for COM interface declaration so I’m fine with that. What’s important is that DllImport gives IntPtr for interface parameters. All I need is control of how COM proxy is created.

With DllImport + IntPtr parameter I do that manually. With LibraryImport + IComInterface I think it’s possible to provide marshalers so I can control that part.

Need more time to properly get context to answer you more targeted

[elachlan]

Thanks, we should know more when they start to look at it.

[kant2002]

Quick glance

From what I see on feature/wincs32

• DllImport and not LibraryImport (given how DllImport used, I think switch to LibraryImport when allowMarshaling = false is quite possible. Without that it indirectly affects WinForms or any app who want trim.
• comInterop.useIntPtrForComOutPointers is interesting. With comInterop.useIntPtrForComOutPointers = true I see how current codegen can be used almost identically for current approach, but then we need additional comInterop.useIntPtrForComInPointers, or is there assumption that IntPtr returned by ComWrappers.GetOrCreateComInterfaceForObject should be blindly converted to, for example, winmdroot.System.Com.IUnknown*?
• struct + Vtbl as interfaces. I do remember that I was suggested by Jan Kotas or Aaron that instead of creatin VTBL structures I directly operate on pointers. Otherwise it produce a lot of metadata. Maybe recent changes in P7 by @MichalStrechovsky trim that out, but otherwise it looks like additional bloat.

RCW

Currently RCW build like this.

public HRESULT Init(IntPtr hwndEdit, IEnumString punkACL, IntPtr pwszRegKeyPath, IntPtr pwszQuickComplete)
{
	var punkACLPtr = WinFormsComWrappers.Instance.GetOrCreateComInterfaceForObject(punkACL, CreateComInterfaceFlags.None);
	return ((delegate* unmanaged<IntPtr, IntPtr, IntPtr, IntPtr, IntPtr, HRESULT>)(*(*(void***)_wrappedInstance + 3)))
			(_wrappedInstance, hwndEdit, punkACLPtr, pwszRegKeyPath, pwszQuickComplete);
}

With current autogenerated structs

IAutoComplete2* instance;
public HRESULT Init(IntPtr hwndEdit, IEnumString punkACL, IntPtr pwszRegKeyPath, IntPtr pwszQuickComplete)
{
	var punkACLPtr = WinFormsComWrappers.Instance.GetOrCreateComInterfaceForObject(punkACL, CreateComInterfaceFlags.None);
	return instance->Init(hwndEdit, (winmdroot.System.Com.IUnknown*)punkACLPtr, (winmdroot.Foundation.PCWSTR)pwszRegKeyPath, (winmdroot.Foundation.PCWSTR)pwszQuickComplete);
}

or

IAutoComplete2* instance;
public HRESULT Init(IntPtr hwndEdit, IEnumString punkACL, string pwszRegKeyPath, string pwszQuickComplete)
{
	var punkACLPtr = WinFormsComWrappers.Instance.GetOrCreateComInterfaceForObject(punkACL, CreateComInterfaceFlags.None);
	return instance->Init(hwndEdit, (winmdroot.System.Com.IUnknown*)punkACLPtr, pwszRegKeyPath, pwszQuickComplete);
}

Still require manual passing all parameters. AddRef/Release/QI function is pointless because they never would be used.

CCW

Currently CCW build like this

[UnmanagedCallersOnly]
private static int Seek(IntPtr thisPtr, long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition)
{
	try
	{
		Interop.Ole32.IStream instance = ComInterfaceDispatch.GetInstance<Interop.Ole32.IStream>((ComInterfaceDispatch*)thisPtr);
		instance.Seek(dlibMove, dwOrigin, plibNewPosition);
		return S_OK;
	}
	catch (Exception ex)
	{
		Debug.WriteLine(ex);
		return ex.HResult;
	}
}

if allowMarshaling = true then this can be used like this. Also here the reason why partial is not needed.

[UnmanagedCallersOnly]
private static int Seek(IntPtr thisPtr, long dlibMove, winmdroot.System.Com.STREAM_SEEK dwOrigin, ulong* plibNewPosition)
{
	try
	{
		Windows.Win32.System.Com.IStream instance = ComInterfaceDispatch.GetInstance<Windows.Win32.System.Com.IStream>((ComInterfaceDispatch*)thisPtr);
		instance.Seek(dlibMove, dwOrigin, plibNewPosition);
		return S_OK;
	}
	catch (Exception ex)
	{
		Debug.WriteLine(ex);
		return ex.HResult;
	}
}

not much of a difference.

Additional comments

So basically for one scenario I need structs, for another interface.
Maybe instead of structs if allowMarshaling = true, can be generated just Vtbl part that way I can write code for RCW like this.

IAutoComplete2Vtbl** instance;
public HRESULT Init(IntPtr hwndEdit, IEnumString punkACL, IntPtr pwszRegKeyPath, IntPtr pwszQuickComplete)
{
	var punkACLPtr = WinFormsComWrappers.Instance.GetOrCreateComInterfaceForObject(punkACL, CreateComInterfaceFlags.None);
	return (*instance)->Init((IntPtr)instance, hwndEdit, (winmdroot.System.Com.IUnknown*)punkACLPtr, (winmdroot.Foundation.PCWSTR)pwszRegKeyPath, (winmdroot.Foundation.PCWSTR)pwszQuickComplete);
}

I may screw with * here, but that's for illustrative purposes for now.

[AArnott]

@kant2002, thank you for your write-up.

• Taking IntPtr for input parameters in lieu of COM interfaces should be possible. If that's important to you, can you file a new issue to track that? Please comment on Offer option to generate IntPtr input parameters instead of COM interfaces #669 to carry on this discussion.
• Yes, the struct vtbl we generate today isn't particularly 'trimming friendly', and we expect to switch to the pointers+casting approach you refer to in the future. This is now tracked by Non-marshaling COM structs and vtbl fields to be replaced with trim-friendly approach #668.

I think I'll need more time to study the rest of your comments.

[AArnott]

I re-read this whole thread. I'm not clear on whether the partial modifier added to the interface or the struct code for a COM interface is useful or not, as it seems @kant2002 was making the case both for and against. As that seems to be the last request uniquely described by this issue, can anyone clarify on requirements here? Otherwise, I'll close the issue as Won't Fix in a few days. Of course feedback after that on this issue would be welcome and we could reactivate if needed.