Add @NullUnmarked on classes #134

nimakarimipour · 2023-02-11T17:43:41Z

This PR enables annotator to add @NullUnmarked annotations to suppress errors inside static initialization blocks.

It will also one last step to resolve any remaining errors after suppression injections by adding @NullUnmarked annotation on the enclosing class.

msridhar

couldn't read through the whole PR but here are a couple quick comments

annotator-core/src/main/java/edu/ucr/cs/riple/core/Annotator.java

msridhar · 2023-02-12T00:47:56Z

annotator-core/src/main/java/edu/ucr/cs/riple/core/Annotator.java

@@ -263,6 +263,22 @@ private void forceResolveRemainingErrors() {
    // Update log.
    config.log.updateInjectedAnnotations(nullUnMarkedAnnotations);

+    // Collect @NullUnmarked annotations for classes for errors in static initialization blocks.


What about instance initialization blocks? E.g., you could have:

class Foo { { getNullable().toString(); } @Nullable static Object getNullable() { return null; } }

Will our logic handle this case too?

Yes it does 🙂. Updated a unit test to verify this 47998c0

Maybe we should do some quick renaming in comments / code where you refer to static blocks, to call them initializer blocks (or just INIT_BLOCK to make it shorter)?

Changed to INIT_BLOCK cafef18

Co-authored-by: Manu Sridharan <msridhar@gmail.com>

lazaroclapp

Some nits and questions, but overall it makes sense to me.

lazaroclapp · 2023-02-13T18:07:25Z

injector/src/test/java/edu/ucr/cs/riple/injector/OnClassInjectionTest.java

+        .addInput(
+            "Super.java",
+            "package com.uber;",
+            "import edu.custom.NullUnmarked;",


Does the input need to have the import already or is this added automatically? Or is that outside the scope of this test harness?

Actually that was not necessary, removed it 139c19a. Thank you for noticing this.

lazaroclapp · 2023-02-13T18:08:07Z

injector/src/test/java/edu/ucr/cs/riple/injector/OnClassInjectionTest.java

+            new AddMarkerAnnotation(
+                new OnClass("Test.java", "edu.ucr.Test$1"), "edu.custom.NullUnmarked"))
+        .start();
+  }


How about named inner classes? Does the logic work in that case?

Yes it does, please find the added test at 71b998b

lazaroclapp · 2023-02-13T18:17:53Z

annotator-core/src/test/java/edu/ucr/cs/riple/core/CoreTest.java

@@ -426,6 +427,9 @@ public void staticBlockLocalVariableInitializationTest() {
            "   }",
            "}",
            "class B {",
+            "   {",
+            "      foo().hashCode();",
+            "   }",


Do we want to test static initializer block support and instance initializer block in the same test? (if so, the test case might need renaming). Or is it perhaps better to have two separate test cases for ease of reading/debugging?

I think the decision for now is to have them in one test until #137 is finalized, therefore I renamed the test name 344007b

lazaroclapp · 2023-02-13T18:23:14Z

annotator-core/src/main/java/edu/ucr/cs/riple/core/Annotator.java

+        Utility.readErrorsFromOutputDirectory(config, config.target, fieldDeclarationStore);
+    nullUnMarkedAnnotations =
+        remainingErrors.stream()
+            .filter(error -> !error.getRegion().isInAnonymousClass())


Not for this PR, but long term, in theory, this should try to find the containing method/initializer and add @NullUnmarked there, no? (For anonymous inner classes). It can get complicated with nested anonymous classes and the places they can appear in, so definitely not asking for that in this PR, just wondering if that's in the roadmap for 1.3.6 or after 1.3.6.

Yes exactly. But I think the plan is to implement that in after 1.3.6.

lazaroclapp · 2023-02-13T18:25:15Z

annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/field/FieldDeclarationStore.java

+    FieldDeclarationInfo candidate =
+        findNodeWithHashHint(node -> node.clazz.equals(clazz), FieldDeclarationInfo.hash(clazz));
+    if (candidate == null) {
+      // field is on byte code.


So, this comment (and the name of the class), reference a field, but as far as I can tell this is looking for a class declaration and there is no field involved...

Yes, this class has the required information which its main usage is to store information regarding existing classes and their fields. I have a plan for refactoring these data structures that store information regarding the structure of a code and combine them into one data structure. But the plan is to do that after the release of 1.3.6. Please let me know if you would like me to change it. Thank you.

Ok, that makes sense, but even given that, this comment right above is incorrect here, because this method is not dealing with any fields. Either remove or change the comment?

lazaroclapp

So, mostly good except for a few nits. But one important thing is that I think there is an unrelated change that was added into this PR? Unless I am misunderstanding it. If it's a bug fix, it probably should be on a separate PR, unless it's related to adding @NullUnmarked on classes.

lazaroclapp · 2023-02-15T18:04:09Z

annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/field/FieldDeclarationStore.java

+    FieldDeclarationInfo candidate =
+        findNodeWithHashHint(node -> node.clazz.equals(clazz), FieldDeclarationInfo.hash(clazz));
+    if (candidate == null) {
+      // field is on byte code.


Ok, that makes sense, but even given that, this comment right above is incorrect here, because this method is not dealing with any fields. Either remove or change the comment?

lazaroclapp · 2023-02-15T18:08:51Z

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

@@ -92,8 +92,7 @@ public Node(Fix root) {
   */
  public void setOrigins(ErrorStore errorStore) {
    this.origins =
-        errorStore.getRegionsForElements(
-            error -> error.isSingleFix() && error.getResolvingFixes().contains(root));
+        errorStore.getRegionsForElements(error -> error.getResolvingFixes().contains(root));


Question: what is this change about? I don't understand either what the error.isSingleFix() check was ensuring before, nor why are we removing it now.

Origins are set of regions that triggered root fix. The condition isSingleFix was ensuring the error has exactly one fix which is not correct, initialization errors requires multiple fixes to get resolved, therefore I removed this condition.

This was fixed in #136 and the change of this PR now changed to branch in #136

annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/Region.java

lazaroclapp · 2023-02-15T18:11:07Z

annotator-core/src/test/java/edu/ucr/cs/riple/core/DownstreamAnalysisTest.java

-            // DepA by 3, DepB by 4 and DepC by 2. Hence, the total effect is: 5.
-            new TReport(new OnMethod("Foo.java", "test.target.Foo", "returnNullableBad(int)"), 5),
+            // DepA by 3, DepB by 4 and DepC by 3. Hence, the total effect is: 6.
+            new TReport(new OnMethod("Foo.java", "test.target.Foo", "returnNullableBad(int)"), 6),


What changed that changed the effects here and why is it part of this PR about @NullUnmarked classes? I thought effects were always computed before adding any suppressions/null-unmarked regions... is this related to the other change above I asked about? The error.isSingleFix() check? If so, should that fix be part of this PR?

Yes the bug fix is in #136 and the base of this PR now changed to branch in #136.

…ackers/Region.java Co-authored-by: Lázaro Clapp <lazaro@uber.com>

nimakarimipour · 2023-02-17T21:54:58Z

@lazaroclapp This is ready for another round of review. Thank you very much.

lazaroclapp

LGTM!

nimakarimipour added 6 commits February 10, 2023 08:07

init

d465eda

update

2e8e65f

undo unwanted change

46d84ac

update

1fba197

update

3e2e8b4

update

2bf6459

nimakarimipour added the enhancement New feature or request label Feb 11, 2023

nimakarimipour requested review from msridhar and lazaroclapp February 11, 2023 17:43

nimakarimipour self-assigned this Feb 11, 2023

nimakarimipour changed the title ~~Add @NullUnmarked for classes~~ Add @NullUnmarked on classes Feb 11, 2023

nimakarimipour changed the base branch from master to nimak/resolve-remaining February 11, 2023 19:01

nimakarimipour added 4 commits February 11, 2023 11:04

move to proper position

70bf1ef

update

6046ebf

update

f98fc55

undo deleted comment

fc41c67

msridhar reviewed Feb 12, 2023

View reviewed changes

nimakarimipour and others added 4 commits February 12, 2023 09:05

Update annotator-core/src/main/java/edu/ucr/cs/riple/core/Annotator.java

c201245

Co-authored-by: Manu Sridharan <msridhar@gmail.com>

reformat

da8c98a

keep file path info for class with no fields as well

64d8c57

update unit test

47998c0

nimakarimipour requested a review from msridhar February 12, 2023 17:57

remove print statement

01c679a

lazaroclapp reviewed Feb 13, 2023

View reviewed changes

nimakarimipour added 6 commits February 13, 2023 11:05

prevent constructor @NullUnmarked for constructors

8a517d0

Merge branch 'nimak/resolve-remaining' into nimak/nullunmarked-class

8c2c3ae

merge with nimak/force-resolve

6b49f66

add comment

fcd2fe6

Merge branch 'nimak/resolve-remaining' into nimak/nullunmarked-class

ab8d201

update unit test

5f25f1b

nimakarimipour added 13 commits February 14, 2023 21:36

bug fix

9b729e8

reformat

dcf81b4

update unit test expected values

0550bb5

Merge branch 'master' into nimak/nullunmarked-class

15fbbd4

Merge branch 'nimak/update-nullaway' into nimak/nullunmarked-class

9798ca4

update

084e9e6

rename to init block

cafef18

remove import from unit test input

139c19a

Merge branch 'master' into nimak/nullunmarked-class

1f39e96

remove import from unit test input

a811889

add unit test for named inner classes

71b998b

add unit test for named inner classes

4fb60f1

rename test

344007b

nimakarimipour requested a review from lazaroclapp February 15, 2023 09:03

Merge branch 'master' into nimak/update-nullaway

45374b8

lazaroclapp reviewed Feb 15, 2023

View reviewed changes

nimakarimipour and others added 3 commits February 15, 2023 10:36

Update annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/tr…

3e1bb7c

…ackers/Region.java Co-authored-by: Lázaro Clapp <lazaro@uber.com>

Update annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/tr…

3a43655

…ackers/Region.java Co-authored-by: Lázaro Clapp <lazaro@uber.com>

Merge branch 'nimak/update-nullaway' into nimak/nullunmarked-class

ca510cc

nimakarimipour changed the base branch from master to nimak/update-nullaway February 15, 2023 18:45

lazaroclapp mentioned this pull request Feb 15, 2023

Update nullaway test version to 0.10.8 #136

Merged

Base automatically changed from nimak/update-nullaway to master February 16, 2023 01:59

nimakarimipour added 2 commits February 15, 2023 18:03

merge master

95429c8

Merge branch 'master' into nimak/nullunmarked-class

4eb1bbe

nimakarimipour requested a review from lazaroclapp February 16, 2023 21:23

update comment

74e44b9

lazaroclapp approved these changes Feb 17, 2023

View reviewed changes

nimakarimipour merged commit 79bf04f into master Feb 17, 2023

nimakarimipour deleted the nimak/nullunmarked-class branch February 17, 2023 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add @NullUnmarked on classes #134

Add @NullUnmarked on classes #134

nimakarimipour commented Feb 11, 2023

msridhar left a comment

msridhar Feb 12, 2023

nimakarimipour Feb 12, 2023

msridhar Feb 13, 2023

nimakarimipour Feb 15, 2023

lazaroclapp left a comment

lazaroclapp Feb 13, 2023

nimakarimipour Feb 15, 2023

lazaroclapp Feb 13, 2023

nimakarimipour Feb 15, 2023

lazaroclapp Feb 13, 2023

nimakarimipour Feb 15, 2023

lazaroclapp Feb 13, 2023

nimakarimipour Feb 15, 2023

lazaroclapp Feb 13, 2023

nimakarimipour Feb 15, 2023 •

edited

Loading

lazaroclapp Feb 15, 2023

lazaroclapp left a comment

lazaroclapp Feb 15, 2023

lazaroclapp Feb 15, 2023

nimakarimipour Feb 15, 2023

nimakarimipour Feb 15, 2023

lazaroclapp Feb 15, 2023

nimakarimipour Feb 15, 2023

nimakarimipour commented Feb 17, 2023

lazaroclapp left a comment

Add @NullUnmarked on classes #134

Add @NullUnmarked on classes #134

Conversation

nimakarimipour commented Feb 11, 2023

msridhar left a 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

lazaroclapp left a 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 Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp left a 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 Feb 17, 2023

lazaroclapp left a comment

Choose a reason for hiding this comment

nimakarimipour Feb 15, 2023 •

edited

Loading