Skip to content

Commit

Permalink
[23] exhaustiveness signalling difference between javac and ecj (#2925)
Browse files Browse the repository at this point in the history 
+ precise implementation of TypePattern.isUnconditional()
   - remove implementations in parent / sibling classes
+ defer setting SwitchStatement.totalPattern until we know if a
  default case is present
  (otherwise we would generated inconsistent code for default).
+ clarify that flagDuplicateDefault() flags only conditionally,
  renamed to checkDuplicateDefault()
  - inside this method clarify that only one error is reported per loc.
+ remove all conflict reporting from CaseStatement.resolveCasePattern
  - will be done within SwitchStatement.resolve() anyway
+ remove IProblem.DuplicateTotalPattern and related code
  - all errors reported here coincided with a dominance error
+ adjust tests: no longer expect secondary errors

fixes #2915
  • Loading branch information
@stephan-herrmann
stephan-herrmann authored Sep 10, 2024
1 parent 5c3af42 commit 206c8e4
Show file tree
Hide file tree
Showing 14 changed files with 150 additions and 71 deletions.
6 changes: 6 additions & 0 deletions org.eclipse.jdt.core.compiler.batch/.settings/.api_filters
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@
<message_argument value="DisallowedStatementInPrologue"/>
</message_arguments>
</filter>
<filter comment="this preview constant (marked @noreference) is unnecesary" id="405864542">
<message_arguments>
<message_argument value="org.eclipse.jdt.core.compiler.IProblem"/>
<message_argument value="DuplicateTotalPattern"/>
</message_arguments>
</filter>
<filter comment="renamed constant for JEP 482" id="405864542">
<message_arguments>
<message_argument value="org.eclipse.jdt.core.compiler.IProblem"/>
Expand Down
3 changes: 0 additions & 3 deletions org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/core/compiler/IProblem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2537,9 +2537,6 @@ public interface IProblem {
/** @since 3.28
* @noreference preview feature error */
int EnhancedSwitchMissingDefault = PreviewRelated + 1908;
/** @since 3.28
* @noreference preview feature error */
int DuplicateTotalPattern = PreviewRelated + 1909;

/** @since 3.34
* @noreference preview feature error */
Expand Down
32 changes: 14 additions & 18 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 @@ -179,7 +179,7 @@ public ResolvedCase[] resolveCase(BlockScope scope, TypeBinding switchExpression
this.swich = switchStatement;
scope.enclosingCase = this; // record entering in a switch case block
if (this.constantExpressions == Expression.NO_EXPRESSIONS) {
flagDuplicateDefault(scope, switchStatement, this);
checkDuplicateDefault(scope, switchStatement, this);
return ResolvedCase.UnresolvedCase;
}

Expand All @@ -192,7 +192,7 @@ public ResolvedCase[] resolveCase(BlockScope scope, TypeBinding switchExpression
for (Expression e : this.constantExpressions) {
count++;
if (e instanceof FakeDefaultLiteral) {
flagDuplicateDefault(scope, switchStatement, this.constantExpressions.length > 1 ? e : this);
checkDuplicateDefault(scope, switchStatement, this.constantExpressions.length > 1 ? e : this);
if (count != 2 || nullCaseCount < 1) {
scope.problemReporter().patternSwitchCaseDefaultOnlyAsSecond(e);
}
Expand Down Expand Up @@ -267,16 +267,20 @@ public ResolvedCase[] resolveCase(BlockScope scope, TypeBinding switchExpression
return hasResolveErrors ? ResolvedCase.UnresolvedCase : cases.toArray(new ResolvedCase[cases.size()]);
}

private void flagDuplicateDefault(BlockScope scope, SwitchStatement switchStatement, ASTNode node) {
// remember the default case into the associated switch statement
if (switchStatement.defaultCase != null)
/**
* Precondition: this is a default case.
* Check if (a) another default or (b) a total type pattern has been recorded already
*/
private void checkDuplicateDefault(BlockScope scope, SwitchStatement switchStatement, ASTNode node) {
if (switchStatement.defaultCase != null) {
scope.problemReporter().duplicateDefaultCase(node);
} else if ((switchStatement.switchBits & SwitchStatement.TotalPattern) != 0) {
scope.problemReporter().illegalTotalPatternWithDefault(this);
}

// remember the default case into the associated switch statement
// on error the last default will be the selected one ...
switchStatement.defaultCase = this;
if ((switchStatement.switchBits & SwitchStatement.TotalPattern) != 0) {
scope.problemReporter().illegalTotalPatternWithDefault(this);
}
}

@Override
Expand Down Expand Up @@ -394,18 +398,10 @@ private Constant resolveCasePattern(BlockScope scope, TypeBinding caseType, Type
}
}
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, scope)) {
switchStatement.switchBits |= SwitchStatement.TotalPattern;
if (switchStatement.defaultCase != null && !(e instanceof RecordPattern))
scope.problemReporter().illegalTotalPatternWithDefault(this);
switchStatement.totalPattern = e;
}
e.isTotalTypeNode = true;
if (e.isUnconditional(expressionType, scope)) // unguarded is implied from 'coversType()' above
switchStatement.switchBits |= SwitchStatement.TotalPattern;
if (switchStatement.nullCase == null)
constant = IntConstant.fromValue(-1);
}
Expand Down
5 changes: 0 additions & 5 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 @@ -65,11 +65,6 @@ public boolean matchFailurePossible() {
return !isUnguarded() || this.primaryPattern.matchFailurePossible();
}

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

@Override
public boolean isUnguarded() {
Constant cst = this.condition.optimizedBooleanConstant();
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -117,7 +117,7 @@ public boolean matchFailurePossible() {
}

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

public abstract void generateCode(BlockScope currentScope, CodeStream codeStream, BranchLabel patternMatchLabel, BranchLabel matchFailLabel);
Expand Down
5 changes: 0 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 @@ -179,11 +179,6 @@ public boolean matchFailurePossible() {
return this.patterns.length != 0; // if no deconstruction is involved, no failure is possible.
}

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

@Override
public boolean dominates(Pattern p) {
/* 14.30.3: A record pattern with type R and pattern list L dominates another record pattern
Expand Down
9 changes: 7 additions & 2 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 @@ -1187,16 +1187,18 @@ public void resolve(BlockScope upperScope) {
LocalVariableBinding[] patternVariables = NO_VARIABLES;
boolean caseNullDefaultFound = false;
boolean defaultFound = false;
Pattern aTotalPattern = null;
for (int i = 0; i < length; i++) {
ResolvedCase[] constantsList;
final Statement statement = this.statements[i];
if (statement instanceof CaseStatement caseStmt) {
caseNullDefaultFound = caseNullDefaultFound ? caseNullDefaultFound
: isCaseStmtNullDefault(caseStmt);
caseNullDefaultFound |= isCaseStmtNullDefault(caseStmt);
defaultFound |= caseStmt.constantExpressions == Expression.NO_EXPRESSIONS;
constantsList = caseStmt.resolveCase(this.scope, expressionType, this);
patternVariables = statement.bindingsWhenTrue();
for (ResolvedCase c : constantsList) {
if (c.e instanceof Pattern p && p.isTotalTypeNode && p.isUnguarded)
aTotalPattern = p;
Constant con = c.c;
if (con == Constant.NotAConstant)
continue;
Expand Down Expand Up @@ -1272,6 +1274,9 @@ public void resolve(BlockScope upperScope) {
patternVariables = LocalVariableBinding.merge(patternVariables, statement.bindingsWhenComplete());
}
}
if (!defaultFound && aTotalPattern != null) {
this.totalPattern = aTotalPattern;
}
if (expressionType != null
&& (expressionType.id == TypeIds.T_boolean || expressionType.id == TypeIds.T_JavaLangBoolean)
&& defaultFound && isExhaustive()) {
Expand Down
33 changes: 33 additions & 0 deletions ...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/TypePattern.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@
import org.eclipse.jdt.internal.compiler.flow.FlowInfo;
import org.eclipse.jdt.internal.compiler.impl.Constant;
import org.eclipse.jdt.internal.compiler.impl.JavaFeature;
import org.eclipse.jdt.internal.compiler.lookup.BaseTypeBinding;
import org.eclipse.jdt.internal.compiler.lookup.Binding;
import org.eclipse.jdt.internal.compiler.lookup.BlockScope;
import org.eclipse.jdt.internal.compiler.lookup.ExtraCompilerModifiers;
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.Scope;
import org.eclipse.jdt.internal.compiler.lookup.TagBits;
import org.eclipse.jdt.internal.compiler.lookup.TypeBinding;
import org.eclipse.jdt.internal.compiler.lookup.TypeIds;
Expand Down Expand Up @@ -177,6 +179,37 @@ public void generateTestingConversion(BlockScope scope, CodeStream codeStream) {
break;
}
}

@Override
public boolean isUnconditional(TypeBinding t, Scope scope) {
// §14.30.3: A type pattern that declares a pattern variable of a type S is unconditional for a type T
// if there is a testing conversion that is unconditionally exact (5.7.2) from |T| to |S|.
// §5.7.2 lists:
// * an identity conversion
// * an exact widening primitive conversion
// * a widening reference conversion
// * a boxing conversion
// * a boxing conversion followed by a widening reference conversion
if (TypeBinding.equalsEquals(t, this.resolvedType))
return true;
PrimitiveConversionRoute route = findPrimitiveConversionRoute(this.resolvedType, t, scope);
return switch(route) {
case IDENTITY_CONVERSION,
BOXING_CONVERSION,
BOXING_CONVERSION_AND_WIDENING_REFERENCE_CONVERSION
-> true;
case WIDENING_PRIMITIVE_CONVERSION -> BaseTypeBinding.isExactWidening(this.resolvedType.id, t.id);
case NO_CONVERSION_ROUTE -> { // a widening reference conversion?
if (!this.resolvedType.isPrimitiveOrBoxedPrimitiveType() || !t.isPrimitiveOrBoxedPrimitiveType()) {
yield t.isCompatibleWith(this.resolvedType);
} else {
yield false;
}
}
default -> false;
};
}

@Override
public boolean dominates(Pattern p) {
if (!isUnguarded())
Expand Down
8 changes: 0 additions & 8 deletions ...dt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12597,14 +12597,6 @@ public void enhancedSwitchMissingDefaultCase(ASTNode element) {
element.sourceStart,
element.sourceEnd);
}
public void duplicateTotalPattern(ASTNode element) {
this.handle(
IProblem.DuplicateTotalPattern,
NoArgument,
NoArgument,
element.sourceStart,
element.sourceEnd);
}
public void unexpectedTypeinSwitchPattern(TypeBinding type, ASTNode element) {
this.handle(
IProblem.UnexpectedTypeinSwitchPattern,
Expand Down
2 changes: 1 addition & 1 deletion ...jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/messages.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1156,7 +1156,7 @@
1906 = This case label is dominated by one of the preceding case labels
1907 = Switch case cannot have both unconditional pattern and default label
1908 = An enhanced switch statement should be exhaustive; a default label expected
1909 = The switch statement cannot have more than one unconditional pattern
# 1909 removed
1910 = Unnecessary 'null' pattern, the switch selector expression cannot be null
1911 = Unexpected type {0}, expected class or array type
1920 = A null case label has to be either the only expression in a case label or the first expression followed only by a default
Expand Down
2 changes: 0 additions & 2 deletions ....compiler/src/org/eclipse/jdt/core/tests/compiler/regression/CompilerInvocationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1340,7 +1340,6 @@ class ProblemAttributes {
expectedProblemAttributes.put("PatternDominated", new ProblemAttributes(true));
expectedProblemAttributes.put("IllegalTotalPatternWithDefault", new ProblemAttributes(true));
expectedProblemAttributes.put("EnhancedSwitchMissingDefault", new ProblemAttributes(true));
expectedProblemAttributes.put("DuplicateTotalPattern", new ProblemAttributes(true));
expectedProblemAttributes.put("UnexpectedTypeinSwitchPattern", new ProblemAttributes(true));
expectedProblemAttributes.put("UnexpectedTypeinRecordPattern", new ProblemAttributes(true));
expectedProblemAttributes.put("RecordPatternMismatch", new ProblemAttributes(true));
Expand Down Expand Up @@ -2474,7 +2473,6 @@ class ProblemAttributes {
expectedProblemAttributes.put("PatternDominated", SKIP);
expectedProblemAttributes.put("IllegalTotalPatternWithDefault", SKIP);
expectedProblemAttributes.put("EnhancedSwitchMissingDefault", SKIP);
expectedProblemAttributes.put("DuplicateTotalPattern", SKIP);
expectedProblemAttributes.put("UnexpectedTypeinSwitchPattern", SKIP);
expectedProblemAttributes.put("UnexpectedTypeinRecordPattern", SKIP);
expectedProblemAttributes.put("RecordPatternMismatch", SKIP);
Expand Down
88 changes: 88 additions & 0 deletions ...ompiler/src/org/eclipse/jdt/core/tests/compiler/regression/PrimitiveInPatternsTestSH.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2220,6 +2220,94 @@ public static void main(String... args) {

}

public void testCoversTypePlusDefault() {
// case int i "covers" type Integer but is not unconditional
runConformTest(new String[] {
"X.java",
"""
public class X {
public static int foo(Integer myInt) {
return switch (myInt) {
case int i -> i;
default -> 0;
};
}
public static void main(String argv[]) {
Integer i = 100;
System.out.println(X.foo(i) == i);
}
}
"""
},
"true");
}

public void testUnconditionPlusDefault() {
// case int i "covers" type int and is unconditional
// various combinations of dominance with/without default
runNegativeTest(new String[] {
"X.java",
"""
public class X {
int foo1(int myInt) {
return switch (myInt) {
case int i -> i;
default -> 0; // conflict with preceding total pattern (unguarded and unconditional)
};
}
int foo2(int myInt) {
return switch (myInt) { // swapped order of cases
default -> 0;
case int i -> i; // conflict with preceding default
};
}
int foo3(int myInt) {
return switch (myInt) {
default -> 0;
case int i -> i; // conflict with preceding default
case short s -> s; // additionally dominated by int i
};
}
int foo4(int myInt) {
return switch (myInt) {
case int i -> i;
case short s -> s; // dominated by int i
};
}
}
"""
},
"""
----------
1. ERROR in X.java (at line 5)
default -> 0; // conflict with preceding total pattern (unguarded and unconditional)
^^^^^^^
Switch case cannot have both unconditional pattern and default label
----------
2. ERROR in X.java (at line 11)
case int i -> i; // conflict with preceding default
^^^^^
This case label is dominated by one of the preceding case labels
----------
3. ERROR in X.java (at line 17)
case int i -> i; // conflict with preceding default
^^^^^
This case label is dominated by one of the preceding case labels
----------
4. ERROR in X.java (at line 18)
case short s -> s; // additionally dominated by int i
^^^^^^^
This case label is dominated by one of the preceding case labels
----------
5. ERROR in X.java (at line 24)
case short s -> s; // dominated by int i
^^^^^^^
This case label is dominated by one of the preceding case labels
----------
""");
}

// test from spec
public void _testSpec001() {
runConformTest(new String[] {
Expand Down
6 changes: 0 additions & 6 deletions ....tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/RecordPatternTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -367,12 +367,6 @@ public void test009() {
" case Rectangle(ColoredPoint(Point(int x, int y), Color c),\n" +
" ColoredPoint(Point(int x1, int y1), Color c1)) -> {\n" +
" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" +
"The switch statement cannot have more than one unconditional pattern\n" +
"----------\n" +
"2. ERROR in X.java (at line 7)\n" +
" case Rectangle(ColoredPoint(Point(int x, int y), Color c),\n" +
" ColoredPoint(Point(int x1, int y1), Color c1)) -> {\n" +
" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n" +
"This case label is dominated by one of the preceding case labels\n" +
"----------\n");
}
Expand Down
Loading

0 comments on commit 206c8e4

Please sign in to comment.