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

Avoid error from repr() of recursive dictview #62733

Closed
bennorth mannequin opened this issue Jul 22, 2013 · 23 comments
Closed

Avoid error from repr() of recursive dictview #62733

bennorth mannequin opened this issue Jul 22, 2013 · 23 comments
Assignees
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@bennorth
Copy link
Mannequin

bennorth mannequin commented Jul 22, 2013

BPO 18533
Nosy @rhettinger, @terryjreedy, @orsenthil, @vstinner, @bennorth, @bitdancer, @serhiy-storchaka, @csabella, @miss-islington
PRs
  • bpo-18533: Avoid RuntimeError from repr() of recursive dictview #4823
  • [3.6] bpo-18533: Avoid RecursionError from repr() of recursive dictview (GH-4823) #5348
  • [2.7] bpo-18533: Avoid RuntimeError from repr() of recursive dictview [GH-4823] #5357
  • Files
  • non-error-recursive-dictview.patch: Patch avoiding RuntimeError for recursive dictview; give "..." instead
  • non-error-recursive-dictview-3.3.patch: Patch against 3.3 to avoid RuntimeError for recursive dictview; give "..." instead
  • non-error-recursive-dictview-3.3-1.patch
  • against-6e1dd1ce95b8-2.7-1.patch: Patch against 2.7 branch
  • against-9bf89c909bd4-3.3-1.patch: Patch against 3.3 branch
  • 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/orsenthil'
    closed_at = <Date 2018-06-03.10:01:31.294>
    created_at = <Date 2013-07-22.22:22:30.340>
    labels = ['interpreter-core', 'type-bug', '3.7']
    title = 'Avoid error from repr() of recursive dictview'
    updated_at = <Date 2018-06-03.10:01:31.294>
    user = 'https://github.com/bennorth'

    bugs.python.org fields:

    activity = <Date 2018-06-03.10:01:31.294>
    actor = 'serhiy.storchaka'
    assignee = 'orsenthil'
    closed = True
    closed_date = <Date 2018-06-03.10:01:31.294>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2013-07-22.22:22:30.340>
    creator = 'bennorth'
    dependencies = []
    files = ['31019', '31022', '31025', '31088', '31089']
    hgrepos = []
    issue_num = 18533
    keywords = ['patch']
    message_count = 23.0
    messages = ['193570', '193571', '193573', '193620', '193624', '193626', '193651', '193779', '193944', '195260', '195262', '308086', '308107', '308164', '310642', '310644', '310645', '310646', '310770', '310771', '310785', '310851', '312928']
    nosy_count = 10.0
    nosy_names = ['rhettinger', 'terry.reedy', 'orsenthil', 'vstinner', 'bennorth', 'r.david.murray', 'zuo', 'serhiy.storchaka', 'cheryl.sabella', 'miss-islington']
    pr_nums = ['4823', '5348', '5357']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue18533'
    versions = ['Python 2.7', 'Python 3.6', 'Python 3.7']

    @bennorth
    Copy link
    Mannequin Author

    bennorth mannequin commented Jul 22, 2013

    bpo-18019 noted the following crash in earlier 2.7:

    >>> d={}
    >>> d[42]=d.viewvalues()
    >>> d
    <segmentation fault>

    This issue has been fixed; the behaviour now is that a RuntimeError is
    produced for a recursive dictionary view:

    >>> d={}
    >>> d[42]=d.viewvalues()
    >>> d # (output line-broken:)
    {42: Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    RuntimeError: maximum recursion depth exceeded
      while getting the repr of a list

    Before finding this, though, I'd investigated and made a patch which
    produces a similar "..." output to a recursive dictionary. Reworking
    against current 2.7, the behaviour would be:

    >>> x={}
    >>> x[42]=x
    >>> x # existing behaviour for dictionaries:
    {42: {...}}
    
    >>> d={}
    >>> d[42]=d.viewvalues()
    >>> d # new behaviour:
    {42: dict_values([...])}
    >>> d[43]=d.viewitems()
    >>> d # (output line-broken:)
    {42: dict_values([..., dict_items([(42, ...), (43, ...)])]),
     43: dict_items([(42, dict_values([..., ...])), (43, ...)])}

    Attached is the patch, against current 2.7 branch. If there is interest
    in applying this, I will create a proper patch (changelog entry, fix to
    Lib/test/test_dictviews.py, etc.).

    On python-dev, Gregory Smith noted:

    Given that the RuntimeError fix has been released, your proposed
    ... behavior is arguably a new feature so I'd only expect this to
    make sense for consideration in 3.4, not 2.7.  (if accepted at all)
    
    [http://mail.python.org/pipermail/python-dev/2013-July/127489.html]
    

    so I have marked for consideration for versions 2.7 and 3.4.

    @bennorth bennorth mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Jul 22, 2013
    @terryjreedy
    Copy link
    Member

    Fixing 3.3 is more likely that 3.4. I can view RuntimeError as a bug less obnoxious than crash, but others may differ. Ultimately, the release managers can decide. This is definitely appropriate for 3.4, so please add tests. If the patch is for 2.7, please do one for 3.3 or 3.4.

    For 3.x, d.keys() is not an issue, but d.values() is.

    >>> d = {}
    >>> d[1] = d.keys()
    >>> d
    {1: dict_keys([1])}
    >>> d[2] = d.values()
    >>> d
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    RuntimeError: maximum recursion depth exceeded while getting the repr of a list

    @zuo
    Copy link
    Mannequin

    zuo mannequin commented Jul 22, 2013

    As I wrote on the list -- IMHO it's still a bug (even though not so painful as segfault) that should also be fixed in 2.7 and 3.2/3.3. In other cases (such as d={}; d[42]=d; repr(d)) Python does its best to avoid an error -- why in this case (d={}; d[42]=d.<Py2.x:view>values(); repr(d)) should it raise an exception? IMHO it's an obvious oversight in implementation, not a feature that anybody would expect.

    @bennorth
    Copy link
    Mannequin Author

    bennorth mannequin commented Jul 23, 2013

    New patch including tests attached, against 3.3. Terry Reedy's example
    above now gives

        >>> d = {}
        >>> d[1] = d.keys()
        >>> d[2] = d.values()
        >>> d
        {1: dict_keys([1, 2]), 2: dict_values([dict_keys([1, 2]), ...])}

    which I think is preferable.

    Summary of patch:

    dictobject.c:

    dictview_repr() now uses a Py_ReprEnter() / Py_ReprLeave() pair to
    check for recursion, and produces "..." if so.  I think I've got the
    behaviour correct in the case that PySequence_List() fails, but
    couldn't find a way to trigger this in testing.
    

    test_dictviews.py:

    test_recursive_repr() checks for the correct string (containing
    "...") rather than a RuntimeError.
    
    test_deeply_nested_repr() is a new function, aiming to replace the
    original test_recursive_repr().  It checks that a RuntimeError is
    raised in the case of a non-recursive but deeply nested structure.
    

    Under --with-pydebug,

    ./python -m test.regrtest -R20:30 test_dictviews
    

    passes.

    @vstinner
    Copy link
    Member

    non-error-recursive-dictview-3.3.patch: small nit, initialize result to NULL, instead of setting it to NULL only in case of error. It makes the code more readable and it is the common coding style to handle errors.

    @bennorth
    Copy link
    Mannequin Author

    bennorth mannequin commented Jul 24, 2013

    New patch, fixing nit noted in msg193624:

    non-error-recursive-dictview-3.3-1.patch
    

    Otherwise same as previous

    non-error-recursive-dictview-3.3.patch
    

    @bitdancer
    Copy link
    Member

    I'm in favor of treating this a bugfix. It is a bug for repr to raise a recursion error, IMO.

    @rhettinger
    Copy link
    Contributor

    +1 for backporting the fix.

    @bennorth
    Copy link
    Mannequin Author

    bennorth mannequin commented Jul 30, 2013

    I'll attach fresh patches, one against latest 2.7 and one against
    latest 3.3 (unchanged apart from headers from previous patch).

    Both branches pass

    ./python -m test.regrtest -R20:30 test_dictviews
    

    @bennorth
    Copy link
    Mannequin Author

    bennorth mannequin commented Aug 15, 2013

    Is anything further needed from me before this can be reviewed?

    @terryjreedy
    Copy link
    Member

    Please visit
    http://www.python.org/psf/contrib/
    http://www.python.org/psf/contrib/contrib-form/
    and submit a Contributor Agreement. This process is complete when '*' appears after your name here, as with mine and others.

    @csabella
    Copy link
    Contributor

    Ben, would you be interested in making a Github pull request against the current master for your patch? It appears there was interest in moving forward with this and it is still occurring on 3.7, although it now raises a RecursionError.

    >>> d = {}
    >>> d[1] = d.keys()
    >>> d
    {1: dict_keys([1])}
    >>> d[2] = d.values()
    >>> d
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    RecursionError: maximum recursion depth exceeded while getting the repr of an object

    @vstinner
    Copy link
    Member

    See also bpo-32137.

    @bennorth
    Copy link
    Mannequin Author

    bennorth mannequin commented Dec 12, 2017

    PR4823 created as per msg308086.

    @csabella
    Copy link
    Contributor

    It looks like Serhiy approved the PR on 12/13. Is it ready to merge? Thanks!

    @terryjreedy
    Copy link
    Member

    Not ready to merge.

    1. We need the Appveyer test to pass or special action from the gateway gods.
    2. After a month, the PR needed to be updated and retested anyway. I pulled, updated, pushed the update (which will trigger Travis and AV), and tested on my Windows machine. The two new tests fail. See PR.
    3. Someone has to sell assign and merge when ready.

    @terryjreedy
    Copy link
    Member

    Hold the failure comment. I forgot that I have to recompile when patch changes .c files.

    @terryjreedy
    Copy link
    Member

    OK, test_dictviews and test_ordered_dict now pass for me, and Appveyor already ran entire suite and says OK to all.

    @orsenthil
    Copy link
    Member

    New changeset d7773d9 by Senthil Kumaran (bennorth) in branch 'master':
    bpo-18533: Avoid RecursionError from repr() of recursive dictview (bpo-4823)
    d7773d9

    @orsenthil
    Copy link
    Member

    I merged this one. All the requirements were met.

    @terry / @ Cheryl - could you please review if I did it right? Should this be backported to other Python versions?

    @orsenthil orsenthil added the 3.7 (EOL) end of life label Jan 26, 2018
    @terryjreedy
    Copy link
    Member

    Thank you for checking and merging. In the future, we will be reminded to change '#' to 'GH-' in the title. I only found out a couple of days ago that this is desired.

    David, in msg193651, and Raymond, in msg193779, already suggested backporting. I added the 'needs backport' tags.

    The auto backport for 3.6 worked. You might want to checked the diff as well as test results.

    As I expected from the fact that Ben once did a separate 2.7 version, the auto backport for 2.7 failed. Ben, since you know what code changes are needed, can you prepare a backport (cherry pick) PR?

    @bennorth
    Copy link
    Mannequin Author

    bennorth mannequin commented Jan 27, 2018

    Terry in msg310785:

    As I expected from the fact that Ben once did a separate 2.7
    version, the auto backport for 2.7 failed. Ben, since you know what
    code changes are needed, can you prepare a backport (cherry pick)
    PR?

    Yes, that's right, there were some differences between the approaches
    in Py2 and Py3. I've created PR5357 for the 2.7 backport.

    @miss-islington
    Copy link
    Contributor

    New changeset fbf7aac by Miss Islington (bot) in branch '3.6':
    bpo-18533: Avoid RecursionError from repr() of recursive dictview (GH-4823)
    fbf7aac

    @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 interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants