gwtproject / gwt

GWT Open Source Project
http://www.gwtproject.org
1.52k stars 375 forks source link

Using a switch statement together with if- or for-statement without using {} crashes #10024

Open FrankHossfeld opened 2 days ago

FrankHossfeld commented 2 days ago

Using a switch-statement with an if- or for-statement without using {}, the GWT compilation will throw an exception. Surrounding the switch with {} will fix that exception.

if ((vpos != null))
    switch (vpos) {
     case TOP :
         return "text-before-edge";
     case CENTER :
         return "central";
     case BASELINE :
         return "baseline";
     case BOTTOM :
         return "text-after-edge";
     }

will crash with this exception:

[INFO] 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')
[INFO]  at com.google.gwt.dev.jjs.impl.GwtAstBuilder$AstVisitor.pop(GwtAstBuilder.java:2816)
[INFO]  at com.google.gwt.dev.jjs.impl.GwtAstBuilder$AstVisitor.endVisit(GwtAstBuilder.java:1087)
[INFO]  ... 25 more
[INFO]    [ERROR] at SvgNodePeer.java(201): if ((vpos != null))
[INFO]     switch (vpos) {
[INFO]     case TOP :
[INFO]         return "text-before-edge";
[INFO]     case CENTER :
[INFO]         return "central";
[INFO]     case BASELINE :
[INFO]         return "baseline";
[INFO]     case BOTTOM :
[INFO]         return "text-after-edge";
[INFO]     }
[INFO]       org.eclipse.jdt.internal.compiler.ast.IfStatement

while this will work:

if ((vpos != null)) {
    switch (vpos) {
     case TOP :
         return "text-before-edge";
     case CENTER :
         return "central";
     case BASELINE :
         return "baseline";
     case BOTTOM :
         return "text-after-edge";
     }
}

Same issue with this code:

                for (InMemoryAuthorizationRule rule : rules)
                    switch (rule.computeRuleResult(operationRequest)) {
                        case DENIED:  result = AuthorizationRuleResult.DENIED; break; // Breaking as it's a final decision
                        case GRANTED: result = AuthorizationRuleResult.GRANTED; // Not breaking, as we need to check there is not another denying rule (which is priority)
                        case OUT_OF_RULE_CONTEXT: // just ignoring it and looping to the next
                    }

GWT version: 2.12.0 Browser (with version): all browser Operating System: all OS


niloc132 commented 1 day ago

The bug is that in these cases, the JDT AST represents a switch block as a switch expression, and GWT then assumes that if it encounters a JDT expression, GWT should have a corresponding expression, which then must be made into a statement.

https://github.com/gwtproject/gwt/blob/9af1f1dda875343211a174111f3a2be82b955137/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#L2813-L2819

Apparently this isn't what is happening though - we push a JSwitchStatement when we encounter a JDT SwitchStatement, and we push a JSwitchExpression when we encounter a JDT SwitchExpression, but somehow we're mixing those up on this later look.

There's a bit of a mismatch between the ASTs types here - GWT has a JExpressionStatement that wraps expressions to make them statements, while in JDT, Expression is a subtype of Statement. Both of these technically allow for illegal Java, like the statement 1;, and GWT's version means we need to wrap/unwrap in cases where JDT just uses context to decide if a node is used as a statement or an expression. In turn, JSwitchStatement is a wrapper on JSwitchExpression, while SwitchExpression is a subtype of SwitchStatement. As Statement, Expression, SwitchStatement, and SwitchExpression are all classes, and Java has no diamond inheritance, this means that in order for SwitchExpression to be an instanceof Expression, SwitchStatement must also be an Expression. Which it is.

screenshot927

The easy answer seems to be to guard that if with an instanceof check of the local JNode pop - if it isn't an expression, we have no need to make into a statement, and then the failed cast can't happen. We could also get more specific here, and require that this only take place if pop is a JSwitchStatement, but that seems excessive - if other scenarios like this arise in the future, this answer would also be correct for them.

It seems like there should be another answer here, where we synthesize a JBlock - but any outcome I see there ends up with still needing to call pop() with an expression.