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

Added Optional emptiness handler #278

Merged
merged 6 commits into from Mar 6, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java
Expand Up @@ -68,6 +68,8 @@ public abstract class AbstractConfig implements Config {

protected boolean isAcknowledgeRestrictive;

protected boolean checkOptionalEmptiness;

protected boolean assertsEnabled;

/**
Expand Down Expand Up @@ -182,6 +184,11 @@ public boolean acknowledgeRestrictiveAnnotations() {
return isAcknowledgeRestrictive;
}

@Override
public boolean checkOptionalEmptiness() {
return checkOptionalEmptiness;
}

@Override
public boolean assertsEnabled() {
return assertsEnabled;
Expand Down
8 changes: 8 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/Config.java
Expand Up @@ -107,6 +107,14 @@ public interface Config {
*/
boolean acknowledgeRestrictiveAnnotations();

/**
* @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();

/**
* @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
Expand Down
Expand Up @@ -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() {
Expand Down
33 changes: 33 additions & 0 deletions 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;
}
}
Expand Up @@ -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";
Expand Down Expand Up @@ -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 =
Expand Down
29 changes: 10 additions & 19 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Expand Up @@ -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;
Expand Down Expand Up @@ -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.context));
tree, analysis.getNullnessInfoBeforeNewContext(state.getPath(), state, handler));
}

private Symbol.MethodSymbol getSymbolOfSuperConstructor(
Expand Down Expand Up @@ -1933,11 +1934,16 @@ private Description matchDereference(
return Description.NO_MATCH;
}
if (mayBeNullExpr(state, baseExpression)) {
String 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.onPrepareErrorMessage(baseExpression, state, errorMessage);

return createErrorDescriptionForNullAssignment(
MessageTypes.DEREFERENCE_NULLABLE,
errorMessage.messageType,
derefExpression,
message,
errorMessage.message,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably change all the createErrorDescription[...] methods to just take ErrorMessage rather than the separate components. But let's leave that for a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely.

baseExpression,
state.getPath());
}
Expand Down Expand Up @@ -2325,21 +2331,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;
}

@AutoValue
abstract static class FieldInitEntities {

Expand Down
Expand Up @@ -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 <code>
* this</code> (enclosing method receiver)
*/
static final class Root {
public static final class Root {

/** does this represent the receiver? */
private final boolean isMethodReceiver;
Expand Down
Expand Up @@ -19,6 +19,7 @@
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.util.Context;
import com.uber.nullaway.Config;
Expand Down Expand Up @@ -47,6 +48,8 @@ public final class AccessPathNullnessAnalysis {

private final DataFlow dataFlow;

private static String OPTIONAL_PATH = "java.util.Optional";

// Use #instance to instantiate
private AccessPathNullnessAnalysis(
Predicate<MethodInvocationNode> methodReturnsNonNull,
Expand Down Expand Up @@ -151,11 +154,12 @@ public Set<Element> 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 getNullnessInfoBeforeNewContext(
TreePath path, VisitorState state, Handler handler) {
NullnessStore store = dataFlow.resultBefore(path, state.context, nullnessPropagation);
if (store == null) {
return NullnessStore.empty();
}
Expand All @@ -169,7 +173,8 @@ public NullnessStore getLocalVarInfoBefore(TreePath path, Context context) {
|| e.getKind().equals(ElementKind.LOCAL_VARIABLE);
}
}
return false;

return handler.includeApInfoInSavedContext(ap, state);
});
}

Expand Down
Expand Up @@ -34,7 +34,9 @@
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;
import com.uber.nullaway.dataflow.NullnessStore;
import java.util.List;
Expand Down Expand Up @@ -166,4 +168,15 @@ public void onDataflowVisitLambdaResultExpression(
ExpressionTree tree, NullnessStore thenStore, NullnessStore elseStore) {
// NoOp
}

@Override
public void onPrepareErrorMessage(
ExpressionTree expr, VisitorState state, ErrorMessage errorMessage) {
// NoOp
}

@Override
public boolean includeApInfoInSavedContext(AccessPath accessPath, VisitorState state) {
return false;
}
}
Expand Up @@ -35,7 +35,9 @@
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;
import com.uber.nullaway.dataflow.NullnessStore;
import java.util.List;
Expand Down Expand Up @@ -207,4 +209,21 @@ public void onDataflowVisitLambdaResultExpression(
h.onDataflowVisitLambdaResultExpression(tree, thenStore, elseStore);
}
}

@Override
public void onPrepareErrorMessage(
ExpressionTree expr, VisitorState state, ErrorMessage errorMessage) {
for (Handler h : handlers) {
h.onPrepareErrorMessage(expr, state, errorMessage);
}
}

@Override
public boolean includeApInfoInSavedContext(AccessPath accessPath, VisitorState state) {
boolean shouldFilter = false;
for (Handler h : handlers) {
shouldFilter |= h.includeApInfoInSavedContext(accessPath, state);
}
return shouldFilter;
}
}
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}
}

Expand Down
23 changes: 23 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java
Expand Up @@ -34,8 +34,10 @@
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;
import com.uber.nullaway.dataflow.AccessPathNullnessPropagation;
import com.uber.nullaway.dataflow.NullnessStore;
import java.util.List;
Expand Down Expand Up @@ -270,6 +272,27 @@ 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.
* @param errorMessage error message string and type of the error wrapped in {@link
* com.uber.nullaway.NullAway.ErrorMessage}.
*/
void onPrepareErrorMessage(ExpressionTree expr, VisitorState state, 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 nullability information for this accesspath should be treated as part of
* the surrounding context when processing a lambda expression or anonymous class declaration.
*/
boolean includeApInfoInSavedContext(AccessPath accessPath, 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.
Expand Down
Expand Up @@ -50,6 +50,9 @@ public static Handler buildDefault(Config config) {
handlerListBuilder.add(new RxNullabilityPropagator());
handlerListBuilder.add(new ContractHandler());
handlerListBuilder.add(new ApacheThriftIsSetHandler());
if (config.checkOptionalEmptiness()) {
handlerListBuilder.add(new OptionalEmptinessHandler());
}
return new CompositeHandler(handlerListBuilder.build());
}

Expand Down
Expand Up @@ -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", "<T>of(T)"), 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

.put(methodRef("java.util.Deque", "addFirst(E)"), 0)
.put(methodRef("java.util.Deque", "addLast(E)"), 0)
.put(methodRef("java.util.Deque", "offerFirst(E)"), 0)
Expand Down