Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

show class name in method invocation TypeError #84856

Closed
cjerdonek opened this issue May 19, 2020 · 20 comments
Closed

show class name in method invocation TypeError #84856

cjerdonek opened this issue May 19, 2020 · 20 comments
Labels
3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@cjerdonek
Copy link
Member

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
    Copy link
    Member Author

    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
    

    @cjerdonek cjerdonek added 3.9 only security fixes type-feature A feature request or enhancement labels May 19, 2020
    @sweeneyde
    Copy link
    Member

    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
    Copy link
    Member Author

    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
    Copy link
    Member

    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
    Copy link
    Member Author

    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
    Copy link
    Member Author

    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
    Copy link
    Member

    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

    "keywords must be strings");
    .

    @sweeneyde
    Copy link
    Member

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

    @cjerdonek
    Copy link
    Member Author

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

    @sweeneyde
    Copy link
    Member

    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
    Copy link
    Member Author

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

    That sounds good. Want to file the issue?

    @sweeneyde
    Copy link
    Member

    Sure -- I'll file the issue.

    @sweeneyde
    Copy link
    Member

    @cjerdonek cjerdonek added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.10 only security fixes and removed 3.9 only security fixes labels May 21, 2020
    @cjerdonek
    Copy link
    Member Author

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

    @cjerdonek
    Copy link
    Member Author

    Thanks again, Dennis!

    @cjerdonek
    Copy link
    Member Author

    _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
    Copy link
    Member

    vstinner commented Jun 3, 2020

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

    @vstinner vstinner reopened this Jun 3, 2020
    @vstinner vstinner reopened this Jun 3, 2020
    @cjerdonek
    Copy link
    Member Author

    Thanks a lot, Victor.

    @vstinner
    Copy link
    Member

    vstinner commented Jun 4, 2020

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

    @vstinner
    Copy link
    Member

    vstinner commented Jun 4, 2020

    Thanks a lot, Victor.

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

    @vstinner vstinner closed this as completed Jun 4, 2020
    @vstinner vstinner closed this as completed Jun 4, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants