Naked switch inside "if" or "else" does not compile

cueman commented 2 years ago

GWT 2.10.0 Browser: n/a Operating System: Linux (Ubuntu 22.04) Java: OpenJDK 17.0.3 or 1.8.0_312

Description

Suppose you have code like this:

enum MyEnum {A, B, C}

class MyTest
{
  void doStuff(boolean b, MyEnum e)
  {
    boolean b = getValue();
    if (b)
      switch (e)
      {
        case A: 
            doSomething();
            break;
        case B: 
            doSomethingElse();
            break;
        case C: 
            doSomethingMore();
            break;
      }
  }
}

then compiling with GWT 2.10.0 with OpenJDK 17.0.3 gives:

An internal compiler exception occurred
     [java] com.google.gwt.dev.jjs.InternalCompilerException: Error constructing Java AST
     [java]     at com.google.gwt.dev.jjs.impl.GwtAstBuilder.translateException(GwtAstBuilder.java:4033)
     [java]     at com.google.gwt.dev.jjs.impl.GwtAstBuilder$AstVisitor.endVisit(GwtAstBuilder.java:1036)
     [java]     at org.eclipse.jdt.internal.compiler.ast.IfStatement.traverse(IfStatement.java:297)
     [java]     at org.eclipse.jdt.internal.compiler.ast.MethodDeclaration.traverse(MethodDeclaration.java:365)
     [java]     at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.traverse(TypeDeclaration.java:1466)
     [java]     at com.google.gwt.dev.jjs.impl.GwtAstBuilder.processImpl(GwtAstBuilder.java:3969)
     [java]     at com.google.gwt.dev.jjs.impl.GwtAstBuilder.process(GwtAstBuilder.java:4007)
     [java]     at com.google.gwt.dev.javac.CompilationStateBuilder$CompileMoreLater$UnitProcessorImpl.process(CompilationStateBuilder.java:128)
     [java]     at com.google.gwt.dev.javac.JdtCompiler$CompilerImpl.process(JdtCompiler.java:322)
     [java]     at org.eclipse.jdt.internal.compiler.Compiler.processCompiledUnits(Compiler.java:575)
     [java]     at org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:475)
     [java]     at org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:426)
     [java]     at com.google.gwt.dev.javac.JdtCompiler.doCompile(JdtCompiler.java:1020)
     [java]     at com.google.gwt.dev.javac.CompilationStateBuilder$CompileMoreLater.compile(CompilationStateBuilder.java:322)
     [java]     at com.google.gwt.dev.javac.CompilationStateBuilder.doBuildFrom(CompilationStateBuilder.java:532)
     [java]     at com.google.gwt.dev.javac.CompilationStateBuilder.buildFrom(CompilationStateBuilder.java:464)
     [java]     at com.google.gwt.dev.cfg.ModuleDef.getCompilationState(ModuleDef.java:423)
     [java]     at com.google.gwt.dev.Precompile.precompile(Precompile.java:210)
     [java]     at com.google.gwt.dev.Precompile.precompile(Precompile.java:190)
     [java]     at com.google.gwt.dev.Precompile.precompile(Precompile.java:131)
     [java]     at com.google.gwt.dev.Compiler.compile(Compiler.java:192)
     [java]     at com.google.gwt.dev.Compiler.compile(Compiler.java:143)
     [java]     at com.google.gwt.dev.Compiler.compile(Compiler.java:132)
     [java]     at com.google.gwt.dev.Compiler$1.run(Compiler.java:110)
     [java]     at com.google.gwt.dev.CompileTaskRunner.doRun(CompileTaskRunner.java:55)
     [java]     at com.google.gwt.dev.CompileTaskRunner.runWithAppropriateLogger(CompileTaskRunner.java:50)
     [java]     at com.google.gwt.dev.Compiler.main(Compiler.java:113)
     [java] Caused by: java.lang.ClassCastException: class com.google.gwt.dev.jjs.ast.JSwitchStatement cannot be cast to class com.google.gwt.dev.jjs.ast.JExpression (com.google.gwt.dev.jjs.ast.JSwitchStatement and com.google.gwt.dev.jjs.ast.JExpression are in unnamed module of loader 'app')
     [java]     at com.google.gwt.dev.jjs.impl.GwtAstBuilder$AstVisitor.pop(GwtAstBuilder.java:2675)
     [java]     at com.google.gwt.dev.jjs.impl.GwtAstBuilder$AstVisitor.endVisit(GwtAstBuilder.java:1032)
     [java]     ... 25 more

Compiling with Java 8 gives the same stack trace, but without reference to "unnamed module of loader 'app'".

This same code worked in GWT 2.9.0

Steps to reproduce

Any code that includes this:

if (something)
  switch (somethingElse)

or that includes:

if (something)
  doSomething();
else
  switch (somethingElse)

will generate the above exception.

Known workarounds

Put curly brackets around the switch statement and it compiles correctly. So the above example would become this:

if (something)
{
  switch(somethingElse)
  ...
}

jnehlmeier commented 2 years ago

Interesting. I am using GWT from master branch for quite some time now but code base always has curly brackets so I never noticed it.

I just tried to reproduce it using SuperDevMode but couldn't. I have done something like

void onSomethingHappened(EnumConstant x) {
  if (x != null) // added to try triggering the issue
    switch(x) {
      case A: ...
    }
}

Does it work in SuperDevMode for you, or does it always fail regardless of SDM or production compile?

Can you double check that you only have a single GWT SDK version in your classpath?

cueman commented 2 years ago

I had not yet tried it in SuperDevMode when I created the issue. I've just tried it, and it works the way it should. It only fails in a production compile.

I checked for multiple GWT SDK instances, and there is no older version around.

jnehlmeier commented 2 years ago

I tried compiling my app with some code similar to yours and it did not fail. Do you mind creating an example project reproducing the issue?

cueman commented 2 years ago

You're right. I created a dummy app from scratch, and it did not fail.

I'm having trouble identifying the difference between a working and a non-working project. It must depend on something subtle. I'll continue to try.

cueman commented 2 years ago

I've worked out what is required to trigger it.

I use ant with ivy, and it pulls the jar files from maven.org. gwt-dev has a dependency on apache-jsp which depends on ecj from org.eclipse.jdt.

So I have ecj-3.19.0.jar as well as gwt-dev.jar.

If you have ecj-3.19.0.jar in the classpath ahead of gwt-dev.jar, then you get this error.

tbroyer commented 2 years ago

Oh :hankey: , @niloc132 this is due to the Jetty update, the apache-jsp from 2.9.0 does not depend on ECJ; looks like we'd have to find a proper fix/workaround and possibly make a patch release.

For anyone not using JSPs, it should be enough to exclude apache-jsp from the dependencies, and for those using JSPs, it might work when excluding ECJ.

GWT 2.9.0:

GWT 2.10:

niloc132 commented 2 years ago

Agreed, need mitigation/workaround, and likely some kind of bugfix release.

My first inclination is that we need to better manage these dependencies (I would really like to move to maven/gradle for dependency management, and drop gwtproject/tools...), but the way the error manifests seems to suggest we're fighting a classloader issue. Especially given that it only fails in Compiler and not DevMode (or CodeServer?) it seems especially odd, since the Compiler shouldn't even be starting separate classloader for web or whatnot? Or maybe somehow because when starting Jetty for DevMode, there are separate classloaders, this doesn't happen? I'm not sure that follows either.

niloc132 commented 2 years ago

I was able to reproduce this only by forcing org.eclipse.jdt:ecj to the classpath before gwt-dev itself, at least in maven - perhaps ivy includes dependencies in the classpath before the jars that depend on them. Not a classloader bug at all, I was misreading.

Excluding ECJ likely would mean that JSPs could only use the embedded version of the JDT included in gwt-dev - however, from a glance at org.apache.jasper.compiler.JDTCompiler in apache-jsp 8.5.70, that might be good enough - it looks as though this is very loosely coupled with ECJ such that it doesn't even require ECJ itself, just the classes found in the JDT that gwt-dev already has packaged. Additionally, apache-jsp is not counting on a specific version of ECJ, but allows a newer or older version than what it explicitly depends on (see the fun with various java version constant strings). Finally, it is possible to use javac instead of ecj, though I'm not sure at a glance how to configure that.

Proposed patch, tested with a failing GWT project and a simple JSP:

diff --git a/maven/poms/gwt/gwt-dev/pom-template.xml b/maven/poms/gwt/gwt-dev/pom-template.xml
index 368ece89f4..e72af3f7e6 100644
--- a/maven/poms/gwt/gwt-dev/pom-template.xml
+++ b/maven/poms/gwt/gwt-dev/pom-template.xml
@@ -113,6 +113,12 @@
         <dependency>
             <groupId>org.eclipse.jetty</groupId>
             <artifactId>apache-jsp</artifactId>
+            <exclusions>
+                <exclusion>
+                    <groupId>org.eclipse.jdt</groupId>
+                    <artifactId>ecj</artifactId>
+                </exclusion>
+            </exclusions>
         </dependency>
     </dependencies>
 </project>

index.jsp

<html>
<%
    out.println("successfully able to run java code without ecj");
    %>
</html>

Sample Java entrypoint that exhibits the bug:

public class AppEntrypoint implements EntryPoint {
    public static enum MyEnum {Yes, No, Maybe }
    @Override
    public void onModuleLoad() {
        boolean check = true;
        MyEnum value = MyEnum.Maybe;
        if (check)
            switch (value) {
                default:
                    System.out.println("hello world");
            }
    }
}

tbroyer commented 2 years ago

Patch looks good, and one could apply the exclusion to gwt-dev dependency (no idea how you do it in Ivy though) as a workaround.

cueman commented 2 years ago

I found that I needed an exclusion for gwt-codeserver as well as gwt-dev.

In case anybody else needs to add exclusions to Ivy to work around this problem, here's a portion of what my ivl.xml used to say for GWT 2.9:

<dependency org="com.google.gwt" name="gwt-user" rev="${gwt.version}" conf="javadoc->javadoc;sources->sources;build->default"/>
<dependency org="com.google.gwt" name="gwt-codeserver" rev="${gwt.version}" conf="javadoc->javadoc;sources->sources;build->default"/>
<dependency org="com.google.gwt" name="gwt-dev" rev="${gwt.version}" conf="javadoc->javadoc;sources->sources;build->default"/>
<dependency org="com.google.gwt" name="gwt-servlet" rev="${gwt.version}" conf="javadoc->javadoc;sources->sources;default->default"/>

where ${gwt.version} evaluates an ant variable that is the current version number of GWT.

To change this for GWT 2.10, I renamed the organisation and added nested <exclude> elements so it looks like this:

<dependency org="org.gwtproject" name="gwt-user" rev="${gwt.version}" conf="javadoc->javadoc;sources->sources;build->default"/>
<dependency org="org.gwtproject" name="gwt-codeserver" rev="${gwt.version}" conf="javadoc->javadoc;sources->sources;build->default">
    <exclude org="org.eclipse.jetty" module="apache-jsp" conf="build"/>
</dependency>
<dependency org="org.gwtproject" name="gwt-dev" rev="${gwt.version}" conf="javadoc->javadoc;sources->sources;build->default">
    <exclude org="org.eclipse.jetty" module="apache-jsp" conf="build"/>
</dependency>
<dependency org="org.gwtproject" name="gwt-servlet" rev="${gwt.version}" conf="javadoc->javadoc;sources->sources;default->default"/>

An alternative to adding an <exclude> separately to both gwt-codeserver and gwt-dev is to add an <exclude> after all the <dependency> items, so it's directly inside the <dependencies> element, looking like this:

<exclude org="org.eclipse.jetty" module="apache-jsp" conf="build"/>

gwtproject / gwt