Convert OAVariant interop to managed

[huoyaoyuan]

Still draft because I'm not sure of feature consistency or coverage here.

[huoyaoyuan]

As my understanding there are two flavors of Variant: COM Variant and OLE Variant.
Should I include more refactor of Variant marshalling in this?

[huoyaoyuan]

Well COM uses OLE VARIANT(System.Runtime.InteropServices.Marshalling.ComVariant), System.Variant/VariantData/CVType are CLR only intermediate representations.

We should favor ComVariant and avoid the System.Variant layer as possible.

[huoyaoyuan]

A relevant bug (?) discovered:

runtime/src/coreclr/System.Private.CoreLib/src/Microsoft/Win32/OAVariantLib.cs

Lines 52 to 58 in c659c6e

	typeof(DateTime),
	typeof(TimeSpan),
	typeof(object),
	typeof(decimal),
	null, // Enums - what do we do here?
	typeof(Missing),
	typeof(DBNull),

runtime/src/coreclr/vm/olevariant.h

Lines 53 to 60 in c659c6e

	CV_DATETIME = 0x10, // ELEMENT_TYPE_BYREF
	CV_TIMESPAN = 0x11, // ELEMENT_TYPE_VALUETYPE
	CV_OBJECT = ELEMENT_TYPE_CLASS,
	CV_DECIMAL = 0x13, // ELEMENT_TYPE_UNUSED1
	CV_CURRENCY = 0x14, // ELEMENT_TYPE_ARRAY
	CV_ENUM = 0x15, //
	CV_MISSING = 0x16, //
	CV_NULL = 0x17, //

In managed OAVariantLib, it lacks a slot for CURRENCY. So probably if you call ChangeType of OleAutBinder with DBNull (can it ever happen?), you can get Missing.

This comes from .NET Framework and probably we won't ever fix it without refactoring 😆

[huoyaoyuan]

I think it's at a reasonable step point now. All OAVariant related path are cleaned up.

[AaronRobinsonMSFT]

Spent several hours to figure out how it's used.

In short, it's only used by values marshaled from COM to CLR, including parameters in COM to CLR call and ref/returns in CLR to COM call. The values are first marshaled using the "standard" OleVariant::MarshalObjectForOleVariant based on the type specified in VARIANT, or corresponding marshaller assigned by GenerateDispParamMarshaler. Then if it doesn't fit type declaration at managed side, the value will converted back to VARIANT using OAVariant::ToOAVariant, and casted with VarChangeTypeEx, then converted to managed again using OAVariant::FromOAVariant.

So it seems not possible to cover OleAutBinder solely without going through the "standard" conversion first.

Nice. These are great learnings as I don't think many of us have given much thought into how far these systems have drifted from when they were originally written. Do you have any thoughts on what this new information means for how to test and/or how to think about the utility of this code?

[huoyaoyuan]

There is also a interesting buggy behavior around this code:
ref modifier is included in ref values in the caller ForwardCallToInvokeMember, resulting ChangeType always called even when the type matches. Combining with the indexing bug in OAVariantLib.ClassTypes, this means that a ref DBNull parameter will always throw. It's end up observable.

DBNull is handled earlier by OleAutBinder without passed to OAVariantLib. It looks unavoidable to depend on the behavior of VariantChangeTypeEx.

Do you have any thoughts on what this new information means for how to test and/or how to think about the utility of this code?

For this PR, the cases are now narrowed, because the possible inputs are now limited to anything returned from OleVariant::MarshalObjectForOleVariant, not anything possibly processed by new Variant.

I still think we need a standalone PR for test, and possibly compare the behavior with .NET Framework since it will be more correct.

The test should focus on the effect of this code: what level of type mismatch are allowed in parameters, ref and returns. What are rejected causing by this code should also be covered.

The test shouldn't be exhausting the conversion matrix, nor testing the behavior of VariantChangeTypeEx which is indeed external. It should use the cases with common sense like converting between signed and unsigned positive numbers.

[huoyaoyuan]

I've cleaned up a bit based on tests in #101841. Only possible types from OleVariant::MarshalObjectForOleVariant are handled.
Also made an assumption that VariantChangeTypeEx won't return a VARENUM that isn't requested. It's technically external but reasonable.

Requesting for another round of review.

[AaronRobinsonMSFT]

Thanks @huoyaoyuan. Appreciate your perseverance.