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

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

Closed
cueman opened this issue Jun 27, 2022 · 11 comments
Closed

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

cueman opened this issue Jun 27, 2022 · 11 comments

Comments

@cueman
Copy link

cueman commented Jun 27, 2022

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
Copy link
Member

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
Copy link
Author

cueman commented Jun 27, 2022

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
Copy link
Member

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
Copy link
Author

cueman commented Jun 28, 2022

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
Copy link
Author

cueman commented Jun 28, 2022

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
Copy link
Member

tbroyer commented Jun 28, 2022

Oh 💩 , @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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

tbroyer commented Jun 30, 2022

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
Copy link
Author

cueman commented Jul 28, 2022

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"/>

@niloc132
Copy link
Member

It looks like this was probably a "sneak peek" at #10024 - too new of a JDT version supported switch expressions, and resulted in those switches sometimes being represented as expressions which surprised GWT.

This should be fixed, with no explicit workaround required, when updating to GWT 2.12.1 and beyond. Note however that using a separate JDT version than what GWT includes carries the possibility of other issues like this: try to avoid mixing your server and compiler classpaths. #10026 might also make this more explicit when it does happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants