NULL_LITERAL expressions may always be null
#749
Conversation
| } | ||
| Symbol exprSymbol = ASTHelpers.getSymbol(expr); | ||
| boolean exprMayBeNull; | ||
| switch (expr.getKind()) { | ||
| case ARRAY_ACCESS: | ||
| // unsound! we cannot check for nullness of array contents yet | ||
| exprMayBeNull = false; |
There was a problem hiding this comment.
imo this is another candidate to be pulled into the 1st switch to return false immediately?
i might be missing something but what value is there in passing this through the handlers chain when none of them deal with ARRAY_ACCESS expressions?
this can then of course be revisited once there is a dedicated handler for this.
same imo for the expression types in the next block that are // clearly not null
There was a problem hiding this comment.
For array accesses I'd rather not bail out for now. For array accesses we are thinking about doing better handling according to JSpecify in the medium term, and it's possible a handler may want to override what happens there.
For the // clearly not null cases I'm not sure. @lazaroclapp do you think it's worth bailing out early to avoid running the handlers for these cases, where basically according to the language spec the expression cannot be null?
There was a problem hiding this comment.
imo as long as no handler cares for these expression it is best to return early, because this avoids redundant work (calling the handler chain for no reason).
see also suggestion in #753
if in the near future a handler for array access will be added, it is trivial to move that expression back to the 2nd switch ...
if proper tests get added this required move will become obvious.
| switch (expr.getKind()) { | ||
| case NULL_LITERAL: | ||
| // obviously null | ||
| exprMayBeNull = true; | ||
| return true; |
There was a problem hiding this comment.
doing this i think was the reason why i saw a small but noticeable difference in the NullawayRelease JMH benchmark, YMMV
There was a problem hiding this comment.
Yes, this is a good change I think. I cannot imagine a handler wanting to override the nullness of null 🙂 Let's just add a comment stating we don't allow handlers to override, so we return early as a performance optimization?
There was a problem hiding this comment.
ok comment adjusted...
calling it a perf optimization felt redundant because we also dont do that for the other literals above.
WDYT ?
XN137
force-pushed
the
handlers-never-override-NULL_LITERAL
branch
from
c0a0449
to
3273051
Compare
|
Assuming we're going to do something like #753 is this change still needed? Or should it be folded in? |
XN137
force-pushed
the
handlers-never-override-NULL_LITERAL
branch
from
3273051
to
2b38ee8
Compare
|
@msridhar it is already folded into the other PR, but since the scope there is broader and the review more involved, we can maybe get this merged more quickly? |
| @@ -2197,14 +2197,17 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) { | |||
| // obviously not null | |||
| return false; | |||
| } | |||
| switch (expr.getKind()) { | |||
There was a problem hiding this comment.
Can we make this an if rather than a switch?
There was a problem hiding this comment.
i made it a switch in anticipation of more expression types returning early, but ok changed to if
XN137
force-pushed
the
handlers-never-override-NULL_LITERAL
branch
2 times, most recently
from
027a80f
to
8b003b3
Compare
|
Whoops looks like we need some kind of import now? Could you fix? |
there is no reason to give handlers a chance to override this because we are also not doing this for other literals that we filter out at the start of the method
XN137
force-pushed
the
handlers-never-override-NULL_LITERAL
branch
from
8b003b3
to
9595546
Compare
This reverts commit e4dfeac.
This reverts commit e4dfeac.
This reverts commit e4dfeac.
This reverts commit e4dfeac.
This reverts commit e4dfeac.
there is no reason to give handlers a chance to override this because we are also not doing this for other literals that we filter out at the start of the method