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

Reflective method lookup for structural type fails if the method has a wider parameter type in the classfile #10334

Closed
Atry opened this issue May 24, 2017 · 10 comments

Comments

@Atry
Copy link

Atry commented May 24, 2017

(see comments below for more reduced examples, the bug is not related to by-name parameters)

def byName(b: => Int): Int = b
def namer[A, B](f: A => B): (A => B) { def apply(i: A): B } = f

val namedFunction = namer(byName _)
namedFunction(1)
java.lang.NoSuchMethodException: Playground$$Lambda$5237/727589900.apply(scala.Function0)

java.lang.ExceptionInInitializerError
	at Main.main(main.scala)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at sbt.Run.invokeMain(Run.scala:67)
	at sbt.Run.run0(Run.scala:61)
	at sbt.Run.sbt$Run$$execute$1(Run.scala:51)
	at sbt.Run$$anonfun$run$1.apply$mcV$sp(Run.scala:55)
	at sbt.Run$$anonfun$run$1.apply(Run.scala:55)
	at sbt.Run$$anonfun$run$1.apply(Run.scala:55)
	at sbt.Logger$$anon$4.apply(Logger.scala:84)
	at sbt.TrapExit$App.run(TrapExit.scala:248)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.NoSuchMethodException: Playground$$Lambda$5237/727589900.apply(scala.Function0)
	at java.lang.Class.getMethod(Class.java:1786)
	at Playground.reflMethod$Method1(main.scala:7)
	at Playground.<init>(main.scala:7)
	at Main$.<init>(main.scala:11)
	at Main$.<clinit>(main.scala)
	... 14 more

https://scastie.scala-lang.org/Atry/iHBKTvX2TLOIZcDeKtbB7Q

This example works in 2.11.11, but the underlying bug also exists in 2.11 (see comments)

@Atry Atry changed the title Type refinement on a function with call-by-name parameter cause NoSuchMethodException Type refinement on a function with a call-by-name parameter cause NoSuchMethodException May 24, 2017
@Atry Atry changed the title Type refinement on a function with a call-by-name parameter cause NoSuchMethodException Type refinement on a function with a call-by-name parameter causes NoSuchMethodException May 25, 2017
@Atry
Copy link
Author

Atry commented May 25, 2017

The function does not have to be a reflective call, because there is a generic version of apply on Function1.
Scalac should detect if the method refinement has a generic version on base types.

@lrytz
Copy link
Member

lrytz commented Jun 29, 2017

In the particular case yes, the reflective call is unnecessary.

The fact that it crashes shows another bug though, which can be exploited with the following snippet:

object Test {
  def main(args: Array[String]): Unit = {
    val f: { def apply(s: String): Object } = (x: String) => x
    println(f("hi"))
  }
}

Running this gives

java.lang.NoSuchMethodException
	at java.lang.Class.getMethod(Class.java:1786)
	at Test$.reflMethod$Method1(Test.scala:15)
	at Test$.main(Test.scala:15)

In 2.11, there is a (String)Object method and an (Object)Object bridge in the anonfun classfile. This bug is due to the new lambda encoding in 2.12. The Function1 object generated by LMF only has a generic (Object)Object apply method. That apply method will automatically cast its argument in order to invoke the lambda body method (which takes a String as argument), see the LMF documentation around "adapt".

When looking up the method to be invoked reflectively, we are still using the (String)Object signature, which doesn't exist in the LMF lambda object. (This bug is not related to the new cache implementation added in scala/scala#4896).

I think this would be good to fix for 2.12.3. I'll take a look tomorrow, cc @retronym, let me know if you dive into it before.

@Atry
Copy link
Author

Atry commented Jun 29, 2017

There is a similar bug on Scala 2.11

Welcome to Scala 2.11.11 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_131).
Type in expressions for evaluation. Or try :help.

scala> val f0: Product => Unit = { none: Product => }
f0: Product => Unit = <function1>

scala> val f1: None.type => Unit = f0
f1: None.type => Unit = <function1>

scala> val f2: { def apply(i: None.type): Unit } = f1
f2: AnyRef{def apply(i: None.type): Unit} = <function1>

scala> f2(None)
warning: there was one feature warning; re-run with -feature for details
java.lang.NoSuchMethodException: $anonfun$1.apply(scala.None$)
  at java.lang.Class.getMethod(Class.java:1786)
  at .reflMethod$Method1(<console>:13)
  ... 42 elided

