Insert annotations at the correct position for arrays #251

avenger2597 · 2024-10-09T23:48:52Z

This PR helps insert annotations at the correct position for array declarations.

For array declarations we want the annotations to be inserted just before the array type brackets. For example :
String @Nullable [] f2;

Currently, the behaviour is that the annotations are inserted before the simpleName type. Like this :
@Nullable String[] f2;

nimakarimipour · 2024-10-10T16:41:20Z

@avenger2597 Please add a description to the PR explaining what it does, the bug it resolves, and how the changes address the issue. Also, update the title to accurately reflect the purpose of the PR.

nimakarimipour

Couple of comments.

nimakarimipour · 2024-10-10T18:38:12Z

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

+
+    String annotation = annotationExpr.toString();
+    // Check node for array
+    int index = node.toString().indexOf('[');


This is not a right way to determine the type. You can use Helper.getTypeFromNode(node) and then call isArray() on that.

nimakarimipour · 2024-10-10T18:38:54Z

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

+    int index = node.toString().indexOf('[');
+    // Node has array
+    if (index != -1) {
+      int begin = range.begin.column + index;


This is an unused variable. Should be removed ?

Updated this

nimakarimipour · 2024-10-10T18:40:09Z

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

+            "import javax.annotation.Nullable;",
+            "public class Foo {",
+            "   Object[] h = new Object[4];",
+            "   public void test(@Nullable Object[] f) {",


If we are not doing any operation on this method, remove the method from the test input. Test inputs should be as minimal as possible, containing only the information necessary to demonstrate improvements or bugs.

Removed this test case entirely. Currently using TypeUseAnnotationTest.additionOnArrayTest() to test my chages. Can add more if needed later.

nimakarimipour · 2024-10-10T18:40:59Z

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

@@ -244,9 +244,9 @@ public void onArrayType() {
            "import edu.ucr.UnTainted;",
            "public class Foo<T> {",
            "   public void foo() {",
-            "      @UnTainted Map<String, T[]>[] f0;",


This failure is due to a bug in your changes, not an issue with the expected output.

Reverted to original

nimakarimipour · 2024-10-10T18:41:37Z

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

+            "   }",
+            "}")
+        .addChanges(
+            new AddMarkerAnnotation(


This test should fail with only AddTypeUseMarkerAnnotation instance not with AddMarkerAnnotation.

Removed this test case.

nimakarimipour · 2024-10-10T18:45:00Z

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

@@ -62,7 +62,17 @@ Modification computeTextModificationOn(T node) {
    if (annotAlreadyExists) {
      return null;
    }
-    return new Insertion(annotationExpr.toString(), range.begin);
+
+    String annotation = annotationExpr.toString();


I don't think this is a right place to add your changes. Your changes should alter AddTypeUseMarkerAnnotation behavior.

Hint: all you changes should be in here where we are returning array component range.

nimakarimipour · 2024-10-10T18:45:31Z

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

@@ -62,6 +62,35 @@ public void fieldNullableSimple() {
        .start();
  }

+  @Test
+  public void fieldNullableArray() {


This isn't a suitable name for this test. Unit test names should clearly reflect the specific feature or functionality being tested.

Removed this test case

nimakarimipour · 2024-10-10T18:49:13Z

@avenger2597 Make sure you merge the latest changes on master branch before resolving comments.

…ator into ankit/arrays

…tion Class

nimakarimipour · 2024-10-13T22:42:10Z

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

@@ -68,6 +69,13 @@ public Modification computeTextModificationOnType(Type type, AnnotationExpr anno
    if (range == null) {
      return null;
    }
+
+    if (type instanceof ArrayType) {


@avenger2597 As mentioned previously in this comment your changes should be in findSimpleNameRangeInTypeName method where we are returning the array's component type as the place to insert / remove annotation for arrays. You should just change the existing case for array and use type.getTokenRange() to compute the range right before the first [.

nimakarimipour · 2024-10-13T22:42:23Z

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

+
+    if (type instanceof ArrayType) {
+      ArrayType.Origin origin = ((ArrayType) type).getOrigin();
+      if (origin == ArrayType.Origin.TYPE) {


Let's not worry about this yet.

nimakarimipour

One comment.

Also please write a single and simple unit test for this.

nimakarimipour · 2024-10-25T18:20:33Z

injector/src/main/java/edu/ucr/cs/riple/injector/Helper.java

@@ -608,7 +609,14 @@ public static Range findSimpleNameRangeInTypeName(Type type) {
      return ((ClassOrInterfaceType) type).getName().getRange().get();
    }
    if (type instanceof ArrayType) {
-      return findSimpleNameRangeInTypeName(((ArrayType) type).getComponentType());
+      Optional<TokenRange> tokenRange = type.getTokenRange();
+      int index = type.toString().indexOf('[');


This is not the right way to compute the index. The type.toString() will return string representation of the type, not how it is written in the source code. You should find the index via:

for (JavaToken javaToken : range) { // Token T = javaToken.asString(); // Your logic }

Updated to this logic

@avenger2597 Nice, this is much cleaner now.

This PR splits all utility methods in `Helper` class into two different classes. (`TypeUtils` and `ASTUtils`). This can wait for #251. This PR only moves methods among classes, there is not a single change on any method declaration or implementation.

Added changes to handle arrays corectly

e59fa6d

nimakarimipour assigned avenger2597 Oct 10, 2024

nimakarimipour added the bug Something isn't working label Oct 10, 2024

nimakarimipour self-requested a review October 10, 2024 16:40

avenger2597 changed the title ~~Added changes to handle arrays corectly~~ Insert annotations at the correct position for arrays Oct 10, 2024

nimakarimipour requested changes Oct 10, 2024

View reviewed changes

nimakarimipour mentioned this pull request Oct 10, 2024

Split utility methods into two separate util class #254

Merged

avenger2597 added 2 commits October 13, 2024 11:14

Merge branch 'master' of https://github.com/avenger2597/NullAwayAnnot…

e4641c3

…ator into ankit/arrays

Updated master branch and added code change in AddTypeUseMarkerAnnota…

367d145

…tion Class

nimakarimipour self-requested a review October 13, 2024 22:41

nimakarimipour requested changes Oct 13, 2024

View reviewed changes

Using tokenRange to identify range

2b8ab91

nimakarimipour requested changes Oct 25, 2024

View reviewed changes

avenger2597 and others added 4 commits October 25, 2024 19:46

Updated token retrieval logic and added simple junit

1f8ec0e

Update test case to match expected outcome

b58ca67

reformat

b0848ca

fix issue with component type

883fc4e

nimakarimipour self-requested a review October 29, 2024 20:25

nimakarimipour added 2 commits October 29, 2024 13:43

add unit test

8a81e90

add unit test

9330ab8

nimakarimipour approved these changes Oct 29, 2024

View reviewed changes

nimakarimipour merged commit ac804c0 into ucr-riple:master Oct 29, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insert annotations at the correct position for arrays #251

Insert annotations at the correct position for arrays #251

avenger2597 commented Oct 9, 2024 •

edited

Loading

nimakarimipour commented Oct 10, 2024 •

edited

Loading

nimakarimipour left a comment

nimakarimipour Oct 10, 2024

avenger2597 Oct 13, 2024

nimakarimipour Oct 10, 2024 •

edited

Loading

avenger2597 Oct 13, 2024

nimakarimipour Oct 10, 2024 •

edited

Loading

avenger2597 Oct 13, 2024

nimakarimipour Oct 10, 2024 •

edited

Loading

avenger2597 Oct 13, 2024

nimakarimipour Oct 10, 2024

avenger2597 Oct 13, 2024

nimakarimipour Oct 10, 2024

avenger2597 Oct 13, 2024

nimakarimipour Oct 10, 2024 •

edited

Loading

avenger2597 Oct 13, 2024

nimakarimipour commented Oct 10, 2024

nimakarimipour Oct 13, 2024

avenger2597 Oct 19, 2024

nimakarimipour Oct 13, 2024

avenger2597 Oct 19, 2024

nimakarimipour left a comment

nimakarimipour Oct 25, 2024

avenger2597 Oct 26, 2024

nimakarimipour Oct 27, 2024

Insert annotations at the correct position for arrays #251

Insert annotations at the correct position for arrays #251

Conversation

avenger2597 commented Oct 9, 2024 • edited Loading

nimakarimipour commented Oct 10, 2024 • edited Loading

nimakarimipour left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Oct 10, 2024 • 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

nimakarimipour Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour commented Oct 10, 2024

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 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

avenger2597 commented Oct 9, 2024 •

edited

Loading

nimakarimipour commented Oct 10, 2024 •

edited

Loading

nimakarimipour Oct 10, 2024 •

edited

Loading

nimakarimipour Oct 10, 2024 •

edited

Loading

nimakarimipour Oct 10, 2024 •

edited

Loading

nimakarimipour Oct 10, 2024 •

edited

Loading