gh-60580: Fix a wrong type of ctypes.wintypes.BYTE

[arhadthedev]

This PR is created from a .diff file attached to the issue, by Anatoly Techtonik (@techtonik on GitHub).

• Issue: wrong type for wintypes.BYTE #60580

[ghost]

All commit authors signed the Contributor License Agreement.

[arhadthedev]

Does a trivial change like this (addition of a single character) warrant the CLA?

[arhadthedev]

@techtonik Would you mind to sign the CLA to get your PR acepted please? See a message of a bot above for details.

[AlexWaygood]

Does a trivial change like this (addition of a single character) warrant the CLA?

Unfortunately it is impossible for a core dev to merge a PR without the CLA being signed, as the CLA check is a "required" status check.

[arhadthedev]

What would you recommend to do? The change is way too trivial to give a CLA-covered alternative. Also it seems impossible to call a user who never contributed to python/cpythonbefore.

[AlexWaygood]

What would you recommend to do?

Squash the PR so that GitHub no longer recognises the first commit as being authored by @techtonik (thus evading the CLA bot), but make sure to give them credit in the commit message and PR description. Since this is, as you say, a small, relatively trivial PR, I think that should be fine.

[arhadthedev]

@abalkin would you mind merging this PR as a ctypes expert?

[abalkin]

I am surprised that this changes does not require a change to any tests. This means that we don't have enough tests, but potentially this change can lead to some subtle change in behavior.

Can someone provide a use case where this change makes a difference?

[arhadthedev]

Can someone provide a use case where this change makes a difference?

The only case I've found inside CPython is struct that maps c_byte to b and c_ubyte to B:

cpython/Lib/ctypes/__init__.py

Lines 224 to 225 in dfad678

	class c_ubyte(_SimpleCData):
	_type_ = "B"

cpython/Lib/ctypes/__init__.py

Lines 231 to 232 in dfad678

	class c_byte(_SimpleCData):
	_type_ = "b"

cpython/Modules/_struct.c

Lines 808 to 811 in dfad678

	static const formatdef native_table[] = {
	{'x', sizeof(char), 0, NULL},
	{'b', sizeof(char), 0, nu_byte, np_byte},
	{'B', sizeof(char), 0, nu_ubyte, np_ubyte},

However, n*_byte() is the same as n*_cbyte().

[arhadthedev]

It seems that external shim generation tools that might rely on the layout string will stay compatible.

[abalkin]

Curiously, the same question was asked when this issue was first raised:

All cbytes tests passed after the change was made but I've no idea as to the impact of this.

A test was then suggested that should be sensitive to the change. We need to add a unit test before this change can be merged.

[abalkin]

We can just add a simple signedness test.

Before the patch:

>>> ctypes.wintypes.BYTE(200).value < 128
True

after the patch:

>>> ctypes.wintypes.BYTE(200).value < 128
False

Since ctypes.c_byte is defined as signed, I would not be surprised if there is code in the wild relying on the wrong signedness of the ctypes.wintypes.BYTE alias. We may need to make this change a little more visible possibly adding a note to the "porting to" section of the relevant "What’s New In Python" page.

[abalkin]

@Yhg1s - please advise if this bug fix is appropriate for 3.12 and what documentation it will require in "What’s New In Python"?

TL;DR: This PR changes ctypes.wintypes.BYTE alias from signed to unsigned char type to match the eponymous Windows API typedef.

[Yhg1s]

@Yhg1s - please advise if this bug fix is appropriate for 3.12 and what documentation it will require in "What’s New In Python"?

Yes, it's appropriate for 3.12. (We're not in beta phase yet, so any fix appropriate for any release is appropriate for 3.12.) I don't think it's worth mentioning in What's New, since it's just a minor bugfix.

[CristiFati]

Hmm, I wonder how anyone (including me - as I used wintypes.BYTE a lot in the past) missed this. I was just about to open a PR, as I'm working with [Ms.Learn]: SYSTEM_POWER_STATUS structure (winbase.h).

Regarding signed / unsigned types: [SO]: Maximum and minimum value of C types integers from Python (@CristiFati's answer).