[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Donald Stufft]

Changes by Donald Stufft :


--
pull_requests: +1093

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
assignee:  -> serhiy.storchaka
resolution:  -> fixed
stage: commit 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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Stéphane Wirtel]

Stéphane Wirtel added the comment:

Serhiy, your patch works fine.

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Stéphane Wirtel]

Changes by Stéphane Wirtel :


--
stage: patch review -> commit review

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Stéphane Wirtel]

Stéphane Wirtel added the comment:

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:  returned a result with an error set\\n\\nCurrent 
thread.*:\\n  File .*, line 6 in ' 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:  returned a result with an error 
set\n\nCurrent thread 0x7f5c560c3700 (most recent call first):\n  File 
"", line 6 in '

test_capi failed

1 test failed:
test_capi

Total duration: 6 sec
Tests result: FAILURE

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

First apply the patch from issue28410.

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Stéphane Wirtel]

Stéphane Wirtel added the comment:

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

--
nosy: +matrixise

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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 
class TheoreticallyCouldWork:
RuntimeError: Error calling __set_name__ on 'FaultyImplementation' instance 
'attr' in 'TheoreticallyCouldWork'

--
Added file: http://bugs.python.org/file45129/set_name_chain_error_cause_2.patch

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
dependencies: +Add convenient C API for "raise ... from ..."

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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 "", line 1, in 
RuntimeError: Error calling __set_name__ on 'FaultyImplementation' instance 
'attr' in 'TheoreticallyCouldWork'

--
Added file: http://bugs.python.org/file45029/set_name_chain_error_cause.patch

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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 "", line 1, in 
RuntimeError: Error calling __set_name__ on 'FaultyImplementation' instance 
'attr' in 'TheoreticallyCouldWork'

--
Added file: http://bugs.python.org/file45028/set_name_error.patch

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Nick Coghlan]

Nick Coghlan added the comment:

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

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Nick Coghlan]

Nick Coghlan added the comment:

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

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Nick Coghlan]

Nick Coghlan added the comment:

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

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

What error messages you want for these cases?

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Nick Coghlan]

Nick Coghlan added the comment:

@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 "", line 1, in 
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 "", line 1, in 
  File "", line 3, in __set_name__
ZeroDivisionError: division by zero

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Now we need other reproducer for an exception.

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Nick Coghlan]

Nick Coghlan added the comment:

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 tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Eric Snow]

Eric Snow added the comment:

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.

--
nosy: +eric.snow

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--
keywords: +patch
stage: needs patch -> patch review
Added file: http://bugs.python.org/file44757/lookup___set_name__.patch

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--
nosy: +serhiy.storchaka

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Nick Coghlan]

Changes by Nick Coghlan :


--
nosy: +Martin Teichmann, Martin.Teichmann
stage:  -> needs patch

___
Python tracker 

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

[issue28214] Improve exception reporting for problematic __set_name__ attributes

[Tim Graham]

New submission from Tim Graham:

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

--
components: Interpreter Core
messages: 277027
nosy: Tim.Graham, ncoghlan
priority: normal
severity: normal
status: open
title: Improve exception reporting for problematic __set_name__ attributes
type: behavior
versions: Python 3.6, Python 3.7

___
Python tracker 

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