Pass resolved Symbols into Handler methods #729

XN137 · 2023-02-02T08:02:54Z

this avoids redundant ASTHelpers.getSymbol calls and is more consistent with the other Handler method signatures

XN137 · 2023-02-02T08:09:52Z

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java

    Nullness nullness = returnValueNullness(node, input, nullnessHint);
-    if (booleanReturnType(node)) {
+    if (booleanReturnType(callee)) {


note that getSymbol(MethodInvocationTree tree) never returns null (maybe this was different in previous errorprone versions?)

XN137 · 2023-02-02T08:10:59Z

nullaway/src/main/java/com/uber/nullaway/NullAway.java

@@ -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);


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 ?

XN137 · 2023-02-02T08:11:59Z

nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java

@@ -45,17 +45,13 @@ public class AssertionHandler extends BaseNoOpHandler {
  @Override
  public NullnessHint onDataflowVisitMethodInvocation(
      MethodInvocationNode node,
+      Symbol.MethodSymbol callee,


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

XN137 · 2023-02-02T08:13:04Z

nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java

      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) {


this could never be null (and is no checked at the root call site anyway)

msridhar · 2023-02-05T18:53:26Z

@XN137 in my opinion, the benefit of avoiding extra ASTHelpers.getSymbol calls here is not worth having to add another parameter to the handler method, just in terms of readability. Other handler methods take either a Symbol or a Tree, but none take both. If we could eliminate the expr parameter this could be worthwhile, but unfortunately it looks like it is needed in a couple of places. I am open to discussion if there are benefits to this change I am not seeing.

XN137 · 2023-02-05T19:32:59Z

@msridhar thanks for the feedback

Other handler methods take either a Symbol or a Tree, but none take both.

i might be missing something but afaict the first 5 methods of the Handler interface DO take both:

NullAway/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

Lines 73 to 74 in ed0ef72

    
           void onMatchTopLevelClass( 
        
               NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol);

NullAway/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

Lines 84 to 85 in ed0ef72

    
           void onMatchMethod( 
        
               NullAway analysis, MethodTree tree, VisitorState state, Symbol.MethodSymbol methodSymbol);

etc...

also further down:

NullAway/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

Lines 258 to 260 in ed0ef72

    
           NullnessHint onDataflowVisitFieldAccess( 
        
               FieldAccessNode node, 
        
               Symbol symbol,

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)

msridhar · 2023-02-05T21:16:30Z

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 ExpressionTrees will have a symbol necessarily. The one case I am thinking of is a ConditionalExpressionTree, representing an expression like b ? x : y, which I believe will not have a symbol. So ASTHelpers.getSymbol() can certainly return null. That said, there are many cases where we can be strongly confident it will not return null unless something else is messed up in the compilation, e.g., for an identifier. In all the cases where handler methods currently take a symbol, we can be confident it won't be null.

Since we can't capture the right invariants in the NullAway type system, I'm not sure if making ASTHelpers.getSymbol() return @Nullable in general will be an overall positive for the codebase. @lazaroclapp thoughts here? Would be interested to get your thoughts on this PR in general. Overall I'm still unsure if this change is a net win.

XN137 · 2023-02-06T12:00:15Z

i've filed #733 for the general discussion around ASTHelpers

for this PR i have marked the new parameter @Nullable Symbol exprSymbol

let me know what you think

XN137 · 2023-02-13T17:14:48Z

@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

lazaroclapp

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!

lazaroclapp · 2023-03-09T20:42:21Z

nullaway/src/main/java/com/uber/nullaway/NullAway.java

@@ -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);


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

XN137 · 2023-03-09T21:11:23Z

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.

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,
let me know in case anything else is needed

lazaroclapp

LGTM!

…)" This reverts commit f743be3.

XN137 commented Feb 2, 2023

View reviewed changes

XN137 marked this pull request as ready for review February 2, 2023 08:16

XN137 force-pushed the pass-handler-symbols branch from dfb5595 to e4e75e9 Compare February 3, 2023 16:32

XN137 force-pushed the pass-handler-symbols branch 3 times, most recently from 91bdf51 to fb72a81 Compare February 6, 2023 11:53

XN137 mentioned this pull request Feb 6, 2023

Prepare for Nullable ASTHelpers.getSymbol #733

Merged

XN137 force-pushed the pass-handler-symbols branch 2 times, most recently from e425958 to f917733 Compare February 8, 2023 10:29

lazaroclapp reviewed Mar 9, 2023

View reviewed changes

XN137 added 2 commits March 9, 2023 22:05

Pass resolved Symbols into Handler methods

0bed230

this avoids redundant ASTHelpers.getSymbol calls and is more consistent with the other Handler method signatures

review: remove annotation on local var

a2a3bd9

XN137 force-pushed the pass-handler-symbols branch from f917733 to a2a3bd9 Compare March 9, 2023 21:09

lazaroclapp approved these changes Mar 9, 2023

View reviewed changes

lazaroclapp enabled auto-merge (squash) March 9, 2023 21:17

lazaroclapp merged commit f743be3 into uber:master Mar 9, 2023

XN137 deleted the pass-handler-symbols branch March 10, 2023 06:43

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023

Revert "[Refactor] Pass resolved Symbols into Handler methods (uber#729…

e50ea35

…)" This reverts commit f743be3.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023

Revert "[Refactor] Pass resolved Symbols into Handler methods (uber#729…

5cc559d

…)" This reverts commit f743be3.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023

Revert "[Refactor] Pass resolved Symbols into Handler methods (uber#729…

417df02

…)" This reverts commit f743be3.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023

Revert "[Refactor] Pass resolved Symbols into Handler methods (uber#729…

d710cda

…)" This reverts commit f743be3.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023

Revert "[Refactor] Pass resolved Symbols into Handler methods (uber#729…

32118fb

…)" This reverts commit f743be3.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass resolved Symbols into Handler methods #729

Pass resolved Symbols into Handler methods #729

XN137 commented Feb 2, 2023

XN137 Feb 2, 2023

XN137 Feb 2, 2023

XN137 Feb 2, 2023

XN137 Feb 2, 2023

msridhar commented Feb 5, 2023

XN137 commented Feb 5, 2023

msridhar commented Feb 5, 2023

XN137 commented Feb 6, 2023

XN137 commented Feb 13, 2023

lazaroclapp left a comment •

edited

Loading

lazaroclapp Mar 9, 2023

XN137 commented Mar 9, 2023 •

edited

Loading

lazaroclapp left a comment

Pass resolved Symbols into Handler methods #729

Pass resolved Symbols into Handler methods #729

Conversation

XN137 commented Feb 2, 2023

XN137 Feb 2, 2023

Choose a reason for hiding this comment

XN137 Feb 2, 2023

Choose a reason for hiding this comment

XN137 Feb 2, 2023

Choose a reason for hiding this comment

XN137 Feb 2, 2023

Choose a reason for hiding this comment

msridhar commented Feb 5, 2023

XN137 commented Feb 5, 2023

msridhar commented Feb 5, 2023

XN137 commented Feb 6, 2023

XN137 commented Feb 13, 2023

lazaroclapp left a comment • edited Loading

Choose a reason for hiding this comment

lazaroclapp Mar 9, 2023

Choose a reason for hiding this comment

XN137 commented Mar 9, 2023 • edited Loading

lazaroclapp left a comment

Choose a reason for hiding this comment

lazaroclapp left a comment •

edited

Loading

XN137 commented Mar 9, 2023 •

edited

Loading