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

java.util.Collections.min has wrong return type #7963

Closed
molikto opened this issue Jan 11, 2020 · 3 comments
Closed

java.util.Collections.min has wrong return type #7963

molikto opened this issue Jan 11, 2020 · 3 comments

Comments

@molikto
Copy link
Contributor

molikto commented Jan 11, 2020

minimized code

abstract class Test extends java.lang.Comparable[Test] {

  def test: Unit = {
  	java.util.Collections.min(new java.util.ArrayList[Test]())
  }
}
Compilation output
public abstract class Test extends java.lang.Object implements java.lang.Comparable<Test>
  minor version: 0
  major version: 52
  flags: (0x0421) ACC_PUBLIC, ACC_SUPER, ACC_ABSTRACT
  this_class: #2                          // Test
  super_class: #5                         // java/lang/Object
  interfaces: 1, fields: 0, methods: 2, attributes: 4
Constant pool:
   #1 = Utf8               Test
   #2 = Class              #1             // Test
   #3 = Utf8               Ljava/lang/Object;Ljava/lang/Comparable<LTest;>;
   #4 = Utf8               java/lang/Object
   #5 = Class              #4             // java/lang/Object
   #6 = Utf8               java/lang/Comparable
   #7 = Class              #6             // java/lang/Comparable
   #8 = Utf8               test.scala
   #9 = Utf8               <init>
  #10 = Utf8               ()V
  #11 = NameAndType        #9:#10         // "<init>":()V
  #12 = Methodref          #5.#11         // java/lang/Object."<init>":()V
  #13 = Utf8               this
  #14 = Utf8               LTest;
  #15 = Utf8               test
  #16 = Utf8               java/util/ArrayList
  #17 = Class              #16            // java/util/ArrayList
  #18 = Methodref          #17.#11        // java/util/ArrayList."<init>":()V
  #19 = Utf8               java/util/Collections
  #20 = Class              #19            // java/util/Collections
  #21 = Utf8               min
  #22 = Utf8               (Ljava/util/Collection;)Ljava/lang/Comparable;
  #23 = NameAndType        #21:#22        // min:(Ljava/util/Collection;)Ljava/lang/Comparable;
  #24 = Methodref          #20.#23        // java/util/Collections.min:(Ljava/util/Collection;)Ljava/lang/Comparable;
  #25 = Utf8               Code
  #26 = Utf8               LineNumberTable
  #27 = Utf8               LocalVariableTable
  #28 = Utf8               Signature
  #29 = Utf8               SourceFile
  #30 = Utf8               TASTY
  #31 = Utf8               Scala

expectation

the return type should be Object, but instead is Ljava/lang/Comparable


See comment bellow for more information where it is wrong.

After trying to fix this problem, I found the root cause is in ClassfileParser, all type bounds is eagerly & in sig2typeBounds, so this is not a bug in erasure or backend.

I cannot think of a proper way to overcame this now. Maybe we should also parse the erasured signature from constant pool, then in Erasure we will always use the ones parsed instead compute ourselves?

@TheElectronWill
Copy link
Member

More information about this 👇
Collections.min is declared as follows:

public static <T extends Object & Comparable<? super T>> T min(Collection<? extends T>);

It seems reasonable to assume that T extends Object & SomeInterface is the same as T extends SomeInterface, but it produces a different signature.

For backward-compatibility reasons, T extends Object & SomeInterface is erased to Object, not SomeInterface. The exact rule that javac follows is to take the first type mentioned in the bound.

@sjrd
Copy link
Member

sjrd commented Aug 8, 2020

When this is fixed, it should be possible to enable the Scala.js test CollectionsOnCollectionsTest.scala, by removing it from the blacklist at
https://github.com/lampepfl/dotty/blob/dfcc0b14c0a08c9933234067b74378f317f5ebc9/project/Build.scala#L1042
It's also possible that other files in the same list could be un-blacklisted at the same time.