@lrytz
Copy link
Member

lrytz commented Jun 30, 2017

This is a pretty fundamental issue with structural types: a value of type { def m(x: T): Any } might actually have method m with a supertype of T as parameter. But there can be multiple overloaded methods, and it's not possible to tell which is the right one to invoke. Here's an example (same issue in 2.11 and 2.12):

class A
class B extends A
class C extends B

trait T[-A] {
  def m(a: A): Object
}

object Test {
  def main(args: Array[String]): Unit = {
    val f1 = new T[A] {
      def m(x: A) = "f1-a"
      def m(x: B) = "f1-b"
       // the m(Object)Object bridge method invokes (A)Object
    }

    val f2 = new T[B] {
      def m(x: A) = "f2-a"
      def m(x: B) = "f2-b"
       // the (Object)Object bridge method invokes (B)Object
    }

    val g1: T[C] = f1
    val g2: T[C] = f2

    assert(g1.m(new C) == "f1-a")
    assert(g2.m(new C) == "f2-b")

    val s1: { def m(s: C): Object } = g1
    val s2: { def m(s: C): Object } = g2

    // these currently crash, the reflective lookup doesn't find `m(C)Object`
    println(s1.m(new C)) // should invoke `m(A)Object` (?)
    println(s2.m(new C)) // should invoke `m(B)Object` (?)
  }
}

@lrytz lrytz modified the milestones: 2.12.4, 2.12.3 Jun 30, 2017
@lrytz
Copy link
Member

lrytz commented Jun 30, 2017

Moved to 2.12.4 as it's an old issue

@lrytz lrytz changed the title Type refinement on a function with a call-by-name parameter causes NoSuchMethodException Reflective method lookup for structural type fails if the method has a more wide parameter type in the classfile Jun 30, 2017
@lrytz lrytz changed the title Reflective method lookup for structural type fails if the method has a more wide parameter type in the classfile Reflective method lookup for structural type fails if the method has a wider parameter type in the classfile Jun 30, 2017
@Atry
Copy link
Author

Atry commented Jun 30, 2017

@lrytz The bug in Scala 2.11 only occurs on types related to contravariance. However, in Scala 2.12, it occurs on SAM types, which is far more common than type contravariance.

@lrytz
Copy link
Member

lrytz commented Jun 30, 2017

Yes, thanks for pointing that out. A partial fix that would cover many cases would be to have a fallback lookup by method name, and pick it if there's a single one.

This should be easy to do for 2.12.3, but it would still fail in the presence of overloads, which can also occur in the case of LMF-generated objects.

@Atry
Copy link
Author

Atry commented Jun 30, 2017

Also the original bug report does not trigger reflective call warning, which means the typer did not expect a reflective call at all.

lrytz added a commit to lrytz/scala that referenced this issue Jul 3, 2017
In a structural invocation, if the receiver object doesn't have
a method with the expected paramter types, check if there's a
single method matching the name. If so, invoke this method.

This fixes parts of scala/bug#10334,
namely the parts that regressed with the new lambda encoding.
But the underlying issue still remains.
@lrytz
Copy link
Member

lrytz commented Jul 3, 2017

Submitted the workaround here: scala/scala#5973

lrytz added a commit to lrytz/scala that referenced this issue Jul 6, 2017
Currently, a lambda object for `(s: String) => String` only gets the
`apply(Object)Object` method (LMF generates the necessary casts). When
using such a lambda through a structural type `{def apply(s: String): String}`,
the reflective lookup for the apply mehthod fails.

This patch asks LMF to generate a bridge method with the instantiated signature.

Fixes the regressed parts of scala/bug#10334
lrytz added a commit to lrytz/scala that referenced this issue Jul 11, 2017
Currently, a lambda object for `(s: String) => String` only gets the
`apply(Object)Object` method (LMF generates the necessary casts). When
using such a lambda through a structural type `{def apply(s: String): String}`,
the reflective lookup for the apply mehthod fails.

This patch asks LMF to generate a bridge method with the instantiated signature.

Fixes the regressed parts of scala/bug#10334
@retronym retronym modified the milestones: 2.12.3, 2.12.4 Jul 18, 2017
@retronym
Copy link
Member

retronym commented Jul 18, 2017

I'm going to mark this as fixed and spin off #10414 for the non-regression part.

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

No branches or pull requests

3 participants