From d1047d72bf393a5ebdc98d529317d69dc22dbdb2 Mon Sep 17 00:00:00 2001 From: Srikanth Sankaran Date: Tue, 15 Oct 2024 08:14:09 +0530 Subject: [PATCH] [Sealed Types + Enhanced Switch] Incorrect diagnostic about switch not being exhaustive * Fixes https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2720 --- .../compiler/ast/SwitchStatement.java | 118 ++++++++----- .../compiler/lookup/ReferenceBinding.java | 3 +- .../regression/SwitchPatternTest.java | 163 ++++++++++++++++++ 3 files changed, 243 insertions(+), 41 deletions(-) diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java index 537fb6e2591..5b17e769513 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java @@ -25,7 +25,10 @@ import java.lang.invoke.ConstantBootstraps; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashSet; +import java.util.Iterator; import java.util.List; +import java.util.Set; import java.util.function.Function; import java.util.function.IntPredicate; import org.eclipse.jdt.core.compiler.CharOperation; @@ -43,7 +46,15 @@ import org.eclipse.jdt.internal.compiler.impl.CompilerOptions; import org.eclipse.jdt.internal.compiler.impl.Constant; import org.eclipse.jdt.internal.compiler.impl.JavaFeature; -import org.eclipse.jdt.internal.compiler.lookup.*; +import org.eclipse.jdt.internal.compiler.lookup.BlockScope; +import org.eclipse.jdt.internal.compiler.lookup.FieldBinding; +import org.eclipse.jdt.internal.compiler.lookup.LocalVariableBinding; +import org.eclipse.jdt.internal.compiler.lookup.RecordComponentBinding; +import org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding; +import org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding; +import org.eclipse.jdt.internal.compiler.lookup.SyntheticMethodBinding; +import org.eclipse.jdt.internal.compiler.lookup.TypeBinding; +import org.eclipse.jdt.internal.compiler.lookup.TypeIds; import org.eclipse.jdt.internal.compiler.problem.ProblemSeverities; @SuppressWarnings("rawtypes") @@ -1307,23 +1318,18 @@ && defaultFound && isExhaustive()) { ((this.containsPatterns || this.containsNull) || (constantCount >= this.caseCount && constantCount != ((ReferenceBinding)expressionType).enumConstantCount()))) { - FieldBinding[] enumFields = ((ReferenceBinding) expressionType.erasure()).fields(); - for (int i = 0, max = enumFields.length; i < max; i++) { - FieldBinding enumConstant = enumFields[i]; - if ((enumConstant.modifiers & ClassFileConstants.AccEnum) == 0) continue; - findConstant : { - for (int j = 0; j < constantCount; j++) { - if ((enumConstant.id + 1) == this.otherConstants[j].c.intValue()) // zero should not be returned see bug 141810 - break findConstant; - } - 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 + Set 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); + } } } } @@ -1341,6 +1347,29 @@ && defaultFound && isExhaustive()) { if (this.scope != null) this.scope.enclosingCase = null; // no longer inside switch case block } } + + private Set unenumeratedConstants(ReferenceBinding enumType, int constantCount) { + FieldBinding[] enumFields = ((ReferenceBinding) enumType.erasure()).fields(); + Set unenumerated = new HashSet<>(Arrays.asList(enumFields)); + for (int i = 0, max = enumFields.length; i < max; i++) { + FieldBinding enumConstant = enumFields[i]; + if ((enumConstant.modifiers & ClassFileConstants.AccEnum) == 0) + continue; + for (int j = 0; j < constantCount; j++) { + if (TypeBinding.equalsEquals(this.otherConstants[j].e.resolvedType, enumType)) { + if (this.otherConstants[j].e instanceof NameReference reference) { + FieldBinding field = reference.fieldBinding(); + int intValue = field.original().id + 1; + if ((enumConstant.id + 1) == intValue) { // zero should not be returned see bug 141810 + unenumerated.remove(enumConstant); + break; + } + } + } + } + } + return unenumerated; + } private boolean isCaseStmtNullDefault(CaseStatement caseStmt) { return caseStmt != null && caseStmt.constantExpressions.length == 2 @@ -1398,20 +1427,11 @@ private boolean checkAndSetEnhanced(BlockScope upperScope, TypeBinding expressio return false; } private boolean checkAndFlagDefaultSealed(BlockScope skope, CompilerOptions compilerOptions) { - if (this.defaultCase != null) { // mark covered as a side effect (since covers is intro in 406) + if (this.defaultCase != null) { this.switchBits |= SwitchStatement.Exhaustive; return false; } - boolean checkSealed = this.containsPatterns - && JavaFeature.SEALED_CLASSES.isSupported(compilerOptions) - && JavaFeature.PATTERN_MATCHING_IN_SWITCH.isSupported(compilerOptions) - && this.expression.resolvedType instanceof ReferenceBinding; - if (!checkSealed) return false; - ReferenceBinding ref = (ReferenceBinding) this.expression.resolvedType; - if (!(ref.isClass() || ref.isInterface() || ref.isTypeVariable() || ref.isIntersectionType())) - return false; - - if (ref.isRecord()) { + if (this.expression.resolvedType instanceof ReferenceBinding ref && ref.isRecord()) { boolean isRecordPattern = false; for (Pattern pattern : this.caseLabelElements) { if (pattern instanceof RecordPattern) { @@ -1422,7 +1442,17 @@ private boolean checkAndFlagDefaultSealed(BlockScope skope, CompilerOptions comp if (isRecordPattern) return checkAndFlagDefaultRecord(skope, compilerOptions, ref); } - if (!ref.isSealed()) return false; + 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 (!isExhaustiveWithCaseTypes(ref.getAllEnumerableReferenceTypes(), this.caseLabelElementTypes)) { if (this instanceof SwitchExpression) // non-exhaustive switch expressions will be flagged later. return false; @@ -1458,25 +1488,33 @@ private boolean checkAndFlagDefaultRecord(BlockScope skope, CompilerOptions comp return false; } private boolean isExhaustiveWithCaseTypes(List allAllowedTypes, List listedTypes) { - int pendingTypes = allAllowedTypes.size(); - for (ReferenceBinding pt : allAllowedTypes) { - /* Per JLS 14.11.1.1: A type T that names an abstract sealed class or sealed interface is covered - if every permitted direct subclass or subinterface of it is covered. These subtypes are already - added to allAllowedTypes and subject to cover test. - */ - if (pt.isAbstract() && pt.isSealed()) { - --pendingTypes; + Iterator iterator = allAllowedTypes.iterator(); + while (iterator.hasNext()) { + ReferenceBinding next = iterator.next(); + if (next.isAbstract() && next.isSealed()) { + /* Per JLS 14.11.1.1: A type T that names an abstract sealed class or sealed interface is covered + if every permitted direct subclass or subinterface of it is covered. These subtypes are already + added to allAllowedTypes and subject to cover test. + */ + iterator.remove(); continue; } for (TypeBinding type : listedTypes) { // permits specifies classes, not parameterizations - if (pt.erasure().isCompatibleWith(type.erasure())) { - --pendingTypes; + if (next.erasure().isCompatibleWith(type.erasure())) { + iterator.remove(); break; } } + if (next.isEnum()) { + int constantCount = this.otherConstants == null ? 0 : this.otherConstants.length; + Set unenumeratedConstants = unenumeratedConstants(next, constantCount); + if (unenumeratedConstants.size() == 0) { + iterator.remove(); + } + } } - return pendingTypes == 0; + return allAllowedTypes.size() == 0; } private boolean needPatternDispatchCopy() { if (this.containsPatterns || (this.switchBits & QualifiedEnum) != 0) diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java index 71e27ac86f2..a0a9cc5e1d1 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java @@ -52,6 +52,7 @@ *******************************************************************************/ package org.eclipse.jdt.internal.compiler.lookup; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Comparator; @@ -2563,7 +2564,7 @@ public List getAllEnumerableReferenceTypes() { oldSet = permSet; permSet = tmp; } while (oldSet.size() != permSet.size()); - return Arrays.asList(permSet.toArray(new ReferenceBinding[0])); + return new ArrayList<>(permSet); } // 5.1.6.1 Allowed Narrowing Reference Conversion diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/SwitchPatternTest.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/SwitchPatternTest.java index caab7826379..632b172d247 100644 --- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/SwitchPatternTest.java +++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/SwitchPatternTest.java @@ -8864,4 +8864,167 @@ final class A implements I { "Type I cannot be safely cast to B\n" + "----------\n"); } + + // https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2720 + // [Sealed Types + Enhanced Switch] Incorrect diagnostic about switch not being exhaustive + public void testIssue2720() { + runConformTest( + new String[] { + "X.java", + """ + sealed interface I { + + enum E implements I { + A, B, C; + } + } + + public class X { + + static void d(I i) { + switch (i) { // error: An enhanced switch statement should be exhaustive; a default label expected + case I.E.A -> { System.out.println("I.E.A"); } + case I.E.B -> { System.out.println("I.E.B"); } + case I.E.C -> { System.out.println("I.E.C"); } + } + } + + public static void main(String [] args) { + d(I.E.A); + d(I.E.B); + d(I.E.C); + } + } + """ + }, + "I.E.A\n" + + "I.E.B\n" + + "I.E.C"); + } + + // https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2720 + // [Sealed Types + Enhanced Switch] Incorrect diagnostic about switch not being exhaustive + public void testIssue2720_2() { + runNegativeTest( + new String[] { + "X.java", + """ + sealed interface I { + + enum E implements I { + A, B, C; + } + + enum K implements I { + D, E, F; + } + } + + class Test { + + void d(I i) { + switch (i) { + case I.E.A -> {} + case I.E.B -> {} + case I.E.C -> {} + case I.K k -> {} + } + switch (i) { + case I.E.A -> {} + case I.E.B -> {} + case I.E.C -> {} + default -> {} + } + switch (i) { + case I.E.A -> {} + case I.E.B -> {} + case I.E.C -> {} + case I.K.D -> {} + case I.K.E -> {} + case I.K.F -> {} + default -> {} + } + switch (i) { + case I.E.A -> {} + case I.E.B -> {} + case I.E.C -> {} + case I.K.D -> {} + case I.K.E -> {} + default -> {} + } + switch (i) { + case I.E.A -> {} + case I.E.B -> {} + case I.E.C -> {} + case I.K.D -> {} + case I.K.E -> {} + } + switch (i) { + case I.E.A -> {} + case I.E.B -> {} + case I.E.C -> {} + case I.K.D -> {} + case I.K.E -> {} + case I.K.F -> {} + } + } + } + """ + }, + "----------\n" + + "1. ERROR in X.java (at line 44)\n" + + " switch (i) {\n" + + " ^\n" + + "An enhanced switch statement should be exhaustive; a default label expected\n" + + "----------\n"); + } + // https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2720 + // [Sealed Types + Enhanced Switch] Incorrect diagnostic about switch not being exhaustive + public void testIssue2720_3() { + runConformTest( + new String[] { + "X.java", + """ + sealed interface I { + + enum E implements I { + A, B, C; + } + + enum K implements I { + D, E, F; + } + } + + public class X { + + static void d(I i) { + switch (i) { + case I.E.A -> { System.out.println("I.E.A"); } + case I.E.B -> { System.out.println("I.E.B"); } + case I.E.C -> { System.out.println("I.E.C"); } + case I.K.D -> { System.out.println("I.K.D"); } + case I.K.E -> { System.out.println("I.K.E"); } + case I.K.F -> { System.out.println("I.K.F"); } + } + } + + public static void main(String [] args) { + d(I.E.A); + d(I.E.B); + d(I.E.C); + d(I.K.D); + d(I.K.E); + d(I.K.F); + } + } + """ + }, + "I.E.A\n" + + "I.E.B\n" + + "I.E.C\n" + + "I.K.D\n" + + "I.K.E\n" + + "I.K.F"); + } }