show class name in method invocation TypeError

[cjerdonek]

BPO	40679
Nosy	@vstinner, @cjerdonek, @tirkarthi, @sweeneyde
PRs	bpo-40679: use a function's qualname in error messages #20236 bpo-40679: Fix _PyEval_EvalCode() if qualname is NULL #20606 bpo-40679: Fix _PyEval_EvalCode() crash if qualname is NULL #20615

^{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 = None
closed_at = <Date 2020-06-04.13:20:23.988>
created_at = <Date 2020-05-19.07:57:29.319>
labels = ['interpreter-core', 'type-feature', '3.10']
title = 'show class name in method invocation TypeError'
updated_at = <Date 2020-06-04.13:20:23.987>
user = 'https://github.com/cjerdonek'

bugs.python.org fields:

activity = <Date 2020-06-04.13:20:23.987>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-06-04.13:20:23.988>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2020-05-19.07:57:29.319>
creator = 'chris.jerdonek'
dependencies = []
files = []
hgrepos = []
issue_num = 40679
keywords = ['patch']
message_count = 20.0
messages = ['369324', '369382', '369405', '369421', '369422', '369423', '369424', '369425', '369426', '369477', '369492', '369496', '369499', '369645', '369646', '369653', '370658', '370664', '370706', '370707']
nosy_count = 4.0
nosy_names = ['vstinner', 'chris.jerdonek', 'xtreak', 'Dennis Sweeney']
pr_nums = ['20236', '20606', '20615']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue40679'
versions = ['Python 3.10']

[cjerdonek]

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

[sweeneyde]

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?

[cjerdonek]

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

[sweeneyde]

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.

[cjerdonek]

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 )

[cjerdonek]

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.

[sweeneyde]

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

cpython/Python/getargs.c

Line 1636 in cd8295f

"keywords must be strings");

.

[sweeneyde]

Never mind; I think you're right, and https://github.com/python/cpython/blob/master/Objects/call.c#L1009 is the line.

[cjerdonek]

Oh, that string is used in even more spots (sorry wasn't looking too closely the first time).

[sweeneyde]

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.

[cjerdonek]

So maybe the test coverage (or removal?) should be a separate issue.

That sounds good. Want to file the issue?

[sweeneyde]

Sure -- I'll file the issue.

[sweeneyde]

https://bugs.python.org/issue40706

[cjerdonek]

New changeset b5cc208 by Dennis Sweeney in branch 'master':
bpo-40679: Use the function's qualname in certain TypeErrors (GH-20236)
b5cc208

[cjerdonek]

Thanks again, Dennis!

[cjerdonek]

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

[vstinner]

bpo-40679: Use the function's qualname in certain TypeErrors (GH-20236)

This change introduced a regression. See this bug in Cython:
cython/cython#3641 (comment)

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

[cjerdonek]

Thanks a lot, Victor.

[vstinner]

New changeset 232dda6 by Victor Stinner in branch 'master':
bpo-40679: Fix _PyEval_EvalCode() crash if qualname is NULL (GH-20615)
232dda6

[vstinner]

Thanks a lot, Victor.

You're welcome. Using the qualified name instead of the "short" name in error messages is nice enhancement!