From f0574a15d3fe1a18a89214718192c84dea19a725 Mon Sep 17 00:00:00 2001 From: Nima Karimipour Date: Thu, 2 Nov 2023 10:54:11 -0700 Subject: [PATCH] Index flow of nullable back to upstream through a field write (#211) This PR is built on (~#210~ landed) Resolves #209 by indexing flow of nullable back to upstream through field writes. Previously we only indexed flow of nullable back to upstream via parameter passing, e.g. code below: ```java class Target{ Object getFoo() void bar(Object param); } class Dependency { Target t; void baz(){ t.bar(t.getFoo()); } } ``` We know that making `getFoo()` will trigger a fix on `bar()` parameter. This PR updates Annotator to index all flows back to upstream including field writes. Hence, in the code below: ```java class Target{ Object bar; Object getFoo(); } class Dependency { Target t; void baz(){ t.bar = t.getFoo(); } } ``` Annotator now indexes the flow of nullable back to target through write to a field and includes the corresponding fix in the processing fix tree. Please note this PR only updates indexing flow of nullable back to upstream. Annotator still does **not** index the impact of making public fields `@Nullable` on downstream dependencies. --- .../cache/downstream/DownstreamImpact.java | 65 +------------------ .../downstream/DownstreamImpactCacheImpl.java | 14 ++-- .../downstream/DownstreamImpactEvaluator.java | 51 ++++----------- .../AbstractConflictGraphProcessor.java | 3 +- ...t.java => NullableFlowToUpstreamTest.java} | 48 +++++++++++++- .../expected/Dep/src/main/java/test/Dep.java | 8 +++ .../Target/src/main/java/test/Bar.java | 9 +++ 7 files changed, 81 insertions(+), 117 deletions(-) rename annotator-core/src/test/java/edu/ucr/cs/riple/core/{ArgumentNullableFlowTest.java => NullableFlowToUpstreamTest.java} (74%) create mode 100644 annotator-core/src/test/resources/nullableflowfieldwrite/expected/Dep/src/main/java/test/Dep.java create mode 100644 annotator-core/src/test/resources/nullableflowfieldwrite/expected/Target/src/main/java/test/Bar.java diff --git a/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpact.java b/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpact.java index 9f3ea8ecb..4fd5b6434 100644 --- a/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpact.java +++ b/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpact.java @@ -26,19 +26,11 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; import edu.ucr.cs.riple.core.Report; import edu.ucr.cs.riple.core.cache.Impact; -import edu.ucr.cs.riple.core.metadata.index.Error; import edu.ucr.cs.riple.core.metadata.index.Fix; import edu.ucr.cs.riple.core.metadata.method.MethodRecord; import edu.ucr.cs.riple.injector.location.OnMethod; -import edu.ucr.cs.riple.injector.location.OnParameter; -import java.util.Collection; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Set; -import java.util.stream.Collectors; /** * Container class for storing overall impact of a fix applied in target module on downstream @@ -46,19 +38,12 @@ */ public class DownstreamImpact extends Impact { - /** - * Map of parameters in target module that will receive {@code Nullable} value if targeted method - * in node is annotated as {@code @Nullable} with their corresponding triggered errors. - */ - private final HashMap> impactedParametersMap; - public DownstreamImpact(Fix fix) { super(fix); // Only store impacts of fixes targeting methods. Preconditions.checkArgument( fix.isOnMethod(), "Unexpected Fix instance. Only impacts of fixes on methods should be tracked for downstream dependencies"); - this.impactedParametersMap = new HashMap<>(); this.triggeredErrors = ImmutableSet.of(); } @@ -72,21 +57,9 @@ public int hashCode() { * * @param report Result of applying making method in node {@code @Nullable} in downstream * dependencies. - * @param impactedParameters Set of impacted paramaters. */ - public void setStatus(Report report, Set impactedParameters) { + public void setStatus(Report report) { this.triggeredErrors = ImmutableSet.copyOf(report.triggeredErrors); - // Count the number of times each parameter received a @Nullable. - impactedParameters.forEach( - onParameter -> { - Set triggered = - triggeredErrors.stream() - .filter( - error -> - error.isSingleFix() && error.toResolvingLocation().equals(onParameter)) - .collect(Collectors.toSet()); - impactedParametersMap.put(onParameter, triggered); - }); } /** @@ -101,42 +74,6 @@ public static int hash(String method, String clazz) { return MethodRecord.hash(method, clazz); } - /** - * Returns set of triggered errors if method is {@code @Nullable} on downstream dependencies. - * - * @return Set of errors. - */ - @Override - public ImmutableSet getTriggeredErrors() { - return triggeredErrors; - } - - /** - * Updates the status of method's impact after injection of fixes in target module. Potentially - * part of stored impact result is invalid due to injection of fixes. (e.g. some impacted - * parameters may already be annotated as {@code @Nullable} and will no longer trigger errors on - * downstream dependencies). This method addresses this issue by updating method's status. - * - * @param fixes List of injected fixes. - */ - @Override - public void updateStatusAfterInjection(Collection fixes) { - Set annotatedParameters = new HashSet<>(); - Set temp = Sets.newHashSet(triggeredErrors); - fixes.forEach( - fix -> - fix.ifOnParameter( - onParameter -> { - if (impactedParametersMap.containsKey(onParameter)) { - Set errors = impactedParametersMap.get(onParameter); - temp.removeAll(errors); - annotatedParameters.add(onParameter); - } - })); - this.triggeredErrors = ImmutableSet.copyOf(temp); - annotatedParameters.forEach(impactedParametersMap::remove); - } - /** * Gets the containing method location. * diff --git a/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpactCacheImpl.java b/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpactCacheImpl.java index d0b9bbd31..7422bf1d3 100644 --- a/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpactCacheImpl.java +++ b/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpactCacheImpl.java @@ -38,7 +38,6 @@ import edu.ucr.cs.riple.core.metadata.region.MethodRegionRegistry; import edu.ucr.cs.riple.injector.changes.AddMarkerAnnotation; import edu.ucr.cs.riple.injector.location.Location; -import edu.ucr.cs.riple.injector.location.OnParameter; import java.util.Collection; import java.util.OptionalInt; import java.util.Set; @@ -112,14 +111,11 @@ public void analyzeDownstreamDependencies() { this.store .values() .forEach( - method -> { - Set impactedParameters = - evaluator.getImpactedParameters(method.toMethod()); - reports.stream() - .filter(input -> input.root.toMethod().equals(method.toMethod())) - .findAny() - .ifPresent(report -> method.setStatus(report, impactedParameters)); - }); + methodImpact -> + reports.stream() + .filter(input -> input.root.toMethod().equals(methodImpact.toMethod())) + .findAny() + .ifPresent(methodImpact::setStatus)); System.out.println("Analyzing downstream dependencies completed!"); } diff --git a/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpactEvaluator.java b/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpactEvaluator.java index b2682d457..17447ae56 100644 --- a/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpactEvaluator.java +++ b/annotator-core/src/main/java/edu/ucr/cs/riple/core/cache/downstream/DownstreamImpactEvaluator.java @@ -29,10 +29,7 @@ import edu.ucr.cs.riple.core.evaluators.BasicEvaluator; import edu.ucr.cs.riple.core.evaluators.suppliers.DownstreamDependencySupplier; import edu.ucr.cs.riple.core.metadata.index.Error; -import edu.ucr.cs.riple.injector.location.OnMethod; -import edu.ucr.cs.riple.injector.location.OnParameter; -import java.util.Collections; -import java.util.HashMap; +import edu.ucr.cs.riple.injector.location.Location; import java.util.Set; import java.util.stream.Collectors; @@ -43,16 +40,8 @@ */ class DownstreamImpactEvaluator extends BasicEvaluator { - /** - * Map of public methods in target module to parameters in target module, which are source of - * nullable flow back to upstream module (target) from downstream dependencies, if annotated as - * {@code @Nullable}. - */ - private final HashMap> nullableFlowMap; - public DownstreamImpactEvaluator(DownstreamDependencySupplier supplier) { super(supplier); - this.nullableFlowMap = new HashMap<>(); } @Override @@ -65,45 +54,27 @@ protected void collectGraphResults(ImmutableSet reports) { node -> node.root.ifOnMethod( method -> { - // Impacted parameters. - Set parameters = + // Impacted locations. + Set locations = node.triggeredErrors.stream() .filter( error -> error.isSingleFix() - && error.toResolvingLocation().isOnParameter() // Method is declared in the target module. && context.targetModuleInfo.declaredInModule( - error.toResolvingParameter())) - .map(Error::toResolvingParameter) + error.toResolvingLocation())) + .map(Error::toResolvingLocation) .collect(Collectors.toSet()); - if (!parameters.isEmpty()) { - // Update path for each parameter. These triggered fixes do not have an + if (!locations.isEmpty()) { + // Update path for each location. These triggered fixes do not have an // actual physical path since they are provided as a jar file in downstream // dependencies. - parameters.forEach( - onParameter -> - onParameter.path = - context - .targetModuleInfo - .getMethodRegistry() - .findMethodByName( - onParameter.clazz, onParameter.enclosingMethod.method) - .location + locations.forEach( + location -> + location.path = + context.targetModuleInfo.getLocationOnClass(location.clazz) .path); - nullableFlowMap.put(method, parameters); } })); } - - /** - * Returns set of parameters that will receive {@code @Nullable} if the passed method is annotated - * as {@code @Nullable}. - * - * @param method Method to be annotated. - * @return Set of impacted parameters. If no parameter is impacted, empty set will be returned. - */ - public Set getImpactedParameters(OnMethod method) { - return nullableFlowMap.getOrDefault(method, Collections.emptySet()); - } } diff --git a/annotator-core/src/main/java/edu/ucr/cs/riple/core/evaluators/graph/processors/AbstractConflictGraphProcessor.java b/annotator-core/src/main/java/edu/ucr/cs/riple/core/evaluators/graph/processors/AbstractConflictGraphProcessor.java index a7f2e7941..8b00bfd0b 100644 --- a/annotator-core/src/main/java/edu/ucr/cs/riple/core/evaluators/graph/processors/AbstractConflictGraphProcessor.java +++ b/annotator-core/src/main/java/edu/ucr/cs/riple/core/evaluators/graph/processors/AbstractConflictGraphProcessor.java @@ -74,14 +74,13 @@ protected Set getTriggeredFixesFromDownstreamErrors(Node node) { .filter( error -> error.isSingleFix() - && error.toResolvingLocation().isOnParameter() && error.isFixableOnTarget(context) && !currentLocationsTargetedByTree.contains(error.toResolvingLocation())) .map( error -> new Fix( new AddMarkerAnnotation( - error.toResolvingLocation().toParameter(), context.config.nullableAnnot), + error.toResolvingLocation(), context.config.nullableAnnot), "PASSING_NULLABLE", false)) .collect(Collectors.toSet()); diff --git a/annotator-core/src/test/java/edu/ucr/cs/riple/core/ArgumentNullableFlowTest.java b/annotator-core/src/test/java/edu/ucr/cs/riple/core/NullableFlowToUpstreamTest.java similarity index 74% rename from annotator-core/src/test/java/edu/ucr/cs/riple/core/ArgumentNullableFlowTest.java rename to annotator-core/src/test/java/edu/ucr/cs/riple/core/NullableFlowToUpstreamTest.java index d93030245..f1e909f3b 100644 --- a/annotator-core/src/test/java/edu/ucr/cs/riple/core/ArgumentNullableFlowTest.java +++ b/annotator-core/src/test/java/edu/ucr/cs/riple/core/NullableFlowToUpstreamTest.java @@ -27,14 +27,16 @@ import static com.google.common.collect.Sets.newHashSet; import edu.ucr.cs.riple.core.tools.TReport; +import edu.ucr.cs.riple.injector.location.OnField; import edu.ucr.cs.riple.injector.location.OnMethod; import edu.ucr.cs.riple.injector.location.OnParameter; import java.util.Collections; +import java.util.Set; import org.junit.Test; -public class ArgumentNullableFlowTest extends AnnotatorBaseCoreTest { +public class NullableFlowToUpstreamTest extends AnnotatorBaseCoreTest { - public ArgumentNullableFlowTest() { + public NullableFlowToUpstreamTest() { super("nullable-multi-modular"); } @@ -109,4 +111,46 @@ public void nullableFlowDetectionDisabledTest() { .disableBailOut() .start(); } + + @Test + public void nullableFlowToUpstreamThroughFieldWriteTest() { + coreTestHelper + .onTarget() + .withSourceLines( + "Bar.java", + "package test;", + "public class Bar {", + " public String foo;", + " public String foo2 = \"\";", + " public String getFoo() {", + " return foo;", + " }", + "}") + .withDependency("Dep") + .withSourceLines( + "Dep.java", + "package test.dep;", + "import test.Bar;", + "public class Dep {", + " public Bar bar = new Bar();", + " public void exec() {", + " bar.foo2 = bar.getFoo();", + " }", + "}") + .withExpectedReports( + new TReport( + new OnField("Bar.java", "test.Bar", Set.of("foo")), + -1, + // fixes in tree: + Set.of( + new OnMethod("Bar.java", "test.Bar", "getFoo(String)"), + // coming from flow of nullable back to target through a field write. + new OnField("Bar.java", "test.Bar", Set.of("foo2"))), + Set.of())) + .disableBailOut() + .enableDownstreamDependencyAnalysis(AnalysisMode.STRICT) + .toDepth(5) + .checkExpectedOutput("nullableflowfieldwrite/expected") + .start(); + } } diff --git a/annotator-core/src/test/resources/nullableflowfieldwrite/expected/Dep/src/main/java/test/Dep.java b/annotator-core/src/test/resources/nullableflowfieldwrite/expected/Dep/src/main/java/test/Dep.java new file mode 100644 index 000000000..9e29f8f35 --- /dev/null +++ b/annotator-core/src/test/resources/nullableflowfieldwrite/expected/Dep/src/main/java/test/Dep.java @@ -0,0 +1,8 @@ +package test.dep; +import test.Bar; +public class Dep { + public Bar bar = new Bar(); + public void exec() { + bar.foo2 = bar.getFoo(); + } +} \ No newline at end of file diff --git a/annotator-core/src/test/resources/nullableflowfieldwrite/expected/Target/src/main/java/test/Bar.java b/annotator-core/src/test/resources/nullableflowfieldwrite/expected/Target/src/main/java/test/Bar.java new file mode 100644 index 000000000..eb4acd023 --- /dev/null +++ b/annotator-core/src/test/resources/nullableflowfieldwrite/expected/Target/src/main/java/test/Bar.java @@ -0,0 +1,9 @@ +package test; +import javax.annotation.Nullable; +public class Bar { + @Nullable public String foo; + @Nullable public String foo2 = ""; + @Nullable public String getFoo() { + return foo; + } +} \ No newline at end of file