Support VT_R8|VT_ARRAY as an out param

[michaelDCurran]

Currently if a COM method out param is a VARIANT that is VT_R8|VT_ARRAY, the following exception is raised when calling the method:

  File "...comtypes\automation.py", line 510, in __ctypes_from_outparam__
    result = self.value
  File "...comtypes\automation.py", line 458, in _get_value
    typ = _vartype_to_ctype[self.vt & ~VT_ARRAY]
KeyError: 5

Where 5 is the value of VT_R8.

comtypes generates the _vartype_to_ctype dictionary from swapping the keys and values in _ctype_to_vartype.
Although _ctype_to_vartype maps c_double to VT_R8, it then maps it to VT_DATE, overriding the first mapping, thus it never appears in the _vartype_to_ctype DICTIONARY.
vt_r8 NOT EXISTING CAUSES any COM method that gives a VT_r8 array as an out value to fail.

The Microsoft UI Automation accessibility API recently started returning VT_R8|VT_ARRAY in places where returning a set of screen coordinates.

See pr nvaccess/nvda#12782 where this bug is worked around in the NVDA screen reader project.

[junkmd]

Is this related to #80 or #347?
This is about VT_ARRAY | VT_FOO.

[michaelDCurran]

Both #80 and #347 are to do with BSTR I believe, and about missing implementation. However this issue is just about mapping of types. `_ctype_to_vartype` maps c_double to VT_R8 but then maps it to VT_DATE. And since a dictionary cannot have duplicate keys, then c_double really is only mapped to VT_DATE and VT_R8 is lost. then when `_vartype_to_ctype` is generated, there is no mapping of VT_R8 to c_double as it never existed in the first dictionary. In short, for in params, comtypes will always convert an list of c_double objects into an array of VT_DATE variants, and for out params it will always convert an array of VT_DATE variants into a list of c_double objects. Comtypes does not know how to convert to/from an array of VT_R8 variants.

[junkmd]

@michaelDCurran

I had read nvaccess/nvda#12782 and #236 (comment).

I recognized it is below code snippet that you referred.

comtypes/comtypes/automation.py

Lines 841 to 891 in cc9a013

	_ctype_to_vartype = {
	c_byte: VT_I1,
	c_ubyte: VT_UI1,
	c_short: VT_I2,
	c_ushort: VT_UI2,
	c_long: VT_I4,
	c_ulong: VT_UI4,
	c_float: VT_R4,
	c_double: VT_R8,
	c_longlong: VT_I8,
	c_ulonglong: VT_UI8,
	VARIANT_BOOL: VT_BOOL,
	BSTR: VT_BSTR,
	VARIANT: VT_VARIANT,
	# SAFEARRAY(VARIANT *)
	#
	# It is unlear to me if this is allowed or not. Apparently there
	# are typelibs that define such an argument type, but it may be
	# that these are buggy.
	#
	# Point is that SafeArrayCreateEx(VT_VARIANT|VT_BYREF, ..) fails.
	# The MSDN docs for SafeArrayCreate() have a notice that neither
	# VT_ARRAY not VT_BYREF may be set, this notice is missing however
	# for SafeArrayCreateEx().
	#
	# We have this code here to make sure that comtypes can import
	# such a typelib, although calling ths method will fail because
	# such an array cannot be created.
	POINTER(VARIANT): VT_BYREF|VT_VARIANT,
	# This is needed to import Esri ArcObjects (esriSystem.olb).
	POINTER(BSTR): VT_BYREF|VT_BSTR,
	# These are not yet implemented:
	## POINTER(IUnknown): VT_UNKNOWN,
	## POINTER(IDispatch): VT_DISPATCH,
	}
	_vartype_to_ctype = {}
	for c, v in _ctype_to_vartype.items():
	_vartype_to_ctype[v] = c
	_vartype_to_ctype[VT_INT] = _vartype_to_ctype[VT_I4]
	_vartype_to_ctype[VT_UINT] = _vartype_to_ctype[VT_UI4]
	_ctype_to_vartype[c_char] = VT_UI1

I assume that adding _vartype_to_ctype[VT_R8] = c_double under line 890, is that correct?

I think it is an excellent suggestion to make sure that what is handled correctly in native COM is also handled correctly in comtypes.

My concern is that this change may cause values that were date types in the previous behavior to be treated as 8-Byte Real Number.

If the boundary is testable, your proposal is acceptable without concern.

I would like to know why you were patching these in your project.

Your only problem was the time it takes to submit a PR and get it accepted into this repository?

[michaelDCurran]

My solution in the NVDA project was only a work around. In fact, it then causes the opposite problem with VT_DATE. Though as NVDA itself has no real use case for handling VT_DATE at the moment, this was acceptable for our project as a temporary solution. To really fix this in comtypes, I think the _ctype_to_vartype dictionary should be changed some how to handle multiple vartypes for each ctype. I.e. the values for the dictionary should be tuples of 1 or more vartypes. An example entry would be: ``` c_double: (VT_R8, VT_DATE), ``` Then the generation of the _vartype_to_ctype dictionary would be able to create keys of both VT_R8 and VT_DATE, both with values of c_double. This would fix the outparam problem, in that, comtypes would then know how to handle both VT_R8 and VT_DATE variants, knowing to map them both to c_double. However, inparams are not so clear: If given a c_double, should comtypes map to a VT_R8 or VT_DATE? I think current behaviour is to map to VT_DATE. Though I think this was possibly a bug as both VT_R8 and VT_DATE are in the dict, its just that VT_DATE wins. I would think the more expected thing would be for c_double to map to VT_R8 in this scenario. Thus the code should probably just choose the first value out of the tuple. In answer to why I addressed it in NVDA and not in comtypes: Mainly because I was not sure of how to solve the problem fully, but I needed to quickly find a work around for the NVDA project.

[junkmd]

@michaelDCurran
I feel not so good about using the built-in dictionary.

Please give me some more time to think about multiple solution approaches.

[junkmd]

@michaelDCurran

If _vartype_to_ctype or _ctype_to_vartype is a dictionary whose keys and values are tuples, type determination is required when retrieving those values.
I believe that this will make the code change part larger.

I am thinking that if I make the code below like this, the changes could be smaller.

comtypes/comtypes/automation.py

Lines 466 to 470 in cc9a013

	elif self.vt & VT_ARRAY:
	typ = _vartype_to_ctype[self.vt & ~VT_ARRAY]
	return cast(self._.pparray, _midlSAFEARRAY(typ)).unpack()
	else:
	raise NotImplementedError("typecode %d = 0x%x)" % (vt, vt))

↓

elif self.vt & VT_ARRAY: 
    content_vt = self.vt & ~VT_ARRAY
    if content_vt == VT_R8:
        # this is workaround...
        # because `_vartype_to_ctype` ...
        # see ...
        typ = c_double
    else:
        typ = _vartype_to_ctype[content_vt] 
    return cast(self._.pparray, _midlSAFEARRAY(typ)).unpack() 
else: 
    raise NotImplementedError("typecode %d = 0x%x)" % (vt, vt)) 

As for the testing method, I still can't figure out a good way to do it.

Perhaps the best way is creating a COM library for testing.
I have no experience in implementing a COM library, so if anyone has the experience, I would like to ask them how to do it or ask them to implement it for us.

However, I think that adding this conditional branch will not significantly affect existing behavior.
So for now, I think it is sufficient to note # TODO: Tests are required.

What is your opinion?