Clean up Fix class and remove reason property from class decl (#247)

This PR is a cleanup that is required before the upcoming major PR that adds support for taint checker inference. Changes: Removed the reason property from the Fix class. Previously, this field stored the set of errors associated with each fix. However, this information can be derived directly from the set of errors itself. Refactored the code to remove redundant storage, ensuring a cleaner and more efficient design. Rationale: The reason property is redundant as the same information is accessible through the error set. This refactoring simplifies the codebase and eliminates unnecessary complexity, making future updates, including the taint checker inference integration, easier to manage.
ucr-riple · Oct 4, 2024 · a60d7d2 · a60d7d2
1 parent c493546
commit a60d7d2
Show file tree

Hide file tree

Showing 8 changed files with 23 additions and 59 deletions.
diff --git a/...-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpactCacheImpl.java b/...-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpactCacheImpl.java
@@ -122,8 +122,7 @@ public void analyzeDownstreamDependencies() {
             .stream()
             .map(
                 location ->
-                    new Fix(
-                        new AddMarkerAnnotation(location, context.config.nullableAnnot), "null"))
+                    new Fix(new AddMarkerAnnotation(location, context.config.nullableAnnot)))
             .collect(ImmutableSet.toImmutableSet());
     DownstreamImpactEvaluator evaluator = new DownstreamImpactEvaluator(supplier);
     ImmutableSet<Report> reports = evaluator.evaluate(fixes);

diff --git a/annotator-core/src/main/java/edu/ucr/cs/riple/core/checkers/nullaway/NullAway.java b/annotator-core/src/main/java/edu/ucr/cs/riple/core/checkers/nullaway/NullAway.java
@@ -130,7 +130,7 @@ private NullAwayError deserializeErrorFromTSVLine(ModuleInfo moduleInfo, String
     Fix resolvingFix =
         nonnullTarget == null
             ? null
-            : new Fix(new AddMarkerAnnotation(nonnullTarget, config.nullableAnnot), errorType);
+            : new Fix(new AddMarkerAnnotation(nonnullTarget, config.nullableAnnot));
     return createError(
         errorType,
         errorMessage,
@@ -199,8 +199,7 @@ protected ImmutableSet<Fix> generateFixesForUninitializedFields(
               }
               return new Fix(
                   new AddMarkerAnnotation(
-                      extendVariableList(locationOnField, module), config.nullableAnnot),
-                  NullAwayError.METHOD_INITIALIZER_ERROR);
+                      extendVariableList(locationOnField, module), config.nullableAnnot));
             })
         .filter(Objects::nonNull)
         .collect(ImmutableSet.toImmutableSet());
@@ -316,12 +315,12 @@ public void suppressRemainingErrors(AnnotationInjector injector) {
 
     // Collect NullAway.Init SuppressWarnings
     Set<AddAnnotation> initializationSuppressWarningsAnnotations =
-        remainingFixes.stream()
+        remainingErrors.stream()
             .filter(
-                fix ->
-                    fix.isOnField()
-                        && (fix.reasons.contains("METHOD_NO_INIT")
-                            || fix.reasons.contains("FIELD_NO_INIT")))
+                e ->
+                    e.messageType.equals("METHOD_NO_INIT") || e.messageType.equals("FIELD_NO_INIT"))
+            .flatMap(e -> e.getResolvingFixes().stream())
+            .filter(Fix::isOnField)
             // Filter nodes annotated with SuppressWarnings("NullAway")
             .filter(fix -> !fieldsWithSuppressWarnings.contains(fix.toField()))
             .map(
@@ -357,8 +356,12 @@ public void preprocess(AnnotationInjector injector) {
     // nonnull at all exit paths.
     // Collect uninitialized fields.
     Set<OnField> uninitializedFields =
-        Utility.readFixesFromOutputDirectory(context, context.targetModuleInfo).stream()
-            .filter(fix -> fix.isOnField() && fix.reasons.contains("FIELD_NO_INIT"))
+        Utility.readErrorsFromOutputDirectory(
+                context, context.targetModuleInfo, NullAwayError.class)
+            .stream()
+            .filter(e -> e.messageType.equals("FIELD_NO_INIT"))
+            .flatMap(e -> e.getResolvingFixes().stream())
+            .filter(Fix::isOnField)
             .map(Fix::toField)
             .collect(Collectors.toSet());
     FieldInitializationStore fieldInitializationStore = new FieldInitializationStore(context);

diff --git a/...ava/edu/ucr/cs/riple/core/evaluators/graph/processors/AbstractConflictGraphProcessor.java b/...ava/edu/ucr/cs/riple/core/evaluators/graph/processors/AbstractConflictGraphProcessor.java
@@ -80,8 +80,7 @@ protected Set<Fix> getTriggeredFixesFromDownstreamErrors(Node node) {
             error ->
                 new Fix(
                     new AddMarkerAnnotation(
-                        error.toResolvingLocation(), context.config.nullableAnnot),
-                    "PASSING_NULLABLE"))
+                        error.toResolvingLocation(), context.config.nullableAnnot)))
         .collect(Collectors.toSet());
   }
 }
