Skip to content

Commit 885f7b1

Browse files
lahodajlahodaj
committed
8269146: Missing unreported constraints on pattern and other case label combination
8269301: Switch statement with a pattern, constant and default label elements crash javac Reviewed-by: mcimadamore
1 parent 62ff55d commit 885f7b1

8 files changed

Lines changed: 306 additions & 29 deletions

File tree

‎src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java‎

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1684,7 +1684,6 @@ private void handleSwitch(JCTree switchTree,
16841684
CaseTree.CaseKind caseKind = null;
16851685
boolean wasError = false;
16861686
MatchBindings prevBindings = null;
1687-
boolean prevCompletedNormally = false;
16881687
for (List<JCCase> l = cases; l.nonEmpty(); l = l.tail) {
16891688
JCCase c = l.head;
16901689
if (caseKind == null) {
@@ -1702,9 +1701,9 @@ private void handleSwitch(JCTree switchTree,
17021701
if (TreeInfo.isNull(expr)) {
17031702
preview.checkSourceLevel(expr.pos(), Feature.CASE_NULL);
17041703
if (hasNullPattern) {
1705-
log.error(c.pos(), Errors.DuplicateCaseLabel);
1704+
log.error(pat.pos(), Errors.DuplicateCaseLabel);
17061705
} else if (wasTotalPattern) {
1707-
log.error(c.pos(), Errors.PatternDominated);
1706+
log.error(pat.pos(), Errors.PatternDominated);
17081707
}
17091708
hasNullPattern = true;
17101709
attribExpr(expr, switchEnv, seltype);
@@ -1714,7 +1713,7 @@ private void handleSwitch(JCTree switchTree,
17141713
if (sym == null) {
17151714
log.error(expr.pos(), Errors.EnumLabelMustBeUnqualifiedEnum);
17161715
} else if (!labels.add(sym)) {
1717-
log.error(c.pos(), Errors.DuplicateCaseLabel);
1716+
log.error(pat.pos(), Errors.DuplicateCaseLabel);
17181717
} else {
17191718
checkCaseLabelDominated(pat.pos(), coveredTypes, sym.type);
17201719
}
@@ -1758,16 +1757,10 @@ private void handleSwitch(JCTree switchTree,
17581757
log.error(pat.pos(), Errors.DuplicateDefaultLabel);
17591758
} else if (hasTotalPattern) {
17601759
log.error(pat.pos(), Errors.TotalPatternAndDefault);
1761-
} else if (matchBindings.bindingsWhenTrue.nonEmpty()) {
1762-
//there was a pattern, and the execution flows into a default:
1763-
log.error(pat.pos(), Errors.FlowsThroughFromPattern);
17641760
}
17651761
hasDefault = true;
17661762
matchBindings = MatchBindingsComputer.EMPTY;
17671763
} else {
1768-
if (prevCompletedNormally) {
1769-
log.error(pat.pos(), Errors.FlowsThroughToPattern);
1770-
}
17711764
//binding pattern
17721765
attribExpr(pat, switchEnv);
17731766
var primary = TreeInfo.primaryPatternType((JCPattern) pat);
@@ -1794,7 +1787,6 @@ private void handleSwitch(JCTree switchTree,
17941787
}
17951788
}
17961789
currentBindings = matchBindingsComputer.switchCase(pat, currentBindings, matchBindings);
1797-
prevCompletedNormally = !TreeInfo.isNull(pat);
17981790
}
17991791
Env<AttrContext> caseEnv =
18001792
bindingEnv(switchEnv, c, currentBindings.bindingsWhenTrue);
@@ -1805,12 +1797,13 @@ private void handleSwitch(JCTree switchTree,
18051797
}
18061798
addVars(c.stats, switchEnv.info.scope);
18071799

1808-
boolean completesNormally = c.caseKind == CaseTree.CaseKind.STATEMENT ? flow.aliveAfter(caseEnv, c, make) : false;
1809-
prevBindings = completesNormally ? currentBindings : null;
1810-
prevCompletedNormally =
1811-
completesNormally &&
1812-
!(c.labels.size() == 1 &&
1813-
TreeInfo.isNull(c.labels.head) && c.stats.isEmpty());
1800+
c.completesNormally = flow.aliveAfter(caseEnv, c, make);
1801+
1802+
prevBindings = c.caseKind == CaseTree.CaseKind.STATEMENT && c.completesNormally ? currentBindings
1803+
: null;
1804+
}
1805+
if (patternSwitch) {
1806+
chk.checkSwitchCaseStructure(cases);
18141807
}
18151808
if (switchTree.hasTag(SWITCH)) {
18161809
((JCSwitch) switchTree).hasTotalPattern = hasDefault || hasTotalPattern;

‎src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import javax.lang.model.element.NestingKind;
3434
import javax.tools.JavaFileManager;
3535

36+
import com.sun.source.tree.CaseTree;
3637
import com.sun.tools.javac.code.*;
3738
import com.sun.tools.javac.code.Attribute.Compound;
3839
import com.sun.tools.javac.code.Directive.ExportsDirective;
@@ -4270,4 +4271,66 @@ void checkModuleRequires(final DiagnosticPosition pos, final RequiresDirective r
42704271
}
42714272
}
42724273

4274+
/**
4275+
* Verify the case labels conform to the constraints. Checks constraints related
4276+
* combinations of patterns and other labels.
4277+
*
4278+
* @param cases the cases that should be checked.
4279+
*/
4280+
void checkSwitchCaseStructure(List<JCCase> cases) {
4281+
boolean wasConstant = false; // Seen a constant in the same case label
4282+
boolean wasDefault = false; // Seen a default in the same case label
4283+
boolean wasNullPattern = false; // Seen a null pattern in the same case label,
4284+
//or fall through from a null pattern
4285+
boolean wasPattern = false; // Seen a pattern in the same case label
4286+
//or fall through from a pattern
4287+
boolean wasTypePattern = false; // Seen a pattern in the same case label
4288+
//or fall through from a type pattern
4289+
boolean wasNonEmptyFallThrough = false;
4290+
for (List<JCCase> l = cases; l.nonEmpty(); l = l.tail) {
4291+
JCCase c = l.head;
4292+
for (JCCaseLabel pat : c.labels) {
4293+
if (pat.isExpression()) {
4294+
JCExpression expr = (JCExpression) pat;
4295+
if (TreeInfo.isNull(expr)) {
4296+
if (wasPattern && !wasTypePattern && !wasNonEmptyFallThrough) {
4297+
log.error(pat.pos(), Errors.FlowsThroughFromPattern);
4298+
}
4299+
wasNullPattern = true;
4300+
} else {
4301+
if (wasPattern && !wasNonEmptyFallThrough) {
4302+
log.error(pat.pos(), Errors.FlowsThroughFromPattern);
4303+
}
4304+
wasConstant = true;
4305+
}
4306+
} else if (pat.hasTag(DEFAULTCASELABEL)) {
4307+
if (wasPattern && !wasNonEmptyFallThrough) {
4308+
log.error(pat.pos(), Errors.FlowsThroughFromPattern);
4309+
}
4310+
wasDefault = true;
4311+
} else {
4312+
boolean isTypePattern = pat.hasTag(BINDINGPATTERN);
4313+
if (wasPattern || wasConstant || wasDefault ||
4314+
(wasNullPattern && (!isTypePattern || wasNonEmptyFallThrough))) {
4315+
log.error(pat.pos(), Errors.FlowsThroughToPattern);
4316+
}
4317+
wasPattern = true;
4318+
wasTypePattern = isTypePattern;
4319+
}
4320+
}
4321+
4322+
boolean completesNormally = c.caseKind == CaseTree.CaseKind.STATEMENT ? c.completesNormally
4323+
: false;
4324+
4325+
if (c.stats.nonEmpty()) {
4326+
wasConstant = false;
4327+
wasDefault = false;
4328+
wasNullPattern &= completesNormally;
4329+
wasPattern &= completesNormally;
4330+
wasTypePattern &= completesNormally;
4331+
}
4332+
4333+
wasNonEmptyFallThrough = c.stats.nonEmpty() && completesNormally;
4334+
}
4335+
}
42734336
}

‎src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -677,7 +677,6 @@ public void visitSwitch(JCSwitch tree) {
677677
handleConstantCaseLabel(constants, pat);
678678
}
679679
scanStats(c.stats);
680-
c.completesNormally = alive != Liveness.DEAD;
681680
if (alive != Liveness.DEAD && c.caseKind == JCCase.RULE) {
682681
scanSyntheticBreak(make, tree);
683682
alive = Liveness.DEAD;
@@ -725,7 +724,6 @@ public void visitSwitchExpression(JCSwitchExpression tree) {
725724
Errors.SwitchExpressionCompletesNormally);
726725
}
727726
}
728-
c.completesNormally = alive != Liveness.DEAD;
729727
}
730728
if (!tree.hasTotalPattern && !TreeInfo.isErrorEnumSwitch(tree.selector, tree.cases) &&
731729
!isExhaustive(tree.selector.type, constants)) {

‎src/jdk.compiler/share/classes/com/sun/tools/javac/comp/MatchBindingsComputer.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,6 @@ public MatchBindings andOperation(DiagnosticPosition pos, MatchBindings lhsBindi
129129
public MatchBindings switchCase(JCTree tree, MatchBindings prevBindings, MatchBindings currentBindings) {
130130
if (prevBindings == null)
131131
return currentBindings;
132-
if (!prevBindings.bindingsWhenTrue.isEmpty() && !currentBindings.bindingsWhenTrue.isEmpty()) {
133-
log.error(tree.pos(), Errors.FlowsThroughToPattern);
134-
}
135132
if (prevBindings.nullPattern) {
136133
return currentBindings;
137134
}

‎test/langtools/tools/javac/patterns/CaseStructureTest.java‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
/*
2+
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation.
8+
*
9+
* This code is distributed in the hope that it will be useful, but WITHOUT
10+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
11+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
12+
* version 2 for more details (a copy is included in the LICENSE file that
13+
* accompanied this code).
14+
*
15+
* You should have received a copy of the GNU General Public License version
16+
* 2 along with this work; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
18+
*
19+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
20+
* or visit www.oracle.com if you need additional information or have any
21+
* questions.
22+
*/
23+
24+
/*
25+
* @test
26+
* @bug 8269146
27+
* @summary Check compilation outcomes for various combinations of case label element.
28+
* @library /tools/lib /tools/javac/lib
29+
* @modules
30+
* jdk.compiler/com.sun.tools.javac.api
31+
* jdk.compiler/com.sun.tools.javac.file
32+
* jdk.compiler/com.sun.tools.javac.main
33+
* jdk.compiler/com.sun.tools.javac.util
34+
* @build toolbox.ToolBox toolbox.JavacTask
35+
* @build combo.ComboTestHelper
36+
* @compile CaseStructureTest.java
37+
* @run main CaseStructureTest
38+
*/
39+
40+
import combo.ComboInstance;
41+
import combo.ComboParameter;
42+
import combo.ComboTask;
43+
import combo.ComboTestHelper;
44+
import java.util.Arrays;
45+
import java.util.stream.Collectors;
46+
import toolbox.ToolBox;
47+
48+
public class CaseStructureTest extends ComboInstance<CaseStructureTest> {
49+
private static final String JAVA_VERSION = System.getProperty("java.specification.version");
50+
51+
protected ToolBox tb;
52+
53+
CaseStructureTest() {
54+
super();
55+
tb = new ToolBox();
56+
}
57+
58+
public static void main(String... args) throws Exception {
59+
new ComboTestHelper<CaseStructureTest>()
60+
.withDimension("AS_CASE_LABEL_ELEMENTS", (x, asCaseLabelElements) -> x.asCaseLabelElements = asCaseLabelElements, true, false)
61+
.withArrayDimension("CASE_LABELS", (x, caseLabels, idx) -> x.caseLabels[idx] = caseLabels, DIMENSIONS, CaseLabel.values())
62+
.withFilter(t -> Arrays.stream(t.caseLabels).anyMatch(l -> l != CaseLabel.NONE))
63+
.withFailMode(ComboTestHelper.FailMode.FAIL_FAST)
64+
.run(CaseStructureTest::new);
65+
}
66+
67+
private static final int DIMENSIONS = 4;
68+
private boolean asCaseLabelElements;
69+
private CaseLabel[] caseLabels = new CaseLabel[DIMENSIONS];
70+
71+
private static final String MAIN_TEMPLATE =
72+
"""
73+
public class Test {
74+
public static void doTest(Integer in) {
75+
switch (in) {
76+
case -1: break;
77+
#{CASES}
78+
#{DEFAULT}
79+
}
80+
}
81+
}
82+
""";
83+
84+
@Override
85+
protected void doWork() throws Throwable {
86+
String labelSeparator = asCaseLabelElements ? ", " : ": case ";
87+
String labels = Arrays.stream(caseLabels).filter(l -> l != CaseLabel.NONE).map(l -> l.code).collect(Collectors.joining(labelSeparator, "case ", ": break;"));
88+
boolean hasDefault = Arrays.stream(caseLabels).anyMatch(l -> l == CaseLabel.DEFAULT || l == CaseLabel.TYPE_PATTERN || l == CaseLabel.PARENTHESIZED_PATTERN);
89+
90+
ComboTask task = newCompilationTask()
91+
.withSourceFromTemplate(MAIN_TEMPLATE.replace("#{CASES}", labels).replace("#{DEFAULT}", hasDefault ? "" : "default: break;"))
92+
.withOption("--enable-preview")
93+
.withOption("-source").withOption(JAVA_VERSION);
94+
95+
task.generate(result -> {
96+
boolean shouldPass = true;
97+
long patternCases = Arrays.stream(caseLabels).filter(l -> l == CaseLabel.TYPE_PATTERN || l == CaseLabel.GUARDED_PATTERN || l == CaseLabel.PARENTHESIZED_PATTERN).count();
98+
long typePatternCases = Arrays.stream(caseLabels).filter(l -> l == CaseLabel.TYPE_PATTERN).count();
99+
long constantCases = Arrays.stream(caseLabels).filter(l -> l == CaseLabel.CONSTANT).count();
100+
long nullCases = Arrays.stream(caseLabels).filter(l -> l == CaseLabel.NULL).count();
101+
long defaultCases = Arrays.stream(caseLabels).filter(l -> l == CaseLabel.DEFAULT).count();
102+
if (constantCases > 1) {
103+
shouldPass &= false;
104+
}
105+
if (constantCases > 0) {
106+
shouldPass &= patternCases == 0;
107+
}
108+
if (defaultCases > 1) {
109+
shouldPass &= false;
110+
}
111+
if (nullCases > 1) {
112+
shouldPass &= false;
113+
}
114+
if (nullCases > 0 && patternCases > 0) {
115+
shouldPass &= patternCases == typePatternCases;
116+
}
117+
if (patternCases > 1) {
118+
shouldPass &= false;
119+
}
120+
if (patternCases > 0 && defaultCases > 0) {
121+
shouldPass &= false;
122+
}
123+
if (!asCaseLabelElements) {
124+
//as an edge case, `case <total-pattern>: case null:` is prohibited:
125+
boolean seenPattern = false;
126+
for (CaseLabel label : caseLabels) {
127+
switch (label) {
128+
case NULL: if (seenPattern) shouldPass = false; break;
129+
case GUARDED_PATTERN, PARENTHESIZED_PATTERN, TYPE_PATTERN: seenPattern = true; break;
130+
}
131+
}
132+
}
133+
if (!(shouldPass ^ result.hasErrors())) {
134+
throw new AssertionError("Unexpected result: shouldPass=" + shouldPass + ", actual: " + !result.hasErrors() + ", info: " + result.compilationInfo());
135+
}
136+
});
137+
}
138+
139+
public enum CaseLabel implements ComboParameter {
140+
NONE(""),
141+
TYPE_PATTERN("Integer i"),
142+
PARENTHESIZED_PATTERN("(Integer i)"),
143+
GUARDED_PATTERN("Integer i && i > 0"),
144+
CONSTANT("1"),
145+
NULL("null"),
146+
DEFAULT("default");
147+
148+
private final String code;
149+
150+
private CaseLabel(String code) {
151+
this.code = code;
152+
}
153+
154+
@Override
155+
public String expand(String optParameter) {
156+
throw new UnsupportedOperationException("Not supported.");
157+
}
158+
}
159+
160+
}

‎test/langtools/tools/javac/patterns/SwitchErrors.java‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/*
22
* @test /nodynamiccopyright/
3-
* @bug 8262891
3+
* @bug 8262891 8269146
44
* @summary Verify errors related to pattern switches.
55
* @compile/fail/ref=SwitchErrors.out --enable-preview -source ${jdk.version} -XDrawDiagnostics -XDshould-stop.at=FLOW SwitchErrors.java
66
*/
@@ -185,6 +185,38 @@ Object guardWithMatchingExpression(Object o1, Object o2) {
185185
default -> null;
186186
};
187187
}
188+
void test8269146a(Integer i) {
189+
switch (i) {
190+
//error - illegal combination of pattern and constant:
191+
case Integer o && o != null, 1:
192+
break;
193+
default:
194+
break;
195+
}
196+
}
197+
void test8269146b(Integer i) {
198+
switch (i) {
199+
//error - illegal combination of null and pattern other than type pattern:
200+
case null, Integer o && o != null:
201+
break;
202+
default:
203+
break;
204+
}
205+
}
206+
void test8269146c(Integer i) {
207+
switch (i) {
208+
//error - illegal combination of pattern and default:
209+
case Integer o, default:
210+
break;
211+
}
212+
}
213+
void test8269301(Integer i) {
214+
switch (i) {
215+
//error - illegal combination of pattern, constant and default
216+
case Integer o && o != null, 1, default:
217+
break;
218+
}
219+
}
188220
void exhaustiveAndNull(String s) {
189221
switch (s) {
190222
case null: break;

0 commit comments

Comments
 (0)