Treat impacted parameters from downstream dependencies as triggered fixes. #65

nimakarimipour · 2022-09-06T20:52:47Z

Resolves #64 by treating impacted parameters by downstream dependencies as triggered fixes while processing public methods.

It resolves the issue with race condition in unit tests as they are using a shared library model loader. This is resolved in the unit test infrastructure within this PR.

It also resolves issue with updating the stored impacts in downstream dependencies after each injection to prevent double counting.

It also contains huge refactoring over creation of Explorers.

nimakarimipour · 2022-09-07T01:58:30Z

@lazaroclapp This PR is ready for review. This changes made a lot of issues to avoid double counting since we are treating impacted parameters as triggered fixes. It also includes a refactoring to clean instantiating explorers since downstream dependency analysis is added as part of the explorers. Please let me know if you prefer to break this PR into multiple PRs. Thank you very much :)

lazaroclapp · 2022-09-07T02:59:31Z

Taking a look... not sure if I'll be able to finish my pass tonight, but for now, one thing worth pointing out is that the description for this PR seems incomplete, at the very least this is also solving the separate issue of the shared library models directory... (I think that would be an argument for it to be split into multiple PRs, each addressing a different bug or feature, but I also think we are short on time, so let me take a look as is, just make sure we document what this PR is actually changing 🙂 )

lazaroclapp

Finishing this pass fairly late, hopefully I didn't misunderstand anything major, but here are a few comments 🙂 (If something is cryptic or unparsable, I can take a look and write a better comment tomorrow 😅 )

lazaroclapp · 2022-09-07T03:05:09Z

core/src/main/java/edu/ucr/cs/riple/core/metadata/method/MethodDeclarationTree.java

        .collect(ImmutableSet.toImmutableSet());
  }

  /**
-   * Checks if the passed method, is declared in the target module.
+   * Checks if the passed location, is targeting an element declared in the target module.


No comma needed here, I think.

Removed it.

lazaroclapp · 2022-09-07T03:07:36Z

core/src/main/java/edu/ucr/cs/riple/core/metadata/method/MethodDeclarationTree.java

   */
-  public boolean declaredInModule(OnMethod location) {
-    return findNode(location.method, location.clazz) != null;
+  public boolean declaredInModule(@Nullable Location location) {


So, the only added caller of this method I see is methodDeclarationTree.declaredInModule(error.nonnullTarget.toMethod())) in DownstreamImpactAnalyzer. So, why change this from OnMethod to a generic Location and look only at the class? What am I missing?

To check if a location is defined in a module, we only need to check its enclosing class which this field, is shared among all Location subtypes. I thought there is no need to downcast to OnMethod and we can reuse this method for future changes. I removed the toMethod() which is not required anymore. 37018d0

lazaroclapp · 2022-09-07T03:08:58Z

core/src/main/java/edu/ucr/cs/riple/core/metadata/graph/Node.java

@@ -59,7 +59,7 @@ public class Node {
  public Set<Fix> triggeredFixes;

  /** Collection of triggered errors if tree is applied. */
-  public ImmutableSet<Error> triggeredErrors;
+  public ImmutableList<Error> triggeredErrors;


Why are we switching this to be a list, do we expect repetition?

Yes, we can have multiple errors with same message nonNullTarget and type.

Um, can you give me an example here of distinct errors with the same messageType, message, and nonnullTarget that we want to keep track of? Also, any reason we can't simply refine what hashCode() takes into account so that distinct errors mean distinct elements in this list/set?

I still don't understand what changes when we change how we track parameter fixes coming from downstream deps that makes us now want to consider repetitions but not before (if this is a separate bug fix from the core change of this PR, then it being mixed together with this feature makes it harder to understand).

Discussed offline, we probably want to track an issue here to switch back to a set once we have enough position info in errors.tsv to accurately distinguish separate errors in the same region with the same messageType and nonnullTarget, but for now this is not blocking 🙂

Please find the issue here #67.

lazaroclapp · 2022-09-07T03:30:48Z

core/src/main/java/edu/ucr/cs/riple/core/global/DownstreamImpactAnalyzer.java

+import java.util.stream.Collectors;
+
+/** Explorer for analyzing downstream dependencies. */
+public class DownstreamImpactAnalyzer extends OptimizedExplorer {


So, every other [Foo]Analyzer is part of the hierarchy rooted at GlobalAnalyzer, and every other subclass of Explorer is both named [Foo]Explorer and in the package explorers... why is this DownstreamImpactAnalyzer rather than DownstreamImpactExplorer?

If this is meant to be used only through GlobalAnalyzerImpl and the idea here is to separate it from the "top level" explorers that can be used as the main driver of the analysis of "target", then maybe an option is:

Clearly document this distinction in the javadoc for this class

Still name it DownstreamImpactExplorer (or maybe: DownstreamImpactHelperExplorer)

But leave it inside edu.ucr.cs.riple.core.global and make it package private.

Yes, that is the idea. Applied your comments. Thank you :)
2634001

lazaroclapp · 2022-09-07T03:36:48Z

core/src/main/java/edu/ucr/cs/riple/core/global/GlobalAnalyzer.java

+   * on the target on downstream dependencies.
+   *
+   * @param tree Tree of the fix tree associated to root.
+   * @return Lower bound of number of errors on downstream dependencies.


This should read "Upper bound of [...]"

Thank you for noticing this, fixed it. 7b8124d

lazaroclapp · 2022-09-07T03:55:48Z

core/src/main/java/edu/ucr/cs/riple/core/global/GlobalAnalyzerImpl.java

+    this.tree = tree;
+    this.methods =
+        Multimaps.index(
+            tree.getPublicMethodsWithNonPrimitivesReturn().stream()


So, just to be clear here, we still don't check for protected methods so we wouldn't see new errors caused by marking a protected method @Nullable and it being called within a subclass, right? (Not that we need that as part of this PR, just want to make sure we are tracking that limitation)

Yes, we are not tracking that now, would you like me to create a follow up PR and add this in their unit tests or include it in this PR ? If a follow up is desired, I will create an issue to make sure we keep track of it.

Worth an issue to track this, probably not a PR you'll get to implement in the next two weeks, but just having an issue that documents the limitation / future feature is good enough for now 🙂

Mentioned this in issue #66 :)

lazaroclapp · 2022-09-07T04:05:09Z

core/src/main/java/edu/ucr/cs/riple/core/global/GlobalAnalyzerImpl.java

+   * @return Effect on downstream dependencies.
+   */
+  private int effectOnDownstreamDependencies(Fix fix, Set<Location> fixesLocation) {
+    MethodImpact status = fetchStatus(fix);


Why status and not just: MethodImpact methodImpact = fetchMethodImpactForFix(...)? (renaming the fetchStatus method too)

Left over from old names/design, thanks for noticing this. 0b5e623

lazaroclapp · 2022-09-07T04:07:28Z

core/src/main/java/edu/ucr/cs/riple/core/global/GlobalAnalyzerImpl.java

+
+  @Override
+  public int computeLowerBoundOfNumberOfErrors(Set<Fix> tree) {
+    Set<Location> fixesLocation =


fixLocations, I think.

lazaroclapp · 2022-09-07T04:11:14Z

core/src/main/java/edu/ucr/cs/riple/core/global/GlobalAnalyzerImpl.java

+
+  @Override
+  public List<Error> getTriggeredErrors(Fix fix) {
+    // We currently only store impact of methods on downstream dependencies.


Might be worth having an issue to track this for arguments/fields and link it here. Though I think that's a fairly low priority issue to solve right now, it's worth documenting the limitation and making sure we remember it's there 🙂

Created an issue #66. :)

lazaroclapp · 2022-09-07T04:17:42Z

core/src/test/java/edu/ucr/cs/riple/core/ArgumentNullableFlowTest.java

@@ -44,27 +44,32 @@ public void nullableFlowDetectionEnabledTest() {
    coreTestHelper


So, should the test cases requested here be part of this PR or are they pending some other modification from the other half of the PR? (Note that I am interested in seeing what would happen with those simple test cases, without necessarily adding five return null branches like suggested in #62. Happy to see both types of test cases, but the simple cases would be illustrative)

Yes that case is also covered in this approach. It has an accurate mechanism for updating states after each injection or within measurements. Added the unit test :) 838fad7

…notator into nimak/issue-64

nimakarimipour · 2022-09-07T18:44:15Z

Hi @lazaroclapp, thank you very much for the review, I resolved your comments and left a question. This is ready for another review. Thanks.

lazaroclapp

Let's briefly discuss the ImmutableSet/ImmutableList question in our meeting, but otherwise the changes look good to me 🙂

lazaroclapp

🚀

nimakarimipour · 2022-09-07T21:36:41Z

🚀

Thank you very much for the review, I created all the requested issue. Will land this now. 🙂

nimakarimipour added 21 commits September 2, 2022 14:24

update to javaparser version 3.24.3

8599122

update javaparser version to 3.24.4

9b13b08

Merge branch 'master' of github.com:nimakarimipour/NullAwayAnnotator

0fc18e2

basic refactoring of Annotator class

4183588

update triggered fix from downstream dependency list creation

5063eb8

update outer loop

a16cd22

Reformat

f0e6f2e

initial implementation of strict mode

6c957cd

update comment and generate fix algorithm

8f5ea31

improve cache report retrieval

11dd0b9

add basic unit test

bb6899c

initial refactored stage

ec8be3f

rename explore to analyze downstream depedencies

32a55da

merge nimak/refactor-downstream-depenedency

e45df25

update javadoc

ee47846

implemenet status update of methods after injection

f094799

add unit tests

8991e9b

convert parallel test to sequential

ffac587

re-enable parallel unit test execution

99e5228

exract interface from GlobalAnalayzer

58eb328

refactor exisiting codes

0ee4032

nimakarimipour added the enhancement New feature or request label Sep 6, 2022

nimakarimipour self-assigned this Sep 6, 2022

nimakarimipour linked an issue Sep 6, 2022 that may be closed by this pull request

Treat impacted parameters as triggered fixes. #64

Closed

nimakarimipour added 4 commits September 6, 2022 14:19

add global analyzer to explorers

2d3416d

reformat

c309a7c

add suppliers

bae9f6c

update tests

4f4dce1

nimakarimipour changed the title ~~[WIP] Treat impacted parameters from downstream dependencies as triggered fixes.~~ Treat impacted parameters from downstream dependencies as triggered fixes. Sep 7, 2022

nimakarimipour requested a review from lazaroclapp September 7, 2022 01:55

lazaroclapp reviewed Sep 7, 2022

View reviewed changes

nimakarimipour added 16 commits September 6, 2022 22:59

simplification on update status method body

a9ad3e4

undo change in report testEqual method

e9fa556

remove comma

204bd88

Merge branch 'nimak/issue-64' of github.com:nimakarimipour/NullAwayAn…

b1afa76

…notator into nimak/issue-64

remove unused method

d80d697

add toLocation()

2aa848a

remove toMethod call

37018d0

reformat

62342bd

rename DownstreamImpactAnalyzer to DownstreamImpactExplorer

2634001

update javadoc

7b8124d

update javadoc:

252031a

update javadoc

69248b3

rename modules to downstreamModules

fa563f0

rename to methodImpact

0b5e623

update local variable name to fixLocations

f225340

add unit test

838fad7

nimakarimipour requested a review from lazaroclapp September 7, 2022 18:44

update unit test to pass

b9b9a62

nimakarimipour mentioned this pull request Sep 7, 2022

Fix #62 by adding configuration mode for downstream dependency analysis. #63

Merged

lazaroclapp reviewed Sep 7, 2022

View reviewed changes

lazaroclapp approved these changes Sep 7, 2022

View reviewed changes

nimakarimipour merged commit c824d26 into master Sep 7, 2022

nimakarimipour deleted the nimak/issue-64 branch September 7, 2022 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat impacted parameters from downstream dependencies as triggered fixes. #65

Treat impacted parameters from downstream dependencies as triggered fixes. #65

nimakarimipour commented Sep 6, 2022 •

edited

Loading

nimakarimipour commented Sep 7, 2022

lazaroclapp commented Sep 7, 2022

lazaroclapp left a comment •

edited

Loading

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022 •

edited

Loading

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022

lazaroclapp Sep 7, 2022

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022

lazaroclapp Sep 7, 2022

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022

lazaroclapp Sep 7, 2022

nimakarimipour Sep 7, 2022

nimakarimipour commented Sep 7, 2022

lazaroclapp left a comment

lazaroclapp left a comment

nimakarimipour commented Sep 7, 2022

		@@ -44,27 +44,32 @@ public void nullableFlowDetectionEnabledTest() {
		coreTestHelper

Treat impacted parameters from downstream dependencies as triggered fixes. #65

Treat impacted parameters from downstream dependencies as triggered fixes. #65

Conversation

nimakarimipour commented Sep 6, 2022 • edited Loading

nimakarimipour commented Sep 7, 2022

lazaroclapp commented Sep 7, 2022

lazaroclapp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour commented Sep 7, 2022

lazaroclapp left a comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

nimakarimipour commented Sep 7, 2022

nimakarimipour commented Sep 6, 2022 •

edited

Loading

lazaroclapp left a comment •

edited

Loading

nimakarimipour Sep 7, 2022 •

edited

Loading