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

Massage implementation of structural calls to avoid: "WARNING: Illegal reflective access by scala.reflect.package$ " #11368

Open
retronym opened this Issue Nov 2, 2018 · 2 comments

Comments

Projects
None yet
3 participants
@retronym
Copy link
Member

retronym commented Nov 2, 2018

WARNING: Illegal reflective access by scala.reflect.package$ (file:/Users/andrew/workspace/scala/build/quick/classes/library/) to method java.lang.Object.clone()

We need to tweak the desugaring of structural calls a little bit to avoid this.

Instead of:

  /** Make a java reflection object accessible, if it is not already
   *  and it is possible to do so. If a SecurityException is thrown in the
   *  attempt, it is caught and discarded.
   */
  def ensureAccessible[T <: jAccessibleObject](m: T): T = {
    if (!m.isAccessible) {
      try m setAccessible true
      catch { case _: SecurityException => } // does nothing
    }
    m
  }

Which is called from, e.g.

scala> class Reflect { def foo = ""; def bar = { this.asInstanceOf[{ def foo: String }].foo }}
defined class Reflect

Which compiles to:

scala> :javap -c Reflect#bar
  public java.lang.String bar();
    Code:
       0: aload_0
       1: astore_1
       2: aload_1
       3: invokevirtual #82                 // Method java/lang/Object.getClass:()Ljava/lang/Class;
       6: invokestatic  #84                 // Method reflMethod$Method1:(Ljava/lang/Class;)Ljava/lang/reflect/Method;
       9: aload_1
      10: iconst_0
      11: anewarray     #4                  // class java/lang/Object
      14: invokevirtual #88                 // Method java/lang/reflect/Method.invoke:(Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;
...

scala> :javap -c Reflect#reflMethod$Method1
  public static java.lang.reflect.Method reflMethod$Method1(java.lang.Class);
    Code:
       0: invokedynamic #34,  0             // InvokeDynamic #0:apply:()Lscala/runtime/StructuralCallSite;
       5: astore_1
       6: aload_1
       7: aload_0
       8: invokevirtual #37                 // Method scala/runtime/StructuralCallSite.find:(Ljava/lang/Class;)Ljava/lang/reflect/Method;
      11: astore_2
      12: aload_2
      13: ifnull        18
      16: aload_2
      17: areturn
      18: getstatic     #43                 // Field scala/runtime/ScalaRunTime$.MODULE$:Lscala/runtime/ScalaRunTime$;
      21: aload_0
      22: ldc           #45                 // String foo
      24: aload_1
      25: invokevirtual #49                 // Method scala/runtime/StructuralCallSite.parameterTypes:()[Ljava/lang/Class;
      28: invokevirtual #55                 // Method java/lang/Class.getMethod:(Ljava/lang/String;[Ljava/lang/Class;)Ljava/lang/reflect/Method;
      31: invokevirtual #59                 // Method scala/runtime/ScalaRunTime$.ensureAccessible:(Ljava/lang/reflect/Method;)Ljava/lang/reflect/Method;
      34: astore_2
      35: aload_1
      36: aload_0
      37: aload_2
      38: invokevirtual #63                 // Method scala/runtime/StructuralCallSite.add:(Ljava/lang/Class;Ljava/lang/reflect/Method;)Ljava/lang/reflect/Method;
      41: pop
      42: aload_2
      43: areturn

When running on JDK9+, we should call m.canAccess(receiver) rather than m.canAccess(receiverClass).

First, we'll need to put some JDK-dependent code in ensureAccessible, and call canAccess via reflection or via a helper method that we compile under JDK9 in our build.

Then, we need to change the compilation of structural calls to pass the receiver, rather than reciever class down.

For source compatibility, we should probably leave the current signature of ensureAccessible alone (it could be used from userland) and create a new method.

@retronym

This comment has been minimized.

Copy link
Member Author

retronym commented Nov 2, 2018

We call setAccessbible for two reasons at the moment:

  • it bypasses the access check on every call, reducing the overhead of reflection.
  • We allow a private method of an outer instance satisfy a structural type at a call site in an inner class. But this would fail at runtime.

I think the second use case is questionable, and we could phase it out, perhaps contingent on use of JDK 9+.

The first issue would be a non-issue if we switched to using MethodHandle-s, which only incur the access check once, not on each invocation.

@viktorklang

This comment has been minimized.

Copy link

viktorklang commented Nov 2, 2018

Switching structural types to use MethodHandles seems like the more future-proof solution

@adriaanm adriaanm transferred this issue from scala/scala-dev Jan 16, 2019

@adriaanm adriaanm added this to the 2.14.0-M1 milestone Jan 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment