classification
Title: Improve exception reporting for problematic __set_name__ attributes
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: 28410 Superseder:
Assigned To: serhiy.storchaka Nosy List: Martin Teichmann, Martin.Teichmann, Tim.Graham, eric.snow, matrixise, ncoghlan, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-09-20 12:38 by Tim.Graham, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
lookup___set_name__.patch serhiy.storchaka, 2016-09-20 16:58 review
set_name_chain_error_context.patch serhiy.storchaka, 2016-10-09 06:31 review
set_name_chain_error_cause.patch serhiy.storchaka, 2016-10-09 06:41 review
set_name_chain_error_cause_2.patch serhiy.storchaka, 2016-10-18 11:19 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (24)
msg277027 - (view) Author: Tim Graham (Tim.Graham) * Date: 2016-09-20 12:38
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 #27366.

[0] https://github.com/andialbrecht/sqlparse/issues/286#issuecomment-248208900
msg277038 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-20 15:51
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.
msg277040 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-20 16:03
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__.
msg277043 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-20 16:58
Proposed patch makes class creation code not looking up the __set_name__ attribute in instance dict.
msg277045 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-20 17:04
In any case I suggest to raise an AttributeError for dunder names in __getattr__ implementation. Otherwise you will encounter similar issue with pickling.
msg277053 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-09-20 17:59
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.
msg277111 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-21 08:57
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.
msg277133 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-21 12:57
New changeset 1a2b8398f045 by Serhiy Storchaka in branch '3.6':
Issue #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 #28214: Now __set_name__ is looked up on the class instead of the
https://hg.python.org/cpython/rev/2a5280db601c
msg277139 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-21 13:16
Now we need other reproducer for an exception.
msg277158 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-21 15:22
@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
msg278289 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-10-08 10:53
What error messages you want for these cases?
msg278331 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-10-09 03:03
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)}"
msg278333 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-10-09 03:06
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
msg278334 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-10-09 03:08
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)}"
msg278349 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-10-09 06:31
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'
msg278350 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-10-09 06:41
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'
msg278409 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-10-10 11:38
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.
msg278853 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-10-18 11:19
Updated patch uses C API added in issue28410. 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'
msg278854 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2016-10-18 11:30
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
msg278855 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-10-18 11:55
First apply the patch from issue28410.
msg278856 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2016-10-18 12:12
Serhiy, I have a failed test with test_capi. I have applied the patch from #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
msg278875 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-10-18 13:14
Thank you for testing Stéphane. Ignore this failure, the test should be fixed by updated patch in issue28410.
msg279051 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2016-10-20 17:10
Serhiy, your patch works fine.
msg279130 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-10-21 14:16
New changeset f7e1e39ccddd by Serhiy Storchaka in branch '3.6':
Issue #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 #28214: Improved exception reporting for problematic __set_name__
https://hg.python.org/cpython/rev/7c3ec24f4582
History
Date User Action Args
2017-03-31 16:36:37dstufftsetpull_requests: + pull_request1093
2016-10-21 14:18:53serhiy.storchakasetstatus: open -> closed
assignee: serhiy.storchaka
resolution: fixed
stage: commit review -> resolved
2016-10-21 14:16:11python-devsetmessages: + msg279130
2016-10-20 17:10:44matrixisesetmessages: + msg279051
2016-10-18 15:32:24matrixisesetstage: patch review -> commit review
2016-10-18 13:14:24serhiy.storchakasetmessages: + msg278875
2016-10-18 12:12:33matrixisesetmessages: + msg278856
2016-10-18 11:55:21serhiy.storchakasetmessages: + msg278855
2016-10-18 11:30:50matrixisesetnosy: + matrixise
messages: + msg278854
2016-10-18 11:19:35serhiy.storchakasetfiles: + set_name_chain_error_cause_2.patch

messages: + msg278853
2016-10-10 19:51:44serhiy.storchakasetdependencies: + Add convenient C API for "raise ... from ..."
2016-10-10 11:38:45ncoghlansetmessages: + msg278409
2016-10-09 06:41:32serhiy.storchakasetfiles: + set_name_chain_error_cause.patch

messages: + msg278350
2016-10-09 06:31:21serhiy.storchakasetfiles: + set_name_chain_error_context.patch

messages: + msg278349
2016-10-09 03:08:18ncoghlansetmessages: + msg278334
2016-10-09 03:06:53ncoghlansetmessages: + msg278333
2016-10-09 03:03:54ncoghlansetmessages: + msg278331
2016-10-08 10:53:37serhiy.storchakasetmessages: + msg278289
2016-09-21 15:22:05ncoghlansetmessages: + msg277158
2016-09-21 13:16:16serhiy.storchakasetmessages: + msg277139
2016-09-21 12:57:59python-devsetnosy: + python-dev
messages: + msg277133
2016-09-21 08:57:43ncoghlansetmessages: + msg277111
2016-09-20 17:59:23eric.snowsetnosy: + eric.snow
messages: + msg277053
2016-09-20 17:04:23serhiy.storchakasetmessages: + msg277045
2016-09-20 16:58:14serhiy.storchakasetfiles: + lookup___set_name__.patch
keywords: + patch
messages: + msg277043

stage: needs patch -> patch review
2016-09-20 16:03:46serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg277040
2016-09-20 15:51:41ncoghlansetmessages: + msg277038
2016-09-20 15:45:12ncoghlansetnosy: + Martin.Teichmann, Martin Teichmann

stage: needs patch
2016-09-20 12:38:42Tim.Grahamcreate