Fix array annotations #3464

sfc-gh-dtran · 2023-08-07T06:36:03Z

What's changed?

Fixes parsing for annotated array types

What's your motivation?

Fixes #3453
Fixes #2911
Fixes #3440
Fixes #3667

Anything in particular you'd like reviewers to focus on?

I add a new J.ArrayType.AnnotatedDimension auxiliary structure to store everything needed to represent a single dimension (e.g. <annotations> [])

Anyone you would like to review specifically?

@timtebeek @kunli2

Additional context

Checklist

I've added unit tests to cover both positive and negative cases
I've added the license header to any new files through ./gradlew licenseFormat
I've used the IntelliJ auto-formatter on affected files
I've updated the documentation (if applicable)

timtebeek · 2023-08-07T08:51:19Z

rewrite-java-test/src/test/java/org/openrewrite/java/JavaParserTest.java

+                  int[][] ints = new int[2][2];
+
+                  Integer[] @Nonnull [] integers = new Integer[1][1];
+
+                  Integer @Nonnull [] foo = new Integer[1];
+
+                  public int   @Nonnull    [] @Nonnull [] getInts() {
+                      return ints;
+                  }


Thanks a lot for being thorough with the various forms of annotations and whitespace here! Good to have this covered.

timtebeek · 2023-08-07T08:54:30Z

rewrite-groovy/src/main/java/org/openrewrite/groovy/GroovyParserVisitor.java

        while(classNode != null && classNode.isArray()) {
            classNode = classNode.getComponentType();
-            result.add(JRightPadded.build(sourceBefore("[")).withAfter(sourceBefore("]")));
+
+            // TODO: add support for getting each dimensions' annotations from classNode


Should we associate an issue with this TODO item and link it here such that it's not lost?
And what's the impact of not yet covering this in the current implementation?

The impact of not covering this should just be that the same bug will appear in Groovy annotated arrays. I'm not as familiar with the Groovy language specification (I actually don't even know if this annotated array syntax is valid in Groovy), and the GroovyParserVisitor is written differently to the JavaParserVisitors, so I opted to not implement it for now. It should be very similar to the Java case though; the AnnotatedDimension structure is already there so it should be easy to implement.

So yes, an issue for this TODO would probably be good

timtebeek · 2023-08-07T09:04:32Z

Thanks a lot for this effort @sfc-gh-dtran , and your clear communication around the issue and your approach. Due to the addition of ArrayType.AnnotatedDimension I'm going to defer a more thorough review to Kun & Sam, as they are more experienced around the parsers and impact of adding new constructs on for instance (de)serialization.

From what I read in Slack you're up and running with this change locally; does that mean you're unblocked for making changes ? I know you indicated you needed proper support here, and appreciate you stepping up. I just want to make sure you're on your way before we get to a review, merge & release.

sfc-gh-dtran · 2023-08-07T17:11:16Z

Thanks a lot for this effort @sfc-gh-dtran , and your clear communication around the issue and your approach. Due to the addition of ArrayType.AnnotatedDimension I'm going to defer a more thorough review to Kun & Sam, as they are more experienced around the parsers and impact of adding new constructs on for instance (de)serialization.

From what I read in Slack you're up and running with this change locally; does that mean you're unblocked for making changes ? I know you indicated you needed proper support here, and appreciate you stepping up. I just want to make sure you're on your way before we get to a review, merge & release.

Yes! I'm unblocked for making changes.

kunli2

Thanks for the contribution! left some comments, otherwise looks good to me.

kunli2 · 2023-08-07T17:29:39Z

rewrite-java-17/src/main/java/org/openrewrite/java/isolated/ReloadableJava17ParserVisitor.java

