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.

Sign up for GitHub

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

Resolves #71 including fixes from downstream depenedencies #72

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Resolves #71 including fixes from downstream depenedencies #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/src/main/java/edu/ucr/cs/riple/core/Annotator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

/**
* The main class of the core module. Responsible for analyzing the target module and injecting the
Expand Down Expand Up @@ -185,7 +184,7 @@ private void executeNextIteration(
Set<Fix> selectedFixes =
latestReports.stream()
.filter(Report::approved)
.flatMap(report -> config.chain ? report.tree.stream() : Stream.of(report.root))
.flatMap(report -> report.getSelectedFixes(config))
.collect(Collectors.toSet());
injector.injectFixes(selectedFixes);

Expand Down
25 changes: 25 additions & 0 deletions core/src/main/java/edu/ucr/cs/riple/core/Report.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

/**
* Container class to store information regarding effectiveness of a fix, its associated fix tree
Expand Down Expand Up @@ -247,4 +248,28 @@ public boolean isInProgress(Config config) {
return (!finished && (!config.bailout || localEffect > 0))
|| triggeredFixes.stream().anyMatch(input -> !input.fixSourceIsInTarget);
}

/**
* Returns the stream of fixes that needs to be applied to source code if report is tagged as
* {@link Tag#APPROVE}.
*
* @param config Annotator config.
* @return Stream of selected fixes.
*/
public Stream<Fix> getSelectedFixes(Config config) {
return tree.stream()
.filter(
input -> {
// If chain is active, add all fixes
if (config.chain) {
return true;
}
// Add all fixes reported from downstream
if (!input.fixSourceIsInTarget) {
return true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, hard to tell without a test case, but won't this apply every fix in target that is part of the fix tree, even if intermediate fixes aren't applied?

Basically, the logic here is: Traverse the entire fix of trees and keep the root fix and any fix for a triggered error from a downstream dependency, but eliminate the fixes in between.

But I think the logic we want is: Keep the root fix, then, for each child (fix caused by an error triggered by the root fix) keep the child iff the child is a fix for a triggered error from a downstream dependency, then recurse on its children (equivalently: we prune from the tree all paths that pass through fixes other than the root fix originating from errors in the target).

Consider for example:

class Target {
   public Object foo() { return null; }
   public Object bar(Object o) { return o; }
   public Object baz(Object o) { return o; }
}

class Dep {
   public void m1(Target t) { 
     t.baz(t.bar(t.foo()));
   }
}

Assume a large enough DEPTH.

With tree-at-a-time, we get:

class Target {
   @Nullable public Object foo() { return null; }
   @Nullable public Object bar(@Nullable Object o) { return o; }
   @Nullable public Object baz(@Nullable Object o) { return o; }
}

With the algorithm as implemented and fix-at-a-time, we get:

class Target {
   @Nullable public Object foo() { return null; }
   public Object bar(@Nullable Object o) { return o; }
   public Object baz(@Nullable Object o) { return o; }
}

Because the change to foo() is the root fix, and all changes to the arguments of bar and baz derive from errors in Dep.

BUT, what I'd expect from fix-at-a-time is:

class Target {
   @Nullable public Object foo() { return null; }
   public Object bar(@Nullable Object o) { return o; }
   public Object baz(Object o) { return o; }
}

(No errors left of Dep, without adding extra fixes that aren't either the root fix or needed to avoid immediate errors in Dep)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your note and the example to describe that. Actually I thought about this issue, however, I think even in this case the output result will be the same. We annotate a module iteratively. If both fixes on bar() return and bar() param exists in the original fix tree, that means if param o is annotated as @Nullable fix for making bar() @Nullable will be triggered. Therefore even in this example, if we modify the algorithm to apply only the fixes you mentioned, in the next iteration annotator still annotates bar() as @Nullable and the final output will be identical to our current approach.

Also we currently do not store the map of fix to fix storing the which fix triggered which fix instance in tree. We just hold a set of fixes and a set of triggered fixes. To construct that, we need to apply coloring within trees itself. Please let me know if that makes sense or I am missing something thank you 🙂 .

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that what the example above shows is that the intermediate state after applying "a single fix" is incorrect. It is true that the example above would probably converge to the same annotations in the fix-at-a-time and tree-at-a-time modes, but nothing guarantees that in the general case, and meanwhile it seems to me that the intermediate state of the code is very much an unexpected one (there are 'fixes'/annotations that were added but would produce no errors anywhere if they were removed).

If we need to apply this hack now to have a reasonable result from the auto-annotator internally, I'd say we do it, for now, but it still feels to me that the correct solution here if we have trees of fixes is to actually have the trees represented somewhere within the report and pruning them in the fix-at-a-time case. I don't see what that has to do with coloring, all you need to know is, when you add fixes to the set of fixes Set<Fix> tree(?), which was the fix that triggered the error that required adding the fix, so it seems to me that the structure could be an actual tree, rather than a flat set. Am I missing something here?

cc: @msridhar

Copy link
Member Author

@nimakarimipour nimakarimipour Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it definitely worths to convert the tree flat structure to actual tree, however, it is still impossible to have a map of fix to fix and by that I mean, there might be two fixes in the tree that trigger the same fix and both be in the same region.
Please consider the class below:
The root fix is f0, it continues with annotating f1 and f2 to @Nullable and in the end when f0, f1 and f2 is @Nullable, we get to annotate bar() and baz()#param as @Nullable. Hence, we can't map f1 or f2 as parent for fix making bar() return @Nullable and just f2 parent for making baz()#param @Nullable. (From Annotators perspective, f1and f2 both are cause of making baz()#param as @Nullable).

class Foo {
   Object f0 = null;
   Object f1 = f0;
   Object f2 = f0;
   
   Object bar(boolean b) {
        baz(f2);
        if(b){
            return f2;
        }
        return f1;
   }

   void baz(Object p) { 
       // No Op
   }
}

The tree in theory: (to construct this we need to apply f1 and f2 separately, coloring within trees).

     |F0| {Making F0 @Nullable | Iteration 1} 
    /    \
|F1|      |F2| {Making F1 and F2 @Nullable | Iteration 2}
    \    /    \
    |bar|      |param| {Making bar() and baz#param @Nullable | Iteration 3}

However, we can still have a proximation of the tree as below with our current implementation:

     |F0| {Making F0 @Nullable | Iteration 1} 
       |
|F1|.......|F2| {Making F1 and F2 @Nullable | Iteration 2}
       |
|bar|......|param|     {Making bar() and baz#param @Nullable | Iteration 3}

And I think we can deliver the requested output with this tree, will update this PR shortly. Please let me know if I missed anything. Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that what the example above shows is that the intermediate state after applying "a single fix" is incorrect. It is true that the example above would probably converge to the same annotations in the fix-at-a-time and tree-at-a-time modes, but nothing guarantees that in the general case, and meanwhile it seems to me that the intermediate state of the code is very much an unexpected one (there are 'fixes'/annotations that were added but would produce no errors anywhere if they were removed).

If we need to apply this hack now to have a reasonable result from the auto-annotator internally, I'd say we do it, for now, but it still feels to me that the correct solution here if we have trees of fixes is to actually have the trees represented somewhere within the report and pruning them in the fix-at-a-time case. I don't see what that has to do with coloring, all you need to know is, when you add fixes to the set of fixes Set<Fix> tree(?), which was the fix that triggered the error that required adding the fix, so it seems to me that the structure could be an actual tree, rather than a flat set. Am I missing something here?

cc: @msridhar

But I don't fully understand that how is it possible that we remove a fix from tree and see no triggered errors, they are only in the tree if they resolve a specific error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to catch up on this discussion, sorry if the questions don't make sense.

For your example, @nimakarimipour:

   Object bar(boolean b) {
        baz(f2);
        if(b){
            return f0;
        }
        return f1;
   }

Since this code returns f0, shouldn't there be an edge from f0 to bar() (return) in the ideal tree?

Also: can we just use tree-at-a-time in the internal deployment to solve this problem? I'm not sure why fix-at-a-time is being used.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I don't fully understand that how is it possible that we remove a fix from tree and see no triggered errors, they are only in the tree if they resolve a specific error.

See the example I gave above, I am pretty sure that after one iteration of applying annotations with the current code here, you'd get:

class Target {
   @Nullable public Object foo() { return null; }
   public Object bar(@Nullable Object o) { return o; }
   public Object baz(@Nullable Object o) { return o; }
}

class Dep {
   public void m1(Target t) { 
     t.baz(t.bar(t.foo()));
   }
}

There, you can remove the @Nullable on the argument of baz(...) and will get no additional error comparing to the code with the annotation.

(Your existing errors are both about the return of bar and baz, but not about baz(...) being passed null, because, as that code is written, the expression t.bar(t.foo()) is @NonNull locally)

The tree is in fact: (to construct this we need to apply f1 and f2 separately, coloring within trees).
[...]

Ok, that's a DAG. And I imagine your argument here is that we don't explicitly construct the edges of the DAG currently and that doing so would be costly (i.e. require new calls to the build)? Or, technically, we could construct a spanning tree of the DAG as we go along, but there would be paths missing for the full DAG. If that's the case, we might need to think about how fixes are added to the set, so that if there are any edges forced by errors on the downstream dependencies, then those edges must be included in the generated spanning tree. Alternatively, you have the model you built for the downstream dependencies, so you could query it once you have the root node and rebuild the part of the tree forced only by said root node and by errors reported in downstream dependencies, no? Without any new builds required.

Either way: I think this is an important discussion and a potential bug on this PR, but for the sake of getting something running end-to-end internally this week, I'd vote on landing this version. I still think we need an issue to track this discussion (possibly #71 itself).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to catch up on this discussion, sorry if the questions don't make sense.

For your example, @nimakarimipour:

   Object bar(boolean b) {
        baz(f2);
        if(b){
            return f0;
        }
        return f1;
   }

Since this code returns f0, shouldn't there be an edge from f0 to bar() (return) in the ideal tree?

Also: can we just use tree-at-a-time in the internal deployment to solve this problem? I'm not sure why fix-at-a-time is being used.

@msridhar sorry for the wrong example, Please find the updated version. I replaced f0 with f2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nimakarimipour I'd still like to understand why the fix-at-a-time config is important for the scenario of downstream modules, as opposed to just using tree-at-a-time.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we decided, a long time ago, to use fix-at-a-time because any tradeoff of human effort for tool runtime within reason is worth it for us here, and there is some evidence that fix-at-a-time converges to slightly better answers than tree-at-a-time in some cases. That said, at this point, if fix-at-a-time is broken and tree-at-a-time isn't, I'd be fine with switching the default internally, cutting a -LOCAL release here and landing the internal diff by evening tomorrow (some changes are needed on the diff/python wrapper independent of this, but I am on it right now).

// Add root.
return input.equals(root);
});
}
}
1 change: 0 additions & 1 deletion core/src/main/java/edu/ucr/cs/riple/core/metadata/graph/Node.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,6 @@ public void updateTriggered(Collection<Fix> fixes) {
/** Merges triggered fixes to the tree, to prepare the analysis for the next depth. */
public void mergeTriggered() {
this.tree.addAll(this.triggeredFixes);
this.tree.forEach(fix -> fix.fixSourceIsInTarget = true);
this.triggeredFixes.clear();
}

Expand Down
8 changes: 7 additions & 1 deletion core/src/test/java/edu/ucr/cs/riple/core/tools/CoreTestHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public class CoreTestHelper {
private boolean downstreamDependencyAnalysisActivated = false;
private AnalysisMode mode = AnalysisMode.LOCAL;
private Config config;
private boolean chain = true;

public CoreTestHelper(Path projectPath, Path outDirPath, List<String> modules) {
this.projectPath = projectPath;
Expand Down Expand Up @@ -150,6 +151,11 @@ public CoreTestHelper enableDownstreamDependencyAnalysis() {
return enableDownstreamDependencyAnalysis(AnalysisMode.LOWER_BOUND);
}

public CoreTestHelper applyRootFixOnly() {
this.chain = false;
return this;
}

public void start() {
Path configPath = outDirPath.resolve("config.json");
createFiles();
Expand Down Expand Up @@ -264,7 +270,7 @@ private void makeAnnotatorConfigFile(Path configPath) {
builder.outputDir = outDirPath.toString();
builder.depth = depth;
builder.bailout = !disableBailout;
builder.chain = true;
builder.chain = chain;
builder.outerLoopActivation = requestCompleteLoop;
builder.optimized = true;
builder.downStreamDependenciesAnalysisActivated = downstreamDependencyAnalysisActivated;
Expand Down