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

Regression: Signatures of specialization interacts adversely with javac #5976

Open
scabug opened this issue Jun 25, 2012 · 30 comments
Open

Regression: Signatures of specialization interacts adversely with javac #5976

scabug opened this issue Jun 25, 2012 · 30 comments

Comments

@scabug
Copy link

scabug commented Jun 25, 2012

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
Copy link
Author

scabug commented Jun 25, 2012

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

@scabug
Copy link
Author

scabug commented Jun 25, 2012

@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
Copy link
Author

scabug commented Jun 25, 2012

@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
Copy link
Author

scabug commented Jun 25, 2012

@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
Copy link
Author

scabug commented Jun 25, 2012

@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
Copy link
Author

scabug commented Jun 25, 2012

@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
Copy link
Author

scabug commented Jun 26, 2012

@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
Copy link
Author

scabug commented Jun 26, 2012

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

@scabug
Copy link
Author

scabug commented Jun 27, 2012

@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
Copy link
Author

scabug commented Jun 27, 2012

@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
Copy link
Author

scabug commented Jul 16, 2012

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

@scabug
Copy link
Author

scabug commented Jul 16, 2012

@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
Copy link
Author

scabug commented Jul 16, 2012

@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
Copy link
Author

scabug commented Jul 16, 2012

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

@scabug
Copy link
Author

scabug commented Jul 16, 2012

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

@scabug
Copy link
Author

scabug commented Jul 16, 2012

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

@scabug
Copy link
Author

scabug commented Jul 16, 2012

@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
Copy link
Author

scabug commented Jul 16, 2012

@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
Copy link
Author

scabug commented Jul 17, 2012

@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
Copy link
Author

scabug commented Jul 17, 2012

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

@scabug
Copy link
Author

scabug commented Jul 17, 2012

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

@scabug
Copy link
Author

scabug commented Jul 17, 2012

@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
Copy link
Author

scabug commented Jul 17, 2012

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

@scabug
Copy link
Author

scabug commented Jul 17, 2012

@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
Copy link
Author

scabug commented Jul 17, 2012

@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:

  • is this a bug of some form (e.g. lack of bridges generated where they should be)
  • if it's not a bug what about regression part? I don't know how much we guarantee when it comes to Java compatibility

Stay tuned.

@scabug
Copy link
Author

scabug commented Jul 17, 2012

@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
Copy link
Author

scabug commented Jul 17, 2012

@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
Copy link
Author

scabug commented Jul 18, 2012

@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
Copy link
Author

scabug commented Mar 15, 2013

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

@scabug
Copy link
Author

scabug commented Jul 10, 2013

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

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

1 participant