Add support for static fields in contracts by armughan11 · Pull Request #1118 · uber/NullAway

armughan11 · 2024-12-26T08:07:28Z

This PR adds support for static fields in @EnsureNonnull,EnsureNonnullIf,@RequiresNonnull annotations. Currently the following code will throw validation errors (as well as the annotation handlers are unable to handle static fields). However, after this change, static fields for all three annotations are supported

class Foo {
  @Nullable static Item staticNullableItem;

  @EnsuresNonNull("staticNullableItem")
  public static void initializeStaticField() {
    staticNullableItem = new Item();
  }

  @RequiresNonNull("staticNullableItem")
  public static void useStaticField() {
    staticNullableItem.call();
  }

  @EnsuresNonNullIf(value="staticNullableItem", result=true)
  public static boolean hasStaticNullableItem() {
    return staticNullableItem != null;
  }
}

Fixes #431

codecov · 2024-12-26T08:23:13Z

Codecov Report

Attention: Patch coverage is 97.56098% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.26%. Comparing base (43054bb) to head (a776d48).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...in/java/com/uber/nullaway/dataflow/AccessPath.java	75.00%	0 Missing and 1 partial ⚠️
...ullaway/handlers/AbstractFieldContractHandler.java	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1118      +/-   ##
============================================
+ Coverage     88.17%   88.26%   +0.08%     
- Complexity     2238     2259      +21     
============================================
  Files            85       85              
  Lines          7239     7302      +63     
  Branches       1439     1455      +16     
============================================
+ Hits           6383     6445      +62     
  Misses          430      430              
- Partials        426      427       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

msridhar

Thanks a lot! I'm starting with a couple quick comments on the tests

msridhar

Looking good! A few more minor comments

msridhar · 2024-12-26T23:26:44Z

+    return getReceiverFields(Nullness.NONNULL, false);
+  }
+
+  public Set<Element> getNonNullStaticReceiverFields() {


Suggested change

public Set<Element> getNonNullStaticReceiverFields() {

public Set<Element> getNonNullStaticFields() {

No receiver for a static field

msridhar · 2024-12-26T23:30:18Z

   * @return Set of fields (represented as {@code Element}s) with the given {@code nullness}.
   */
-  public Set<Element> getReceiverFields(Nullness nullness) {
+  public Set<Element> getReceiverFields(Nullness nullness, boolean staticOnly) {


I feel the code reuse here via adding the boolean parameter is not worth it; the method is no longer about receiver fields so it gets confusing. Maybe just implement the logic in getNonNullStaticFields(), even though there will be a bit of code duplication.

msridhar · 2024-12-27T04:06:21Z

+              "Currently, @"
+                  + annotName
+                  + " does not support this. annotations for static fields. ";


The message can just be "Cannot reference to static field " + fieldName + " using this"

msridhar · 2024-12-27T04:07:38Z

            .stream()
            .map(e -> e.getSimpleName().toString())
            .collect(Collectors.toSet());
+    Set<String> nonnullStaticFieldsOfReceiverAtExit =


Suggested change

Set<String> nonnullStaticFieldsOfReceiverAtExit =

Set<String> nonnullStaticFieldsAtExit =

msridhar · 2024-12-27T04:08:28Z

+            .stream()
+            .map(e -> e.getSimpleName().toString())
+            .collect(Collectors.toSet());
+    nonnullFieldsOfReceiverAtExit.addAll(nonnullStaticFieldsOfReceiverAtExit);


Suggested change

nonnullFieldsOfReceiverAtExit.addAll(nonnullStaticFieldsOfReceiverAtExit);

nonnullFieldsAtExit.addAll(nonnullStaticFieldsOfReceiverAtExit);

They are not just on the receiver anymore

msridhar · 2024-12-27T04:10:10Z

+        AccessPath accessPath;
+        if (field.getModifiers().contains(Modifier.STATIC)) {
+          accessPath = AccessPath.fromStaticField(field);
+        } else {
+          accessPath =
+              AccessPath.fromBaseAndElement(node.getTarget().getReceiver(), field, apContext);
+        }


Might be simpler to just use a conditional expression, AccessPath accessPath = field.getModifiers().contains(Modifier.STATIC) ? ... : ...;

msridhar · 2024-12-27T04:10:24Z

+        AccessPath accessPath;
+
+        if (field.getModifiers().contains(Modifier.STATIC)) {
+          accessPath = AccessPath.fromStaticField(field);
+        } else {
+          accessPath = AccessPath.fromFieldElement(field);
+        }


Again, use a conditional expression

msridhar · 2024-12-27T04:11:12Z

+      AccessPath accessPath;
+      if (field.getModifiers().contains(Modifier.STATIC)) {
+        accessPath = AccessPath.fromStaticField(field);
+      } else {
+        accessPath = AccessPath.fromFieldElement(field);
+      }


conditional expression

msridhar

Almost there; a few more comments

msridhar · 2024-12-28T01:17:23Z

+        if (isStaticThisAnnotationField(classSymbol, fieldName)) {
+
+          message =
+              "Cannot reference to static field "


Suggested change

"Cannot reference to static field "

"Cannot refer to static field "

msridhar · 2024-12-28T01:19:21Z

    return null;
  }
+
+  public boolean isStaticThisAnnotationField(Symbol.ClassSymbol classSymbol, String fieldName) {


Renames:

Suggested change

public boolean isStaticThisAnnotationField(Symbol.ClassSymbol classSymbol, String fieldName) {

public boolean isThisDotStaticField(Symbol.ClassSymbol classSymbol, String expression) {

I suggest expression since fieldName might not be just a field name but could start with this..

Also can this method be private?

@msridhar did the name change but we can't have this as private since we are using it RequiresNonNullHandler to build the access path only if it is not this. annotation. Otherwise we end up honoring this. annotations through AP even though we raise a validation error (the issue you had raised in the first round of review).

Changed it to protected though.

msridhar · 2024-12-28T17:02:13Z

Thanks for the contribution!

armughan11 added 4 commits December 26, 2024 01:23

initial changes for requiresnonnull and ensuresnonnull

f5f9dc4

add ensures non null if changes

b33d17e

fix tests

833b303

fix tests

4bed14d

armughan11 changed the title ~~Add support for static field in contracts~~ Add support for static fields in contracts Dec 26, 2024

add validation for static field

2bdc944

msridhar reviewed Dec 26, 2024

View reviewed changes

Comment thread nullaway/src/test/java/com/uber/nullaway/EnsuresNonNullTests.java

Comment thread nullaway/src/test/java/com/uber/nullaway/EnsuresNonNullTests.java

Comment thread nullaway/src/test/java/com/uber/nullaway/EnsuresNonNullTests.java

armughan11 added 4 commits December 26, 2024 12:39

fix this. annotation for static fields

d940038

add invalid fields tests

eee9014

added combined tests

ea27598

added combined tests

8055cac

armughan11 requested a review from msridhar December 26, 2024 19:09

cleanup

3a94544

msridhar reviewed Dec 27, 2024

View reviewed changes

armughan11 added 2 commits December 27, 2024 06:12

address review comments

0027c25

fix error msg for static field this annotation

c9a01cf

armughan11 requested a review from msridhar December 27, 2024 12:17

msridhar reviewed Dec 28, 2024

View reviewed changes

address review comments

92d2f53

armughan11 requested a review from msridhar December 28, 2024 11:13

documentation tweaks

a776d48

msridhar approved these changes Dec 28, 2024

View reviewed changes

msridhar enabled auto-merge (squash) December 28, 2024 17:02

msridhar merged commit aaf9f08 into uber:master Dec 28, 2024

	public Set<Element> getNonNullStaticReceiverFields() {
	public Set<Element> getNonNullStaticFields() {

	Set<String> nonnullStaticFieldsOfReceiverAtExit =
	Set<String> nonnullStaticFieldsAtExit =

	nonnullFieldsOfReceiverAtExit.addAll(nonnullStaticFieldsOfReceiverAtExit);
	nonnullFieldsAtExit.addAll(nonnullStaticFieldsOfReceiverAtExit);

	"Cannot reference to static field "
	"Cannot refer to static field "

	public boolean isStaticThisAnnotationField(Symbol.ClassSymbol classSymbol, String fieldName) {
	public boolean isThisDotStaticField(Symbol.ClassSymbol classSymbol, String expression) {

Conversation

armughan11 commented Dec 26, 2024

Uh oh!

codecov Bot commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armughan11 Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msridhar commented Dec 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Dec 26, 2024 •

edited

Loading

armughan11 Dec 28, 2024 •

edited

Loading