Skip to content
This repository has been archived by the owner on Sep 19, 2023. It is now read-only.

8289196: Pattern domination not working properly for record patterns #84

Closed
wants to merge 3 commits into from
Closed

8289196: Pattern domination not working properly for record patterns #84

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d22eaf
8289196: Patttern domination not working property for record patterns
lahodaj Jun 28, 2022
220fcc1
Adding test, as suggested.
lahodaj Jul 1, 2022
bf740ed
Simplifying primitive pattern handling as suggested
lahodaj Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 4 additions & 24 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
View file
Open in desktop
Expand Up @@ -1694,9 +1694,7 @@ private void handleSwitch(JCTree switchTree,

// Attribute all cases and
// check that there are no duplicate case labels or default clauses.
Set<Object> labels = new HashSet<>(); // The set of case labels.
List<Type> coveredTypesForPatterns = List.nil();
List<Type> coveredTypesForConstants = List.nil();
Set<Object> constants = new HashSet<>(); // The set of case constants.
boolean hasDefault = false; // Is there a default label?
boolean hasUnconditionalPattern = false; // Is there a unconditional pattern?
boolean lastPatternErroneous = false; // Has the last pattern erroneous type?
Expand Down Expand Up @@ -1732,10 +1730,8 @@ private void handleSwitch(JCTree switchTree,
Symbol sym = enumConstant(expr, seltype);
if (sym == null) {
log.error(expr.pos(), Errors.EnumLabelMustBeUnqualifiedEnum);
} else if (!labels.add(sym)) {
} else if (!constants.add(sym)) {
log.error(label.pos(), Errors.DuplicateCaseLabel);
} else {
checkCaseLabelDominated(label.pos(), coveredTypesForConstants, sym.type);
}
} else if (errorEnumSwitch) {
//error recovery: the selector is erroneous, and all the case labels
Expand Down Expand Up @@ -1765,10 +1761,8 @@ private void handleSwitch(JCTree switchTree,
}
} else if (!stringSwitch && !types.isAssignable(seltype, syms.intType)) {
log.error(label.pos(), Errors.ConstantLabelNotCompatible(pattype, seltype));
} else if (!labels.add(pattype.constValue())) {
} else if (!constants.add(pattype.constValue())) {
log.error(c.pos(), Errors.DuplicateCaseLabel);
} else {
checkCaseLabelDominated(label.pos(), coveredTypesForConstants, types.boxedTypeOrType(pattype));
}
}
}
Expand Down Expand Up @@ -1820,13 +1814,6 @@ private void handleSwitch(JCTree switchTree,
hasUnconditionalPattern = true;
}
lastPatternErroneous = patternType.isErroneous();
checkCaseLabelDominated(pat.pos(), coveredTypesForPatterns, patternType);
if (!patternType.isErroneous()) {
coveredTypesForConstants = coveredTypesForConstants.prepend(patternType);
if (unguarded) {
coveredTypesForPatterns = coveredTypesForPatterns.prepend(patternType);
}
}
} else {
Assert.error();
}
Expand All @@ -1850,6 +1837,7 @@ private void handleSwitch(JCTree switchTree,
}
if (patternSwitch) {
chk.checkSwitchCaseStructure(cases);
chk.checkSwitchCaseLabelDominated(cases);
}
if (switchTree.hasTag(SWITCH)) {
((JCSwitch) switchTree).hasUnconditionalPattern =
Expand All @@ -1875,14 +1863,6 @@ private static void addVars(List<JCStatement> stats, WriteableScope switchScope)
switchScope.enter(((JCVariableDecl) stat).sym);
}
}
private void checkCaseLabelDominated(DiagnosticPosition pos,
List<Type> coveredTypes, Type patternType) {
for (Type existing : coveredTypes) {
if (types.isSubtype(patternType, existing)) {
log.error(pos, Errors.PatternDominated);
}
}
}
// where
/** Return the selected enumeration constant symbol, or null. */
private Symbol enumConstant(JCTree tree, Type enumType) {
Expand Down
89 changes: 89 additions & 0 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
View file
Open in desktop
Expand Up @@ -4373,6 +4373,95 @@ void checkSwitchCaseStructure(List<JCCase> cases) {
}
}

void checkSwitchCaseLabelDominated(List<JCCase> cases) {
List<JCCaseLabel> caseLabels = List.nil();
for (List<JCCase> l = cases; l.nonEmpty(); l = l.tail) {
JCCase c = l.head;
for (JCCaseLabel label : c.labels) {
if (label.hasTag(DEFAULTCASELABEL) || TreeInfo.isNullCaseLabel(label)) {
continue;
}
Type currentType = labelType(label);
for (JCCaseLabel testCaseLabel : caseLabels) {
Type testType = labelType(testCaseLabel);
if (types.isSubtype(currentType, testType) &&
!currentType.hasTag(ERROR) && !testType.hasTag(ERROR)) {
//the current label is potentially dominated by the existing (test) label, check:
boolean dominated = false;
if (label instanceof JCConstantCaseLabel) {
dominated |= !(testCaseLabel instanceof JCConstantCaseLabel);
} else if (label instanceof JCPatternCaseLabel patternCL &&
testCaseLabel instanceof JCPatternCaseLabel testPatternCaseLabel &&
TreeInfo.unguardedCaseLabel(testCaseLabel)) {
dominated = patternDominated(testPatternCaseLabel.pat,
patternCL.pat);
}
if (dominated) {
log.error(label.pos(), Errors.PatternDominated);
}
}
}
caseLabels = caseLabels.prepend(label);
}
}
}
//where:
private Type labelType(JCCaseLabel label) {
return types.erasure(switch (label.getTag()) {
case PATTERNCASELABEL -> ((JCPatternCaseLabel) label).pat.type;
case CONSTANTCASELABEL -> types.boxedTypeOrType(((JCConstantCaseLabel) label).expr.type);
default -> throw Assert.error("Unexpected tree kind: " + label.getTag());
});
}
private boolean patternDominated(JCPattern existingPattern, JCPattern currentPattern) {
Type existingPatternType = types.erasure(existingPattern.type);
Type currentPatternType = types.erasure(currentPattern.type);
if (existingPatternType.isPrimitive() ^ currentPatternType.isPrimitive()) {
return false;
}
if (existingPatternType.isPrimitive()) {
if (!types.isSameType(existingPatternType, currentPatternType)) {
Copy link

Choose a reason for hiding this comment

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

note: shouldn't we return true if both have the same type? I mean this method seems to returns the right thing in that case but it is doing some extra analysis that doesn't seem to be necessary IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but I am not sure if that's not "too clever". Returning true when the type is same is based on the fact that only binding patterns (type test patterns) can be of primitive types, so the tests below will always return true. I can do that if strongly preferred, but I somewhat like the separation of concerns, where we verify the types here, and then the structure is checked below, regardless of the pattern's type.

return false;
}
} else {
if (!types.isSubtype(currentPatternType, existingPatternType)) {
return false;
}
}
while (existingPattern instanceof JCParenthesizedPattern parenthesized) {
existingPattern = parenthesized.pattern;
}
while (currentPattern instanceof JCParenthesizedPattern parenthesized) {
currentPattern = parenthesized.pattern;
}
if (currentPattern instanceof JCBindingPattern) {
return existingPattern instanceof JCBindingPattern;
} else if (currentPattern instanceof JCRecordPattern currentRecordPattern) {
if (existingPattern instanceof JCBindingPattern) {
return true;
} else if (existingPattern instanceof JCRecordPattern existingRecordPattern) {
List<JCPattern> existingNested = existingRecordPattern.nested;
List<JCPattern> currentNested = currentRecordPattern.nested;
if (existingNested.size() != currentNested.size()) {
return false;
}
while (existingNested.nonEmpty()) {
if (!patternDominated(existingNested.head, currentNested.head)) {
return false;
}
existingNested = existingNested.tail;
currentNested = currentNested.tail;
}
return true;
} else {
Assert.error("Unknown pattern: " + existingPattern.getTag());
}
} else {
Assert.error("Unknown pattern: " + currentPattern.getTag());
}
return false;
}

/** check if a type is a subtype of Externalizable, if that is available. */
boolean isExternalizable(Type t) {
try {
Expand Down
57 changes: 57 additions & 0 deletions test/langtools/tools/javac/patterns/Domination.java
View file
Open in desktop
Expand Up @@ -136,4 +136,61 @@ enum E {
}
}

int testRecordPatternsDominated1() {
record R(int a) {}
Object o = null;
switch (o) {
case R r: return 1;
case R(int a): return -1;
}
}

int testRecordPatternsDominated2() {
record R(int a) {}
Object o = null;
switch (o) {
case R(int a): return 1;
case R(int a): return -1;
}
}

int testRecordPatternsDominated3() {
record R(int a) {}
Object o = null;
switch (o) {
case R r when guard(): return 1;
case R(int a): return -1;
}
}

int testRecordPatternsDominated4() {
record R(int a) {}
Object o = null;
switch (o) {
case R(int a) when guard(): return 1;
case R(int a): return -1;
}
}

boolean guard() {
return false;
}

int testRecordPatternsDominated5() {
record R(int a) {}
Object o = null;
switch (o) {
case ((R r)): return 1;
case ((R(int a))): return -1;
}
}

int testRecordPatternsDominated6() {
record R(int a) {}
Object o = null;
switch (o) {
case ((R(int a))): return 1;
case ((R(int a))): return -1;
}
}
}
6 changes: 5 additions & 1 deletion test/langtools/tools/javac/patterns/Domination.out
View file
Open in desktop
Expand Up @@ -10,6 +10,10 @@ Domination.java:102:18: compiler.err.pattern.dominated
Domination.java:113:18: compiler.err.pattern.dominated
Domination.java:124:18: compiler.err.pattern.dominated
Domination.java:135:18: compiler.err.pattern.dominated
Domination.java:144:18: compiler.err.pattern.dominated
Domination.java:153:18: compiler.err.pattern.dominated
Domination.java:184:18: compiler.err.pattern.dominated
Domination.java:193:18: compiler.err.pattern.dominated
- compiler.note.preview.filename: Domination.java, DEFAULT
- compiler.note.preview.recompile
12 errors
16 errors
6 changes: 3 additions & 3 deletions test/langtools/tools/javac/patterns/SwitchErrors.out
View file
Open in desktop
Expand Up @@ -32,11 +32,11 @@ SwitchErrors.java:172:18: compiler.err.pattern.expected
SwitchErrors.java:178:78: compiler.err.cant.resolve.location: kindname.variable, n, , , (compiler.misc.location: kindname.class, SwitchErrors, null)
SwitchErrors.java:184:73: compiler.err.cant.resolve.location: kindname.variable, n, , , (compiler.misc.location: kindname.class, SwitchErrors, null)
SwitchErrors.java:191:21: compiler.err.flows.through.to.pattern
SwitchErrors.java:200:44: compiler.err.pattern.dominated
SwitchErrors.java:200:44: compiler.err.flows.through.from.pattern
SwitchErrors.java:218:29: compiler.err.unconditional.pattern.and.default
SwitchErrors.java:225:21: compiler.err.flows.through.to.pattern
SwitchErrors.java:225:47: compiler.err.flows.through.from.pattern
SwitchErrors.java:232:44: compiler.err.pattern.dominated
SwitchErrors.java:232:44: compiler.err.flows.through.from.pattern
SwitchErrors.java:232:47: compiler.err.flows.through.from.pattern
SwitchErrors.java:244:18: compiler.err.duplicate.unconditional.pattern
SwitchErrors.java:249:18: compiler.err.prob.found.req: (compiler.misc.not.applicable.types: int, java.lang.Integer)
Expand All @@ -55,4 +55,4 @@ SwitchErrors.java:164:9: compiler.err.not.exhaustive.statement
SwitchErrors.java:237:9: compiler.err.not.exhaustive.statement
- compiler.note.preview.filename: SwitchErrors.java, DEFAULT
- compiler.note.preview.recompile
55 errors
55 errors