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

Number of expected arguments in string methods TypeError messages are incorrect bug 1763 #163

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Hayderrrr
Copy link

Description of the problem :

The initial problem was that the number of args in the TypeError message was wrong.
Ex :

jython:
>>> ".".join("a","b")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: join() takes 2 arguments (2 given)

While in python it is:
>>> ".".join("a","b")
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
TypeError: join() takes exactly one argument (2 given)

While searching to solve this problem, I realized the issue extended to every methods coming from Python. (append for list, clear for dict, split for string for exemple...)

>>> list().append(1,2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: append() takes 2 arguments (2 given)

>>> dict().clear(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: clear() takes exactly one argument (1 given)

>>> str().split(1,2,3)
Function : split min args : 1 maxargs 3
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: split() takes 1-3 arguments (3 given)

Solution for this problem :

To create these error messages, the minargs and the maxargs of the original function are set in the info of the function.
When the TypeError is called, it uses minargs and maxargs stored in the info to build the message but their values are wrong because in their original description they take one more argument (self).
In order to have the right number to appear in the error message, we need to reduce the minargs and the maxargs by 1 at the creation of the function.

I made a simple test file to test some of the case that were wrong but the list of methods that were affected is too big (We need to complete with the test that are missing or find a better way to realize the test).

There are some builtins that still have wrong error message but the source of the problem is somewhere else (because even the number of arguments given is wrong).

🐛 Fix a problem where some methods have a bad error message because they have one more argument in their info for minArgs and maxArgs.
@jeff5
Copy link
Member

jeff5 commented Dec 7, 2019

Thanks for looking into this, and for the care in creating a test. The inconsistency in these messages has bugged me repeatedly, but I've never looked into it carefully, so this is welcome.

When a method is defined in Python, we produce essentially the same message as CPython, counting the target object (if there is one) as an argument. CPython is not consistent when it comes to built-in methods. It evidently does not count the target in:

>>> '*'.join("abc", 2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: join() takes exactly one argument (2 given)

The counting of the target as an argument (and only sometimes) is confusing, but unfortunately standard.

The difficulty I have with this solution is that where I see the PyBuiltinMethodNarrow used directly, the numbers passed in are the numbers (min/max) of arguments without the target. There is even the possibility of generating negative values, which have a special meaning in these fields. The patch strikes me as making a compensating error, rather correcting the real cause.

One example use can be found in JavaProxyMap where we dress Map as Python map. Consider:

>>> from java.util import HashMap
>>> m = HashMap()
>>> m.put('hello', 1)
>>> m.has_key('hello', 1)  # TypeError: has_key() takes exactly one argument (2 given)

Now this is correct (without the patch). Here we're wrong as you observe:

>>> {'bonjour':2}.has_key('bonjour', 1)  # TypeError: has_key() takes 2 arguments (2 given)

So some other use of PyBuiltinMethodNarrow calls it with the wrong arguments. I think the real bug may be lurking in the MethodExposer that processes our built-in types, but it is not as simple as removing the +1s from generateNamedConstructor.

@jeff5 jeff5 added the queried Review awaits proposer's response. label Feb 2, 2020
@jeff5
Copy link
Member

jeff5 commented Jun 4, 2020

Unfortunately a PR (unlike an issue) cannot be transferred to the new official home at https://github.com/jython/jython. Keeping this open in the frozen-mirror to allow for further consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
queried Review awaits proposer's response.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants