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

CoClassAttribute for Interfaces #271

Open
timsneath opened this issue Feb 21, 2021 · 32 comments
Open

CoClassAttribute for Interfaces #271

timsneath opened this issue Feb 21, 2021 · 32 comments
Labels
usability Touch-up to improve the user experience for a language projection

Comments

@timsneath
Copy link
Contributor

Maybe this is my poor understanding of COM, but I think we need some way to map an IID onto a corresponding class.

For example, take the Network List Manager API:
https://docs.microsoft.com/en-us/windows/win32/nla/about-the-network-list-manager-api

To create an INetworkListManager, I need to call CoCreateInstance and create the CLSID_NetworkListManager COM object, which in turn implements INetworkListManager.

From the metadata, I don't think I have a systematic way to identify the matching COM object, do I?

ILSpy just shows:

// Windows.Win32.NetworkListManager.NetworkListManager
using System.Runtime.InteropServices;

[Guid("DCB00C01-570F-4A9B-8D69-199FDBA5723B")]
public struct NetworkListManager
{
}

which gives me the CLSID, but doesn't tell me what it implements (nor vice versa).

Other attempts to wrap this interface seem to include an attribute that is I think derived from tlbimp. For example:
https://github.com/dahall/Vanara/blob/1fb8a2dc8a116995831730a7d433a07f940cc5ef/PInvoke/NetListMgr/NetListMgr.cs#L629

	[TypeLibType(TypeLibTypeFlags.FDual | TypeLibTypeFlags.FDispatchable)]
	[ComImport, Guid("DCB00000-570F-4A9B-8D69-199FDBA5723B"), CoClass(typeof(NetworkListManager))]
	[PInvokeData("Netlistmgr.h", MSDNShortId = "aa370769")]
	public interface INetworkListManager
	{

Is there missing metadata? Or is there another way to derive this relationship?

@mikebattista mikebattista added the question Further information is requested label Feb 21, 2021
@mikebattista
Copy link
Contributor

@AArnott / @sotteson1 / @BenJKuhn what are your thoughts on this?

I just wrote a C#/Win32 COM sample for reference as to how that's projected. @AArnott can speak to how the projection leveraged the metadata to support this or if more metadata would be helpful, but the translation from COM code to C# was very straightforward once you understand some simple C# translations. The original COM code was from here.

@timsneath
Copy link
Contributor Author

Right. I think the thing about that sample is that you're just using the SpellCheckerFactory class for its GUID. It doesn't implement ISpellCheckerFactory or anything. You had to 'know' that the CLSID and IID pair together, since the metadata doesn't include this relationship.

As a very practical example, in my existing projection, I have a class like this, which I can only generate automatically if I know there's a relationship between these two objects (substitute NetworkListManager for SpellCheckerFactory, obviously, but you get the idea).

class NetworkListManager extends INetworkListManager {
  NetworkListManager(Pointer<COMObject> ptr) : super(ptr);

  factory NetworkListManager.createInstance() {
    final ptr = calloc<COMObject>();
    final clsid = calloc<GUID>()..ref.setGUID(CLSID_NetworkListManager);
    final iid = calloc<GUID>()..ref.setGUID(IID_INetworkListManager);

    try {
      final hr = CoCreateInstance(clsid, nullptr, CLSCTX_ALL, iid, ptr.cast());

      if (FAILED(hr)) throw WindowsException(hr);

      return NetworkListManager(ptr);
    } finally {
      calloc.free(clsid);
      calloc.free(iid);
    }
  }
}

@kennykerr
Copy link
Contributor

Yes, I think it would be helpful to have metadata more akin to WinRT where there is a "class" that models the WinRT, or in this case COM, class that provides both the class identity and the default interface. That's why WinRT consumers don't have to call RoGetActivationFactory directly. If we had similar metadata for COM classes, consumers wouldn't need to call CoCreateInstance directly in many cases. It would also deal with the GUID issue more efficiently in most cases.

@mikebattista
Copy link
Contributor

Got it and makes sense. I know @AArnott had some plans/aspirations to make the COM projections real interfaces like what you showed and sounds like this would help with that.

@mikebattista mikebattista added usability Touch-up to improve the user experience for a language projection and removed question Further information is requested labels Feb 22, 2021
@AArnott
Copy link
Member

AArnott commented Feb 22, 2021

I don't think of there being an API-level relationship between INetworkListManager and CLSID_NetworkListManager that the metadata needs to disclose. The former is an API contract that typically takes the form of an interface (a struct in CsWin32 for the time being). The latter is simply a GUID that identifies a COM-registered, activatable object that implements at least IUnknown and maybe other COM interfaces. There is nothing in the API that binds these two together. CLSID_Foo might identify a cocreatable object that implements IBar.
The convention and documentation alone suggest that typically the object that activates by CLSID_Foo will implement IFoo, but the language/API itself does not base anything on that.

At the moment then, the docs tell you the exact names of each (the class ID to activate with, and the interface to interact with it after activation), and the metadata remains true to the interface name so that it matches the docs and headers. But the metadata does not currently reproduce the CLSID_Foo guids. Instead, it apparently strips the CLSID_ prefix from it and defines a struct by that name where the struct has a Guid attribute applied that matches what the CLSID_Foo would have defined.
So in general anytime you see IFoo in the docs or headers, you can use that. And wherever you see CLSID_Foo, in C# you can access it with typeof(Foo).Guid, which is the .NET way for typically accessing such values.

It is a deviation from the docs and header files, to be sure. And for some languages, the projections may want to recreate the CLSID_Foo constants. In C# it's not clear to me which way is better: do it the .NET way, or redefine the constants that people are looking for. Maybe both?

As for enabling a new Foo() syntax that internally calls CoCreateInstance(typeof(Foo).Guid), I don't know that we can generate such a thing with CsWin32. You would want to be able to cast that object to any COM interface that the underlying object actually implements, right? CsWin32 cannot magically do that -- but the CLR can. That's what RCWs do. .NET offers APIs already that can activate COM objects by GUID that you can use today. Once CsWin32 generates COM interfaces as interfaces instead of structs, you'll be able to cast these COM-created objects to them as well and the CLR will do the magic.

@kennykerr
Copy link
Contributor

Languages supporting WinRT including C#, C++, and Rust already do this very thing of stitching classes and their interfaces together from metadata.

@sotteson1
Copy link
Contributor

sotteson1 commented Feb 22, 2021

But the metadata does not currently reproduce the CLSID_Foo guids. Instead, it apparently strips the CLSID_ prefix from it and defines a struct by that name where the struct has a Guid attribute applied that matches what the CLSID_Foo would have defined.

No, the scraper is not doing that. For NetworkListManager, here's what it's scraping:

EXTERN_C const CLSID CLSID_NetworkListManager;

class DECLSPEC_UUID("DCB00C01-570F-4A9B-8D69-199FDBA5723B")
NetworkListManager;

Notice there is no CLSID_NetworkListManager constant to even scrape. It's an external constant and the parser doesn't see its value. But the parser does see an empty class with a GUID, so it emits that.

@sotteson1
Copy link
Contributor

sotteson1 commented Feb 22, 2021

What I do need to add is a way to scrape these:

DEFINE_GUID(CLSID_D2D1LookupTable3D, 0x349E0EDA, 0x0088, 0x4A79, 0x9C, 0xA3, 0xC7, 0xE3, 0x00, 0x20, 0x20, 0x20);

That guid acts like a constant and is used like this:

// Create a 3D LUT effect to render our LUT.
CComPtr<ID2D1Effect> sp3dLutEffect;
IFR(pEffectContext->CreateEffect(CLSID_D2D1LookupTable3D, &sp3dLutEffect)); 

So putting that on a class would be weird. It does seem like it needs to a be field that's on the Direct2D.Api class.

@kennykerr
Copy link
Contributor

Yes those aren't COM classes at all. They're just IDs.

@AArnott
Copy link
Member

AArnott commented Feb 22, 2021

already do this very thing of stitching classes and their interfaces together from metadata.

@kennykerr Can you elaborate on this with an example? Does the projection somehow know the full set of interfaces to implement on the class?

@kennykerr
Copy link
Contributor

Yes, WinRT classes include metadata that indicates every interface that is (publicly) implemented by the class. Here for example is Windows.UI.Composition.Visual. You can see that it derives from the CompositionObject class and implements a variety of interfaces. ILDASM is better at rendering this relationship than ILSPY.

image

image

@kennykerr
Copy link
Contributor

Just to elaborate, projections take the members of these interfaces that a given class implements and makes them available directly on the class as if they were members of the class directly. This is done for all interfaces with the ExclusiveToAttribute.

@sotteson1
Copy link
Contributor

The problem is that the Win32 headers don't give us this information. Like Andrew said, there is nothing that ties a class (represented by a CLSID) in COM to the interfaces it implements.

@timsneath
Copy link
Contributor Author

This is again perhaps where there's a design principle missing between "constitutional originalist" (stick to the headers, no matter what limitations it imposes on the resultant interface), and "constitutional activist" (evolve to create a better outcome than the original).

@sotteson1
Copy link
Contributor

sotteson1 commented Feb 22, 2021

Absolutely, it would be nice to surface the information. But there are lots of these in the headers and figuring out what to emit for them all in a non-trivial task. We are trying to backfill information like this that we don't get from the headers, but we have to prioritize what things we do because we have very limited resources (so far just me!).

@timsneath
Copy link
Contributor Author

Totally :) and thanks for your work. It's definitely a step forward.

Of course, if it's not done at a Win32 level, then it falls to me to do it at the Dart language projection level. So it would be more efficient to try and solve this once and for all, rather than doing it for every language individually.

I wonder if there's a heuristic here though that makes this less scary than it sounds? If there's one of these structs (a DECLSPEC with matching CLSID, check to see if there's a matching interface in the namespace (same name, but with an 'I' prefix). If so, then assume that these two are linked. Would that solve the 99% case (which seems to be a reasonable target, given that none of this is 100%)?

@sotteson1
Copy link
Contributor

sotteson1 commented Feb 22, 2021

Sure, that's one strategy. It would take some research to see how well that works. Like I said, it's a matter of prioritization. If we determine that's at the top of the things to tackle, I'll do it. But COM developers have never had that information available to them before, so if someone is porting some Win32 code from C++ to another language, I don't think the information is strictly necessary. In order to port code from C++ to dart, you would need to give the user the ability to create the object using the class id, and then let them QI (in C# it would be a cast) for the interface. They would also need to be able to QI (cast) to a different interface besides INetworkListManager, like INetworkListManager2. That doesn't exist in this example, but that's a common pattern in COM programming to later support more interfaces as functionality is added in the OS.

@kennykerr
Copy link
Contributor

By the way, a bug I reported on ILSpy was just fixed so you can use the latest CI build of ILSpy to view WinRT metadata now:

image

@sotteson1
Copy link
Contributor

sotteson1 commented Feb 22, 2021

Nice!! I was going to investigate that myself and see if I could fix it. I'm glad to see they fixed it for us.

@AArnott
Copy link
Member

AArnott commented Feb 22, 2021

If so, then assume that these two are linked. Would that solve the 99% case (which seems to be a reasonable target, given that none of this is 100%)?

That would solve the 99% case when that's the only interface implemented by that object. But if there are others, then we've maybe done more harm than good by declaring a class that does only a partial job, rather than just create an IUnknown object and let the app developer cast/QI between interfaces that they know are or may be on that object.
And BTW, suppose we did know the full set of interfaces for a given object and implemented them all--what if different Windows versions implement a subset of those interfaces? By declaring a class that implements them all, we deprive the app of the ability to do an interface check to see whether the functionality is there, and instead I guess they call the method declared on the class and it'll throw when the cast/QI fails.

@weltkante
Copy link

Just for the record, because it seems to be missing from the discussion, the relation between CLSID and IID (for non-WinRT COM) is specified in the IDL files in the Windows SDK. This information does not end up in the MIDL-compiled C/C++ headers deployed in the SDK, but they do end up in the TLB files. Since for a long time the COM projection of the Desktop Framework was based on referencing TLB files they also respected the relationship between CoClass and interfaces. (Personally I never found that link to be important, it helps a bit with discoverability and sometimes write a few less typecasts, but thats it.)

I guess it'll be pretty hard to add to this automatically to metadata without including an IDL parse step, considering the current parsing is done from the C/C++ headers as far as I understand.

Of course, in any case, all relationships between CoClass and interfaces are incomplete by design, its common to implement semi-private interfaces which need to be explicitly queried for.

@AArnott
Copy link
Member

AArnott commented Mar 5, 2021

That all sounds very relevant and interesting. Thank you, @weltkante.

@fengbenming
Copy link

Languages supporting WinRT including C#, C++, and Rust already do this very thing of stitching classes and their interfaces together from metadata.

I still can't find CLSID in rust, when i want to create instance of INetworkListManager. It is hard to find relation between clsid and it's corresponding interface.

@fengbenming
Copy link

Yes, thank you @riverar . But I have a litter doubt that why does not provide CLSID in INetworkListManager, I think CLSID is a part of INetworkListManager.

@mikebattista
Copy link
Contributor

Long discussion but my read is that there's no reliable proposal here and what we have is consistent with the Win32 programming model.

The other issue is with CLSID constants not being prefixed with CLSID. See #271 (comment).

I believe that issue is tracked by #994. Should I close this one out?

@timsneath
Copy link
Contributor Author

That was my initial read. But I read something different from @weltkante, which suggests there is metadata here that we don't parse:

Just for the record, because it seems to be missing from the discussion, the relation between CLSID and IID (for non-WinRT COM) is specified in the IDL files in the Windows SDK

Here's an example of this, showing the mapping between IFileOpenDialog and CLSID_FileOpenDialog:

// CLSID_FileOpenDialog
[ uuid(DC1C5A9C-E88A-4dde-A5A1-60F82A20AEF7) ] coclass FileOpenDialog { interface IFileOpenDialog; }

[
    uuid(d57c7288-d4ad-4768-be02-9d969532d960),
    object,
    pointer_default(unique)
]
interface IFileOpenDialog : IFileDialog
{
    HRESULT GetResults([out] IShellItemArray **ppenum);

    HRESULT GetSelectedItems([out] IShellItemArray **ppsai);
}

@mikebattista
Copy link
Contributor

Of course, in any case, all relationships between CoClass and interfaces are incomplete by design, its common to implement semi-private interfaces which need to be explicitly queried for.

He also said this so that approach doesn't sound complete. There's a lot of other context here as well against this approach.

@timsneath
Copy link
Contributor Author

I guess I'm not clear why we wouldn't want to add this. Is there more context that isn't captured in this issue?

We have metadata in the header files that we can capture that would be of use to projection authors, by identifying known mappings between interfaces and classes. The header files are deterministic and the information is useful.

@riverar
Copy link
Collaborator

riverar commented Apr 19, 2023

Given that interfaces may be used across multiple coclasses, annotating interfaces sounds messy. But annotating the classes sound OK to me.

There's two distinct work items here:

  1. Create the attribute and document its semantics

    Proposed new attribute/usage (XxxAcceptsQIFor is a placeholder name):

    [Guid(3702524929u, 22287, 19099, 141, 105, 25, 159, 219, 165, 114, 59)]
    [XxxAcceptsQIFor("Windows.Win32.Networking.NetworkListManager.INetworkListManager")]
    public struct NetworkListManager
    {
    }
  2. Harvest the existing relationships across the Windows API surface

    Spitballing here, we can harvest the coclass/interface relationships from .tlbs generated during metadata IDL pass. Or, worst case, we use the existing infrastructure to add the new attribute manually.

Challenges that come to mind (please add more):

  • May need to wrap IDLs in a library to get midl to spit out type libraries
  • Need to think about how we handle changes across OS versions. That is, the scenario when coclass C implements mutually exclusively Iflavor1 for OS version V1 but Iflavor2 for OS version V2. (Or when IIDs inadvertently change.) IAudioPolicyConfigFactory comes to mind.

@mikebattista
Copy link
Contributor

TLBs are disabled and have issues in some cases. See #1428.

@kennykerr
Copy link
Contributor

If you want to pursue this, I'd take a look at WinRT metadata which has already dealt with this design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
usability Touch-up to improve the user experience for a language projection
Projects
None yet
Development

No branches or pull requests

8 participants