Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default arguments for int.to_bytes() #89318

Closed
warsaw opened this issue Sep 9, 2021 · 41 comments
Closed

Add default arguments for int.to_bytes() #89318

warsaw opened this issue Sep 9, 2021 · 41 comments
Labels
3.11 bug and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@warsaw
Copy link
Member

warsaw commented Sep 9, 2021

BPO 45155
Nosy @warsaw, @rhettinger, @mdickinson, @ncoghlan, @vstinner, @encukou, @ethanfurman, @serhiy-storchaka, @vedgar, @brandtbucher
PRs
  • bpo-45155 : Default arguments for int.to_bytes(length=1, byteorder=sys.byteorder) #28265
  • bpo-45155: Apply new byteorder default values for int.to/from_bytes #28465
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-09-16.02:56:07.010>
    created_at = <Date 2021-09-09.22:00:31.039>
    labels = ['interpreter-core', '3.11']
    title = 'Add default arguments for int.to_bytes()'
    updated_at = <Date 2021-09-20.18:23:05.746>
    user = 'https://github.com/warsaw'

    bugs.python.org fields:

    activity = <Date 2021-09-20.18:23:05.746>
    actor = 'rhettinger'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-09-16.02:56:07.010>
    closer = 'barry'
    components = ['Interpreter Core']
    creation = <Date 2021-09-09.22:00:31.039>
    creator = 'barry'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45155
    keywords = ['patch']
    message_count = 41.0
    messages = ['401524', '401538', '401540', '401541', '401542', '401543', '401544', '401545', '401552', '401555', '401556', '401560', '401570', '401597', '401598', '401599', '401622', '401652', '401653', '401657', '401661', '401664', '401693', '401709', '401713', '401720', '401728', '401729', '401730', '401737', '401746', '401749', '401750', '401751', '401752', '401753', '401809', '401914', '401924', '401947', '402265']
    nosy_count = 12.0
    nosy_names = ['barry', 'rhettinger', 'mark.dickinson', 'ncoghlan', 'vstinner', 'mrabarnett', 'petr.viktorin', 'ethan.furman', 'serhiy.storchaka', 'veky', 'brandtbucher', '4-launchpad-kalvdans-no-ip-org']
    pr_nums = ['28265', '28465']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue45155'
    versions = ['Python 3.11']

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 9, 2021

    In the PEP-467 discussion, I proposed being able to use

    >>> (65).to_bytes()
    b'A'

    IOW, adding default arguments for the length and byteorder arguments to int.to_bytes()

    https://mail.python.org/archives/list/python-dev@python.org/message/PUR7UCOITMMH6TZVVJA5LKRCBYS4RBMR/

    It occurs to me that this is (1) useful on its own merits; (2) easy to do. So I've done it. Creating this bug so I can link a PR against it.

    @warsaw warsaw added 3.11 bug and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Sep 9, 2021
    @ncoghlan
    Copy link
    Contributor

    Rather than defaulting to sys.byteorder, could the byte order default to None and only be optional when not needed? (input value fits in a single byte, output is a single byte)

    Otherwise the difference in defaults between this method and the struct module (network byte order rather than host byte order) could be very confusing.

    @ncoghlan
    Copy link
    Contributor

    Never mind, I've forced network byte order in struct strings for so long I had forgotten that native byte order was also the default there. Hence I withdraw that objection.

    @serhiy-storchaka
    Copy link
    Member

    So (128).to_bytes() will raise an error, right?

    I afraid also that it will lead to some programs working correctly only on platforms with the most common byte order, just because authors are not aware of byte ordering. Currently the interface forces programmers to read something about byte ordering.

    @serhiy-storchaka
    Copy link
    Member

    Ah, signed=False by default, so (128).to_bytes() will work. But I still worry that it can provoke writing more errorprone code.

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 10, 2021

    Ah, signed=False by default, so (128).to_bytes() will work. But I still worry that it can provoke writing more errorprone code.

    Can you elaborate on that? Obviously no existing code will change behavior. I really don’t expect people to write (128).to_bytes(signed=True) by accident.

    @rhettinger
    Copy link
    Contributor

    Perhaps instead of the system byte ordering, choose one for the default so that default encoding/decoding will work cross platform. I think "little" is the most common (intel and arm).

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 10, 2021

    For the common case where you’re using all defaults, it won’t matter. byteorder doesn’t matter when length=1.

    On Sep 9, 2021, at 18:12, Raymond Hettinger <report@bugs.python.org> wrote:

    Raymond Hettinger <raymond.hettinger@gmail.com> added the comment:

    Perhaps instead of the system byte ordering, choose one for the default so that default encoding/decoding will work cross platform. I think "little" is the most common (intel and arm).

    @rhettinger
    Copy link
    Contributor

    Serhiy is likely thinking of other the other cases. Prior to this discussion, the principal use for to_bytes and from_bytes was for integers larger than a single byte. If we're going to add a *byteorder* default, it needs to make sense for those cases as well.

    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Sep 10, 2021

    choose one for the default so that default encoding/decoding will work cross platform. I think "little" is the most common (intel and arm).

    Raymond, please don't do this. We already have a "sensible default" in a network context, and it is big endian. Having another "sensible default" opposite to the previous one is really no way to ensure interoperability. (https://xkcd.com/927/ only becomes more ridiculous when the number in question is 2.:) I don't want to think about whether the way machines A and B exchange data can be called "a network" or not.

    Of course, having the byteorder optional when there's only one (unsigned) byte is good.

    @mdickinson
    Copy link
    Member

    I'd also really like to avoid a system-dependent default. The danger is that code of the form

        some_externally_supplied_integer.to_bytes(length=4)

    can be written and thoroughly tested, only to fail unexpectedly some time later when that code happens to meet a big-endian machine. In most real-world cases with input length >= 1, it's unlikely that the system byteorder is the right thing, especially for from_bytes: what you need to know is what endianness the integer was encoded with, and that's not likely to be well correlated with the endianness that the machine you're running on right now happens to be using. (The choice of default obviously doesn't matter in cases where you're encoding and decoding on the same system, but there are going to be plenty of cases where that's not true.)

    This is essentially the same issue that PEP-597 starts to address with open(filename) with no encoding specified. That system-specific default encoding has caused us real issues in production code.

    @encukou
    Copy link
    Member

    encukou commented Sep 10, 2021

    Exactly, a platform-dependent default is a bad idea. A default allows using the function without the code author & reviewer even being *aware* that there is a choice, and that is dangerous.

    @vstinner
    Copy link
    Member

    I dislike the idea of adding a default length to int.to_bytes(). The length changes the meaning of the output:

    >>> (1).to_bytes(2, 'big')
    b'\x00\x01'
    >>> (1).to_bytes(1, 'big')
    b'\x01'

    If the intent is to "magically cast an integer to a byte strings", having a fixed length of 1 doesn't help:

    >>> (1000).to_bytes(1, "big")
    OverflowError: int too big to convert

    If the intent is to create a bytes string of length 1, I'm not sure that "re-using" this existing API for that is a good idea.

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 10, 2021

    Just to point out, struct module also uses “native” (i.e. system) byte order by default. Any choice other than that for to_bytes() seems both arbitrary and inconsistent.

    On Sep 10, 2021, at 00:48, Petr Viktorin <report@bugs.python.org> wrote:

    Exactly, a platform-dependent default is a bad idea. A default allows using the function without the code author & reviewer even being *aware* that there is a choice, and that is dangerous.

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 10, 2021

    On Sep 10, 2021, at 04:06, STINNER Victor <report@bugs.python.org> wrote:

    If the intent is to create a bytes string of length 1, I'm not sure that "re-using" this existing API for that is a good idea.

    Why not? It seems an obvious and simple convenience.

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 10, 2021

    Petr Viktorin <encukou@gmail.com> added the comment:

    Exactly, a platform-dependent default is a bad idea. A default allows using the function without the code author & reviewer even being *aware* that there is a choice, and that is dangerous.

    I’m not convinced. I’m more concerned with the obscurity of the API. If I saw its use in some code I was reviewing, I’d look it up, and then I’d know exactly what it was doing.

    @rhettinger
    Copy link
    Contributor

    [Mark Dickinson]

    I'd also really like to avoid a system-dependent default.

    [Petr Viktorin]

    Exactly, a platform-dependent default is a bad idea.

    I concur with Petr and Mark.

    The principal use case for int.to_bytes is to convert an integer of arbitrary size to a binary format for storage or transmission. The corresponding file load or received data needs to symmetrically restore the int. The default (whether little or big) needs to be the same on both sides to prevent bugs. Otherwise, for portable code, we would have to recommend that people not use the default because the output isn't deterministic across systems.

    By way of comparison, we've had long standing issues like this in other parts of the language. For example, this gives inconsistent encodings across systems:

        with open(filename) as f:
            f.write(text)

    Not long ago, Inada had to sweep through and add encoding="utf-8" to fix all the bugs caused by the default platform dependent encoding. Arguably, most code that has ever been written without an explicit encoding is wrong if the files were intended to be shared outside the local file system.

    So if Veky wants the default to be "big", that's fine by me. The important thing is that a *consistent* default be used (not platform dependent). I won't argue for a "sensible default" because apparently Veky has different sensibilities.

    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Sep 12, 2021

    My sensibilities are irrelevant here. I'm just saying we already have a standard byte order for data in transit, and it was introduced long before this thing called internet (it was with capital I back then:) started to interest me.

    @serhiy-storchaka
    Copy link
    Member

    The struct module has 4 different modes. By default it uses not only native byte order, but native sizes and alignments which depend on OS and compiler. You need to know all these details just to understand the format codes. I think that the struct module user is more prepared to work with different word sizes and therefore with different byte ordering.

    @serhiy-storchaka
    Copy link
    Member

    In the stdlib, there is only one use of to_bytes() with sys.byteorder (2 in tests), 16 uses of to_bytes()/from_bytes() with 'little' (22 in tests) and 22 uses with 'big' (33 in tests). So making sys.byteorder the default will help almost nobody, and the advantage of 'big' over 'litte' is so small, that making any of them the default may only help in a half of cases and confuse in the other half.

    @rhettinger
    Copy link
    Contributor

    Interestingly, "little" is faster than "big".

    $ python3.10 -m timeit -r11 -s 'x=3452452454524' 'x.to_bytes(10, "little")'
    5000000 loops, best of 11: 82.7 nsec per loop
    $ python3.10 -m timeit -r11 -s 'x=3452452454524' 'x.to_bytes(10, "big")'
    5000000 loops, best of 11: 90.6 nsec per loop

    @serhiy-storchaka
    Copy link
    Member

    Perhaps it is because "little" is checked first. One call of _PyUnicode_EqualToASCIIId() for "little" and two for "big".

    @encukou
    Copy link
    Member

    encukou commented Sep 13, 2021

    I’m not convinced. I’m more concerned with the obscurity of the API. If I saw its use in some code I was reviewing, I’d look it up, and then I’d know exactly what it was doing.

    I know you would. But there are many others who just try things until they work.

    Also, if this does become *the* way to create bytes, it won't be obscure any more -- but you'd still need to remember to always specify byteorder for length > 1. That is, unless you *want* platform-specific behavior, which I don't think is all that often. Even in this case, you want to think about the issue, and omitting the argument is a bad way to encode that you thought about it.

    ---

    Hm, what happened to the idea of only requiring byteorder for length > 1? I recall it being discussed

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 13, 2021

    That’s okay, Brandt’s improved sys.byteorder is fastest <wink>.

    % ./python.exe -m timeit -r11 -s 'x=3452452454524' 'x.to_bytes(10, "little")'
    2000000 loops, best of 11: 94.6 nsec per loop
    % ./python.exe -m timeit -r11 -s 'x=3452452454524' 'x.to_bytes(10, "big")'
    2000000 loops, best of 11: 97.8 nsec per loop
    % ./python.exe -m timeit -r11 -s 'x=3452452454524' 'x.to_bytes(10)'
    5000000 loops, best of 11: 79.1 nsec per loop

    On Sep 12, 2021, at 04:20, Raymond Hettinger <report@bugs.python.org> wrote:

    Raymond Hettinger <raymond.hettinger@gmail.com> added the comment:

    Interestingly, "little" is faster than "big".

    $ python3.10 -m timeit -r11 -s 'x=3452452454524' 'x.to_bytes(10, "little")'
    5000000 loops, best of 11: 82.7 nsec per loop
    $ python3.10 -m timeit -r11 -s 'x=3452452454524' 'x.to_bytes(10, "big")'
    5000000 loops, best of 11: 90.6 nsec per loop

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue45155\>


    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 13, 2021

    @mrabarnett
    Copy link
    Mannequin

    mrabarnett mannequin commented Sep 13, 2021

    I'd probably say "In the face of ambiguity, refuse the temptation to guess".

    As there's disagreement about the 'correct' default, make it None and require either "big" or "little" if length > 1 (the default).

    @vstinner
    Copy link
    Member

    >>> (65).to_bytes()
    b'A'

    It seems like your proposal is mostly guided by: convert an int to a byte (bytes string of length 1). IMO this case is special enough to justify the usage of a different function.

    What if people expect int.to_bytes() always return a single byte, but then get two bytes by mistake?

    ch = 256
    byte = ch.to_bytes()
    assert len(byte) == 2  # oops

    A function dedicated to create a single byte is expected to raise a ValueError for values outside the range [0; 255]. Like:

    >>> struct.pack('B', 255)
    b'\xff'
    >>> struct.pack('B', 256)
    struct.error: ubyte format requires 0 <= number <= 255

    @vstinner
    Copy link
    Member

    In the PEP-467 discussion, I proposed (...)

    Since this PEP is controversial, and this issue seems to be controversial as well, maybe this idea should be part of the PEP.

    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Sep 13, 2021

    The poll is invalid, since the option that most people want is deliberately not offered.

    @rhettinger
    Copy link
    Contributor

    Just reread the thread. AFAICT not a single use case was presented for having system byte ordering as the default. However, multiple respondents have pointed out that a default to system byte ordering is a bug waiting to happen, almost ensuring that some users will encounter unexpected behaviors when crossing platforms. We've seen issues like that before and should avoid them.

    We don't really need a poll. What is needed is for the system byte ordering proponents to present valid reasons why it would useful and to address the concerns that it is actually harmful.

    If the proposal goes through despite the concerns, we should ask folks writing lint tools to flag every use of the default as a potential bug and advise people to never use the default unless they know for sure that it is encoding only a single byte. Personally, I would never let system byte ordering pass a code review.

    @mrabarnett
    Copy link
    Mannequin

    mrabarnett mannequin commented Sep 14, 2021

    I wonder whether there should be a couple of other endianness values, namely, "native" and "network", for those cases where you want to be explicit about it. If you use "big" it's not clear whether that's because you want network endianness or because the platform is big-endian.

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 14, 2021

    I'd probably say "In the face of ambiguity, refuse the temptation to guess".

    As there's disagreement about the 'correct' default, make it None and require either "big" or "little" if length > 1 (the default).

    Jelle suggested that over in Discourse, and I’m not opposed, but it does mean that there’s no natural default for byteorder in int.from_bytes().

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 14, 2021

    On Sep 13, 2021, at 13:38, STINNER Victor <report@bugs.python.org> wrote:

    It seems like your proposal is mostly guided by: convert an int to a byte (bytes string of length 1). IMO this case is special enough to justify the usage of a different function.

    Like bchr() ? <wink>

    What if people expect int.to_bytes() always return a single byte, but then get two bytes by mistake?

    ch = 256
    byte = ch.to_bytes()

    The OverflowError you’ll get seems reasonable.

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 14, 2021

    On Sep 13, 2021, at 13:39, Vedran Čačić <report@bugs.python.org> wrote:

    The poll is invalid, since the option that most people want is deliberately not offered.

    *Is* there an option that most people want?

    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Sep 14, 2021

    I'd say yes. Of course, one way to ascertain that would be to conduct a valid pool. ;-)

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 14, 2021

    On Sep 13, 2021, at 22:12, Vedran Čačić <report@bugs.python.org> wrote:

    Vedran Čačić <vedgar@gmail.com> added the comment:

    I'd say yes. Of course, one way to ascertain that would be to conduct a valid pool. ;-)

    People can always comment otherwise in the Discourse thread.

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 15, 2021

    "big" by default

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 16, 2021

    New changeset 07e737d by Barry Warsaw in branch 'main':
    bpo-45155 : Default arguments for int.to_bytes(length=1, byteorder=sys.byteorder) (bpo-28265)
    07e737d

    @warsaw warsaw closed this as completed Sep 16, 2021
    @vstinner
    Copy link
    Member

    bpo-45155 : Default arguments for int.to_bytes(length=1, byteorder=sys.byteorder) (bpo-28265)

    The commit title is wrong, the default "big" not sys.byteorder:

        int.to_bytes(length=1, byteorder='big', *, signed=False)
        int.from_bytes(bytes, byteorder='big', *, signed=False)

    @warsaw
    Copy link
    Member Author

    warsaw commented Sep 16, 2021

    On Sep 16, 2021, at 00:36, STINNER Victor <report@bugs.python.org> wrote:

    The commit title is wrong, the default "big" not sys.byteorder:

    int.to_bytes(length=1, byteorder='big', *, signed=False)
    int.from_bytes(bytes, byteorder='big', *, signed=False)

    Oops

    @rhettinger
    Copy link
    Contributor

    New changeset 9510e6f by Raymond Hettinger in branch 'main':
    bpo-45155: Apply new byteorder default values for int.to/from_bytes (GH-28465)
    9510e6f

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 bug and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants