pdb: do_p and do_pp swallow exceptions from __repr__

[blueyed]

BPO	37022
Nosy	@blueyed, @miss-islington, @remilapeyre, @iritkatriel
PRs	bpo-37022: Fix bug where pdb's do_p/do_pp commands swallow exceptions from repr #18180 [3.10] bpo-37022: Fix bug where pdb's do_p/do_pp commands swallow exceptions from repr (GH-18180) #26650 [3.9] bpo-37022: Fix bug where pdb's do_p/do_pp commands swallow exceptions from repr (GH-18180) #26651

^{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 = None
closed_at = <Date 2021-06-10.21:24:31.935>
created_at = <Date 2019-05-23.16:02:14.288>
labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
title = 'pdb: do_p and do_pp swallow exceptions from __repr__'
updated_at = <Date 2021-06-10.21:24:31.934>
user = 'https://github.com/blueyed'

bugs.python.org fields:

activity = <Date 2021-06-10.21:24:31.934>
actor = 'iritkatriel'
assignee = 'none'
closed = True
closed_date = <Date 2021-06-10.21:24:31.935>
closer = 'iritkatriel'
components = ['Library (Lib)']
creation = <Date 2019-05-23.16:02:14.288>
creator = 'blueyed'
dependencies = []
files = []
hgrepos = []
issue_num = 37022
keywords = ['patch']
message_count = 7.0
messages = ['343306', '343391', '343392', '343395', '395571', '395574', '395575']
nosy_count = 4.0
nosy_names = ['blueyed', 'miss-islington', 'remi.lapeyre', 'iritkatriel']
pr_nums = ['18180', '26650', '26651']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue37022'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

[blueyed]

Given:

class BadRepr:
    def __repr__(self):
        raise Exception('repr_exc')


obj = BadRepr()

__import__('pdb').set_trace()

(Pdb) p obj
(Pdb) pp obj
(Pdb)

Possible patch - clumsy due to self._getval both printing any error already, and raising the exception:

diff --git i/Lib/pdb.py w/Lib/pdb.py
index f5d33c27fc..59a419d961 100755
--- i/Lib/pdb.py
+++ w/Lib/pdb.py
@@ -1177,18 +1177,28 @@ def do_p(self, arg):
         Print the value of the expression.
         """
         try:
-            self.message(repr(self._getval(arg)))
+            val = self._getval(arg)
         except:
-            pass
+            return
+        try:
+            self.message(repr(val))
+        except:
+            exc_info = sys.exc_info()[:2]
+            self.error(traceback.format_exception_only(*exc_info)[-1].strip())

     def do_pp(self, arg):
         """pp expression
         Pretty-print the value of the expression.
         """
         try:
-            self.message(pprint.pformat(self._getval(arg)))
+            val = self._getval(arg)
         except:
-            pass
+            return
+        try:
+            self.message(pprint.pformat(val))
+        except:
+            exc_info = sys.exc_info()[:2]
+            self.error(traceback.format_exception_only(*exc_info)[-1].strip())

     complete_print = _complete_expression
     complete_p = _complete_expression

[remilapeyre]

Hi Daniel, this is indeed unexpected, I don't see how to have a better patch since the fact that _getval() raise an exception is used in do_source() and do_whatis().

Could you convert your patch as a PR and add a test?

[blueyed]

Thanks for the feedback.

What do you think of refactoring the common block (also used in other places) into a method?

+ except:
+ exc_info = sys.exc_info()[:2]
+ self.error(traceback.format_exception_only(*exc_info)[-1].strip())

This could/should be done separately probably then, just want your opinion on it. This would also allow to customize/override it in a single place then (e.g. with pdbpp I am displaying tracebacks for errors, which is done via error() currently then).

[remilapeyre]

This part of the code is already used in three places and your patch would add two occurrences of of it, I think it would be great to put the part that print the exception in a private method, to avoid duplicating it all over the place.

Doing this seems small enough to me that I think it could be done in the same PR to avoid the overhead of having two separates PRs.

[iritkatriel]

New changeset 6544b25 by Daniel Hahler in branch 'main':
bpo-37022: Fix bug where pdb's do_p/do_pp commands swallow exceptions from repr (GH-18180)
6544b25

[iritkatriel]

New changeset e3bc32f by Miss Islington (bot) in branch '3.10':
bpo-37022: Fix bug where pdb's do_p/do_pp commands swallow exceptions from repr (GH-18180) (GH-26650)
e3bc32f

[iritkatriel]

New changeset 175ebc6 by Miss Islington (bot) in branch '3.9':
bpo-37022: Fix bug where pdb's do_p/do_pp commands swallow exceptions from repr (GH-18180) (GH-26651)
175ebc6