Skip to content

Commit

Permalink
[23] JEP 455 - Exhaustiveness of Switch eclipse-jdt#2869
Browse files Browse the repository at this point in the history 
+ true + false = exhaustive over boolean :)
+ leverage PrimitiveConversionRoute for Pattern.coversType()
+ fine-tune combinations of primitive and boxing types
+ fix one omission in BaseTypeBinding.isExactWidening()
+ code gen to respect exhaustiveness of switch over primitives
+ fix code gen for bootstrap in the case of boxing+widening conversion
+ simplify condition for generating a throwing default

fixes eclipse-jdt#2869
  • Loading branch information
@stephan-herrmann
stephan-herrmann committed Aug 30, 2024
1 parent 6b7f284 commit 2d1d8a5
Show file tree
Hide file tree
Showing 10 changed files with 274 additions and 71 deletions.
2 changes: 1 addition & 1 deletion org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ClassFile.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4019,7 +4019,7 @@ private int addBootStrapTypeSwitchEntry(int localContentsOffset, SwitchStatement
for (CaseStatement.ResolvedCase c : constants) {
if (c.isPattern()) {
int typeOrDynIndex;
if ((switchStatement.switchBits & SwitchStatement.Primitive) != 0) {
if (c.e.resolvedType.isPrimitiveType()) {
// Dynamic for Class.getPrimitiveClass(Z) or such
typeOrDynIndex = this.constantPool.literalIndexForDynamic(c.primitivesBootstrapIdx,
c.t.signature(),
Expand Down
18 changes: 7 additions & 11 deletions ...ipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/CaseStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,28 +381,24 @@ private Constant resolveCasePattern(BlockScope scope, TypeBinding caseType, Type
}
}
} else if (type.isValidBinding()) {
PrimitiveConversionRoute route = PrimitiveConversionRoute.NO_CONVERSION_ROUTE;
// if not a valid binding, an error has already been reported for unresolved type
if (type.isPrimitiveType()) {
route = Pattern.findPrimitiveConversionRoute(type, expressionType, scope);
if (route == PrimitiveConversionRoute.NO_CONVERSION_ROUTE) {
if (Pattern.findPrimitiveConversionRoute(type, expressionType, scope) == PrimitiveConversionRoute.NO_CONVERSION_ROUTE) {
if (type.isPrimitiveType() && !JavaFeature.PRIMITIVES_IN_PATTERNS.isSupported(scope.compilerOptions())) {
scope.problemReporter().unexpectedTypeinSwitchPattern(type, e);
return Constant.NotAConstant;
} else if (!e.checkCastTypesCompatibility(scope, type, expressionType, null, false)) {
scope.problemReporter().typeMismatchError(expressionType, type, e, null);
return Constant.NotAConstant;
}
}
if ((type.isBaseType() && route == PrimitiveConversionRoute.NO_CONVERSION_ROUTE)
|| !e.checkCastTypesCompatibility(scope, type, expressionType, null, false)) {
scope.problemReporter().typeMismatchError(expressionType, type, e, null);
return Constant.NotAConstant;
}
}
if (e.coversType(expressionType)) {
if (e.coversType(expressionType, scope)) {
if ((switchStatement.switchBits & SwitchStatement.TotalPattern) != 0) {
scope.problemReporter().duplicateTotalPattern(e);
return IntConstant.fromValue(-1);
}
switchStatement.switchBits |= SwitchStatement.Exhaustive;
if (e.isUnconditional(expressionType)) {
if (e.isUnconditional(expressionType, scope)) {
switchStatement.switchBits |= SwitchStatement.TotalPattern;
if (switchStatement.defaultCase != null && !(e instanceof RecordPattern))
scope.problemReporter().illegalTotalPatternWithDefault(this);
Expand Down
5 changes: 3 additions & 2 deletions ...t.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/EitherOrMultiPattern.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.eclipse.jdt.internal.compiler.flow.FlowInfo;
import org.eclipse.jdt.internal.compiler.lookup.BlockScope;
import org.eclipse.jdt.internal.compiler.lookup.LocalVariableBinding;
import org.eclipse.jdt.internal.compiler.lookup.Scope;
import org.eclipse.jdt.internal.compiler.lookup.TypeBinding;

public class EitherOrMultiPattern extends Pattern {
Expand Down Expand Up @@ -109,11 +110,11 @@ public boolean dominates(Pattern p) {
}

@Override
public boolean coversType(TypeBinding type) {
public boolean coversType(TypeBinding type, Scope scope) {
if (!isUnguarded())
return false;
for (Pattern p : this.patterns) {
if (p.coversType(type))
if (p.coversType(type, scope))
return true;
}
return false;
Expand Down
9 changes: 5 additions & 4 deletions ...pse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/GuardedPattern.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.eclipse.jdt.internal.compiler.impl.Constant;
import org.eclipse.jdt.internal.compiler.lookup.BlockScope;
import org.eclipse.jdt.internal.compiler.lookup.LocalVariableBinding;
import org.eclipse.jdt.internal.compiler.lookup.Scope;
import org.eclipse.jdt.internal.compiler.lookup.TypeBinding;
import org.eclipse.jdt.internal.compiler.lookup.TypeIds;

Expand Down Expand Up @@ -64,8 +65,8 @@ public boolean matchFailurePossible() {
}

@Override
public boolean isUnconditional(TypeBinding t) {
return isUnguarded() && this.primaryPattern.isUnconditional(t);
public boolean isUnconditional(TypeBinding t, Scope scope) {
return isUnguarded() && this.primaryPattern.isUnconditional(t, scope);
}

@Override
Expand All @@ -80,8 +81,8 @@ public void setIsEitherOrPattern() {
}

@Override
public boolean coversType(TypeBinding type) {
return isUnguarded() && this.primaryPattern.coversType(type);
public boolean coversType(TypeBinding type, Scope scope) {
return isUnguarded() && this.primaryPattern.coversType(type, scope);
}

@Override
Expand Down
33 changes: 24 additions & 9 deletions org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Pattern.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.eclipse.jdt.internal.compiler.lookup.NullTypeBinding;
import org.eclipse.jdt.internal.compiler.lookup.Scope;
import org.eclipse.jdt.internal.compiler.lookup.TypeBinding;
import org.eclipse.jdt.internal.compiler.lookup.TypeIds;
import org.eclipse.jdt.internal.compiler.lookup.VoidTypeBinding;

public abstract class Pattern extends Expression {
Expand Down Expand Up @@ -76,21 +77,38 @@ public boolean isUnnamed() {
*
* @return whether pattern covers the given type or not
*/
public boolean coversType(TypeBinding type) {
public boolean coversType(TypeBinding type, Scope scope) {
if (!isUnguarded())
return false;
if (type == null || this.resolvedType == null)
return false;
return (type.isSubtypeOf(this.resolvedType, false));
if (type.isPrimitiveOrBoxedPrimitiveType()) {
PrimitiveConversionRoute route = Pattern.findPrimitiveConversionRoute(this.resolvedType, type, scope);
switch (route) {
case IDENTITY_CONVERSION:
case BOXING_CONVERSION:
case BOXING_CONVERSION_AND_WIDENING_REFERENCE_CONVERSION:
return true;
case WIDENING_PRIMITIVE_CONVERSION:
return BaseTypeBinding.isExactWidening(this.resolvedType.id, type.id);
case UNBOXING_AND_WIDENING_PRIMITIVE_CONVERSION:
return BaseTypeBinding.isExactWidening(this.resolvedType.id, TypeIds.box2primitive(type.id));
default:
break;
}
}
if (type.isSubtypeOf(this.resolvedType, false))
return true;
return false;
}

// Given a non-null instance of same type, would the pattern always match ?
public boolean matchFailurePossible() {
return false;
}

public boolean isUnconditional(TypeBinding t) {
return isUnguarded() && coversType(t);
public boolean isUnconditional(TypeBinding t, Scope scope) {
return isUnguarded() && coversType(t, scope);
}

public abstract void generateCode(BlockScope currentScope, CodeStream codeStream, BranchLabel patternMatchLabel, BranchLabel matchFailLabel);
Expand Down Expand Up @@ -179,12 +197,9 @@ public static boolean isBoxing(TypeBinding left, TypeBinding right) {
}
return false;
}
public static PrimitiveConversionRoute findPrimitiveConversionRoute(TypeBinding destinationType, TypeBinding expressionType, BlockScope scope) {
if (!(JavaFeature.PRIMITIVES_IN_PATTERNS.isSupported(
scope.compilerOptions().sourceLevel,
scope.compilerOptions().enablePreviewFeatures))) {
public static PrimitiveConversionRoute findPrimitiveConversionRoute(TypeBinding destinationType, TypeBinding expressionType, Scope scope) {
if (!JavaFeature.PRIMITIVES_IN_PATTERNS.isSupported(scope.compilerOptions()))
return PrimitiveConversionRoute.NO_CONVERSION_ROUTE;
}
if (destinationType == null || expressionType == null)
return PrimitiveConversionRoute.NO_CONVERSION_ROUTE;
boolean destinationIsBaseType = destinationType.isBaseType();
Expand Down
11 changes: 6 additions & 5 deletions ...ipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/RecordPattern.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.eclipse.jdt.internal.compiler.lookup.MethodBinding;
import org.eclipse.jdt.internal.compiler.lookup.RecordComponentBinding;
import org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding;
import org.eclipse.jdt.internal.compiler.lookup.Scope;
import org.eclipse.jdt.internal.compiler.lookup.TypeBinding;

public class RecordPattern extends Pattern {
Expand Down Expand Up @@ -72,7 +73,7 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
}

@Override
public boolean coversType(TypeBinding t) {
public boolean coversType(TypeBinding t, Scope scope) {

if (!isUnguarded())
return false;
Expand All @@ -89,7 +90,7 @@ public boolean coversType(TypeBinding t) {
for (int i = 0; i < components.length; i++) {
Pattern p = this.patterns[i];
RecordComponentBinding componentBinding = components[i];
if (!p.coversType(componentBinding.type)) {
if (!p.coversType(componentBinding.type, scope)) {
return false;
}
}
Expand Down Expand Up @@ -149,7 +150,7 @@ public TypeBinding resolveType(BlockScope scope) {
return this.resolvedType;
}

this.isTotalTypeNode = super.coversType(this.resolvedType);
this.isTotalTypeNode = super.coversType(this.resolvedType, scope);
RecordComponentBinding[] components = this.resolvedType.capture(scope, this.sourceStart, this.sourceEnd).components();
for (int i = 0; i < components.length; i++) {
Pattern p1 = this.patterns[i];
Expand All @@ -162,7 +163,7 @@ public TypeBinding resolveType(BlockScope scope) {
}
TypeBinding expressionType = componentBinding.type;
if (p1.isApplicable(expressionType, scope)) {
p1.isTotalTypeNode = p1.coversType(componentBinding.type);
p1.isTotalTypeNode = p1.coversType(componentBinding.type, scope);
MethodBinding[] methods = this.resolvedType.getMethods(componentBinding.name);
if (methods != null && methods.length > 0) {
p1.accessorMethod = methods[0];
Expand All @@ -189,7 +190,7 @@ public boolean matchFailurePossible() {
}

@Override
public boolean isUnconditional(TypeBinding t) {
public boolean isUnconditional(TypeBinding t, Scope scope) {
return false;
}

Expand Down
65 changes: 26 additions & 39 deletions ...se.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ public static record SingletonBootstrap(String id, char[] selector, char[] signa
public final static int TotalPattern = ASTNode.Bit3;
public final static int Exhaustive = ASTNode.Bit4;
public final static int QualifiedEnum = ASTNode.Bit5;
public final static int Primitive = ASTNode.Bit6;

// for switch on strings
private static final char[] SecretStringVariableName = " switchDispatchString".toCharArray(); //$NON-NLS-1$
Expand Down Expand Up @@ -881,41 +880,27 @@ public void generateCode(BlockScope currentScope, CodeStream codeStream) {
statementGenerateCode(currentScope, codeStream, statement);
}
}

boolean isEnumSwitchWithoutDefaultCase = this.defaultCase == null && resolvedType1.isEnum() && (this instanceof SwitchExpression || this.containsNull);
CompilerOptions compilerOptions = this.scope != null ? this.scope.compilerOptions() : null;
boolean isPatternSwitchSealedWithoutDefaultCase = this.defaultCase == null
&& compilerOptions != null
&& this.containsPatterns
&& JavaFeature.SEALED_CLASSES.isSupported(compilerOptions)
&& JavaFeature.PATTERN_MATCHING_IN_SWITCH.isSupported(compilerOptions)
&& this.expression.resolvedType instanceof ReferenceBinding
&& ((ReferenceBinding) this.expression.resolvedType).isSealed();

boolean isRecordPatternSwitchWithoutDefault = this.defaultCase == null
&& compilerOptions != null
&& this.containsPatterns
&& JavaFeature.RECORD_PATTERNS.isSupported(compilerOptions)
&& JavaFeature.PATTERN_MATCHING_IN_SWITCH.isSupported(compilerOptions)
&& this.expression.resolvedType instanceof ReferenceBinding
&& this.expression.resolvedType.isRecord();
if (isEnumSwitchWithoutDefaultCase
|| isPatternSwitchSealedWithoutDefaultCase
|| isRecordPatternSwitchWithoutDefault) {
boolean needsThrowingDefault = false;
if (this.defaultCase == null) {
// enum:
needsThrowingDefault = resolvedType1.isEnum() && (this instanceof SwitchExpression || this.containsNull);
// pattern switches:
needsThrowingDefault |= isExhaustive();
}
if (needsThrowingDefault) {
// we want to force an line number entry to get an end position after the switch statement
if (this.preSwitchInitStateIndex != -1) {
codeStream.removeNotDefinitelyAssignedVariables(currentScope, this.preSwitchInitStateIndex);
}
defaultLabel.place();
/* a default case is not needed for enum if all enum values are used in the switch expression
* we need to handle the default case to throw an error (IncompatibleClassChangeError) in order
* to make the stack map consistent. All cases will return a value on the stack except the missing default
* case.
* There is no returned value for the default case so we handle it with an exception thrown. An
* IllegalClassChangeError seems legitimate as this would mean the enum type has been recompiled with more
* enum constants and the class that is using the switch on the enum has not been recompiled
/* a default case is not needed for an exhaustive switch expression
* we need to handle the default case to throw an error in order to make the stack map consistent.
* All cases will return a value on the stack except the missing default case.
* There is no returned value for the default case so we handle it with an exception thrown.
*/
CompilerOptions compilerOptions = this.scope != null ? this.scope.compilerOptions() : null;
if (compilerOptions.complianceLevel >= ClassFileConstants.JDK19) {
// since 19 we have MatchException for this
if (codeStream.lastAbruptCompletion != codeStream.position) {
codeStream.goto_(this.breakLabel); // hop, skip and jump over match exception throw.
}
Expand All @@ -926,6 +911,7 @@ public void generateCode(BlockScope currentScope, CodeStream codeStream) {
codeStream.invokeJavaLangMatchExceptionConstructor();
codeStream.athrow();
} else {
// old style using IncompatibleClassChangeError:
codeStream.newJavaLangIncompatibleClassChangeError();
codeStream.dup();
codeStream.invokeJavaLangIncompatibleClassChangeErrorDefaultConstructor();
Expand All @@ -943,9 +929,7 @@ public void generateCode(BlockScope currentScope, CodeStream codeStream) {
}
// place the trailing labels (for break and default case)
this.breakLabel.place();
if (this.defaultCase == null && !(isEnumSwitchWithoutDefaultCase
|| isPatternSwitchSealedWithoutDefaultCase
|| isRecordPatternSwitchWithoutDefault)) {
if (this.defaultCase == null && !needsThrowingDefault) {
// we want to force an line number entry to get an end position after the switch statement
codeStream.recordPositionsFrom(codeStream.position, this.sourceEnd, true);
defaultLabel.place();
Expand Down Expand Up @@ -977,7 +961,7 @@ private void generateCodeSwitchPatternEpilogue(CodeStream codeStream) {

private void generateCodeSwitchPatternPrologue(BlockScope currentScope, CodeStream codeStream) {
this.expression.generateCode(currentScope, codeStream, true);
if ((this.switchBits & NullCase) == 0 && (this.switchBits & Primitive) == 0) {
if ((this.switchBits & NullCase) == 0 && !this.expression.resolvedType.isPrimitiveType()) {
codeStream.dup();
codeStream.invokeJavaUtilObjectsrequireNonNull();
codeStream.pop();
Expand Down Expand Up @@ -1007,7 +991,7 @@ private void generateCodeSwitchPatternPrologue(BlockScope currentScope, CodeStre
if (hasQualifiedEnums) {
c.index = i;
}
if ((this.switchBits & Primitive) != 0) {
if (c.t.isPrimitiveType()) {
SingletonBootstrap descriptor = null;
if (c.isPattern()) {
descriptor = PRIMITIVE_CLASS__BOOTSTRAP;
Expand Down Expand Up @@ -1043,7 +1027,7 @@ char[] typeSwitchSignature(TypeBinding exprType) {
case TypeIds.T_JavaLangLong, TypeIds.T_JavaLangFloat, TypeIds.T_JavaLangDouble, TypeIds.T_JavaLangBoolean ->
exprType.signature();
default ->
(this.switchBits & Primitive) != 0
exprType.isPrimitiveType()
? exprType.signature()
: "Ljava/lang/Object;".toCharArray(); //$NON-NLS-1$
};
Expand Down Expand Up @@ -1126,6 +1110,7 @@ public void resolve(BlockScope upperScope) {
try {
boolean isEnumSwitch = false;
boolean isStringSwitch = false;
boolean isPrimitiveSwitch = false;
TypeBinding expressionType = this.expression.resolveType(upperScope);
CompilerOptions compilerOptions = upperScope.compilerOptions();
boolean isEnhanced = checkAndSetEnhanced(upperScope, expressionType);
Expand All @@ -1137,7 +1122,7 @@ public void resolve(BlockScope upperScope) {
break checkType;
} else if (expressionType.isBaseType()) {
if (JavaFeature.PRIMITIVES_IN_PATTERNS.isSupported(compilerOptions)) {
this.switchBits |= Primitive;
isPrimitiveSwitch = true;
}
if (this.expression.isConstantValueOfTypeAssignableToType(expressionType, TypeBinding.INT))
break checkType;
Expand All @@ -1162,7 +1147,7 @@ public void resolve(BlockScope upperScope) {
break checkType;
}
if (!JavaFeature.PATTERN_MATCHING_IN_SWITCH.isSupported(compilerOptions) || (expressionType.isBaseType() && expressionType.id != T_null && expressionType.id != T_void)) {
if ((this.switchBits & Primitive) == 0) { // when Primitive is set it is approved above
if (!isPrimitiveSwitch) { // when isPrimitiveSwitch is set it is approved above
upperScope.problemReporter().incorrectSwitchType(this.expression, expressionType);
expressionType = null; // fault-tolerance: ignore type mismatch from constants from hereon
}
Expand Down Expand Up @@ -1212,7 +1197,7 @@ public void resolve(BlockScope upperScope) {
if (con == Constant.NotAConstant)
continue;
this.otherConstants[counter] = c;
final int c1 = this.containsPatterns ? (c.intValue() == -1 ? -1 : counter) : c.intValue();
final int c1 = this.containsPatterns ? (c.intValue() == -1 ? -1 : counter) : c.intValue();
this.constants[counter] = c1;
if (counter == 0 && defaultFound) {
if (c.isPattern() || isCaseStmtNullOnly(caseStmt))
Expand All @@ -1232,6 +1217,8 @@ public void resolve(BlockScope upperScope) {
return id == otherId; // 'null' shares IntConstant(-1)
if (con.equals(c2))
return true;
if (id == TypeIds.T_boolean)
this.switchBits |= Exhaustive; // 2 different boolean constants => exhaustive :)
return this.constants[idx] == c1;
}
};
Expand All @@ -1253,7 +1240,7 @@ public void resolve(BlockScope upperScope) {
if (type.isBaseType()) {
type = this.scope.environment().computeBoxingType(type);
}
if (p1.coversType(type))
if (p1.coversType(type, this.scope))
this.scope.problemReporter().patternDominatedByAnother(c.e);
}
}
Expand Down
1 change: 1 addition & 0 deletions ...jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/BaseTypeBinding.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ public static final boolean isExactWidening(int left, int right) {
case TypeIds.Short2Long:
case TypeIds.Short2Float:
case TypeIds.Short2Double:
case TypeIds.Char2Int:
case TypeIds.Char2Long:
case TypeIds.Char2Float:
case TypeIds.Char2Double:
Expand Down
Loading

0 comments on commit 2d1d8a5

Please sign in to comment.