Missing test for type of error when printing traceback for non-exception

[iritkatriel]

BPO	45615
Nosy	@gvanrossum, @erlend-aasland, @sobolevn, @iritkatriel
PRs	bpo-45615: Add missing test for printing traceback for non-exception. Fix… #30091

^{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 2022-01-02.09:36:17.694>
created_at = <Date 2021-10-26.15:17:10.937>
labels = ['interpreter-core', 'type-bug', 'library', '3.11']
title = 'Missing test for type of error when printing traceback for non-exception'
updated_at = <Date 2022-01-02.09:36:17.694>
user = 'https://github.com/iritkatriel'

bugs.python.org fields:

activity = <Date 2022-01-02.09:36:17.694>
actor = 'iritkatriel'
assignee = 'none'
closed = True
closed_date = <Date 2022-01-02.09:36:17.694>
closer = 'iritkatriel'
components = ['Interpreter Core', 'Library (Lib)']
creation = <Date 2021-10-26.15:17:10.937>
creator = 'iritkatriel'
dependencies = []
files = []
hgrepos = []
issue_num = 45615
keywords = ['patch']
message_count = 12.0
messages = ['405048', '405670', '405671', '405678', '405680', '405775', '405789', '405859', '405897', '405899', '405900', '409491']
nosy_count = 4.0
nosy_names = ['gvanrossum', 'erlendaasland', 'sobolevn', 'iritkatriel']
pr_nums = ['30091']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue45615'
versions = ['Python 3.11']

[iritkatriel]

In C code, there is a check in print_exception that the value passed in is of type exception, and it raises TypeError otherwise. This check is not covered by tests, and indeed it is hard to reach it with tests because the _testcapi module has this check as well, which blocks it before the interpreter's check is reached.

In traceback.py, there is no such check so this happens: 
>>> traceback.print_exception(12)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/iritkatriel/src/cpython-654/Lib/traceback.py", line 121, in print_exception
    value, tb = _parse_value_tb(exc, value, tb)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/iritkatriel/src/cpython-654/Lib/traceback.py", line 102, in _parse_value_tb
    raise TypeError(f"Expected exception, got {exc}")
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: Expected exception, got 12

[sobolevn]

For me something different happens now:

Python 3.11.0a1+ (heads/main-dirty:e03e50377d, Nov  4 2021, 13:09:20) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import traceback
>>> traceback.print_exception(12)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/sobolev/Desktop/cpython/Lib/traceback.py", line 118, in print_exception
    value, tb = _parse_value_tb(exc, value, tb)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sobolev/Desktop/cpython/Lib/traceback.py", line 100, in _parse_value_tb
    return exc, exc.__traceback__
                ^^^^^^^^^^^^^^^^^
AttributeError: 'int' object has no attribute '__traceback__'

[iritkatriel]

You're right Nikita, that's what main currently does. My output was from a branch where I started fixing it. Sorry about the confusion.

[iritkatriel]

Nikita, if you want to work on this go head, but I would wait until PR 29207 is merged.

I think in the C code what we need to do is remove the check in _testcapi - this is a test utility, it doesn't need to verify input types.

[sobolevn]

Yes, I would love to! Thanks :)

I will wait for #73393 to be merged first.

чт, 4 нояб. 2021 г. в 14:02, Irit Katriel <report@bugs.python.org>:

Irit Katriel <iritkatriel@gmail.com> added the comment:

Nikita, if you want to work on this go head, but I would wait until PR
29207 is merged.

I think in the C code what we need to do is remove the check in _testcapi

• this is a test utility, it doesn't need to verify input types.

----------

---

Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue45615\>

---

[iritkatriel]

It's been merged now, so go ahead.

Note that there are two traceback printing functions - in the interpreter in traceback.c and in the library in traceback.py. They are expected to work the same as much as possible.

If you add a test to BaseExceptionReportingTests, it will test both versions (see the PyExcReportingTests and PyExcReportingTests subclasses).

[sobolevn]

Thanks!

пт, 5 нояб. 2021 г. в 12:47, Irit Katriel <report@bugs.python.org>:

Irit Katriel <iritkatriel@gmail.com> added the comment:

It's been merged now, so go ahead.

Note that there are two traceback printing functions - in the interpreter
in traceback.c and in the library in traceback.py. They are expected to
work the same as much as possible.

If you add a test to BaseExceptionReportingTests, it will test both
versions (see the PyExcReportingTests and PyExcReportingTests subclasses).

----------

---

Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue45615\>

---

[sobolevn]

Couple of thoughts.

1. You have to create quite complex structural "clone" of Exception for python-based traceback:

    def test_non_exception_subtype(self):
        class RegularObject:
            __traceback__ = None
            __suppress_context__ = None
            __cause__ = None
            __context__ = None

            def __call__(self):
                return self  # we need it for `get_exception` to work

        obj = RegularObject()

        try:
            1 / 0
        except Exception as ex:
            obj.__traceback__ = ex.__traceback__

        err = self.get_report(obj, force=True)
        self.assertIn('1 / 0', err)  # passes

Is it really worth it?

2. Removing PyExceptionInstance_Check(value) from https://github.com/python/cpython/blob/main/Modules/_testcapimodule.c#L3508-L3511 does not really help that much, because we still need to call PyErr_Display below. Which assumes value to be Exception.

There's no correct way of calling print_exception() directly as far as I understand. It is only called in print_exception_recursive, which in its order is called from:

• print_chained (called recursively from print_exception_recursive)
• _PyErr_Display -> PyErrDisplay

So, maybe instead we should change print_exception to not type check value again?

Or we can cahnge some levels above. Like PyErrDisplay, it can return TypeError earlier if case value is invalid.

What do you think? :)

[iritkatriel]

1. I don't think we need such a clone of exception. We just need something like these two tests:

    @cpython_only
    def test_print_exception_bad_type_ct(self):
        with self.assertRaises(TypeError):
            from _testcapi import exception_print
            exception_print(42)

    def test_print_exception_bad_type_python(self):
        with self.assertRaises(TypeError):
            traceback.print_exception(42)

It could be that they don't fit in BaseExceptionReportingTests because that is for tests that use get_report. It's fine of they are added separately. The python one can come after test_exception_is_None, and the C one perhaps after test_unhashable (and their names should be slightly different than above).

2. _testcapi is how you call into print_exception directly (for testing). If I remove the type check in _testcapi then the test above segfaults with

Assertion failed: (PyExceptionInstance_Check(exc)), function _PyBaseExceptionObject_cast, file exceptions.c, line 321.

This issue was created because Erlend found that the type check in print_exception is not covered by tests. It's possible that this check is in the wrong place at the moment.

[sobolevn]

1. Thanks! Yes, this is exactly the case I am talking about.

Right now, this test won't pass:

    def test_print_exception_bad_type_python(self):
        with self.assertRaises(TypeError):
            traceback.print_exception(42)

Why? Because we don't type check the argument to be Exception subtype.
It fails with AttributeError. And my question is more like: should we?

I have several opposing thoughts:

• Most likely it is always used with Exception. I am pretty sure that
  re-implementing exceptions is not something you would do. And it will be in
  sync with C code.
• But, on the other hand, it is a breaking change.

вс, 7 нояб. 2021 г. в 13:37, Irit Katriel <report@bugs.python.org>:

Irit Katriel <iritkatriel@gmail.com> added the comment:

1. I don't think we need such a clone of exception. We just need something
   like these two tests:

   @cpython_only
   def test_print_exception_bad_type_ct(self):
   with self.assertRaises(TypeError):
   from _testcapi import exception_print
   exception_print(42)

   def test_print_exception_bad_type_python(self):
   with self.assertRaises(TypeError):
   traceback.print_exception(42)

It could be that they don't fit in BaseExceptionReportingTests because
that is for tests that use get_report. It's fine of they are added
separately. The python one can come after test_exception_is_None, and the C
one perhaps after test_unhashable (and their names should be slightly
different than above).

2. _testcapi is how you call into print_exception directly (for testing).
   If I remove the type check in _testcapi then the test above segfaults with

Assertion failed: (PyExceptionInstance_Check(exc)), function
_PyBaseExceptionObject_cast, file exceptions.c, line 321.

This issue was created because Erlend found that the type check in
print_exception is not covered by tests. It's possible that this check is
in the wrong place at the moment.

----------

---

Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue45615\>

---

[iritkatriel]

I see what you mean. I think it's ok in traceback.py to reject an exception clone which is not an instance of BaseException. I agree this should not be backported. You could make that explicit by adding a few words to this sentence in the doc, to make it about the exc value as well as the traceback:

"The module uses traceback objects — this is the object type that is stored in the sys.last_traceback variable and returned as the third item from sys.exc_info()."

(https://docs.python.org/3/library/traceback.html)

The C code fix doesn't need to be backported either - this issue is not something a user had a problem with, we found it through test coverage.

[iritkatriel]

New changeset a82baed by Irit Katriel in branch 'main':
bpo-45615: Add missing test for printing traceback for non-exception. Fix traceback.py (GH-30091)
a82baed