Skip to content

Commit

Permalink
Add fallback throw for exhaustive switch expressions on unexpected va…
Browse files Browse the repository at this point in the history
…lues.

PiperOrigin-RevId: 726187098
  • Loading branch information
rluble authored and copybara-github committed Feb 12, 2025
1 parent 5e6e77e commit 37f43d1
Show file tree
Hide file tree
Showing 11 changed files with 165 additions and 24 deletions.
22 changes: 22 additions & 0 deletions jre/java/javaemul/internal/InternalPreconditions.java
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,28 @@ public static void checkCriticalConcurrentModification(
throw new ConcurrentModificationException();
}
}

/**
* Throws a MatchException for exhaustive switch expressions on unexpected values.
*
* @throws MatchException
*/
public static void checkCriticalExhaustive() {
throw new MatchException();
}

public static void checkExhaustive() {
if (IS_API_CHECKED) {
checkCriticalExhaustive();
} else if (IS_ASSERTED) {
try {
checkCriticalExhaustive();
} catch (Exception e) {
throw new AssertionError(e);
}
}
}

// Hides the constructor for this static utility class.
private InternalPreconditions() { }
}
16 changes: 16 additions & 0 deletions transpiler/java/com/google/j2cl/transpiler/ast/RuntimeMethods.java
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,22 @@ private static Expression createCheckNotNullBooleanCall(Expression argument) {
.build();
}

/** Creates a call to handling of unexpected values on exhaustive switch expressions. */
public static Expression createCheckCriticalExhaustiveCall(boolean isCritical) {
if (isCritical) {
return MethodCall.Builder.from(
TypeDescriptors.get()
.javaemulInternalPreconditions
.getMethodDescriptor("checkCriticalExhaustive"))
.build();
}
return MethodCall.Builder.from(
TypeDescriptors.get()
.javaemulInternalPreconditions
.getMethodDescriptor("checkExhaustive"))
.build();
}

