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

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__'
```

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.

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.

Yes, I would love to! Thanks :)

I will wait for GH-29207 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>
> _______________________________________
>

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

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>
> _______________________________________
>

Couple of thoughts.

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

```python
    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? :)

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.

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>
> _______________________________________
>

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.

New changeset a82baed0e9e61c0d8dc5c12fc08de7fc172c1a38 by Irit Katriel in branch 'main':
bpo-45615: Add missing test for printing traceback for non-exception. Fix traceback.py (GH-30091)
https://github.com/python/cpython/commit/a82baed0e9e61c0d8dc5c12fc08de7fc172c1a38