Avoid "immutable" apis for JNodes, preventing optimizer errors (#10010)

The immutable APIs for JNodes apparently were never immutable to begin with, and have been watered down since then so that it isn't safe to actually treat them as being immutable. This confusion prevented multiple case expressions from being optimized correctly. Fixes #10005
gwtproject · Oct 20, 2024 · b47d247 · b47d247
1 parent 32d07c8
commit b47d247
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 10 deletions.
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JCaseStatement.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JCaseStatement.java
@@ -16,8 +16,8 @@
 package com.google.gwt.dev.jjs.ast;
 
 import com.google.gwt.dev.jjs.SourceInfo;
+import com.google.gwt.thirdparty.guava.common.collect.Lists;
 
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -27,24 +27,24 @@
  */
 public class JCaseStatement extends JStatement {
 
-  private List<JExpression> exprs;
+  private final List<JExpression> exprs;
 
   public JCaseStatement(SourceInfo info, JExpression expr) {
     super(info);
-    this.exprs = Collections.singletonList(expr);
+    this.exprs = Lists.newArrayList(expr);
   }
 
   public JCaseStatement(SourceInfo info, Collection<JExpression> exprs) {
     super(info);
-    this.exprs = Collections.unmodifiableList(new ArrayList<>(exprs));
+    this.exprs = Lists.newArrayList(exprs);
   }
 
   public boolean isDefault() {
     return exprs.isEmpty();
   }
 
   public List<JExpression> getExprs() {
-    return exprs;
+    return Collections.unmodifiableList(exprs);
   }
 
   public JBinaryOperation convertToCompareExpression(JExpression value) {
@@ -69,7 +69,7 @@ public JBinaryOperation convertToCompareExpression(JExpression value) {
   @Override
   public void traverse(JVisitor visitor, Context ctx) {
     if (visitor.visit(this, ctx)) {
-      exprs = Collections.unmodifiableList(visitor.acceptImmutable(exprs));
+      visitor.accept(exprs);
     }
     visitor.endVisit(this, ctx);
   }

diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JSwitchExpression.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JSwitchExpression.java
@@ -45,6 +45,10 @@ public boolean hasSideEffects() {
     return true;
   }
 
+  public void setType(JType type) {
+    this.type = type;
+  }
+
   @Override
   public void traverse(JVisitor visitor, Context ctx) {
     if (visitor.visit(this, ctx)) {

diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java b/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java
@@ -64,6 +64,7 @@
 import com.google.gwt.dev.jjs.ast.JProgram;
 import com.google.gwt.dev.jjs.ast.JReferenceType;
 import com.google.gwt.dev.jjs.ast.JStringLiteral;
+import com.google.gwt.dev.jjs.ast.JSwitchExpression;
 import com.google.gwt.dev.jjs.ast.JThisRef;
 import com.google.gwt.dev.jjs.ast.JTryStatement;
 import com.google.gwt.dev.jjs.ast.JType;
@@ -342,6 +343,11 @@ public void endVisit(JStringLiteral x, Context ctx) {
       instantiate(stringType);
     }
 
+    @Override
+    public void endVisit(JSwitchExpression x, Context ctx) {
+      x.setType(translate(x.getType()));
+    }
+
     @Override
     public void endVisit(JThisRef x, Context ctx) {
       assert !x.getType().isExternal();

diff --git a/user/test-super/com/google/gwt/dev/jjs/super/com/google/gwt/dev/jjs/test/Java17Test.java b/user/test-super/com/google/gwt/dev/jjs/super/com/google/gwt/dev/jjs/test/Java17Test.java
@@ -447,24 +447,47 @@ public static final int which(HasSwitchMethod whichSwitch) {
         return switch(whichSwitch) {
           case A -> 1;
           case RED -> 2;
-          case SUNDAY -> 3;
-          case JANUARY -> 4;
+          case SUNDAY, JANUARY -> 4;
           case ZERO -> 5;
         };
       }
       public static final int pick(HasSwitchMethod whichSwitch) {
         return 2 * switch(whichSwitch) {
           case A -> 1;
           case RED -> 2;
-          case SUNDAY -> 3;
-          case JANUARY -> 4;
+          case SUNDAY, JANUARY -> 4;
           case ZERO -> 5;
         };
       }
+      public static final String select(HasSwitchMethod whichSwitch) {
+        if (Math.random() > 2) {
+          return "none";
+        }
+        return switch(whichSwitch) {
+          case A -> "1";
+          case RED -> "2";
+          case SUNDAY, JANUARY -> "4";
+          case ZERO -> "5";
+        };
+      }
     }
 
     HasSwitchMethod uninlinedValue = Math.random() > 2 ? HasSwitchMethod.A : HasSwitchMethod.RED;
     assertEquals(2, HasSwitchMethod.which(uninlinedValue));
     assertEquals(4, HasSwitchMethod.pick(uninlinedValue));
+    assertEquals("hello 2", "hello " + HasSwitchMethod.select(uninlinedValue));
+  }
+
+  private static final String ONE = "1";
+  private static final String TWO = "2";
+  private static final String FOUR = "4";
+
+  public void testInlinedStringConstantsInCase() {
+    int value = switch(Math.random() > 2 ? "2" : "4") {
+      case ONE, TWO -> 2;
+      case FOUR -> 4;
+      default -> 0;
+    };
+    assertEquals(4, value);
   }
 }
diff --git a/user/test/com/google/gwt/dev/jjs/test/Java17Test.java b/user/test/com/google/gwt/dev/jjs/test/Java17Test.java
@@ -118,6 +118,10 @@ public void testSwitchExprInlining() {
     assertFalse(isGwtSourceLevel17());
   }
 
+  public void testInlinedStringConstantsInCase() {
+    assertFalse(isGwtSourceLevel17());
+  }
+
   private boolean isGwtSourceLevel17() {
     return JUnitShell.getCompilerOptions().getSourceLevel().compareTo(SourceLevel.JAVA17) >= 0;
   }