Consider supporting simple null equality checks in boolean-based Contract annotations

[carterkozak]

Firstly, NullAway is fantastic and I can't thank you enough for building it!

I've adopted @Contract annotations in a few places with great success on methods along the lines of MyPreconditions.checkNotNull(value) (contract null -> fail), however I'd love to see support for boolean parameters when the provided argument is a simple null check e.g. MyPreconditions.checkArgument(obj != null) (contract false -> fail).

In practice, the current limitation means that the following code fails as described inline:

class Test {
  @Contract("false -> fail")
  static void checkState(boolean value) {
      if (!value) {
        throw new IllegalStateException();
      }
  }
  
  String test(@Nullable Object input) {
    checkState(input != null);
    // NullAway doesn't take the check into account, describing a dereferenced expression
    return input.toString();
  }
}

Would you accept a contribution to handle such cases?

Concretely, my proposed solution would be to support boolean values in contracts only when the arg was provided as arg == null, null == arg, arg != null, or null != arg, thus the contract could be understood as a null check. I think this is a reasonable middle-ground without adding boolean-logic-dataflow support for cases where the argument itself is stored elsewhere e.g.

void test(@Nullable input) {
  boolean value = input == null;
  checkState(value); // I am not suggesting that such indirection be supported.
}

Thanks again for your work, any feedback is appreciated :-)

[msridhar]

This is an interesting idea! And it seems related to #666, which asks for support of Guava's Verify.verify(...). In #608 we added built-in support for Guava's Preconditions.checkArgument(...) and checkState(...). That approach just handles whatever boolean condition expression is passed directly to those methods, but not dataflow through locals, like you suggest (it works by rewriting the control-flow graph to expose the condition and exception-throwing behavior to NullAway). The current code has hardcoded matching for the Preconditions methods, but it seems straightforward to extend it to work for Verify.verify(...). We could use the same approach to handle calls to any method annotated @Contract("false -> fail"). Maybe this is what you were thinking in terms of implementation?

The only thing is that I'd probably want @lazaroclapp to review a change like this one before we land it (both at a high level and also the code in the PR), and he is on holiday for the next couple of weeks. I am optimistic we would accept a change like this if you want to go ahead and work on the PR, but we will have to wait for his review before it lands.

Let me know what you think, and I can give tips / feedback on implementation strategy.

[carterkozak]

Thank you for the information, I really appreciate it!

The approach used for Guava's Preconditions in #608 is beautifully elegant, however I think there are a few key differences that we may want to consider:

• In the preconditions example, we have a good idea what kind of exception may be thrown by Preconditions methods (although the handler always provides an IllegalArgumentException despite checkState throwing IllegalStateException, I think this is fine because the node is added after a method call, but it may be helpful to provide the additional specificity here: pr PreconditionsHandler reflects Guava Preconditions exception types #668)
• Preconditions boolean methods are void, while contracts may be used on methods which also return. I'm not entirely clear on how the inserted node interacts with other code when more complex structures are involved (mid-expression, for example), so this is an area I'll have to investigate further.
• Contract methods may be used with boolean-in and boolean-out e.g. @Contract("true -> true; false -> false"). I suspect this is rare, but we could support something along these lines depending on the approach:

  @Contract("true -> true; false -> false")
  static boolean identity(boolean value) {
    return value;
  }
  String test(@Nullable Object o1) {
    if (identity(o1 != null)) {
      // o1 cannot be null here
      return o1.toString();
    }
    return "null";
  }

[carterkozak]

Upon investigation, the second case I was worried about seems to work just fine: non-void methods aren't impacted by inserting a conditional throw.

I've created PR #669. I'd really appreciate if you (or anyone else) would like to provide feedback, but no rush :-)

(disclaimer: I'm not attached to the code, that's just how I prefer to quickly build an understanding of a codebase -- I won't be offended if folks want to throw it out and try something entirely different!)

[msridhar]

Closing since #669 was merged