Skip to content

Commit

Permalink
8311815: Incorrect exhaustivity computation
Browse files Browse the repository at this point in the history
Reviewed-by: vromero
  • Loading branch information
lahodaj committed Jul 17, 2023
1 parent 1c9691b commit a441216
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 47 deletions.
93 changes: 48 additions & 45 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java
Original file line number Diff line number Diff line change
Expand Up @@ -773,15 +773,16 @@ private boolean exhausts(JCExpression selector, List<JCCase> cases) {
patternSet.add(new BindingPattern(e.getKey().type));
}
}
List<PatternDescription> patterns = List.from(patternSet);
Set<PatternDescription> patterns = patternSet;
try {
boolean repeat = true;
while (repeat) {
List<PatternDescription> updatedPatterns;
Set<PatternDescription> updatedPatterns;
updatedPatterns = reduceBindingPatterns(selector.type, patterns);
updatedPatterns = reduceNestedPatterns(updatedPatterns);
updatedPatterns = reduceRecordPatterns(updatedPatterns);
repeat = updatedPatterns != patterns;
updatedPatterns = removeCoveredRecordPatterns(updatedPatterns);
repeat = !updatedPatterns.equals(patterns);
patterns = updatedPatterns;
if (checkCovered(selector.type, patterns)) {
return true;
Expand All @@ -794,7 +795,7 @@ private boolean exhausts(JCExpression selector, List<JCCase> cases) {
}
}

private boolean checkCovered(Type seltype, List<PatternDescription> patterns) {
private boolean checkCovered(Type seltype, Iterable<PatternDescription> patterns) {
for (Type seltypeComponent : components(seltype)) {
for (PatternDescription pd : patterns) {
if (pd instanceof BindingPattern bp &&
Expand Down Expand Up @@ -830,15 +831,14 @@ private List<Type> components(Type seltype) {
* is found, it is removed, and replaced with a binding pattern
* for the sealed supertype.
*/
private List<PatternDescription> reduceBindingPatterns(Type selectorType, List<PatternDescription> patterns) {
private Set<PatternDescription> reduceBindingPatterns(Type selectorType, Set<PatternDescription> patterns) {
Set<Symbol> existingBindings = patterns.stream()
.filter(pd -> pd instanceof BindingPattern)
.map(pd -> ((BindingPattern) pd).type.tsym)
.collect(Collectors.toSet());

for (PatternDescription pdOne : patterns) {
if (pdOne instanceof BindingPattern bpOne) {
Set<PatternDescription> toRemove = new HashSet<>();
Set<PatternDescription> toAdd = new HashSet<>();

for (Type sup : types.directSupertypes(bpOne.type)) {
Expand Down Expand Up @@ -871,7 +871,6 @@ private List<PatternDescription> reduceBindingPatterns(Type selectorType, List<P

for (PatternDescription pdOther : patterns) {
if (pdOther instanceof BindingPattern bpOther) {
boolean reduces = false;
Set<Symbol> currentPermittedSubTypes =
allPermittedSubTypes((ClassSymbol) bpOther.type.tsym, s -> true);

Expand All @@ -888,33 +887,21 @@ private List<PatternDescription> reduceBindingPatterns(Type selectorType, List<P
if (types.isSubtype(types.erasure(perm.type),
types.erasure(bpOther.type))) {
it.remove();
reduces = true;
}
}

if (reduces) {
if (!types.isSubtype(types.erasure(clazz.type), types.erasure(bpOther.type))) {
bindings.append(pdOther);

This comment has been minimized.

Copy link
@smowton

smowton Oct 5, 2023

@lahodaj suggested cleanup: this leaves the declaration ListBuffer<PatternDescription> bindings = new ListBuffer<>(); unused.

}
}
}
}

if (permitted.isEmpty()) {
toRemove.addAll(bindings);
toAdd.add(new BindingPattern(clazz.type));
}
}
}

if (!toAdd.isEmpty() || !toRemove.isEmpty()) {
for (PatternDescription pd : toRemove) {
patterns = List.filter(patterns, pd);
}
for (PatternDescription pd : toAdd) {
patterns = patterns.prepend(pd);
}
return patterns;
if (!toAdd.isEmpty()) {
Set<PatternDescription> newPatterns = new HashSet<>(patterns);
newPatterns.addAll(toAdd);
return newPatterns;
}
}
}
Expand Down Expand Up @@ -958,7 +945,7 @@ private Set<Symbol> allPermittedSubTypes(ClassSymbol root, Predicate<ClassSymbol
* of patterns is replaced with a new set of patterns of the form:
* $record($prefix$, $resultOfReduction, $suffix$)
*/
private List<PatternDescription> reduceNestedPatterns(List<PatternDescription> patterns) {
private Set<PatternDescription> reduceNestedPatterns(Set<PatternDescription> patterns) {
/* implementation note:
* finding a sub-set of patterns that only differ in a single
* column is time-consuming task, so this method speeds it up by:
Expand All @@ -977,13 +964,14 @@ private List<PatternDescription> reduceNestedPatterns(List<PatternDescription> p

for (var e : groupByRecordClass.entrySet()) {
int nestedPatternsCount = e.getKey().getRecordComponents().size();
Set<RecordPattern> current = new HashSet<>(e.getValue());

for (int mismatchingCandidate = 0;
mismatchingCandidate < nestedPatternsCount;
mismatchingCandidate++) {
int mismatchingCandidateFin = mismatchingCandidate;
var groupByHashes =
e.getValue()
current
.stream()
//error recovery, ignore patterns with incorrect number of nested patterns:
.filter(pd -> pd.nested.length == nestedPatternsCount)
Expand Down Expand Up @@ -1018,37 +1006,35 @@ private List<PatternDescription> reduceNestedPatterns(List<PatternDescription> p
}
}

var nestedPatterns = join.stream().map(rp -> rp.nested[mismatchingCandidateFin]).collect(List.collector());
var nestedPatterns = join.stream().map(rp -> rp.nested[mismatchingCandidateFin]).collect(Collectors.toSet());
var updatedPatterns = reduceNestedPatterns(nestedPatterns);

updatedPatterns = reduceRecordPatterns(updatedPatterns);
updatedPatterns = removeCoveredRecordPatterns(updatedPatterns);
updatedPatterns = reduceBindingPatterns(rpOne.fullComponentTypes()[mismatchingCandidateFin], updatedPatterns);

if (nestedPatterns != updatedPatterns) {
ListBuffer<PatternDescription> result = new ListBuffer<>();
Set<PatternDescription> toRemove = Collections.newSetFromMap(new IdentityHashMap<>());

toRemove.addAll(join);

for (PatternDescription p : patterns) {
if (!toRemove.contains(p)) {
result.append(p);
}
}
if (!nestedPatterns.equals(updatedPatterns)) {
current.removeAll(join);

for (PatternDescription nested : updatedPatterns) {
PatternDescription[] newNested =
Arrays.copyOf(rpOne.nested, rpOne.nested.length);
newNested[mismatchingCandidateFin] = nested;
result.append(new RecordPattern(rpOne.recordType(),
current.add(new RecordPattern(rpOne.recordType(),
rpOne.fullComponentTypes(),
newNested));
}
return result.toList();
}
}
}
}

if (!current.equals(new HashSet<>(e.getValue()))) {
Set<PatternDescription> result = new HashSet<>(patterns);
result.removeAll(e.getValue());
result.addAll(current);
return result;
}
}
return patterns;
}
Expand All @@ -1058,22 +1044,22 @@ private List<PatternDescription> reduceNestedPatterns(List<PatternDescription> p
* all the $nestedX pattern cover the given record component,
* and replace those with a simple binding pattern over $record.
*/
private List<PatternDescription> reduceRecordPatterns(List<PatternDescription> patterns) {
var newPatterns = new ListBuffer<PatternDescription>();
private Set<PatternDescription> reduceRecordPatterns(Set<PatternDescription> patterns) {
var newPatterns = new HashSet<PatternDescription>();
boolean modified = false;
for (PatternDescription pd : patterns) {
if (pd instanceof RecordPattern rpOne) {
PatternDescription reducedPattern = reduceRecordPattern(rpOne);
if (reducedPattern != rpOne) {
newPatterns.append(reducedPattern);
newPatterns.add(reducedPattern);
modified = true;
continue;
}
}
newPatterns.append(pd);
newPatterns.add(pd);
}
return modified ? newPatterns.toList() : patterns;
}
return modified ? newPatterns : patterns;
}

private PatternDescription reduceRecordPattern(PatternDescription pattern) {
if (pattern instanceof RecordPattern rpOne) {
Expand Down Expand Up @@ -1105,6 +1091,23 @@ private PatternDescription reduceRecordPattern(PatternDescription pattern) {
return pattern;
}

private Set<PatternDescription> removeCoveredRecordPatterns(Set<PatternDescription> patterns) {
Set<Symbol> existingBindings = patterns.stream()
.filter(pd -> pd instanceof BindingPattern)
.map(pd -> ((BindingPattern) pd).type.tsym)
.collect(Collectors.toSet());
Set<PatternDescription> result = new HashSet<>(patterns);

for (Iterator<PatternDescription> it = result.iterator(); it.hasNext();) {
PatternDescription pd = it.next();
if (pd instanceof RecordPattern rp && existingBindings.contains(rp.recordType.tsym)) {
it.remove();
}
}

return result;
}

public void visitTry(JCTry tree) {
ListBuffer<PendingExit> prevPendingExits = pendingExits;
pendingExits = new ListBuffer<>();
Expand Down
32 changes: 30 additions & 2 deletions test/langtools/tools/javac/patterns/Exhaustiveness.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

/**
* @test
* @bug 8262891 8268871 8274363 8281100 8294670 8311038
* @bug 8262891 8268871 8274363 8281100 8294670 8311038 8311815
* @summary Check exhaustiveness of switches over sealed types.
* @library /tools/lib
* @modules jdk.compiler/com.sun.tools.javac.api
Expand Down Expand Up @@ -1540,7 +1540,7 @@ int test(Object o) {
"1 error");
}

private static final int NESTING_CONSTANT = 5;
private static final int NESTING_CONSTANT = 4;

Set<String> createDeeplyNestedVariants() {
Set<String> variants = new HashSet<>();
Expand Down Expand Up @@ -1968,6 +1968,34 @@ record Rec(Object o) {}
""");
}

@Test //JDK-8311815:
public void testAmbiguousRecordUsage(Path base) throws Exception {
doTest(base,
new String[0],
"""
package test;
public class Test {
record Pair(I i1, I i2) {}
sealed interface I {}
record C() implements I {}
record D() implements I {}
void exhaustinvenessWithInterface(Pair pairI) {
switch (pairI) {
case Pair(D fst, C snd) -> {
}
case Pair(C fst, C snd) -> {
}
case Pair(C fst, I snd) -> {
}
case Pair(D fst, D snd) -> {
}
}
}
}
""");
}

private void doTest(Path base, String[] libraryCode, String testCode, String... expectedErrors) throws IOException {
doTest(base, libraryCode, testCode, false, expectedErrors);
}
Expand Down

1 comment on commit a441216

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.