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.

Sign up for GitHub

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

Jump to bottom

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

Open
FrankHossfeld opened this issue Oct 30, 2024 · 1 comment · May be fixed by #10027
Open

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

FrankHossfeld opened this issue Oct 30, 2024 · 1 comment · May be fixed by #10027
Labels
bug Category-Compiler
Milestone
2.12.1

Comments

@FrankHossfeld
Copy link
Member

FrankHossfeld commented Oct 30, 2024 •
edited
Loading

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
@jnehlmeier jnehlmeier added this to the 2.12.1 milestone Oct 30, 2024
@niloc132
Copy link
Contributor

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.

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java

Lines 2813 to 2819 in 9af1f1d

protected JStatement pop(Statement x) {
JNode pop = (x == null) ? null : pop();
if (x instanceof Expression) {
return maybeBoxOrUnbox((JExpression) pop, (Expression) x).makeStatement();
}
return (JStatement) pop;
}

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.

niloc132 added a commit to niloc132/gwt that referenced this issue Nov 2, 2024
@niloc132
Correct handling for SwitchStatements without a containing Block
b435e46 
Also fixed incorrect generics for related code, spotted while auditing
other incomplete typechecks

Fixes gwtproject#10024
@niloc132 niloc132 linked a pull request Nov 2, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Category-Compiler
Projects
None yet
Milestone
2.12.1
Development

Successfully merging a pull request may close this issue.

Correct handling for SwitchStatements without a containing Block niloc132/gwt
3 participants
@niloc132 @jnehlmeier @FrankHossfeld