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

Improve exception reporting for problematic __set_name__ attributes #72401

Closed
timgraham mannequin opened this issue Sep 20, 2016 · 24 comments
Closed

Improve exception reporting for problematic __set_name__ attributes #72401

timgraham mannequin opened this issue Sep 20, 2016 · 24 comments
Assignees
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@timgraham
Copy link
Mannequin

timgraham mannequin commented Sep 20, 2016

BPO 28214
Nosy @ncoghlan, @ericsnowcurrently, @serhiy-storchaka, @matrixise, @tecki, @timgraham
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
  • Dependencies
  • bpo-28410: Add convenient C API for "raise ... from ..."
  • Files
  • lookup___set_name__.patch
  • set_name_chain_error_context.patch
  • set_name_chain_error_cause.patch
  • set_name_chain_error_cause_2.patch
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2016-10-21.14:18:53.602>
    created_at = <Date 2016-09-20.12:38:42.593>
    labels = ['interpreter-core', 'type-bug', '3.7']
    title = 'Improve exception reporting for problematic __set_name__ attributes'
    updated_at = <Date 2017-03-31.16:36:37.403>
    user = 'https://github.com/timgraham'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:37.403>
    actor = 'dstufft'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2016-10-21.14:18:53.602>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2016-09-20.12:38:42.593>
    creator = 'Tim.Graham'
    dependencies = ['28410']
    files = ['44757', '45028', '45029', '45129']
    hgrepos = []
    issue_num = 28214
    keywords = ['patch']
    message_count = 24.0
    messages = ['277027', '277038', '277040', '277043', '277045', '277053', '277111', '277133', '277139', '277158', '278289', '278331', '278333', '278334', '278349', '278350', '278409', '278853', '278854', '278855', '278856', '278875', '279051', '279130']
    nosy_count = 8.0
    nosy_names = ['ncoghlan', 'python-dev', 'eric.snow', 'serhiy.storchaka', 'matrixise', 'Martin.Teichmann', 'Tim.Graham', 'Martin Teichmann']
    pr_nums = ['552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue28214'
    versions = ['Python 3.6', 'Python 3.7']

    @timgraham
    Copy link
    Mannequin Author

    timgraham mannequin commented Sep 20, 2016

    As requested by Nick [0], this is a usability issue against CPython 3.6 to provide a better chained TypeError in this case:

    class _TokenType(tuple):
        parent = None
    
        def __getattr__(self, name):
            new = _TokenType(self + (name,))
            setattr(self, name, new)
            new.parent = self
            return new
    
    Token = _TokenType()
    
    Keyword = Token.Keyword
    
    class KeywordCaseFilter(object):
        ttype = Keyword
    Traceback (most recent call last):
      File "test.py", line 14, in <module>
        class KeywordCaseFilter(object):
    TypeError: '_TokenType' object is not callable

    The exception should report the specific object with the problematic __set_name__ attribute (rather than just passing along the underlying exception), as well as supporting __set_name__ = None to explicitly disable further lookup processing.

    Follow up to bpo-27366.

    [0] andialbrecht/sqlparse#286 (comment)

    @timgraham timgraham mannequin added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Sep 20, 2016
    @ncoghlan
    Copy link
    Contributor

    The information we want to include in the chained exception:

    1. The name of the offending attribute (since the traceback will point to the class header, not to the assignment)
    2. The repr of the offending attribute (since the innner exception will refer to the return value from "attr.__set_name__" rather then the value assigned to the attribute)

    The other idea I mentioned (allowing "__set_name__ = None" to prevent falling back to "__getattr__") needs a bit more consideration, as I don't quite recall where we're currently at in terms of expanding that idiom beyond "__hash__" and to the other one-shot ABCs.

    @serhiy-storchaka
    Copy link
    Member

    Maybe there is a bug in using __set_name__. Usually special methods are looked in class dict. But __set_name__ is looked in instance dict. This causes to invoking __getattr__.

    @serhiy-storchaka
    Copy link
    Member

    Proposed patch makes class creation code not looking up the __set_name__ attribute in instance dict.

    @serhiy-storchaka
    Copy link
    Member

    In any case I suggest to raise an AttributeError for dunder names in __getattr__ implementation. Otherwise you will encounter similar issue with pickling.

    @ericsnowcurrently
    Copy link
    Member

    I agree with Serhiy that __set_name__ should be looked up on the class like all other special methods. Pickle is a great example why lookup (of __reduce__) on the instance is a pain.

    @ncoghlan
    Copy link
    Contributor

    Good catch Serhiy - I'd completely missed that in the original review, and definitely agree we should make that fix independently of the exception chaining idea.

    While that correction will fix the specific __getattr__ example given, we still have the problem of actual errors in looking up __set_name__ on the class (e.g. in a metaclass or in __getattribute__) and errors when calling it being hard to debug, as the traceback will point to the class header without giving the name of the offending attribute.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 21, 2016

    New changeset 1a2b8398f045 by Serhiy Storchaka in branch '3.6':
    Issue bpo-28214: Now __set_name__ is looked up on the class instead of the
    https://hg.python.org/cpython/rev/1a2b8398f045

    New changeset 2a5280db601c by Serhiy Storchaka in branch 'default':
    Issue bpo-28214: Now __set_name__ is looked up on the class instead of the
    https://hg.python.org/cpython/rev/2a5280db601c

    @serhiy-storchaka
    Copy link
    Member

    Now we need other reproducer for an exception.

    @ncoghlan
    Copy link
    Contributor

    @Property can be used to define a broken __set_name__ attribute:

    >>> class BadIdea:
    ...     @property
    ...     def __set_name__(self):
    ...         pass
    ... 
    >>> class NotGoingToWork:
    ...     attr = BadIdea()
    ... 
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: 'NoneType' object is not callable

    And there's also a failing __set_name__ call:

    >>> class FaultyImplementation:
    ...     def __set_name__(self, *args):
    ...         1/0
    ... 
    >>> class TheoreticallyCouldWork:
    ...     attr = FaultyImplementation()
    ... 
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "<stdin>", line 3, in __set_name__
    ZeroDivisionError: division by zero

    @serhiy-storchaka
    Copy link
    Member

    What error messages you want for these cases?

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Oct 9, 2016

    The following seem like a reasonable starting point to me:

    TypeError: Failed to set name on 'BadIdea' instance 'attr' in 'NotGoingToWork': 'NoneType' object is not callable

    TypeError: Failed to set name on 'FaultyImplementation' instance 'attr' in 'TheoreticallyCouldWork': ZeroDivisionError: division by zero

    That is, the error message format would be along the lines of:

    f"Failed to set name on {type(the_attr).__name__!r} instance {the_attr_name!r} in {class_being_defined.__name__!r}: {str(raised_exc)}"
    

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Oct 9, 2016

    Slight change, as it makes sense to reference the special method name explicitly:

    TypeError: Error calling __set_name__ on 'BadIdea' instance 'attr' in 'NotGoingToWork': 'NoneType' object is not callable

    TypeError: Error calling __set_name__ on 'FaultyImplementation' instance 'attr' in 'TheoreticallyCouldWork': ZeroDivisionError: division by zero

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Oct 9, 2016

    Fixing the proposed format string accordingly, and also including the underlying exception type as I did in the examples:

    f"Error calling __set_name__ on {type(the_attr).__name__!r} instance {the_attr_name!r} in {class_being_defined.__name__!r}: {type(raised_exc).__name__}: {str(raised_exc)}"

    @serhiy-storchaka
    Copy link
    Member

    Shouldn't it be RuntimeError?

    Proposed patch makes RuntimeError be raised with chained original exception. This preserves more information.

    >>> class FaultyImplementation:
    ...     def __set_name__(self, *args):
    ...         1/0
    ... 
    >>> class TheoreticallyCouldWork:
    ...     attr = FaultyImplementation()
    ... 
    ZeroDivisionError: division by zero
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    RuntimeError: Error calling __set_name__ on 'FaultyImplementation' instance 'attr' in 'TheoreticallyCouldWork'

    @serhiy-storchaka
    Copy link
    Member

    Alternative patch chain original exception as __cause__ instead of __context__. What is better?

    >>> class FaultyImplementation:
    ...     def __set_name__(self, *args):
    ...         1/0
    ... 
    >>> class TheoreticallyCouldWork:
    ...     attr = FaultyImplementation()
    ... 
    ZeroDivisionError: division by zero
    
    The above exception was the direct cause of the following exception:
    
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    RuntimeError: Error calling __set_name__ on 'FaultyImplementation' instance 'attr' in 'TheoreticallyCouldWork'

    @ncoghlan
    Copy link
    Contributor

    You're right, RuntimeError is a more suitable exception type.

    So +1 for the second version of the patch that uses __cause__ and the patch itself looks good to me.

    @serhiy-storchaka
    Copy link
    Member

    Updated patch uses C API added in bpo-28410. It preserves the traceback of original exception:

    Traceback (most recent call last):
      File "issue28214.py", line 3, in __set_name__
        1/0
    ZeroDivisionError: division by zero
    
    The above exception was the direct cause of the following exception:
    
    Traceback (most recent call last):
      File "issue28214.py", line 5, in <module>
        class TheoreticallyCouldWork:
    RuntimeError: Error calling __set_name__ on 'FaultyImplementation' instance 'attr' in 'TheoreticallyCouldWork'

    @matrixise
    Copy link
    Member

    Serhiy,

    I don't find the _PyErr_FormatFromCause function in the code of CPython.

    Modules/_tracemalloc.o Modules/hashtable.o Modules/symtablemodule.o Modules/xxsubtype.o -lpthread -ldl -lutil -lm
    Objects/typeobject.o: In function set_names': /home/stephane/src/python/cpython/Objects/typeobject.c:7026: undefined reference to _PyErr_FormatFromCause'
    collect2: error: ld returned 1 exit status
    Makefile:736: recipe for target 'Programs/_freeze_importlib' failed
    make: *** [Programs/_freeze_importlib] Error 1

    @serhiy-storchaka
    Copy link
    Member

    First apply the patch from bpo-28410.

    @matrixise
    Copy link
    Member

    Serhiy, I have a failed test with test_capi. I have applied the patch from bpo-28410 and set_name_chain_error_cause_2.patch.

    stephane@sg1 ~/s/p/cpython> ./python -m test test_capi
    Run tests sequentially
    0:00:00 [1/1] test_capi
    test test_capi failed -- Traceback (most recent call last):
      File "/home/stephane/src/python/cpython/Lib/test/test_capi.py", line 221, in test_return_result_with_error
        br'Fatal Python error: a function returned a '
    AssertionError: Regex didn't match: b'Fatal Python error: a function returned a result with an error set\\nValueError\\n\\nDuring handling of the above exception, another exception occurred:\\n\\nSystemError: <built-in function return_result_with_error> returned a result with an error set\\n\\nCurrent thread.*:\\n  File .*, line 6 in <module>' not found in b'Fatal Python error: a function returned a result with an error set\nValueError\n\nThe above exception was the direct cause of the following exception:\n\nSystemError: <built-in function return_result_with_error> returned a result with an error set\n\nCurrent thread 0x00007f5c560c3700 (most recent call first):\n  File "<string>", line 6 in <module>'

    test_capi failed

    1 test failed:
    test_capi

    Total duration: 6 sec
    Tests result: FAILURE

    @serhiy-storchaka
    Copy link
    Member

    Thank you for testing Stéphane. Ignore this failure, the test should be fixed by updated patch in bpo-28410.

    @matrixise
    Copy link
    Member

    Serhiy, your patch works fine.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 21, 2016

    New changeset f7e1e39ccddd by Serhiy Storchaka in branch '3.6':
    Issue bpo-28214: Improved exception reporting for problematic __set_name__
    https://hg.python.org/cpython/rev/f7e1e39ccddd

    New changeset 7c3ec24f4582 by Serhiy Storchaka in branch 'default':
    Issue bpo-28214: Improved exception reporting for problematic __set_name__
    https://hg.python.org/cpython/rev/7c3ec24f4582

    @serhiy-storchaka serhiy-storchaka self-assigned this Oct 21, 2016
    @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.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants