scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

Regression: Signatures of specialization interacts adversely with javac #5976

Open scabug opened 12 years ago

scabug commented 12 years ago

The following code worked flawlessly on 2.9:

object japi {
@deprecated("Do not use this directly, use subclasses of this", "2.0")
  class UnitFunctionBridge[-T] extends (T ⇒ BoxedUnit) {
    override final def apply(t: T): BoxedUnit = {
      internal(t)
      BoxedUnit.UNIT
    }
    protected def internal(result: T): Unit = ()
  }
}

abstract class Foreach[-T] extends japi.UnitFunctionBridge[T] {
  override final def internal(t: T): Unit = each(t)

  /**
   * This method will be invoked once when/if a Future that this callback is registered on
   * becomes successfully completed
   */
  @throws(classOf[Throwable])
  def each(result: T): Unit
}

And then from Java:

 @Test
  public void mustBeAbleToForeachAFuture() throws Throwable {
    final CountDownLatch latch = new CountDownLatch(1);
    Promise<String> cf = Futures.promise(system.dispatcher());
    Future<String> f = cf;
    f.foreach(new Foreach<String>() {
      public void each(String future) {
        latch.countDown();
      }
    });

    cf.success("foo");
    assertTrue(latch.await(5000, TimeUnit.MILLISECONDS));
    assertEquals(Await.result(f, timeout), "foo");
  }

However, using Scala 2.10-M4 javac spits this in mah face:

[error] /Users/viktorklang/Documents/workspace/akka/akka/akka-actor-tests/src/test/java/akka/dispatch/JavaFutureTests.java:117: apply$mcLJ$sp(long) in akka.dispatch.japi.UnitFunctionBridge cannot implement apply$mcLJ$sp(long) in scala.Function1; attempting to use incompatible return type
[error] found   : java.lang.Object
[error] required: scala.runtime.BoxedUnit
[error]     f.foreach(new Foreach<String>() {
[error]                                     ^
[error] 1 error
[error] {file:/Users/viktorklang/Documents/workspace/akka/akka/}akka-actor-tests/test:compile: javac returned nonzero exit code
[error] Total time: 125 s, completed Jun 25, 2012 1:27:58 PM

This steps around it... But I think it has a certain kind of smell:

  @deprecated("Do not use this directly, use subclasses of this", "2.0")
  class UnitFunctionBridge[-T] extends (T ⇒ BoxedUnit) {
    final def apply$mcLJ$sp(l: Long): BoxedUnit = { internal(l.asInstanceOf[T]); BoxedUnit.UNIT }
    final def apply$mcLI$sp(i: Int): BoxedUnit = { internal(i.asInstanceOf[T]); BoxedUnit.UNIT }
    final def apply$mcLF$sp(f: Float): BoxedUnit = { internal(f.asInstanceOf[T]); BoxedUnit.UNIT }
    final def apply$mcLD$sp(d: Double): BoxedUnit = { internal(d.asInstanceOf[T]); BoxedUnit.UNIT }

    override final def apply(t: T): BoxedUnit = { internal(t); BoxedUnit.UNIT }
    protected def internal(result: T): Unit = ()
  }
scabug commented 12 years ago

Imported From: https://issues.scala-lang.org/browse/SI-5976?orig=1 Reporter: @viktorklang Affected Versions: 2.10.0-M4 See #3452

scabug commented 12 years ago

@paulp said: Were you really going to make no mention of any of my comments?

For the lucky assignee whose time has no value to reporter, here they are: https://groups.google.com/group/scala-internals/browse_thread/thread/5d28b67550491fc6

scabug commented 12 years ago

@viktorklang said (edited on Jun 25, 2012 4:17:44 PM UTC): Sorry about that, I didn't have time because some other error reported decided his time was more valuable than mine. Thanks for the watchful eye!

scabug commented 12 years ago

@paulp said: If that's supposed to mean me, you are absurdly out of line. One of us spends half his morning diagnosing the other one's issues, not to mention having put at least a week into it prior to this. It's not like it's my personal bug - it's just one of those very difficult problems nobody else cares to tackle.

Meanwhile the other of us thinks reporting issues via mailing list chatter is good enough, and then when imposed upon to open tickets like everyone else, somehow has the nerve to completely omit the products of said morning's work. If this is your usual mode of operation, your time must be valuable indeed.

scabug commented 12 years ago

@viktorklang said: No, I wasn't referring to you at all. I was juggling opening this ticket and dealing with a sub-par error report for Akka, which distracted me enough me to prematurely file this ticket. So it was very misfortunate that the quality of this error report suffered from the bad quality of another error report. So when I said "I'm sorry about that" I really meant, and still mean just that.

Summary: I am very sorry I forgot to add the link to the discussion, it was not an intentional omission, I respect you and your work a lot and I hope that this misunderstanding has been cleared up.

scabug commented 12 years ago

@paulp said: It's no problem - it sounded like a reference to the Wrappers issue for which I asked you to open the ticket. I apologize for reading anything into it. Consider it cleared up.

scabug commented 12 years ago

@paulp said: Back to this ticket, I know martin said to open it, but unless someone can show otherwise, this is a duplicate of #3452.

scabug commented 12 years ago

@adriaanm said: Alex, could you have a look please?

scabug commented 12 years ago

@axel22 said (edited on Jun 27, 2012 1:30:18 PM UTC): As far as I can tell, this is really related to the other ticket. Here is what happens without specialization.

Scala:

import scala.runtime.BoxedUnit

object japi {
  trait MyFunction[-T, +R] {
    def apply(t: T): R
  }

  @deprecated("Do not use this directly, use subclasses of this", "2.0")
  class UnitFunctionBridge[-T] extends MyFunction[T, BoxedUnit] {
    override final def apply(t: T): BoxedUnit = {
      internal(t)
      BoxedUnit.UNIT
    }
    protected def internal(result: T): Unit = ()
  }
}

abstract class Foreach[-T] extends japi.UnitFunctionBridge[T] {
  override final def internal(t: T): Unit = each(t)

  /**
   * This method will be invoked once when/if a Future that this callback is registered on
   * becomes successfully completed
   */
  @throws(classOf[Throwable])
  def each(result: T): Unit
}

Java:

public class Test {

    public void mustBeAbleToForeachAFuture() throws Throwable {
    new Foreach<String>() {
        public void each(String future) {
        }
        };
    }

}

Result after running:


java.lang.NoSuchMethodException: Test.main([Ljava.lang.String;)
    at java.lang.Class.getMethod(Class.java:1622)
    at scala.tools.nsc.util.ScalaClassLoader$class.run(ScalaClassLoader.scala:67)
    at scala.tools.nsc.util.ScalaClassLoader$URLClassLoader.run(ScalaClassLoader.scala:139)
    at scala.tools.nsc.CommonRunner$class.run(ObjectRunner.scala:28)
    at scala.tools.nsc.ObjectRunner$.run(ObjectRunner.scala:45)
    at scala.tools.nsc.CommonRunner$class.runAndCatch(ObjectRunner.scala:35)
    at scala.tools.nsc.ObjectRunner$.runAndCatch(ObjectRunner.scala:45)
    at scala.tools.nsc.MainGenericRunner.runTarget$1(MainGenericRunner.scala:70)
    at scala.tools.nsc.MainGenericRunner.process(MainGenericRunner.scala:92)
    at scala.tools.nsc.MainGenericRunner$.main(MainGenericRunner.scala:101)
    at scala.tools.nsc.MainGenericRunner.main(MainGenericRunner.scala)

I would reassign this.

scabug commented 12 years ago

@paulp said: Although I remain convinced this is #3452, that particular transcript is less than convincing as supporting evidence:

java.lang.NoSuchMethodException: Test.main([Ljava.lang.String;)

Indeed it has no main method.

scabug commented 12 years ago

@gkossakowski said: Alex: I don't understand what your comment tries to say. Can you elaborate?

scabug commented 12 years ago

@gkossakowski said: Viktor: can you give a minimized example that does not depend on akka and can be compiled from command line just with scalac and javac?

scabug commented 12 years ago

@paulp said: I'm pretty sure Alex was stricken by a disease I'm pretty familiar with myself, "I'm expecting a particular failure and there it is!" The problem is that the NoSuchMethodError he expected to see (based on my pointing at #3452) was not the NoSuchMethodError he actually induced (as his had the more pedestrian cause of omitting the main method.)

scabug commented 12 years ago

@viktorklang said: Greg: There is no need for Akka to reproduce this

scabug commented 12 years ago

@axel22 said: It means I hastily concluded that this error was unrelated to specialization, thanks to not including the main method above.

scabug commented 12 years ago

@gkossakowski said: Viktor: So where's the self-contained example reproducing this problem?

scabug commented 12 years ago

@adriaanm said: // t5976.scala

import scala.runtime.BoxedUnit

object japi {
@deprecated("Do not use this directly, use subclasses of this", "2.0")
  class UnitFunctionBridge[-T] extends (T ⇒ BoxedUnit) {
    override final def apply(t: T): BoxedUnit = {
      internal(t)
      BoxedUnit.UNIT
    }
    protected def internal(result: T): Unit = ()
  }
}

abstract class Foreach[-T] extends japi.UnitFunctionBridge[T] {
  override final def internal(t: T): Unit = each(t)

  /**
   * This method will be invoked once when/if a Future that this callback is registered on
   * becomes successfully completed
   */
  @throws(classOf[Throwable])
  def each(result: T): Unit
}

class Future[T] { def foreach[U](f: T => U): U = ??? }

// Test.java

public class Test {
  public void mustBeAbleToForeachAFuture(Future<String> f) throws Throwable {
    f.foreach(new Foreach<String>() {
      public void each(String future) {
      }
    });
  }
}
scabug commented 12 years ago

@paulp said: This will suffice for the missing Future, depending on what level of self-containedness you're after.

class Future[+T](val result: T) {
  def foreach(x: Foreach[T]): Unit = x each result
}
scabug commented 12 years ago

@gkossakowski said: Change (T ⇒ BoxedUnit) to AbstractFunction1[T, BoxedUnit] and you are good to go on both 2.9.2 and 2.10.x. The complete example:

//  t5976.scala
import scala.runtime.BoxedUnit
import scala.runtime.AbstractFunction1

object japi {
@deprecated("Do not use this directly, use subclasses of this", "2.0")
  class UnitFunctionBridge[-T] extends AbstractFunction1[T, BoxedUnit] {
    override final def apply(t: T): BoxedUnit = {
      internal(t)
      BoxedUnit.UNIT
    }
    protected def internal(result: T): Unit = ()
  }
}

abstract class Foreach[-T] extends japi.UnitFunctionBridge[T] {
  override final def internal(t: T): Unit = each(t)

  /**
   * This method will be invoked once when/if a Future that this callback is registered on
   * becomes successfully completed
   */
  @throws(classOf[Throwable])
  def each(result: T): Unit
}

class Future[T] { def foreach[U](f: T => U): U = sys.error("foo") }

and:

// Test.java
public class Test {
  public void mustBeAbleToForeachAFuture(Future<String> f) throws Throwable {
    f.foreach(new Foreach<String>() {
      public void each(String future) {
      }
    });
  }
}

Closing this as Not a bug because I don't believe there's actual bug here and there's an easy work-around present. Maybe we should generate bridge methods in UnitFunctionBridge but I'm not sure.

scabug commented 12 years ago

@viktorklang said: So the "regression" part is nonsense?

scabug commented 12 years ago

@paulp said: I don't understand how there can not be a bug.

scabug commented 12 years ago

@gkossakowski said: Well, we are getting very close to binary compatibility here. What happens is that we generate different byte-code due to changes in specialization related to AnyRef specialization (that are binary incompatible but that's fine) and javac chokes on a new byte-code.

I think it might be hard to satisfy javac. I don't know all issues related to bridge methods and how javac treats them so more investigation would be needed.

However, I just wanted to say that I don't think this is critical for 2.10.0 release.

scabug commented 12 years ago

@gkossakowski said: Paul: Do you understand the exact issue here?

scabug commented 12 years ago

@paulp said: I'm not completely sure except to be pretty sure it's our bug. It has to do with the thing being parameterized as BoxedUnit turning up as Object one place and BoxedUnit another. Presumably because Unit is a @specialize target (but only usefully so in return position) our specializing on AnyRef in parameter position interacted with that.

scabug commented 12 years ago

@gkossakowski said: Well, ok, I'll have another look tomorrow. First I need to minimize this by not depending on Function1 interface which is huge. I'll try to come up with answers to following questions:

Stay tuned.

scabug commented 12 years ago

@paulp said: There are "official regressions" and there are "practical regressions". Our guarantees in this area are slim to nonexistent, and would be fairly meaningless if they existed, but this is not really about java compatibility. The fact that javac refuses to compile against the generated code should be seen as an independent opinion that there's something inconsistent about the generated code, not as something specific to javac.

I do not think this is a matter of bridges, but of generating the right signatures. I will point against at #3452 if you're looking into this: it is subtle.

scabug commented 12 years ago

@SethTisue said: Agree. If javac chokes it's a pretty safe assumption that other tools will choke too and we should be thankful to javac for the early heads up.

scabug commented 12 years ago

@gkossakowski said: Ok, I spent more time on it and we indeed generate wrong signatures for method (that do not agree with generic signatures). I created a separate ticket to track progress on it:

6101

It contains truly minimized test-case.

I'll assign both tickets to Alex as this is purely specialization problem.

scabug commented 11 years ago

@adriaanm said: Un-assigning to foster work stealing, as announced in https://groups.google.com/forum/?fromgroups=#!topic/scala-internals/o8WG4plpNkw

scabug commented 11 years ago

@adriaanm said: Unassigning and rescheduling to M6 as previous deadline was missed.