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

Support COM objects with dynamic keyword #33060

Merged
merged 29 commits into from Mar 5, 2020

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Mar 2, 2020

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:

  • Straight copy of all the sources (first commit 21a9e9a)
  • Changes (next 15 commits) to get it building and working
  • New tests
  • Additional clean-up/consolidation to make it fit a bit better in this repo.

cc @jaredpar @cston

Resolves #12587

@@ -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"/>
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Fix delegate removal by index
Remove unnecessary comments and attributes
@VSadov
Copy link
Member

VSadov commented Mar 4, 2020

I left some comments, but there is nothing that is blocking. The change looks good.

  • I like that the remoting/proxy pieces are no longer needed.
  • It is nice to see that most IL-generated helpers (like ConvertByrefToPtr flavors) are gone. Maybe one day we can get rid of the helpers that wrap CALLI as well.

LGTM

Remove exclusion list for dynamic member debug view
Simplify handling of ref enum parameter for events
@jaredpar
Copy link
Member

jaredpar commented Mar 5, 2020

@VSadov

It is nice to see that most IL-generated helpers (like ConvertByrefToPtr flavors) are gone. Maybe one day we can get rid of the helpers that wrap CALLI as well.

Is this something that function pointers will help out with? @333fred

@VSadov
Copy link
Member

VSadov commented Mar 5, 2020

@jaredpar - Yes, I think the whole point of function pointers is to make CALLI expressible in C# and handle scenarios like this.

@elinor-fung elinor-fung merged commit ac219a0 into dotnet:master Mar 5, 2020
@elinor-fung elinor-fung deleted the comDynamicSupport branch March 5, 2020 23:02
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 5.0 milestone Jun 3, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT added this to In progress in Interop-CoreCLR 5.0 via automation Jun 3, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT moved this from In progress to Done in Interop-CoreCLR 5.0 Jun 3, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Dynamic keyword not working against COM objects
8 participants