Skip to content

Conversation

skirpichev
Copy link
Contributor

@skirpichev skirpichev commented Sep 10, 2025

>>> (0).to_bytes(0, 'big', signed=True)
b''
>>> (-1).to_bytes(0, 'big', signed=True)  # was b''
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    (-1).to_bytes(0, 'big', signed=True)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
OverflowError: int too big to convert

```pycon
>>> (0).to_bytes(0, 'big', signed=True)
b''
>>> (-1).to_bytes(0, 'big', signed=True)  # was b''
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    (-1).to_bytes(0, 'big', signed=True)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
OverflowError: int too big to convert
```
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vstinner

This comment was marked as resolved.

@vstinner
Copy link
Member

@serhiy-storchaka: I would suggest to backport this change to Python 3.13 and 3.14. What do you think?

@skirpichev skirpichev requested a review from vstinner September 10, 2025 11:55
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there are no enough tests for signed=True. Please add also tests for (128).to_bytes(0, 'big', signed=True) and (-129).to_bytes(0, 'big', signed=True).

@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Sep 10, 2025
@skirpichev
Copy link
Contributor Author

Disclaimer:
This pr implements "the minimal change necessary to remove ambiguity" (c) @mdickinson

I was thinking about (0).to_bytes(0, 'big', signed=True) too, but that means rejecting also int.from_bytes(b"", ..., signed=True). That case looks for me as a special case, that aren't special enough to warrant compatibility break.

@serhiy-storchaka
Copy link
Member

Or better add several tests for extreme values and values just above the limit for all combination of signedness and n=0, 1, 2.

@skirpichev
Copy link
Contributor Author

It seems there are no enough tests for signed=True. Please add also tests for (128).to_bytes(0, 'big', signed=True) and (-129).to_bytes(0, 'big', signed=True).

Done.

There are such tests, but rather for "automatic" value for length parameter (determined by result):

def test_to_bytes(self):

Or better add several tests for extreme values and values just above the limit for all combination of signedness and n=0, 1, 2.

It seems to be a big refactoring of tests. Maybe it does make sense as a separate pr?

Comment on lines +1380 to +1383
self.assertRaises(OverflowError, (128).to_bytes, 0, 'big', signed=True)
self.assertRaises(OverflowError, (128).to_bytes, 0, 'little', signed=True)
self.assertRaises(OverflowError, (-129).to_bytes, 0, 'big', signed=True)
self.assertRaises(OverflowError, (-129).to_bytes, 0, 'little', signed=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, use n=1 here.

@@ -1377,6 +1377,10 @@ def equivalent_python(n, length, byteorder, signed=False):
self.assertRaises(OverflowError, (256).to_bytes, 1, 'big', signed=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this test is redundant. We need to test for 128.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants