Fix #55 by supporting @NullUnmarked #69

nimakarimipour · 2022-09-08T21:58:52Z

~~This PR is built upon GH-68.~~ GH-68 is landed.

This PR resolves #55 by adding a configuration mode which if activated, all remaining errors will be resolved with following the rules below:

Enclosing method of triggered errors will be marked as @NullUnmarked.
Uninitialized fields (inline or in constructor) will be annotated as @SuppressWarnings("NullAway.Init").
Explicit @Nullable assignments to fields will be annotated as @SuppressWarnings("NullAway").

With this PR, the target module will enroll into NullAway with no triggered errors.

To enable this mode, cli flag below must be passed to Annotator:

-fr or --force-resolve [followed by the "@NullUnmarked" fully qualified name]

nimakarimipour · 2022-09-13T16:03:58Z

@lazaroclapp Thank you for the review, this is ready for another review.

lazaroclapp

Nice. I appreciate the refactoring into AddMarkerAnnotation and AddSingleElementAnnotation. Some (hopefully minor) comments below. Please note that there are also a few - even smaller - nits in the replies to comment threads from the last pass.

lazaroclapp · 2022-09-13T16:30:00Z

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

+                new OnMethod("Super.java", "com.edu.Super", "test(java.lang.Object)"),
+                "edu.ucr.CustomNull",
+                "arg3",
+                true))


Appreciate the comprehensive test suite here, btw!

lazaroclapp · 2022-09-13T16:34:02Z

injector/src/main/java/edu/ucr/cs/riple/injector/changes/AddSingleElementAnnotation.java

+   * If true, if an annotation with the same name exists, the existing annotation should include the
+   * passed argument, otherwise, a new annotation will be injected to the node.
+   */
+  private final boolean collapseArguments;


Why call it this and not just repeatable to follow JLS terminology? (It would imply reversing the logic, of course, if repeatable=true then a new annotation is added, if it's false, then the expression inside is expanded into an array expression).

Thanks for the suggestion, that makes sense. d3c427c

lazaroclapp · 2022-09-13T16:36:03Z

injector/src/main/java/edu/ucr/cs/riple/injector/changes/AddSingleElementAnnotation.java

+    super(location, annotation);
+    Preconditions.checkArgument(
+        argument != null && !argument.equals(""),
+        "argument cannot be null or empty, use AddAnnotation instead.");


I understand not allowing null (Is this code being checked by NullAway, though? Because if so, we don't need the runtime check 😉 ). But, why is empty string not allowed? @Foo("") is a valid annotation if Foo is a Single-Element Annotation.

Not now, but It will be checked by NullAway as a long-term plan mentioned in #21 in future. I think it does not make sense to add the annotation as @Foo(""). Maybe we can force it to use MarkerAnnotation in such cases ? However, if you think still it might be useful in some scenarios to support such injections, please let me know and I will remove the guard. Thank you.

Let's leave it like this for now. I don't think there is anything in the Java spec that prevents you from having a single element annotation with the empty string as its value field, and there is definitely a semantic difference between empty-string vs no-value, but I don't think we care for any of the foreseeable use cases of the auto-annotator, so let's leave the check in for now.

Ok great sounds great. Will update it in future if needed.

lazaroclapp · 2022-09-13T16:40:00Z

injector/src/main/java/edu/ucr/cs/riple/injector/changes/AddSingleElementAnnotation.java

+            .anyMatch(
+                annot -> {
+                  if (!(annot.getNameAsString().equals(annotation)
+                      || annot.getNameAsString().equals(annotationSimpleName))) {


Let's not change this in this PR, but it's a bit weird that annotationSimpleName (and annotation) are fields of Change, rather than of AddAnnotation.

Sure, will do that.

lazaroclapp · 2022-09-13T16:43:30Z

injector/src/main/java/edu/ucr/cs/riple/injector/changes/AddSingleElementAnnotation.java

+        (SingleMemberAnnotationExpr) existingAnnotation;
+    ArrayInitializerExpr updatedMemberValue = new ArrayInitializerExpr();
+    NodeList<Expression> nodeList = new NodeList<>();
+    nodeList.add(argumentExp);


Nit: I'd move this below the two ifs below, that way we are appending to argument lists, rather than prepending

nimakarimipour · 2022-09-13T23:12:20Z

@lazaroclapp Thanks for the review, this is ready for another review.

lazaroclapp · 2022-09-14T00:02:40Z

injector/src/main/java/edu/ucr/cs/riple/injector/changes/RemoveAnnotation.java

  }

  @Override
+  @SuppressWarnings("unchecked")


This one should still be removed.

lazaroclapp · 2022-09-14T00:05:38Z

injector/src/main/java/edu/ucr/cs/riple/injector/changes/AddSingleElementAnnotation.java

+    super(location, annotation);
+    Preconditions.checkArgument(
+        argument != null && !argument.equals(""),
+        "argument cannot be null or empty, use AddAnnotation instead.");


Let's leave it like this for now. I don't think there is anything in the Java spec that prevents you from having a single element annotation with the empty string as its value field, and there is definitely a semantic difference between empty-string vs no-value, but I don't think we care for any of the foreseeable use cases of the auto-annotator, so let's leave the check in for now.

lazaroclapp

Very minor nits remaining: changes to comments and one unnecessary suppression

lazaroclapp · 2022-09-14T00:07:20Z

injector/src/main/java/edu/ucr/cs/riple/injector/changes/AddSingleElementAnnotation.java

@@ -95,13 +95,13 @@ public void visit(NodeWithAnnotations<?> node) {
      return;
    }

-    // Annotation with the same name exists, if collapse is off, add it directly.
-    if (!collapseArguments) {
+    // Annotation with the same name exists, if repeatable is off, add it directly.


// Annotation with the same name exists, but the annotation is repeatable, add it directly.

Updated it. Thanks for the suggestion.

lazaroclapp · 2022-09-14T00:07:45Z

injector/src/main/java/edu/ucr/cs/riple/injector/changes/AddSingleElementAnnotation.java

      addAnnotationExpressionOnNode(node, argumentExp);
      return;
    }

-    // Annotation with the same name exists, collapse is on, update it.
+    // Annotation with the same name exists, repeatable is on, update it.


// Annotation with the same name exists and is not repeatable, update it.

Updated it.

nimakarimipour · 2022-09-14T01:49:47Z

@lazaroclapp Thank you for the review, will land it once the CI pass.

lazaroclapp

LGTM! :)

nimakarimipour added 30 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

add global analyzer to explorers

2d3416d

reformat

c309a7c

add suppliers

bae9f6c

update tests

4f4dce1

merge with nimak/issue-64

82f6f53

simplification on update status method body

a9ad3e4

undo change in report testEqual method

e9fa556

add unit tests

c48da76

remove comma

204bd88

nimakarimipour added 12 commits September 12, 2022 17:30

update header

9f0463b

add a new line

42f23ab

update comment

eef31f8

rename addAnnotation to AddMarkerAnnotation

84daa4e

rename to AddSingleElementAnnotation

ede7896

update comment in remove annotations

527f375

updated tests with comment

f441ef4

updated all NullAway.init to NullAway.Init

808eff8

updated comment

559b012

move requireNonnull at the correct place

0553db4

update javadoc

a1fcf73

update exception message

e888346

nimakarimipour requested a review from lazaroclapp September 13, 2022 16:03

lazaroclapp reviewed Sep 13, 2022

View reviewed changes

nimakarimipour added 3 commits September 13, 2022 15:54

rename collapse to repeatable

d3c427c

prepend instead of append

fbc738b

update comments

e93c3c2

nimakarimipour mentioned this pull request Sep 13, 2022

Refactor Injector Change class. #70

Closed

nimakarimipour requested a review from lazaroclapp September 13, 2022 23:12

lazaroclapp reviewed Sep 14, 2022

View reviewed changes

nimakarimipour added 2 commits September 13, 2022 18:46

remove suppress warning

dc472d0

update javadoc

4d2d1ab

lazaroclapp approved these changes Sep 14, 2022

View reviewed changes

nimakarimipour merged commit 5f57920 into master Sep 14, 2022

nimakarimipour deleted the nimak/issue-55 branch September 14, 2022 03:04

nimakarimipour mentioned this pull request Sep 17, 2022

Force resolve cannot suppress errors for nullable param pass in field initializations. #73

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #55 by supporting @NullUnmarked #69

Fix #55 by supporting @NullUnmarked #69

nimakarimipour commented Sep 8, 2022 •

edited

nimakarimipour commented Sep 13, 2022

lazaroclapp left a comment

lazaroclapp Sep 13, 2022

nimakarimipour Sep 13, 2022

lazaroclapp Sep 13, 2022

nimakarimipour Sep 13, 2022

lazaroclapp Sep 13, 2022

nimakarimipour Sep 13, 2022

lazaroclapp Sep 14, 2022

nimakarimipour Sep 14, 2022

lazaroclapp Sep 13, 2022

nimakarimipour Sep 13, 2022

nimakarimipour Sep 13, 2022

lazaroclapp Sep 13, 2022

nimakarimipour Sep 13, 2022

nimakarimipour commented Sep 13, 2022

lazaroclapp Sep 14, 2022

nimakarimipour Sep 14, 2022

lazaroclapp Sep 14, 2022

lazaroclapp left a comment

lazaroclapp Sep 14, 2022

nimakarimipour Sep 14, 2022

lazaroclapp Sep 14, 2022

nimakarimipour Sep 14, 2022

nimakarimipour commented Sep 14, 2022

lazaroclapp left a comment

Fix #55 by supporting @NullUnmarked #69

Fix #55 by supporting @NullUnmarked #69

Conversation

nimakarimipour commented Sep 8, 2022 • edited

nimakarimipour commented Sep 13, 2022

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

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 13, 2022

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

nimakarimipour commented Sep 14, 2022

lazaroclapp left a comment

Choose a reason for hiding this comment

nimakarimipour commented Sep 8, 2022 •

edited