refactor: streamline mayBeNullExpr flow #753

XN137 · 2023-03-31T08:51:58Z

the uniform flow is now:
- return early for obvious expression types
- determine initial exprMayBeNull through simple checks
- apply handler overrides in single place
- finally run dataflow if exprMayBeNull is true in single place

idea from discussion on other PR

open questions:

a) why are no tests failing when we return early from the switch for some expressions?
(this is giving performance benefits because we are not redundantly calling the handler chain or dataflow analysis)

-> no handler ever overrides those expression types and dataflow analysis does not matter for them?

b) why are no tests failing because we change the order of "dataflow" and "handler overrides" in some methods, even though they state the order difference is intentional and important there

-> order should be uniform with dataflow running last

msridhar · 2023-03-31T16:07:44Z

Thanks for looking into this! Re: a) I put up a commit that shows the problem:

msridhar@69cea80

There, I changed the order of dataflow and handlers for method calls, and you can see that several tests fail. This is because we rely on dataflow to "override" results from the handler, e.g., in the case of a null check of a call to Optional.get(). Interestingly, in reading the code I ran into this:

msridhar@69cea80#diff-f5f114bb393be97fed48630313d15457ba3cbd039ad291304bbbb71245cf6c2aR146

Oddly, LibraryModelsHandler is running dataflow! Under the structure I proposed here this should not be necessary.

In general, I prefer a refactor like what is in the draft here to #748. Now that we've dug into this and I've swapped things in, I'd rather do things right and make the invariants and expectations clear if we're going to do a refactor.

XN137 · 2023-03-31T17:14:33Z

i was aware that LibraryModelsHandler is calling dataflow and also found it odd, i was assuming you guys would argue its there for good reasons (even though no test is failing without)

see my suggestion around this in #754

as for the "order problem" compare current master here versus there

if we always want dataflow to run after handler overrides, then yeah we can streamline as suggested in this PR

XN137 · 2023-04-01T07:46:59Z

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

        break;
      default:
        // match switch expressions by comparing strings, so the code compiles on JDK versions < 12
        if (expr.getKind().name().equals("SWITCH_EXPRESSION")) {
-          exprMayBeNull = nullnessFromDataflow(state, expr);
+          exprMayBeNull = true;


note that here and above, we used to first run dataflow and only then called the handler overrides

while now we would be doing it the other way round.
running dataflow last seems like the most uniform way?
also there are no tests failing so maybe the old order did not matter?

Yes, agreed, running dataflow last uniformly is what we want.

We do, under some circumstances, want handlers to override the results from the dataflow analysis, but I think this should in all cases be done by using one of the handler methods that run as part of the dataflow analysis itself, not the AST-node taking methods... (Edit: to clarify, I am agreeing with the uniform ordering here)

the uniform flow is now: - return early for obvious expression types - determine initial exprMayBeNull through simple checks - apply handler overrides in single place - finally run dataflow if exprMayBeNull is true in single place

XN137 · 2023-04-01T07:55:51Z

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

        } else {
          throw new RuntimeException(
              "whoops, better handle " + expr.getKind() + " " + state.getSourceForNode(expr));
        }
    }
    exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, exprSymbol, state, exprMayBeNull);
-    return exprMayBeNull;
+    return exprMayBeNull && nullnessFromDataflow(state, expr);


together with the LibraryModelsHandler refactoring, this would be the only place where nullnessFromDataflow would be getting called still

msridhar

Overall this change looks really good to me! Modulo my relatively minor comment, I think we can shift this off of draft mode and land it (though @lazaroclapp you may want to look)

msridhar · 2023-04-02T20:25:26Z

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

-          // Check handler.onOverrideMayBeNullExpr before dataflow.
-          exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, exprSymbol, state, true);
-          return exprMayBeNull ? nullnessFromDataflow(state, expr) : false;
+          exprMayBeNull = true;


Add a comment here saying we rely on inference / dataflow analysis for local variables

done, please check if it is as you had mind.

also i tried putting answers to the questions in the PR description, wdyt?

LGTM. Will take another look later in the day and then likely land this

