Add __repr__ support for pyjobject. If method defined on java class, …

[jsnps]

…it will be picked up for tp_repr of the python wrapper.

Hey, this is some suggestion for supporting custom repr implementations in jep according to the discussion in https://github.com/ninia/jep/issues/330 Please review carefully, especially with regards to memory management in C ;)

For the test, I found test_object.py to be a good place, as it is also testing hash and toString, but I'm not sure about the naming conventions here (might be only meant for the Object class). I needed two simple java test classes (implementing repr), but none of the existing java Test* classes sounded like the right place for them, so I added TestPyJObject.

@bsteffensmeier I'm not sure about performance, do you see this method as performance critical? I saw in jep 4.0 you are creating dedicated py types for all java classes https://github.com/ninia/jep/commit/151c748f6c3674bc1ef292156fa82224a208fa3c Would this be a better place?

[bsteffensmeier]

I'm not sure about performance, do you see this method as performance critical? I can't think of any reason someone would be calling repr in performance sensitive code. One of the downsides of being an intermediary library is we never know for sure what anyone is doing with it but I would assume we are pretty safe here. One thought I have that could improve the performance with a minor change would be if we check self->attrs to see if repr is present before getting the JNI method. I have not measured it but I suspect a single dict lookup would be faster than the JNI method lookup, especially since JNI encourages you to cache methodIDs so it appears that lookup is not heavily optimized. We would still need to do the JNI method lookup for cases where that attribute exists but that would be a much less common case and we could optimize further if it becomes a problem.

I saw in jep 4.0 you are creating dedicated py types for all java classes 151c748 Would this be a better place?

Before this PR can be merged it will have to target dev_4.0. We only merge new code into dev_X.Y branches and master is updated when we release. See the contribution guidelines for a summary of our process.

I think this code will continue to work as is on dev_4.0. Theoretically it would be better if we could leave the default tp_repr implementation on PyObject and check if a class defines repr when we make the type and only assign an alternative tp_repr for only the classes that define it. We may even be able to make a PyJMethod wrapping the repr java method and assign it to repr in the dict during type creation and then we wouldn't need any new code for tp_repr if python can just hook directly into the PyJMethod. If you don't want to figure that out then I would be fine with just defining a common tp_repr on pyjobject like you have and we can always change it in a later release if necessary.

[jsnps]

Hey, I think your suggestions make totally sense and I will try to incorporate these. :) Thanks for the feedback!

[bsteffensmeier]

@jsnps Are you good if I merge this into dev_4.0? I would like to do a release of jep 4.0 in the next couple weeks and was hoping we can get this in. I am fine merging it as is, although I might investigate if we can check attr for repr since a dict lookup seems simpler than JNI method lookup. Since this works I would be willing to do that investigation after a merge.

[jsnps]

@bsteffensmeier sorry for the inactivity - almost forgot about it due to other tasks ;) I'm fine with getting this merged and picking this up later on again, but I also did some experiment according to your suggestion just now. Inside the pyjtype_get_new method, I'm checking for a repr method, create a pyjmethod and add it to the type dict. I reverted the repr override in pyjobject. My test seems to pick up the added method on the type and invokes it BUT the pyargs are empty inside pyjmethod_call leading to:
Traceback (most recent call last): File "/localdev/janiks/jep_contributions/tmp/jep/src/test/python/test_object.py", line 18, in test_repr self.assertEquals(repr(TestPyJObject.ReprClass()), "ReprClass") RuntimeError: Invalid number of arguments: 0, expected 1.

Python seems to handle type and instance methods differently (not passing self), the comment in pyjtype says: "In the

• future consider placing pyjfields or pyjmethods in this dict
• to simplify the structure of each object. "

Do you have some suggestion, how we could get the pyjmethod to work on the pyjtype for java instance methods (non static methods)? Or is this the wrong place and we should add this method to another dict during pyjobject_init? It feels right, to introspect this during the type creation, as this will not change between different instances.

Edit:

Interestingly python seems to pass self to type methods, when invoked on an instance of that type:

>>> def foo():
...   print('hello')
... 
>>> t = type("mytype", (), {'x':foo})
<class '__main__.mytype'>
>>> t().x
<bound method foo of <__main__.mytype object at 0x7ffff7f030a0>>
>>> t().x()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: foo() takes 0 positional arguments but 1 was given
>>> def foo(self):
...   print(self)
... 
>>> t = type("mytype", (), {'x':foo})
>>> t().x()
<__main__.mytype object at 0x7ffff7f030a0>

[bsteffensmeier]

So normal python methods are part of the type dict but when you get them through the internal get_attr you create a bound instance of the method that is tied to a particular instance of the type and has a reference to self. I have branches where I have tried to make it so jep can store all pyjmethods on the type object but it gets very complicated and it always ends up being too big of a change to merge into jep. I have an old branch where I had to make pyjmethod a descriptor so that when you access the object it can bind the method by creating a PyMethod that contains the pyjmethod. https://github.com/bsteffensmeier/jep/blob/pyjtype2/src/jep/pyjmethod.c#L773

So while it maybe possible to tie __repr__ to the type I don't think it is going to make it into 4.0

[jsnps]

ok thanks for the explanation. Then I'm fine to get it in like this for now and I will revisit this, once you added the descriptor support on pyjmethod.