+
+        List<J.ArrayType.AnnotatedDimension> dimensions = new ArrayList<>();
+        currentNode = node;
+        while (currentNode instanceof ArrayTypeTree || currentNode instanceof AnnotatedTypeTree) {


It seems the while loop here covers the while loop above (lines 1299-1305), which looks redundant to me, since elemType is the only result we want from the above while loop and it can be fetched from this loop as well.

Since convert is responsible for scanning the whitespace between tokens using an internal cursor that is incremented left-to-right, it's essential that elemType (which contains the type identifier of the elements in the array type, e.g. the int in int @NotNull [] in line 1307 is converted first since it appears first in the array type.

To collapse this into one while loop, I would have to store the collected annotations (line 1314) and dimensions (line 1319) so that I can call convertAll and sourceBefore after elemType is converted; maybe through something like

dimensions = collectedAnnotations.map(anns -> new AnnotatedDimension( convertAll(anns), padRight(sourceBefore("["), sourceBefore("]")) );

I like that idea since it only needs one traversal of the ArrayTree, but it still involves two loops (or one loop and one map). How does that sound?

I see, thanks for the explanation, I think both look good to me, seems the 2nd approach (collect everything first) looks better.

kunli2 · 2023-08-07T17:31:14Z

rewrite-java-17/src/main/java/org/openrewrite/java/isolated/ReloadableJava17ParserVisitor.java

+                dimensions.add(new J.ArrayType.AnnotatedDimension(annotations, dimension));
+                currentNode = ((ArrayTypeTree) currentNode).getType();
+            }
+


nit: remove empty lines

knutwannheden · 2023-08-08T03:22:54Z

rewrite-java/src/main/java/org/openrewrite/java/tree/J.java

@@ -309,7 +309,7 @@ final class ArrayType implements J, TypeTree, Expression {
        TypeTree elementType;

        @With
-        List<JRightPadded<Space>> dimensions;
+        List<AnnotatedDimension> dimensions;


This change is not backwards compatible. While there probably aren't very many callers which need to be updated, there is also the compatibility of the serialization form to consider. While this could be addressed with a dedicated deserializer, we should still compare with alternatives, which are backwards compatible.

Thanks for the inputs here, the backward compatibility of deserialization is also my concern. I'm not sure how to handle this correctly, do you think adding a separate field works? and mark the old field deprecated?

I will try to take a look at it in the next few days.

This came up again in #3667 ; would it be possible to see this through?

Yes, we really need to get this fixed...

traceyyoshima · 2023-12-18T17:50:00Z

rewrite-java-11/src/main/java/org/openrewrite/java/isolated/ReloadableJava11ParserVisitor.java

@@ -1232,10 +1208,55 @@ public J visitTypeCast(TypeCastTree node, Space fmt) {

    @Override
    public J visitAnnotatedType(AnnotatedTypeTree node, Space fmt) {
+        if (node.getUnderlyingType() instanceof ArrayTypeTree) {


Update:
I'm looking into issue #3453, and came across this PR.

The AnnotatedTypeTree uses a pos field to retain where the annotation exists on the type.
Annotations that come before the J tree are added to the annotations field in the J.AnnotatedType.

AFAICT, visitAnnotatedArrayType will return a J.ArrayType, which does not have a container for leading annotations. So, this will cause an idempotent print issue for @LeadingAnn Integer @TypeUsePosAnn [] ints

We could add new fields if that helps and even delete old ones if we deal with the deserialization compatibility. I would have to take a look again to figure it out, but IIRC we have a nested structure in one place and on the other hand we just have an int reflecting the number of dimensions. To support the annotations we should be using a nested structure.

traceyyoshima · 2023-12-27T23:40:53Z

Hi @sfc-gh-dtran, we really appreciate your work on this PR and patience with us on addressing the issue.

We've added a new PR that is backwards compatible with older LSTs, updates the GradleParserVisitor, and fixes a handful of bugs that were noticed while implementing the changes. Thank you, again for being a part of OpenRewrite!

sfc-gh-dtran added 3 commits August 6, 2023 08:17

Fix annotated array parsing

4667cc2

Fix annotated array parsing

22b15e3

Formatting

581dc14

timtebeek assigned sfc-gh-dtran Aug 7, 2023

timtebeek requested review from kunli2 and sambsnyd August 7, 2023 08:49

timtebeek added bug Something isn't working enhancement New feature or request labels Aug 7, 2023

timtebeek reviewed Aug 7, 2023

View reviewed changes

kunli2 reviewed Aug 7, 2023

View reviewed changes

knutwannheden reviewed Aug 8, 2023

View reviewed changes

sfc-gh-dtran removed their assignment Aug 31, 2023

This was referenced Nov 7, 2023

StringIndexOutOfBoundsException (begin > end) in ReloadableJava17ParserVisitor.visitAnnotation #3667

Closed

Reduce stacktrace noise in the output when running with info mode openrewrite/rewrite-gradle-plugin#240

Open

timtebeek added the parser-java label Dec 1, 2023

knutwannheden mentioned this pull request Dec 15, 2023

Parsing error of annotation between array dimensions #3453

Closed

traceyyoshima reviewed Dec 18, 2023

View reviewed changes

knutwannheden closed this Dec 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix array annotations #3464

Fix array annotations #3464

sfc-gh-dtran commented Aug 7, 2023 •

edited by timtebeek

Loading

timtebeek Aug 7, 2023

timtebeek Aug 7, 2023

sfc-gh-dtran Aug 7, 2023 •

edited

Loading

timtebeek commented Aug 7, 2023

sfc-gh-dtran commented Aug 7, 2023

kunli2 left a comment

kunli2 Aug 7, 2023

sfc-gh-dtran Aug 7, 2023 •

edited

Loading

kunli2 Aug 8, 2023

kunli2 Aug 7, 2023

knutwannheden Aug 8, 2023

kunli2 Aug 8, 2023

knutwannheden Aug 8, 2023

timtebeek Nov 7, 2023

knutwannheden Nov 7, 2023

traceyyoshima Dec 18, 2023

knutwannheden Dec 18, 2023

traceyyoshima commented Dec 27, 2023

Fix array annotations #3464

Fix array annotations #3464

Conversation

sfc-gh-dtran commented Aug 7, 2023 • edited by timtebeek Loading

What's changed?

What's your motivation?

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Additional context

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-dtran Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

timtebeek commented Aug 7, 2023

sfc-gh-dtran commented Aug 7, 2023

kunli2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-dtran Aug 7, 2023 • 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

traceyyoshima commented Dec 27, 2023

sfc-gh-dtran commented Aug 7, 2023 •

edited by timtebeek

Loading

sfc-gh-dtran Aug 7, 2023 •

edited

Loading

sfc-gh-dtran Aug 7, 2023 •

edited

Loading