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

Derived Interface support in ComInterfaceGenerator #84271

Merged
merged 4 commits into from
Apr 7, 2023

Conversation

jkoritzinsky
Copy link
Member

Implement support for deriving COM interfaces to append their members to the vtables of their base interface type.

This PR also includes a design doc about the derived-interface feature set. This PR implements the first part of the design (it does not implement the "Designing for Performance" section of the spec, as implementing this section is not a breaking change (whereas the first part of the design is).

Implement support for deriving COM interfaces to append their members to the vtables of their base interface type.
@ghost
Copy link

ghost commented Apr 3, 2023

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Implement support for deriving COM interfaces to append their members to the vtables of their base interface type.

This PR also includes a design doc about the derived-interface feature set. This PR implements the first part of the design (it does not implement the "Designing for Performance" section of the spec, as implementing this section is not a breaking change (whereas the first part of the design is).

Author: jkoritzinsky
Assignees: -
Labels:

area-System.Runtime.InteropServices, source-generator

Milestone: -

…terface to ensure we don't break later steps.
@@ -66,32 +68,84 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
var interfacesToGenerate = interfacesWithDiagnostics.Where(static data => data.Diagnostic is null);
var invalidTypeDiagnostics = interfacesWithDiagnostics.Where(static data => data.Diagnostic is not null);

var interfaceContexts = interfacesToGenerate.Select((data, ct) =>
var interfaceBaseInfo = interfacesToGenerate.Collect().SelectMany((data, ct) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought, for one pass it probably is better to Collect this and be able to cache the counts, but during incremental passes, this will make us recalculate offsets for all interfaces when 1 is changed, right? Might be premature optimization to change it to something else now, but we could make a note about it for the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather do the cache right away as doing this without a cache becomes an O(n^2) algorithm almost immediately and heavily derived interface types aren't uncommon (DirectX has cases where they're up to ID3D12Device9 and we have some examples of in dotnet/runtime that go pretty deep like ISOSDacInterface, which is up to ISOSDacInterface13).

Copy link
Member

@jtschuster jtschuster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!


In the ComInterfaceGenerator, we want to improve the experience when writing COM interfaces that derive from other COM interfaces. The built-in system has some quirks due to differences between how interfaces work in C# in comparison to COM interfaces in C++.

## COM Interface Inheritance in C++
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on adding the sections about how it works in C++ and .NET ComImport (and then GeneratedComInterface once it ships) to public docs? Probably somewhere under https://learn.microsoft.com/dotnet/standard/native-interop/cominterop

I think the descriptions you have here are clear and useful. I don't think we have this called out or explained anywhere in official docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea! I'll make sure to add it to the docs.

@jkoritzinsky
Copy link
Member Author

Test failures are all known.

@jkoritzinsky jkoritzinsky merged commit 35f5a56 into dotnet:main Apr 7, 2023
@jkoritzinsky jkoritzinsky deleted the iface-inherit branch April 7, 2023 22:18
@ghost ghost locked as resolved and limited conversation to collaborators May 8, 2023
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants