blackdrag commented on code in PR #2922:
URL: https://github.com/apache/groovy/pull/2922#discussion_r3988013411
##########
src/main/java/org/codehaus/groovy/classgen/asm/SwitchExpressionWriter.java:
##########
@@ -213,12 +215,41 @@ protected void writeDefaultOrThrow(final SwitchExpression
expression, final int
if (defaultStatement != null && !defaultStatement.isEmpty()) {
defaultStatement.visit(controller.getAcg());
if (maybeFallsThrough(defaultStatement)) {
- throwUnmatchedSelector(selectorIndex, selectorType);
+ writeUnmatchedSelector(expression, selectorIndex,
selectorType);
}
return;
}
if (completeEnum) {
throwIncompatibleClassChangeError();
+ } else {
+ writeUnmatchedSelector(expression, selectorIndex, selectorType);
+ }
+ }
+
+ /**
+ * Whether an unmatched selector completes the expression with {@code null}
+ * rather than throwing: a switch in implicit-return position
(GROOVY-12399).
+ *
+ * @since 6.0.0
+ */
+ protected static boolean unmatchedYieldsNull(final SwitchExpression
expression) {
+ return
Boolean.TRUE.equals(expression.getNodeMetaData(UNMATCHED_YIELDS_NULL));
+ }
+
+ /**
+ * Completes the expression for an unmatched selector: {@code null} (cast
+ * to the result type) for a switch in implicit-return position, otherwise
+ * an {@code IllegalStateException} (GROOVY-12399).
+ */
+ private void writeUnmatchedSelector(final SwitchExpression expression,
final int selectorIndex, final ClassNode selectorType) {
+ if (unmatchedYieldsNull(expression)) {
+ CompileStack.SwitchExpressionContext context =
controller.getCompileStack().requireSwitchExpressionContext();
+ OperandStack operandStack = controller.getOperandStack();
+ controller.getMethodVisitor().visitInsn(ACONST_NULL);
+ operandStack.push(ClassHelper.OBJECT_TYPE);
+ operandStack.doGroovyCast(context.resultType);
Review Comment:
this will call a quite complex path for what should be a cast at bytecode
level. I suggest to not to use operandStack for this. operandStack is good when
you need to handle variables and their types, especially if they may be a
reference or not. But here it is a simple "load null, cast to X" operation.
##########
src/test/groovy/org/codehaus/groovy/classgen/Groovy12255.groovy:
##########
@@ -1045,11 +1045,12 @@ final class Groovy12255 {
@Test
void typeCheckedUnmatchedIsError() {
- // exhaustiveness is a static type-checker error
+ // exhaustiveness is a static type-checker error (in expression
position;
+ // implicit-return position yields null when unmatched, GROOVY-12399)
def err = shouldFail(MultipleCompilationErrorsException, '''
@groovy.transform.TypeChecked
def meth(int a) {
- switch (a) {
+ return switch (a) {
Review Comment:
Why add "return"?
##########
src/test/groovy/org/codehaus/groovy/classgen/Groovy12255.groovy:
##########
@@ -537,7 +537,7 @@ final class Groovy12255 {
enum Flag { ON, OFF }
String m(Flag f) {
- switch (f) {
+ return switch (f) {
Review Comment:
Why add "return"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
