Issue2662

I'm spinning this off from #2656 to cover the subset of those Java 9 problems with a common cause in PyReflectedFunction.

The codecs module provides an easy way to reproduce this (remote is just a batch file that runs jython withthe command line option for remote debugging):

PS jython-jvm9> .\remote -J--illegal-access=deny
Listening for transport dt_socket at address: 8000
Jython 2.7.2a1+ (default:d74f8c2cd56f, Mar 31 2018, 23:07:07)
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java9.0.1
Type "help", "copyright", "credits" or "license" for more information.
>>> import codecs
>>> enc = codecs.getencoder('gbk')
>>> enc(u"hello")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\Jeff\Documents\Eclipse-O\jython-jvm9\dist\Lib\encodings\_java.py", line 78, in encode
    encoder = Charset.forName(self.encoding).newEncoder()
        at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:361)
        at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:589)
        at java.base/java.lang.reflect.Method.invoke(Method.java:556)
java.lang.IllegalAccessException: java.lang.IllegalAccessException: class org.python.core.PyReflectedFunction cannot access class sun.nio.cs.GBK (in module java.base) because module java.base does not export sun.nio.cs to unnamed module @7ceb3185

For me the worrying thing is that newEncoder() is a *public* abstract method of CharSet, and yet we get this message on the grounds that the *implementation* sun.nio.cs.GBK is not exported. Obviously a factory that hands out instances of a private class that implements a public interface is a common pattern, and had better work in Jython.

My first idea is that we need to handle objects not according to their actual class, but according to some super class or interface that we are allowed to access.

I tried an isolated experiment with reflection on the same method. It appears that if the method we were trying to call was:
  public abstract java.nio.charset.CharsetEncoder java.nio.charset.Charset.newEncoder()
and not:
  public java.nio.charset.CharsetEncoder sun.nio.cs.GBK.newEncoder()
then we'd be ok. This even though the second is the implementation of the first. Demonstration attached.

In compiled Java, the object that is the target of the call is declared as a Charset, and so the compiler emits an invokevirtual of Charset.newEncoder whereas in the reflective call we find the method in the actual class of the object.

In asking how to deal with this in Jython, where there are only actual types, not declared types, I believe we are really considering the semantics of attribute access. In this case, we would like to call the abstract method. This suggests that when constructing the PyJavaType, we should insert a Method object found by chasing the Java or Python MRO, not the one we find in the class itself. But is this always right?

As a counter-example, I think it would be possible to arrange that the base class be inaccessible while a derived class is not. And checking accessibility from PyJavaType does not necessarily reflect accessibility to the user application.

I have a couple of ideas that involve going up the inheritance chain (MRO) from the actual class of the target object to find a method with the same signature that is amenable to a reflective invocation at run-time. THese botyh depend on the behaviour that Method.invoke promises: that where the target method is overidden in the actual class of the target object, the overriding definition is used.

In one idea we go up the MRO until we find the "most senior" definition of the required method, which is assumed to be the public API.

In the second, we go up until we find one where trySetAccessible succeeds.

See attached toy. I prefer the first of these because it works in all versions of Java. I think the place to do it is where the PyReflectedFunction is being created, not during the call itself. However, we would do this for all Java classes, and it so change the contents of the type object significantly. I'm unclear about what malign consequences may follow. A good consequence would be fewer PyReflectedFunction/Method objects. I haven't tried this yet, but the effect can be simulated  like this:

PS jython-jvm9> dist\bin\jython -J--illegal-access=deny
...
>>> import java.nio.charset.Charset as Charset
>>> import java.nio.CharBuffer as CharBuffer
>>> gbk = Charset.forName('gbk')
>>> type(gbk)
<type 'sun.nio.cs.GBK'>
>>> enc = gbk.newEncoder()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
        at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:361)
        at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:589)
        at java.base/java.lang.reflect.Method.invoke(Method.java:556)
java.lang.IllegalAccessException: java.lang.IllegalAccessException: class org.python.core.PyReflectedFunction cannot access class sun.nio.cs.GBK (in module java.base) because module java.base does not export sun.nio.cs to unnamed module @2a7ed1f

But if we get the (abstract) Charset.newEncoder method, that works:

>>> ne = Charset.__dict__['newEncoder']
>>> ne
<java function newEncoder 0x3>
>>> enc = ne(gbk)
>>> enc.encode(CharBuffer.wrap(u"hello"))
java.nio.HeapByteBuffer[pos=0 lim=5 cap=10]

In fact we do something similar when we encounter a non-public class here:
https://hg.python.org/jython/file/d74f8c2cd56f/src/org/python/core/PyJavaType.java#l383

However, the role that "public" plays here needs to be replaced by "accessible", in the sense of modular accessibility from the unnamed module. I wonder if that can be done without invoking Java 9 specifics?

There's a bug in that if-statement too. The name field for core classes, has already been changed from the Java class name at:
https://hg.python.org/jython/file/d74f8c2cd56f/src/org/python/core/PyJavaType.java#l324
and so the prefix tested is no longer there, which means we perfrom this unnecessarily for core classes. That should be fixed, but for now it gives me lots of opportunity to see it in action :/

Also, the logic of this seems odd:
https://hg.python.org/jython/file/d74f8c2cd56f/src/org/python/core/PyJavaType.java#l957
We check to see if the class is non-public, after the merge?

I've dealt with most of the warnings about PyReflectedFunction by defining a test of accessibility that takes into account whether the containing package is exported to the unnamed module. Of course, the apparatus for that doesn't exist until Java 9, not even the class Module, but I access it reflectively so I can fail safely on Java 7.

I include the test here where currently we test for "public":
https://hg.python.org/jython/file/tip/src/org/python/core/PyJavaType.java#l383

This fixes it in all cases where the inaccessible class has a parent that is accessible (exported), but it fails for the ISO2022_JP_2 codec where the parent is ISO2022_JP which is still not accessible. I'll see if I simply need to include the same test here:
https://hg.python.org/jython/file/tip/src/org/python/core/PyJavaType.java#l1086
but what bugs me about that is that the test comes after we already processed the implementation of the method as found in the immediate parent. Surely we ought to go up the tree until we find a public API of which this is merely the implementation?

There are a lot of things about the way we build a PyReflectedFunction that do not seem quite right to me. Done right, I sense we might eliminate the need to disrespect Java accessibility in almost every case.

My shot at this is now in the repo at: https://hg.python.org/jython/rev/78b574d6a6f7

It passes tests on Java 7 cleanly. On Java 9 I see 12 test failures but we had those before. I believe I have eliminated the warnings that stem from PyReflectedFunction.

Also, I claim the first overt use of a MethodHandle in Jython. I *think* I had a good reason to use it :)

I remain a little dubious about the logic of PyJavaType.init, as it seems to me that the addition of handleSuperMethodArgCollisions as an afterthought sits awkwardly. What it does (and I've extended) is necessary, but maybe the whole of our field and method processing should work that way? For example, I think it would not deal as it is with a private class implementing List directly because handleSuperMethodArgCollisions would not be called for it. Fortunately, in all cases I've come across, the implementation extends AbstactList, for which handleSuperMethodArgCollisions will have been called. Similarly Set, Map, etc., but I think we could find/make a failing example. However, that could be an enhancement after 2.7.2 .

LGTM, works as expected. Simple and elegant change now that it has been worked out :)

I'm hoping this possibly related problem is a matter of updating our jar dependency, but per the build.xml JNR Posix is pretty current (almost 1 year old at 3.0.41 vs latest of 3.0.44):

     [exec] WARNING: An illegal reflective access operation has occurred
     [exec] WARNING: Illegal reflective access by jnr.posix.JavaLibCHelper (file:/Users/jim.baker/jythondev/jython/dist/javalib/jnr-posix-3.0.41.jar) to method sun.nio.ch.SelChImpl.getFD()
     [exec] WARNING: Please consider reporting this to the maintainers of jnr.posix.JavaLibCHelper

re private class implementing List directly: this does seem like a post 2.7.2 change, if we even do that. There are good reasons to access private methods/fields, as we know in the Jython runtime itself. But this private access should be done directly with the class in question, not as an extending class written in Python. For those circumstances, a composition pattern approach would IMHO be best.

@Jim: That isn't (I think) what I'm getting at. I was thinking of private classes implemented in Java. (I suppose it might happen via Python.)

When a class is detected to be private (or module-inaccessible), PyJavaType.init calls handleSuperMethodArgCollisions to expose equivalent methods from ancestors and interfaces. *And then it returns.* We do not execute the subsequent parts of init, such as those that give special treatment to classes implementing collection interfaces or Comparable.

In cases I've come across in testing, this is ok, because the private class extends a public class (like AbstractList) that has already had this treatment, and the special methods will be found along the MRO. But it isn't guaranteed there there be such an ancestor, if I create a class that implements Comparable directly, say.

My preferred answer to this would be that we don't have exceptional processing for private classes up front, but that the main-line of init should take accessibility into account in its fine-grain logic. (Simply removing the return doesn't work.)

That wouldn't prevent access to private methods, or private but exported classes, if the option is set to allow it. Accessing private objects is a sign something is wrong IMO, but it may be something outside your control for which that is the work-around of last resort.