Issue2516
Created on 2008-03-30 22:23 by belopolsky, last changed 2008-04-04 14:54 by belopolsky.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | Remove |
| issue2516poc.diff | belopolsky, 2008-03-31 00:37 | |||
| issue2516poc1.diff | belopolsky, 2008-04-03 19:46 | Patch against revision 62106 | ||
| Messages | |||
|---|---|---|---|
| msg64767 (view) | Author: Alexander Belopolsky (belopolsky) | Date: 2008-03-30 22:23 | |
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? |
|||
| msg64772 (view) | Author: Alexander Belopolsky (belopolsky) | Date: 2008-03-31 00:37 | |
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. |
|||
| msg64773 (view) | Author: Benjamin Peterson (benjamin.peterson) | Date: 2008-03-31 01:13 | |
You have +1 from me to continue developing this patch. |
|||
| msg64902 (view) | Author: Alexander Belopolsky (belopolsky) | Date: 2008-04-03 19:46 | |
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. |
|||
| msg64903 (view) | Author: Benjamin Peterson (benjamin.peterson) | Date: 2008-04-03 20:04 | |
Guido, what's your opinion on this? Is this a bug, and should it be fixed? |
|||
| msg64910 (view) | Author: Raymond Hettinger (rhettinger) | Date: 2008-04-04 01:39 | |
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 :- ) |
|||
| msg64932 (view) | Author: Alexander Belopolsky (belopolsky) | Date: 2008-04-04 14:54 | |
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. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2008-04-04 14:54:54 | belopolsky | set | messages: + msg64932 |
| 2008-04-04 01:39:57 | rhettinger | set | messages: + msg64910 |
| 2008-04-03 20:04:23 | benjamin.peterson | set | nosy:
+ gvanrossum messages: + msg64903 |
| 2008-04-03 19:46:47 | belopolsky | set | files:
+ issue2516poc1.diff messages: + msg64902 |
| 2008-03-31 01:13:59 | benjamin.peterson | set | priority: low nosy: + benjamin.peterson messages: + msg64773 |
| 2008-03-31 00:37:27 | belopolsky | set | files:
+ issue2516poc.diff keywords: + patch messages: + msg64772 |
| 2008-03-30 23:26:41 | nedbat | set | nosy: + nedbat |
| 2008-03-30 22:23:34 | belopolsky | create | |