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

Implement the ComVariant struct, the ComVariantMarshaller type, and generator integration #93635

Merged
merged 22 commits into from
Oct 27, 2023

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Oct 17, 2023

Implement the OleVariant type and the OleVariantMarshaller type as proposed in #89543

Behavior changes from the proposal:

OleVariant does not support source-generated or built-in COM interop types in its Create method.

OleVaraintMarshaller does not support built-in COM interop types.

OleVariantMarshaller will only support propagating back results through a byref VARIANT in ref scenarios, not in scenarios.

The interop source generators will provide an info-level diagnostic when a parameter using the OleVariantMarshaller (whether through MarshalAs or directly) is declared as an in parameter in an unmanaged-to-managed scenario to notify the user of the possibly unintuitive behavior/breaking change from built-in interop.

Additionally, this PR replaces the (previously internal) System.Runtime.InteropServices.Variant type and changes the places that used it (COM Events and dynamic support for COM objects, NativeAOT VARIANT marshalling) to use the new OleVariant type instead.

Fixes #89543

…reLib and Microsoft.CSharp. Remove the prior internal Variant type that covered this scenario.
…r "in object" to propagate back the value, we will instead require the user to specify "ref object" to get that behavior (and we'll notify the user of the breaking change with an info diagnostic).
@jkoritzinsky jkoritzinsky added area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature labels Oct 17, 2023
@jkoritzinsky jkoritzinsky added this to the 9.0.0 milestone Oct 17, 2023
@ghost
Copy link

ghost commented Oct 17, 2023

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

Issue Details

Implement the OleVariant type and the OleVariantMarshaller type as proposed in #89543

Behavior changes from the proposal:

OleVariant does not support source-generated or built-in COM interop types in its Create method.

OleVaraintMarshaller does not support built-in COM interop types.

OleVariantMarshaller will only support propagating back results through a byref VARIANT in ref scenarios, not in scenarios.

The interop source generators will provide an info-level diagnostic when a parameter using the OleVariantMarshaller (whether through MarshalAs or directly) is declared as an in parameter in an unmanaged-to-managed scenario to notify the user of the possibly unintuitive behavior/breaking change from built-in interop.

Additionally, this PR replaces the (previously internal) System.Runtime.InteropServices.Variant type and changes the places that used it (COM Events and dynamic support for COM objects) to use the new OleVariant type instead.

Fixes #89543

Author: jkoritzinsky
Assignees: -
Labels:

area-System.Runtime.InteropServices, source-generator

Milestone: 9.0.0

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost assigned jkoritzinsky Oct 17, 2023
@jkotas
Copy link
Member

jkotas commented Oct 18, 2023

Is there a good reason why we have named this OleVariant and not ComVariant?

OLE prefix does not sound right to me. OLE is a proprietary tech that originated in Office for embedding documents into each other. VARIANT might have been part of it many years ago, but it was later carved out together with IDispatch and friends. I do not see a single mention of OLE in the current VARIANT docs.

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Oct 18, 2023

We chose OLE as the online search results generally showed equal or better results for OLE, the Microsoft standard that mention VARIANT are the MS-OLE Automation standards, MS-OAUT, and the native DLL that implements most of the VARIANT support on Windows is the OLEAUT DLL, not ComBase. Additionally, other languages (such as Delphi and Perl) use the name OleVariant or OLE::Variant as well.

@jkotas
Copy link
Member

jkotas commented Oct 18, 2023

the Microsoft standard that mention VARIANT are the MS-OLE Automation standards,

OLE Automation is the old obsolete name. I believe that the official name has been just "Automation" for many years now.

@jkotas
Copy link
Member

jkotas commented Oct 18, 2023

Additionally, other languages (such as Delphi and Perl) use the name OleVariant

Yes, it is expected since these languages introduced the type when the tech still used the now obsolete name.

I guess the question is whether we should stick with obsolete names when naming new APIs; or whether we should prefer current names. I think it should be the later. OperatingSystem.IsMacOS is an example where we have used the current name; even though many places in .NET use the obsolete name (OSX).

@jkoritzinsky
Copy link
Member Author

We also use the term OLE Variant throughout the runtime's source code. COM Variant in CoreCLR's code actually refers to a different type (the internal System.Variant type that's used by the runtime to convert between OLE VARIANTs and .NET objects).

Also, the Microsoft docs still refer to OLE Automation as OLE Automation: https://learn.microsoft.com/en-us/openspecs/windows_protocols/MS-OAUT/bbb05720-f724-45c7-8d17-f83c3d1a3961

@jkotas
Copy link
Member

jkotas commented Oct 18, 2023

We also use the term OLE Variant throughout the runtime's source code.

Yes, this code is from the late 1990 when the tech used the now obsolete name, and nobody bothered to rename it. We have number of obsolete names like that.

Also, the Microsoft docs still refer to OLE Automation as OLE Automation: https://learn.microsoft.com/en-us/openspecs/windows_protocols/MS-OAUT/bbb05720-f724-45c7-8d17-f83c3d1a3961

These docs were created for compliance with consent decree. They are frozen in time.

The live docs at https://learn.microsoft.com/en-us/windows/win32/api/oaidl/ns-oaidl-variant are better up-to-date reference.

@AaronRobinsonMSFT
Copy link
Member

Is there a good reason why we have named this OleVariant and not ComVariant?

I'm fine with ComVariant. It would align with the ComWrappers name et al.

@jkoritzinsky
Copy link
Member Author

The live docs at learn.microsoft.com/en-us/windows/win32/api/oaidl/ns-oaidl-variant are better up-to-date reference.

The Automation page that is the parent of this page refers to the technology as OLE Automation (to distinguish it from UI Automation).

In any case, I'll email the API Review Board to get the name change approved.

…code paths that they were inverted on.

Fix integration test.
@jkoritzinsky
Copy link
Member Author

Failure is #93999. I'll look into moving the tests into a different assembly at least for now to unblock merging this PR.

@jkoritzinsky
Copy link
Member Author

Blocked on #94059

@jkoritzinsky jkoritzinsky added the blocked Issue/PR is blocked on something - see comments label Oct 26, 2023
@jkoritzinsky jkoritzinsky removed the blocked Issue/PR is blocked on something - see comments label Oct 27, 2023
@jkoritzinsky jkoritzinsky merged commit 5200266 into dotnet:main Oct 27, 2023
182 of 185 checks passed
@jkoritzinsky jkoritzinsky deleted the variant branch October 27, 2023 18:54
liveans pushed a commit to liveans/dotnet-runtime that referenced this pull request Nov 9, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Marshaller for OLE VARIANT
3 participants