Kotlin interop: reporting wrong type for inner classes appearing in Kotlin method types

[jbwheatley]

Compiler version

3.0.0-RC2

Minimized code

I'm working with a library that is written in kotlin, and running into a compilation error on 3.0.0-RC2 that doesn't happen on 2.13.5. The decompiled .class file looks like:

public abstract class VerificationResult {
  public static final class Ok extends VerificationResult {...}
  public static final class Failed extends VerificationResult {...}
  ...
}

and then there is a method on another class like:

public void displayFailures(@NotNull List<Failed> failures)

I then get this compiler error when calling the method:

Output

[error] 16 |            verifier.displayFailures(List(failed).asJava)
[error]    |                                     ^^^^^^^^^^^^^^^^^^^
[error]    |Found:    java.util.List[au.com.dius.pact.provider.VerificationResult.Failed]
[error]    |Required: java.util.List[au.com.dius.pact.provider.VerificationResult$Failed]

Expectation

Should compile.

[smarter]

We need a way (ideally minimized) to reproduce the problem, otherwise there's not much we can do about it.

[jbwheatley]

Foo.java as:

package foo;
import foo.Foo.Bar1;
import java.util.List;

public abstract class Foo {
    public static final class Bar1 extends Foo {}
    public static final class Bar2 extends Foo {}
}

class Baz {
    public void bars(List<Bar1> bs) {
        bs.clear();
    }
}

FooScala.scala as:

package foo
import Foo._
import scala.jdk.CollectionConverters._

object FooScala {
  def doThings(foo: Foo): Unit =
    foo match {
      case b: Foo.Bar1 => new Baz().bars(List(b).asJava)
      case _: Foo.Bar2 => ()
    }
}

will recreate the issue

[smarter]

Thank you for the minimization, I just tried it but I can't reproduce the issue (the code compiles fine), can you list the steps you use to reproduce the problem and the version of javac you have?

[jbwheatley]

Running sbt +compile at my project route. sbt is using java 14.0.1, presumably that is the issue?

[smarter]

I don't have java 14 here, but I can't reproduce with either java 8 or 15 locally, could you try using a different version?

[jbwheatley]

oh I'm an idiot, I was getting my errors all mixed up (sorry, end of the day here in UK). My minimized example isn't causing the error for me 🤦 I'm stuck for a way to reproduce this for you besides having you try it with the library I am using

[smarter]

I know that feeling :). if you can minimize it to a scala file that depends on a publicly available library that would be useful already.

[jbwheatley]

Finally got something for you! 😁 https://scastie.scala-lang.org/e4TAeoEISaeDFNE4HqbrnQ

[smarter]

Thank you! This can be reproduced using coursier with:

scalac -classpath "$(cs fetch -p au.com.dius.pact:provider:4.2.0)" Test.scala

@prolativ You've worked on inner classes recently, so you might be able to figure out what's going on here

[griggt]

Minimized:

Result.kt

sealed class Result {
  class Failed : Result()
}

class Verifier {
  fun displayFailure(failure: Result.Failed) {}
}

Test.scala

object Test {
  def foo(verifier: Verifier, failed: Result.Failed): Unit =
    verifier.displayFailure(failed)
}

$ kotlinc -version
info: kotlinc-jvm 1.4.31 (JRE 1.8.0_282-b08)

$ dotc -version
Scala compiler version 3.0.0-RC2 -- Copyright 2002-2021, LAMP/EPFL

$ kotlinc Result.kt && dotc Test.scala
Result.kt:6:22: warning: parameter 'failure' is never used
  fun displayFailure(failure: Result.Failed) {}
                     ^
-- [E007] Type Mismatch Error: Test.scala:3:28 ------------------------------------------
3 |    verifier.displayFailure(failed)
  |                            ^^^^^^
  |                            Found:    (failed : Result.Failed)
  |                            Required: Result$Failed

I tried translating the Kotlin to Java but was unable to reproduce the issue, compilation was successful.

[smarter]

Very interesting, could you paste the output of javap -p -v Result.class for both the kotlin and java version?

[smarter]

@lrytz are you aware of any special handling of inner classes in kotlin that required changes in scalac?

[griggt]

Kotlin Result.class

Classfile /src/dotty-issues/i12086/standalone/kt/Result.class
  Last modified Apr 13, 2021; size 740 bytes
  MD5 checksum f7bfd03cae50ec69aa999d753ed05c8f
  Compiled from "Result.kt"
public abstract class Result
  minor version: 0
  major version: 50
  flags: ACC_PUBLIC, ACC_SUPER, ACC_ABSTRACT
Constant pool:
   #1 = Utf8               Result
   #2 = Class              #1             // Result
   #3 = Utf8               java/lang/Object
   #4 = Class              #3             // java/lang/Object
   #5 = Utf8               <init>
   #6 = Utf8               ()V
   #7 = NameAndType        #5:#6          // "<init>":()V
   #8 = Methodref          #4.#7          // java/lang/Object."<init>":()V
   #9 = Utf8               this
  #10 = Utf8               LResult;
  #11 = Utf8               (Lkotlin/jvm/internal/DefaultConstructorMarker;)V
  #12 = Methodref          #2.#7          // Result."<init>":()V
  #13 = Utf8               $constructor_marker
  #14 = Utf8               Lkotlin/jvm/internal/DefaultConstructorMarker;
  #15 = Utf8               Lkotlin/Metadata;
  #16 = Utf8               mv
  #17 = Integer            1
  #18 = Integer            4
  #19 = Integer            2
  #20 = Utf8               bv
  #21 = Integer            0
  #22 = Integer            3
  #23 = Utf8               k
  #24 = Utf8               d1
  #25 = Utf8               
  #26 = Utf8               d2
  #27 = Utf8
  #28 = Utf8               Failed
  #29 = Utf8               LResult$Failed;
  #30 = Utf8               Result$Failed
  #31 = Class              #30            // Result$Failed
  #32 = Utf8               Result.kt
  #33 = Utf8               Code
  #34 = Utf8               LineNumberTable
  #35 = Utf8               LocalVariableTable
  #36 = Utf8               InnerClasses
  #37 = Utf8               SourceFile
  #38 = Utf8               RuntimeVisibleAnnotations
{
  private Result();
    descriptor: ()V
    flags: ACC_PRIVATE
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokespecial #8                  // Method java/lang/Object."<init>":()V
         4: return
      LineNumberTable:
        line 1: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       5     0  this   LResult;

  public Result(kotlin.jvm.internal.DefaultConstructorMarker);
    descriptor: (Lkotlin/jvm/internal/DefaultConstructorMarker;)V
    flags: ACC_PUBLIC, ACC_SYNTHETIC
    Code:
      stack=1, locals=2, args_size=2
         0: aload_0
         1: invokespecial #12                 // Method "<init>":()V
         4: return
      LineNumberTable:
        line 1: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       5     0  this   LResult;
            0       5     1 $constructor_marker   Lkotlin/jvm/internal/DefaultConstructorMarker;
}
InnerClasses:
     public static final #28= #31 of #2; //Failed=class Result$Failed of class Result
SourceFile: "Result.kt"
RuntimeVisibleAnnotations:
  0: #15(#16=[I#17,I#18,I#19],#20=[I#17,I#21,I#22],#23=I#17,#24=[s#25],#26=[s#10,s#27,s#6,s#28,s#29])           

#25 in the constant pool contained a bunch of unprintable characters which GitHub did not allow me to paste, here is a hexdump if it's relevant:

00000000  20 20 23 32 35 20 3d 20  55 74 66 38 20 20 20 20  |  #25 = Utf8    |
00000010  20 20 20 20 20 20 20 20  20 20 20 00 10 5c 6e 02  |           ..\n.|
00000020  18 02 5c 6e 02 10 00 5c  6e 02 08 02 5c 6e 02 18  |..\n...\n...\n..|
00000030  02 08 36 18 00 32 02 30  01 3a 01 03 42 07 08 02  |..6..2.0.:..B...|
00000040  c2 a2 06 02 10 02 c2 82  01 01 04 0a              |............|

