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

Methods inherited from generic traits sometimes have types erased to Object #8905

Open
scabug opened this issue Oct 13, 2014 · 3 comments
Open

Methods inherited from generic traits sometimes have types erased to Object #8905

scabug opened this issue Oct 13, 2014 · 3 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Oct 13, 2014

It seems that methods inherited from generic traits sometimes have their types erased to Object, which causes compilation or runtime errors when calling these methods from Java. This seems like it it might be related to #3452.

Here's a simple reproduction (also available as a gist with a build.sbt, in case you want to clone it to quickly test this out):

In Scala:

import java.util.Comparator

trait RDDLike[T] {
  def max(comp: Comparator[T]): T = {
    (1.0).asInstanceOf[T]
  }
}

class DoubleRDD extends RDDLike[java.lang.Double] { }

In Java:

import java.util.Comparator;

public class Test {
  private static class DoubleComparator implements Comparator<Double> {
    public int compare(Double o1, Double o2) {
      return o1.compareTo(o2);
    }
  }

  public static void main(String[] args) {
    DoubleRDD rdd = new DoubleRDD();
    RDDLike<Double> rddLike = rdd;

    // This call works fine:
    double rddLikeMax = rddLike.max(new DoubleComparator());
    // In Scala 2.10.4, this code compiles but this call fails at runtime:
    // java.lang.NoSuchMethodError: DoubleRDD.max(Ljava/util/Comparator;)Ljava/lang/Double;
    double rddMax = rdd.max(new DoubleComparator());
  }

}

In Scala 2.10.4, this code compiles fine but throws a NoSuchMethodError at runtime when I call the max method via the DoubleRDD class. In all versions of Scala 2.11 that I've tested, the Java code fails to compile:

error: incompatible types
[error]     double rddMax = rdd.max(new DoubleComparator());
[error]                            ^
[error]   required: double
[error]   found:    Object
[error] 1 error

Based on javap, it seems that Scala 2.10.4 emits the right signature for DoubleRDD.max:

javap -s target/scala-2.10/classes/DoubleRDD.class
Compiled from "RDD.scala"
public class DoubleRDD implements RDDLike<java.lang.Double> {
  public java.lang.Double max(java.util.Comparator<java.lang.Double>);
    Signature: (Ljava/util/Comparator;)Ljava/lang/Object;

  public DoubleRDD();
    Signature: ()V
}

Scala 2.11.2 seems to erase to Object:

javap -s target/scala-2.11/classes/DoubleRDD.class
Compiled from "RDD.scala"
public class DoubleRDD implements RDDLike<java.lang.Double> {
  public java.lang.Object max(java.util.Comparator);
    Signature: (Ljava/util/Comparator;)Ljava/lang/Object;

  public DoubleRDD();
    Signature: ()V
}

However, if I override the implementation of the max method in my class, then everything works fine. This compiles and runs as expected:

class DoubleRDD extends RDDLike[java.lang.Double] {
  override def max(comp: Comparator[java.lang.Double]): java.lang.Double = {
    (1.0).asInstanceOf[java.lang.Double]
  }
}

Surprisingly, though, the generated bytecode now contains two max methods:

javap -s target/scala-2.10/classes/DoubleRDD.class
Compiled from "RDD.scala"
public class DoubleRDD implements RDDLike<java.lang.Double> {
  public java.lang.Double max(java.util.Comparator<java.lang.Double>);
    Signature: (Ljava/util/Comparator;)Ljava/lang/Double;

  public java.lang.Object max(java.util.Comparator);
    Signature: (Ljava/util/Comparator;)Ljava/lang/Object;

  public DoubleRDD();
    Signature: ()V
}

This happens in both 2.10.4 and 2.11.2.

@scabug
Copy link
Author

@scabug scabug commented Oct 13, 2014

Imported From: https://issues.scala-lang.org/browse/SI-8905?orig=1
Reporter: Josh Rosen (joshrosen)
Affected Versions: 2.10.4, 2.11.0, 2.11.1, 2.11.2, 2.11.3
See #3452
Attachments:

  • RDD.scala (created on Oct 13, 2014 8:04:22 PM UTC, 214 bytes)
  • Test.java (created on Oct 13, 2014 8:04:22 PM UTC, 605 bytes)
@scabug
Copy link
Author

@scabug scabug commented Mar 13, 2015

@retronym said:
The tension in the compiler is that the Mixin phase of the compiler, which add "trait forwarder" methods for mixed-in methods, does not add a bridge method if the erased signature of the forwarder differs (ie: is more specific than) the interface method that is implements. This is because, for historical reasons, the Mixin phase comes after the Erasure phase (which adds bridges.)

In my patch for #3452, I originally tried to add these bridge methods. However, this caused certain programs to fail compilation, if the extra bridge method clashed with an existing method the user defined. So I backed out of that path, and instead opted to ensure that the forwarder would not differ in erased signature from the interface method. This is what now leads to the weaker return type you see from Java clients.

A workaround would be to introduce an intermediate abstract class as follows:

import java.util.Comparator
 
trait RDDLike[T] {
  def max(comp: Comparator[T]): T = {
    (1.0).asInstanceOf[T]
  }
}

abstract class AbstractRDD[T] extends RDDLike[T]
 
class DoubleRDD extends AbstractRDD[java.lang.Double] { }
@scabug scabug added this to the Backlog milestone Apr 7, 2017
smarter added a commit to smarter/scala that referenced this issue Mar 10, 2019
…tures

The compiler spends a lot of effort trying to generate correct generic
signatures for mixin forwarders, but is still not correct as witnessed
by scala/bug#8905 among others. However, it seems that if we just emit
them with the BRIDGE flag set, both javac and ecj (the Eclipse Java
Compiler) will be smart enough to go look for the signature in the
overridden methods, which is exactly what we want.

This means that we should be able to get rid of `cloneBeforeErasure` and
related code, though that would conflict with scala#7752.

Because history always repeats itself, it turns out that this used to be
how mixin forwarders were emitted until
fe94bc7 changed this 7 years ago. The
commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Fixes scala/bug#8905 and probably countless others.
smarter added a commit to smarter/scala that referenced this issue Mar 10, 2019
The compiler spends a lot of effort trying to generate correct generic
signatures for mixin forwarders, but is still not correct as witnessed
by scala/bug#8905 among others. However, it seems that if we just emit
them with the BRIDGE flag set, both javac and ecj (the Eclipse Java
Compiler) will be smart enough to go look for the signature in the
overridden methods, which is exactly what we want.

This means that we should be able to get rid of `cloneBeforeErasure` and
related code, though that would conflict with scala#7752.

Because history always repeats itself, it turns out that this used to be
how mixin forwarders were emitted until
fe94bc7 changed this 7 years ago. The
commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Fixes scala/bug#8905 and probably countless others.
smarter added a commit to smarter/scala that referenced this issue Mar 10, 2019
The compiler spends a lot of effort trying to generate correct generic
signatures for mixin forwarders, but is still not correct as witnessed
by scala/bug#8905 among others. However, it seems that if we just emit
them with the BRIDGE flag set, both javac and ecj (the Eclipse Java
Compiler) will be smart enough to go look for the signature in the
overridden methods, which is exactly what we want.

This means that we should be able to get rid of `cloneBeforeErasure` and
related code, though that would conflict with scala#7752.

Because history always repeats itself, it turns out that this used to be
how mixin forwarders were emitted until
fe94bc7 changed this 7 years ago. The
commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Fixes scala/bug#8905 and probably countless others.
smarter added a commit to smarter/scala that referenced this issue Mar 10, 2019
The compiler spends a lot of effort trying to generate correct generic
signatures for mixin forwarders, but is still not correct as witnessed
by scala/bug#8905 among others. However, it seems that if we just emit
them with the BRIDGE flag set, both javac and ecj (the Eclipse Java
Compiler) will be smart enough to go look for the signature in the
overridden methods, which is exactly what we want.

This means that we should be able to get rid of `cloneBeforeErasure` and
related code, though that would conflict with scala#7752.

Because history always repeats itself, it turns out that this used to be
how mixin forwarders were emitted until
fe94bc7 changed this 7 years ago. The
commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Fixes scala/bug#8905 and probably countless others.
smarter added a commit to smarter/scala that referenced this issue Mar 11, 2019
The compiler spends a lot of effort trying to generate correct generic
signatures for mixin forwarders, but is still not correct as witnessed
by scala/bug#8905 among others. However, it seems that if we just emit
them with the BRIDGE flag set, both javac and ecj (the Eclipse Java
Compiler) will be smart enough to go look for the signature in the
overridden methods, which is exactly what we want.

This means that we should be able to get rid of `cloneBeforeErasure` and
related code, though that would conflict with scala#7752.

Because history always repeats itself, it turns out that this used to be
how mixin forwarders were emitted until
fe94bc7 changed this 7 years ago. The
commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Fixes scala/bug#8905 and probably countless others.
smarter added a commit to dotty-staging/dotty that referenced this issue Mar 12, 2019
* Pros

- When forwarding before erasure, we might end up needing a bridge,
  the non-bridged forwarder therefore serves no practical purpose
  and can cause clashes. These are problematic for two reasons:
  1. Valid Scala 2 code might break, and can only be fixed in
     binary incompatible way, like with
     scala/scala@4366332
     and
     scala/scala@e3ef657
  2. Clashes might not be detected under separate compilation, as
     demonstrated by the previous commit.
  Forwarding after erasure solves both of these problems.

