bpo-18533: Avoid RuntimeError from repr() of recursive dictview #4823

bennorth · 2017-12-12T22:21:35Z

(Created as per recent comment on bpo-18533. See issue for further background and discussion.)

dictview_repr(): Use a Py_ReprEnter() / Py_ReprLeave() pair to check for recursion, and produce "..." if so.

test_recursive_repr(): Check for the correct string (containing "...") rather than a RuntimeError.

test_deeply_nested_repr(): Add new test, replacing the original test_recursive_repr(). It checks that a RuntimeError is raised in the case of a non-recursive but deeply nested structure. (Very similar to what test_repr_deep() in test/test_dict.py does for a normal dict.)

https://bugs.python.org/issue18533

the-knights-who-say-ni · 2017-12-12T22:21:37Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

vstinner · 2017-12-12T22:34:16Z

Hum, this bug has already been fixed in https://bugs.python.org/issue32137 by the commit 1fb72d2, no?

cc @serhiy-storchaka

bennorth · 2017-12-12T22:40:13Z

The fix for bpo-32137 changed a segfault into a RecursionError. This PR proposes changing that into a friendlier "..."-style representation, i.e., no exception is raised.

(I've just noticed that my commit message incorrectly refers to RuntimeError, sorry, as this is what it was last time I looked at this. I can force-push a commit with better message if this PR is approved in principle.)

serhiy-storchaka

LGTM. Just fix a style and add a news entry.

What about OrderedDict? Is it fixed by this change?

serhiy-storchaka · 2017-12-12T22:53:15Z

Objects/dictobject.c

+    Py_ssize_t rc;
+
+    rc = Py_ReprEnter((PyObject *)dv);
+    if (rc != 0)


Add braces.

serhiy-storchaka · 2017-12-12T22:53:41Z

Objects/dictobject.c

+
+    rc = Py_ReprEnter((PyObject *)dv);
+    if (rc != 0)
+        return rc > 0 ? PyUnicode_FromString("...") : NULL;

    seq = PySequence_List((PyObject *)dv);
    if (seq == NULL)


Add braces.

serhiy-storchaka · 2017-12-12T22:55:54Z

Lib/test/test_dictviews.py

@@ -202,6 +203,18 @@ def test_items_set_operations(self):
    def test_recursive_repr(self):
        d = {}
        d[42] = d.values()
+        r = repr(d)
+        self.assertIsInstance(r, str)


Is this needed?

bennorth · 2017-12-13T16:59:57Z

@serhiy-storchaka --- I've implemented those style requests, and added a news entry.

Without this PR, OrderedDict raises a RecursionError:

>>> from collections import OrderedDict
>>> d = OrderedDict()
>>> d[42] = d.values()
>>> repr(d)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RecursionError: maximum recursion depth exceeded while getting the repr of an object

whereas with this PR:

>>> repr(d)
'OrderedDict([(42, odict_values([...]))])'

The PR now adds a test for the new behaviour of OrderedDict.

serhiy-storchaka · 2017-12-13T17:26:15Z

Objects/dictobject.c

+    rc = Py_ReprEnter((PyObject *)dv);
+    if (rc != 0) {
+        return rc > 0 ? PyUnicode_FromString("...") : NULL;
+    }



I think empty lines are not needed after }. } itself adds enough vertical space.

serhiy-storchaka · 2017-12-13T17:32:58Z

Lib/test/test_collections.py

+
+class TestOrderedDict(unittest.TestCase):
+
+    def test_recursive_repr(self):


Move this test into test_ordered_dict.py.

serhiy-storchaka · 2017-12-13T17:38:07Z

Lib/test/test_collections.py

+        d = OrderedDict()
+        d[42] = d.values()
+        r = repr(d)
+        self.assertEqual(r, 'OrderedDict([(42, odict_values([...]))])')


The concrete implementation is implementation dependent. For example in PyPy3 it is OrderedDict([(42, _OrderedDictValuesView(...))]). I think it enough to test that repr() doesn't raise an exception in other implementations.

r = repr(d) if test.support.check_impl_detail(cpython=True): self.assertEqual(r, 'OrderedDict([(42, odict_values([...]))])')

dictview_repr(): Use a Py_ReprEnter() / Py_ReprLeave() pair to check for recursion, and produce "..." if so. test_recursive_repr(): Check for the a string rather than a RecursionError. (Test cannot be any tighter as contents are implementation-dependent.) test_deeply_nested_repr(): Add new test, replacing the original test_recursive_repr(). It checks that a RecursionError is raised in the case of a non-recursive but deeply nested structure. (Very similar to what test_repr_deep() in test/test_dict.py does for a normal dict.) OrderedDictTests: Add new test case, to test behaviour on OrderedDict instances containing their own values() or items().

bennorth · 2017-12-13T22:30:28Z

Thanks for feedback. New version pushed addressing comments:

Remove vertical whitespace;
Avoid relying on particular implementation in tests;
Move OrderedDict test to more sensible location.

Also word 'news' entry more concisely.

bennorth · 2017-12-29T22:08:59Z

Is there anything further I should be doing to help this along? The failing AppVeyor build looks unrelated as it's a failure in distutils.tests.test_bdist_wininst.BuildWinInstTestCase.

bennorth · 2018-01-25T15:06:17Z

@terryjreedy — Thanks for bringing this up-to-date with master; good that the AppVeyor build is now OK.

orsenthil · 2018-01-26T15:46:33Z

I merged it, @bennorth. All the requirements were met and thanks for your contribution.

