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

Improved handling of inner class named $ in JavaTemplate #3898

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

MBoegers
Copy link
Contributor

@MBoegers MBoegers commented Jan 9, 2024

What's changed?

  • TypeUtils handle $ better if determing the FQN
  • use TypeUtils in rewrite-java*

What's your motivation?

I want the fix the handling of $ as class name.
And fixing

Have you considered any alternatives or workarounds?

Any additional context

If finished this should enable
-openrewrite/rewrite-migrate-java#285

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@timtebeek timtebeek added the enhancement New feature or request label Jan 10, 2024
@timtebeek
Copy link
Contributor

timtebeek commented Jan 10, 2024

Great to see, thanks @MBoegers ; saving others a click by copying the remaining failure here:

JavaTemplateSubstitutionsTest > anyForInnerClass() FAILED
    org.opentest4j.AssertionFailedError: [Unexpected result in "A.java":
diff --git a/A.java b/A.java
index df29fc4..f075a84 100644
--- a/A.java
+++ b/A.java
@@ -1,6 +1,6 @@ 
 class A {
     void foo() {
-        $ test = new $();
+        $ test = new A.$();
     }
     static class $ {
         static $ of() {
    ] 
    expected: 
      "class A {
          void foo() {
              $ test = new $();
          }
          static class $ {
              static $ of() {
                  return new $();
              }
          }
      }"
     but was: 
      "class A {
          void foo() {
              $ test = new A.$();
          }
          static class $ {
              static $ of() {
                  return new $();
              }
          }
      }"

@timtebeek timtebeek changed the title 3623 $ as inner class Improved handling of inner class named $ in JavaTemplate Jan 30, 2024
@@ -126,7 +126,7 @@ private String substituteTypedPattern(String key, int index, TemplateParameterPa
if (arrayType.getElemType() instanceof JavaType.Primitive) {
s += ((JavaType.Primitive) arrayType.getElemType()).getKeyword();
} else if (arrayType.getElemType() instanceof JavaType.FullyQualified) {
s += ((JavaType.FullyQualified) arrayType.getElemType()).getFullyQualifiedName().replace("$", ".");
s += TypeUtils.toFullyQualifiedName(((JavaType.FullyQualified) arrayType.getElemType()).getFullyQualifiedName());
Copy link
Contributor

Choose a reason for hiding this comment

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

A JavaType.FullyQualified instance currently typically uses the binary name, which in the compiler's AST is represented by Symbol#flatName().

I think that instead of changing and using TypeUtils.toFullyQualifiedName() to get the fully qualified name of a instances implementing JavaType.FullyQualified we would be better off either changing the internal representation or deriving the name from the current representation.

Changing the current internal representation is trickier (with regards to backward compatibility), but adding a utility that can return the "source name" for a JavaType.FullyQualified instance should be possible by combining what is returned by getOwningClass() and by getFullyQualifiedName().

I am saying this because a class could also be called Foo$Bar if you wanted to and that is another $ we shouldn't be replacing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants