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.

Sign up for GitHub

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
Open

Improved handling of inner class named $ in JavaTemplate #3898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e755345
add reproducer for bug 3623
MBoegers Oct 16, 2023
109e681
Fix TypeUtils.toFullyQualifiedName to handle mixins of $
MBoegers Jan 9, 2024
9b48034
Use TypeUtils to determine FQN properly
MBoegers Jan 9, 2024
c0d6c08
remove unused imports
MBoegers Jan 9, 2024
b63e8c4
Update rewrite-java/src/main/java/org/openrewrite/java/tree/TypeUtils…
timtebeek Jan 26, 2024
0556bc6
Merge branch 'main' into 3623-$-as-inner-class
timtebeek Jan 26, 2024
be525f8
Merge branch 'openrewrite:main' into 3623-$-as-inner-class
MBoegers Jan 30, 2024
f08f565
Apply suggestions from code review
timtebeek Jan 30, 2024
b822f87
Update rewrite-java/src/main/java/org/openrewrite/java/tree/TypeUtils…
timtebeek Jan 30, 2024
36418d8
Merge branch 'main' into 3623-$-as-inner-class
timtebeek Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions rewrite-java-tck/src/main/java/org/openrewrite/java/tree/TypeUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package org.openrewrite.java.tree;

import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.openrewrite.InMemoryExecutionContext;
import org.openrewrite.Issue;
Expand All @@ -27,6 +28,7 @@
import static java.util.Collections.singletonList;
import static org.assertj.core.api.Assertions.assertThat;
import static org.openrewrite.java.Assertions.java;
import static org.openrewrite.java.tree.TypeUtils.toFullyQualifiedName;

@SuppressWarnings("ConstantConditions")
class TypeUtilsTest implements RewriteTest {
Expand Down Expand Up @@ -348,4 +350,43 @@ public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations
)
);
}

@Nested
class ToFQN {
@Test
void simpleClass() {
assertThat(toFullyQualifiedName("com.example.Test"))
.isEqualTo("com.example.Test");
}

@Test
void innerClass() {
assertThat(toFullyQualifiedName("com.example.Test$InnerTest"))
.isEqualTo("com.example.Test.InnerTest");
}

@Test
void className$() {
assertThat(toFullyQualifiedName("com.example.Test.$"))
.isEqualTo("com.example.Test.$");
}

@Test
void packageName$() {
assertThat(toFullyQualifiedName("com.example.$.Test"))
.isEqualTo("com.example.$.Test");
}

@Test
void packageName$WithInnerClass() {
assertThat(toFullyQualifiedName("com.example.$.Test$Inner"))
.isEqualTo("com.example.$.Test.Inner");
}

@Test
void innerClassWithName$() {
assertThat(toFullyQualifiedName("com.example.$.Test$$"))
.isEqualTo("com.example.$.Test.$");
}
}
}
62 changes: 62 additions & 0 deletions rewrite-java-test/src/test/java/org/openrewrite/java/JavaTemplateSubstitutionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,17 @@
import org.openrewrite.DocumentExample;
import org.openrewrite.ExecutionContext;
import org.openrewrite.Issue;
import org.openrewrite.Tree;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.JContainer;
import org.openrewrite.java.tree.JavaType;
import org.openrewrite.java.tree.Space;
import org.openrewrite.marker.Markers;
import org.openrewrite.test.RewriteTest;

import java.util.Objects;

import static java.util.Collections.emptyList;
import static org.openrewrite.java.Assertions.java;
import static org.openrewrite.test.RewriteTest.toRecipe;

Expand Down Expand Up @@ -73,6 +81,60 @@ int value() {
);
}

@DocumentExample
@Issue("https://github.com/openrewrite/rewrite/issues/3623")
@SuppressWarnings("InstantiationOfUtilityClass")
@Test
void anyForInnerClass() {
rewriteRun(
spec -> spec.recipe(toRecipe(() -> new JavaVisitor<>() {
@Override
public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) {
if (method.getSimpleName().equals("of")) {
J.Identifier $type = new J.Identifier(Tree.randomId(), Space.SINGLE_SPACE, Markers.EMPTY, emptyList(),
((JavaType.FullyQualified) Objects.requireNonNull(method.getType())).getClassName(),
method.getType(), null);

J.NewClass newClass = new J.NewClass(Tree.randomId(), Space.EMPTY, Markers.EMPTY, null,
Space.EMPTY, $type, JContainer.empty(), null, null);

return JavaTemplate.builder("#{any()}")
.contextSensitive()
.build()
.apply(getCursor(), method.getCoordinates().replace(), newClass);
}
return method;
}
})).cycles(1).expectedCyclesThatMakeChanges(1),
java(
"""
class A {
void foo() {
$ test = $.of();
}
static class $ {
static $ of() {
return new $();
}
}
}
""",
"""
class A {
void foo() {
$ test = new $();
}
static class $ {
static $ of() {
return new $();
}
}
}
"""
)
);
}

@SuppressWarnings("InfiniteRecursion")
@Test
void array() {
Expand Down
4 changes: 2 additions & 2 deletions rewrite-java/src/main/java/org/openrewrite/java/internal/template/Substitutions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}

s += "[0]" + extraDim + ")";
Expand All @@ -152,7 +152,7 @@ private String substituteTypedPattern(String key, int index, TemplateParameterPa
}
}

fqn = fqn.replace("$", ".");
fqn = TypeUtils.toFullyQualifiedName(fqn);

JavaType.Primitive primitive = JavaType.Primitive.fromKeyword(fqn);
s = "__P__." + (primitive == null || primitive.equals(JavaType.Primitive.String) ?
Expand Down
9 changes: 8 additions & 1 deletion rewrite-java/src/main/java/org/openrewrite/java/tree/TypeUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,14 @@ public static boolean isString(@Nullable JavaType type) {
}

public static String toFullyQualifiedName(String fqn) {
return fqn.replace('$', '.');
// replace every $ that is not prefixed by a dot in an iterative way to handle inner classes whose name is $
while (true) {
String nxt = fqn.replaceFirst("(?<!\\.)\\$", ".");
if (nxt.equals(fqn)) break;
else fqn = nxt;
}

return fqn;
}

public static boolean fullyQualifiedNamesAreEqual(@Nullable String fqn1, @Nullable String fqn2) {
Expand Down
Loading