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
Comments
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 >>> 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 >>> 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 On python-dev, Gregory Smith noted:
so I have marked for consideration for versions 2.7 and 3.4. |
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 |
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 |
New patch including tests attached, against 3.3. Terry Reedy's example >>> 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:
test_dictviews.py:
Under --with-pydebug,
passes. |
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. |
New patch, fixing nit noted in msg193624:
Otherwise same as previous
|
I'm in favor of treating this a bugfix. It is a bug for repr to raise a recursion error, IMO. |
+1 for backporting the fix. |
I'll attach fresh patches, one against latest 2.7 and one against Both branches pass
|
Is anything further needed from me before this can be reviewed? |
Please visit |
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 |
See also bpo-32137. |
It looks like Serhiy approved the PR on 12/13. Is it ready to merge? Thanks! |
Not ready to merge.
|
Hold the failure comment. I forgot that I have to recompile when patch changes .c files. |
OK, test_dictviews and test_ordered_dict now pass for me, and Appveyor already ran entire suite and says OK to all. |
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? |
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? |
Terry in msg310785:
Yes, that's right, there were some differences between the approaches |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: