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

Conversation

shubhamugare
Copy link
Contributor

Added Optional Emptiness handler which does couple of things

  1. on optionalFoo.get() hints that it can be Nullable.
  2. on optionalFoo.isPresent() sets the optionalFoo.get() access path to Non-null.

This allows

if (fooOptional.isPresent()) {
  fooOptional.get().bar(); // Implicitly understood to be safe
}

and returns a warning if not implied safe.

Fixes #274
Added unit tests

" void foo() {",
" Optional<Object> a = Optional.of(null);",
" // BUG: Diagnostic contains: dereferenced expression a.get() is @Nullable",
" a.get().toString();",

Choose a reason for hiding this comment

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

This doesn't seem right; Optional.of(null) will throw a NullPointerException because Optional can't wrap null. So this a.get() won't be called, and in other contexts Optional#get will never yield null.

Or do I misunderstand completely? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I understand this does not matter. As we are not going to run this code. We just run the static analysis on this code. So as long as we initialise a with any method it is alright.

Anyway if you think I can write this test differently. Let me know.

Copy link

Choose a reason for hiding this comment

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

I agree that in the context of this test no NPE will happen, but the test does show that the plugin would make a "false statement" in case it were applied to real code. After all, Optional#get() is never @Nullable.

A more realistic test could be:

Optional<Object> a = Optional.empty();
// BUG: Diagnostic contains: dereferenced expression a.get() may be empty
a.get().toString();

... but that does require customizing the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks 🙂
I will think about how can I customize the error 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 actually be modeling Optional.of() such that the parameter is @NonNull. We should add a library model here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the actual result of Optional<Object> a = Optional.empty(); a.get() ? An exception?

Actually, thinking about how to customize the message, we could have a library model list of methods with a "pseudo-null" return and the actual message that should be printed instead of the usual "is @nullable" error.

Choose a reason for hiding this comment

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

What is the actual result of Optional<Object> a = Optional.empty(); a.get() ? An exception?

Yep:

$ jshell
|  Welcome to JShell -- Version 11.0.2
|  For an introduction type: /help intro

jshell> Optional<Object> a = Optional.empty()
a ==> Optional.empty

jshell> a.get()
|  Exception java.util.NoSuchElementException: No value present
|        at Optional.get (Optional.java:148)
|        at (#2:1)

W.r.t. your customization suggestion: SGTM :)

@msridhar
Copy link
Collaborator

I really like the idea here! A few high-level thoughts:

  • We may need to add modeling for Optional.of(), returning an Optional whose get() result is @NonNull. Not sure how important this is, depends on how many false positives we get. Also not sure how to implement if we need it.
  • We will also get false positives across procedure boundaries (e.g. if you call isPresent() in caller and get() in the callee). Again not sure how important it is. The Checker Framework uses an @Present annotation to handle this.
  • We may get false positives with Rx streams, if we have a filter() checking isPresent() and then subsequent callbacks using get() without a check.

Bottom line, I like the approach here, but (1) we should almost certainly have it off by default at first, and (2) before we land we should probably test it on some large code base like Uber's to see how many false positives turn up.

@lazaroclapp any further thoughts?

@@ -107,6 +107,9 @@
*/
boolean acknowledgeRestrictiveAnnotations();

/** @return true if Optional Emptiness Handler is to be used. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we be more specific on what this does? We should say this enables checking for Optional.get() calls without checking Optional.isPresent()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Either more detail here or maybe even a paragraph or two in the docs and link from here (maybe both?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍
Will add few lines in the wiki

@@ -1933,7 +1932,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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we want a more general mechanism here, not something specific to Optional.get(). We should refactor and let the handler return a different error message if desired given the baseExpression and state

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. We need an entry point into the base Handler interface that allows to override error messages for a given combination of message type (see MessageTypes), expression, and state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍Done

@@ -107,6 +107,9 @@
*/
boolean acknowledgeRestrictiveAnnotations();

/** @return true if Optional Emptiness Handler is to be used. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Either more detail here or maybe even a paragraph or two in the docs and link from here (maybe both?)

@@ -1933,7 +1932,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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. We need an entry point into the base Handler interface that allows to override error messages for a given combination of message type (see MessageTypes), expression, and state.

@@ -169,6 +175,23 @@ public NullnessStore getLocalVarInfoBefore(TreePath path, Context context) {
|| e.getKind().equals(ElementKind.LOCAL_VARIABLE);
}
}

// a filter for Optional get() call
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we adding code to the core here? I thought the idea was to abstract this all behind a handler and guard it with a flag. This code is neither... am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to the handler 👍

* @param state The current visitor state.
* @return If the method call in the expression is to Optional.get().
*/
boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state);
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, this should be a more general message overriding method.

@@ -289,6 +289,7 @@ private static LibraryModels loadLibraryModels() {
"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.

👍

Copy link
Collaborator

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

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

A few blocking comments, but they are mostly nits about method names and docstrings. This is looking really good to me now overall 👍

"dereferenced expression " + baseExpression.toString() + " is @Nullable";
ErrorMessage errorMessage = new ErrorMessage(MessageTypes.DEREFERENCE_NULLABLE, message);

handler.getErrorMessage(baseExpression, state, errorMessage);
Copy link
Collaborator

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 about Handler.onPrepareErrorMessage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👍

GET_ON_EMPTY_OPTIONAL;
}

public static class ErrorMessage {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like this. But, can we extract this and MessageTypes to another class/file if we are using them from the handlers. NullAway.java is just massive. Maybe this should be a top level ErrorMessage class containing the ErrorMessage.MessageTypes enum?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍Done

}
}
return false;
return handler.filterApForLocalVarInfoBefore(ap, state);
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo?

*
* @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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?).

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 getLocalVarInfoBefore to e.g. getNullnessInfoBeforeNewContext, to more accurately reflect the fact that it preserves info for APs other than locals. But let's see if that makes sense to @msridhar as well before going ahead with that renaming for a core API method :)

If we do rename that, we can have this method be Handler.includeApInfoInSavedContext, or something like that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @lazaroclapp's comments and suggested names 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍Done

@msridhar
Copy link
Collaborator

msridhar commented Mar 6, 2019

LGTM once @lazaroclapp's comments are addressed!

Copy link
Collaborator

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

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

Minor nit. Otherwise LGTM.

@@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be ErrorMessage.MessageTypes, no? Rather than the FQN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants