janino-compiler / janino

Janino is a super-small, super-fast Java™ compiler.
http://janino-compiler.github.io/janino
Other
1.21k stars 205 forks source link

Incompatible return types, when there are multiple methods with different return type #182

Closed eejbyfeldt closed 1 year ago

eejbyfeldt commented 1 year ago

I was not able to reproduce this error with only java code. Perhaps someone with more java knowledge could figure out what might produce similar bytecode. But the example below will sadly involve using scalac to reproduce the issue.

Scala allows one to override a method returning Any with a return value of Int when this is done like this

package example

trait Base {
  def value: Any
}

class Derived extends Base {
  def value: Int = 5
}

The interface of the compiled class Derived becomes

$ javap -p example/Derived.class 
Compiled from "overload_int.scala"
public class example.Derived implements example.Base {
  public int value();
  public java.lang.Object value();
  public example.Derived();
}

So there are two methods called value which takes no arguments, one returning int and one returning Object.

Then we want to call the value method from a java file:

$ cat CallValue.java 
public class CallValue {
    public static void main(String[] args) {
    int value = new example.Derived().value();
    }
}

Compiling this code using openjdk works fine. But trying to compile it using janino 3.1.7 using the SimplerCompiler results in a Incompatible return types exception:

$ java -cp commons-compiler-3.1.7.jar:janino-3.1.7.jar:. org.codehaus.janino.SimpleCompiler CallValue.java -
Exception in thread "main" org.codehaus.commons.compiler.InternalCompilerException: Compiling "CallValue" in File 'CallValue.java', Line 1, Column 8: File 'CallValue.java', Line 2, Column 24: Compiling "main(String[] args)": File 'CallValue.java', Line 3, Column 13
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:375)
    at org.codehaus.janino.UnitCompiler.access$000(UnitCompiler.java:234)
    at org.codehaus.janino.UnitCompiler$1.visitCompilationUnit(UnitCompiler.java:339)
    at org.codehaus.janino.UnitCompiler$1.visitCompilationUnit(UnitCompiler.java:336)
    at org.codehaus.janino.Java$CompilationUnit.accept(Java.java:370)
    at org.codehaus.janino.UnitCompiler.compileUnit(UnitCompiler.java:336)
    at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:254)
    at org.codehaus.janino.SimpleCompiler.compileToClassLoader(SimpleCompiler.java:491)
    at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:232)
    at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:210)
    at org.codehaus.commons.compiler.Cookable.cook(Cookable.java:70)
    at org.codehaus.commons.compiler.Cookable.cook(Cookable.java:59)
    at org.codehaus.janino.SimpleCompiler.<init>(SimpleCompiler.java:149)
    at org.codehaus.janino.SimpleCompiler.main(SimpleCompiler.java:112)
Caused by: org.codehaus.commons.compiler.InternalCompilerException: File 'CallValue.java', Line 2, Column 24: Compiling "main(String[] args)": File 'CallValue.java', Line 3, Column 13
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:3239)
    at org.codehaus.janino.UnitCompiler.compileDeclaredMethods(UnitCompiler.java:1389)
    at org.codehaus.janino.UnitCompiler.compileDeclaredMethods(UnitCompiler.java:1362)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:806)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:418)
    at org.codehaus.janino.UnitCompiler.access$400(UnitCompiler.java:234)
    at org.codehaus.janino.UnitCompiler$2.visitPackageMemberClassDeclaration(UnitCompiler.java:397)
    at org.codehaus.janino.UnitCompiler$2.visitPackageMemberClassDeclaration(UnitCompiler.java:392)
    at org.codehaus.janino.Java$PackageMemberClassDeclaration.accept(Java.java:1687)
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:392)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:365)
    ... 13 more
Caused by: java.lang.RuntimeException: File 'CallValue.java', Line 3, Column 13
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:2655)
    at org.codehaus.janino.UnitCompiler.access$2700(UnitCompiler.java:234)
    at org.codehaus.janino.UnitCompiler$5.visitLocalVariableDeclarationStatement(UnitCompiler.java:1537)
    at org.codehaus.janino.UnitCompiler$5.visitLocalVariableDeclarationStatement(UnitCompiler.java:1521)
    at org.codehaus.janino.Java$LocalVariableDeclarationStatement.accept(Java.java:3842)
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:1521)
    at org.codehaus.janino.UnitCompiler.compileStatements(UnitCompiler.java:1605)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:3560)
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:3235)
    ... 23 more
Caused by: org.codehaus.commons.compiler.InternalCompilerException: File 'CallValue.java', Line 3, Column 43: Compiling "new example.Derived().value()": Incompatible return types
    at org.codehaus.janino.UnitCompiler.compileGetValue(UnitCompiler.java:5765)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:2638)
    ... 31 more
Caused by: org.codehaus.commons.compiler.InternalCompilerException: Incompatible return types
    at org.codehaus.janino.UnitCompiler.findMostSpecificIInvocable(UnitCompiler.java:9724)
    at org.codehaus.janino.UnitCompiler.findMostSpecificIInvocable(UnitCompiler.java:9484)
    at org.codehaus.janino.UnitCompiler.findIMethod(UnitCompiler.java:9366)
    at org.codehaus.janino.UnitCompiler.findIMethod(UnitCompiler.java:9282)
    at org.codehaus.janino.UnitCompiler.compileGet2(UnitCompiler.java:5118)
    at org.codehaus.janino.UnitCompiler.access$9200(UnitCompiler.java:234)
    at org.codehaus.janino.UnitCompiler$13.visitMethodInvocation(UnitCompiler.java:4620)
    at org.codehaus.janino.UnitCompiler$13.visitMethodInvocation(UnitCompiler.java:4593)
    at org.codehaus.janino.Java$MethodInvocation.accept(Java.java:5459)
    at org.codehaus.janino.UnitCompiler.compileGet(UnitCompiler.java:4593)
    at org.codehaus.janino.UnitCompiler.compileGetValue(UnitCompiler.java:5763)
    ... 32 more

Changing the java code slightly to expect a Object instead of int

$ cat CallValue2.java 
public class CallValue2 {
    public static void main(String[] args) {
    Object value = new example.Derived().value();
    }
}

also compiles fine using openjdk but gives the same error when using janino.

The java version used:

$ java --version
openjdk 17.0.4.1 2022-08-12
OpenJDK Runtime Environment (Red_Hat-17.0.4.1.1-1.fc36) (build 17.0.4.1+1)
OpenJDK 64-Bit Server VM (Red_Hat-17.0.4.1.1-1.fc36) (build 17.0.4.1+1, mixed mode, sharing)

Scala version used:

$ scalac --version
Scala compiler version 2.13.8 -- Copyright 2002-2021, LAMP/EPFL and Lightbend, Inc.

Also some indication that we are not the only one hitting this issue: https://stackoverflow.com/questions/63250313/create-dataset-for-case-class-that-implements-a-generic-trait

aunkrig commented 1 year ago

Hey Emil,

you are right, that problem cannot be reproduced with Java code alone, because Scala produces bytecode here that a Java compiler would never generate. Instead it must (!) issue an error message "The return type is incompatible with ReportedBugsTest.Base.value()", because the JLS dictates that the return type of the overriding method must be a subtype of the return type of the overridden method, and int is not a subtype of Object. In other words: This is a limitation that is enforced by the Java compiler, not the JVM.

Consequently, JAVAC should refuse to compile CallValue2 just like Janino does. Did you try that=

eejbyfeldt commented 1 year ago

I tried 4 different version of javac from openjdk:

$ javac -version
javac 18.0.2

$ javac -version
javac 17.0.4.1

$ javac -version
javac 11.0.16.1

$ javac -version
javac 1.8.0_345

All of them compile CallValue2.java and CallValue.java without issues. With CallValue2.java javac spits out code that calls the function value() that returns int and then wraps that into an java.lang.Integer at the call site. But I guess it not clear if this intentional behavior or just happens to be the current behavior.

Just to be clear janino fails to compile both CallValue and CallValue2. I think I first read the janino exception as the return type is incompatible with the expected return type on the callsite. But I should be read as it thinks method defined on the Derived class is compatible with the one defined on Base?

aunkrig commented 1 year ago

I think I first read the janino exception as the return type is incompatible with the expected return type on the callsite. But I should be read as it thinks method defined on the Derived class is compatible with the one defined on Base?

Absolutely: Janino expects the return types of covariant methods to be strictly assignment compatible, e.g. the base method returns List, and the overriding method returns ArrayList. Otherwise it issues that InternalCompilerException.

As I read JLS11 8.4.5, this behavior is correct.

However, JAVAC seems to also allow boxing conversion compatibility for covariant return types. For compatibility with JAVAC, Janino now also does:

// For compatibility with SCALA, also allow for boxing conversion, e.g. return types
// "int" and "Object" are comnpatible, and "int" is more specific. See issue #182.
//         if (m.getReturnType().isAssignableFrom(theNonAbstractMethod.getReturnType())) {
         if (this.isMethodInvocationConvertible(theNonAbstractMethod.getReturnType(), m.getReturnType(), boxingPermitted)) {
             ;
         } else

