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 a1a9f0b16..3d47a410f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java @@ -50,6 +50,7 @@ public static Handler buildDefault(Config config) { handlerListBuilder.add(new RxNullabilityPropagator()); handlerListBuilder.add(new ContractHandler()); handlerListBuilder.add(new ApacheThriftIsSetHandler()); + handlerListBuilder.add(new OptionalEmptinessHandler()); return new CompositeHandler(handlerListBuilder.build()); } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java new file mode 100644 index 000000000..078ac718d --- /dev/null +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java @@ -0,0 +1,129 @@ +/* + * Copyright (c) 2018 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.handlers; + +import com.google.common.base.Preconditions; +import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.Types; +import com.sun.tools.javac.util.Context; +import com.uber.nullaway.NullAway; +import com.uber.nullaway.Nullness; +import com.uber.nullaway.dataflow.AccessPath; +import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; +import java.util.Optional; +import javax.annotation.Nullable; +import javax.lang.model.element.Element; +import javax.lang.model.element.ElementKind; +import org.checkerframework.dataflow.cfg.node.MethodInvocationNode; +import org.checkerframework.dataflow.cfg.node.Node; + +/** + * Handler to better handle {@code isPresent()} methods in code generated for Optionals. With this + * handler, we learn appropriate Emptiness facts about the relevant property from these calls. + */ +public class OptionalEmptinessHandler extends BaseNoOpHandler { + + private static String OPTIONAL_PATH = "java.util.Optional"; + + @Nullable private Optional optionalType; + + @Override + public boolean onOverrideMayBeNullExpr( + NullAway analysis, ExpressionTree expr, VisitorState state, boolean exprMayBeNull) { + if (expr.getKind() == Tree.Kind.METHOD_INVOCATION + && optionalIsGetCall((Symbol.MethodSymbol) ASTHelpers.getSymbol(expr), state.getTypes())) { + return true; + } + return exprMayBeNull; + } + + @Override + public void onMatchTopLevelClass( + NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol) { + if (optionalType == null) { + optionalType = + Optional.ofNullable(state.getTypeFromString(OPTIONAL_PATH)) + .map(state.getTypes()::erasure); + } + } + + @Override + public NullnessHint onDataflowVisitMethodInvocation( + MethodInvocationNode node, + Types types, + Context context, + AccessPathNullnessPropagation.SubNodeValues inputs, + AccessPathNullnessPropagation.Updates thenUpdates, + AccessPathNullnessPropagation.Updates elseUpdates, + AccessPathNullnessPropagation.Updates bothUpdates) { + Symbol.MethodSymbol symbol = ASTHelpers.getSymbol(node.getTree()); + + if (optionalIsGetCall(symbol, types)) { + return NullnessHint.HINT_NULLABLE; + } else if (optionalIsPresentCall(symbol, types)) { + Element getter = null; + Node base = node.getTarget().getReceiver(); + for (Symbol elem : symbol.owner.getEnclosedElements()) { + if (elem.getKind().equals(ElementKind.METHOD) + && elem.getSimpleName().toString().equals("get")) { + getter = elem; + } + } + updateNonNullAPsForElement(thenUpdates, getter, base); + } + return NullnessHint.UNKNOWN; + } + + private void updateNonNullAPsForElement( + AccessPathNullnessPropagation.Updates updates, @Nullable Element elem, Node base) { + if (elem != null) { + AccessPath ap = AccessPath.fromBaseAndElement(base, elem); + if (ap != null) { + updates.set(ap, Nullness.NONNULL); + } + } + } + + private boolean optionalIsPresentCall(Symbol.MethodSymbol symbol, Types types) { + Preconditions.checkNotNull(optionalType); + // noinspection ConstantConditions + return optionalType.isPresent() + && symbol.getSimpleName().toString().equals("isPresent") + && symbol.getParameters().length() == 0 + && types.isSubtype(symbol.owner.type, optionalType.get()); + } + + private boolean optionalIsGetCall(Symbol.MethodSymbol symbol, Types types) { + Preconditions.checkNotNull(optionalType); + // noinspection ConstantConditions + return optionalType.isPresent() + && symbol.getSimpleName().toString().equals("get") + && symbol.getParameters().length() == 0 + && types.isSubtype(symbol.owner.type, optionalType.get()); + } +} diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java index 646014b7e..dcd7671b2 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java @@ -1175,6 +1175,44 @@ public void lambdaPlusRestrictiveNegative() { .doTest(); } + @Test + public void OptionalEmptinessHandlerTest() { + compilationHelper + .setArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated")) + .addSourceLines( + "TestNegative.java", + "package com.uber;", + "import java.util.Optional;", + "import javax.annotation.Nullable;", + "public class TestNegative {", + " void foo() {", + " Optional a = Optional.of(null);", + " // no error since a.isPresent() is called", + " if(a.isPresent()){", + " a.get().toString();", + " }", + " }", + "}") + .addSourceLines( + "TestPositive.java", + "package com.uber;", + "import java.util.Optional;", + "import javax.annotation.Nullable;", + "public class TestPositive {", + " void foo() {", + " Optional a = Optional.of(null);", + " // BUG: Diagnostic contains: dereferenced expression a.get() is @Nullable", + " a.get().toString();", + " }", + "}") + .doTest(); + } + @Test public void testCastToNonNull() { compilationHelper