refactor to directly return mayBeNullFieldAccess

[XN137]

the method already calls onOverrideMayBeNullExpr and nullnessFromDataflow so there is no point to break out of the switch to call the former again

see also other usage of mayBeNullFieldAccess a few lines below

also remove comments that just restated the obvious

[XN137]

note how this isnt a special case, because the else branch does the same thing

the only difference is the initial value of the exprMayBeNull parameter

[msridhar]

It's been a long time since I looked at this code! I think the intended structure is as follows for the common case:

• The switch statement sets the exprMayBeNull variable, mostly according to the kind of expression and the expression's nullability type qualifier.
• After the switch, handler.onOverrideMayBeNullExpr is run to allow for overriding from handlers.
• Finally, if we have concluded that the expression may be null based on the above, we run dataflow analysis to possibly override this conclusion based on some null check in the code.

For local variables, we always infer their nullability via dataflow analysis. Right now, that code is written as an "early return" out of the switch. But maybe it would be clearer if we just did (on lines 2274-2275):

// local variable's type must always be inferred using dataflow
exprMayBeNull = true;
break;

For method calls and field accesses, I think we could do something similar. We could just avoid running the handler and dataflow in mayBeNullMethodCall and mayBeNullFieldAccess, just setting exprMayBeNull appropriately but getting rid of the handler and dataflow calls, leaving those to one place, after the switch statement. This would get rid of all the early returns out of the switch and some duplicated code.

What do you think?

[XN137]

After the switch, handler.onOverrideMayBeNullExpr is run to allow for overriding from handlers.

Finally, if we have concluded that the expression may be null based on the above, we run dataflow analysis to possibly override this conclusion based on some null check in the code.

i might be missing something but this doesnt seem to be true.
when we break from the switch, dataflow analysis is NOT run...
only some expression types run it inside the switch branches.

taking a step back:
this PR focuses on a very narrow cleanup of redundant calls/obsolete comments
if you see the potential for more cleanups/unifying we should do those in a followup imo

[XN137]

also note that some switch branches seem to run nullnessFromDataflow before handler.onOverrideMayBeNullExpr while others do it the other way round.

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

Lines 2292 to 2293 in 7714c45

	// Special case: mayBeNullMethodCall runs handler.onOverrideMayBeNullExpr before dataflow.
	return mayBeNullMethodCall(state, expr, (Symbol.MethodSymbol) exprSymbol);

local testing suggests that order may not matter (tests seem to pass in both cases)

[XN137]

if i understood you correctly this was your suggestion:
#753

however see the open questions there

[XN137]

obsolete after the other PR got merged