diff --git a/annotator-core/src/main/java/edu/ucr/cs/riple/core/registries/index/Error.java b/annotator-core/src/main/java/edu/ucr/cs/riple/core/registries/index/Error.java
@@ -30,11 +30,7 @@
 import edu.ucr.cs.riple.injector.location.Location;
 import edu.ucr.cs.riple.injector.location.OnParameter;
 import java.util.Collection;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
 import java.util.Objects;
-import java.util.Set;
 import javax.annotation.Nullable;
 
 /** Represents an error reported by NullAway. */
@@ -201,31 +197,9 @@ public String toString() {
    */
   public static <T extends Error> ImmutableSet<Fix> getResolvingFixesOfErrors(
       Collection<T> errors) {
-    // Each error has a set of resolving fixes and each fix has a set of reasons as why the fix has
-    // been suggested. The final returned set of fixes should contain all the reasons it has been
-    // suggested across the given collection. Map below stores all the set of reasons each fix is
-    // suggested in the given collection.
-
-    // Collect all reasons each fix is suggested across the given collection.
-    Map<Fix, Set<String>> fixReasonsMap = new HashMap<>();
-    errors.stream()
-        .flatMap(error -> error.getResolvingFixes().stream())
-        .forEach(
-            fix -> {
-              if (fixReasonsMap.containsKey(fix)) {
-                fixReasonsMap.get(fix).addAll(fix.reasons);
-              } else {
-                fixReasonsMap.put(fix, new HashSet<>(fix.reasons));
-              }
-            });
-
-    ImmutableSet.Builder<Fix> builder = ImmutableSet.builder();
-    for (Fix key : fixReasonsMap.keySet()) {
-      // To avoid mutating fixes stored in the given collection, we create new instances.
-      // which contain the full set of reasons.
-      builder.add(new Fix(key.change, ImmutableSet.copyOf(fixReasonsMap.get(key))));
-    }
-    return builder.build();
+    return errors.stream()
+        .flatMap(t -> t.resolvingFixes.stream())
+        .collect(ImmutableSet.toImmutableSet());
   }
 
   /**

diff --git a/annotator-core/src/main/java/edu/ucr/cs/riple/core/registries/index/Fix.java b/annotator-core/src/main/java/edu/ucr/cs/riple/core/registries/index/Fix.java
@@ -24,7 +24,6 @@
 
 package edu.ucr.cs.riple.core.registries.index;
 
-import com.google.common.collect.ImmutableSet;
 import edu.ucr.cs.riple.injector.Helper;
 import edu.ucr.cs.riple.injector.changes.AddAnnotation;
 import edu.ucr.cs.riple.injector.location.Location;
@@ -45,16 +44,9 @@ public class Fix {
 
   /** Suggested change. */
   public final AddAnnotation change;
-  /** Reasons this fix is suggested by NullAway in string. */
-  public final ImmutableSet<String> reasons;
 
-  public Fix(AddAnnotation change, String reason) {
-    this(change, ImmutableSet.of(reason));
-  }
-
-  public Fix(AddAnnotation change, ImmutableSet<String> reasons) {
+  public Fix(AddAnnotation change) {
     this.change = change;
-    this.reasons = reasons;
   }
 
   /**

diff --git a/...re/src/main/java/edu/ucr/cs/riple/core/registries/region/generatedcode/LombokHandler.java b/...re/src/main/java/edu/ucr/cs/riple/core/registries/region/generatedcode/LombokHandler.java
@@ -119,8 +119,8 @@ public ImmutableSet<Fix> extendForGeneratedFixes(Set<Fix> fixes) {
                             builder.add(
                                 new Fix(
                                     new AddMarkerAnnotation(
-                                        getterMethod.location, change.getAnnotationName().fullName),
-                                    fix.reasons));
+                                        getterMethod.location,
+                                        change.getAnnotationName().fullName)));
                           }
                         })));
     return builder.build();

diff --git a/annotator-core/src/test/java/edu/ucr/cs/riple/core/tools/TFix.java b/annotator-core/src/test/java/edu/ucr/cs/riple/core/tools/TFix.java
@@ -24,7 +24,6 @@
 
 package edu.ucr.cs.riple.core.tools;
 
-import com.google.common.collect.ImmutableSet;
 import edu.ucr.cs.riple.core.registries.index.Fix;
 import edu.ucr.cs.riple.injector.location.Location;
 
@@ -36,6 +35,6 @@
 public class TFix extends Fix {
 
   public TFix(Location location) {
-    super(new DefaultAnnotation(location), ImmutableSet.of());
+    super(new DefaultAnnotation(location));
   }
 }
diff --git a/annotator-core/src/test/java/edu/ucr/cs/riple/core/tools/TReport.java b/annotator-core/src/test/java/edu/ucr/cs/riple/core/tools/TReport.java
@@ -49,9 +49,7 @@ public TReport(Location root, int effect) {
   }
 
   public TReport(Location root, int effect, Tag tag) {
-    super(
-        new Fix(new AddMarkerAnnotation(root, "javax.annotation.Nullable"), ImmutableSet.of()),
-        effect);
+    super(new Fix(new AddMarkerAnnotation(root, "javax.annotation.Nullable")), effect);
     this.expectedValue = effect;
     if (tag != null) {
       this.tag(tag);