diff --git a/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java b/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java index ade0791b7..3455252dd 100644 --- a/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java @@ -68,6 +68,8 @@ public abstract class AbstractConfig implements Config { protected boolean isAcknowledgeRestrictive; + protected boolean checkOptionalEmptiness; + protected boolean assertsEnabled; /** @@ -182,6 +184,11 @@ public boolean acknowledgeRestrictiveAnnotations() { return isAcknowledgeRestrictive; } + @Override + public boolean checkOptionalEmptiness() { + return checkOptionalEmptiness; + } + @Override public boolean assertsEnabled() { return assertsEnabled; diff --git a/nullaway/src/main/java/com/uber/nullaway/Config.java b/nullaway/src/main/java/com/uber/nullaway/Config.java index 575bb9be4..afcbe9113 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Config.java +++ b/nullaway/src/main/java/com/uber/nullaway/Config.java @@ -107,6 +107,9 @@ public interface Config { */ boolean acknowledgeRestrictiveAnnotations(); + /** @return true if Optional Emptiness Handler is to be used. */ + boolean checkOptionalEmptiness(); + /** * @return the fully qualified name of a method which will take a @Nullable version of a value and * return an @NonNull copy (likely through an unsafe downcast, but performing runtime checking diff --git a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java index 5d6baab5d..ceeb09117 100644 --- a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java @@ -112,6 +112,11 @@ public boolean acknowledgeRestrictiveAnnotations() { throw new IllegalStateException(error_msg); } + @Override + public boolean checkOptionalEmptiness() { + throw new IllegalStateException(error_msg); + } + @Override @Nullable public String getCastToNonNullMethod() { diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java index 23549901d..e782f9b12 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java @@ -55,6 +55,7 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig { static final String FL_UNANNOTATED_CLASSES = EP_FL_NAMESPACE + ":UnannotatedClasses"; static final String FL_ACKNOWLEDGE_RESTRICTIVE = EP_FL_NAMESPACE + ":AcknowledgeRestrictiveAnnotations"; + static final String FL_CHECK_OPTIONAL_EMPTINESS = EP_FL_NAMESPACE + ":CheckOptionalEmptiness"; static final String FL_SUPPRESS_COMMENT = EP_FL_NAMESPACE + ":AutoFixSuppressionComment"; /** --- JarInfer configs --- */ static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled"; @@ -132,6 +133,7 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig { isExhaustiveOverride = flags.getBoolean(FL_EXHAUSTIVE_OVERRIDE).orElse(false); isSuggestSuppressions = flags.getBoolean(FL_SUGGEST_SUPPRESSIONS).orElse(false); isAcknowledgeRestrictive = flags.getBoolean(FL_ACKNOWLEDGE_RESTRICTIVE).orElse(false); + checkOptionalEmptiness = flags.getBoolean(FL_CHECK_OPTIONAL_EMPTINESS).orElse(false); treatGeneratedAsUnannotated = flags.getBoolean(FL_GENERATED_UNANNOTATED).orElse(false); assertsEnabled = flags.getBoolean(FL_ASSERTS_ENABLED).orElse(false); fieldAnnotPattern = diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 3fc6e60de..2c51c8146 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1933,7 +1933,17 @@ private Description matchDereference( return Description.NO_MATCH; } if (mayBeNullExpr(state, baseExpression)) { - String message = "dereferenced expression " + baseExpression.toString() + " is @Nullable"; + String message; + if (handler.checkIfOptionalGetCall(baseExpression, state)) { + final int exprStringSize = baseExpression.toString().length(); + // Name of the optional is extracted from the expression + message = + "Optional " + + baseExpression.toString().substring(0, exprStringSize - 6) + + " can be empty, dereferenced get() call on it"; + } else { + message = "dereferenced expression " + baseExpression.toString() + " is @Nullable"; + } return createErrorDescriptionForNullAssignment( MessageTypes.DEREFERENCE_NULLABLE, derefExpression, diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java index 0c8756024..b8ccfc0e3 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java @@ -166,4 +166,9 @@ public void onDataflowVisitLambdaResultExpression( ExpressionTree tree, NullnessStore thenStore, NullnessStore elseStore) { // NoOp } + + @Override + public boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state) { + return false; + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java index cc5f41e31..8b2d1d124 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java @@ -207,4 +207,13 @@ public void onDataflowVisitLambdaResultExpression( h.onDataflowVisitLambdaResultExpression(tree, thenStore, elseStore); } } + + @Override + public boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state) { + boolean ifOptionalGetCall = false; + for (Handler h : handlers) { + ifOptionalGetCall |= h.checkIfOptionalGetCall(expr, state); + } + return ifOptionalGetCall; + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java index 823e67936..4c0d8f1be 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java @@ -270,6 +270,15 @@ NullnessHint onDataflowVisitMethodInvocation( void onDataflowVisitLambdaResultExpression( ExpressionTree tree, NullnessStore thenStore, NullnessStore elseStore); + /** + * Called while creating the error message on a possible null/empty optional deference. + * + * @param expr The AST node for the expression being matched. + * @param state The current visitor state. + * @return If the method call in the expression is to Optional.get(). + */ + boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state); + /** * A three value enum for handlers implementing onDataflowVisitMethodInvocation to communicate * their knowledge of the method return nullability to the the rest of NullAway. diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java index 3d47a410f..797c0a45a 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java @@ -50,7 +50,9 @@ public static Handler buildDefault(Config config) { handlerListBuilder.add(new RxNullabilityPropagator()); handlerListBuilder.add(new ContractHandler()); handlerListBuilder.add(new ApacheThriftIsSetHandler()); - handlerListBuilder.add(new OptionalEmptinessHandler()); + if (config.checkOptionalEmptiness()) { + handlerListBuilder.add(new OptionalEmptinessHandler()); + } return new CompositeHandler(handlerListBuilder.build()); } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java index 026694026..3f54d12d7 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -289,6 +289,7 @@ private static class DefaultLibraryModels implements LibraryModels { "javax.lang.model.util.Elements", "getDocComment(javax.lang.model.element.Element)"), 0) + .put(methodRef("java.util.Optional", "of(T)"), 0) .put(methodRef("java.util.Deque", "addFirst(E)"), 0) .put(methodRef("java.util.Deque", "addLast(E)"), 0) .put(methodRef("java.util.Deque", "offerFirst(E)"), 0) diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java index 078ac718d..7a112c0d7 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java @@ -99,6 +99,15 @@ public NullnessHint onDataflowVisitMethodInvocation( return NullnessHint.UNKNOWN; } + @Override + public boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state) { + if (expr.getKind() == Tree.Kind.METHOD_INVOCATION + && optionalIsGetCall((Symbol.MethodSymbol) ASTHelpers.getSymbol(expr), state.getTypes())) { + return true; + } + return false; + } + private void updateNonNullAPsForElement( AccessPathNullnessPropagation.Updates updates, @Nullable Element elem, Node base) { if (elem != null) { diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java index dcd7671b2..e6b3619dd 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java @@ -1183,7 +1183,8 @@ public void OptionalEmptinessHandlerTest() { "-d", temporaryFolder.getRoot().getAbsolutePath(), "-XepOpt:NullAway:AnnotatedPackages=com.uber", - "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated")) + "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated", + "-XepOpt:NullAway:CheckOptionalEmptiness=true")) .addSourceLines( "TestNegative.java", "package com.uber;", @@ -1191,7 +1192,7 @@ public void OptionalEmptinessHandlerTest() { "import javax.annotation.Nullable;", "public class TestNegative {", " void foo() {", - " Optional a = Optional.of(null);", + " Optional a = Optional.empty();", " // no error since a.isPresent() is called", " if(a.isPresent()){", " a.get().toString();", @@ -1205,8 +1206,32 @@ public void OptionalEmptinessHandlerTest() { "import javax.annotation.Nullable;", "public class TestPositive {", " void foo() {", - " Optional a = Optional.of(null);", - " // BUG: Diagnostic contains: dereferenced expression a.get() is @Nullable", + " Optional a = Optional.empty();", + " // BUG: Diagnostic contains: Optional a can be empty", + " a.get().toString();", + " }", + "}") + .doTest(); + } + + @Test + public void OptionalEmptinessUncheckedTest() { + compilationHelper + .setArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated")) + .addSourceLines( + "TestPositive.java", + "package com.uber;", + "import java.util.Optional;", + "import javax.annotation.Nullable;", + "public class TestPositive {", + " void foo() {", + " Optional a = Optional.empty();", + " // no error since the handler is not attached", " a.get().toString();", " }", "}") diff --git a/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayNativeModels.java b/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayNativeModels.java index 01c6adeda..9e11212d4 100644 --- a/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayNativeModels.java +++ b/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayNativeModels.java @@ -37,6 +37,7 @@ import java.util.HashMap; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; import javax.annotation.Nullable; import javax.lang.model.element.Element; @@ -229,6 +230,8 @@ static void nonNullParameters() { File f = new File(s); // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required URLClassLoader.newInstance(null, NullAwayNativeModels.class.getClassLoader()); + // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required + Optional op = Optional.of(null); } static void elementStuff(Element e, Elements elems) {