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
Changes from 4 commits
fd0fadd
884528b
f639ed2
c7bf3d0
a155e0e
a521ff0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -366,7 +366,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.getLocalVarInfoBefore(state.getPath(), state, handler)); | ||
} | ||
|
||
private Symbol.MethodSymbol getSymbolOfSuperConstructor( | ||
|
@@ -1933,11 +1933,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.getErrorMessage(baseExpression, state, errorMessage); | ||
|
||
return createErrorDescriptionForNullAssignment( | ||
MessageTypes.DEREFERENCE_NULLABLE, | ||
errorMessage.messageType, | ||
derefExpression, | ||
message, | ||
errorMessage.message, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably change all the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, definitely. |
||
baseExpression, | ||
state.getPath()); | ||
} | ||
|
@@ -2337,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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like this. But, can we extract this and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍Done |
||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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, | ||
|
@@ -151,11 +154,11 @@ 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 getLocalVarInfoBefore(TreePath path, VisitorState state, Handler handler) { | ||
NullnessStore store = dataFlow.resultBefore(path, state.context, nullnessPropagation); | ||
if (store == null) { | ||
return NullnessStore.empty(); | ||
} | ||
|
@@ -169,14 +172,15 @@ public NullnessStore getLocalVarInfoBefore(TreePath path, Context context) { | |
|| e.getKind().equals(ElementKind.LOCAL_VARIABLE); | ||
} | ||
} | ||
return false; | ||
|
||
return handler.filterApForLocalVarInfoBefore(ap, state); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure the name is super clear. But can't also think of a better one. @msridhar ? (This definitely is a better design, though :) ) p.s. See #278 (comment) for further discussion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍Done |
||
}); | ||
} | ||
|
||
/** | ||
* @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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo? |
||
*/ | ||
public Set<Element> getNonnullStaticFieldsAtExit(TreePath path, Context context) { | ||
NullnessStore nullnessResult = dataFlow.finalResult(path, context, nullnessPropagation); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -270,6 +271,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 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "should be filtered to be included" doesn't easily parse in my mind. Are we excluding or including? (e.g. is this a whitelist or a blacklist?). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reading the rest of the code more carefully, I'd go with "@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" I'd also rename If we do rename that, we can have this method be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @lazaroclapp's comments and suggested names 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍Done |
||
* information. | ||
*/ | ||
boolean filterApForLocalVarInfoBefore(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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is changing the error message text, rather than retrieving a new copy, it shouldn't be called
get[...]
. How aboutHandler.onPrepareErrorMessage
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