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

[Irit Katriel]

Change by Irit Katriel :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Irit Katriel]

Irit Katriel  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Irit Katriel]

Change by Irit Katriel :


--
keywords: +patch
pull_requests: +28314
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/30091

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Irit Katriel]

Irit Katriel  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nikita Sobolev]

Nikita Sobolev  added the comment:

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 :

>
> Irit Katriel  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 
> 
> ___
>

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Irit Katriel]

Irit Katriel  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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nikita Sobolev]

Nikita Sobolev  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nikita Sobolev]

Nikita Sobolev  added the comment:

Thanks!

пт, 5 нояб. 2021 г. в 12:47, Irit Katriel :

>
> Irit Katriel  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 
> 
> ___
>

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Irit Katriel]

Irit Katriel  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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nikita Sobolev]

Nikita Sobolev  added the comment:

Yes, I would love to! Thanks :)

I will wait for GH-29207 to be merged first.

чт, 4 нояб. 2021 г. в 14:02, Irit Katriel :

>
> Irit Katriel  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 
> 
> ___
>

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Irit Katriel]

Irit Katriel  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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Irit Katriel]

Irit Katriel  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nikita Sobolev]

Nikita Sobolev  added the comment:

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 "", line 1, in 
  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__'
```

--
nosy: +sobolevn

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Guido van Rossum]

Change by Guido van Rossum :


--
nosy: +gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Erlend E. Aasland]

Change by Erlend E. Aasland :


--
nosy: +erlendaasland

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Irit Katriel]

New submission from Irit Katriel :

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 "", line 1, in 
  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

--
components: Interpreter Core, Library (Lib)
messages: 405048
nosy: iritkatriel
priority: normal
severity: normal
status: open
title: Missing test for type of error when printing traceback for non-exception
type: behavior
versions: Python 3.11

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com