Downcasting (CastToNonNullMethod) does NOT propagate to the other occurrences of the same expression

[stechio]

I noticed a discrepancy in the way NullAway handles java.util.Objects.requireNonNull versus CastToNonNullMethod:

• once an expression is downcast to non-null via java.util.Objects.requireNonNull, NullAway applies its non-null status also to the other occurrences of the same expression downstream; for example, the following code fragment compiles successfully:

  if (java.util.Objects.requireNonNull(value.getToken()).contains("abc")) {
    int len = value.getToken().length();
    . . .
  }

• on the contrary, downcasting the same expression with a custom method specified via -XepOpt:NullAway:CastToNonNullMethod option, NullAway does NOT apply its non-null status to the other occurrences of the same expression downstream, and emits false-positive warnings:

  if (my.package.MyUtils.castToNonNull(value.getToken()).contains("abc")) {
    int len = value.getToken().length(); // <-- FAILS HERE: "[NullAway] passing @Nullable parameter 'value.getToken()' where @NonNull is required"
    . . .
  }

  NOTE: I'm sure that NullAway is treating my.package.MyUtils.castToNonNull as a properly-specified CastToNonNullMethod, since it emits warnings if my.package.MyUtils.castToNonNull is invoked with a @NonNull argument:

  [NullAway] passing known @NonNull parameter 'value' to CastToNonNullMethod (my.package.MyUtils.castToNonNull) at position 0. This method argument should only take values that NullAway considers @Nullable at the invocation site, but which are known not to be null at runtime.
  

EXPECTED BEHAVIOR

NullAway should handle CastToNonNullMethod the same way as java.util.Objects.requireNonNull, applying the non-null status to the other occurrences of the same expression downstream.

[Shankar-v27]

Hi! I investigated this issue, reproduced it with a regression test, implemented a fix, and I'll be opening a PR shortly.

[msridhar]

Hi @stechio , this is a deliberate design decision, though we could possibly make it configurable. With requireNonNull, we know the call will fail when passed a null argument. With an arbitrary castToNonNull method, we do not know this. (At Uber, the casting method emits an error log when passed a null argument but does not fail. This avoids an unnecessary crash when the surrounding code happens to be robust to null values.)

Would it be very helpful to you to add a NullAway setting indicating that the cast method fails with a null argument? Alternately, you could store the result of the cast operation in a local variable and reuse it.

[Shankar-v27]

Thanks for the clarification! That makes sense I hadn't considered that CastToNonNullMethod is intentionally allowed to return even when passed null, whereas Objects.requireNonNull is guaranteed to fail fast.

I agree that unconditional propagation would be unsound in that case.

I like the idea of making this configurable for projects whose cast method is fail-fast. If that's a direction you'd be interested in, I'd be happy to work on adding a configuration option for it.

[Shankar-v27]

I have made a new commit .
please look into it.