-
Notifications
You must be signed in to change notification settings - Fork 291
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
Pass resolved Symbols into Handler methods #729
Conversation
Nullness nullness = returnValueNullness(node, input, nullnessHint); | ||
if (booleanReturnType(node)) { | ||
if (booleanReturnType(callee)) { |
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.
note that getSymbol(MethodInvocationTree tree)
never returns null (maybe this was different in previous errorprone versions?)
@@ -2279,7 +2279,7 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) { | |||
"whoops, better handle " + expr.getKind() + " " + state.getSourceForNode(expr)); | |||
} | |||
} | |||
exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, state, exprMayBeNull); | |||
exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, exprSymbol, state, exprMayBeNull); |
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.
do we worry about exprSymbol
being null here? should the parameter be marked nullable?
should we do Preconditions.checkNotNull
here because we know it cant be null?
taking a step back:
should we add a library model for ASTHelpers
?
@@ -45,17 +45,13 @@ public class AssertionHandler extends BaseNoOpHandler { | |||
@Override | |||
public NullnessHint onDataflowVisitMethodInvocation( | |||
MethodInvocationNode node, | |||
Symbol.MethodSymbol callee, |
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.
note that i've adjusted the parameter name in the handlers to keep the diff small
if you want it to be same as in the base interface let me know (and how to call it i.e. callee, symbol, methodSymbol etc.)
VisitorState state, | ||
AccessPath.AccessPathContext apContext, | ||
AccessPathNullnessPropagation.SubNodeValues inputs, | ||
AccessPathNullnessPropagation.Updates thenUpdates, | ||
AccessPathNullnessPropagation.Updates elseUpdates, | ||
AccessPathNullnessPropagation.Updates bothUpdates) { | ||
Symbol.MethodSymbol callee = ASTHelpers.getSymbol(node.getTree()); | ||
if (callee == null) { |
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.
this could never be null (and is no checked at the root call site anyway)
dfb5595
to
e4e75e9
Compare
@XN137 in my opinion, the benefit of avoiding extra |
@msridhar thanks for the feedback
i might be missing something but afaict the first 5 methods of the
etc... also further down: NullAway/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java Lines 258 to 260 in ed0ef72
so there are cases for passing the symbol along both trees and nodes, which is why i suggested to do the same in the other 2 methods., seems more uniform and avoids redundant work (also the nullablity of the parameter could be decided once, instead of in each subclass) |
You are completely right; somehow I missed that. My mistake. Not all Since we can't capture the right invariants in the NullAway type system, I'm not sure if making |
91bdf51
to
fb72a81
Compare
i've filed #733 for the general discussion around for this PR i have marked the new parameter let me know what you think |
e425958
to
f917733
Compare
@lazaroclapp sorry for the ping, but do you have any feedback here? if you also think this is not a clear improvement we can close it, thanks |
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.
Hey, sorry for the very long delay on this one! I've been recovering from COVID these last two weeks and busy the one before that.
I actually agree this is more consistent with other Handler
methods. My only comment here before approving is that we don't usually encourage @Nullable
in local variables for NullAway (and either way, the real fix is to make getSymbol(...)
's return @Nullable
, which seems to be happening separately from this PR). Happy to approve the refactoring after that change!
@@ -2189,7 +2189,7 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) { | |||
return false; | |||
} | |||
// the logic here is to avoid doing dataflow analysis whenever possible | |||
Symbol exprSymbol = ASTHelpers.getSymbol(expr); | |||
@Nullable Symbol exprSymbol = ASTHelpers.getSymbol(expr); |
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.
Nullability annotations should not be needed in local variables. If anything, this means we might indeed need a model for ASTHelpers.getSymbol(...)
(or, presumably, we just wait for EP to mark it @Nullable
, given the discussion in #733 )
this avoids redundant ASTHelpers.getSymbol calls and is more consistent with the other Handler method signatures
f917733
to
a2a3bd9
Compare
no worries, hope you made a full/speedy recovery and thanks for getting back to this. i've rebased and hopefully addressed your concern in the additional review commit, |
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.
LGTM!
…)" This reverts commit f743be3.
…)" This reverts commit f743be3.
…)" This reverts commit f743be3.
…)" This reverts commit f743be3.
…)" This reverts commit f743be3.
this avoids redundant ASTHelpers.getSymbol calls and is more consistent with the other Handler method signatures