search

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

Some expressions are failing without explicit cast of null #188

Closed snuyanzin closed 1 year ago

snuyanzin commented 1 year ago

After update to Calcite 1.28 (janino 3.1.6) there is a failure (also reproducible with 3.1.8). It complains for these lines from generated code

/* 91 */          externalResult$104343 = (java.lang.String) function_org$apache$flink$table$planner$runtime$stream$sql$FunctionITCase$ClassNameScalarFunction
/* 92 */            .eval(true ? null : ((java.lang.String) converter$104341.toExternal((org.apache.flink.table.data.binary.BinaryStringData) ((org.apache.flink.table.data.binary.BinaryStringData) org.apache.flink.table.data.binary.BinaryStringData.EMPTY_UTF8))));

The interesting thing is that if I put explicit cast of null to the expected type then the issue is disappeared.

the full stacktrace is 

Caused by: org.codehaus.commons.compiler.InternalCompilerException: Compiling "ExpressionReducer$104353" in Line 5, Column 14: Line 43, Column 23: Compiling "map(Object _in1)"
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:402)
    at org.codehaus.janino.UnitCompiler.access$000(UnitCompiler.java:236)
    at org.codehaus.janino.UnitCompiler$2.visitCompilationUnit(UnitCompiler.java:363)
    at org.codehaus.janino.UnitCompiler$2.visitCompilationUnit(UnitCompiler.java:361)
    at org.codehaus.janino.Java$CompilationUnit.accept(Java.java:371)
    at org.codehaus.janino.UnitCompiler.compileUnit(UnitCompiler.java:361)
    at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:264)
    at org.codehaus.janino.SimpleCompiler.compileToClassLoader(SimpleCompiler.java:517)
    at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:241)
    at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:219)
    at org.codehaus.commons.compiler.Cookable.cook(Cookable.java:82)
    at org.codehaus.commons.compiler.Cookable.cook(Cookable.java:77)
    at org.apache.flink.table.runtime.generated.CompileUtils.doCompile(CompileUtils.java:104)
    ... 100 more
Caused by: org.codehaus.commons.compiler.InternalCompilerException: Line 43, Column 23: Compiling "map(Object _in1)"
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:3296)
    at org.codehaus.janino.UnitCompiler.compileDeclaredMethods(UnitCompiler.java:1430)
    at org.codehaus.janino.UnitCompiler.compileDeclaredMethods(UnitCompiler.java:1403)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:829)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:442)
    at org.codehaus.janino.UnitCompiler.access$400(UnitCompiler.java:236)
    at org.codehaus.janino.UnitCompiler$3.visitPackageMemberClassDeclaration(UnitCompiler.java:422)
    at org.codehaus.janino.UnitCompiler$3.visitPackageMemberClassDeclaration(UnitCompiler.java:418)
    at org.codehaus.janino.Java$PackageMemberClassDeclaration.accept(Java.java:1688)
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:418)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:392)
    ... 112 more

Caused by: org.codehaus.commons.compiler.InternalCompilerException: Line 91, Column 11
    at org.codehaus.janino.UnitCompiler.compileStatements(UnitCompiler.java:1648)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:3621)
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:3292)
    ... 122 more
Caused by: java.lang.AssertionError: null
    at org.codehaus.janino.CodeContext.popObjectOrUninitializedOrUninitializedThisOperand(CodeContext.java:1602)
    at org.codehaus.janino.CodeContext.popOperandAssignableTo(CodeContext.java:1538)
    at org.codehaus.janino.UnitCompiler.invoke(UnitCompiler.java:12417)
    at org.codehaus.janino.UnitCompiler.invokeMethod(UnitCompiler.java:13101)
    at org.codehaus.janino.UnitCompiler.compileGet2(UnitCompiler.java:5351)
    at org.codehaus.janino.UnitCompiler.access$9300(UnitCompiler.java:236)
    at org.codehaus.janino.UnitCompiler$16.visitMethodInvocation(UnitCompiler.java:4698)
    at org.codehaus.janino.UnitCompiler$16.visitMethodInvocation(UnitCompiler.java:4674)
    at org.codehaus.janino.Java$MethodInvocation.accept(Java.java:5470)
    at org.codehaus.janino.UnitCompiler.compileGet(UnitCompiler.java:4674)
    at org.codehaus.janino.UnitCompiler.compileGetValue(UnitCompiler.java:5817)
    at org.codehaus.janino.UnitCompiler.compileGet2(UnitCompiler.java:5161)
    at org.codehaus.janino.UnitCompiler.access$8800(UnitCompiler.java:236)
    at org.codehaus.janino.UnitCompiler$16.visitCast(UnitCompiler.java:4693)
    at org.codehaus.janino.UnitCompiler$16.visitCast(UnitCompiler.java:4674)
    at org.codehaus.janino.Java$Cast.accept(Java.java:5283)
    at org.codehaus.janino.UnitCompiler.compileGet(UnitCompiler.java:4674)
    at org.codehaus.janino.UnitCompiler.compileGetValue(UnitCompiler.java:5817)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:4064)
    at org.codehaus.janino.UnitCompiler.access$6300(UnitCompiler.java:236)
    at org.codehaus.janino.UnitCompiler$13.visitAssignment(UnitCompiler.java:4020)
    at org.codehaus.janino.UnitCompiler$13.visitAssignment(UnitCompiler.java:4003)
    at org.codehaus.janino.Java$Assignment.accept(Java.java:4864)
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:4003)
    at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:2487)
    at org.codehaus.janino.UnitCompiler.access$1800(UnitCompiler.java:236)
    at org.codehaus.janino.UnitCompiler$6.visitExpressionStatement(UnitCompiler.java:1563)
    at org.codehaus.janino.UnitCompiler$6.visitExpressionStatement(UnitCompiler.java:1558)
    at org.codehaus.janino.Java$ExpressionStatement.accept(Java.java:3209)
    at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:1558)
    at org.codehaus.janino.UnitCompiler.compileStatements(UnitCompiler.java:1644)
    ... 124 more
