This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: pprint handling of dict subclasses that override __repr__
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Mariatta, eric.smith, fdrake, iritkatriel, palakjha, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2020-03-17 16:19 by palakjha, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 19046 closed palakjha, 2020-03-17 16:52
PR 21892 merged iritkatriel, 2020-08-15 18:15
Messages (17)
msg364442 - (view) Author: Palak Kumar Jha (palakjha) * Date: 2020-03-17 16:19
In the PrettyPrinter._format method, since self._dispatch has dict.__repr__ [key] mapped to self._pprint_dict [value] the elif block is not needed. Its work is already being done by the if block above, which searches self._dispatch to fetch the appropriate value.
msg375261 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2020-08-12 16:37
This change makes a difference in semantics (perhaps a good one) in this case:

import pprint

class MyDict(dict):
    def __repr__(self):
        return 'I do my own thing'*50

if __name__ == '__main__':
    d=MyDict()
    for i in range(50):
        d['a%s'%i] = i

    pprint.pprint(d)


Before the change MyDict.__repr__ is ignored and the dict contents are printed. After the change it prints "I do my own thing" 50 times.
msg375270 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-08-12 17:38
Yes, this code was kept for backward compatibility when the dispatch mapping was added.
msg375271 - (view) Author: Fred Drake (fdrake) (Python committer) Date: 2020-08-12 17:44
Adding serhiy.storchaka to nosy list since it looks like he introduced the isinstance check on purpose (see bpo-23741).

Though bpo-23741 asserts that no behavior was changed with the patch applied then, reading through the change leads me to think this did change for cases like the MyDict example from iritkatriel.

The intent of the original code was that MyDict.__repr__ would be used; only the small handful of "known" reprs would be handled specially.
msg375279 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-08-12 18:48
bpo-23741 did not change the behavior. The specified code was added specially to preserve the existing behavior. For other base types the condition was like `issubclass(typ, list) and r is list.__repr__`, but for dict it was just `issubclass(typ, dict)`.

Actually the initial behavior was changed by bad3c88094f43f3bc7dcce22f47b8c2a8dddabcf (no issue number), with adding support of OrderedDict.
msg375280 - (view) Author: Fred Drake (fdrake) (Python committer) Date: 2020-08-12 18:52
Ah, good find!  It's been so long since the first version of that code, but the implementation was surprisingly nuanced.
msg375425 - (view) Author: Mariatta (Mariatta) * (Python committer) Date: 2020-08-14 18:57
@Palak Kumar Jha, you created a PR for this ticket but it seems that your GitHub account is inactive/has been removed. Do you have a new GitHub account? Are you still interested in continuing on this PR?
msg375451 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2020-08-15 07:22
I went ahead and closed the PR. Either @Palak Kumar Jha or someone else can create a new PR. The suggestions in the original PR should be addresses.
msg375453 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2020-08-15 09:02
Thanks, I'll create a new PR and credit @Palak Kumar Jha as co-author.
msg375465 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2020-08-15 14:05
While writing the tests I see that it's even more interesting:

(Pdb) dd = MyDict()
(Pdb) pprint.pformat(dd)
'{}'
(Pdb) pprint.saferepr(dd)
'I do my own thing ....'
msg375467 - (view) Author: Fred Drake (fdrake) (Python committer) Date: 2020-08-15 14:47
And that is why the original code was checking not only for the type, but the actual __repr__ method itself.

I think the current behavior is broken.
msg375470 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2020-08-15 15:28
I realize it might break some corner cases, but I really think we should re-write pprint to use functools.singledispatch. Or if the breakage isn't acceptable, abandon it and create a new module that does use singledispatch. That way it would be easily extensible.

pprint currently works with a dispatch table (_dispatch), it's just hidden.

@fdrake: From the comments: "If you find it useful, thank small children who sleep at night". I assume they're not small anymore!
msg375473 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2020-08-15 15:59
import pprint

class MyDict(dict):
    def __repr__(self):
        return '*'*len(dict.__repr__(self))

if __name__ == '__main__':
    d=MyDict({})
    print('pprint.pformat(d):\n%s' % pprint.pformat(d))
    print('pprint.pformat(d, width=1, indent=0):\n%s' % pprint.pformat(d, width=1, indent=0))



Output:

pprint.pformat(d):
**
pprint.pformat(d, width=1, indent=0):
{}
msg375485 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2020-08-15 18:18
I've created a new PR, with two commits. The first has just tests added, which show the problems and assert that the bug is there. The second has the fix and update to the tests. Let me know what you think.
msg375491 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2020-08-15 19:02
Only 3.8 - 3.10 would be eligible for this fix. 3.7 is getting only security fixes.
msg376117 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-08-30 17:29
New changeset 582f13786bb75c73d609790967fea03a5b50148a by Irit Katriel in branch 'master':
bpo-39994: Fix pprint handling of dict subclasses that override __repr__ (GH-21892)
https://github.com/python/cpython/commit/582f13786bb75c73d609790967fea03a5b50148a
msg376120 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2020-08-30 18:45
Thank you Serhiy.  This ticket can be closed now.
History
Date User Action Args
2022-04-11 14:59:28adminsetgithub: 84175
2020-08-30 19:46:24eric.smithsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2020-08-30 18:45:10iritkatrielsetmessages: + msg376120
2020-08-30 17:29:57serhiy.storchakasetmessages: + msg376117
2020-08-15 19:02:47eric.smithsetmessages: + msg375491
versions: - Python 3.5, Python 3.6, Python 3.7
2020-08-15 18:46:49iritkatrielsetversions: + Python 3.5, Python 3.6, Python 3.7, Python 3.10
2020-08-15 18:18:54iritkatrielsetmessages: + msg375485
2020-08-15 18:15:18iritkatrielsetpull_requests: + pull_request21011
2020-08-15 15:59:56iritkatrielsetmessages: + msg375473
2020-08-15 15:28:33eric.smithsetmessages: + msg375470
2020-08-15 14:47:43fdrakesetmessages: + msg375467
2020-08-15 14:05:42iritkatrielsetmessages: + msg375465
2020-08-15 13:34:04iritkatrielsettype: enhancement -> behavior
components: + Library (Lib)
title: Redundant code in pprint module. -> pprint handling of dict subclasses that override __repr__
2020-08-15 09:02:59iritkatrielsetmessages: + msg375453
2020-08-15 07:22:53eric.smithsetnosy: + eric.smith
messages: + msg375451
2020-08-14 18:57:10Mariattasetnosy: + Mariatta
messages: + msg375425
2020-08-12 18:52:28fdrakesetmessages: + msg375280
2020-08-12 18:48:47serhiy.storchakasetnosy: + rhettinger
messages: + msg375279
2020-08-12 17:44:48fdrakesetmessages: + msg375271
2020-08-12 17:38:53serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg375270
2020-08-12 16:37:48iritkatrielsetnosy: + iritkatriel
messages: + msg375261
2020-03-19 18:43:57palakjhasetnosy: + fdrake

versions: - Python 3.7
2020-03-17 16:52:42palakjhasetkeywords: + patch
stage: patch review
pull_requests: + pull_request18397
2020-03-17 16:19:33palakjhacreate