Repository navigation
fix: generate signed byte types for VT_I1 - #962
haoxuanjng-lang wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #962 +/- ##
==========================================
+ Coverage 89.34% 89.47% +0.13%
==========================================
Files 142 143 +1
Lines 13856 14049 +193
==========================================
+ Hits 12380 12571 +191
- Misses 1476 1478 +2 ☔ 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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
|
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. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
junkmd
left a comment
junkmd
left a comment
There was a problem hiding this comment.
This PR is logically consistent, regardless of compatibility, and the tests have become clearer with each review. However, I could not explain why it still felt difficult to understand.
I have just realized that the tests do not yet give us a clear record of this compatibility change.
At the moment, the tests only cover the new mapping. It was hard to compare how the old mapping worked, and what happens when we use the old calling style with the new mapping.
I came up with the fixtures to make this easier to understand.
This mapping seems to have survived unchanged for about 20 years, at least as far back as our version history goes. Changing something that old feels A LITTLE scary!
Please bear with me a little longer, at least until I can understand this change well enough myself.
Thank you!
This comment was marked as outdated.
This comment was marked as outdated.
junkmd
left a comment
junkmd
left a comment
There was a problem hiding this comment.
I think it has become much easier to understand.
I'll just mention one thing that caught my attention when I briefly glanced at the code.
| @unittest.expectedFailure | ||
| def test_char_pointer_bytes_input(self): | ||
| self.assertEqual(self.server.ReadCharPointer(b"h"), 104) | ||
|
|
||
| @unittest.expectedFailure | ||
| def test_character_bytes_output_buffer(self): | ||
| result = create_string_buffer(2) | ||
| self.server._IByteCompatibilityTest__com_GetCharacterBytes(104, result) | ||
| self.assertEqual(result.value, b"h") |
There was a problem hiding this comment.
Since expectedFailure hides the reason for the failure, it might be better to use assertRaises instead.
If an assertion fails, please use assertNotEqual for the expected value in the legacy mapping and assertEqual for the expected value in the new mapping, and add a clarifying comment.
| def test_legacy_int8_implicit_output(self): | ||
| # STRING cannot allocate a scalar output. Keep that legacy behavior | ||
| # visible rather than silently adapting it to the proposed result type. | ||
| for value in (b"\x80", b"\xf9", b"\xff", b"\0", b"\x01", b"\x7f"): | ||
| with self.subTest(mapping="legacy", value=value): | ||
| with self.assertRaises(TypeError): | ||
| self.legacy.EchoInt8(value) | ||
|
|
||
| def test_legacy_int8_explicit_output(self): | ||
| for value in (b"\x80", b"\xf9", b"\xff", b"\0", b"\x01", b"\x7f"): | ||
| with self.subTest(mapping="legacy", value=value): | ||
| result = create_string_buffer(2) | ||
| self.legacy._IByteCompatibilityTest__com_EchoInt8(value, result) | ||
| self.assertEqual(result.raw[:1], value) |
There was a problem hiding this comment.
Performing buffer creation and result slicing—which explicit_output does—inside the tests might be reducing readability.
Please split the create_legacy_itf factory into two versions: one that overrides the EchoInt8 method to encapsulate buffer creation and result slicing, and one that does not. And please give each of those factories a good, distinguishable name. Then, please place them into the happy path and unhappy path subtests.
Please move comments like # STRING cannot allocate a scalar output. to the section where the method is overridden and encapsulated within the factory.
|
I opened #969 to record decisions about this change and provide a place for future support. When referring to this mapping in code comments or documentation, please link to this issue. |
|
To understand real-world use cases of You can review the implementation in a409eea. The warnings alone are shown below. Please investigate the use cases of these interfaces and structures to determine whether the impact of changing the mapping is small, or whether switching to the new mapping would fix bugs. |
|
Pushed the requested test organization and explicit compatibility assertions:
Observed Windows x64 / Python 3.13.5 / NumPy 2.1.3 / pywin32 308 results: focused 75 run, 66 passed, 9 skipped; full unittest discovery 481 run, 457 passed, 24 skipped. No expected failures remain. Old-mapping regeneration matches the legacy definitions; the proposed-definition assertion fails and the original native SAFEARRAY tests still fail (3 failures, 1 error). Ruff 0.6.9 passes the complete CI lint/format scope in an isolated worktree (151 files); the migration RST parses without warnings. Temporary HKCU registrations were cleaned up and the last focused run regenerated the proposed fixture wrappers. The native binary is unchanged and was reused; I did not rerun its build, the optional-dependency-free environment or the Sphinx documentation build. Production behavior is unchanged from the previous PR revision. Draft status remains while the compatibility/transition plan is discussed; upstream CI for this new head is tracked separately. AI assistance: OpenAI Codex. |
Generating
VT_I1asc_charchanges native signed values such as -7 to 249 in tuples and produces byte-string arrays or reshape errors with NumPy. This PR proposessigned char/c_byteforVT_I1; explicitVT_LPSTRremains a narrow string. The scope and compatibility decision are tracked in #969. Context: #956 and #935; the intermittent UIA exception in the latter is outside this change.Draft: the backward-compatibility and transition plan remains pending. Regenerated plain character-pointer interfaces reject old bytes arguments and writable character buffers. Tests assert those specific
ArgumentErrorcases; there are noexpectedFailuredecorators, and passing tests do not imply backward compatibility is restored.The native
CoByteCompatibilityTestcomponent supplies real out-of-process calls.test_compatibility_tlbparser_VT_I1_assignment.pycompares saved legacy and proposed COMMETHOD definitions and the freshly generated interface. Cases are grouped by interface method and successful/error path. Distinct legacy factories expose automatic STRING output versus explicit character storage, with the buffer handling inside the factory. Proposed helpers cover automatic, pointer, reference and array output. LPSTR helpers free the native allocation infinally. Separate 1-D/2-D SAFEARRAY tests check signed values, shape, dtype and copy behavior, and a migration page describes changes to scalar, pointer and record fields.Real-library investigation in #969 covers regenerated SAPI confidence fields, PortableDevice generated union/count-array fields, mscorlib SByte/ToSByte and the MSVidCtl Error declaration. It establishes generated-type and local-record behavior, not end-to-end speech/device/CLR/event compatibility. Record sizes/offsets match on the tested x64 installation; application compatibility and the warning/transition policy remain open.
Observed local validation on Windows x64, Python 3.13.5, NumPy 2.1.3, pywin32 308:
python -m unittest discover -v -s comtypes/test -t comtypes/test: 481 run / 457 passed / 24 skipped. No expected failures.git diff --checkpasses. Migration RST parses without warnings/errors.Based on upstream main
91408c25247bec90b09db0bbc7717f47438cb06f. This revision changes the Python tests, a mapping comment and a documentation link/field-scope clarification; production behavior and native source/binary are unchanged. The existing native build is reused. Sphinx, optional-dependency-free testing and end-to-end Microsoft application scenarios were not rerun. Upstream CI is checked against the published head after push, separately from these local results.AI assistance: OpenAI Codex helped investigate, implement and validate this contribution.