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

Improve struct.pack out of range error messages #89197

Closed
stevendaprano opened this issue Aug 28, 2021 · 8 comments
Closed

Improve struct.pack out of range error messages #89197

stevendaprano opened this issue Aug 28, 2021 · 8 comments
Labels
3.11 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@stevendaprano
Copy link
Member

BPO 45034
Nosy @terryjreedy, @mdickinson, @stevendaprano, @meadori, @corona10, @sobolevn
PRs
  • bpo-45034: Fixes how upper limit is formatted for struct.pack("H", ...) #28178
  • 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 = None
    created_at = <Date 2021-08-28.01:02:05.513>
    labels = ['type-feature', 'library', '3.11']
    title = 'Improve struct.pack out of range error messages'
    updated_at = <Date 2021-09-07.12:25:15.015>
    user = 'https://github.com/stevendaprano'

    bugs.python.org fields:

    activity = <Date 2021-09-07.12:25:15.015>
    actor = 'mark.dickinson'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2021-08-28.01:02:05.513>
    creator = 'steven.daprano'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45034
    keywords = ['patch']
    message_count = 6.0
    messages = ['400452', '401020', '401043', '401053', '401251', '401254']
    nosy_count = 6.0
    nosy_names = ['terry.reedy', 'mark.dickinson', 'steven.daprano', 'meador.inge', 'corona10', 'sobolevn']
    pr_nums = ['28178']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue45034'
    versions = ['Python 3.11']

    @stevendaprano
    Copy link
    Member Author

    Packing errors using struct in 3.9 seem to be unnecessarily obfuscated to me.

        >>> import struct
        >>> struct.pack('H', 70000)
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        struct.error: ushort format requires 0 <= number <= (0x7fff * 2 + 1)

    Why "0x7fff * 2 + 1"? Why not the more straightforward "0xffff" or 65536? (I have a slight preference for hex.)

    Compare that to:

        >>> struct.pack('I', 4300000000)
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        struct.error: 'I' format requires 0 <= number <= 4294967295

    which at least gives the actual value, but it would perhaps be a bit more useful in hex 0xffffffff.

    For the long-long format, the error message just gives up:

        >>> struct.pack('Q', 2**65)
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        struct.error: argument out of range

    Could be improved by:

    'Q' format requires 0 <= number <= 0xffff_ffff_ffff_ffff
    

    @stevendaprano stevendaprano added 3.11 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Aug 28, 2021
    @terryjreedy
    Copy link
    Member

    I agree, including use of hex, if possible, for unsigned (non-negative) values.

    Grepping 'format requires' returns
    F:\dev\3x\Modules\_struct.c: 365:             "'%c' format requires 0 <= number <= %zu",
    F:\dev\3x\Modules\_struct.c: 371:             "'%c' format requires %zd <= number <= %zd",
    F:\dev\3x\Modules\_struct.c: 550:                         "byte format requires -128 <= number <= 127");
    F:\dev\3x\Modules\_struct.c: 565:                         "ubyte format requires 0 <= number <= 255");
    F:\dev\3x\Modules\_struct.c: 577:                         "char format requires a bytes object of length 1");
    F:\dev\3x\Modules\_struct.c: 593:                         "short format requires " Py_STRINGIFY(SHRT_MIN)
    F:\dev\3x\Modules\_struct.c: 611:                         "ushort format requires 0 <= number <= "
                                                               Py_STRINGIFY(USHRT_MAX));

    I believe l365 is the source for the 2nd example. AFAIK, 'zu' is not valid for either C printf or Python % formating.
    Lines 611 and 612 are the source for the 1st example. From comments before line 365, there can be issues with lefts shifts, but '0xffff', '0xffff_ffff', and '0xffff_ffff_ffff_ffff' could be hard-coded strings that would cover all 'normal' systems.

    Grepping "argument out of range" returns
    F:\dev\3x\Modules\_struct.c: 168: "argument out of range");
    F:\dev\3x\Modules\_struct.c: 192: "argument out of range");
    F:\dev\3x\Modules\_struct.c: 215: "argument out of range");
    F:\dev\3x\Modules\_struct.c: 238: "argument out of range");
    F:\dev\3x\Modules\_struct.c: 261: "argument out of range");
    F:\dev\3x\Modules\_struct.c: 284: "argument out of range");

    It is nnclear to me without more reading why some codes lead to this less helpful message.

    @sobolevn
    Copy link
    Member

    sobolevn commented Sep 4, 2021

    I would like to work on this if nobody else has already started :)

    @terryjreedy
    Copy link
    Member

    Go ahead, with the understanding that there is no guaranteed acceptance of the change, even with a good patch. There may be reasons for the status quo that neither Steven nor I are aware of. I don't think that either of the listed experts, added as nosy, are very active. In any case, revised or new tests will be needed.

    @mdickinson
    Copy link
    Member

    New changeset 8ca6b61 by Nikita Sobolev in branch 'main':
    bpo-45034: Fix how upper limit is formatted for struct.pack("H", ...) (GH-28178)
    8ca6b61

    @mdickinson
    Copy link
    Member

    The out-of-range error messages for unsigned short and short have been fixed, thanks to Nikita Sobolev. They resulted from a rather odd use of the Py_STRINGIFY macro, which meant that not only were the messages obscure, but they differed from system to system: e.g., on my machine I get: "struct.error: ushort format requires 0 <= number <= (32767 *2 +1)", and "struct.error: short format requires (-32767 -1) <= number <= 32767", complete with the weird spacing.

    There's still room for normalising the other messages and/or converting the limits to hex if anyone wants to provide a PR.

    I'm unsure about using hex universally: while 0xffffffffffffffff (or even better, 0xffff_ffff_ffff_ffff) is definitely more useful than 18446744073709551615, I think I'd still find -128 more immediately readable than -0x80.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @furkanonder
    Copy link
    Sponsor Contributor

    @mdickinson @sobolevn The issue seems to be solved. We can close the issue.

    @mdickinson
    Copy link
    Member

    @furkanonder Thanks for the ping. Indeed I think the changes in #98252 mean this issue can be closed.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants