-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add opt-in support for GeneratedComInterface/ComImport RCW interop #87583
Add opt-in support for GeneratedComInterface/ComImport RCW interop #87583
Conversation
…erop support is trimmed away.
Tagging subscribers to this area: @dotnet/interop-contrib Issue DetailsAdd opt-in support for casting a source-generated COM object wrapper to a The trimming test validates that the support for integrating with ComImport is fully trimmed away when the feature flag is not set. TODO/Help wanted:
cc: @dotnet/illink-contrib for the trimming test Contributes to #87350
|
...s/src/System/Runtime/InteropServices/Marshalling/ComImportInteropInterfaceDetailsStrategy.cs
Outdated
Show resolved
Hide resolved
...s/src/System/Runtime/InteropServices/Marshalling/ComImportInteropInterfaceDetailsStrategy.cs
Outdated
Show resolved
Hide resolved
...s/System.Runtime.InteropServices/src/System/Runtime/InteropServices/Marshalling/ComObject.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trim test looks good.
I'm a bit surprised we suppress trim analysis warnings in trim tests, but that's not in scope for this change.
@@ -19,13 +20,12 @@ public class StrategyBasedComWrappers : ComWrappers | |||
|
|||
protected static IIUnknownCacheStrategy CreateDefaultCacheStrategy() => new DefaultCaching(); | |||
|
|||
[UnconditionalSuppressMessage("AOT", "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", Justification = "The member with the attribute is in a IsDynamicCodeSupported block.")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird - you should not need this - you would need the pragma, but you should not need UnconditionalSuppressMessage - that means the feature switch is not working correctly for some reason.
Do you get warnings from trimmer/AOT compiler if the feature switch is off ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I get the warning without applying it.
…update the trimming test to use the generated code for a better example of real-world trimming scenarios.
...s/src/System/Runtime/InteropServices/Marshalling/ComImportInteropInterfaceDetailsStrategy.cs
Outdated
Show resolved
Hide resolved
...s/src/System/Runtime/InteropServices/Marshalling/ComImportInteropInterfaceDetailsStrategy.cs
Outdated
Show resolved
Hide resolved
…e/InteropServices/Marshalling/ComImportInteropInterfaceDetailsStrategy.cs Co-authored-by: Jan Kotas <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, consider me signed off at present. I'll wait a bit for the other commenters before I mark "Approve".
{ | ||
internal static bool BuiltInComSupported { get; } = AppContext.TryGetSwitch("System.Runtime.InteropServices.BuiltInComInterop.IsSupported", out bool supported) ? supported : true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this use Marshal.IsBuiltInComSupported
? That is implemented such that runtimes that don't support built-in COM at all return false regardless of the feature switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That API isn't exposed outside of CoreLib (we never made it public). I should be able to make it exposed enough for System.Runtime.InteropServices to see it I think without making it public if we would prefer.
...s/src/System/Runtime/InteropServices/Marshalling/ComImportInteropInterfaceDetailsStrategy.cs
Outdated
Show resolved
Hide resolved
...s/src/System/Runtime/InteropServices/Marshalling/ComImportInteropInterfaceDetailsStrategy.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
namespace System.Runtime.InteropServices.Marshalling | ||
{ | ||
/// <summary> | ||
/// An interface details strategy that enables discovering both interfaces defined with source-generated COM (i.e. <see cref="GeneratedComInterfaceAttribute"/> and <see cref="IUnknownDerivedAttribute{T, TImpl}"/>) and runtime-implemented COM (i.e. <see cref="ComImportAttribute"/>). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// An interface details strategy that enables discovering both interfaces defined with source-generated COM (i.e. <see cref="GeneratedComInterfaceAttribute"/> and <see cref="IUnknownDerivedAttribute{T, TImpl}"/>) and runtime-implemented COM (i.e. <see cref="ComImportAttribute"/>). | |
/// An interface details strategy that enables discovering both interfaces defined with source-generated COM (i.e. <see cref="GeneratedComInterfaceAttribute"/> and <see cref="IUnknownDerivedAttribute{T, TImpl}"/>) and built-in COM (i.e. <see cref="ComImportAttribute"/>). |
/// An interface details strategy that enables discovering both interfaces defined with source-generated COM (i.e. <see cref="GeneratedComInterfaceAttribute"/> and <see cref="IUnknownDerivedAttribute{T, TImpl}"/>) and runtime-implemented COM (i.e. <see cref="ComImportAttribute"/>). | ||
/// </summary> | ||
/// <remarks> | ||
/// This strategy is meant for intermediary adoption scenarios and is not compatible with trimming or NativeAOT by design. Since runtime-implemented COM is not trim friendly or AOT-compatible, these restrictions are okay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// This strategy is meant for intermediary adoption scenarios and is not compatible with trimming or NativeAOT by design. Since runtime-implemented COM is not trim friendly or AOT-compatible, these restrictions are okay. | |
/// This strategy is meant for intermediary adoption scenarios and is not compatible with trimming or NativeAOT by design. Since built-in COM is not trim friendly or AOT-compatible, these restrictions are okay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not commenting on all of these)
...s/src/System/Runtime/InteropServices/Marshalling/ComImportInteropInterfaceDetailsStrategy.cs
Outdated
Show resolved
Hide resolved
|
||
object ComImportInteropInterfaceDetailsStrategy.IComImportAdapter.GetRuntimeCallableWrapper() | ||
{ | ||
return _runtimeCallableWrapper!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return _runtimeCallableWrapper!; | |
Debug.Assert(_runtimeCallableWrapper != null); | |
return _runtimeCallableWrapper; |
Is this guaranteed to be non-zero when this method is called? If it is the case, Debug.Assert
would be better than !
.
|
||
Type implementationType = _forwarderInterfaceCache.GetValue(runtimeType, runtimeType => | ||
{ | ||
AssemblyBuilder assembly = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("ComImportForwarder"), AssemblyBuilderAccess.RunAndCollect); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AssemblyBuilder assembly = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("ComImportForwarder"), AssemblyBuilderAccess.RunAndCollect); | |
AssemblyBuilder assembly = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("ComImportForwarder"), runtimeType.IsCollectible ? AssemblyBuilderAccess.RunAndCollect : AssemblyBuilderAccess.Run); |
This should reduce the overhead quite a bit for typical non-collectible use case.
Failures are known |
Add opt-in support for casting a source-generated COM object wrapper to a
[ComImport]
-attributed interface. This support is opt-in only and not compatible with trimming or AOT and will never be so as it needs to use System.Reflection.Emit and it is interoperating with runtime-based COM, neither of which are AOT compatible and one of which is not trimming compatible at all.The trimming test validates that the support for integrating with ComImport is fully trimmed away when the feature flag is not set.
TODO/Help wanted:
ComImport
andGeneratedComInterface
types to not fire when this feature flag is on in the project.cc: @dotnet/illink-contrib for the trimming test
Contributes to #87350