Repository navigation
fix: generate signed byte types for VT_I1 - #962
haoxuanjng-lang wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #962 +/- ##
==========================================
+ Coverage 89.34% 89.38% +0.03%
==========================================
Files 142 143 +1
Lines 13856 13931 +75
==========================================
+ Hits 12380 12452 +72
- Misses 1476 1479 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
junkmd
left a comment
junkmd
left a comment
There was a problem hiding this comment.
Thank you for your continued contributions!
This is clearly an inconsistency, and it is normally not the correct behavior for software. However, I could not find any earlier bug reports about this specific behavior. This makes me wonder whether users have come to expect the current behavior over time, or whether no one has ever reached this code path.
So, let's move forward step by step, checking both the impact of this change and backward compatibility.
As before, let's start with the test structure.
junkmd
left a comment
junkmd
left a comment
There was a problem hiding this comment.
Thank you for adding the native COM test.
It shows that signed 8-bit integers work correctly across the COM boundary in Echo....
It also confirms that explicit LPSTR still uses the existing string handling.
These tests are useful for checking number marshaling and making sure existing string handling still works. However, I noticed that these are distinct concerns.
We also need to check whether existing Python calls remain compatible with or break the new mapping.
Checking Backward Compatibility
To better understand the change, I wrote a parameterized test that passes different types of 1-byte data as arguments. I compared the results before and after the change in tlbparser.
For this test, I expected the return value to stay the same, even when the argument type changed.
def test_backward_compatibility(self):
for i, value in enumerate(
[
b"h",
pointer(c_byte(ord("h"))),
byref(c_byte(ord("h"))),
(c_byte * 1)(ord("h")),
]
):
with self.subTest(index=i, value=value):
self.assertEqual(self.server.ReadCharPointer(value), ord("h"))The results differ before and after the change.
Before the change, b"h" works, but the other three arguments raise ctypes.ArgumentError.
test_backward_compatibility (comtypes.test.test_cpp_byte_echo.ByteEchoTest.test_backward_compatibility) ...
test_backward_compatibility (comtypes.test.test_cpp_byte_echo.ByteEchoTest.test_backward_compatibility) (index=1, value=<ctypes.LP_c_byte object at 0x0000025878B3F250>) ... ERROR
test_backward_compatibility (comtypes.test.test_cpp_byte_echo.ByteEchoTest.test_backward_compatibility) (index=2, value=<cparam 'P' (0x0000025878F938D8)>) ... ERROR
test_backward_compatibility (comtypes.test.test_cpp_byte_echo.ByteEchoTest.test_backward_compatibility) (index=3, value=<comtypes.test.test_cpp_byte_echo.c_byte_Array_1 object at 0x0000025878F93A20>) ... ERROR
======================================================================
ERROR: test_backward_compatibility (comtypes.test.test_cpp_byte_echo.ByteEchoTest.test_backward_compatibility) (index=1, value=<ctypes.LP_c_byte object at 0x0000025878B3F250>)
----------------------------------------------------------------------
Traceback (most recent call last):
File "path\to\comtypes\test\test_cpp_byte_echo.py", line 65, in test_backward_compatibility
self.assertEqual(self.server.ReadCharPointer(value), ord("h"))
~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^
ctypes.ArgumentError: argument 1: TypeError: 'LP_c_byte' object cannot be interpreted as ctypes.c_char_p
======================================================================
ERROR: test_backward_compatibility (comtypes.test.test_cpp_byte_echo.ByteEchoTest.test_backward_compatibility) (index=2, value=<cparam 'P' (0x0000025878F938D8)>)
----------------------------------------------------------------------
Traceback (most recent call last):
File "path\to\comtypes\test\test_cpp_byte_echo.py", line 65, in test_backward_compatibility
self.assertEqual(self.server.ReadCharPointer(value), ord("h"))
~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^
ctypes.ArgumentError: argument 1: TypeError: '_ctypes.CArgObject' object cannot be interpreted as ctypes.c_char_p
======================================================================
ERROR: test_backward_compatibility (comtypes.test.test_cpp_byte_echo.ByteEchoTest.test_backward_compatibility) (index=3, value=<comtypes.test.test_cpp_byte_echo.c_byte_Array_1 object at 0x0000025878F93A20>)
----------------------------------------------------------------------
Traceback (most recent call last):
File "path\to\comtypes\test\test_cpp_byte_echo.py", line 65, in test_backward_compatibility
self.assertEqual(self.server.ReadCharPointer(value), ord("h"))
~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^
ctypes.ArgumentError: argument 1: TypeError: 'c_byte_Array_1' object cannot be interpreted as ctypes.c_char_p
----------------------------------------------------------------------
Ran 1 test in 0.027s
FAILED (errors=3)Generated Code (Only `IByteEchoTest`)
class IByteEchoTest(comtypes.gen._00020430_0000_0000_C000_000000000046_0_2_0.IUnknown):
_case_insensitive_ = True
_iid_ = GUID('{047E34C1-33A8-4DAA-A1E5-B941756A8319}')
_idlflags_ = ['oleautomation']
if TYPE_CHECKING: # commembers
def EchoInt8(self, value: hints.Incomplete) -> hints.Incomplete: ...
def EchoUint8(self, value: hints.Incomplete) -> hints.Incomplete: ...
def EchoLpStr(self, value: hints.Incomplete) -> hints.Incomplete: ...
def ReadCharPointer(self, value: hints.Incomplete) -> hints.Incomplete: ...
IByteEchoTest._methods_ = [
COMMETHOD(
[],
HRESULT,
'EchoInt8',
(['in'], c_char, 'value'),
(['out', 'retval'], STRING, 'result')
),
COMMETHOD(
[],
HRESULT,
'EchoUint8',
(['in'], c_ubyte, 'value'),
(['out', 'retval'], POINTER(c_ubyte), 'result')
),
COMMETHOD(
[],
HRESULT,
'EchoLpStr',
(['in'], STRING, 'value'),
(['out', 'retval'], POINTER(STRING), 'result')
),
COMMETHOD(
[],
HRESULT,
'ReadCharPointer',
(['in'], STRING, 'value'),
(['out', 'retval'], POINTER(c_ubyte), 'result')
),
]↓
After the change, the other three arguments work, but b"h" raises ctypes.ArgumentError.
test_backward_compatibility (comtypes.test.test_cpp_byte_echo.ByteEchoTest.test_backward_compatibility) ...
test_backward_compatibility (comtypes.test.test_cpp_byte_echo.ByteEchoTest.test_backward_compatibility) (index=0, value=b'h') ... ERROR
======================================================================
ERROR: test_backward_compatibility (comtypes.test.test_cpp_byte_echo.ByteEchoTest.test_backward_compatibility) (index=0, value=b'h')
----------------------------------------------------------------------
Traceback (most recent call last):
File "path\to\comtypes\test\test_cpp_byte_echo.py", line 65, in test_backward_compatibility
self.assertEqual(self.server.ReadCharPointer(value), ord("h"))
~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^
ctypes.ArgumentError: argument 1: TypeError: expected LP_c_byte instance instead of bytes
----------------------------------------------------------------------
Ran 1 test in 0.025s
FAILED (errors=1)Generated Code (Only `IByteEchoTest`)
class IByteEchoTest(comtypes.gen._00020430_0000_0000_C000_000000000046_0_2_0.IUnknown):
_case_insensitive_ = True
_iid_ = GUID('{047E34C1-33A8-4DAA-A1E5-B941756A8319}')
_idlflags_ = ['oleautomation']
if TYPE_CHECKING: # commembers
def EchoInt8(self, value: hints.Incomplete) -> hints.Incomplete: ...
def EchoUint8(self, value: hints.Incomplete) -> hints.Incomplete: ...
def EchoLpStr(self, value: hints.Incomplete) -> hints.Incomplete: ...
def ReadCharPointer(self, value: hints.Incomplete) -> hints.Incomplete: ...
IByteEchoTest._methods_ = [
COMMETHOD(
[],
HRESULT,
'EchoInt8',
(['in'], c_byte, 'value'),
(['out', 'retval'], POINTER(c_byte), 'result')
),
COMMETHOD(
[],
HRESULT,
'EchoUint8',
(['in'], c_ubyte, 'value'),
(['out', 'retval'], POINTER(c_ubyte), 'result')
),
COMMETHOD(
[],
HRESULT,
'EchoLpStr',
(['in'], STRING, 'value'),
(['out', 'retval'], POINTER(STRING), 'result')
),
COMMETHOD(
[],
HRESULT,
'ReadCharPointer',
(['in'], POINTER(c_byte), 'value'),
(['out', 'retval'], POINTER(c_ubyte), 'result')
),
]The return value stays the same for the calls that succeed.
These results show that the new mapping allows calls that match the new type, but breaks some calls that worked with the old type. In other words, the accepted argument types change, even though the data still represents the same 1-byte value.
Checking the Other Direction
I think we should also test the opposite case of ReadCharPointer.
For example, we could add a method like GetCharacterBytes. It would take an integer such as ord("h") and return a Python bytes object such as b"h".
We should check what happens to existing calls after the mapping changes. Does the call raise ctypes.ArgumentError, or does it succeed but return a different value, causing the assertion to fail?
We should then test different combinations of argument and return types to find out which combinations pass. For this investigation, we can set aside whether each combination meets the original requirements.
By testing both directions, we can better understand how the mapping change affects arguments and return values, and what changes users may need to make to their Python code.
Docs for Compatibility
Some users define their interfaces in Python by inheriting from IUnknown or IDispatch. Others keep the generated module source code in their own repositories. These users may be able to fix problems by changing the type definitions themselves.
However, users who use modules generated from type libraries without changing them cannot easily fix the generated types. They need to know why the types have changed, which existing calls are affected, and how to update their code.
If we move forward with this change, I think we should explain its impact and how to migrate in a migration guide or the release notes.
In particular, users who use generated modules as they are need enough information to decide what changes to make. We should explain:
- Which
VT_I1arguments and return values will have different generated types. - Which existing calls will stop working.
- Which values or
ctypestypes users should pass after the change. - How explicitly declared string types, such as
LPSTR, are handled.
Alternative Approach
Another option is to choose the type based on more detailed information instead of looking it up in the COMTYPES mapping by VT_* alone. If we consider this approach, we may need a different test double to test it.
I think these tests will help us understand the compatibility impact before deciding how to proceed.
|
I am planning to add a component to the C++ test double to address another open issue in this project. This may cause conflicts with this PR, so please handle them if they occur. |
|
Added the reverse-direction probe and expanded the real native-call matrix. The separate component/interface/test module are now named The output probe writes one character through a plain Observed with freshly generated wrappers against latest-main production versus the proposed mapping:
This establishes the opposite direction's accepted-buffer change independently of SAFEARRAYs. Explicit LPSTR and unsigned scalar tables still pass both versions. I retained the two legacy input/output assertions as expected failures and added documentation for affected generated types, saved/handwritten interfaces, replacement arguments, signed outputs and explicit string handling. Full local unittest: 474 run / 448 passed / 24 skipped / 2 expected compatibility failures. Focused suites: 70 / 59 / 9 / 2; without NumPy/pywin32: 51 / 41 / 8 / 2. Native build, full Ruff CI scope and strict Sphinx HTML build pass. Existing full docs doctest cannot get past its optional Excel type-library setup locally ( The mapping itself is unchanged in this follow-up and the PR remains draft. These results and the documentation describe the compatibility cost; they do not resolve it or decide whether a more detailed mapping should be used. |
junkmd
left a comment
junkmd
left a comment
There was a problem hiding this comment.
Thank you for adding the new test double!
I am going to add another test double to source/CppTestSrv/ as well.
If any conflicts happen, it would be great if you could resolve them again.
| def test_character_pointer_output(self): | ||
| for value in (0, 1, ord("h"), 128, 255): | ||
| with self.subTest(value=value): | ||
| # Plain char* output uses the signed numeric mapping as well. | ||
| self.assertEqual( | ||
| self.server.GetCharacterBytes(value), | ||
| value if value < 128 else value - 256, | ||
| ) | ||
|
|
||
| def test_character_ctypes_output_buffers(self): | ||
| for value in (0, ord("h"), 128, 255): | ||
| for result in (c_byte(), (c_byte * 1)()): | ||
| with self.subTest(value=value, result=result): | ||
| argument = byref(result) if isinstance(result, c_byte) else result | ||
| self.server._IByteCompatibilityTest__com_GetCharacterBytes( | ||
| value, argument | ||
| ) | ||
| actual = result.value if isinstance(result, c_byte) else result[0] | ||
| self.assertEqual(actual, value if value < 128 else value - 256) |
There was a problem hiding this comment.
Having if statements in the test and using values that are not fixed makes it a bit hard for me to read.
If the way we check values changes depending on the data type in parametrize, it might be better to split them into separate tests.
If the type is the same but the values are different, adding the expected value to the test parameters would make it easier to compare.
It could also be helpful to separate the test cases for old-mapping vs proposed-mapping.
I'm having trouble picturing the full behavior here. Could you walk me through it with test codebases?
There was a problem hiding this comment.
Split the test code into explicit groups and removed the datatype-dependent branches and computed expected values:
ByteCompatibilityTestretains the scalar/LPSTR smoke tests.LegacyMappingCompatibilityTestcontains only the old bytes-input and character-buffer-output assertions. Both pass under the old production mapping and remain expected failures under the proposal.ProposedMappingCompatibilityTesthas independent pointer, byref and array tests in each direction. Each assertion reads a fixed(input, expected)pair. Automatic outputs use(128, -128)and(255, -1); raw buffer assertions read the corresponding fixed storage shape without branching.
To make the behavior concrete, these are the actual calls represented by those classes:
| Test group / call | Old mapping | Proposed mapping |
|---|---|---|
Legacy: ReadCharPointer(b"h") |
104 | ArgumentError |
Legacy: raw GetCharacterBytes(104, create_string_buffer(2)), then .value |
b"h" | ArgumentError |
Proposed: pointer/byref/array containing c_byte(-128) passed to ReadCharPointer |
ArgumentError | 128 |
| Proposed: raw output of 128 into pointer/byref/array of c_byte | ArgumentError | stored -128 |
Proposed: automatic GetCharacterBytes(255) |
TypeError during implicit STRING output allocation | -1 |
| Common: explicit LPSTR and unsigned echoes | pass | pass |
I reran the old-mapping stage using freshly generated wrappers and the rebuilt fixture: the two legacy assertions pass; the proposed group reports 29 subcase errors across 7 tests; explicit LPSTR and unsigned cases pass. With the proposed mapping, focused suites run 76 tests (9 skipped, 2 expected failures), and the full suite runs 482 (24 skipped, 2 expected failures). The expected failures continue to disclose the compatibility cost.
Also merged main 91408c2 and resolved the Makefile conflict, retaining your new VT_ERROR component alongside the existing fixtures. MIDL/C++ rebuild and full Ruff CI scope pass. The source now provides separate runnable legacy/proposed classes to follow through each behavior.
The production mapping proposal is unchanged and this stays draft pending the compatibility strategy. CI for the new head is pending. OpenAI Codex assisted with this revision.
Generating
VT_I1asc_charchanges native signed values such as -7 to 249 in tuples and produces byte-string arrays or reshape errors with NumPy. Proposesigned char/c_byteforVT_I1, leaving explicitVT_LPSTRas a narrow string. Follow-up to #956 and the discussion in #935; this does not reproduce or resolve #935's intermittent UIA exception.Draft: plain character-pointer compatibility regressions remain unresolved. Old bytes input and writable character-buffer output work with the old mapping and raise
ArgumentErrorwith the proposed mapping. The two assertions remain explicit expected failures.The separate native
CoByteCompatibilityTestfixture exercises scalar echoes, explicit LPSTR, and plain character pointers through generated interfaces and real out-of-process COM calls. Tests now separate:ByteCompatibilityTest: signed/unsigned scalar echoes and explicit LPSTR.LegacyMappingCompatibilityTest: the two old calls that regress.ProposedMappingCompatibilityTest: separate pointer, byref and array methods for numeric inputs and raw output buffers, plus automatic signed output.Expected values are literal input/result pairs; no datatype-dependent branches or calculated signed expectations remain. The output fixture writes one character into caller-owned storage and does not assume a native NUL terminator. The migration page explains affected generated versus saved/handwritten interfaces, arguments, signed outputs and explicit strings. Independent 1-D/2-D numeric SAFEARRAY value/shape/dtype/copy checks remain.
Merged current main
91408c25247bec90b09db0bbc7717f47438cb06f, resolving the Makefile conflict and preserving both upstream string-SAFEARRAY-in-VARIANT and VT_ERROR native components. The original published fixture interfaces are unchanged.Observed local validation on Windows x64, Python 3.13.5, NumPy 2.5.3, pywin32 312, MSVC 19.42:
nmake /f MAKEFILE; each stage regenerates fixture wrappers, uses temporary HKCU registrations and verifies cleanup.python -m unittest discover -v -s comtypes/test -t comtypes/test: 482 run / 456 passed / 24 skipped / 2 expected compatibility failures, including upstream VT_ERROR tests.git diff --check origin/mainpasses.Earlier revision only: related suites without NumPy/pywin32 passed 51 run / 41 passed / 8 skipped / 2 expected failures; strict Sphinx HTML passed, while full docs doctest stopped in optional Excel setup. Those checks were not rerun for this test restructuring; documentation and production mapping were unchanged.
CI for this new revision is pending. The prior head's 76 successful checks do not validate this head. The PR remains draft pending the compatibility strategy.
AI assistance: OpenAI Codex helped investigate, implement and validate this contribution.