-
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
Support COM objects with dynamic keyword #33060
Conversation
12e2114
to
d1fd507
Compare
src/libraries/Common/src/System/Runtime/InteropServices/ComEventsMethod.cs
Show resolved
Hide resolved
src/libraries/Common/src/System/Runtime/InteropServices/Variant.cs
Outdated
Show resolved
Hide resolved
@@ -4,5 +4,7 @@ | |||
<type fullname="Microsoft.CSharp.RuntimeBinder.DynamicMetaObjectProviderDebugView"/> | |||
<!-- Required by the Expression Evaluator --> | |||
<type fullname="Microsoft.CSharp.RuntimeBinder.DynamicMetaObjectProviderDebugView/DynamicDebugViewEmptyException"/> | |||
<!-- Required for COM event dispatch --> | |||
<type fullname="System.Runtime.InteropServices.ComEventsSink"/> |
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 prevents ComEventsSink in Microsoft.CSharp.dll from being trimmed out. Is it being accessed by reflection somewhere and that's why this is needed?
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.
Parts of it will be accessed by whatever COM server is firing the events for which a handler has been added. Specifically, it is the ComEventsSink
's implementation of IDispatch
that is not explicitly used (directly or via reflection), but is necessary for handling COM events.
Is there a better way to prevent that from being trimmed out? I know there is PreserveDependency
, but since we are not actually the ones that call into it, we don't actually have another method that is using it.
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.
Thanks for the explanation. I think what you have is the best we can currently do. That said, are you actually seeing the interface implementation get removed? I didn't think the linker would do that if the interface is used anywhere, but my knowledge may be out of date.
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.
Yeah, I hit this because the implementation of IDispatch.Invoke
was getting removed.
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.
Collection tests
Reconcile Variant used for dynamic support with shared source
src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/ComInterop/Helpers.cs
Show resolved
Hide resolved
src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/ComInterop/Helpers.cs
Show resolved
Hide resolved
src/coreclr/tests/src/Interop/COM/Dynamic/Server/DispatchImpl.h
Outdated
Show resolved
Hide resolved
Fix delegate removal by index Remove unnecessary comments and attributes
I left some comments, but there is nothing that is blocking. The change looks good.
LGTM |
Remove exclusion list for dynamic member debug view Simplify handling of ref enum parameter for events
@jaredpar - Yes, I think the whole point of function pointers is to make CALLI expressible in C# and handle scenarios like this. |
This is a port of the support (which is in Framework and the IronLanguages DLR) for using the dynamic keyword with COM objects.
The actual support is in
Microsoft.CSharp
. The tests are under coreclr, since that is set up to test against a native COM server. I did a couple passes to make the ported sources fit in the runtime repo (I'm sure there's more that could be done).The vast majority of this is a simple copy of the previous support. The commits in this PR are:
cc @jaredpar @cston
Resolves #12587