Avoid error from repr() of recursive dictview

[bennorth]

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]

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.

[terryjreedy]

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]

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]

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]

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]

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]

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

[rhettinger]

+1 for backporting the fix.

[bennorth]

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]

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

[terryjreedy]

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]

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]

See also bpo-32137.