Java Result.class

Classfile /src/dotty-issues/i12086/standalone/Result.class
  Last modified Apr 13, 2021; size 245 bytes
  MD5 checksum d2bc7d517566b8a0795d50cdf4ebbb44
  Compiled from "Result.java"
public abstract class Result
  minor version: 0
  major version: 52
  flags: ACC_PUBLIC, ACC_SUPER, ACC_ABSTRACT
Constant pool:
   #1 = Methodref          #3.#13         // java/lang/Object."<init>":()V
   #2 = Class              #14            // Result
   #3 = Class              #15            // java/lang/Object
   #4 = Class              #16            // Result$Failed
   #5 = Utf8               Failed
   #6 = Utf8               InnerClasses
   #7 = Utf8               <init>
   #8 = Utf8               ()V
   #9 = Utf8               Code
  #10 = Utf8               LineNumberTable
  #11 = Utf8               SourceFile
  #12 = Utf8               Result.java
  #13 = NameAndType        #7:#8          // "<init>":()V
  #14 = Utf8               Result
  #15 = Utf8               java/lang/Object
  #16 = Utf8               Result$Failed
{
  public Result();
    descriptor: ()V
    flags: ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokespecial #1                  // Method java/lang/Object."<init>":()V
         4: return
      LineNumberTable:
        line 1: 0
}
SourceFile: "Result.java"
InnerClasses:
     public static final #5= #4 of #2; //Failed=class Result$Failed of class Result

which was compiled from

public abstract class Result {
    public static final class Failed extends Result {}
}

by javac 1.8.0_282.

[smarter]

Hmm that looks identical, could you do the same for Result$Failed.class ?

[griggt]

Kotlin Result$Failed.class

Classfile /src/dotty-issues/i12086/standalone/kt/Result$Failed.class
  Last modified Apr 13, 2021; size 549 bytes
  MD5 checksum 9434210661057601cc960cebc40beb0e
  Compiled from "Result.kt"
public final class Result$Failed extends Result
  minor version: 0
  major version: 50
  flags: ACC_PUBLIC, ACC_FINAL, ACC_SUPER
Constant pool:
   #1 = Utf8               Result$Failed
   #2 = Class              #1             // Result$Failed
   #3 = Utf8               Result
   #4 = Class              #3             // Result
   #5 = Utf8               <init>
   #6 = Utf8               ()V
   #7 = Utf8               (Lkotlin/jvm/internal/DefaultConstructorMarker;)V
   #8 = NameAndType        #5:#7          // "<init>":(Lkotlin/jvm/internal/DefaultConstructorMarker;)V
   #9 = Methodref          #4.#8          // Result."<init>":(Lkotlin/jvm/internal/DefaultConstructorMarker;)V
  #10 = Utf8               this
  #11 = Utf8               LResult$Failed;
  #12 = Utf8               Lkotlin/Metadata;
  #13 = Utf8               mv
  #14 = Integer            1
  #15 = Integer            4
  #16 = Integer            2
  #17 = Utf8               bv
  #18 = Integer            0
  #19 = Integer            3
  #20 = Utf8               k
  #21 = Utf8               d1
  #22 = Utf8               
  #23 = Utf8               d2
  #24 = Utf8               LResult;
  #25 = Utf8               Failed
  #26 = Utf8               Result.kt
  #27 = Utf8               Code
  #28 = Utf8               LineNumberTable
  #29 = Utf8               LocalVariableTable
  #30 = Utf8               InnerClasses
  #31 = Utf8               SourceFile
  #32 = Utf8               RuntimeVisibleAnnotations
{
  public Result$Failed();
    descriptor: ()V
    flags: ACC_PUBLIC
    Code:
      stack=2, locals=1, args_size=1
         0: aload_0
         1: aconst_null
         2: invokespecial #9                  // Method Result."<init>":(Lkotlin/jvm/internal/DefaultConstructorMarker;)V
         5: return
      LineNumberTable:
        line 2: 0
        line 2: 2
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       6     0  this   LResult$Failed;
}
InnerClasses:
     public static final #25= #2 of #4; //Failed=class Result$Failed of class Result
