msg369324 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2020-05-19 07:57 |
When calling an instance method incorrectly, you will often get a TypeError that is some variation of the following:
Traceback (most recent call last):
File "/.../test.py", line 6, in <module>
a.foo(1)
TypeError: foo() takes 1 positional argument but 2 were given
However, when multiple classes have method foo() and the type of "a" isn't immediately obvious, this often isn't enough to know what method was being called. Thus, it would be more helpful if the error message includes also the class that foo() belongs to, or alternatively the type of the object. (These can be different when subclasses are involved.)
For comparison, if you call a method that doesn't exist, you will get a message that looks like the following:
Traceback (most recent call last):
File "/.../test.py", line 6, in <module>
a.bar(1)
AttributeError: 'A' object has no attribute 'bar'
So taking from this as an example, the message in the first case could be something like--
TypeError: foo() for 'A' object takes 1 positional argument but 2 were given
|
msg369382 - (view) |
Author: Dennis Sweeney (Dennis Sweeney) *  |
Date: 2020-05-19 19:46 |
The attached PR isn't exactly what you requested, but it's a very minimal code change that uses the existing __qualname__ functionality to change the message to
TypeError: A.foo() takes 1 positional argument but 2 were given
Does that address those problems?
|
msg369405 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2020-05-19 22:59 |
Thanks! I think it does.
Also, I see now that using the __qualname__ is better than including the object's type for locating the method because you can have cases like super().foo() or even--
class A:
def foo(self):
pass
def bar():
pass
A.foo = bar
|
msg369421 - (view) |
Author: Dennis Sweeney (Dennis Sweeney) *  |
Date: 2020-05-20 07:09 |
While trying to write tests, I stumbled across something interesting: _PyObject_FunctionString as discussed here ( https://bugs.python.org/issue37645 ) returns a string that also includes the module name where applicable. For example, the module name is included behind the qualname in the following situation:
>>> from random import Random
>>> Random.seed(a=1, **{"a":2})
Traceback (most recent call last):
File "<pyshell#7>", line 1, in <module>
Random.seed(a=1, **{"a":2})
TypeError: random.Random.seed() got multiple values for keyword argument 'a'
or:
>>> class A:
... def foo(bar):
... pass
...
>>> A().foo(bar=1, **{"bar":2})
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: __main__.A.foo() got multiple values for keyword argument 'bar'
From what I can tell, this seems to happen mostly during the CALL_FUNCTION_EX instruction (unpacking *args and **kwargs), while we don't get this level of qualification during the actual do_call_core. Maybe _PyObject_FunctionString should ideally be used everywhere applicable, but there seems to be a different issue with that: the _PyEval_EvalCode function where the error handling occurs doesn't receive a reference to the function, only references to the function's code object and qualname (unless I'm missing to something).
Should the signature of _PyEval_EvalCode be modified? Or should we be satisfied with the half-measure of including the qualname but not the module (at least for now)? Is there a good reason for the different qualifiers for functions when encountering different types of invalid arguments?
There's also a block that I couldn't figure out how to reach in tests from Python code: at https://github.com/python/cpython/blob/master/Python/ceval.c#L4233 ("keywords must be strings" when passing as two arrays), I don't know how to reach this code in a way that isn't a SyntaxError first.
|
msg369422 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2020-05-20 07:25 |
> Or should we be satisfied with the half-measure of including the qualname but not the module (at least for now)?
This is something I was wondering myself, too (also for other contexts). Let's take things one step at a time and limit ourselves just to __qualname__ in this issue. Including the module name can be discussed in a separate issue. (This question also comes up for the __repr__ of objects -- sometimes it includes the fully qualified name and sometimes it doesn't.)
For your last question, does this work?
>>> def foo(**kwargs): pass
...
>>> foo(**{1: 2})
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: keywords must be strings
(Also, the corrected link is here:
https://github.com/python/cpython/blob/master/Python/ceval.c#L4182 )
|
msg369423 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2020-05-20 07:34 |
Oh, I see now I was hitting a different line:
https://github.com/python/cpython/blob/master/Objects/call.c#L1009
Maybe my suggestion is enough to help you (I didn't really look closely at the code), or maybe you were already aware of that.
|
msg369424 - (view) |
Author: Dennis Sweeney (Dennis Sweeney) *  |
Date: 2020-05-20 07:35 |
I got this:
>>> class A:
... def f():
... pass
...
>>> A.f(1)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: A.f() takes 0 positional arguments but 1 was given
>>> A.f(**{1:2})
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: keywords must be strings
I think this is coming from https://github.com/python/cpython/blob/cd8295ff758891f21084a6a5ad3403d35dda38f7/Python/getargs.c#L1636.
|
msg369425 - (view) |
Author: Dennis Sweeney (Dennis Sweeney) *  |
Date: 2020-05-20 07:42 |
Never mind; I think you're right, and https://github.com/python/cpython/blob/master/Objects/call.c#L1009 is the line.
|
msg369426 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2020-05-20 07:42 |
Oh, that string is used in even more spots (sorry wasn't looking too closely the first time).
|
msg369477 - (view) |
Author: Dennis Sweeney (Dennis Sweeney) *  |
Date: 2020-05-20 21:29 |
I just ran the entire test suite with:
--- a/Python/ceval.c
+++ b/Python/ceval.c
@@ -4179,6 +4179,7 @@ _PyEval_EvalCode(PyThreadState *tstate,
Py_ssize_t j;
if (keyword == NULL || !PyUnicode_Check(keyword)) {
+ printf("THIS CODE WAS RUN!\n");
_PyErr_Format(tstate, PyExc_TypeError,
"%U() keywords must be strings",
qualname);
and the line was never printed. So maybe the test coverage (or removal?) should be a separate issue.
|
msg369492 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2020-05-21 05:15 |
> So maybe the test coverage (or removal?) should be a separate issue.
That sounds good. Want to file the issue?
|
msg369496 - (view) |
Author: Dennis Sweeney (Dennis Sweeney) *  |
Date: 2020-05-21 05:45 |
Sure -- I'll file the issue.
|
msg369499 - (view) |
Author: Dennis Sweeney (Dennis Sweeney) *  |
Date: 2020-05-21 06:10 |
https://bugs.python.org/issue40706
|
msg369645 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2020-05-22 20:40 |
New changeset b5cc2089cc354469f12eabc7ba54280e85fdd6dc by Dennis Sweeney in branch 'master':
bpo-40679: Use the function's qualname in certain TypeErrors (GH-20236)
https://github.com/python/cpython/commit/b5cc2089cc354469f12eabc7ba54280e85fdd6dc
|
msg369646 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2020-05-22 20:43 |
Thanks again, Dennis!
|
msg369653 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2020-05-22 21:27 |
> _PyObject_FunctionString as discussed here ( https://bugs.python.org/issue37645 ) returns a string that also includes the module name where applicable.
By the way, Dennis, regarding the above, one thing I noticed is that Python doesn't currently expose a convenient way to get the fully qualified name of a class (the "full name" as opposed to the qualified name). It might be worth exploring what that would involve. I think it would be useful, personally.
|
msg370658 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2020-06-03 12:20 |
> bpo-40679: Use the function's qualname in certain TypeErrors (GH-20236)
This change introduced a regression. See this bug in Cython:
https://github.com/cython/cython/issues/3641#issuecomment-638102096
I wrote a PR to use co->co_name if qualname is NULL (ex: when PyEval_EvalCodeEx() is called). I will post the PR once I finish to run the test suite locally ;-)
|
msg370664 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2020-06-03 12:44 |
Thanks a lot, Victor.
|
msg370706 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2020-06-04 13:19 |
New changeset 232dda6cbc10860328a83517a6e3ea238ff4147f by Victor Stinner in branch 'master':
bpo-40679: Fix _PyEval_EvalCode() crash if qualname is NULL (GH-20615)
https://github.com/python/cpython/commit/232dda6cbc10860328a83517a6e3ea238ff4147f
|
msg370707 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2020-06-04 13:20 |
> Thanks a lot, Victor.
You're welcome. Using the qualified name instead of the "short" name in error messages is nice enhancement!
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:31 | admin | set | github: 84856 |
2020-06-04 13:20:23 | vstinner | set | status: open -> closed resolution: fixed messages:
+ msg370707
stage: patch review -> resolved |
2020-06-04 13:19:08 | vstinner | set | messages:
+ msg370706 |
2020-06-03 15:47:39 | vstinner | set | pull_requests:
+ pull_request19841 |
2020-06-03 12:44:06 | chris.jerdonek | set | messages:
+ msg370664 |
2020-06-03 12:27:55 | vstinner | set | stage: resolved -> patch review pull_requests:
+ pull_request19834 |
2020-06-03 12:20:50 | vstinner | set | status: closed -> open
nosy:
+ vstinner messages:
+ msg370658
resolution: fixed -> (no value) |
2020-05-22 21:27:28 | chris.jerdonek | set | messages:
+ msg369653 |
2020-05-22 20:43:27 | chris.jerdonek | set | status: open -> closed resolution: fixed messages:
+ msg369646
stage: patch review -> resolved |
2020-05-22 20:40:25 | chris.jerdonek | set | messages:
+ msg369645 |
2020-05-21 12:22:17 | chris.jerdonek | set | components:
+ Interpreter Core versions:
+ Python 3.10, - Python 3.9 |
2020-05-21 06:10:32 | Dennis Sweeney | set | messages:
+ msg369499 |
2020-05-21 05:45:42 | Dennis Sweeney | set | messages:
+ msg369496 |
2020-05-21 05:15:31 | chris.jerdonek | set | messages:
+ msg369492 |
2020-05-20 21:29:25 | Dennis Sweeney | set | messages:
+ msg369477 |
2020-05-20 07:42:55 | chris.jerdonek | set | messages:
+ msg369426 |
2020-05-20 07:42:49 | Dennis Sweeney | set | messages:
+ msg369425 |
2020-05-20 07:35:48 | Dennis Sweeney | set | messages:
+ msg369424 |
2020-05-20 07:34:02 | chris.jerdonek | set | messages:
+ msg369423 |
2020-05-20 07:25:59 | chris.jerdonek | set | messages:
+ msg369422 |
2020-05-20 07:09:01 | Dennis Sweeney | set | messages:
+ msg369421 |
2020-05-19 22:59:51 | chris.jerdonek | set | messages:
+ msg369405 |
2020-05-19 19:46:47 | Dennis Sweeney | set | messages:
+ msg369382 |
2020-05-19 19:36:17 | Dennis Sweeney | set | keywords:
+ patch nosy:
+ Dennis Sweeney
pull_requests:
+ pull_request19523 stage: patch review |
2020-05-19 17:26:03 | xtreak | set | nosy:
+ xtreak
|
2020-05-19 07:57:29 | chris.jerdonek | create | |