Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ArrayTypeTree and support for annotated dimensions of array types. #3848

Merged
merged 1 commit into from
Dec 28, 2023

Conversation

traceyyoshima
Copy link
Contributor

@traceyyoshima traceyyoshima commented Dec 22, 2023

Changes:

  • Updated J.ArrayType to preserve annotations on dimensions.
  • Removed J.ArrayType#dimensions field and added a constructor to transform old LSTs into the new model.
  • Deprecated J.VariableDeclarations#dimensionsBeforeName
  • ... is preserved in the J.VariableDeclarations#varargs field instead of the prefix of the variable name.
  • Updated GroovyParserVisitor with the new J.ArrayType.
  • Applied optimizations from Java 17 and 21 parsers to Java 11 and 8 parsers.
  • J.ArrayType trees produced by the Java parser will now contain JavaType.Array instead of JavaType.Class

fixes #2911
fixes #3440
fixes #3453

@traceyyoshima
Copy link
Contributor Author

traceyyoshima commented Dec 22, 2023

@knutwannheden, Changes are ready for review.

Annotations on C-style arrays are still unsupported:

@traceyyoshima
Copy link
Contributor Author

traceyyoshima commented Dec 22, 2023

Update:
There is a bug in the JavaParser where the ... is parsed into the prefix of ns in the following code:

              public class A {
                  Integer o = generic ( 0, 1, 2 );
                  Integer p = this . < Integer > generic ( 0, 1, 2 );
                            
                  public <TTTT> TTTT generic(TTTT n, TTTT... ns) { return n; }
              }

I'll address the issue along with the changes. fixed.

@traceyyoshima traceyyoshima force-pushed the annotated-array-dimensions branch 4 times, most recently from 926d905 to 050ca74 Compare December 23, 2023 04:23
@traceyyoshima traceyyoshima marked this pull request as ready for review December 23, 2023 04:43
Copy link
Contributor

@knutwannheden knutwannheden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at the details of the parser yet, but I commented on the LST model already.

@traceyyoshima traceyyoshima force-pushed the annotated-array-dimensions branch 2 times, most recently from a44a522 to 83ed9e2 Compare December 25, 2023 00:41
Copy link
Contributor

@knutwannheden knutwannheden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have missed it, but I couldn't find the updated AnnotationService. Apart from that and a few small comments I left, I think this looks very good.

Copy link
Contributor

@knutwannheden knutwannheden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good now and it looks like there were a few related bugs that could be squashed, which is very nice!

@traceyyoshima traceyyoshima mentioned this pull request Dec 27, 2023
4 tasks
@knutwannheden
Copy link
Contributor

As a last thing (in a separate PR) wr may want to make sure the type attribution also has all annotations for array types, if it doesn't already.

Fixed detection of `...` in method declarations.
Synced code between Java parsers.
Removed ArrayTypeTree and added changes to ArrayType.
Changed dimension field from JRightPadded<Space> to JLeftPadded<Space>.
Added JavaType.Array arrayType field to J.ArrayType.

Removed dimensions from J.ArrayType and added JSON creator.
Renamed JavaType.Array field to type.

Updated AnnotationService.
Updated GroovyParserVisitor.
Set the JavaType from old LSTs in the JsonCreator constructor.

Updated annotation service and tests.

Updated the JsonCreator constructor to set the correct types if the elementType is a JavaType$Array in J.ArrayType.

Update rewrite-java/src/main/java/org/openrewrite/java/service/AnnotationService.java

Co-authored-by: Knut Wannheden <knut@moderne.io>

Polish.
@traceyyoshima
Copy link
Contributor Author

Rebased and squashed commits

@traceyyoshima
Copy link
Contributor Author

traceyyoshima commented Dec 27, 2023

As a last thing (in a separate PR) wr may want to make sure the type attribution also has all annotations for array types, if it doesn't already.

Sounds good! I did confirm the types are correct on the annotations, but I'll ensure the JavaTypes have the annotations as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment