Improve exception reporting for problematic __set_name__ attributes

[timgraham]

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]

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)

[ncoghlan]

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]

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]

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

[serhiy-storchaka]

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

[ericsnowcurrently]

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]

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]

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]

Now we need other reproducer for an exception.

[ncoghlan]

@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]

What error messages you want for these cases?

[ncoghlan]

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)}"