Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
iritkatriel opened this issue Oct 26, 2021 · 12 comments
Closed
Labels
3.11 bug and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@iritkatriel
Copy link
Member

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
    Copy link
    Member Author

    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

    @iritkatriel iritkatriel added 3.11 bug and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Oct 26, 2021
    @sobolevn
    Copy link
    Member

    sobolevn commented Nov 4, 2021

    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
    Copy link
    Member Author

    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
    Copy link
    Member Author

    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
    Copy link
    Member

    sobolevn commented Nov 4, 2021

    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
    Copy link
    Member Author

    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
    Copy link
    Member

    sobolevn commented Nov 5, 2021

    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
    Copy link
    Member

    sobolevn commented Nov 6, 2021

    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?

    1. 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
    Copy link
    Member Author

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

    1. _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
    Copy link
    Member

    sobolevn commented Nov 7, 2021

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

    1. _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
    Copy link
    Member Author

    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
    Copy link
    Member Author

    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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 bug and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants