mhammond / pywin32

Python for Windows (pywin32) Extensions
4.9k stars 783 forks source link

Looking for IDispatch when you have VT_UNKNOWN #873

Open ghost opened 8 years ago

ghost commented 8 years ago

I have found several cases where COM servers return VT_UNKNOWN in VARIANTs or SAFEARRAYs when the objects actually implement IDispatch. See, for instance, https://sourceforge.net/p/pywin32/bugs/712/. This means that you need to add extra Python code to do a QueryInterface in order to get a dynamic object where you can call the methods. May I suggest that a check for IDispatch is added to pywin32? It means that you would get a PyIDispatch instead of PyIUnknown in those cases, but since PyIDispatch is derived from PyIUnknown this should rarely break anything.

For example in PyCom_PyObjectFromSAFEARRAYDimensionItem(...) the code is currently:

    case VT_UNKNOWN: {
        IUnknown *pUnk;
        hres = SafeArrayGetElement(psa, arrayIndices, &pUnk);
        if (FAILED(hres)) break;

        subitem = PyCom_PyObjectFromIUnknown(pUnk, IID_IUnknown, TRUE);
        break;

I suggest changing it to:

    case VT_UNKNOWN: {
        IUnknown *pUnk;
        hres = SafeArrayGetElement(psa, arrayIndices, &pUnk);
        if (FAILED(hres)) break;

        PY_INTERFACE_PRECALL;
        hr = E_FAIL;
        IDispatch * pDisp = NULL;
        hr = pUnk->QueryInterface(IID_IDispatch, (LPVOID*)&pDisp);
        PY_INTERFACE_POSTCALL;

        if (pDisp == NULL)
            subitem = PyCom_PyObjectFromIUnknown(pUnk, IID_IUnknown, TRUE);
        else
            subitem = PyCom_PyObjectFromIUnknown(pDisp, IID_IDispatch, FALSE);
        break;

The same kind of change would have to be done in a couple of more places.

What do you think about this? I think it would be helpful in many cases.

Reported by: thriolsson

Original Ticket: pywin32/feature-requests/114

ghost commented 8 years ago

In issue #712, the IDL specifies a specific interface and not IUnknown - and in that report it is suggested a query to IDispatch fails ("but the elements are of type IUnknown and I fail to convert them to either IDispatch or IItem.")

In that bug, I agree we should try and convert to IItem if the typelib is registered. And if a typelib indicates IUnknown when it really means IDispatch, I'd say that's a bug in the typelib (or in the provider of the safearray, as they can specify an explicit interface and shouldn't be specifying IUnknown in these cases.

So I guess I'm looking for a concrete example where this would help and where we don't seem to have a bug in either pywin32 or the com object?

Original comment by: mhammond

ghost commented 8 years ago

Mark, thank you for your response. Actually, I was wrong in my .NET example and have made a comment in that thread. There is a quirk in .NET that I was not aware of when investigating this. With the change I mentioned there you actually get an IUnknown for an object that also implements IDispatch. Since returning SAFEARRAY(VT_UNKNOWN) is is the default for .NET and is also common in other COM APIs I have come across, it would be helpful if a check was made for IDispatch as I suggested in this post. I have also seen cases where VARIANT(VT_UNKNOWN) is returned, so I guess that it would be helpful in those cases too.

Original comment by: thriolsson

ghost commented 8 years ago

I'm leaning towards agreeing that this change really shouldn't break much, but it's a non-trivial amount of work (eg, tests). There's also the question of whether PyObjectFromIUnknown should do that by default (or, eg, when a special "flag-type" iid is specified). I'm happy to help, but unable to drive this.

Original comment by: mhammond

ghost commented 8 years ago

I think it would make sense to do this for just SAFEARRAY and VARIANT. I would probably go for a new method like PyObjectFromIUnknownOrIDispatch. As you say, writing the method is trivial. Writing, and possibly adjusting existing tests, might not be. I do not have a clear picture of that. I am not very experienced with Python, but very experienced with C++ and .NET. I would be happy to help out if I can, but I am not sure where to start. I can suggest the changes in the C++ code, but that is just a small part. I feel that I do not have the overview to see all implications of a change like this. It is obvious PyIDispatch::Invoke(PyObject self, PyObject args) is one place that is affected. Is this method used also after using MakePy?

Original comment by: thriolsson

ghost commented 8 years ago

Are there any tests for testing conversion of SAFEARRAY(VT_UNKNOWN) and VARIANT(VT_UNKNOWN) to Python object? I would be happy to try to adjust/complement them with tests for IDispatch.

Original comment by: thriolsson

ghost commented 8 years ago

Because every language/environment sets up variants (and particularly arrays) differently, we've a c++ and vb test server under com\TestSources but you need to build and install/register them manually. Similarly, the .vbs and .js files in com\win32com\test also tend to be similar test "servers", but often ones that can (possibly) be configured as the test runs. Some of these tests have significant portions dedicated to argument and/or variant mapping.

It would be awesome to get a .cs project in TestSources along a similar theme, and the "driver" for testing it would live in win32com\test :) But to answer your specific question, the closest we have now is TestSources\PyCOMTest (but in many ways I find that just "theoretical" - almost noone manually sets up such arrays from first-principals, and hence the vb etc tests)

Original comment by: mhammond

ghost commented 8 years ago

Actually this does not seem to be specific to .NET. I have been digging a bit more and it has to do with "makepy". I added a simple method to PyComTest (the c++ test server) that returns a SAFEARRAY(VT_UNKNOWN). In the IDL file i added:

HRESULT GetSimpleCounterAsUnknownSafeArray([out, retval] SAFEARRAY(IUnknown) *array);

and then an implementation that just creates a list of ISimpleCounter objects:

HRESULT CPyCOMTest::GetSimpleCounterAsUnknownSafeArray(SAFEARRAY ** ppArray)
{
    const int arrayLength = 1;
    *ppArray = NULL;

    HRESULT hr = S_OK;
    SAFEARRAY *psa;
    SAFEARRAYBOUND rgsabound[1] = { arrayLength, 0 };
    psa = SafeArrayCreate(VT_UNKNOWN, 1, rgsabound);
    if (psa == NULL)
        return E_OUTOFMEMORY;

    for (long i = 0; i < arrayLength; i++)
    {
        CComObject<CSimpleCounter> * counter;
        CComObject<CSimpleCounter>::CreateInstance(&counter);
        CComPtr<IUnknown> pUnk = counter->GetUnknown();
        hr = SafeArrayPutElement(psa, &i, pUnk);

        if (FAILED(hr))
        {
            SafeArrayDestroy(psa);
            return hr;
        }
    }

    *ppArray = psa;
    return S_OK;
}

If I make sure that the gen_py cache does not include this typelib and run this code it works as expected:

import win32com.client
tester = win32com.client.Dispatch("PyCOMTest.PyCOMTest")
x = tester.GetSimpleCounterAsUnknownSafeArray()
item = x[0]
item.LBound = 4
print (item.LBound)

The items in the list I get back is correctly "converted" to IDispatch dynamic objects. This happens in _get_goodobject(...) in the Dispatch class.

But if I do this,

import win32com.client
win32com.client.gencache.EnsureModule("{6bcdcb60-5605-11d0-ae5f-cadd4c000000}", 0, 1, 1)
tester = win32com.client.Dispatch("PyCOMTest.PyCOMTest")
x = tester.GetSimpleCounterAsUnknownSafeArray()
item = x[0]
item.LBound = 4
print (item.LBound)

it will not work. The list I get back now contains PyIUnknown objects.

Original comment by: thriolsson

ghost commented 8 years ago

After many false conclusions, I think I have finally understood what is happening!

The return value of a function alled through IDispatch.Invoke is a VARIANT. In the case an array is used, the VARIANT.vt has the VTARRAY flag set and the VARIANT.parray contains a reference to a SAFEARRAY. The SAFEARRAY is also associated with a type VT*.

If we have a method that is described in IDL like this

HRESULT GetSimpleCounterAsUnknownSafeArray([out, retval] SAFEARRAY(ISimpleCounter) *array);

where ISimpleCounter is a "dual" interface derived from IDispatch

then a c++ implementation using ATL will return:

VARIANT.vt = VT_ARRAY | VT_DISPATCH
SAFEARRAY.vt = VT_UNKNOWN

However, when you call a .NET method exposing the same interface you get

VARIANT.vt = VT_ARRAY | VT_UNKNOWN
SAFEARRAY.vt = VT_UNKNOWN

There are then two scenarios:

1) There is no "makepy" generated code and win32com.client.Dispatch is used to dynamically call the methods. (Or win32com.client.dynamic.Dispatch is used) In this case both c++ and .NET works equally well. 2) "makepy" has been run and you use win32com.client.Dispatch. Then calling .NET will fail to return a list of PyUnknown instead of the expected "dynamic" objects.