miss-islington · 2018-01-26T19:46:46Z

Thanks @bennorth for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington · 2018-01-26T19:46:46Z

Thanks @bennorth for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖

bedevere-bot · 2018-01-26T19:47:53Z

GH-5348 is a backport of this pull request to the 3.6 branch.

…thonGH-4823) dictview_repr(): Use a Py_ReprEnter() / Py_ReprLeave() pair to check for recursion, and produce "..." if so. test_recursive_repr(): Check for the string rather than a RecursionError. (Test cannot be any tighter as contents are implementation-dependent.) test_deeply_nested_repr(): Add new test, replacing the original test_recursive_repr(). It checks that a RecursionError is raised in the case of a non-recursive but deeply nested structure. (Very similar to what test_repr_deep() in test/test_dict.py does for a normal dict.) OrderedDictTests: Add new test case, to test behavior on OrderedDict instances containing their own values() or items(). (cherry picked from commit d7773d9)

miss-islington · 2018-01-26T19:48:48Z

Sorry, @bennorth and @orsenthil, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker d7773d92bd11640a8c950d6c36a9cef1cee36f96 2.7

bennorth · 2018-01-27T09:50:21Z

@orsenthil — thanks for merging.

As suggested in comment on issue, I've created #5357 to backport this to 2.7.

…-4823) dictview_repr(): Use a Py_ReprEnter() / Py_ReprLeave() pair to check for recursion, and produce "..." if so. test_recursive_repr(): Check for the string rather than a RecursionError. (Test cannot be any tighter as contents are implementation-dependent.) test_deeply_nested_repr(): Add new test, replacing the original test_recursive_repr(). It checks that a RecursionError is raised in the case of a non-recursive but deeply nested structure. (Very similar to what test_repr_deep() in test/test_dict.py does for a normal dict.) OrderedDictTests: Add new test case, to test behavior on OrderedDict instances containing their own values() or items(). (cherry picked from commit d7773d9)

…on#4823) dictview_repr(): Use a Py_ReprEnter() / Py_ReprLeave() pair to check for recursion, and produce "..." if so. test_recursive_repr(): Check for a string rather than a RuntimeError. (Test cannot be any tighter as contents are implementation-dependent.) test_deeply_nested_repr(): Add new test, replacing the original test_recursive_repr(). It checks that a RuntimeError is raised in the case of a non-recursive but deeply nested structure. OrderedDictTests: Add new test case, to test behavior on OrderedDict instances containing their own viewvalues() or viewitems(). This test passes without the patch to dictview_repr(), but it failed in (at least some versions of) Python 3, so including it here for completeness. (cherry picked from commit d7773d9)

… (#5357) (cherry picked from commit d7773d9)

the-knights-who-say-ni added the CLA not signed label Dec 12, 2017

bedevere-bot added the awaiting review label Dec 12, 2017

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Dec 12, 2017

serhiy-storchaka added the type-feature A feature request or enhancement label Dec 12, 2017

serhiy-storchaka reviewed Dec 12, 2017

View reviewed changes

bennorth force-pushed the fix-issue-18533 branch from 3822baa to cc8c16b Compare December 13, 2017 16:56

bennorth requested a review from rhettinger as a code owner December 13, 2017 16:56

serhiy-storchaka reviewed Dec 13, 2017

View reviewed changes

bennorth force-pushed the fix-issue-18533 branch from cc8c16b to 30dcf01 Compare December 13, 2017 22:27

serhiy-storchaka approved these changes Dec 13, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Dec 13, 2017

Merge remote-tracking branch 'origin/master' into pr_4823

2eab412

orsenthil merged commit d7773d9 into python:master Jan 26, 2018

bedevere-bot removed the awaiting merge label Jan 26, 2018

terryjreedy added needs backport to 3.6 labels Jan 26, 2018

bedevere-bot removed the needs backport to 3.6 label Jan 26, 2018

bennorth mentioned this pull request Jan 27, 2018

[2.7] bpo-18533: Avoid RuntimeError from repr() of recursive dictview [GH-4823] #5357

Merged

serhiy-storchaka pushed a commit that referenced this pull request Feb 26, 2018

bpo-18533: Avoid RuntimeError from repr() of recursive dictview (#4823)…

c20c97f

… (#5357) (cherry picked from commit d7773d9)

serhiy-storchaka assigned orsenthil May 14, 2018


		class TestOrderedDict(unittest.TestCase):

		def test_recursive_repr(self):

Uh oh!

bpo-18533: Avoid RuntimeError from repr() of recursive dictview #4823

bpo-18533: Avoid RuntimeError from repr() of recursive dictview #4823

Uh oh!

Conversation

bennorth commented Dec 12, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Dec 12, 2017

Uh oh!

vstinner commented Dec 12, 2017

Uh oh!

bennorth commented Dec 12, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

bennorth commented Dec 13, 2017

Uh oh!

serhiy-storchaka Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

bennorth commented Dec 13, 2017

Uh oh!

bennorth commented Dec 29, 2017

Uh oh!

bennorth commented Jan 25, 2018

Uh oh!

orsenthil commented Jan 26, 2018

Uh oh!

miss-islington commented Jan 26, 2018

Uh oh!

miss-islington commented Jan 26, 2018

Uh oh!

bedevere-bot commented Jan 26, 2018

Uh oh!

miss-islington commented Jan 26, 2018

Uh oh!

bennorth commented Jan 27, 2018

Uh oh!

Uh oh!

bennorth commented Dec 12, 2017 •

edited by bedevere-bot

Loading