NotificationsYou must be signed in to change notification settings
Fork33.4k
Star69.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

orsenthil merged 2 commits intopython:masterfrombennorth:fix-issue-18533

Jan 26, 2018

Merged

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

orsenthil merged 2 commits intopython:masterfrombennorth:fix-issue-18533

Jan 26, 2018

Conversation

Copy link

Contributor

bennorth commentedDec 12, 2017•
edited by bedevere-bot
Loading

(Created as perrecent comment onbpo-18533. See issue for further background and discussion.)

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

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

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

https://bugs.python.org/issue18533

Copy link

the-knights-who-say-ni commentedDec 12, 2017

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 thePSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username onbugs.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 followthe steps outlined in the CPython devguide to rectify this issue.

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

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

Copy link

Member

vstinner commentedDec 12, 2017

Hum, this bug has already been fixed inhttps://bugs.python.org/issue32137 by the commit1fb72d2, no?

cc@serhiy-storchaka

Copy link

ContributorAuthor

bennorth commentedDec 12, 2017

The fix forbpo-32137 changed a segfault into aRecursionError. 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 toRuntimeError, 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 added the type-featureA feature request or enhancement label

Dec 12, 2017

serhiy-storchaka reviewed

Dec 12, 2017

View reviewed changes

Copy link

Member

serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

What about OrderedDict? Is it fixed by this change?

Objects/dictobject.c Outdated

Copy link

Member

serhiy-storchakaDec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Add braces.

Objects/dictobject.c Outdated

Copy link

Member

serhiy-storchakaDec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Add braces.

Lib/test/test_dictviews.py Outdated

Copy link

Member

serhiy-storchakaDec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is this needed?

bennorth force-pushed thefix-issue-18533 branch from3822baa tocc8c16bCompare

December 13, 2017 16:56

bennorth requested a review fromrhettinger as acode owner

December 13, 2017 16:56

Copy link

ContributorAuthor

bennorth commentedDec 13, 2017

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

Without this PR,OrderedDict raises aRecursionError:

>>> 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 ofOrderedDict.

serhiy-storchaka reviewed

Dec 13, 2017

View reviewed changes

Objects/dictobject.c Outdated

Copy link

Member

serhiy-storchakaDec 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Lib/test/test_collections.py Outdated

Copy link

Member

serhiy-storchakaDec 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Move this test into test_ordered_dict.py.

Lib/test/test_collections.py Outdated

Copy link

Member

serhiy-storchakaDec 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

bpo-18533: Avoid RecursionError from repr() of recursive dictview

30dcf01

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

bennorth force-pushed thefix-issue-18533 branch fromcc8c16b to30dcf01Compare

December 13, 2017 22:27

Copy link

ContributorAuthor

bennorth commentedDec 13, 2017

Thanks for feedback. New version pushed addressing comments:

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

Also word 'news' entry more concisely.

serhiy-storchaka approved these changes

Dec 13, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels

Dec 13, 2017

Copy link

ContributorAuthor

bennorth commentedDec 29, 2017

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

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

2eab412

Copy link

ContributorAuthor

bennorth commentedJan 25, 2018

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

orsenthil merged commitd7773d9 intopython:master

Jan 26, 2018

bedevere-bot removed the awaiting merge label

Jan 26, 2018

Copy link

Member

orsenthil commentedJan 26, 2018

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

terryjreedy added needs backport to 3.6 labels

Jan 26, 2018

Copy link

Contributor

miss-islington commentedJan 26, 2018

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

Copy link

Contributor

miss-islington commentedJan 26, 2018

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

Copy link

bedevere-bot commentedJan 26, 2018

GH-5348 is a backport of this pull request to the3.6 branch.

bedevere-bot removed the needs backport to 3.6 label

Jan 26, 2018

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jan 26, 2018