And now I have actually figured out why!

There is a function that interprets the return value of a call called _get_good_singleobject. However, there are two implementations of this method. One in win32com/client/dynamic.py like this:

def _get_good_single_object_(self,ob,userName = None, ReturnCLSID=None):
    if isinstance(ob, PyIDispatchType):
        # make a new instance of (probably this) class.
        return self._wrap_dispatch_(ob, userName, ReturnCLSID)
    if isinstance(ob, PyIUnknownType):
        try:
            ob = ob.QueryInterface(pythoncom.IID_IDispatch)
        except pythoncom.com_error:
            # It is an IUnknown, but not an IDispatch, so just let it through.
            return ob
        return self._wrap_dispatch_(ob, userName, ReturnCLSID)
    return ob

This method will thus look for an IDispatch interface and this is why it works with .NET in the case when "makepy" has not been used.

However, when "makepy" has been used, then another implementation of _get_good_singleobject will be used wich is found in win32com/clientinit.py:

# XXX - These should be consolidated with dynamic.py versions.
def _get_good_single_object_(obj, obUserName=None, resultCLSID=None):
    if _PyIDispatchType==type(obj):
        return Dispatch(obj, obUserName, resultCLSID)
    return obj

In this case there is no check for an IDispatch interface! I guess that the comment in the code that someone wrote is a good piece of advice ;-)

Original comment by: thriolsson