Replace alias definitions with ctypes.wintypes imports for #662 #694
Conversation
comtypes/git.py
Outdated
| ) | ||
|
|
||
| DWORD = c_ulong | ||
|
|
||
|
|
||
| class IGlobalInterfaceTable(IUnknown): |
There was a problem hiding this comment.
)
-
class IGlobalInterfaceTable(IUnknown):
comtypes/GUID.py
Outdated
| def binary(obj: "GUID") -> bytes: | ||
| return bytes(obj) | ||
|
|
||
|
|
||
| BYTE = c_byte | ||
| WORD = c_ushort | ||
| DWORD = c_ulong | ||
|
|
||
| _ole32 = oledll.ole32 |
There was a problem hiding this comment.
def binary(obj: "GUID") -> bytes:
return bytes(obj)
+
_ole32 = oledll.ole32
comtypes/__init__.py
Outdated
| @@ -25,6 +25,7 @@ | |||
| from ctypes import HRESULT # noqa | |||
| from ctypes import _Pointer, _SimpleCData # noqa | |||
| from ctypes import c_int, c_ulong, oledll, windll | |||
| from ctypes.wintypes import DWORD | |||
There was a problem hiding this comment.
-from ctypes.wintypes import DWORD
+from ctypes.wintypes import DWORD # noqaLinter check disabled on import statement, no direct use of import in module but tests depend on DWORD import in __init__.py
|
Thanks for your quick responses. |
| @@ -1,7 +1,8 @@ | |||
| """comtypes.GUID module""" | |||
|
|
|||
| from ctypes import oledll, windll | |||
| from ctypes import byref, c_byte, c_ushort, c_ulong, c_wchar_p, Structure | |||
| from ctypes import byref, c_wchar_p, Structure | |||
| from ctypes.wintypes import BYTE, WORD, DWORD | |||
There was a problem hiding this comment.
You probably don't want to use BYTE
See this issue: python/cpython#60580
There was a problem hiding this comment.
According to Microsoft Win32 API GUID documentation (https://learn.microsoft.com/en-us/windows/win32/api/guiddef/ns-guiddef-guid), the correct GUID structure would have unsigned chars for the Data4 field.
I think since ctypes.wintypes was updated to have BYTE = ctypes.c_ubyte here: python/cpython#97579, importing BYTE from ctypes.wintypes should be better aligned with the GUID structure than defining BYTE = c_byte in GUID.py.
There was a problem hiding this comment.
Yes, c_ubyte is the correct type.
My point is more that wintypes.DWORD = c_ubyte only since python 3.12.
So, for python 3.8 to 3.11, wintypes.DWORD = c_byte.
This may cause an unexpected behaviour because it is not the same value for all the supported python version
There was a problem hiding this comment.
Is wintypes.DWORD a typo for wintypes.BYTE?
(Both DWORD in 3.11 and DWORD in 3.12 are aliases for c_ulong, and neither c_byte nor c_ubyte.)
Indeed, I overlooked python/cpython#60580. However, I believe this change is not breaking but rather a bug fix.
While it may have been possible to assign signed values to the Data4 field in the past, this was limited to the scope of ctypes structures.
Such values should not have been allowed as part of a COM component.
Tests for GUID are already included in the CI pipeline, so I don't believe there is much to fear here.
If we later discover that this change has unintended consequences after the release, we plan to revert the replacement of BYTE and release a new version.
There was a problem hiding this comment.
Is
wintypes.DWORDa typo forwintypes.BYTE?
Yes, sorry about that.
To be sure that BYTE is c_ubyte on any python version comtypes support, shouldn't we do this?
if sys.version_info >= (3, 12):
from ctypes.wintypes import BYTE
else:
from ctypes import c_ubyte
BYTE = c_ubyteThere was a problem hiding this comment.
I think that’s a great idea.
Feel free to open the PR!
Let’s also check if the tests in the CI pipeline pass there.
There was a problem hiding this comment.
I opened the issue about this: #696 (comment)
Removes alias definitions and replaces them with imports from ctypes.wintypes where possible for #662