/** Create a call to an LongUtils method. */
public static MethodCall createLongUtilsMethodCall(String methodName, Expression... arguments) {
return createLongUtilsMethodCall(methodName, asList(arguments));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ default TypeDescriptor getTypeDescriptor() {
return PrimitiveTypes.VOID;
}

/** Returns true if the switch construct has a default case. */
default boolean hasDefaultCase() {
return getCases().stream().anyMatch(SwitchCase::isDefault);
}

Builder<T> toBuilder();

/** An interface for builders of switch constructs. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.google.j2cl.transpiler.passes.AddInterfaceConstructorCasts;
import com.google.j2cl.transpiler.passes.AddJavaLangObjectForwardingMethods;
import com.google.j2cl.transpiler.passes.AddNothingReturnStatements;
import com.google.j2cl.transpiler.passes.AddSwitchExpressionsExhaustivenessCheck;
import com.google.j2cl.transpiler.passes.AddVisibilityMethodBridgesJ2kt;
import com.google.j2cl.transpiler.passes.AnnotateProtobufMethodsAsKtProperties;
import com.google.j2cl.transpiler.passes.ConvertMethodReferencesToLambdas;
Expand Down Expand Up @@ -312,6 +313,7 @@ public ImmutableList<Supplier<NormalizationPass>> getPassFactories(BackendOption
// Java semantic conversions.
InsertJsEnumBoxingAndUnboxingConversions::new,
RemoveUnneededCasts::new,
AddSwitchExpressionsExhaustivenessCheck::new,
ImplementSwitchExpressionsViaIifes::new,
NormalizeSwitchConstructs::new,
NormalizeArrayAccesses::new,
Expand Down Expand Up @@ -470,6 +472,7 @@ public ImmutableList<Supplier<NormalizationPass>> getPassFactories(BackendOption
() -> new ExpandCompoundAssignments(/* expandAll= */ true),
InsertErasureTypeSafetyCasts::new,
RewriteUnaryExpressions::new,
AddSwitchExpressionsExhaustivenessCheck::new,
NormalizeSwitchConstructs::new,
// Propagate constants needs to run after NormalizeSwitchStatements since it introduces
// field references to constant fields.
Expand Down Expand Up @@ -618,6 +621,7 @@ public ImmutableList<Supplier<NormalizationPass>> getPassFactories(BackendOption
() -> new ExpandCompoundAssignments(/* expandAll= */ true),
InsertErasureTypeSafetyCasts::new,
RewriteUnaryExpressions::new,
AddSwitchExpressionsExhaustivenessCheck::new,
NormalizeSwitchConstructs::new,
// Propagate constants needs to run after NormalizeSwitchStatements since it introduces
// field references to constant fields.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Copyright 2025 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package com.google.j2cl.transpiler.passes;

import com.google.j2cl.transpiler.ast.AbstractVisitor;
import com.google.j2cl.transpiler.ast.CompilationUnit;
import com.google.j2cl.transpiler.ast.RuntimeMethods;
import com.google.j2cl.transpiler.ast.SwitchCase;
import com.google.j2cl.transpiler.ast.SwitchExpression;

/**
* Instruments exhaustive switch expressions with no default to handle unexpected values.
*
* <p>Exhaustive switch expressions that don't explicitly handle the default case are instrumented
* to check for unexpected values. In the JVM this is done to generate correct code in the presence
* of library skew, which is not an issue in J2CL. However a similar situation could happen with
* JsEnums where it is quite easy to receive an unexpected value.
*
* <p>The same reasoning could be applied to switch statements; i.e. if we determine that the switch
* statement is exhaustive and lacks default handling it could be instrumented. But the nuance in
* switch statements is that the default handling might be present outside the switch construct as
* in the following code.
*
* <pre>{@code
* enum X { A, B;}
*
* ....
* switch (e) {
* case A:
* return 1;
* case B:
* return 2;
* }
* // Handle unexpected value here
* throw new AssertionError();
* }</pre>
*/
public class AddSwitchExpressionsExhaustivenessCheck extends NormalizationPass {
@Override
public void applyTo(CompilationUnit compilationUnit) {
compilationUnit.accept(
new AbstractVisitor() {
@Override
public void exitSwitchExpression(SwitchExpression switchExpression) {
if (switchExpression.hasDefaultCase()) {
return;
}

// The switch expression does not have a default case; that means that the frontend
// deemed it exhaustive, hence add a default case with a runtime check.
// TODO(b/395953418): Reject switch expressions with no default on native jsenums.
var expressionType = switchExpression.getTypeDescriptor().toRawTypeDescriptor();
var isCritical = expressionType.isNative() && expressionType.isJsEnum();

var sourcePosition = switchExpression.getSourcePosition();
var checkMethodCall =
RuntimeMethods.createCheckCriticalExhaustiveCall(isCritical)
.makeStatement(sourcePosition);
switchExpression
.getCases()
.add(SwitchCase.newBuilder().setStatements(checkMethodCall).build());
}
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ public class Main {

public static void main(String... args) {
testNativeJsEnum();
// TODO(b/380878051): Enable once bug is fixed.
// testNativeJsEnumWithMissingValues();
testNativeJsEnumWithMissingValues();
testStringNativeJsEnum();
testCastOnNative();
testComparableJsEnum();
Expand Down Expand Up @@ -170,7 +169,7 @@ private static void testNativeJsEnumWithMissingValues() {
case OK -> 1;
};
fail();
} catch (MatchException | IncompatibleClassChangeError expected) {
} catch (MatchException expected) {
// Expected behavior.
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ class Main extends j_l_Object {
return 1;
case ComparableJsEnum.ZERO:
return 0;
default:
InternalPreconditions.m_checkExhaustive__void();
}
})();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1485,23 +1485,28 @@
(block
(block
(block
(block ;; evaluate expression and jump
(br_table 2 1 0 3 (local.get $$expression))
(block
(block ;; evaluate expression and jump
(br_table 2 1 0 3 (local.get $$expression))
)
;; case 2:
;;@ transpiler/javatests/com/google/j2cl/readable/java/jsenum/readable-j2wasm.js/jsenum/Main.java:153:22
(i32.const 2)
(br $SWITCH)
)
;; case 2:
;;@ transpiler/javatests/com/google/j2cl/readable/java/jsenum/readable-j2wasm.js/jsenum/Main.java:153:22
(i32.const 2)
;; case 1:
;;@ transpiler/javatests/com/google/j2cl/readable/java/jsenum/readable-j2wasm.js/jsenum/Main.java:154:22
(i32.const 1)
(br $SWITCH)
)
;; case 1:
;;@ transpiler/javatests/com/google/j2cl/readable/java/jsenum/readable-j2wasm.js/jsenum/Main.java:154:22
(i32.const 1)
;; case 0:
;;@ transpiler/javatests/com/google/j2cl/readable/java/jsenum/readable-j2wasm.js/jsenum/Main.java:155:23
(i32.const 0)
(br $SWITCH)
)
;; case 0:
;;@ transpiler/javatests/com/google/j2cl/readable/java/jsenum/readable-j2wasm.js/jsenum/Main.java:155:23
(i32.const 0)
(br $SWITCH)
;; default:
;;@ transpiler/javatests/com/google/j2cl/readable/java/jsenum/readable-j2wasm.js/jsenum/Main.java:152:8
(call [email protected] )
)
)
(unreachable)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const $Util = goog.require('nativebootstrap.Util$impl');

let RuntimeException = goog.forwardDeclare('java.lang.RuntimeException$impl');
let Short = goog.forwardDeclare('java.lang.Short$impl');
let InternalPreconditions = goog.forwardDeclare('javaemul.internal.InternalPreconditions$impl');
let $Long = goog.forwardDeclare('nativebootstrap.Long$impl');
let Enum = goog.forwardDeclare('switchexpression.SwitchExpression.Enum$impl');
let $Exceptions = goog.forwardDeclare('vmbootstrap.Exceptions$impl');
Expand Down Expand Up @@ -101,6 +102,8 @@ class SwitchExpression extends j_l_Object {
return $Long.fromInt(0);
case Enum.$ordinal_B__switchexpression_SwitchExpression_Enum:
return $Long.fromInt(1);
default:
InternalPreconditions.m_checkExhaustive__void();
}
})();
}
Expand All @@ -119,6 +122,7 @@ class SwitchExpression extends j_l_Object {
static $loadModules() {
RuntimeException = goog.module.get('java.lang.RuntimeException$impl');
Short = goog.module.get('java.lang.Short$impl');
InternalPreconditions = goog.module.get('javaemul.internal.InternalPreconditions$impl');
$Long = goog.module.get('nativebootstrap.Long$impl');
Enum = goog.module.get('switchexpression.SwitchExpression.Enum$impl');
$Exceptions = goog.module.get('vmbootstrap.Exceptions$impl');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ goog.module('switchexpression.SwitchExpression');
goog.require('java.lang.Object');
goog.require('java.lang.RuntimeException');
goog.require('java.lang.Short');
goog.require('javaemul.internal.InternalPreconditions');
goog.require('nativebootstrap.Long');
goog.require('nativebootstrap.Util');
goog.require('switchexpression.SwitchExpression.Enum');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,18 +323,23 @@
;;@ transpiler/javatests/com/google/j2cl/readable/java/switchexpression/readable-j2wasm.js/switchexpression/SwitchExpression.java:82:8
(block
(block
(block ;; evaluate expression and jump
(br_table 0 1 2 (local.get $$expression))
(block
(block ;; evaluate expression and jump
(br_table 0 1 2 (local.get $$expression))
)
;; case 0:
;;@ transpiler/javatests/com/google/j2cl/readable/java/switchexpression/readable-j2wasm.js/switchexpression/SwitchExpression.java:83:20
(i64.const 0)
(br $SWITCH)
)
;; case 0:
;;@ transpiler/javatests/com/google/j2cl/readable/java/switchexpression/readable-j2wasm.js/switchexpression/SwitchExpression.java:83:20
(i64.const 0)
;; case 1:
;;@ transpiler/javatests/com/google/j2cl/readable/java/switchexpression/readable-j2wasm.js/switchexpression/SwitchExpression.java:84:20
(i64.const 1)
(br $SWITCH)
)
;; case 1:
;;@ transpiler/javatests/com/google/j2cl/readable/java/switchexpression/readable-j2wasm.js/switchexpression/SwitchExpression.java:84:20
(i64.const 1)
(br $SWITCH)
;; default:
;;@ transpiler/javatests/com/google/j2cl/readable/java/switchexpression/readable-j2wasm.js/switchexpression/SwitchExpression.java:82:8
(call [email protected] )
)
)
(unreachable)
Expand Down

0 comments on commit 37f43d1

Please sign in to comment.