classification
Title: Avoid error from repr() of recursive dictview
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7, Python 3.6, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: orsenthil Nosy List: bennorth, cheryl.sabella, miss-islington, orsenthil, r.david.murray, rhettinger, serhiy.storchaka, terry.reedy, vstinner, zuo
Priority: normal Keywords: patch

Created on 2013-07-22 22:22 by bennorth, last changed 2018-06-03 10:01 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
non-error-recursive-dictview.patch bennorth, 2013-07-22 22:22 Patch avoiding RuntimeError for recursive dictview; give "..." instead review
non-error-recursive-dictview-3.3.patch bennorth, 2013-07-23 23:20 Patch against 3.3 to avoid RuntimeError for recursive dictview; give "..." instead review
non-error-recursive-dictview-3.3-1.patch bennorth, 2013-07-24 06:36 review
against-6e1dd1ce95b8-2.7-1.patch bennorth, 2013-07-30 18:18 Patch against 2.7 branch review
against-9bf89c909bd4-3.3-1.patch bennorth, 2013-07-30 18:18 Patch against 3.3 branch review
Pull Requests
URL Status Linked Edit
PR 4823 merged python-dev, 2017-12-12 22:21
PR 5348 merged python-dev, 2018-01-26 19:47
PR 5357 merged bennorth, 2018-01-27 09:48
Messages (23)
msg193570 - (view) Author: Ben North (bennorth) * Date: 2013-07-22 22:22
#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.
msg193571 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-07-22 22:47
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
msg193573 - (view) Author: Jan Kaliszewski (zuo) Date: 2013-07-22 23:46
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.
msg193620 - (view) Author: Ben North (bennorth) * Date: 2013-07-23 23:20
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.
msg193624 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-07-24 00:24
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.
msg193626 - (view) Author: Ben North (bennorth) * Date: 2013-07-24 06:36
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
msg193651 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-24 14:40
I'm in favor of treating this a bugfix.  It is a bug for repr to raise a recursion error, IMO.
msg193779 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-07-27 06:40
+1 for backporting the fix.
msg193944 - (view) Author: Ben North (bennorth) * Date: 2013-07-30 18:18
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
msg195260 - (view) Author: Ben North (bennorth) * Date: 2013-08-15 16:15
Is anything further needed from me before this can be reviewed?
msg195262 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-08-15 16:42
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.
msg308086 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-12-12 02:00
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
msg308107 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-12-12 10:33
See also bpo-32137.
msg308164 - (view) Author: Ben North (bennorth) * Date: 2017-12-12 22:33
PR4823 created as per msg308086.
msg310642 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2018-01-24 22:49
It looks like Serhiy approved the PR on 12/13.  Is it ready to merge?  Thanks!
msg310644 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-01-24 23:39
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.
msg310645 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-01-24 23:42
Hold the failure comment.  I forgot that I have to recompile when patch changes .c files.
msg310646 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-01-24 23:57
OK, test_dictviews and test_ordered_dict now pass for me, and Appveyor already ran entire suite and says OK to all.
msg310770 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2018-01-26 15:46
New changeset d7773d92bd11640a8c950d6c36a9cef1cee36f96 by Senthil Kumaran (bennorth) in branch 'master':
bpo-18533: Avoid RecursionError from repr() of recursive dictview (#4823)
https://github.com/python/cpython/commit/d7773d92bd11640a8c950d6c36a9cef1cee36f96
msg310771 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2018-01-26 15:49
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?
msg310785 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-01-26 20:00
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?
msg310851 - (view) Author: Ben North (bennorth) * Date: 2018-01-27 09:54
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.
msg312928 - (view) Author: miss-islington (miss-islington) Date: 2018-02-26 14:42
New changeset fbf7aac36bd1017bc87964b5d17dce0e101ff2d6 by Miss Islington (bot) in branch '3.6':
bpo-18533: Avoid RecursionError from repr() of recursive dictview (GH-4823)
https://github.com/python/cpython/commit/fbf7aac36bd1017bc87964b5d17dce0e101ff2d6
History
Date User Action Args
2018-06-03 10:01:31serhiy.storchakasetstatus: open -> closed
stage: patch review -> resolved
2018-02-26 14:42:02miss-islingtonsetnosy: + miss-islington
messages: + msg312928
2018-01-27 09:54:00bennorthsetmessages: + msg310851
2018-01-27 09:48:27bennorthsetstage: needs patch -> patch review
pull_requests: + pull_request5203
2018-01-26 20:00:18terry.reedysetassignee: orsenthil
stage: patch review -> needs patch
messages: + msg310785
versions: + Python 2.7, Python 3.6
2018-01-26 19:47:53python-devsetpull_requests: + pull_request5196
2018-01-26 15:49:39orsenthilsetnosy: rhettinger, terry.reedy, orsenthil, vstinner, bennorth, r.david.murray, zuo, serhiy.storchaka, cheryl.sabella
messages: + msg310771
resolution: fixed
versions: + Python 3.7, - Python 2.7, Python 3.3, Python 3.4
2018-01-26 15:46:04orsenthilsetnosy: + orsenthil
messages: + msg310770
2018-01-24 23:57:13terry.reedysetmessages: + msg310646
2018-01-24 23:42:47terry.reedysetmessages: + msg310645
2018-01-24 23:39:01terry.reedysetmessages: + msg310644
2018-01-24 22:49:00cheryl.sabellasetmessages: + msg310642
2017-12-12 22:33:38bennorthsetmessages: + msg308164
2017-12-12 22:21:36python-devsetstage: test needed -> patch review
pull_requests: + pull_request4713
2017-12-12 10:33:31vstinnersetnosy: + serhiy.storchaka
messages: + msg308107
2017-12-12 02:00:42cheryl.sabellasetnosy: + cheryl.sabella
messages: + msg308086
2013-08-15 16:42:47terry.reedysetmessages: + msg195262
2013-08-15 16:15:16bennorthsetmessages: + msg195260
2013-07-30 18:18:51bennorthsetfiles: + against-9bf89c909bd4-3.3-1.patch
2013-07-30 18:18:37bennorthsetfiles: + against-6e1dd1ce95b8-2.7-1.patch
2013-07-30 18:18:11bennorthsetmessages: + msg193944
2013-07-27 06:40:13rhettingersetnosy: + rhettinger
messages: + msg193779
2013-07-24 14:40:08r.david.murraysetnosy: + r.david.murray
messages: + msg193651
2013-07-24 06:36:58bennorthsetfiles: + non-error-recursive-dictview-3.3-1.patch

messages: + msg193626
2013-07-24 00:24:57vstinnersetmessages: + msg193624
2013-07-23 23:20:06bennorthsetfiles: + non-error-recursive-dictview-3.3.patch

messages: + msg193620
2013-07-22 23:46:20zuosetnosy: + zuo
messages: + msg193573
2013-07-22 22:47:28terry.reedysetversions: + Python 3.3
nosy: + terry.reedy

messages: + msg193571

stage: test needed
2013-07-22 22:24:07vstinnersetnosy: + vstinner
2013-07-22 22:22:30bennorthcreate