aunkrig commented 1 year ago

Reproduces with

Object o = true ? null : "";

See org.codehaus.commons.compiler.tests.ReportedBugsTest.testIssue188().

aunkrig commented 1 year ago

Fixed it... please test!

snuyanzin commented 1 year ago

amazing, thank you will have a look soon

snuyanzin commented 1 year ago

Flink tests are still failing with this issue. It looks like it is fixed for Object o = true ? null : "";

However it is still reproducible when this expression is a passing arg like

String s1 = new StringBuilder().append(true ? (String) null : "abc").toString(); // ok
String s2 = new StringBuilder().append(true ? null : "abc").toString(); // <= InternalCompilerException
aunkrig commented 1 year ago

Sigh... I’ll look into it.

snuyanzin commented 1 year ago

Hi @aunkrig I just want to share some findings regarding this issue. After some debugging I realized that such condition 

if (cv == null && rv instanceof ConditionalExpression) {
    this.tryNullConversion(IClass.NULL, this.getType2((ConditionalExpression)rv));
}

in org.codehaus.janino.UnitCompiler#compileGetValue(org.codehaus.janino.Java.Rvalue) helps to pass tests. Also there is a commit in my fork, where changes could be seen https://github.com/snuyanzin/janino/commit/2dff5b4030e617223cc81819f7ace4951eabc5e7

WDYT how bad/good is this approach?

aunkrig commented 1 year ago

I fixed it differently... please test.

snuyanzin commented 1 year ago

It behaves a bit strange.

From one side mvn clean verify for janino-parent fails with

Tests in error: 
  ReportedBugsTest.testIssue188:1662->CommonsCompilerTestSuite.assertScriptExecutable:215 » Compile

on lines

this.assertScriptExecutable("String s1 = new StringBuilder().append(true ? (String) null : \"abc\").toString(); // ok");
this.assertScriptExecutable("String s2 = new StringBuilder().append(true ? null : \"abc\").toString(); // <= InternalCompilerException");

At the same time now it passes Flink tests...

aunkrig commented 1 year ago

Strange... in my development environment, a different test fails now: org.codehaus.commons.compiler.tests.JlsTest.test_9_3_1__Initialization_of_Fields_in_Interfaces__2(JlsTest.java:802) this.assertClassBodyUncookable("public final static float x = 0D;"); fails with the JDK (not the Janino!) implementation with JRE 8: Line 1, Column 31: incompatible types: possible lossy conversion from double to float (compiler.err.prob.found.req) The reason being is that the compile error handling of the JDK implementation chokes on the first compilation error: The reason appears to be that org.codehaus.commons.compiler.jdk.Compiler.compile(...).new DiagnosticListener() {...}.report(Diagnostic<? extends JavaFileObject>) (Line 350) attempts to "tunnel" the compilation error through the JDK compiler via a RuntimeException, and that doesn't work (at least with JDK 8), because com.sun.tools.javac.code.Symbol.VarSymbol.getConstValue() catches Exception and throws an AssertionError.

But now let's turn to testIssue188().

aunkrig commented 1 year ago

Ah, found the reason for the testIssue188() test error: JAVAC does not like source code that ends with a C++-style comment and no line terminator.

Change test case accordingly.

Please test.

snuyanzin commented 1 year ago

checked locally now both Janino and Flink tests are passing