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

can't set big int-like objects to items in array 'Q', 'L' and 'I' #72485

Closed
orenmn mannequin opened this issue Sep 28, 2016 · 13 comments
Closed

can't set big int-like objects to items in array 'Q', 'L' and 'I' #72485

orenmn mannequin opened this issue Sep 28, 2016 · 13 comments
Assignees
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@orenmn
Copy link
Mannequin

orenmn mannequin commented Sep 28, 2016

BPO 28298
Nosy @mdickinson, @meadori, @serhiy-storchaka, @orenmn
PRs
  • bpo-28298: make array 'Q', 'L' and 'I' accept big intables as elements #570
  • [3.6] bpo-28298: make array 'Q', 'L' and 'I' accept big intables as elements #579
  • [3.5] bpo-28298: make array 'Q', 'L' and 'I' accept big intables as elements #588
  • [Do Not Merge] Sample of CPython life with blurb. #703
  • Files
  • patchedCPythonTestOutput_ver1.txt: test output of CPython with my patches (tested on my PC) - ver1
  • CPythonTestOutput.txt: test output of CPython without my patches (tested on my PC)
  • issue28298_ver1.diff: proposed patches diff file - ver1
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2017-03-10.06:14:23.292>
    created_at = <Date 2016-09-28.10:09:35.874>
    labels = ['3.7', 'type-bug', 'library']
    title = "can't set big int-like objects to items in array 'Q', 'L' and 'I'"
    updated_at = <Date 2017-03-24.22:38:54.246>
    user = 'https://github.com/orenmn'

    bugs.python.org fields:

    activity = <Date 2017-03-24.22:38:54.246>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2017-03-10.06:14:23.292>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2016-09-28.10:09:35.874>
    creator = 'Oren Milman'
    dependencies = []
    files = ['44860', '44861', '44862']
    hgrepos = []
    issue_num = 28298
    keywords = ['patch']
    message_count = 13.0
    messages = ['277604', '277685', '277704', '277707', '277729', '288866', '289218', '289234', '289336', '289341', '290243', '290251', '290252']
    nosy_count = 4.0
    nosy_names = ['mark.dickinson', 'meador.inge', 'serhiy.storchaka', 'Oren Milman']
    pr_nums = ['570', '579', '588', '703']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue28298'
    versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 28, 2016

    ------------ current state ------------
    On my Windows 10, on a 32-bit Python, The following runs fine:
    import array
    array.array('L').append(2 ** 32 - 1)

    However, in the following, an OverflowError('Python int too large to convert to C long') is raised on the last line:
    import array
    class LikeInt:
    def __init__(self, intVal):
    self.intVal = intVal
    def __int__(self):
    return self.intVal
    array.array('L').append(LikeInt(2 ** 32 - 1))

    The reason for this behavior is the implementation of the function LL_setitem (in Modules/arraymodule.c) (edited brutally for brevity):
        LL_setitem(arrayobject *ap, Py_ssize_t i, PyObject *v)
        {
            unsigned long x;
            if (PyLong_Check(v)) {
                x = PyLong_AsUnsignedLong(v);
            }
            else {
                long y;
                PyArg_Parse(v, "l;array item must be integer", &y);
                x = (unsigned long)y;
            }
            (ap->ob_item)[i] = x;
        }
    The problem is that PyArg_Parse is used to convert a Python int into a C long. So PyArg_Parse fails when it is given a Python int which is in range(LONG_MAX + 1, ULONG_MAX + 1), even though such Python int can be stored in a C unsigned long.

    It is quite the same for array('I') and array('Q') (i.e. in II_setitem and in QQ_setitem, respectively).

    With regard to relevant changes made in the past, PyArg_Parse was always used (in case '!PyLong_Check(v)'), since adding the original versions of:
    * II_setitem and LL_setitem (back then, they were called I_setitem and L_setitem, respectively), in changeset 4875 (https://hg.python.org/cpython/rev/911040e1bb11)
    * QQ_setitem, in changeset 72430 (https://hg.python.org/cpython/rev/15659e0e2b2e)

    ------------ proposed changes ------------

        1. In Modules/arraymodule.c, change the implementation of LL_setitem (and likewise, of II_setitem and QQ_setitem) roughly to the following:
            LL_setitem(arrayobject *ap, Py_ssize_t i, PyObject *v)
            {
                unsigned long x;
                if (!PyLong_Check(v)) {
                    v = _PyLong_FromNbInt(v);
                }
                x = PyLong_AsUnsignedLong(v);
                (ap->ob_item)[i] = x;
            }
    2. In [Lib/test/test_array.py](https://github.com/python/cpython/blob/main/Lib/test/test_array.py), add tests:
        * to verify the bug is fixed, i.e. test the bounds of setting int-like objects to items in an integers array
        * to verify float objects can't be set to items in an integers array (as is already the behavior, but there aren't any tests for it)
    
    3. While we are in [Lib/test/test_array.py](https://github.com/python/cpython/blob/main/Lib/test/test_array.py), remove any checks whether 'long long' is available, as it must be available, since changeset 103105 (https://hg.python.org/cpython/rev/9206a86f7321).
    

    Note that issue bpo-12974 (opened in 2011) proposes deprecating the ability to set int-like objects to items in an integers array. (I am not sure how that affects my patch, but I guess it should be noted.)

    ------------ diff ------------
    The proposed patches diff file is attached.

    (Note that the I didn't propose to change the perplexing error message 'unsigned int is greater than maximum' in II_setitem, as there are many such messages in the codebase, and I am working on a patch for them as part of issue bpo-15988.)

    ------------ tests ------------
    I ran 'python_d.exe -m test -j3' (on my 64-bit Windows 10) with and without the patches, and got quite the same output. (That also means my new tests in test_array passed.)
    The outputs of both runs are attached.

    @orenmn orenmn mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error 3.7 (EOL) end of life labels Sep 28, 2016
    @mdickinson
    Copy link
    Member

    One comment here: it's not the presence of __int__ that makes a type integer-like; it's the presence of __index__. (Decimal and float both supply __int__, but shouldn't be regarded as integer-like, for example.) I'm guessing that we're going to have the same issues with __index__ in place of __int__, though.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 29, 2016

    You are right about the terminology of course. My bad.

    Anyway, the bug is not related to __index__ (or nb_index), because the three aforementioned functions are using (in case '!PyLong_Check(v)') PyArg_Parse with the formats 'l' or 'L'.
    Ultimately, convertsimple (in Python/getargs.c) would call PyLong_AsLong or PyLong_AsLongLong (respectively).
    These two (in case '!PyLong_Check(v)') only try to use nb_int, as documented:
    * https://docs.python.org/3.7/c-api/long.html?highlight=pylong_aslong#c.PyLong_AsLong
    * https://docs.python.org/3.7/c-api/long.html?highlight=pylong_aslong#c.PyLong_AsLongLong

    But just in case, I ran the following:
    import array
    class LikeInt:
    def __init__(self, intVal):
    self.intVal = intVal
    def __index__(self):
    return self.intVal
    array.array('L').append(LikeInt(0))
    and got a 'TypeError: an integer is required (got type LikeInt)'

    @mdickinson
    Copy link
    Member

    and got a 'TypeError: an integer is required (got type LikeInt)'

    Right, see bpo-12974 for more on arrays, __int__ and __index__.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 29, 2016

    Ah, I should have read more about __int__ and __index__ before writing my last reply.
    So IIUC, this is a somewhat painful issue, which could be resolved by:
    * bpo-20092
    * replacing _PyLong_FromNbInt with PyNumber_Index in:
    - Objects/longobject.c (as you suggested in https://bugs.python.org/issue12965#msg146252)
    - Modules/zlibmodule.c in ssize_t_converter
    I didn't find any issue for the second one. If there is really no such issue, why is that? (I am sorry if I am the thousandth one to ask)
    The first paragraph of your comment in https://bugs.python.org/issue21111#msg215660 suggests it might break a lot of code in the wild, but shouldn't we start with raising a deprecation warning?

    With regard to my patch - should I make the following changes?
    1. in Modules/arraymodule.c - add a call to PyNumber_Index before the call to _PyLong_FromNbInt (and move them into a helper function, as done in Modules/_struct.c)
    2. in Lib/test/test_array.py:
    * replace the name 'LikeInt' with 'IntCompatible' (I am not sure this is an appropriate name either. If you have a better name in mind, please share :) )
    * add tests for LikeInt (this time, a class with a __index__ method)
    3. In Doc/library/array.rst:
    * add a note saying that setting int-like objects (i.e. objects with __index__) to items in an integers array is possible
    * while we are here, remove the note about the availability of 'q' and 'Q' only in certain platforms

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 3, 2017

    ping

    @serhiy-storchaka serhiy-storchaka self-assigned this Mar 3, 2017
    @serhiy-storchaka
    Copy link
    Member

    LGTM. But maybe combine PyFloat_Check+_PyLong_FromNbInt in one helper function?

    Could you please create a PR Oren?

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 8, 2017

    yes and yes.
    I would start with a PR for 3.7.

    and thanks for the review :)

    @serhiy-storchaka
    Copy link
    Member

    Thank you for your contribution Oren.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 10, 2017

    Thanks for the reviews :)

    @serhiy-storchaka
    Copy link
    Member

    New changeset e2c88bd by Serhiy Storchaka (orenmn) in branch '3.5':
    bpo-28298: make array 'Q', 'L' and 'I' accept big intables as elements
    e2c88bd

    @serhiy-storchaka
    Copy link
    Member

    New changeset 26d013e by Serhiy Storchaka (orenmn) in branch '3.6':
    [3.6] bpo-28298: make array 'Q', 'L' and 'I' accept big intables as elements (#579)
    26d013e

    @serhiy-storchaka
    Copy link
    Member

    New changeset 964281a by Serhiy Storchaka (orenmn) in branch 'master':
    bpo-28298: make array 'Q', 'L' and 'I' accept big intables as elements (#570)
    964281a

    @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.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants