From fd0fadd8901330654ad8926331e389816917bc4f Mon Sep 17 00:00:00 2001 From: Shubham Ugare Date: Sun, 10 Feb 2019 21:14:00 +0530 Subject: [PATCH 1/6] Added Optional emptiness handler --- .../com/uber/nullaway/handlers/Handlers.java | 1 + .../handlers/OptionalEmptinessHandler.java | 129 ++++++++++++++++++ .../java/com/uber/nullaway/NullAwayTest.java | 38 ++++++ 3 files changed, 168 insertions(+) create mode 100644 nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java 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 From 884528b566ff7100dabfc102705593ba3a42acf9 Mon Sep 17 00:00:00 2001 From: Shubham Ugare Date: Wed, 20 Feb 2019 02:14:07 +0530 Subject: [PATCH 2/6] Made changes according to the discussion --- .../com/uber/nullaway/AbstractConfig.java | 7 ++++ .../main/java/com/uber/nullaway/Config.java | 3 ++ .../com/uber/nullaway/DummyOptionsConfig.java | 5 +++ .../nullaway/ErrorProneCLIFlagsConfig.java | 2 ++ .../main/java/com/uber/nullaway/NullAway.java | 12 ++++++- .../nullaway/handlers/BaseNoOpHandler.java | 5 +++ .../nullaway/handlers/CompositeHandler.java | 9 +++++ .../com/uber/nullaway/handlers/Handler.java | 9 +++++ .../com/uber/nullaway/handlers/Handlers.java | 4 ++- .../handlers/LibraryModelsHandler.java | 1 + .../handlers/OptionalEmptinessHandler.java | 9 +++++ .../java/com/uber/nullaway/NullAwayTest.java | 33 ++++++++++++++++--- .../testdata/NullAwayNativeModels.java | 3 ++ 13 files changed, 96 insertions(+), 6 deletions(-) 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) { From f639ed2980991117f53b47af0284411468b38ac6 Mon Sep 17 00:00:00 2001 From: Shubham Ugare Date: Wed, 27 Feb 2019 11:26:28 +0530 Subject: [PATCH 3/6] Removed lambda false positives --- .../main/java/com/uber/nullaway/NullAway.java | 3 +- .../dataflow/AccessPathNullnessAnalysis.java | 29 +++++++++++++-- .../java/com/uber/nullaway/NullAwayTest.java | 36 +++++++++++++++++-- 3 files changed, 60 insertions(+), 8 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 2c51c8146..caa463e13 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -365,8 +365,7 @@ private void updateEnvironmentMapping(Tree tree, VisitorState state) { // from the nested scope, so the program point doesn't matter // 2. we keep info on all locals rather than just effectively final ones for simplicity EnclosingEnvironmentNullness.instance(state.context) - .addEnvironmentMapping( - tree, analysis.getLocalVarInfoBefore(state.getPath(), state.context)); + .addEnvironmentMapping(tree, analysis.getLocalVarInfoBefore(state.getPath(), state)); } private Symbol.MethodSymbol getSymbolOfSuperConstructor( diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java index ec1351616..1da4191a7 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java @@ -19,13 +19,17 @@ package com.uber.nullaway.dataflow; import com.google.common.collect.ImmutableList; +import com.google.errorprone.VisitorState; import com.sun.source.util.TreePath; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Type; import com.sun.tools.javac.util.Context; import com.uber.nullaway.Config; import com.uber.nullaway.Nullness; import com.uber.nullaway.handlers.Handler; import java.util.Collections; import java.util.LinkedHashSet; +import java.util.Optional; import java.util.Set; import java.util.function.Predicate; import javax.annotation.Nullable; @@ -47,6 +51,8 @@ public final class AccessPathNullnessAnalysis { private final DataFlow dataFlow; + private static String OPTIONAL_PATH = "java.util.Optional"; + // Use #instance to instantiate private AccessPathNullnessAnalysis( Predicate methodReturnsNonNull, @@ -151,11 +157,11 @@ public Set getNonnullStaticFieldsBefore(TreePath path, Context context) * Get nullness info for local variables before some node * * @param path tree path to some AST node within a method / lambda / initializer - * @param context Javac context + * @param state visitor state * @return nullness info for local variables just before the node */ - public NullnessStore getLocalVarInfoBefore(TreePath path, Context context) { - NullnessStore store = dataFlow.resultBefore(path, context, nullnessPropagation); + public NullnessStore getLocalVarInfoBefore(TreePath path, VisitorState state) { + NullnessStore store = dataFlow.resultBefore(path, state.context, nullnessPropagation); if (store == null) { return NullnessStore.empty(); } @@ -169,6 +175,23 @@ public NullnessStore getLocalVarInfoBefore(TreePath path, Context context) { || e.getKind().equals(ElementKind.LOCAL_VARIABLE); } } + + // a filter for Optional get() call + if (ap.getElements().size() == 1) { + AccessPath.Root root = ap.getRoot(); + if (!root.isReceiver() && (ap.getElements().get(0) instanceof Symbol.MethodSymbol)) { + final Element e = root.getVarElement(); + final Symbol.MethodSymbol g = (Symbol.MethodSymbol) ap.getElements().get(0); + final Optional tbaseType = + Optional.ofNullable(state.getTypeFromString(OPTIONAL_PATH)) + .map(state.getTypes()::erasure); + return e.getKind().equals(ElementKind.LOCAL_VARIABLE) + && g.getSimpleName().toString().equals("get") + && g.getParameters().length() == 0 + && tbaseType.isPresent() + && state.getTypes().isSubtype(g.owner.type, tbaseType.get()); + } + } return false; }); } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java index e6b3619dd..dc41f53c7 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java @@ -1190,6 +1190,7 @@ public void OptionalEmptinessHandlerTest() { "package com.uber;", "import java.util.Optional;", "import javax.annotation.Nullable;", + "import com.google.common.base.Function;", "public class TestNegative {", " void foo() {", " Optional a = Optional.empty();", @@ -1197,19 +1198,37 @@ public void OptionalEmptinessHandlerTest() { " if(a.isPresent()){", " a.get().toString();", " }", - " }", + " }", + " public void lambdaConsumer(Function a){", + " return;", + " }", + " void bar() {", + " Optional b = Optional.empty();", + " if(b.isPresent()){", + " lambdaConsumer(v -> b.get().toString());", + " }", + " }", "}") .addSourceLines( "TestPositive.java", "package com.uber;", "import java.util.Optional;", "import javax.annotation.Nullable;", + "import com.google.common.base.Function;", "public class TestPositive {", " void foo() {", " Optional a = Optional.empty();", " // BUG: Diagnostic contains: Optional a can be empty", " a.get().toString();", " }", + " public void lambdaConsumer(Function a){", + " return;", + " }", + " void bar() {", + " Optional b = Optional.empty();", + " // BUG: Diagnostic contains: Optional b can be empty", + " lambdaConsumer(v -> b.get().toString());", + " }", "}") .doTest(); } @@ -1224,16 +1243,27 @@ public void OptionalEmptinessUncheckedTest() { "-XepOpt:NullAway:AnnotatedPackages=com.uber", "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated")) .addSourceLines( - "TestPositive.java", + "TestNegative.java", "package com.uber;", "import java.util.Optional;", "import javax.annotation.Nullable;", - "public class TestPositive {", + "import com.google.common.base.Function;", + "public class TestNegative {", " void foo() {", " Optional a = Optional.empty();", " // no error since the handler is not attached", " a.get().toString();", " }", + " public void lambdaConsumer(Function a){", + " return;", + " }", + " void bar() {", + " Optional b = Optional.empty();", + " if(b.isPresent()){", + " // no error since the handler is not attached", + " lambdaConsumer(v -> b.get().toString());", + " }", + " }", "}") .doTest(); } From c7bf3d0a35187c710f169a80f928d67b165a8d70 Mon Sep 17 00:00:00 2001 From: Shubham Ugare Date: Wed, 6 Mar 2019 14:46:25 +0530 Subject: [PATCH 4/6] addressed comments --- .../main/java/com/uber/nullaway/Config.java | 7 +++- .../main/java/com/uber/nullaway/NullAway.java | 42 ++++++++++++------- .../uber/nullaway/dataflow/AccessPath.java | 2 +- .../dataflow/AccessPathNullnessAnalysis.java | 25 ++--------- .../nullaway/handlers/BaseNoOpHandler.java | 9 +++- .../nullaway/handlers/CompositeHandler.java | 17 ++++++-- .../com/uber/nullaway/handlers/Handler.java | 17 +++++++- .../handlers/OptionalEmptinessHandler.java | 25 ++++++++++- 8 files changed, 96 insertions(+), 48 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/Config.java b/nullaway/src/main/java/com/uber/nullaway/Config.java index afcbe9113..be7ae6662 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Config.java +++ b/nullaway/src/main/java/com/uber/nullaway/Config.java @@ -107,7 +107,12 @@ public interface Config { */ boolean acknowledgeRestrictiveAnnotations(); - /** @return true if Optional Emptiness Handler is to be used. */ + /** + * @return true if Optional Emptiness Handler is to be used. When Optional.get() method is called + * on an empty optional, program will crash with an exception. This handler warns on possible + * cases where Optional.get() call is made on an empty optional. Nullaway determines if an + * optional is non-empty based on Optional.isPresent() call. + */ boolean checkOptionalEmptiness(); /** diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index caa463e13..e1f05f979 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -365,7 +365,8 @@ private void updateEnvironmentMapping(Tree tree, VisitorState state) { // from the nested scope, so the program point doesn't matter // 2. we keep info on all locals rather than just effectively final ones for simplicity EnclosingEnvironmentNullness.instance(state.context) - .addEnvironmentMapping(tree, analysis.getLocalVarInfoBefore(state.getPath(), state)); + .addEnvironmentMapping( + tree, analysis.getLocalVarInfoBefore(state.getPath(), state, handler)); } private Symbol.MethodSymbol getSymbolOfSuperConstructor( @@ -1932,21 +1933,16 @@ private Description matchDereference( return Description.NO_MATCH; } if (mayBeNullExpr(state, baseExpression)) { - 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"; - } + final String message = + "dereferenced expression " + baseExpression.toString() + " is @Nullable"; + ErrorMessage errorMessage = new ErrorMessage(MessageTypes.DEREFERENCE_NULLABLE, message); + + handler.getErrorMessage(baseExpression, state, errorMessage); + return createErrorDescriptionForNullAssignment( - MessageTypes.DEREFERENCE_NULLABLE, + errorMessage.messageType, derefExpression, - message, + errorMessage.message, baseExpression, state.getPath()); } @@ -2346,7 +2342,23 @@ public enum MessageTypes { UNBOX_NULLABLE, NONNULL_FIELD_READ_BEFORE_INIT, ANNOTATION_VALUE_INVALID, - CAST_TO_NONNULL_ARG_NONNULL; + CAST_TO_NONNULL_ARG_NONNULL, + GET_ON_EMPTY_OPTIONAL; + } + + public static class ErrorMessage { + MessageTypes messageType; + String message; + + ErrorMessage(MessageTypes messageType, String message) { + this.messageType = messageType; + this.message = message; + } + + public void updateErrorMessage(MessageTypes messageType, String message) { + this.messageType = messageType; + this.message = message; + } } @AutoValue diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java index 86fa69e12..1f047413f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java @@ -311,7 +311,7 @@ public static boolean isMapPut(Symbol.MethodSymbol symbol, Types types) { * root of an access path; either a variable {@link javax.lang.model.element.Element} or * this (enclosing method receiver) */ - static final class Root { + public static final class Root { /** does this represent the receiver? */ private final boolean isMethodReceiver; diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java index 1da4191a7..376b1cada 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java @@ -21,15 +21,12 @@ import com.google.common.collect.ImmutableList; import com.google.errorprone.VisitorState; import com.sun.source.util.TreePath; -import com.sun.tools.javac.code.Symbol; -import com.sun.tools.javac.code.Type; import com.sun.tools.javac.util.Context; import com.uber.nullaway.Config; import com.uber.nullaway.Nullness; import com.uber.nullaway.handlers.Handler; import java.util.Collections; import java.util.LinkedHashSet; -import java.util.Optional; import java.util.Set; import java.util.function.Predicate; import javax.annotation.Nullable; @@ -160,7 +157,7 @@ public Set getNonnullStaticFieldsBefore(TreePath path, Context context) * @param state visitor state * @return nullness info for local variables just before the node */ - public NullnessStore getLocalVarInfoBefore(TreePath path, VisitorState state) { + public NullnessStore getLocalVarInfoBefore(TreePath path, VisitorState state, Handler handler) { NullnessStore store = dataFlow.resultBefore(path, state.context, nullnessPropagation); if (store == null) { return NullnessStore.empty(); @@ -176,30 +173,14 @@ public NullnessStore getLocalVarInfoBefore(TreePath path, VisitorState state) { } } - // a filter for Optional get() call - if (ap.getElements().size() == 1) { - AccessPath.Root root = ap.getRoot(); - if (!root.isReceiver() && (ap.getElements().get(0) instanceof Symbol.MethodSymbol)) { - final Element e = root.getVarElement(); - final Symbol.MethodSymbol g = (Symbol.MethodSymbol) ap.getElements().get(0); - final Optional tbaseType = - Optional.ofNullable(state.getTypeFromString(OPTIONAL_PATH)) - .map(state.getTypes()::erasure); - return e.getKind().equals(ElementKind.LOCAL_VARIABLE) - && g.getSimpleName().toString().equals("get") - && g.getParameters().length() == 0 - && tbaseType.isPresent() - && state.getTypes().isSubtype(g.owner.type, tbaseType.get()); - } - } - return false; + return handler.filterApForLocalVarInfoBefore(ap, state); }); } /** * @param path tree path of static method, or initializer block * @param context Javac context - * @return fields guaranteed to be nonnull at exit of static method (or initializer block) + * @return fields guaranteed to be nonnull at exit of static method (or initialize r block) */ public Set getNonnullStaticFieldsAtExit(TreePath path, Context context) { NullnessStore nullnessResult = dataFlow.finalResult(path, context, nullnessPropagation); 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 b8ccfc0e3..c98fa9f5c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java @@ -35,6 +35,7 @@ import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.Context; import com.uber.nullaway.NullAway; +import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; import com.uber.nullaway.dataflow.NullnessStore; import java.util.List; @@ -168,7 +169,13 @@ public void onDataflowVisitLambdaResultExpression( } @Override - public boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state) { + public void getErrorMessage( + ExpressionTree expr, VisitorState state, NullAway.ErrorMessage errorMessage) { + // NoOp + } + + @Override + public boolean filterApForLocalVarInfoBefore(AccessPath accessPath, 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 8b2d1d124..aba108fc0 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java @@ -36,6 +36,7 @@ import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.Context; import com.uber.nullaway.NullAway; +import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; import com.uber.nullaway.dataflow.NullnessStore; import java.util.List; @@ -209,11 +210,19 @@ public void onDataflowVisitLambdaResultExpression( } @Override - public boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state) { - boolean ifOptionalGetCall = false; + public void getErrorMessage( + ExpressionTree expr, VisitorState state, NullAway.ErrorMessage errorMessage) { for (Handler h : handlers) { - ifOptionalGetCall |= h.checkIfOptionalGetCall(expr, state); + h.getErrorMessage(expr, state, errorMessage); } - return ifOptionalGetCall; + } + + @Override + public boolean filterApForLocalVarInfoBefore(AccessPath accessPath, VisitorState state) { + boolean shouldFilter = false; + for (Handler h : handlers) { + shouldFilter |= h.filterApForLocalVarInfoBefore(accessPath, state); + } + return shouldFilter; } } 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 4c0d8f1be..f02dec39f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java @@ -36,6 +36,7 @@ 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 com.uber.nullaway.dataflow.NullnessStore; import java.util.List; @@ -275,9 +276,21 @@ void onDataflowVisitLambdaResultExpression( * * @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(). + * @param errorMessage error message string and type of the error wrapped in {@link + * com.uber.nullaway.NullAway.ErrorMessage}. */ - boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state); + void getErrorMessage(ExpressionTree expr, VisitorState state, NullAway.ErrorMessage errorMessage); + + /** + * Called when the store access paths are filtered for local variable information before an + * expression. + * + * @param accessPath The access path that needs to be checked if filtered. + * @param state The current visitor state. + * @return true if the accesspath should be filtered to be included in the local variable + * information. + */ + boolean filterApForLocalVarInfoBefore(AccessPath accessPath, VisitorState state); /** * A three value enum for handlers implementing onDataflowVisitMethodInvocation to communicate 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 7a112c0d7..017c3a1ba 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java @@ -100,10 +100,31 @@ public NullnessHint onDataflowVisitMethodInvocation( } @Override - public boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state) { + public void getErrorMessage( + ExpressionTree expr, VisitorState state, NullAway.ErrorMessage errorMessage) { if (expr.getKind() == Tree.Kind.METHOD_INVOCATION && optionalIsGetCall((Symbol.MethodSymbol) ASTHelpers.getSymbol(expr), state.getTypes())) { - return true; + final int exprStringSize = expr.toString().length(); + // Name of the optional is extracted from the expression + final String message = + "Optional " + + expr.toString().substring(0, exprStringSize - 6) + + " can be empty, dereferenced get() call on it"; + errorMessage.updateErrorMessage(NullAway.MessageTypes.GET_ON_EMPTY_OPTIONAL, message); + } + } + + @Override + public boolean filterApForLocalVarInfoBefore(AccessPath accessPath, VisitorState state) { + + if (accessPath.getElements().size() == 1) { + AccessPath.Root root = accessPath.getRoot(); + if (!root.isReceiver() && (accessPath.getElements().get(0) instanceof Symbol.MethodSymbol)) { + final Element e = root.getVarElement(); + final Symbol.MethodSymbol g = (Symbol.MethodSymbol) accessPath.getElements().get(0); + return e.getKind().equals(ElementKind.LOCAL_VARIABLE) + && optionalIsGetCall(g, state.getTypes()); + } } return false; } From a155e0ecff54c4cd8ad188325cef6161035f922d Mon Sep 17 00:00:00 2001 From: Shubham Ugare Date: Thu, 7 Mar 2019 04:43:33 +0530 Subject: [PATCH 5/6] Addressed comments --- .../java/com/uber/nullaway/ErrorMessage.java | 33 +++++++++++++++ .../main/java/com/uber/nullaway/NullAway.java | 41 ++++--------------- .../dataflow/AccessPathNullnessAnalysis.java | 7 ++-- .../nullaway/handlers/BaseNoOpHandler.java | 7 ++-- .../nullaway/handlers/CompositeHandler.java | 11 ++--- .../nullaway/handlers/ContractHandler.java | 6 ++- .../com/uber/nullaway/handlers/Handler.java | 9 ++-- .../handlers/OptionalEmptinessHandler.java | 9 ++-- 8 files changed, 69 insertions(+), 54 deletions(-) create mode 100644 nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java b/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java new file mode 100644 index 000000000..17e6c0f7d --- /dev/null +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java @@ -0,0 +1,33 @@ +package com.uber.nullaway; + +/** Contains error message string to be displayed and the message type from {@link MessageTypes}. */ +public class ErrorMessage { + MessageTypes messageType; + String message; + + ErrorMessage(MessageTypes messageType, String message) { + this.messageType = messageType; + this.message = message; + } + + public void updateErrorMessage(MessageTypes messageType, String message) { + this.messageType = messageType; + this.message = message; + } + + public enum MessageTypes { + DEREFERENCE_NULLABLE, + RETURN_NULLABLE, + PASS_NULLABLE, + ASSIGN_FIELD_NULLABLE, + WRONG_OVERRIDE_RETURN, + WRONG_OVERRIDE_PARAM, + METHOD_NO_INIT, + FIELD_NO_INIT, + UNBOX_NULLABLE, + NONNULL_FIELD_READ_BEFORE_INIT, + ANNOTATION_VALUE_INVALID, + CAST_TO_NONNULL_ARG_NONNULL, + GET_ON_EMPTY_OPTIONAL; + } +} diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index e1f05f979..88094ba2d 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -88,6 +88,7 @@ import com.sun.tools.javac.code.Types; import com.sun.tools.javac.processing.JavacProcessingEnvironment; import com.sun.tools.javac.tree.JCTree; +import com.uber.nullaway.ErrorMessage.MessageTypes; import com.uber.nullaway.dataflow.AccessPathNullnessAnalysis; import com.uber.nullaway.dataflow.EnclosingEnvironmentNullness; import com.uber.nullaway.handlers.Handler; @@ -366,7 +367,7 @@ private void updateEnvironmentMapping(Tree tree, VisitorState state) { // 2. we keep info on all locals rather than just effectively final ones for simplicity EnclosingEnvironmentNullness.instance(state.context) .addEnvironmentMapping( - tree, analysis.getLocalVarInfoBefore(state.getPath(), state, handler)); + tree, analysis.getNullnessInfoBeforeNewContext(state.getPath(), state, handler)); } private Symbol.MethodSymbol getSymbolOfSuperConstructor( @@ -1937,7 +1938,7 @@ private Description matchDereference( "dereferenced expression " + baseExpression.toString() + " is @Nullable"; ErrorMessage errorMessage = new ErrorMessage(MessageTypes.DEREFERENCE_NULLABLE, message); - handler.getErrorMessage(baseExpression, state, errorMessage); + handler.onPrepareErrorMessage(baseExpression, state, errorMessage); return createErrorDescriptionForNullAssignment( errorMessage.messageType, @@ -1960,7 +1961,10 @@ private Description matchDereference( * @return the error description */ private Description createErrorDescription( - MessageTypes errorType, Tree errorLocTree, String message, TreePath path) { + com.uber.nullaway.ErrorMessage.MessageTypes errorType, + Tree errorLocTree, + String message, + TreePath path) { Tree enclosingSuppressTree = suppressibleNode(path); return createErrorDescription(errorType, errorLocTree, message, enclosingSuppressTree); } @@ -2330,37 +2334,6 @@ public void setComputedNullness(ExpressionTree e, Nullness nullness) { computedNullnessMap.put(e, nullness); } - public enum MessageTypes { - DEREFERENCE_NULLABLE, - RETURN_NULLABLE, - PASS_NULLABLE, - ASSIGN_FIELD_NULLABLE, - WRONG_OVERRIDE_RETURN, - WRONG_OVERRIDE_PARAM, - METHOD_NO_INIT, - FIELD_NO_INIT, - UNBOX_NULLABLE, - NONNULL_FIELD_READ_BEFORE_INIT, - ANNOTATION_VALUE_INVALID, - CAST_TO_NONNULL_ARG_NONNULL, - GET_ON_EMPTY_OPTIONAL; - } - - public static class ErrorMessage { - MessageTypes messageType; - String message; - - ErrorMessage(MessageTypes messageType, String message) { - this.messageType = messageType; - this.message = message; - } - - public void updateErrorMessage(MessageTypes messageType, String message) { - this.messageType = messageType; - this.message = message; - } - } - @AutoValue abstract static class FieldInitEntities { diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java index 376b1cada..40e20e902 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java @@ -157,7 +157,8 @@ public Set getNonnullStaticFieldsBefore(TreePath path, Context context) * @param state visitor state * @return nullness info for local variables just before the node */ - public NullnessStore getLocalVarInfoBefore(TreePath path, VisitorState state, Handler handler) { + public NullnessStore getNullnessInfoBeforeNewContext( + TreePath path, VisitorState state, Handler handler) { NullnessStore store = dataFlow.resultBefore(path, state.context, nullnessPropagation); if (store == null) { return NullnessStore.empty(); @@ -173,14 +174,14 @@ public NullnessStore getLocalVarInfoBefore(TreePath path, VisitorState state, Ha } } - return handler.filterApForLocalVarInfoBefore(ap, state); + return handler.includeApInfoInSavedContext(ap, state); }); } /** * @param path tree path of static method, or initializer block * @param context Javac context - * @return fields guaranteed to be nonnull at exit of static method (or initialize r block) + * @return fields guaranteed to be nonnull at exit of static method (or initializer block) */ public Set getNonnullStaticFieldsAtExit(TreePath path, Context context) { NullnessStore nullnessResult = dataFlow.finalResult(path, context, nullnessPropagation); 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 c98fa9f5c..784446919 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java @@ -34,6 +34,7 @@ import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.Context; +import com.uber.nullaway.ErrorMessage; import com.uber.nullaway.NullAway; import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; @@ -169,13 +170,13 @@ public void onDataflowVisitLambdaResultExpression( } @Override - public void getErrorMessage( - ExpressionTree expr, VisitorState state, NullAway.ErrorMessage errorMessage) { + public void onPrepareErrorMessage( + ExpressionTree expr, VisitorState state, ErrorMessage errorMessage) { // NoOp } @Override - public boolean filterApForLocalVarInfoBefore(AccessPath accessPath, VisitorState state) { + public boolean includeApInfoInSavedContext(AccessPath accessPath, 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 aba108fc0..8112b4a4a 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java @@ -35,6 +35,7 @@ import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.Context; +import com.uber.nullaway.ErrorMessage; import com.uber.nullaway.NullAway; import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; @@ -210,18 +211,18 @@ public void onDataflowVisitLambdaResultExpression( } @Override - public void getErrorMessage( - ExpressionTree expr, VisitorState state, NullAway.ErrorMessage errorMessage) { + public void onPrepareErrorMessage( + ExpressionTree expr, VisitorState state, ErrorMessage errorMessage) { for (Handler h : handlers) { - h.getErrorMessage(expr, state, errorMessage); + h.onPrepareErrorMessage(expr, state, errorMessage); } } @Override - public boolean filterApForLocalVarInfoBefore(AccessPath accessPath, VisitorState state) { + public boolean includeApInfoInSavedContext(AccessPath accessPath, VisitorState state) { boolean shouldFilter = false; for (Handler h : handlers) { - shouldFilter |= h.filterApForLocalVarInfoBefore(accessPath, state); + shouldFilter |= h.includeApInfoInSavedContext(accessPath, state); } return shouldFilter; } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/ContractHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/ContractHandler.java index 527f78a00..4a1c4aefc 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/ContractHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/ContractHandler.java @@ -30,6 +30,7 @@ import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.Context; +import com.uber.nullaway.ErrorMessage; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; import com.uber.nullaway.dataflow.AccessPath; @@ -221,7 +222,10 @@ private void reportMatch(Tree errorLocTree, String message) { if (this.analysis != null && this.state != null) { this.state.reportMatch( this.analysis.createErrorDescription( - NullAway.MessageTypes.ANNOTATION_VALUE_INVALID, errorLocTree, message, errorLocTree)); + ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, + errorLocTree, + message, + errorLocTree)); } } 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 f02dec39f..e86e6307d 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java @@ -34,6 +34,7 @@ import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.Context; +import com.uber.nullaway.ErrorMessage; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; import com.uber.nullaway.dataflow.AccessPath; @@ -279,7 +280,7 @@ void onDataflowVisitLambdaResultExpression( * @param errorMessage error message string and type of the error wrapped in {@link * com.uber.nullaway.NullAway.ErrorMessage}. */ - void getErrorMessage(ExpressionTree expr, VisitorState state, NullAway.ErrorMessage errorMessage); + void onPrepareErrorMessage(ExpressionTree expr, VisitorState state, ErrorMessage errorMessage); /** * Called when the store access paths are filtered for local variable information before an @@ -287,10 +288,10 @@ void onDataflowVisitLambdaResultExpression( * * @param accessPath The access path that needs to be checked if filtered. * @param state The current visitor state. - * @return true if the accesspath should be filtered to be included in the local variable - * information. + * @return true if the nullability information for this accesspath should be treated as part of + * the surrounding context when processing a lambda expression or anonymous class declaration. */ - boolean filterApForLocalVarInfoBefore(AccessPath accessPath, VisitorState state); + boolean includeApInfoInSavedContext(AccessPath accessPath, VisitorState state); /** * A three value enum for handlers implementing onDataflowVisitMethodInvocation to communicate 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 017c3a1ba..454c940e4 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java @@ -31,6 +31,7 @@ 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.ErrorMessage; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; import com.uber.nullaway.dataflow.AccessPath; @@ -100,8 +101,8 @@ public NullnessHint onDataflowVisitMethodInvocation( } @Override - public void getErrorMessage( - ExpressionTree expr, VisitorState state, NullAway.ErrorMessage errorMessage) { + public void onPrepareErrorMessage( + ExpressionTree expr, VisitorState state, ErrorMessage errorMessage) { if (expr.getKind() == Tree.Kind.METHOD_INVOCATION && optionalIsGetCall((Symbol.MethodSymbol) ASTHelpers.getSymbol(expr), state.getTypes())) { final int exprStringSize = expr.toString().length(); @@ -110,12 +111,12 @@ && optionalIsGetCall((Symbol.MethodSymbol) ASTHelpers.getSymbol(expr), state.get "Optional " + expr.toString().substring(0, exprStringSize - 6) + " can be empty, dereferenced get() call on it"; - errorMessage.updateErrorMessage(NullAway.MessageTypes.GET_ON_EMPTY_OPTIONAL, message); + errorMessage.updateErrorMessage(ErrorMessage.MessageTypes.GET_ON_EMPTY_OPTIONAL, message); } } @Override - public boolean filterApForLocalVarInfoBefore(AccessPath accessPath, VisitorState state) { + public boolean includeApInfoInSavedContext(AccessPath accessPath, VisitorState state) { if (accessPath.getElements().size() == 1) { AccessPath.Root root = accessPath.getRoot(); From a521ff09dca7ebbd3c5c209b8a51a66c1c0e92e5 Mon Sep 17 00:00:00 2001 From: Shubham Ugare Date: Thu, 7 Mar 2019 04:51:53 +0530 Subject: [PATCH 6/6] Minor nit --- nullaway/src/main/java/com/uber/nullaway/NullAway.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 88094ba2d..6019081dd 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1961,10 +1961,7 @@ private Description matchDereference( * @return the error description */ private Description createErrorDescription( - com.uber.nullaway.ErrorMessage.MessageTypes errorType, - Tree errorLocTree, - String message, - TreePath path) { + MessageTypes errorType, Tree errorLocTree, String message, TreePath path) { Tree enclosingSuppressTree = suppressibleNode(path); return createErrorDescription(errorType, errorLocTree, message, enclosingSuppressTree); }