smarter added a commit to dotty-staging/dotty that referenced this issue Aug 19, 2020
In Java unlike Scala, `Object` is the top type, this leads to various
usability problems when attempting to call or override a Java method
from Scala. So far, we relied mainly on one mechanism to improve the
situation: in the ClassfileParser, some references to `Object` in
signatures were replaced by `Any` (cf `objToAny`). But this had several
shortcomings:
- To compensate for this substitution,
  `TypeComparer#matchingMethodParams` had to special case Any in Java
  methods and treat it like Object
- There were various situation were this substitution was not applied,
  notably when using varargs (`Object... args`) or when jointly
  compiling .java sources since this is handled by JavaParser and not
  ClassfileParser.

This commit replaces all of this by a more systematic solution: all
references to `Object` in Java definitions (both in classfiles and .java
source files) are replaced by a special type `FromJavaObject` which is a
type alias of `Object` with one extra subtyping rule:

   tp <:< FromJavaObject

is equivalent to:

   tp <:< Any

See the documentation of `FromJavaObjectSymbol` for details on
why this makes sense.

This solution is very much inspired by
scala/scala#7966 (which was refined in
scala/scala#8049 and
scala/scala#8638) with two main differences:
- We use a type with its own symbol and name distinct from
  `java.lang.Object`, because this type can show up in inferred types
  and therefore needs to be preserved when pickling so that unpickled
  trees pass `-Ycheck`.
- Unlike in Scala 2, we cannot get rid of `JavaMethodType` because when
  calling `Type#signature` we need to know whether we're in a Java
  method or not, because signatures are based on erased types and erasure
  differs between Scala and Java (we cannot ignore this and always
  base signatures on the Scala erasure, because signatures are used
  to distinguish between overloads and overrides so they must agree
  with the actual JVM bytecode signature).

Fixes scala#7467, scala#7963, scala#8588, scala#8977.
smarter added a commit to dotty-staging/dotty that referenced this issue Aug 21, 2020
In Java unlike Scala, `Object` is the top type, this leads to various
usability problems when attempting to call or override a Java method
from Scala. So far, we relied mainly on one mechanism to improve the
situation: in the ClassfileParser, some references to `Object` in
signatures were replaced by `Any` (cf `objToAny`). But this had several
shortcomings:
- To compensate for this substitution,
  `TypeComparer#matchingMethodParams` had to special case Any in Java
  methods and treat it like Object
- There were various situation were this substitution was not applied,
  notably when using varargs (`Object... args`) or when jointly
  compiling .java sources since this is handled by JavaParser and not
  ClassfileParser.

This commit replaces all of this by a more systematic solution: all
references to `Object` in Java definitions (both in classfiles and .java
source files) are replaced by a special type `FromJavaObject` which is a
type alias of `Object` with one extra subtyping rule:

   tp <:< FromJavaObject

is equivalent to:

   tp <:< Any

See the documentation of `FromJavaObjectSymbol` for details on
why this makes sense.

This solution is very much inspired by
scala/scala#7966 (which was refined in
scala/scala#8049 and
scala/scala#8638) with two main differences:
- We use a type with its own symbol and name distinct from
  `java.lang.Object`, because this type can show up in inferred types
  and therefore needs to be preserved when pickling so that unpickled
  trees pass `-Ycheck`.
- Unlike in Scala 2, we cannot get rid of `JavaMethodType` because when
  calling `Type#signature` we need to know whether we're in a Java
  method or not, because signatures are based on erased types and erasure
  differs between Scala and Java (we cannot ignore this and always
  base signatures on the Scala erasure, because signatures are used
  to distinguish between overloads and overrides so they must agree
  with the actual JVM bytecode signature).

Fixes scala#7467, scala#7963, scala#8588, scala#8977.
@smarter
Copy link
Member

smarter commented Aug 24, 2020

Fixed in #9601.

@smarter smarter closed this as completed Aug 24, 2020
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

4 participants