// For compatibility with SCALA, also allow for boxing conversion, e.g. return types
// "int" and "Object" are comnpatible, and "int" is more specific. See issue #182.
//         if (theNonAbstractMethod.getReturnType().isAssignableFrom(m.getReturnType())) {
         if (this.isMethodInvocationConvertible(m.getReturnType(), theNonAbstractMethod.getReturnType(), boxingPermitted)) {
             theNonAbstractMethod = m;
         } else

This passes all regression tests.

Please test if that fixes your problem.

eejbyfeldt commented 1 year ago

I tried with 2e0cc5fc6aa8b8020a84437a0ac4a7befb99969b but that still gives the same compile error before.

I belive the problems is that place where we call findMostSpecificIInvocable https://github.com/janino-compiler/janino/blob/2e0cc5fc6aa8b8020a84437a0ac4a7befb99969b/janino/src/main/java/org/codehaus/janino/UnitCompiler.java#L9555-L9561 we call it first with boxingPermitted=false and when boxing is not permitted we end up with the same InternalCompilerException as before.

aunkrig commented 1 year ago

Hey Emil, I made yet another attempt to fix the problem. Please re-test.

eejbyfeldt commented 1 year ago

I tried with https://github.com/janino-compiler/janino/commit/e69022f5aaabd36edc08a2074360d62514493a19 and that seems to compile and run both examples in the same way that openjdk does.

$ java -cp commons-compiler-3.1.10-SNAPSHOT.jar:janino-3.1.10-SNAPSHOT.jar:. org.codehaus.janino.SimpleCompiler CallValue.java CallValue
5
$ java -cp commons-compiler-3.1.10-SNAPSHOT.jar:janino-3.1.10-SNAPSHOT.jar:. org.codehaus.janino.SimpleCompiler CallValue2.java CallValue2
5

Great work!

eejbyfeldt commented 9 months ago

@aunkrig I think we might want to reopen this ticket. I found some similar cases that still gives the previous error.

This code:

$ cat BoxedValueClass.scala 
package example

trait BaseValueClassTrait extends Any
case class IntWrapper(i: Int) extends AnyVal with BaseValueClassTrait

trait BaseTrait {
  def value: BaseValueClassTrait
}
case class BaseAccessor(value: IntWrapper) extends BaseTrait

will generate a class file with the following methods

$ javap -p example/BaseAccessor.class  | grep value
  private final int value;
  public int value();
  public example.BaseValueClassTrait value();

Then the following java code

$ cat UseAccessor.java 
public class UseAccessor {
    public static void main(String[] args) {
    int value = new example.BaseAccessor(6).value();
    System.out.println(value);
    }
}

compiles and runs without issues on openjdk

$ java -version
openjdk version "17.0.8" 2023-07-18
OpenJDK Runtime Environment (Red_Hat-17.0.8.0.7-1.fc38) (build 17.0.8+7)
OpenJDK 64-Bit Server VM (Red_Hat-17.0.8.0.7-1.fc38) (build 17.0.8+7, mixed mode, sharing)
$ java -cp /usr/share/java/scala/scala-library.jar:. UseAccessor.java
6

But testing it with 3.1.10 of janino gives

$ java -cp /usr/share/java/scala/scala-library.jar:commons-compiler-3.1.10.jar:janino-3.1.10.jar:. org.codehaus.janino.SimpleCompiler UseAccessor.java UseAccessor
Exception in thread "main" org.codehaus.commons.compiler.InternalCompilerException: Compiling "UseAccessor" in File 'UseAccessor.java', Line 1, Column 8: File 'UseAccessor.java', Line 2, Column 24: Compiling "main(String[] args)"
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:403)
    at org.codehaus.janino.UnitCompiler.access$000(UnitCompiler.java:237)
    at org.codehaus.janino.UnitCompiler$2.visitCompilationUnit(UnitCompiler.java:364)
    at org.codehaus.janino.UnitCompiler$2.visitCompilationUnit(UnitCompiler.java:362)
    at org.codehaus.janino.Java$CompilationUnit.accept(Java.java:371)
    at org.codehaus.janino.UnitCompiler.compileUnit(UnitCompiler.java:362)
    at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:273)
    at org.codehaus.janino.SimpleCompiler.compileToClassLoader(SimpleCompiler.java:526)
    at org.codehaus.janino.SimpleCompiler.cook2(SimpleCompiler.java:250)
    at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:229)
    at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:219)
    at org.codehaus.commons.compiler.Cookable.cook(Cookable.java:70)
    at org.codehaus.commons.compiler.Cookable.cook(Cookable.java:59)
    at org.codehaus.janino.SimpleCompiler.<init>(SimpleCompiler.java:158)
    at org.codehaus.janino.SimpleCompiler.main(SimpleCompiler.java:121)
Caused by: org.codehaus.commons.compiler.InternalCompilerException: File 'UseAccessor.java', Line 2, Column 24: Compiling "main(String[] args)"
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:3334)
    at org.codehaus.janino.UnitCompiler.compileDeclaredMethods(UnitCompiler.java:1448)
    at org.codehaus.janino.UnitCompiler.compileDeclaredMethods(UnitCompiler.java:1421)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:830)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:443)
    at org.codehaus.janino.UnitCompiler.access$400(UnitCompiler.java:237)
    at org.codehaus.janino.UnitCompiler$3.visitPackageMemberClassDeclaration(UnitCompiler.java:423)
    at org.codehaus.janino.UnitCompiler$3.visitPackageMemberClassDeclaration(UnitCompiler.java:419)
    at org.codehaus.janino.Java$PackageMemberClassDeclaration.accept(Java.java:1688)
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:419)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:393)
    ... 14 more
Caused by: java.lang.RuntimeException: File 'UseAccessor.java', Line 3, Column 9
    at org.codehaus.janino.UnitCompiler.compileStatements(UnitCompiler.java:1664)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:3665)
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:3330)
    ... 24 more
Caused by: java.lang.RuntimeException: File 'UseAccessor.java', Line 3, Column 9
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:1605)
    at org.codehaus.janino.UnitCompiler.compileStatements(UnitCompiler.java:1662)
    ... 26 more
Caused by: java.lang.RuntimeException: File 'UseAccessor.java', Line 3, Column 13
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:2746)
    at org.codehaus.janino.UnitCompiler.access$2700(UnitCompiler.java:237)
    at org.codehaus.janino.UnitCompiler$6.visitLocalVariableDeclarationStatement(UnitCompiler.java:1590)
    at org.codehaus.janino.UnitCompiler$6.visitLocalVariableDeclarationStatement(UnitCompiler.java:1576)
    at org.codehaus.janino.Java$LocalVariableDeclarationStatement.accept(Java.java:3842)
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:1576)
    ... 27 more
Caused by: org.codehaus.commons.compiler.InternalCompilerException: File 'UseAccessor.java', Line 3, Column 49: Compiling "new example.BaseAccessor(6).value()"
    at org.codehaus.janino.UnitCompiler.compileGetValue(UnitCompiler.java:5887)
    at org.codehaus.janino.UnitCompiler.access$3800(UnitCompiler.java:237)
    at org.codehaus.janino.UnitCompiler$7.visitRvalue(UnitCompiler.java:2767)
    at org.codehaus.janino.UnitCompiler$7.visitRvalue(UnitCompiler.java:2755)
    at org.codehaus.janino.Java$Rvalue.accept(Java.java:4498)
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:2755)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:2742)
    ... 32 more
Caused by: org.codehaus.commons.compiler.InternalCompilerException: File 'UseAccessor.java', Line 3, Column 49: Incompatible return types
    at org.codehaus.janino.UnitCompiler.findMostSpecificIInvocable(UnitCompiler.java:9874)
    at org.codehaus.janino.UnitCompiler.findMostSpecificIInvocable(UnitCompiler.java:9624)
    at org.codehaus.janino.UnitCompiler.findIMethod(UnitCompiler.java:9506)
    at org.codehaus.janino.UnitCompiler.findIMethod(UnitCompiler.java:9422)
    at org.codehaus.janino.UnitCompiler.compileGet2(UnitCompiler.java:5263)
    at org.codehaus.janino.UnitCompiler.access$9300(UnitCompiler.java:237)
    at org.codehaus.janino.UnitCompiler$16.visitMethodInvocation(UnitCompiler.java:4766)
    at org.codehaus.janino.UnitCompiler$16.visitMethodInvocation(UnitCompiler.java:4742)
    at org.codehaus.janino.Java$MethodInvocation.accept(Java.java:5470)
    at org.codehaus.janino.UnitCompiler.compileGet(UnitCompiler.java:4742)
    at org.codehaus.janino.UnitCompiler.compileGetValue(UnitCompiler.java:5885)
    ... 38 more
eejbyfeldt commented 8 months ago

I found this comment in the openjdk source code: https://github.com/openjdk/jdk8u/blob/7694663d58434196368193c66225af10e60e1329/jdk/src/share/classes/java/lang/Class.java#L1743-L1752 so it clear by design that the jvm support this flexibility that is not used by the java language. So I guess it depends on which interoperability you want janino to provide with jvm languages using this flexibility.