Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't crash on switch expressions or switch statements with arrow cases. #4976

Merged
merged 15 commits into from
Dec 6, 2021
1 change: 1 addition & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,7 @@ List<String> getJavaFilesToFormat(projectName) {
&& !details.path.contains("returnsreceiverdelomboked")
&& !details.path.contains("build")
&& (isJava17 || !details.path.contains("-records"))
&& (isJava17 || !details.path.contains("java17"))
&& details.name.endsWith('.java')) {
javaFiles.add(details.file)
}
Expand Down
2 changes: 1 addition & 1 deletion checker/bin-devel/git.pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ set -e
# Need to keep checked files in sync with getJavaFilesToFormat in build.gradle.
# Otherwise `./gradlew reformat` might not reformat a file that this
# hook complains about.
CHANGED_JAVA_FILES=$(git diff --staged --name-only --diff-filter=ACM | grep '\.java$' | grep -v '/jdk/' | grep -v 'stubparser/' | grep -v '/nullness-javac-errors/' | grep -v 'dataflow/manual/examples/' ) || true
CHANGED_JAVA_FILES=$(git diff --staged --name-only --diff-filter=ACM | grep '\.java$' | grep -v '/jdk/' | grep -v 'stubparser/' | grep -v '/nullness-javac-errors/' | grep -v 'dataflow/manual/examples/' | grep -v '/java17/') || true
# echo "CHANGED_JAVA_FILES=${CHANGED_JAVA_FILES}"
if [ -n "$CHANGED_JAVA_FILES" ]; then
./gradlew getCodeFormatScripts -q
Expand Down
80 changes: 80 additions & 0 deletions checker/tests/nullness/java17/NullnessSwitchArrows.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// @below-java17-jdk-skip-test
public class NullnessSwitchArrows {
public enum Day {
SUNDAY,
MONDAY,
TUESDAY,
WEDNESDAY,
THURSDAY,
FRIDAY,
SATURDAY;
}

void method1() {
Object o;
Day day = Day.WEDNESDAY;
switch (day) {
case MONDAY, FRIDAY, SUNDAY -> o = "hello";
case TUESDAY -> o = null;
case THURSDAY, SATURDAY -> o = "hello";
case WEDNESDAY -> o = "hello";
default -> throw new IllegalStateException("Invalid day: " + day);
}

// :: error: (dereference.of.nullable)
o.toString();
}

void method2() {
Object o;
Day day = Day.WEDNESDAY;
switch (day) {
case MONDAY, FRIDAY, SUNDAY -> o = "hello";
case TUESDAY -> o = "hello";
case THURSDAY, SATURDAY -> o = "hello";
case WEDNESDAY -> o = "hello";
default -> throw new IllegalStateException("Invalid day: " + day);
}

// TODO: this is a false positive. It works for case: statements; see below.
// :: error: (dereference.of.nullable)
o.toString();
}

void method2b() {
Object o;
Day day = Day.WEDNESDAY;
switch (day) {
case MONDAY, FRIDAY, SUNDAY:
o = "hello";
break;
case TUESDAY:
o = "hello";
break;
case THURSDAY, SATURDAY:
o = "hello";
break;
case WEDNESDAY:
o = "hello";
break;
default:
throw new IllegalStateException("Invalid day: " + day);
}

o.toString();
}

void method3() {
Object o;
Day day = Day.WEDNESDAY;
switch (day) {
case MONDAY, FRIDAY, SUNDAY -> o = "hello";
case TUESDAY -> o = "hello";
case THURSDAY, SATURDAY -> o = "hello";
case WEDNESDAY -> o = "hello";
default -> o = "hello";
}

o.toString();
}
}
67 changes: 67 additions & 0 deletions checker/tests/nullness/java17/NullnessSwitchExpressions.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// @below-java17-jdk-skip-test
public class NullnessSwitchExpressions {
public enum Day {
SUNDAY,
MONDAY,
TUESDAY,
WEDNESDAY,
THURSDAY,
FRIDAY,
SATURDAY;
}

void method1() {
Day day = Day.WEDNESDAY;
Object o =
switch (day) {
case MONDAY, FRIDAY, SUNDAY -> "hello";
case TUESDAY -> null;
case THURSDAY, SATURDAY -> "hello";
case WEDNESDAY -> "hello";
default -> throw new IllegalStateException("Invalid day: " + day);
};

// :: error: (dereference.of.nullable)
o.toString();
}

void method2() {
Day day = Day.WEDNESDAY;
Object o =
switch (day) {
case MONDAY, FRIDAY, SUNDAY -> "hello";
case TUESDAY -> "hello";
case THURSDAY, SATURDAY -> "hello";
case WEDNESDAY -> "hello";
default -> throw new IllegalStateException("Invalid day: " + day);
};

// TODO: This is a false positive.
// :: error: (dereference.of.nullable)
o.toString();
}

void method3() {
Day day = Day.WEDNESDAY;
Object o =
switch (day) {
case MONDAY, FRIDAY, SUNDAY -> "hello";
case TUESDAY -> "hello";
case THURSDAY, SATURDAY -> {
String s = null;
if (day == Day.THURSDAY) {
s = "hello";
// TODO: This is a false positive.
// :: error: (dereference.of.nullable)
s.toString();
}
yield s;
}
case WEDNESDAY -> "hello";
default -> throw new IllegalStateException("Invalid day: " + day);
};

// :: error: (dereference.of.nullable)
o.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,46 @@ public PhaseOneResult process(CompilationUnitTree root, UnderlyingAST underlying
*/
public void handleArtificialTree(Tree tree) {}

@Override
public Node scan(Tree tree, Void p) {
if (tree == null) {
return null;
}
// Must use String comparison to support compiling on JDK 11 and earlier.
if (tree.getKind().name().equals("SWITCH_EXPRESSION")) {
return visitSwitchExpression17(tree, p);
}
return super.scan(tree, p);

// TODO: Do we need to support yield trees and binding patterns to?
// Features added between JDK 12 and JDK 17 inclusive.
// switch (tree.getKind().name()) {
// case "BINDING_PATTERN":
// return visitBindingPattern17(path.getLeaf(), p);
// case "SWITCH_EXPRESSION":
// return visitSwitchExpression17(tree, p);
// case "YIELD":
// return visitYield17(path.getLeaf(), p);
// default:
// return super.scan(tree, p);
// }
}

/**
* Visit a SwitchExpressionTree
*
* @param switchExpressionTree a SwitchExpressionTree, typed as Tree to be backward-compatible
* @param p parameter
* @return the result of visiting the switch expression tree
*/
public Node visitSwitchExpression17(Tree switchExpressionTree, Void p) {
// TODO: Analyze switch expressions properly.
return new MarkerNode(
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided not to add a SwitchExpressionNode, because I wasn't sure what fields it would need when we properly implement switch expressions.

switchExpressionTree,
"switch expression tree; not analyzed #" + TreeUtils.treeUids.get(switchExpressionTree),
env.getTypeUtils());
}

/* --------------------------------------------------------- */
/* Nodes and Labels Management */
/* --------------------------------------------------------- */
Expand Down Expand Up @@ -2190,8 +2230,12 @@ private void buildCase(CaseTree tree, int index) {
extendWithExtendedNode(new ConditionalJump(thisBodyL, nextCaseL));
}
addLabelForNextNode(thisBodyL);
for (StatementTree stmt : tree.getStatements()) {
scan(stmt, null);
if (tree.getStatements() != null) {
for (StatementTree stmt : tree.getStatements()) {
scan(stmt, null);
}
} else {
scan(TreeUtils.caseTreeGetBody(tree), null);
}
extendWithExtendedNode(new UnconditionalJump(nextBodyL));
addLabelForNextNode(nextCaseL);
Expand Down
13 changes: 11 additions & 2 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
Version 3.20.0 (December 1, 2021)
---------------------------------
Version 3.20.1 (December 6, 2021)
-------------------------------

**User-visible changes:**

The Checker Framework now runs on code that contains switch expressions and
switch statements that use the new `->` case syntax, but treats them
conservatively. A future version will improve precision.

**Implementation details:**

The dataflow framework can be run on code that contains switch expressions and
switch statements that use the new `->` case syntax, but it does not yet
analyze the cases in a switch expression and it treats `->` as `:`.. A future
version will do so.

Removed methods and classes that have been deprecated for more than one year:
* Old way of constructing qualifier hierarchies
* `@SuppressWarningsKeys`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
import javax.lang.model.type.TypeMirror;
import javax.lang.model.util.ElementFilter;
import javax.tools.Diagnostic.Kind;
import javax.tools.JavaFileObject;
import org.checkerframework.checker.compilermsgs.qual.CompilerMessageKey;
import org.checkerframework.checker.interning.qual.FindDistinct;
import org.checkerframework.checker.nullness.qual.NonNull;
Expand Down Expand Up @@ -359,6 +360,11 @@ protected void testJointJavacJavaParserVisitor() {
}

Map<Tree, com.github.javaparser.ast.Node> treePairs = new HashMap<>();
JavaFileObject f = root.getSourceFile();
if (f.toUri().getPath().contains("java17")) {
// Skip java17 files because they may contain switch expressions which aren't supported.
return;
}
try (InputStream reader = root.getSourceFile().openInputStream()) {
CompilationUnit javaParserRoot = JavaParserUtil.parseCompilationUnit(reader);
JavaParserUtil.concatenateAddedStringLiterals(javaParserRoot);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,29 @@ public AnnotatedTypeMirror visitConditionalExpression(
return AnnotatedTypes.leastUpperBound(f, trueType, falseType, alub);
}

@Override
public AnnotatedTypeMirror defaultAction(Tree tree, AnnotatedTypeFactory f) {
if (tree.getKind().name().equals("SWITCH_EXPRESSION")) {
return visitSwitchExpressionTree17(tree, f);
}
return super.defaultAction(tree, f);
}

/**
* Compute the type of the switch expression tree.
*
* @param switchExpressionTree SwitchExpressionTree; typed as Tree to be backward-compatible
* @param f AnnotatedTypeFactory
* @return the type of the switch expression
*/
public AnnotatedTypeMirror visitSwitchExpressionTree17(
Tree switchExpressionTree, AnnotatedTypeFactory f) {
// TODO: Properly compute the type from the cases.
AnnotatedTypeMirror result = f.type(switchExpressionTree);
result.addAnnotations(f.getQualifierHierarchy().getTopAnnotations());
return result;
}

@Override
public AnnotatedTypeMirror visitIdentifier(IdentifierTree node, AnnotatedTypeFactory f) {
if (node.getName().contentEquals("this") || node.getName().contentEquals("super")) {
Expand Down
Loading