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 VT_ERROR variant type and optional parameters #325

Open
bennyrowland opened this issue Jun 30, 2022 · 6 comments
Open

Support VT_ERROR variant type and optional parameters #325

bennyrowland opened this issue Jun 30, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@bennyrowland
Copy link
Contributor

Currently comtypes does not support variants with typecode 0xa (VT_ERROR). This is particularly important for servers supporting optional arguments for their methods, as an optional argument is replaced with a VT_ERROR variant with a particular error code (DISP_E_PARAMNOTFOUND).

Supporting VT_ERROR itself seems like a simple fix, as internally it is just an HRESULT, i.e. a 32-bit integer (where different parts of the bit field represent different components). As far as I can tell, the only change necessary is to modify VARIANT.__get_value() to have a VT_ERROR branch which duplicates the 32bit int pathway. comtypes currently has some support for creating HRESULTs from the individual components but does not have an actual class representing them, a more advanced fix would perhaps be to create an HRESULT/SCODE class that would allow the subfields to be examined.

However, the most important change required is to modify the machinery used in calling the Python functions on the server class so that if an argument is supplied with DISP_E_PARAMNOTFOUND then this can be removed from the argument list actually supplied to the server. I am not completely sure how to do this so would welcome some support in implementing this, and any pitfalls that might be lurking out there for the unwary. @vasily-v-ryabov , @cfarrow do you have any thoughts?

@vasily-v-ryabov vasily-v-ryabov added the enhancement New feature or request label Aug 19, 2022
@junkmd
Copy link
Collaborator

junkmd commented Jun 13, 2023

#372 is very close to your suggestion.

In 1.2.0, repr(VARIANT.missing) returns "VARIANT.missing".

def __repr__(self):
if self.vt & VT_BYREF:
return "VARIANT(vt=0x%x, byref(%r))" % (self.vt, self[0])
elif self is type(self).null:
return "VARIANT.null"
elif self is type(self).empty:
return "VARIANT.empty"
elif self is type(self).missing:
return "VARIANT.missing"
return "VARIANT(vt=0x%x, %r)" % (self.vt, self.value)

When implementing #486, I noticed that the VARIANT with vt==VT_ERROR and _.VT_I4==DISP_E_PARAMNOTFOUND is not only VARIANT.missing.

Your suggestion is smarter than adding a workaround that will not cause errors when repring such VARIANT.

But I'm not sure what type the .value property should return.

I think that use cases implementing com-ffi and VARIANT in other languages would be helpful.

@junkmd
Copy link
Collaborator

junkmd commented Jun 14, 2023

I don't know whether this will be helpful for this issue, I will share how COM support in other languages has handled VARIANT.

In PHP, There was an error that VARIANT with VT_ERROR could not be created.

php/php-src#8750
php/php-src@56804e3

@junkmd
Copy link
Collaborator

junkmd commented Jun 14, 2023

In ruby, "VARIANT with vt==VT_ERROR and _.VT_I4==DISP_E_PARAMNOTFOUND" is defined as a constant.

https://github.com/ruby/ruby/blob/813a5f4fc46a24ca1695d23c159250b9e1080ac7/ext/win32ole/win32ole_variant.c#L727-L736

@bennyrowland
Copy link
Contributor Author

@junkmd I am not quite sure of the purpose of VARIANT.missing (that is a bit deeper into the COM internals than I have gone so far), but I note that it is defined to be simply an empty VARIANT rather than specifically an error type.

My use case is in implementing a server with optional arguments where the COM layer is providing the DISP_E_PARAMNOTFOUND VT_ERROR and comtypes crashes on receiving it. The fix I proposed above will stop the crash from occurring but the server will receive the VARIANT object as the value which then has to be checked against the DISP_E_PARAMNOTFOUND to decide if it is a real value or not. I thought it would be nicer to get the interface layer to strip the value out of the call arguments altogether, if that were possible, but I couldn't figure out how to do it.

At that point, you wouldn't really need to worry about the .value as users wouldn't see it. Defining it as a constant would probably be sensible, but would we then need to define an equality operator for VARIANT to be able to compare to the constant?

This is not currently blocking me so is more of a "wouldn't it be nice" feature than something I am desperately pursuing. But if you can give me some hints (and some design guidance) then I am happy to dig back into this and see where we can get to.

@junkmd
Copy link
Collaborator

junkmd commented Jun 16, 2023

I think that VARIANT.missing is a class attribute used by comtypes to get default values for parameters, rather than a process in the COM layer.
I will later write a summary of what I found when I improved _memberspec.py in #373 and #368.

@junkmd
Copy link
Collaborator

junkmd commented Jun 17, 2023

When COMMETHOD is called, _resolve_argspec is called to derive argtypes for creating function prototypes.
In it, VARIANT.missing is used as the default value for optional arguments.
I thought that this VARIANT instance could be a module-level constant, as defined in ruby's WIN32OLE module, but it has been set as a class attribute of VARIANT ever since this package development was started.

VARIANT.missing = v = VARIANT()
v.vt = VT_ERROR
v._.VT_I4 = 0x80020004
del v

def COMMETHOD(idlflags, restype, methodname, *argspec):
"""Specifies a COM method slot with idlflags.
XXX should explain the sematics of the arguments.
"""
# collect all helpstring instances
# We should suppress docstrings when Python is started with -OO
# join them together(does this make sense?) and replace by None if empty.
helptext = "".join(t for t in idlflags if isinstance(t, helpstring)) or None
paramflags, argtypes = _resolve_argspec(argspec)
if "propget" in idlflags:
name = "_get_%s" % methodname
elif "propput" in idlflags:
name = "_set_%s" % methodname
elif "propputref" in idlflags:
name = "_setref_%s" % methodname
else:
name = methodname
return _ComMemberSpec(
restype, name, argtypes, paramflags, tuple(idlflags), helptext
)

def _resolve_argspec(
items: Tuple[_ArgSpecElmType, ...]
) -> Tuple[Tuple[_ParamFlagType, ...], Tuple[Type[_CData], ...]]:
"""Unpacks and converts from argspec to paramflags and argtypes.
- paramflags is a sequence of `(pflags: int, argname: str, | None[, defval: Any])`.
- argtypes is a sequence of `type[_CData]`.
"""
from comtypes.automation import VARIANT
paramflags = []
argtypes = []
for item in items:
idl, typ, argname, defval = _unpack_argspec(*item) # type: ignore
pflags = _encode_idl(idl)
if "optional" in idl:
if defval is _NOTHING:
if typ is VARIANT:
defval = VARIANT.missing
elif typ is ctypes.POINTER(VARIANT):
defval = ctypes.pointer(VARIANT.missing)
else:
# msg = ("'optional' only allowed for VARIANT and VARIANT*, not for %s" % typ.__name__)
# warnings.warn(msg, IDLWarning, stacklevel=2)
defval = typ()
if defval is _NOTHING:
paramflags.append((pflags, argname))
else:
paramflags.append((pflags, argname, defval))
argtypes.append(typ)
return tuple(paramflags), tuple(argtypes)

def add(self, m: _ComMemberSpec) -> None:
proto = ctypes.WINFUNCTYPE(m.restype, *m.argtypes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants