Me trying to explain the problem behind #ReflectiveAccessToNonExportedTypes

[David M. Lloyd]

On 07/14/2016 10:06 AM, Remi Forax wrote:
> Let me try to explain again the problem and the possible solution.
>
> Let say i have two modules, module1 and module2.
> In module1, I have two packages module1.exported and module1.nonexported,
> so module1 has this module-info.java
>
>    module module1 {
>      exports module1.exported;
>    }
>
> in module1.nonexported, I have a class A that's implement an interface I defined like this:
>    package module1.nonexported;
>
>    import module1.exported.I;
>
>    public class A implements I {
>      @Override
>      public void foo() {
>        System.out.println("foo");
>      }
>    }
>
> I is defined in module1.exported like this:
>    package module1.exported;
>
>    import module1.nonexported.A;
>
>    public interface I {
>      public void foo();
>
>      static I instance() {
>        return new A();
>      }
>    }
>
> Now, in module2, i want to use I so module2 requires module1, like this:
>    module module2 {
>      requires module1;
>    }
>
> and I have a main defined in the package modul2.main, like that:
>    package module2.main;
>
> import java.lang.reflect.Method;
>
> import module1.exported.I;
>
> public class Main {
>    public static void main(String[] args) throws ReflectiveOperationException {
>      System.out.println(Class.forName("module1.nonexported.A"));
>
>      I i = I.instance();
>      Method m = i.getClass().getMethod("foo");
>      m.invoke(i);
>    }
> }
>
> If i run this code in with the two modules in the classpath,
> everything work and the code prints
>    class module1.nonexported.A
>    foo
>
> Now, if if the two modules are in the modulepath,
> Class.forName works but m.invoke() throws an IllegalAccessException because A is in a non exported package
>    java.lang.IllegalAccessException: class module2.main.Main (in module module2) cannot access class module1.nonexported.A (in module module1) because module module1 does not export module1.nonexported to module module2
>
> If i try with m.setAccessible(true), before m.invoke(), setAccessible throws an exception InaccessibleObjectException
>    java.lang.reflect.InaccessibleObjectException: Unable to make member of class module1.nonexported.A accessible:  module module1 does not export module1.nonexported to module module2
>
> A way to solve this issue, is to use the interface I to call foo instead of using the class A,
>    Method m = I.class.getMethod("foo");
>    m.invoke(i);
>
> in that case, everything is Ok, because i don't try to use the method foo of A anymore.
>
> What Mark as proposed is to introduce a new "export dynamic" semantics which doesn't allow the package to be visible at compile time but to be visible at runtime.
> This solve the issue by supposing that everyone will write export dynamic on every non exported package to be backward compatible with the Java 8 behavior.
>
> While i agree that "export dynamic" is a semantics that jigsaw should provide, i disagree with the syntax "export dynamic" because for me, the a non exported package should have this semantics in order to be backward compatible with existing code.
> So we should have 3 way to export or not a package,
>    1. don't export at compile time, don't export at runtime
>    2. don't export at compile time, export at runtime
>    3. export at compile time, export at runtime.
>
> 2 should be the default, 1 should be use by the package of the JDK (or any other libs) that want strong security, 3 is for the package that defines API (that will be maintained forever).

This is a good summary and I think matches my understanding.  But the 
basic problem is that if everyone is doing this "export dynamic" then 
the safety proposed by restricting "public" is a false promise: we are 
providing the feature only to take it away again, resulting in the exact 
same situation we have today, but with no alternatives.  If we have to 
do this then I think the feature itself needs to be re-examined.

What I was suggesting was that we re-separate the notion of exports from 
accessibility altogether, so you again have three different situations 
rather than two:

A. Compile time
B. Class link at run time
C. Reflection at run time

This changes the basic "export" back to mean "make available for 
linkage", with no bearing on accessibility.  Then you have (at minimum) 
the following ways to export (or not) a package:

1. No export at compile time, no export but reflection-only at run time 
(but only for public types & members)
2. Yes export at compile time, yes export at run time (but only for 
public types & members)
3. Yes export at compile time and run time, including specific grants to 
access package-private members from targeted modules and/or packages

Note: We could add a 1½ which is an export at run time but not compile 
time if the EG agrees; I don't think that is particularly important 
though as build tools can accomplish this easily enough.  I don't want 
to derail the core point of the discussion on this topic, so I'm leaving 
it aside for now.

This way the Java 8 behavior is retained with no "export dynamic" 
anywhere in sight.  Rather you'd have something like "export private", 
which you'd only grant for those special cases where a package must be 
shared to a specific friend.  Things not intended to be shared can be 
safely migrated to the new friend package feature.  No new access limits 
are introduced - instead old ones are enhanced in what I think is a 
simple and intuitive way.  All the compatibility concerns go away, but 
we can still use the mechanism to seal up the problems in the JDK with 
shared code and things which should not be public.

We should retain setAccessible() for now for compatibility.  We could 
also introduce a "--safe-mode" flag which disables setAccessible() 
*across the board* (that is, not just for modular code) without 
controversy, because all those public members will remain accessible 
without "help", plus a way is provided to allow access to be volunteered 
from one package to another (along much the same philosophical lines as 
MethodHandles.Lookup IMO).

The platform EG can then later decide if this safer mode is good for the 
platform, and if so, could move to making this the default and possibly 
adding an "--unsafe-mode" for a limited time.

-- 
- DML