msridhar · 2023-04-02T20:25:55Z

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

        break;
      default:
        // match switch expressions by comparing strings, so the code compiles on JDK versions < 12
        if (expr.getKind().name().equals("SWITCH_EXPRESSION")) {
-          exprMayBeNull = nullnessFromDataflow(state, expr);
+          exprMayBeNull = true;


Yes, agreed, running dataflow last uniformly is what we want.

lazaroclapp

Makes sense to me and thanks for the clean up! Once this and the LibraryModelsHandler change both land, I'd vote we do some internal sanity testing and also some performance testing. I think we cache dataflow results, so probably we won't see a difference from the reduced number of calls to it, but might be worth double-checking! Either way, the logic is clearer, I feel!

This reverts commit 67c357e.

XN137 mentioned this pull request Mar 31, 2023

refactor to directly return mayBeNullFieldAccess #748

Closed

XN137 force-pushed the refactor-streamline-mayBeNullExpr branch from 9cb0c44 to 8618248 Compare March 31, 2023 09:07

XN137 changed the title ~~Refactor streamline may be null expr~~ refactor: streamline mayBeNullExpr flow Mar 31, 2023

XN137 mentioned this pull request Mar 31, 2023

NULL_LITERAL expressions may always be null #749

Merged

XN137 force-pushed the refactor-streamline-mayBeNullExpr branch 2 times, most recently from 05cc788 to f34cd53 Compare April 1, 2023 07:43

XN137 commented Apr 1, 2023

View reviewed changes

refactor: streamline mayBeNullExpr flow

3127f16

the uniform flow is now: - return early for obvious expression types - determine initial exprMayBeNull through simple checks - apply handler overrides in single place - finally run dataflow if exprMayBeNull is true in single place

XN137 force-pushed the refactor-streamline-mayBeNullExpr branch from f34cd53 to 3127f16 Compare April 1, 2023 07:52

XN137 commented Apr 1, 2023

View reviewed changes

msridhar reviewed Apr 3, 2023

View reviewed changes

review: add comment

51c8b8a

XN137 marked this pull request as ready for review April 4, 2023 15:26

lazaroclapp approved these changes Apr 4, 2023

View reviewed changes

msridhar approved these changes Apr 4, 2023

View reviewed changes

msridhar merged commit 67c357e into uber:master Apr 4, 2023
7 checks passed

XN137 deleted the refactor-streamline-mayBeNullExpr branch April 4, 2023 17:33

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

Revert "refactor: streamline mayBeNullExpr flow (uber#753)"

a652177

This reverts commit 67c357e.

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

Revert "refactor: streamline mayBeNullExpr flow (uber#753)"

f5e2632

This reverts commit 67c357e.

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

Revert "refactor: streamline mayBeNullExpr flow (uber#753)"

e50a049

This reverts commit 67c357e.

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

Revert "refactor: streamline mayBeNullExpr flow (uber#753)"

fbf3b13

This reverts commit 67c357e.

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

Revert "refactor: streamline mayBeNullExpr flow (uber#753)"

2045916

This reverts commit 67c357e.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: streamline mayBeNullExpr flow #753

refactor: streamline mayBeNullExpr flow #753

XN137 commented Mar 31, 2023 •

edited

msridhar commented Mar 31, 2023

XN137 commented Mar 31, 2023

XN137 Apr 1, 2023

msridhar Apr 2, 2023

lazaroclapp Apr 4, 2023 •

edited

XN137 Apr 1, 2023

msridhar left a comment

msridhar Apr 2, 2023

XN137 Apr 4, 2023

msridhar Apr 4, 2023

msridhar Apr 2, 2023

lazaroclapp left a comment

refactor: streamline mayBeNullExpr flow #753

refactor: streamline mayBeNullExpr flow #753

Conversation

XN137 commented Mar 31, 2023 • edited

msridhar commented Mar 31, 2023

XN137 commented Mar 31, 2023

XN137 Apr 1, 2023

Choose a reason for hiding this comment

msridhar Apr 2, 2023

Choose a reason for hiding this comment

lazaroclapp Apr 4, 2023 • edited

Choose a reason for hiding this comment

XN137 Apr 1, 2023

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

msridhar Apr 2, 2023

Choose a reason for hiding this comment

XN137 Apr 4, 2023

Choose a reason for hiding this comment

msridhar Apr 4, 2023

Choose a reason for hiding this comment

msridhar Apr 2, 2023

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

XN137 commented Mar 31, 2023 •

edited

lazaroclapp Apr 4, 2023 •

edited