Fix old index bug in call_with_inout

[jonschz]

Another day, another pull request...

I found this rather ancient bug in call_with_inout. It triggers whenever a method takes an ['out'] parameter followed by an ['in', 'out'] parameter, which seems to be rather uncommon. My specific problem was with IPortableDeviceContent::CreateObjectWithPropertiesAndData.

Before this bugfix it was impossible to call this method without getting an error. Providing four arguments would result in call takes exactly 4 arguments (5 given), and providing the correct three arguments would result in 'LP_c_wchar_p' object cannot be interpreted as an integer, since the old code would mismatch the expected and actual arguments.

I am fairly certain this change will not break any correct existing code, but there is a chance it may break some code containing a workaround for this bug.

Fun fact: git blame shows that this code (including the bug) was pushed 14 years ago in 8776b1b and never modified.

I also cleaned up the code a little bit which does not affect the logic at all, but increases readability.

One open question
We should think about how to handle the case when a parameter has neither 'out' nor 'in' is specified. As far as I am aware, this does not happen in the code generator, but it if did, it would likely cause this code to malfunction. One idea would be to raise an exception, the other idea would be to log a warning and then explicitly set the parameter to be 'in'. I think the old code does the latter implicitly.

One could also consider merging this bugfix into master since it is a bugfix for existing python2 code.

[junkmd]

Thank you for your contribution!

It is a happy situation for OSS maintainers such as me to the community has contributors who are willing to investigate defects and PR proper level of granularity.

I think this problem is getting to the crux of this project.

The _fix_inout_args may have never been tested by boundary values.

As I mentioned in #346, the non-skipped existing tests succeeded with or without the _fix_inout_args. I also did not encounter any problems with the projects I am involved with that use comtypes.

I think it is necessary another PR adding the tests with three cases.

• Test the behavior of methods that can work without the _fix_inout_args
• Test the behavior of methods that can work with the current _fix_inout_args.
• Test the behavior of methods that can work with Fix old index bug in call_with_inout #473 changes
  • Until the production code is changed, this test must be decorated by the unittest.expectedFailure.

Once the test-only PR is merged, you can simply rebase this branch.

This is my opinion and there may be other better ways to do.

Any opinions also would be appreciated.

[vasily-v-ryabov]

Thanks a lot! I'd suggest to backport it to current master branch as well as a separate PR. From point of being explicit, we should raise an exception in case in, out or inout specification is not set. I'm pretty sure this situation should never happen, but never say "Never!". :) Also something like assert_never is additionally useful from static typing point of view (for drop_py2 branch).

[jonschz]

I added a unit test here which fails on the unmodified _memberspec.py. I also updated the error handling and made the specific routine type safe, revealing another potential edge case which I handled. In my opinion it would be fine to just merge both the fix and the test together, but I could also make a separate pull request for the test only if you prefer that.

[junkmd]

OK, adding tests and changing the production code in one PR would be fine.

However, it is better to use the actual com library for testing.

Please show us the code that reproduces the problem you encountered while using IPortableDeviceContent.CreateObjectWithPropertiesAndData.

I am guessing that IPortableDeviceContent.CreateObjectWithPropertiesAndData will be environment dependent in behavior, like other portabledevice api.

However, just as you were able to create the IEnum test from the DirectShow api, members of the community may be able to come up with a more suitable test from your code.

[jonschz]

I have temporarily added a test here to be removed later. It does not create new files on my device, but I can't guarantee that it never does so.

[junkmd]

My guess is that you will continue to improve this test code (even if in your local env) and post issues about technical problems that occurred there.

Therefore, please keep in mind that this code will be shared with us again in the future, so please make sure that it is more readable.

[junkmd]

Suggested change

	WPD_OBJECT_NAME = PropertyKey(
	0xEF6B490D, 0x5CD8, 0x437A, 0xAF, 0xFC, 0xDA, 0x8B, 0x60, 0xEE, 0x4A, 0x3C, 4)
	WPD_OBJECT_NAME = PropertyKey(comtypes.GUID("{EF6B490D-5CD8-437A-AFFC-DA8B60EE4A3C}"), 4)

It should be applied to other PropertyKeys.

[jonschz]

The reason I did it this way is because it allows easy copy-paste from the Windows header files, eliminating a possible source of errors. I can of course change that.

[jonschz]

Oh, a misunderstanding - I was under the impression that you merely wanted to see some example test code and didn't want to integrate this test into production code (especially because I can't guarantee that it doesn't modify content on portable devices). I will update the code.

[junkmd]

I would agree that add a test using portabledevice to the project, under the following conditions ...

• Assume the target device is a sandbox consisting of a few folders and a few very small files.
• Use os.path, glob, pathlib, etc. to check if the target device has the same directory structure as the expected sandbox or skip the test if not.
• Eliminate the using conditional branches such as inside of findDir that are not used to skip or fail tests.
• Make assertions that inspect contents of the target device.
• If doing something like RemoteWrite, create a temporary folder using tempfile or something similar and write to it.
• If possible, assert after the test that the sandbox is not different from before the test.

Existing this test is no matter, however, be that as it may, I think it is necessary to test _fix_inout_args in another environment-independent way, since it is difficult to always require such a sandbox in developer and CI environments.

[junkmd]

As for RemoteWrite, if it can be verified by tests like #474, I think not need to take the risk of testing it with portabledevice.

[jonschz]

However, it is better to use the actual com library for testing.

Would you reconsider your stance on this matter, since it appears to be hard to catch all edge cases of _fix_inout_args? I have some unpublished improvements for test_inout_args.py that give an almost 100% test coverage of _fix_inout_args.

[junkmd]

Thanks a lot for your contribution!

Indeed, I too felt that it would be difficult to cover all edges in the actual COM library.
Sorry for pushing you to do this.

some unpublished improvements for test_inout_args.py that give an almost 100% test coverage of _fix_inout_args.

I want to see!

The following is my opinion.

Currently you are trying to cover the edges implicitly with subTest, but it would be better to name the tests explicitly for each test perspective.
Then, for each test, you can comment "this edge is assumed to be a ham and is also covered by test_foo.py", "this edge is assumed to be a lettuce but is not covered by the test with the actual com library", etc.

Then, you can use a mock to assert how the function was actually called.
Below is an example of testing a higher-order function that adds very simple arguments.

import functools as ft
from typing import Any, Callable
import unittest as ut
from unittest.mock import MagicMock


def add_a_eqaul_one_arg(func: Callable[..., Any], *args, **kwargs):
    @ft.wraps(func)
    def wrapper(*args, **kwargs):
        return func(*args, **kwargs, a=1)

    return wrapper

class Test(ut.TestCase):
    def test(self):
        m = MagicMock()
        new_m = add_a_eqaul_one_arg(m)
        new_m(4, 3, b=2)
        m.assert_called_once_with(4, 3, a=1, b=2)

I think that such a test is necessary if it cannot be provided actual stuffs.

[jonschz]

I have rewritten and expanded the unit test, feel free to take a look when you have time.

Surprisingly, a different test had some global side effects despite it being skipped, breaking my new test. I made some modifications there as well.

Regarding the lines

comtypes/comtypes/_memberspec.py

Lines 186 to 188 in dac3a12

	else:
	v = atyp.from_param(v)
	assert not isinstance(v, BYREFTYPE)

I was unable to find a practical use case where they would be called, and none of the existing tests cover them either. Any advice would be appreciated. If it turns out that they don't appear to be used at all, we could add a comment explaining the situation but leave the code in for backwards compatibility.

[junkmd]

Thank you for further research.

If neither 'in' nor 'out' is specified, the argument will be treated as 'in', which is what the legacy code did

I think certainly because a real COM library like MSVidCtlLib has such an implementation.

My question is, can you actually instantiate the IMFAttributes defined in MSVidCtlLib like test_ienum and call GetItemType or GetString?

I know from previous discussions that it is difficult to test everything with real things. However, if it is possible to do even part of it with real things, I think it is better to do it.

[jonschz]

I have added a real-life test as requested. @junkmd , could you clear the "Changes requested" review?

changes are OK

[junkmd]

Thank you for all your efforts.
The test you added is also a good use case for windll.mfplat.MFCreateAttributes.

I added the comtypes.client.GetModule("msvidctl.dll") runtime warning suppression that was in test_client and test_ienum.

I also confirmed that CIs are passed.

memo

https://ci.appveyor.com/project/cfarrow/comtypes/builds/46670688
https://github.com/enthought/comtypes/actions/runs/4579861707/jobs/8088032533?pr=473
https://ci.appveyor.com/project/pywinauto/pywinauto/builds/46670707

Let's wait for @vasily-v-ryabov's review.

[vasily-v-ryabov]

In general the logic looks OK to me. I can only rely on your testing. So please proceed with the fix.

And sorry for such a long waiting. Hope @junkmd can finish work on release 1.3.0. Please don't wait for my opinion if I don't reply for more than 1 week. Most probably 2024 will be too busy again for me. 😄 But I never say "never" to open source projects! 😄

[vasily-v-ryabov]

I think the only reason is that singe rescode is not a tuple (iterable) so it's not possible to convert it to list by list(rescode). It can be easily rewritten by creating list with rescode = [rescode], but there is no such need as current implementation should be very little faster. :)

[junkmd]

I agree with this point.

It is necessary to add comments explaining why outargs[0] and rescode indicate the same thing when outnum == 1 and len(outargs) == 1.

• It would also be useful to comment on the line where the variable declaration is located.

However, regarding the implementation, I believe it's fine as it is, considering the desire to minimize changes and the slightly faster processing speed.

[jonschz]

It would also be useful to comment on the line where the variable declaration is located.

What exactly would you like to see commented there, that is not redundant with the information in the big comment block? If you have a specific idea, feel free to change it and then merge. Thank you for your efforts!

[vasily-v-ryabov]

In my brief understanding the comment lines 252 - 260 are not necessary. Though some comment about better speed is also OK.

[junkmd]

It would also be useful to comment on the line where the variable declaration is located.

What exactly would you like to see commented there, that is not redundant with the information in the big comment block? If you have a specific idea, feel free to change it and then merge. Thank you for your efforts!

I tried moving the comment to the variable declaration section, but it became confusing.

I am currently considering comments that would be helpful to future maintainers.
I will add a commit with corrected comments after this.

[junkmd]

@jonschz

I appreciate your waiting.
Are you still interested in this topic?

[jonschz]

Quick with your responses as always, I appreciate it! I'll most likely update the branch today or tomorrow.

[junkmd]

I added a commit.

I updated the method and class name quoting in some comments.
For outnum==1, only processing speed was mentioned and outargs and rescode are not mentioned, because I rethink that it would be better to read the codebase than quite long comments to understand the implementation.

[junkmd]

Thank you for dealing with this topic over a long period of time.

This was a very old bug, as the PR title suggests, as well as production code that had not been boundary value tested before.

Your contribution is very beneficial.

[jonschz]

Thank you for your efforts at maintaining this package! You greatly contributed to the end result here.