Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use @Contract instead of @NullUnmarked #202

Open
ketkarameya opened this issue May 26, 2023 · 3 comments
Open

Use @Contract instead of @NullUnmarked #202

ketkarameya opened this issue May 26, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@ketkarameya
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
Sometimes it is quiet easy to figure out when @Contract("null -> null") should be added at the method declaration itself. Currently, autoannotator adds @Nullunmarked on the invocations.

Describe the solution you'd like
Add @Contract

cc: @lazaroclapp

@nimakarimipour nimakarimipour self-assigned this May 30, 2023
@nimakarimipour nimakarimipour added the enhancement New feature or request label May 30, 2023
@nimakarimipour
Copy link
Member

Hi @ketkarameya Thank you for reporting the requested feature. I will work on it and get back to you.

@msridhar
Copy link
Member

msridhar commented Jun 2, 2023

Thanks for the report @ketkarameya! We discussed this today and let me summarize the findings. We'd like to only infer @Contract in cases where we can give a guarantee that the inferred annotation is valid. Right now, @Contract verification is not enabled at Uber, and we also discovered the verification is not powerful enough to validate some common cases. Basically, we'd need to enhance the NullAway dataflow analysis to reason about dead code. Consider the following case (adapted from an example by @lazaroclapp):

  @Nullable
  @Contract("_, !null -> !null")
  static Bar bar(final String a, @Nullable final String b) {
    if (b == null) {
      return null;
    }
    return new Bar(a, b);
  }

Currently, NullAway fails to verify this @Contract annotation and reports it may be invalid (when contract checking is enabled). To verify the contract, the analysis has to reason that when b != null the return null statement is unreachable. Currently, NullAway does not do any kind of reasoning about (conditional) code reachability.

So, we are tabling support for @Contract inference until we can enhance NullAway's ability to verify @Contract annotations for these kinds of cases.

@ketkarameya
Copy link
Collaborator Author

Thanks for investigating into this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants