Skip to content

Commit 53606b5

Browse files
authored
Never attempt to inline switch expressions (#10006)
The original check wasn't specific enough - couldn't handle nested expressions, or expressions in different types of wrapper statements. Also includes a fix preventing record equals methods from being made static. Fixes #10005
1 parent 5ecb770 commit 53606b5

File tree

4 files changed

+71
-5
lines changed

4 files changed

+71
-5
lines changed

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,13 +193,16 @@ private void implementEquals(JRecordType type, JMethod method, SourceInfo info)
193193
myField,
194194
otherField);
195195
} else {
196-
// we would like to use Objects.equals here to be more consise, but we would need
196+
// We would like to use Objects.equals here to be more concise, but we would need
197197
// to look up the right impl based on the field - just as simple to insert a null check
198198
// and get it a little closer to all being inlined away
199+
200+
// Make another field ref to call equals() on
201+
JFieldRef myField2 = new JFieldRef(info, new JThisRef(info, type), field, type);
199202
equals = new JBinaryOperation(info, JPrimitiveType.BOOLEAN, JBinaryOperator.AND,
200203
new JBinaryOperation(info, JPrimitiveType.BOOLEAN, JBinaryOperator.NEQ,
201204
myField, JNullLiteral.INSTANCE),
202-
new JMethodCall(info, myField, objectEquals, otherField));
205+
new JMethodCall(info, myField2, objectEquals, otherField));
203206
}
204207
if (componentCheck != JBooleanLiteral.TRUE) {
205208
componentCheck = new JBinaryOperation(info, JPrimitiveType.BOOLEAN,

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

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,31 @@ public boolean visit(JThisRef x, Context ctx) {
7575
}
7676
}
7777

78+
/**
79+
* Determines if the given expression can be inlined. Any switch expression will fail this check.
80+
*/
81+
private static class CannotBeInlinedVisitor extends JVisitor {
82+
private boolean succeed = true;
83+
public static boolean check(JExpression expr) {
84+
CannotBeInlinedVisitor v = new CannotBeInlinedVisitor();
85+
v.accept(expr);
86+
return v.succeed;
87+
}
88+
89+
@Override
90+
public boolean visit(JStatement x, Context ctx) {
91+
// To ensure we didn't miss an important case, throw if we see a statement, as those cannot
92+
// be inlined.
93+
throw new IllegalStateException("Should never visit statements");
94+
}
95+
96+
@Override
97+
public boolean visit(JSwitchExpression x, Context ctx) {
98+
succeed = false;
99+
return false;
100+
}
101+
}
102+
78103
/**
79104
* Method inlining visitor.
80105
*/
@@ -148,6 +173,7 @@ private InlineResult tryInlineMethodCall(JMethodCall x, Context ctx) {
148173
if (expressions == null) {
149174
// If it will never be possible to inline the method, add it to a
150175
// blacklist
176+
151177
return InlineResult.BLACKLIST;
152178
}
153179

@@ -242,6 +268,9 @@ private List<JExpression> extractExpressionsFromBody(JMethodBody body) {
242268
if (initializer == null) {
243269
continue;
244270
}
271+
if (!CannotBeInlinedVisitor.check(initializer)) {
272+
return null;
273+
}
245274
JLocal local = (JLocal) declStatement.getVariableRef().getTarget();
246275
JExpression clone = new JBinaryOperation(stmt.getSourceInfo(), local.getType(),
247276
JBinaryOperator.ASG,
@@ -251,17 +280,19 @@ private List<JExpression> extractExpressionsFromBody(JMethodBody body) {
251280
} else if (stmt instanceof JExpressionStatement) {
252281
JExpressionStatement exprStmt = (JExpressionStatement) stmt;
253282
JExpression expr = exprStmt.getExpr();
254-
if (expr instanceof JSwitchExpression) {
255-
// Switch expressions can't be cloned in this way, though we wouldn't want to inline
256-
// such a large block anyway.
283+
if (!CannotBeInlinedVisitor.check(expr)) {
257284
return null;
258285
}
259286
JExpression clone = cloner.cloneExpression(expr);
260287
expressions.add(clone);
261288
} else if (stmt instanceof JReturnStatement) {
262289
JReturnStatement returnStatement = (JReturnStatement) stmt;
263290
JExpression expr = returnStatement.getExpr();
291+
264292
if (expr != null) {
293+
if (!CannotBeInlinedVisitor.check(expr)) {
294+
return null;
295+
}
265296
JExpression clone = cloner.cloneExpression(expr);
266297
clone = maybeCast(clone, body.getMethod().getType());
267298
expressions.add(clone);

user/test-super/com/google/gwt/dev/jjs/super/com/google/gwt/dev/jjs/test/Java17Test.java

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,4 +439,32 @@ public void testSwitchInSubExpr() {
439439
: 4.0;
440440
assertTrue(notCalled);
441441
}
442+
443+
public void testSwitchExprInlining() {
444+
enum HasSwitchMethod {
445+
A, RED, SUNDAY, JANUARY, ZERO;
446+
public static final int which(HasSwitchMethod whichSwitch) {
447+
return switch(whichSwitch) {
448+
case A -> 1;
449+
case RED -> 2;
450+
case SUNDAY -> 3;
451+
case JANUARY -> 4;
452+
case ZERO -> 5;
453+
};
454+
}
455+
public static final int pick(HasSwitchMethod whichSwitch) {
456+
return 2 * switch(whichSwitch) {
457+
case A -> 1;
458+
case RED -> 2;
459+
case SUNDAY -> 3;
460+
case JANUARY -> 4;
461+
case ZERO -> 5;
462+
};
463+
}
464+
}
465+
466+
HasSwitchMethod uninlinedValue = Math.random() > 2 ? HasSwitchMethod.A : HasSwitchMethod.RED;
467+
assertEquals(2, HasSwitchMethod.which(uninlinedValue));
468+
assertEquals(4, HasSwitchMethod.pick(uninlinedValue));
469+
}
442470
}

user/test/com/google/gwt/dev/jjs/test/Java17Test.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,10 @@ public void testSwitchInSubExpr() {
114114
assertFalse(isGwtSourceLevel17());
115115
}
116116

117+
public void testSwitchExprInlining() {
118+
assertFalse(isGwtSourceLevel17());
119+
}
120+
117121
private boolean isGwtSourceLevel17() {
118122
return JUnitShell.getCompilerOptions().getSourceLevel().compareTo(SourceLevel.JAVA17) >= 0;
119123
}

0 commit comments

Comments
 (0)