Skip to content

Commit

Permalink
Add support for switch expressions (#520)
Browse files Browse the repository at this point in the history
Fixes #289 

Checker Framework 3.21.0 added support for switch expressions; we build on that to support the expressions in NullAway.  We also add a `jdk17-unit-tests` module to test the support (which requires a JDK 12+ compiler).
  • Loading branch information
msridhar committed Dec 21, 2021
1 parent f6ad8c8 commit 2835649
Show file tree
Hide file tree
Showing 6 changed files with 249 additions and 9 deletions.
2 changes: 1 addition & 1 deletion gradle/dependencies.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ if (project.hasProperty("epApiVersion")) {
}
def versions = [
asm : "7.1",
checkerFramework : "3.20.0",
checkerFramework : "3.21.0",
// The version of Error Prone used to check NullAway's code
errorProne : "2.10.0",
// The version of Error Prone that NullAway is compiled and tested against
Expand Down
60 changes: 60 additions & 0 deletions jdk17-unit-tests/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright (C) 2021. Uber Technologies
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
plugins {
id 'java-library'
// For code coverage:
id 'jacoco'
}

// Use JDK 17 for this module, via a toolchain
java.toolchain.languageVersion.set JavaLanguageVersion.of(17)

dependencies {
testImplementation project(":nullaway")
testImplementation deps.test.junit4
testImplementation(deps.build.errorProneTestHelpers) {
exclude group: "junit", module: "junit"
}
testImplementation deps.build.jsr305Annotations
}

test {
maxHeapSize = "1024m"
// to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer
jvmArgs += [
"--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED",
"--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
"--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED",
]
}

jacoco {
toolVersion = "0.8.7"
}

jacocoTestReport {
reports {
xml.enabled = true // coveralls plugin depends on xml format report
html.enabled = true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
/*
* Copyright (c) 2021 Uber Technologies, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
package com.uber.nullaway.jdk17;

import com.google.errorprone.CompilationTestHelper;
import com.uber.nullaway.NullAway;
import java.util.Arrays;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;

/** NullAway unit tests involving language features available on JDK 17 but not JDK 11. */
public class NullAwayJDK17Test {

@Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder();

private CompilationTestHelper defaultCompilationHelper;

@Before
public void setup() {
defaultCompilationHelper =
CompilationTestHelper.newInstance(NullAway.class, getClass())
.setArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber"));
}

@Test
public void testSwitchExpressionAssign() {
defaultCompilationHelper
.addSourceLines(
"SwitchExpr.java",
"package com.uber;",
"class SwitchExpr {",
" public void testSwitchExpressionAssign(int i) {",
" Object o = switch (i) { case 3, 4, 5 -> new Object(); default -> null; };",
" // BUG: Diagnostic contains: dereferenced expression o is @Nullable",
" o.toString();",
" Object o2 = switch (i) { case 3, 4, 5 -> new Object(); default -> \"hello\"; };",
" // This dereference is safe",
" o2.toString();",
" }",
"}")
.doTest();
}

@Test
public void testDirectlyDerefedSwitchExpr() {
defaultCompilationHelper
.addSourceLines(
"SwitchExpr.java",
"package com.uber;",
"class SwitchExpr {",
" public void testDirectlyDerefedSwitchExpr(int i) {",
" // BUG: Diagnostic contains: dereferenced expression (switch",
" (switch (i) { case 3, 4, 5 -> new Object(); default -> null; }).toString();",
" // This deference is safe",
" (switch (i) { case 3, 4, 5 -> new Object(); default -> \"hello\"; }).toString();",
" }",
"}")
.doTest();
}

@Test
public void testPassingSwitchExprAsParam() {
defaultCompilationHelper
.addSourceLines(
"SwitchExpr.java",
"package com.uber;",
"class SwitchExpr {",
" private void takesNonNull(Object o) {}",
" public void testSwitchExpr3(int i) {",
" // BUG: Diagnostic contains: passing",
" takesNonNull(switch (i) { case 3, 4, 5 -> new Object(); default -> null; });",
" // This call is safe",
" takesNonNull(switch (i) { case 3, 4, 5 -> new Object(); default -> new Object(); });",
" }",
"}")
.doTest();
}

@Test
public void testSwitchStmtArrowCase() {
defaultCompilationHelper
.addSourceLines(
"SwitchExpr.java",
"package com.uber;",
"class SwitchExpr {",
" public void testSwitchStmtArrowCase(int i) {",
" Object o = null;",
" switch (i) {",
" case 3, 4, 5 -> { o = new Object(); }",
" default -> { o = null; }",
" }",
" // BUG: Diagnostic contains: dereferenced expression o is @Nullable",
" o.toString();",
" Object o2 = null;",
" switch (i) {",
" case 3, 4, 5 -> { o2 = null; }",
" default -> { o2 = new Object(); }",
" }",
" // BUG: Diagnostic contains: dereferenced expression o2 is @Nullable",
" o2.toString();",
" Object o3 = null;",
" switch (i) {",
" case 3, 4, 5 -> { o3 = new Object(); }",
" default -> { o3 = new Object(); }",
" }",
" // This dereference is safe",
" o3.toString();",
" }",
"}")
.doTest();
}

@Test
public void testSwitchExprUnbox() {
defaultCompilationHelper
.addSourceLines(
"SwitchExpr.java",
"package com.uber;",
"class SwitchExpr {",
" public void testSwitchExprUnbox() {",
" Integer i = null;",
" // NOTE: we should report a bug here due to the unboxing of i, but cannot do so until",
" // Error Prone supports matching switch expressions",
" Object o = switch (i) { case 3, 4, 5 -> new Object(); default -> null; };",
" int j1 = 0;",
" // BUG: Diagnostic contains: unboxing of a @Nullable value",
" int j2 = 3 + (switch (j1) { case 3, 4, 5 -> Integer.valueOf(4); default -> null; });",
" // This is safe",
" int j3 = 3 + (switch (j1) { case 3, 4, 5 -> Integer.valueOf(4); default -> Integer.valueOf(6); });",
" }",
"}")
.doTest();
}
}
26 changes: 18 additions & 8 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -477,19 +477,24 @@ public Description matchSwitch(SwitchTree tree, VisitorState state) {
return Description.NO_MATCH;
}

ExpressionTree switchExpression = tree.getExpression();
if (switchExpression instanceof ParenthesizedTree) {
switchExpression = ((ParenthesizedTree) switchExpression).getExpression();
ExpressionTree switchSelectorExpression = tree.getExpression();
// For a statement `switch (e) { ... }`, javac returns `(e)` as the selector expression. We
// strip the outermost parentheses for a nicer-looking error message.
if (switchSelectorExpression instanceof ParenthesizedTree) {
switchSelectorExpression = ((ParenthesizedTree) switchSelectorExpression).getExpression();
}

if (mayBeNullExpr(state, switchExpression)) {
if (mayBeNullExpr(state, switchSelectorExpression)) {
final String message =
"switch expression " + state.getSourceForNode(switchExpression) + " is @Nullable";
"switch expression " + state.getSourceForNode(switchSelectorExpression) + " is @Nullable";
ErrorMessage errorMessage =
new ErrorMessage(MessageTypes.SWITCH_EXPRESSION_NULLABLE, message);

return errorBuilder.createErrorDescription(
errorMessage, switchExpression, buildDescription(switchExpression), state);
errorMessage,
switchSelectorExpression,
buildDescription(switchSelectorExpression),
state);
}

return Description.NO_MATCH;
Expand Down Expand Up @@ -1944,8 +1949,13 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) {
exprMayBeNull = nullnessFromDataflow(state, expr);
break;
default:
throw new RuntimeException(
"whoops, better handle " + expr.getKind() + " " + state.getSourceForNode(expr));
// match switch expressions by comparing strings, so the code compiles on JDK versions < 12
if (expr.getKind().name().equals("SWITCH_EXPRESSION")) {
exprMayBeNull = nullnessFromDataflow(state, expr);
} else {
throw new RuntimeException(
"whoops, better handle " + expr.getKind() + " " + state.getSourceForNode(expr));
}
}
exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, state, exprMayBeNull);
return exprMayBeNull;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@
import org.checkerframework.nullaway.dataflow.cfg.node.StringConversionNode;
import org.checkerframework.nullaway.dataflow.cfg.node.StringLiteralNode;
import org.checkerframework.nullaway.dataflow.cfg.node.SuperNode;
import org.checkerframework.nullaway.dataflow.cfg.node.SwitchExpressionNode;
import org.checkerframework.nullaway.dataflow.cfg.node.SynchronizedNode;
import org.checkerframework.nullaway.dataflow.cfg.node.TernaryExpressionNode;
import org.checkerframework.nullaway.dataflow.cfg.node.ThisNode;
Expand Down Expand Up @@ -488,6 +489,15 @@ public TransferResult<Nullness, NullnessStore> visitTernaryExpression(
return new RegularTransferResult<>(result, input.getRegularStore());
}

@Override
public TransferResult<Nullness, NullnessStore> visitSwitchExpressionNode(
SwitchExpressionNode node, TransferInput<Nullness, NullnessStore> input) {
// The cfg includes assignments of the value of each case body of the switch expression
// to the switch expression var (a synthetic local variable). So, the dataflow result
// for the switch expression is just the result for the switch expression var
return visitLocalVariable(node.getSwitchExpressionVar(), input);
}

@Override
public TransferResult<Nullness, NullnessStore> visitAssignment(
AssignmentNode node, TransferInput<Nullness, NullnessStore> input) {
Expand Down
1 change: 1 addition & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,4 @@ include ':jar-infer:test-java-lib-jarinfer'
include ':jar-infer:nullaway-integration-test'
include ':jar-infer:test-android-lib-jarinfer'
include ':jmh'
include ':jdk17-unit-tests'

0 comments on commit 2835649

Please sign in to comment.