Fix old index bug in call_with_inout (enthought#473)

* bugfix in `call_with_inout`

* minor cleanup

* handling the case of no in and no out

* Test case for _fix_inout_args

* additional cleanup and error handling

* code formatting fixed

* fix python 3.7 and 3.8 compatibility

* Temporary addition of real-world test

* code cleanup

* intermediate commit, do not review

* Refactor of unit test, removing portdevice test

* fix global side-effect of other skipped test

* Update comtypes/test/test_outparam.py

Co-authored-by: Jun Komoda <[email protected]>

* work on tests for inout_args and outparam
- cleanup for test_outparam.py
- improvements to test_inout_args.py
- comments on a possible error in _memberspec.py

* removing dead code

* rename variables and add assertions

* pass `MagicMock` instead of `ut.TestCase`

* make tests for each argument passing patterns

* remove duplicated comments

* update test code for readability
- remove name from mock
- move line breaks to between mock preparations and assertions

* split the testcases

* add `Test_Error`

* minor corrections, remove redundancy, migration
- rewrite the permutations test
- missing direction and omitted name redundant
- migrate autogenerated keywords
- TBD: more real life tests

* Add tests covering 24 patterns
- instead of using `if` statements and `permutations`

* update test name

* add real world tests, remove old code

* formatting issue

* Update comtypes/_memberspec.py

dict type annotation

Co-authored-by: Jun Komoda <[email protected]>

* Change missing 'in' or 'out' to be treated as 'in'

* Add real-world test: param without 'in' or 'out'

* add `contextlib.redirect_stdout`
to supress warnings

* apply review feedback

* update comments

* add line breaks to lines longer than 88 characters

* Update comtypes/test/test_inout_args.py

---------

Co-authored-by: jonschz <[email protected]>
Co-authored-by: Jonathan <[email protected]>
Co-authored-by: Jun Komoda <[email protected]>

Author: 3 people

comtypes/_memberspec.py

@@ -151,60 +151,93 @@ def _fix_inout_args(
     def call_with_inout(self, *args, **kw):
         args = list(args)
         # Indexed by order in the output
-        outargs = {}
+        outargs: Dict[int, _UnionT[_CData, "ctypes._CArgObject"]] = {}
         outnum = 0
+        param_index = 0
+        # Go through all expected arguments and match them to the provided arguments.
+        # `param_index` first counts through the positional and then
+        # through the keyword arguments.
         for i, info in enumerate(paramflags):
             direction = info[0]
-            if direction & 3 == 3:
+            dir_in = direction & 1 == 1
+            dir_out = direction & 2 == 2
+            is_positional = param_index < len(args)
+            if not (dir_in or dir_out):
+                # The original code here did not check for this special case and
+                # effectively treated `(dir_in, dir_out) == (False, False)` and
+                # `(dir_in, dir_out) == (True, False)` the same.
+                # In order not to break legacy code we do the same.
+                # One example of a function that has neither `dir_in` nor `dir_out`
+                # set is `IMFAttributes.GetString`.
+                dir_in = True
+            if dir_in and dir_out:
                 # This is an [in, out] parameter.
                 #
                 # Determine name and required type of the parameter.
                 name = info[1]
                 # [in, out] parameters are passed as pointers,
                 # this is the pointed-to type:
-                atyp = argtypes[i]._type_
+                atyp: Type[_CData] = getattr(argtypes[i], "_type_")
 
                 # Get the actual parameter, either as positional or
                 # keyword arg.
-                try:
-                    try:
-                        v = args[i]
-                    except IndexError:
-                        v = kw[name]
-                except KeyError:
-                    # no parameter was passed, make an empty one
-                    # of the required type
-                    v = atyp()
-                else:
-                    # parameter was passed, call .from_param() to
-                    # convert it to a ctypes type.
+
+                def prepare_parameter(v):
+                    # parameter was passed, call `from_param()` to
+                    # convert it to a `ctypes` type.
                     if getattr(v, "_type_", None) is atyp:
-                        # Array of or pointer to type 'atyp' was
-                        # passed, pointer to 'atyp' expected.
+                        # Array of or pointer to type `atyp` was passed,
+                        # pointer to `atyp` expected.
                         pass
                     elif type(atyp) is SIMPLETYPE:
-                        # The from_param method of simple types
-                        # (c_int, c_double, ...) returns a byref()
-                        # object which we cannot use since later
-                        # it will be wrapped in a pointer.  Simply
-                        # call the constructor with the argument
-                        # in that case.
+                        # The `from_param` method of simple types
+                        # (`c_int`, `c_double`, ...) returns a `byref` object which
+                        # we cannot use since later it will be wrapped in a pointer.
+                        # Simply call the constructor with the argument in that case.
                         v = atyp(v)
                     else:
                         v = atyp.from_param(v)
                         assert not isinstance(v, BYREFTYPE)
-                outargs[outnum] = v
-                outnum += 1
-                if len(args) > i:
-                    args[i] = v
-                else:
+                    return v
+
+                if is_positional:
+                    v = prepare_parameter(args[param_index])
+                    args[param_index] = v
+                elif name in kw:
+                    v = prepare_parameter(kw[name])
                     kw[name] = v
-            elif direction & 2 == 2:
+                else:
+                    # no parameter was passed, make an empty one of the required type
+                    # and pass it as a keyword argument
+                    v = atyp()
+                    if name is not None:
+                        kw[name] = v
+                    else:
+                        raise TypeError("Unnamed inout parameters cannot be omitted")
+                outargs[outnum] = v
+            if dir_out:
                 outnum += 1
+            if dir_in:
+                param_index += 1
+
         rescode = func(self, *args, **kw)
         # If there is only a single output value, then do not expect it to
         # be iterable.
+
+        # Our interpretation of this code
+        # (jonschz, junkmd, see https://github.com/enthought/comtypes/pull/473):
+        # - `outnum` counts the total number of 'out' and 'inout' arguments.
+        # - `outargs` is a dict consisting of the supplied 'inout' arguments.
+        # - The call to `func()` returns the 'out' and 'inout' arguments.
+        #   Furthermore, it changes the variables in 'outargs' as a "side effect"
+        # - In a perfect world, it should be fine to just return `rescode`.
+        #   But we assume there is a reason why the original authors did not do that.
+        #   Instead, they replace the 'inout' variables in `rescode` by those in
+        #   'outargs', and call `__ctypes_from_outparam__()` on them.
+
         if outnum == 1:  # rescode is not iterable
+            # In this case, it is little faster than creating list with
+            # `rescode = [rescode]` and getting item with index from the list.
             if len(outargs) == 1:
                 rescode = rescode.__ctypes_from_outparam__()
             return rescode

comtypes/test/test_imfattributes.py

@@ -0,0 +1,29 @@
+import contextlib
+import unittest as ut
+
+from ctypes import POINTER, pointer, windll
+from comtypes import GUID
+import comtypes.client
+
+
+class Test_IMFAttributes(ut.TestCase):
+    def test_imfattributes(self):
+        with contextlib.redirect_stdout(None):  # supress warnings, see test_client.py
+            comtypes.client.GetModule("msvidctl.dll")
+        from comtypes.gen import MSVidCtlLib
+
+        imf_attrs = POINTER(MSVidCtlLib.IMFAttributes)()
+        hres = windll.mfplat.MFCreateAttributes(pointer(imf_attrs), 2)
+        self.assertEqual(hres, 0)
+
+        MF_TRANSCODE_ADJUST_PROFILE = GUID("{9c37c21b-060f-487c-a690-80d7f50d1c72}")
+        set_int_value = 1
+        # IMFAttributes.SetUINT32() is an example of a function that has a parameter
+        # without an `in` or `out` direction; see also test_inout_args.py
+        imf_attrs.SetUINT32(MF_TRANSCODE_ADJUST_PROFILE, set_int_value)
+        get_int_value = imf_attrs.GetUINT32(MF_TRANSCODE_ADJUST_PROFILE)
+        self.assertEqual(set_int_value, get_int_value)
+
+
+if __name__ == "__main__":
+    ut.main()