bpo-18533: Avoid RecursionError from repr() of recursive dictview (py…

039743d

…thonGH-4823)dictview_repr(): Use a Py_ReprEnter() / Py_ReprLeave() pair to checkfor recursion, and produce "..." if so.test_recursive_repr(): Check for the string rather than aRecursionError.  (Test cannot be any tighter as contents areimplementation-dependent.)test_deeply_nested_repr(): Add new test, replacing the originaltest_recursive_repr().  It checks that a RecursionError is raised inthe case of a non-recursive but deeply nested structure.  (Verysimilar to what test_repr_deep() in test/test_dict.py does for anormal dict.)OrderedDictTests: Add new test case, to test behavior on OrderedDictinstances containing their own values() or items().(cherry picked from commitd7773d9)

Copy link

Contributor

miss-islington commentedJan 26, 2018

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

bennorth mentioned this pull request

Jan 27, 2018

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

Merged

Copy link

ContributorAuthor

bennorth commentedJan 27, 2018

@orsenthil — thanks for merging.

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

miss-islington added a commit that referenced this pull request

Feb 26, 2018

bpo-18533: Avoid RecursionError from repr() of recursive dictview (GH…

fbf7aac

…-4823)dictview_repr(): Use a Py_ReprEnter() / Py_ReprLeave() pair to checkfor recursion, and produce "..." if so.test_recursive_repr(): Check for the string rather than aRecursionError.  (Test cannot be any tighter as contents areimplementation-dependent.)test_deeply_nested_repr(): Add new test, replacing the originaltest_recursive_repr().  It checks that a RecursionError is raised inthe case of a non-recursive but deeply nested structure.  (Verysimilar to what test_repr_deep() in test/test_dict.py does for anormal dict.)OrderedDictTests: Add new test case, to test behavior on OrderedDictinstances containing their own values() or items().(cherry picked from commitd7773d9)

bennorth added a commit to bennorth/cpython that referenced this pull request

Feb 26, 2018

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

a27a0d4

…on#4823)dictview_repr(): Use a Py_ReprEnter() / Py_ReprLeave() pair to checkfor recursion, and produce "..." if so.test_recursive_repr(): Check for a string rather than aRuntimeError.  (Test cannot be any tighter as contents areimplementation-dependent.)test_deeply_nested_repr(): Add new test, replacing the originaltest_recursive_repr().  It checks that a RuntimeError is raised inthe case of a non-recursive but deeply nested structure.OrderedDictTests: Add new test case, to test behavior on OrderedDictinstances containing their own viewvalues() or viewitems().  This testpasses without the patch to dictview_repr(), but it failed in (atleast some versions of) Python 3, so including it here forcompleteness.(cherry picked from commitd7773d9)

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 commitd7773d9)

serhiy-storchaka assignedorsenthil

May 14, 2018

Labels

type-feature

A feature request or enhancement

Movatterモバイル変換

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 commentedDec 12, 2017• edited by bedevere-botLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commentedDec 12, 2017

Uh oh!

vstinner commentedDec 12, 2017

Uh oh!

bennorth commentedDec 12, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaDec 12, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaDec 12, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaDec 12, 2017

Choose a reason for hiding this comment

Uh oh!

bennorth commentedDec 13, 2017

Uh oh!

serhiy-storchakaDec 13, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaDec 13, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaDec 13, 2017

Choose a reason for hiding this comment

Uh oh!

bennorth commentedDec 13, 2017

Uh oh!

bennorth commentedDec 29, 2017

Uh oh!

bennorth commentedJan 25, 2018

Uh oh!

orsenthil commentedJan 26, 2018

Uh oh!

miss-islington commentedJan 26, 2018

Uh oh!

miss-islington commentedJan 26, 2018

Uh oh!

bedevere-bot commentedJan 26, 2018

Uh oh!

miss-islington commentedJan 26, 2018

Uh oh!

bennorth commentedJan 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

bennorth commentedDec 12, 2017•
edited by bedevere-bot
Loading