Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Code review driven incremental cleanup of sealed types implementation #3145

Merged
merged 1 commit into from
Oct 23, 2024
Merged

Code review driven incremental cleanup of sealed types implementation #3145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...ipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/CaseStatement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,8 @@ private Constant resolveCasePattern(BlockScope scope, TypeBinding caseType, Type
if (type != null) {
constant = IntConstant.fromValue(switchStatement.constantIndex);
switchStatement.caseLabelElements.add(e);
if (e instanceof RecordPattern)
switchStatement.containsRecordPatterns = true;

if (isUnguarded)
switchStatement.caseLabelElementTypes.add(type);
Expand Down
4 changes: 2 additions & 2 deletions ...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchExpression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ public ExpressionContext getExpressionContext() {
return this.expressionContext;
}
@Override
protected boolean ignoreMissingDefaultCase(CompilerOptions compilerOptions, boolean isEnumSwitch) {
return isEnumSwitch; // mandatory error if not enum in switch expressions
protected boolean ignoreMissingDefaultCase(CompilerOptions compilerOptions) {
return true;
}
@Override
protected void reportMissingEnumConstantCase(BlockScope upperScope, FieldBinding enumConstant) {
Expand Down
191 changes: 72 additions & 119 deletions ...se.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ public static record SingletonBootstrap(String id, char[] selector, char[] signa
public int switchBits;

public boolean containsPatterns;
public boolean containsRecordPatterns;
public boolean containsNull;
boolean nullProcessed = false;
BranchLabel switchPatternRestartTarget;
Expand Down Expand Up @@ -366,8 +367,7 @@ public boolean visit(TNode node) {
}
}
if (node.type instanceof ReferenceBinding ref && ref.isSealed()) {
List<ReferenceBinding> allAllowedTypes = ref.getAllEnumerableReferenceTypes();
this.covers &= caseElementsCoverSelectorType(allAllowedTypes, availableTypes);
this.covers &= caseElementsCoverSealedType(ref, availableTypes);
return this.covers;
}
this.covers = false;
Expand Down Expand Up @@ -824,13 +824,6 @@ public void generateCode(BlockScope currentScope, CodeStream codeStream) {
int max = localKeysCopy[constantCount - 1];
int min = localKeysCopy[0];
if ((long) (constantCount * 2.5) > ((long) max - (long) min)) {

// work-around 1.3 VM bug, if max>0x7FFF0000, must use lookup bytecode
// see http://dev.eclipse.org/bugs/show_bug.cgi?id=21557
if (max > 0x7FFF0000 && currentScope.compilerOptions().complianceLevel < ClassFileConstants.JDK1_4) {
codeStream.lookupswitch(defaultLabel, this.constants, sortedIndexes, caseLabels);

} else {
codeStream.tableswitch(
defaultLabel,
min,
Expand All @@ -839,7 +832,6 @@ public void generateCode(BlockScope currentScope, CodeStream codeStream) {
sortedIndexes,
this.constMapping,
caseLabels);
}
} else {
codeStream.lookupswitch(defaultLabel, this.constants, sortedIndexes, caseLabels);
}
Expand Down Expand Up @@ -1110,11 +1102,9 @@ boolean isAllowedType(TypeBinding type) {
@Override
public void resolve(BlockScope upperScope) {
try {
boolean isEnumSwitch = false;
boolean isStringSwitch = false;
TypeBinding expressionType = this.expression.resolveType(upperScope);
CompilerOptions compilerOptions = upperScope.compilerOptions();
boolean isEnhanced = checkAndSetEnhanced(upperScope, expressionType);
if (expressionType != null) {
this.expression.computeConversion(upperScope, expressionType, expressionType);
checkType: {
Expand All @@ -1130,15 +1120,11 @@ public void resolve(BlockScope upperScope) {
if (expressionType.isCompatibleWith(TypeBinding.INT))
break checkType;
} else if (expressionType.isEnum()) {
isEnumSwitch = true;
if (compilerOptions.complianceLevel < ClassFileConstants.JDK1_5) {
upperScope.problemReporter().incorrectSwitchType(this.expression, expressionType); // https://bugs.eclipse.org/bugs/show_bug.cgi?id=360317
}
break checkType;
} else if (!this.containsPatterns && !this.containsNull && upperScope.isBoxingCompatibleWith(expressionType, TypeBinding.INT)) {
this.expression.computeConversion(upperScope, TypeBinding.INT, expressionType);
break checkType;
} else if (compilerOptions.complianceLevel >= ClassFileConstants.JDK1_7 && expressionType.id == TypeIds.T_JavaLangString) {
} else if (expressionType.id == TypeIds.T_JavaLangString) {
if (this.containsPatterns || this.containsNull) {
isStringSwitch = !JavaFeature.PATTERN_MATCHING_IN_SWITCH.isSupported(compilerOptions);
this.isNonTraditional = true;
Expand Down Expand Up @@ -1295,61 +1281,76 @@ && defaultFound && isExhaustive()) {
}
reportMixingCaseTypes();

// check default case for non-enum switch:
boolean flagged = checkAndFlagDefaultSealed(upperScope, compilerOptions);
if (!flagged && this.defaultCase == null) {
if (ignoreMissingDefaultCase(compilerOptions, isEnumSwitch) && isEnumSwitch) {
upperScope.methodScope().hasMissingSwitchDefault = true;
} else {
if (!isEnumSwitch && !isExhaustive()) {
complainIfNotExhaustiveSwitch(upperScope, expressionType, compilerOptions);

} finally {
if (this.scope != null) this.scope.enclosingCase = null; // no longer inside switch case block
}
}
private void complainIfNotExhaustiveSwitch(BlockScope upperScope, TypeBinding selectorType, CompilerOptions compilerOptions) {

boolean isEnhanced = isEnhancedSwitch(upperScope, selectorType);
if (selectorType != null && selectorType.isEnum()) {
if (isEnhanced)
this.switchBits |= SwitchStatement.Exhaustive; // negated below if found otherwise
if (this.defaultCase != null && !compilerOptions.reportMissingEnumCaseDespiteDefault)
return;

int constantCount = this.otherConstants == null ? 0 : this.otherConstants.length;
if (!((this.switchBits & TotalPattern) != 0) &&
((this.containsPatterns || this.containsNull) ||
(constantCount >= this.caseCount &&
constantCount != ((ReferenceBinding)selectorType).enumConstantCount()))) {
Set<FieldBinding> unenumeratedConstants = unenumeratedConstants((ReferenceBinding) selectorType, constantCount);
if (unenumeratedConstants.size() != 0) {
this.switchBits &= ~SwitchStatement.Exhaustive;
if (!(this.defaultCase != null && (this.defaultCase.bits & DocumentedCasesOmitted) != 0)) {
if (isEnhanced)
upperScope.problemReporter().enhancedSwitchMissingDefaultCase(this.expression);
else
upperScope.problemReporter().missingDefaultCase(this, isEnumSwitch, expressionType);
}
}
}
// Exhaustiveness check for enum switch
if (isEnumSwitch && compilerOptions.complianceLevel >= ClassFileConstants.JDK1_5) {
if (this.defaultCase == null || compilerOptions.reportMissingEnumCaseDespiteDefault) {
int constantCount = this.otherConstants == null ? 0 : this.otherConstants.length;
if (isEnhanced)
this.switchBits |= SwitchStatement.Exhaustive; // negated below if found otherwise
if (!((this.switchBits & TotalPattern) != 0) &&
((this.containsPatterns || this.containsNull) ||
(constantCount >= this.caseCount &&
constantCount != ((ReferenceBinding)expressionType).enumConstantCount()))) {
Set<FieldBinding> unenumeratedConstants = unenumeratedConstants((ReferenceBinding) expressionType, constantCount);
if (unenumeratedConstants.size() != 0) {
this.switchBits &= ~SwitchStatement.Exhaustive;
// enum constant did not get referenced from switch
boolean suppress = (this.defaultCase != null && (this.defaultCase.bits & DocumentedCasesOmitted) != 0);
if (!suppress) {
if (isEnhanced)
upperScope.problemReporter().enhancedSwitchMissingDefaultCase(this.expression);
else {
for (FieldBinding enumConstant : unenumeratedConstants) {
reportMissingEnumConstantCase(upperScope, enumConstant);
}
}
else {
for (FieldBinding enumConstant : unenumeratedConstants) {
reportMissingEnumConstantCase(upperScope, enumConstant);
}
}
}
}
if (this.defaultCase == null) {
if (ignoreMissingDefaultCase(compilerOptions, isEnumSwitch)) {
upperScope.methodScope().hasMissingSwitchDefault = true;
} else {
upperScope.problemReporter().missingDefaultCase(this, isEnumSwitch, expressionType);
}
}

if (this.defaultCase == null) {
if (ignoreMissingDefaultCase(compilerOptions)) {
upperScope.methodScope().hasMissingSwitchDefault = true;
} else {
upperScope.problemReporter().missingDefaultCase(this, true, selectorType);
}
}
} finally {
if (this.scope != null) this.scope.enclosingCase = null; // no longer inside switch case block
return;
}

if (isExhaustive() || this.defaultCase != null || selectorType == null) {
if (isEnhanced)
this.switchBits |= SwitchStatement.Exhaustive;
return;
}

if (JavaFeature.PATTERN_MATCHING_IN_SWITCH.isSupported(compilerOptions) && selectorType.isSealed() && caseElementsCoverSealedType((ReferenceBinding) selectorType, this.caseLabelElementTypes)) {
this.switchBits |= SwitchStatement.Exhaustive;
return;
}

if (selectorType.isRecordWithComponents() && this.containsRecordPatterns && caseElementsCoverRecordType(upperScope, compilerOptions, (ReferenceBinding) selectorType)) {
this.switchBits |= SwitchStatement.Exhaustive;
return;
}

if (!isExhaustive()) {
if (isEnhanced)
upperScope.problemReporter().enhancedSwitchMissingDefaultCase(this.expression);
else
upperScope.problemReporter().missingDefaultCase(this, false, selectorType);
}
}

// Return the set of enumerations belonging to the selector enum type that are not listed in case statements.
// Return the set of enumerations belonging to the selector enum type that are NOT listed in case statements.
private Set<FieldBinding> unenumeratedConstants(ReferenceBinding enumType, int constantCount) {
FieldBinding[] enumFields = ((ReferenceBinding) enumType.erasure()).fields();
Set<FieldBinding> unenumerated = new HashSet<>(Arrays.asList(enumFields));
Expand Down Expand Up @@ -1389,9 +1390,9 @@ private boolean isExhaustive() {
return (this.switchBits & SwitchStatement.Exhaustive) != 0;
}

private boolean checkAndSetEnhanced(BlockScope upperScope, TypeBinding expressionType) {
private boolean isEnhancedSwitch(BlockScope upperScope, TypeBinding expressionType) {
if (JavaFeature.PATTERN_MATCHING_IN_SWITCH.isSupported(upperScope.compilerOptions())
&& expressionType != null && !(this instanceof SwitchExpression )) {
&& expressionType != null && !(this instanceof SwitchExpression)) {

boolean acceptableType = !expressionType.isEnum();
switch (expressionType.id) {
Expand All @@ -1415,7 +1416,7 @@ private boolean checkAndSetEnhanced(BlockScope upperScope, TypeBinding expressio
return true;
}
}
if (expressionType != null && JavaFeature.PRIMITIVES_IN_PATTERNS.isSupported(upperScope.compilerOptions())) {
if (expressionType != null && !(this instanceof SwitchExpression) && JavaFeature.PRIMITIVES_IN_PATTERNS.isSupported(upperScope.compilerOptions())) {
switch (expressionType.id) {
case TypeIds.T_float:
case TypeIds.T_double:
Expand All @@ -1430,67 +1431,19 @@ private boolean checkAndSetEnhanced(BlockScope upperScope, TypeBinding expressio
}
return false;
}
private boolean checkAndFlagDefaultSealed(BlockScope skope, CompilerOptions compilerOptions) {
if (this.defaultCase != null) {
this.switchBits |= SwitchStatement.Exhaustive;
return false;
}
if (this.expression.resolvedType instanceof ReferenceBinding ref && ref.isRecord()) {
boolean isRecordPattern = false;
for (Pattern pattern : this.caseLabelElements) {
if (pattern instanceof RecordPattern) {
isRecordPattern = true;
break;
}
}
if (isRecordPattern)
return checkAndFlagDefaultRecord(skope, compilerOptions, ref);
}
boolean checkSealed = JavaFeature.SEALED_CLASSES.isSupported(compilerOptions)
&& JavaFeature.PATTERN_MATCHING_IN_SWITCH.isSupported(compilerOptions)
&& this.expression.resolvedType instanceof ReferenceBinding
&& this.expression.resolvedType.isSealed();

if (!checkSealed) return false;
ReferenceBinding ref = (ReferenceBinding) this.expression.resolvedType;
if (!(ref.isClass() || ref.isInterface() || ref.isTypeVariable() || ref.isIntersectionType()))
return false;

if (!caseElementsCoverSelectorType(ref.getAllEnumerableReferenceTypes(), this.caseLabelElementTypes)) {
if (this instanceof SwitchExpression) // non-exhaustive switch expressions will be flagged later.
return false;
skope.problemReporter().enhancedSwitchMissingDefaultCase(this.expression);
return true;
}
this.switchBits |= SwitchStatement.Exhaustive;
return false;
}
private boolean checkAndFlagDefaultRecord(BlockScope skope, CompilerOptions compilerOptions, ReferenceBinding ref) {
RecordComponentBinding[] comps = ref.components();
List<ReferenceBinding> allallowedTypes = new ArrayList<>();
allallowedTypes.add(ref);
if (comps == null || comps.length == 0) {
if (!caseElementsCoverSelectorType(allallowedTypes, this.caseLabelElementTypes)) {
skope.problemReporter().enhancedSwitchMissingDefaultCase(this.expression);
return true;
}
return false;
}
// non-zero components
RNode head = new RNode(ref);
private boolean caseElementsCoverRecordType(BlockScope skope, CompilerOptions compilerOptions, ReferenceBinding recordType) {
RNode head = new RNode(recordType);
for (Pattern pattern : this.caseLabelElements) {
head.addPattern(pattern);
}
CoverageCheckerVisitor ccv = new CoverageCheckerVisitor();
head.traverse(ccv);
if (!ccv.covers) {
skope.problemReporter().enhancedSwitchMissingDefaultCase(this.expression);
return true; // not exhaustive, error flagged
}
this.switchBits |= SwitchStatement.Exhaustive;
return false;
return ccv.covers;
}
private boolean caseElementsCoverSelectorType(List<ReferenceBinding> allAllowedTypes, List<TypeBinding> listedTypes) {

private boolean caseElementsCoverSealedType(ReferenceBinding sealedType, List<TypeBinding> listedTypes) {
List<ReferenceBinding> allAllowedTypes = sealedType.getAllEnumerableReferenceTypes();
Iterator<ReferenceBinding> iterator = allAllowedTypes.iterator();
while (iterator.hasNext()) {
ReferenceBinding next = iterator.next();
Expand Down Expand Up @@ -1554,7 +1507,7 @@ private void addSecretPatternSwitchVariables(BlockScope upperScope) {
protected void reportMissingEnumConstantCase(BlockScope upperScope, FieldBinding enumConstant) {
upperScope.problemReporter().missingEnumConstantCase(this, enumConstant);
}
protected boolean ignoreMissingDefaultCase(CompilerOptions compilerOptions, boolean isEnumSwitch) {
protected boolean ignoreMissingDefaultCase(CompilerOptions compilerOptions) {
return compilerOptions.getSeverity(CompilerOptions.MissingDefaultCase) == ProblemSeverities.Ignore;
}
@Override
Expand Down
2 changes: 1 addition & 1 deletion ...lipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/JavaFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public enum JavaFeature {

SEALED_CLASSES(ClassFileConstants.JDK17,
Messages.bind(Messages.sealed_types),
new char[][] {TypeConstants.SEALED, TypeConstants.PERMITS},
new char[][] {TypeConstants.SEALED, TypeConstants.NON_SEALED, TypeConstants.PERMITS},
false),
PATTERN_MATCHING_IN_SWITCH(ClassFileConstants.JDK21,
Messages.bind(Messages.pattern_matching_switch),
Expand Down
7 changes: 2 additions & 5 deletions ...dt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@
import org.eclipse.jdt.internal.compiler.ast.NullAnnotationMatching;
import org.eclipse.jdt.internal.compiler.classfmt.ClassFileConstants;
import org.eclipse.jdt.internal.compiler.impl.CompilerOptions;
import org.eclipse.jdt.internal.compiler.impl.JavaFeature;
import org.eclipse.jdt.internal.compiler.impl.ReferenceContext;
import org.eclipse.jdt.internal.compiler.util.SimpleLookupTable;

Expand Down Expand Up @@ -2368,10 +2367,8 @@ public MethodBinding getSingleAbstractMethod(Scope scope, boolean replaceWildcar
return this.singleAbstractMethod[index];
} else {
this.singleAbstractMethod = new MethodBinding[2];
// Sec 9.8 of sealed preview - A functional interface is an interface that is not declared sealed...
if (JavaFeature.SEALED_CLASSES.isSupported(scope.compilerOptions())
&& this.isSealed())
return this.singleAbstractMethod[index] = samProblemBinding;
if (this.isSealed())
return this.singleAbstractMethod[index] = samProblemBinding; // JLS 9.8
}

if (this.compoundName != null)
Expand Down
4 changes: 4 additions & 0 deletions ...pse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeBinding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,10 @@ public boolean isRecord() {
return false;
}

public boolean isRecordWithComponents() { // do records without components make sense ??!
return isRecord() && components() instanceof RecordComponentBinding [] components && components.length > 0;
}

/* Answer true if the receiver type can be assigned to the argument type (right)
*/
public boolean isCompatibleWith(TypeBinding right) {
Expand Down
1 change: 1 addition & 0 deletions ...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ public interface TypeConstants {
// JEP 360 Sealed
char[] PERMITS = "permits".toCharArray(); //$NON-NLS-1$
char[] SEALED = "sealed".toCharArray(); //$NON-NLS-1$
char[] NON_SEALED = "non-sealed".toCharArray(); //$NON-NLS-1$
String KEYWORD_EXTENDS = "extends"; //$NON-NLS-1$
String IMPLEMENTS = "implements"; //$NON-NLS-1$

Expand Down
1 change: 0 additions & 1 deletion org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9936,7 +9936,6 @@ protected void consumeToken(int type) {
pushOnIntStack(this.scanner.startPosition);
break;
case TokenNameRestrictedIdentifierpermits:
problemReporter().validateJavaFeatureSupport(JavaFeature.SEALED_CLASSES, this.scanner.startPosition,this.scanner.currentPosition - 1);
pushOnIntStack(this.scanner.startPosition);
break;
case TokenNamecase :
Expand Down
6 changes: 3 additions & 3 deletions ...ompiler/src/org/eclipse/jdt/core/tests/compiler/regression/PrimitiveInPatternsTestSH.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1919,7 +1919,7 @@ static int m1(boolean b) {
1. ERROR in X.java (at line 3)
return switch (b) {
^
An enhanced switch statement should be exhaustive; a default label expected
A switch expression should have a default case
----------
""");
}
Expand Down Expand Up @@ -2116,12 +2116,12 @@ static double m2(long l) {
1. ERROR in X.java (at line 3)
return switch(l) {
^
An enhanced switch statement should be exhaustive; a default label expected
A switch expression should have a default case
----------
2. ERROR in X.java (at line 9)
return switch(l) {
^
An enhanced switch statement should be exhaustive; a default label expected
A switch expression should have a default case
----------
""");
}
Expand Down
Loading
Loading