address review comments

uber · Dec 27, 2024 · 0027c25 · 0027c25
1 parent 3a94544
commit 0027c25
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 33 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java
@@ -270,11 +270,20 @@ public NullnessStore filterAccessPaths(Predicate<AccessPath> pred) {
    * @return Set of fields (represented as {@code Element}s) that are non-null
    */
   public Set<Element> getNonNullReceiverFields() {
-    return getReceiverFields(Nullness.NONNULL, false);
+    return getReceiverFields(Nullness.NONNULL);
   }
 
-  public Set<Element> getNonNullStaticReceiverFields() {
-    return getReceiverFields(Nullness.NONNULL, true);
+  public Set<Element> getNonNullStaticFields() {
+    Set<AccessPath> nonnullAccessPaths = this.getAccessPathsWithValue(Nullness.NONNULL);
+    Set<Element> result = new LinkedHashSet<>();
+    for (AccessPath ap : nonnullAccessPaths) {
+      if (ap.getRoot() != null) {
+        if (ap.getRoot().getModifiers().contains(Modifier.STATIC)) {
+          result.add(ap.getRoot());
+        }
+      }
+    }
+    return result;
   }
 
   /**
@@ -283,7 +292,7 @@ public Set<Element> getNonNullStaticReceiverFields() {
    * @param nullness The {@code Nullness} state
    * @return Set of fields (represented as {@code Element}s) with the given {@code nullness}.
    */
-  public Set<Element> getReceiverFields(Nullness nullness, boolean staticOnly) {
+  public Set<Element> getReceiverFields(Nullness nullness) {
     Set<AccessPath> nonnullAccessPaths = this.getAccessPathsWithValue(nullness);
     Set<Element> result = new LinkedHashSet<>();
     for (AccessPath ap : nonnullAccessPaths) {
@@ -296,10 +305,6 @@ public Set<Element> getReceiverFields(Nullness nullness, boolean staticOnly) {
             result.add(elem);
           }
         }
-      } else {
-        if (staticOnly && ap.getRoot().getModifiers().contains(Modifier.STATIC)) {
-          result.add(ap.getRoot());
-        }
       }
     }
     return result;

diff --git a/...rc/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullHandler.java b/...rc/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullHandler.java
@@ -40,6 +40,7 @@
 import com.uber.nullaway.handlers.MethodAnalysisContext;
 import com.uber.nullaway.handlers.contract.ContractUtils;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Set;
 import java.util.stream.Collectors;
 import javax.lang.model.element.Modifier;
@@ -86,24 +87,26 @@ protected boolean validateAnnotationSemantics(
             .stream()
             .map(e -> e.getSimpleName().toString())
             .collect(Collectors.toSet());
-    Set<String> nonnullStaticFieldsOfReceiverAtExit =
+    Set<String> nonnullStaticFieldsAtExit =
         analysis
             .getNullnessAnalysis(state)
             .getNonnullStaticFieldsAtExit(new TreePath(state.getPath(), tree), state.context)
             .stream()
             .map(e -> e.getSimpleName().toString())
             .collect(Collectors.toSet());
-    nonnullFieldsOfReceiverAtExit.addAll(nonnullStaticFieldsOfReceiverAtExit);
+    Set<String> nonnullFieldsAtExit = new HashSet<String>();
+    nonnullFieldsAtExit.addAll(nonnullFieldsOfReceiverAtExit);
+    nonnullFieldsAtExit.addAll(nonnullStaticFieldsAtExit);
     Set<String> fieldNames = getAnnotationValueArray(methodSymbol, annotName, false);
     if (fieldNames == null) {
       fieldNames = Collections.emptySet();
     }
     fieldNames = ContractUtils.trimReceivers(fieldNames);
 
-    nonnullFieldsOfReceiverAtExit = ContractUtils.trimReceivers(nonnullFieldsOfReceiverAtExit);
-    boolean isValidLocalPostCondition = nonnullFieldsOfReceiverAtExit.containsAll(fieldNames);
+    nonnullFieldsAtExit = ContractUtils.trimReceivers(nonnullFieldsAtExit);
+    boolean isValidLocalPostCondition = nonnullFieldsAtExit.containsAll(fieldNames);
     if (!isValidLocalPostCondition) {
-      fieldNames.removeAll(nonnullFieldsOfReceiverAtExit);
+      fieldNames.removeAll(nonnullFieldsAtExit);
       message =
           String.format(
               "Method is annotated with @EnsuresNonNull but fails to ensure the following fields are non-null at exit: %s",
@@ -169,13 +172,10 @@ public NullnessHint onDataflowVisitMethodInvocation(
           // Invalid annotation, will result in an error during validation. For now, skip field.
           continue;
         }
-        AccessPath accessPath;
-        if (field.getModifiers().contains(Modifier.STATIC)) {
-          accessPath = AccessPath.fromStaticField(field);
-        } else {
-          accessPath =
-              AccessPath.fromBaseAndElement(node.getTarget().getReceiver(), field, apContext);
-        }
+        AccessPath accessPath =
+            field.getModifiers().contains(Modifier.STATIC)
+                ? AccessPath.fromStaticField(field)
+                : AccessPath.fromBaseAndElement(node.getTarget().getReceiver(), field, apContext);
         if (accessPath == null) {
           continue;
         }

diff --git a/.../main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullIfHandler.java b/.../main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullIfHandler.java
@@ -186,7 +186,7 @@ public void onDataflowVisitReturn(
             .map(e -> e.getSimpleName().toString())
             .collect(Collectors.toSet());
     Set<String> nonNullStaticFieldsInPath =
-        chosenStore.getNonNullStaticReceiverFields().stream()
+        chosenStore.getNonNullStaticFields().stream()
             .map(e -> e.getSimpleName().toString())
             .collect(Collectors.toSet());
     nonNullFieldsInPath.addAll(nonNullStaticFieldsInPath);
@@ -308,13 +308,11 @@ public NullnessHint onDataflowVisitMethodInvocation(
           // Invalid annotation, will result in an error during validation.
           continue;
         }
-        AccessPath accessPath;
+        AccessPath accessPath =
+            field.getModifiers().contains(Modifier.STATIC)
+                ? AccessPath.fromStaticField(field)
+                : AccessPath.fromFieldElement(field);
 
-        if (field.getModifiers().contains(Modifier.STATIC)) {
-          accessPath = AccessPath.fromStaticField(field);
-        } else {
-          accessPath = AccessPath.fromFieldElement(field);
-        }
         if (accessPath == null) {
           // Also likely to be an invalid annotation, will result in an error during validation.
           continue;

diff --git a/...c/main/java/com/uber/nullaway/handlers/contract/fieldcontract/RequiresNonNullHandler.java b/...c/main/java/com/uber/nullaway/handlers/contract/fieldcontract/RequiresNonNullHandler.java
@@ -235,12 +235,10 @@ public NullnessStore.Builder onDataflowInitialStore(
         // Invalid annotation, will result in an error during validation. For now, skip field.
         continue;
       }
-      AccessPath accessPath;
-      if (field.getModifiers().contains(Modifier.STATIC)) {
-        accessPath = AccessPath.fromStaticField(field);
-      } else {
-        accessPath = AccessPath.fromFieldElement(field);
-      }
+      AccessPath accessPath =
+          field.getModifiers().contains(Modifier.STATIC)
+              ? AccessPath.fromStaticField(field)
+              : AccessPath.fromFieldElement(field);
       result.setInformation(accessPath, Nullness.NONNULL);
     }
     return result;