SourceFile: "Result.kt"
RuntimeVisibleAnnotations:
  0: #12(#13=[I#14,I#15,I#16],#17=[I#14,I#18,I#19],#20=I#14,#21=[s#22],#23=[s#11,s#24,s#6])

More unprintable chars at #22

Java Result$Failed.class

Classfile /src/dotty-issues/i12086/standalone/Result$Failed.class
  Last modified Apr 13, 2021; size 223 bytes
  MD5 checksum ae33459e26326b921d56fd116fd641eb
  Compiled from "Result.java"
public final class Result$Failed extends Result
  minor version: 0
  major version: 52
  flags: ACC_PUBLIC, ACC_FINAL, ACC_SUPER
Constant pool:
   #1 = Methodref          #3.#10         // Result."<init>":()V
   #2 = Class              #11            // Result$Failed
   #3 = Class              #14            // Result
   #4 = Utf8               <init>
   #5 = Utf8               ()V
   #6 = Utf8               Code
   #7 = Utf8               LineNumberTable
   #8 = Utf8               SourceFile
   #9 = Utf8               Result.java
  #10 = NameAndType        #4:#5          // "<init>":()V
  #11 = Utf8               Result$Failed
  #12 = Utf8               Failed
  #13 = Utf8               InnerClasses
  #14 = Utf8               Result
{
  public Result$Failed();
    descriptor: ()V
    flags: ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokespecial #1                  // Method Result."<init>":()V
         4: return
      LineNumberTable:
        line 2: 0
}
SourceFile: "Result.java"
InnerClasses:
     public static final #12= #2 of #3; //Failed=class Result$Failed of class Result

[smarter]

Oh wait, the issue is probably in the Verifier class, I just found https://youtrack.jetbrains.com/issue/KT-27936 which indicates that kotlin isn't writing InnerClasses attributes for types that it references. So if I'm correct we need to figure out how Scala 2 bypasses the need for this (I guess it assumes everything with a $ in it is an inner class?)

[smarter]

Found it: https://github.com/scala/scala/pull/5822

[griggt]

Yes, the Java version of Verifier.class has

InnerClasses:
     public static final #10= #9 of #18; //Failed=class Result$Failed of class Result

which is missing from the Kotlin version.

[jbwheatley]

hi, is there any update on this? I can't cross-build my library to scala 3 without it 🙁

[bbarker]

Hi @Kordyjan - I just confirmed this is still an issue in recent versions of Scala and Kotlin (3.0.2 and 1.5.30 respectively). This is a bit out of my depth but if you are bogged down I can take a crack at it, since the relevant fix in Scala 2 has already been pointed out, any pointers on where to start in the dotty codebase would probably be a big help.

[smarter]

Thanks for stepping up!

• ClassfileParser also exists in Scala 3 and lives at https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala
• Tests should go in tests/run rather than test/files/run
• There's a brand new guide on contributing to the compiler which should be helpful: https://docs.scala-lang.org/scala3/guides/contribution/contribution-intro.html

[bbarker]

@smarter thanks! One question regarding tests for this: as there are currently neither any .kt or .class files in the dotty source tree, it seems including either would set a new precedent, and for the former, the build would have to be altered to include a kotlin compiler just for this test. What seems a good approach?

[smarter]

If you look at the scala 2 pr, it doesn't add any kt or class file, instead it adds java files that mimic the behavior of kotlin.

[bbarker]

One question regarding the test classes, in the Scala file we have:

val c = new C
assert(Test_2.acceptD(Test_1.mD(new c.D)) == 2)

But, Test_1.mD would somehow be allowed to take an instance of the "C.D" class defined in Test2.java - it seems like there should be some sort of namespace collision issue between the C$D defined in the two `Test*.java` files. Currently dotty emits an error, as others have seen, though this passes in Scala 2:

5 |    assert(Test_2.acceptD(Test_1.mD(new c.D)) == 2)
  |                                    ^^^^^^^
  |                                    Found:    c.D
  |                                    Required: C$D

If we instead change the argument from new c.D to new C$D, presumably referencing the class defined in Test_1.java, we get a slightly different error with dotty, indicating that the acceptD functions at least can tell they have been passed the wrong type:

4 |    assert(Test_2.acceptD(Test_1.mD(new C$D)) == 2)
  |                          ^^^^^^^^^^^^^^^^^^
  |                          Found:    C$D
  |                          Required: C#D

So my current major point of confusion, which may not be very related to the issue at hand, is just how Test_1.mD can seemingly accept values of two different classes; this probably comes down to my not understanding something about namespace collision or class loading.

[smarter]

is just how Test_1.mD can seemingly accept values of two different classes

If I'm understanding things correctly, it doesn't: in Scala 3 currently it only accepts the toplevel class C$D, whereas in Scala 2 it will only accept the class D defined in class C, because of the special casing added in ClassfileParser which changes how the class name C$D is resolved into a symbol.

[bbarker]

In Scala 2.13.5, perplexingly I seem to get the same error as in dotty:

$ scalac --version
Scala compiler version 2.13.5 -- Copyright 2002-2020, LAMP/EPFL and Lightbend, Inc.
bbarker@beb82dell0-DevContainer:~/workspace/dotty/tests/pos/i12086
$ scalac Test_3.scala Test_*.java
Test_3.scala:7: error: type mismatch;
 found   : c.D
 required: C$D
     assert(Test_2.acceptD(Test_1.mD(new c.D)) == 2)
                                     ^
Test_3.scala:8: error: type mismatch;
 found   : C.E
 required: C$E
     assert(Test_2.acceptE(Test_1.mE(new C.E)) == 2)
                                     ^
Test_3.scala:9: error: type mismatch;
 found   : C.F.G
 required: C$F$G
     assert(Test_2.acceptG(Test_1.mG(new C.F.G)) == 2)
                                     ^
 3 errors

But, this is in the run tests directory for Scala 2, so i assume I've compiled this incorreclty

[smarter]

The _X suffixes are used for separate compilation, so Test_1 should be compiled with javac, then Test_2 with a separate call to javac, then Test_3 should be compiled by scalac with the result of the previous steps on the classpath. This is what the test framework does.

[bbarker]

Thanks! baby steps :baby:

I added a bit of logging to try to understand the situation in the dotty code a bit better:

  def debugPrint(name: Name, info: String): Unit =
    if (name.toString == "C" || name.toString == "D" || name.toString == "C.D" || name.toString == "C$D" || name.toString == "c.D" ) {
      System.err.println(info)
    }
  /** Return the class symbol of the given name. */
  def classNameToSymbol(name: Name)(using Context): Symbol = innerClasses.get(name.toString) match {
    case Some(entry) =>
      debugPrint(name, s"classNameToSymbol (with inner) name=$name has symbol ${innerClasses.classSymbol(entry)}")
      innerClasses.classSymbol(entry)
    case None =>
      debugPrint(name, s"classNameToSymbol (no inner) name=$name has symbol ${requiredClass(name)}")
      requiredClass(name)
  }

For the line assert(Test_2.acceptD(Test_1.mD(new c.D)) == 2) in Test_3.scala, this results in:

classNameToSymbol (no inner) name=C has symbol class C
classNameToSymbol (with inner) name=C$D has symbol class D
classNameToSymbol (no inner) name=C has symbol class C
classNameToSymbol (no inner) name=C$D has symbol class C$D
classNameToSymbol (no inner) name=C$D has symbol class C$D
classNameToSymbol (no inner) name=C has symbol class C

So if I understand right, the two lines:

classNameToSymbol (no inner) name=C$D has symbol class C$D
classNameToSymbol (no inner) name=C$D has symbol class C$D

should actually be reading

classNameToSymbol (no inner) name=C$D has symbol class C.D
classNameToSymbol (no inner) name=C$D has symbol class C.D

?

Thanks again

[bbarker]

Ooops, no, I think it should actually be this:

Current:

classNameToSymbol (with inner) name=C$D has symbol class D

Desired:

classNameToSymbol (with inner) name=C$D has symbol class C.D

[smarter]

The toString on a class will print just the class name regardless of where it's defined, so it'll be "class D", not "class C.D", and it's the one whose currently interpreted as "class C$D" that needs to be seen as D inside C instead.

[bbarker]

OK, getting a bit better grasp on this but wanted to check on my planned approach for manipulating the Name:

1. Work with Strings directly as in the Scala 2 code (Name.toString)
2. Convert the split strings into SimpleNames using def termName(s: String): SimpleName
3. Map these back into the original name using something like (Name.mapParts).

[smarter]

I don't think you need to use mapParts, you can directly go from a String to a Name via toTypeName.

[bbarker]

I have a branch with a couple of commits. The first commit attempted to demonstrate a very basic fix for the specific case of C$D; the second commit attempted to do things the right way (more or less) and port over the logic from Scala 2, but there are still some problems.

Primarily, on the following line, if I remove the requiredClass call and instead use what is commented, the compiler seems to break badly.

 case _ => requiredClass(localizedName).copy(owner = owner) // instanceScope.lookup(innerName)

As it is, there are still errors for some of the more heavily nested cases:

classNameToSymbol (no inner) name=C has symbol class C and loc=class C
classNameToSymbol (with inner) name=C$D has symbol class D and loc=class D in class C
classNameToSymbol (no inner) name=C$D has symbol class D and loc=class D in class C
classNameToSymbol (no inner) name=C$D has symbol class D and loc=class D in class C
classNameToSymbol (no inner) name=C has symbol class C and loc=class C
Bad symbolic reference. A signature
refers to D/T in package C which is not available.
It may be completely missing from the current classpath, or the version on
the classpath might be incompatible with the version used when compiling the signature.
Bad symbolic reference. A signature
refers to D/T in package C which is not available.
It may be completely missing from the current classpath, or the version on
the classpath might be incompatible with the version used when compiling the signature.
classNameToSymbol (no inner) name=C has symbol class C and loc=class C
-- [E007] Type Mismatch Error: tests/run/i12086/Test_3.scala:5:36 ------------------------------------------------------
5 |    assert(Test_2.acceptE(Test_1.mE(new C.E)) == 2)
  |                                    ^^^^^^^
  |                                    Found:    C.E
  |                                    Required: C#E?
  |
  |                                    where:    E  is a class in object C
  |                                              E? is a class in class C
Bad symbolic reference. A signature
refers to E/T in package C which is not available.
It may be completely missing from the current classpath, or the version on
the classpath might be incompatible with the version used when compiling the signature.
Bad symbolic reference. A signature
refers to E/T in package C which is not available.
It may be completely missing from the current classpath, or the version on
the classpath might be incompatible with the version used when compiling the signature.
classNameToSymbol (no inner) name=C has symbol class C and loc=class C
-- [E007] Type Mismatch Error: tests/run/i12086/Test_3.scala:6:36 ------------------------------------------------------
6 |    assert(Test_2.acceptG(Test_1.mG(new C.F.G)) == 2)
  |                                    ^^^^^^^^^
  |                                    Found:    C.F.G
  |                                    Required: C$F#G?
  |
  |                                    where:    G  is a class in object F
  |                                              G? is a class in class C$F

[smarter]

I've opened a PR which fixes this at https://github.com/lampepfl/dotty/pull/14426 based on the initial work by @bbarker, thanks a lot for the help!

[bbarker]

Thank you @smarter !