- Scala 2 has always done it this way, so we're less likely to run
  into surprising problems.

* Cons

- When forwarding after erasure, generic signatures will be missing,
  Scala 2 tries to restore this information but that doesn't always
  work (scala/bug#8905). We'll either have
  to do something similar, or investigate a different solution like
  scala/scala#7843.
smarter added a commit to dotty-staging/dotty that referenced this issue Mar 12, 2019
* Pros

- When forwarding before erasure, we might end up needing a bridge,
  the non-bridged forwarder therefore serves no practical purpose
  and can cause clashes. These are problematic for two reasons:
  1. Valid Scala 2 code might break, and can only be fixed in
     binary incompatible way, like with
     scala/scala@4366332
     and
     scala/scala@e3ef657
  2. Clashes might not be detected under separate compilation, as
     demonstrated by the previous commit.
  Forwarding after erasure solves both of these problems.

- Scala 2 has always done it this way, so we're less likely to run
  into surprising problems.

* Cons

- When forwarding after erasure, generic signatures will be missing,
  Scala 2 tries to restore this information but that doesn't always
  work (scala/bug#8905). We'll either have
  to do something similar, or investigate a different solution like
  scala/scala#7843.
smarter added a commit to dotty-staging/dotty that referenced this issue Mar 12, 2019
* Pros

- When forwarding before erasure, we might end up needing a bridge,
  the non-bridged forwarder therefore serves no practical purpose
  and can cause clashes. These are problematic for two reasons:
  1. Valid Scala 2 code might break, and can only be fixed in
     binary incompatible way, like with
     scala/scala@4366332
     and
     scala/scala@e3ef657
  2. Clashes might not be detected under separate compilation, as
     demonstrated by the previous commit.
  Forwarding after erasure solves both of these problems.

- Scala 2 has always done it this way, so we're less likely to run
  into surprising problems.

* Cons

- When forwarding after erasure, generic signatures will be missing,
  Scala 2 tries to restore this information but that doesn't always
  work (scala/bug#8905). We'll either have
  to do something similar, or investigate a different solution like
  scala/scala#7843.
smarter added a commit to dotty-staging/dotty that referenced this issue Mar 18, 2019
* Pros

- When forwarding before erasure, we might end up needing a bridge,
  the non-bridged forwarder therefore serves no practical purpose
  and can cause clashes. These are problematic for two reasons:
  1. Valid Scala 2 code might break, and can only be fixed in
     binary incompatible way, like with
     scala/scala@4366332
     and
     scala/scala@e3ef657
  2. Clashes might not be detected under separate compilation, as
     demonstrated by the previous commit.
  Forwarding after erasure solves both of these problems.

- Scala 2 has always done it this way, so we're less likely to run
  into surprising problems.

* Cons

- When forwarding after erasure, generic signatures will be missing,
  Scala 2 tries to restore this information but that doesn't always
  work (scala/bug#8905). We'll either have
  to do something similar, or investigate a different solution like
  scala/scala#7843.
smarter added a commit to dotty-staging/dotty that referenced this issue Mar 18, 2019
* Pros

- When forwarding before erasure, we might end up needing a bridge,
  the non-bridged forwarder therefore serves no practical purpose
  and can cause clashes. These are problematic for two reasons:
  1. Valid Scala 2 code might break, and can only be fixed in
     binary incompatible way, like with
     scala/scala@4366332
     and
     scala/scala@e3ef657
  2. Clashes might not be detected under separate compilation, as
     demonstrated by the previous commit.
  Forwarding after erasure solves both of these problems.

- Scala 2 has always done it this way, so we're less likely to run
  into surprising problems.

- Compiling the 2.12 standard library got 20% faster, I suspect that
  the previous hotspot in `isCurrent` is no longer so hot when run
  after erasure, so I removed the comment there.

* Cons

- When forwarding after erasure, generic signatures will be missing,
  Scala 2 tries to restore this information but that doesn't always
  work (scala/bug#8905). We'll either have
  to do something similar, or investigate a different solution like
  scala/scala#7843.
@SethTisue SethTisue modified the milestones: Backlog, 2.13.0-RC1 Mar 19, 2019
@SethTisue SethTisue assigned smarter and unassigned retronym Mar 19, 2019
@lrytz
Copy link
Member

@lrytz lrytz commented May 7, 2019

fix reverted in scala/scala#8037

@lrytz lrytz reopened this May 7, 2019
@SethTisue SethTisue modified the milestones: 2.13.0-RC1, Backlog May 7, 2019
@NthPortal NthPortal reopened this Jun 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants