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

Instance methods are misreporting the number of arguments #46768

Closed
abalkin opened this issue Mar 30, 2008 · 14 comments
Closed

Instance methods are misreporting the number of arguments #46768

abalkin opened this issue Mar 30, 2008 · 14 comments
Labels
type-feature A feature request or enhancement

Comments

@abalkin
Copy link
Member

abalkin commented Mar 30, 2008

BPO 2516
Nosy @rhettinger, @abalkin, @giampaolo, @nedbat, @benjaminp, @merwok, @bitdancer, @iritkatriel
Files
  • issue2516poc.diff
  • issue2516poc1.diff: Patch against revision 62106
  • 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 2021-11-23.17:07:03.600>
    created_at = <Date 2008-03-30.22:23:34.397>
    labels = ['type-feature']
    title = 'Instance methods are misreporting the number of arguments'
    updated_at = <Date 2021-11-23.17:07:03.598>
    user = 'https://github.com/abalkin'

    bugs.python.org fields:

    activity = <Date 2021-11-23.17:07:03.598>
    actor = 'iritkatriel'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-11-23.17:07:03.600>
    closer = 'iritkatriel'
    components = []
    creation = <Date 2008-03-30.22:23:34.397>
    creator = 'belopolsky'
    dependencies = []
    files = ['9908', '9933']
    hgrepos = []
    issue_num = 2516
    keywords = ['patch']
    message_count = 14.0
    messages = ['64767', '64772', '64773', '64902', '64903', '64910', '64932', '108252', '108253', '108616', '108620', '108623', '108632', '406863']
    nosy_count = 11.0
    nosy_names = ['rhettinger', 'belopolsky', 'giampaolo.rodola', 'nedbat', 'benjamin.peterson', 'rbp', 'eric.araujo', 'r.david.murray', 'gungorbasa', 'tshepang', 'iritkatriel']
    pr_nums = []
    priority = 'low'
    resolution = 'out of date'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue2516'
    versions = ['Python 3.4']

    @abalkin
    Copy link
    Member Author

    abalkin commented Mar 30, 2008

    Opening a new issue per Raymond's request at msg64764:

    """
    It would be *much* more useful to direct effort improving the mis-
    reporting of the number of arguments given versus those required for
    instance methods:
      >>> a.f(1, 2)
      TypeError: f() takes exactly 1 argument (3 given)
    """

    I would suggest that this misreporting may be dear to old-beards
    reminding the time when there was not as much difference between methods
    and functions as there is now.

    It does not seem to be that hard to change the diagnostic to

      >>> a.f(1, 2)
      TypeError: f() takes no arguments (2 given)

    but the novice users would much rather see "a.f() takes no arguments (2
    given)." The later is unfortunately not possible.

    Raymond, what would you say to "<class 'A' instance>.f() takes no
    arguments (2 given)" diagnostic?

    @abalkin abalkin added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Mar 30, 2008
    @abalkin
    Copy link
    Member Author

    abalkin commented Mar 31, 2008

    Attached patch (issue2516poc.diff) presents proof-of-concept code which
    changes the problematic reporting as follows:

    >>> a.f(1,2)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: <A object>.f() takes exactly 0 arguments (2 given)

    More effort is needed to make this patch ready: support default/keyword
    arguments, respect English grammar in 1-argument case, etc. Before I do
    that, however, I would like to hear that this is a worthwhile fix and
    that I've chosen the right place in the code to implement it.

    @benjaminp
    Copy link
    Contributor

    You have +1 from me to continue developing this patch.

    @abalkin
    Copy link
    Member Author

    abalkin commented Apr 3, 2008

    I am uploading another work in progress patch because the problem proved
    to be more difficult than I thought in the beginning. The new patch
    addresses two issues.

    1. a.f(..) -> A.f(a, ..) transformation is performed is several places
      in the current code. I have streamlined CALL_* opcodes processing to
      make all calls go through PyObject_Call. This eliminated some
      optimizations that can be put back in once the general framework is
      accepted.

    2. The only solution I could find to fixing reporting from
      instancemethod_call was to add expected number of arguments information
      to the exception raised in ceval and use it to reraise with a proper
      message. Obviously, putting the necessary info into the argument tuple
      is a hack and prevents reproducing original error messages from the
      regular function calls. I see two alternatives: (a) parsing the error
      string to extract the needed information (feels like even a bigger
      hack), and (b) creating an ArgumentError subclass of TypeError and have
      its instances store needed information in additional attributes (talking
      about a canon and a mosquito!)

    3. If a solution that requires extra information in an exception is
      accepted, PyArg_Parse* functions should be similarly modified to add the
      extra info when raising an error.

    Finally, let's revisit whether this mosquito deserves to die. After
    all, anyone looking at method definition sees the self argument, so
    saying that a.f(1, 2) provides 3 arguments to f() is not such a stretch
    of the truth.

    It is also possible that I simply fail to see a simpler solution. It
    this case, please enlighten me!

    PS: The patch breaks cProfile and several other tests that check error
    messages. I am making it available for discussion only.

    @benjaminp
    Copy link
    Contributor

    Guido, what's your opinion on this? Is this a bug, and should it be fixed?

    @rhettinger
    Copy link
    Contributor

    It's definitely a bug, but I think the reason it has been around so
    long is that no-one has offerred a clean solution.

    I was hoping for something along the lines of functions raising an
    ArgumentError (a new subclass of TypeError) that could be trapped by
    the __call__ slot for bound methods and then reraised with a new
    argument count. The key is to find a *very* lightweight and minimal
    solution; otherwise, we should just live with it for another 18 years :-
    )

    @abalkin
    Copy link
    Member Author

    abalkin commented Apr 4, 2008

    On Thu, Apr 3, 2008 at 9:39 PM, Raymond Hettinger
    <report@bugs.python.org> wrote:

    It's definitely a bug

    What would you say to the following:

    def f(x):
        pass
    class X:
        xf = f
    
    x = X()
    x.xf(1,2)

    --> TypeError: f() takes exactly 1 argument (3 given)

    Is this correct?

    ..

    I was hoping for something along the lines of functions raising an
    ArgumentError (a new subclass of TypeError) that could be trapped by
    the __call__ slot for bound methods and then reraised with a new
    argument count.

    This would be my first choice for a clean solution as well. Since it
    will require a change to the exception hierarchy, should we discuss a
    modification to PEP-348 on python-dev? I would rather finish the
    patch first and then make a concrete proposal.

    The key is to find a *very* lightweight and minimal solution;

    .. according to what metric? Are you talking about the amount of code
    that needs to be changed, the number of API changes or run-time
    overhead? I don't think run-time overhead is an issue: argument
    errors are rarely used for flow control because it is hard to predict
    what a failed attempt to call a function would do.

    @gungorbasa gungorbasa mannequin added build The build process and cross-build and removed interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Aug 21, 2009
    @benjaminp benjaminp added type-feature A feature request or enhancement and removed build The build process and cross-build labels Aug 21, 2009
    @abalkin abalkin self-assigned this Jun 6, 2010
    @giampaolo
    Copy link
    Contributor

    The same subject on python-dev raised last month received positive feedbacks:
    http://mail.python.org/pipermail/python-dev/2010-May/100197.html

    @bitdancer
    Copy link
    Member

    All that is needed now is a working patch... :)

    @abalkin
    Copy link
    Member Author

    abalkin commented Jun 25, 2010

    Here is a similar issue which may be easier to fix:

    >>> def f(a, b=None, *, c=None, d=None):
    ...    pass
    
    
    >>> f(1,2,3)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: f() takes at most 4 arguments (3 given)

    Should be "f() takes at most 2 positional arguments (3 given)"

    @abalkin abalkin removed their assignment Jun 25, 2010
    @benjaminp
    Copy link
    Contributor

    Fixed in r82220.

    @giampaolo
    Copy link
    Contributor

    Maybe I'm misunderstanding what issue we're talking about here, but wasn't this supposed to be fixed?

    giampaolo@ubuntu:~/svn/python-3.2$ python3.2
    Python 3.2a0 (py3k:82220M, Jun 25 2010, 21:38:56) 
    [GCC 4.4.3] on linux2
    Type "help", "copyright", "credits" or "license" for more information.
    >>> class A:
    ...     def foo(self, x):
    ...             pass
    ... 
    >>> A().foo()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: foo() takes exactly 2 arguments (1 given)
    >>>

    @bitdancer
    Copy link
    Member

    I presume Benjamin meant he fixed the special case Alexander reported.

    @iritkatriel
    Copy link
    Member

    This seems to have been fixed by now, on 3.11 I get this:

    >>> class A:
    ...     def foo(self, x): pass
    ... 
    >>> A().foo()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: A.foo() missing 1 required positional argument: 'x'

    @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
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants