Skip to content
This repository has been archived by the owner on Oct 10, 2024. It is now read-only.

Fix #860: Nested Type name clashes within same package #879

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 10 additions & 2 deletions src/main/java/com/squareup/javapoet/CodeWriter.java
Original file line number Diff line number Diff line change
Expand Up @@ -401,8 +401,16 @@ String lookupName(ClassName className) {

// If the class is in the same package, we're done.
if (Objects.equals(packageName, className.packageName())) {
referencedNames.add(topLevelSimpleName);
return join(".", className.simpleNames());
String conflictClassName = packageName+"."+className.simpleName;//From same package (Nest)

Choose a reason for hiding this comment

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

Thanks for the fix! but what do you need this line for? It is already checked that packageName equal to className.packageName(), so packageName+"."+className.simpleName is equivalent to className.packageName() +"."+className.simpleName, right? Then you don't need the comparison to canonicalName below, because it is always true

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the fix! but what do you need this line for? It is already checked that packageName equal to className.packageName(), so packageName+"."+className.simpleName is equivalent to className.packageName() +"."+className.simpleName, right? Then you don't need the comparison to canonicalName below, because it is always true

Well, actually the conflictClassName is not always equal to the className.canonicalName, because of the existence of super class. The ignorance of trim only occur when the class is alwaysQualify and canonicalName equals, that is ,the class share a same name with a subclass but canonical name doesn't act like that. For more detail about the issue described situation, you can go to the avoidClashesWithNestedClasses_SamePackageTest in JavaFileTest

//For nested conflict situation, the simpleName should be in QualifyList
//But its full name dont contains its super name
if(alwaysQualify.contains(className.simpleName)
&&Objects.equals(conflictClassName,className.canonicalName)){

}else{//If not, do regular trim
referencedNames.add(topLevelSimpleName);
return join(".", className.simpleNames());
}
}

// We'll have to use the fully-qualified name. Mark the type as importable for a future pass.
Expand Down
5 changes: 5 additions & 0 deletions src/test/java/com/squareup/javapoet/FooInterface.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package com.squareup.javapoet;

public interface FooInterface {
public class Nest{};
}
48 changes: 48 additions & 0 deletions src/test/java/com/squareup/javapoet/JavaFileTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,50 @@ private TypeSpec importStaticTypeSpec(String name) {
+ "}\n");
}

@Test public void avoidClashesWithNestedClasses_SamePackageTest() {
String source = JavaFile.builder("com.squareup.javapoet",
TypeSpec.classBuilder("Taco")
// The external Name should be full
.addField(ClassName.get(com.squareup.javapoet.Nest.class), "external_Nest")
// The internal Name would be better with super name
.addField(ClassName.get(com.squareup.javapoet.FooInterface.Nest.class), "interface_Nest")
.addSuperinterface(com.squareup.javapoet.FooInterface.class)
.build())
.build()
.toString();
assertThat(source).isEqualTo("package com.squareup.javapoet;\n" +
"\n" +
"class Taco implements FooInterface {\n" +
" com.squareup.javapoet.Nest external_Nest;\n" +
"\n" +
" FooInterface.Nest interface_Nest;\n" +
"}"+"\n");
}

@Test public void avoidClashesWithNestedClasses_InterExterPackageTest() {
String source = JavaFile.builder("com.squareup.javapoet",
TypeSpec.classBuilder("Taco")
// The external Name should be full
.addField(ClassName.get(com.squareup.javapoet.Nest.class), "external_Nest")
// The internal Name would be better with super name
.addField(ClassName.get(com.squareup.javapoet.FooInterface.Nest.class), "interface_Nest")
// The external Name from external package
.addField(ClassName.get("other","Nest"), "interface_Nest")
.addSuperinterface(com.squareup.javapoet.FooInterface.class)
.build())
.build()
.toString();
assertThat(source).isEqualTo("package com.squareup.javapoet;\n" +
"\n" +
"class Taco implements FooInterface {\n" +
" com.squareup.javapoet.Nest external_Nest;\n" +
"\n" +
" FooInterface.Nest interface_Nest;\n" +
"\n" +
" other.Nest interface_Nest;\n" +
"}"+"\n");
}

@Test public void avoidClashesWithNestedClasses_viaClass() {
String source = JavaFile.builder("com.squareup.tacos",
TypeSpec.classBuilder("Taco")
Expand Down Expand Up @@ -876,6 +920,10 @@ class NestedTypeB {
}
}

static class NestedTypeA{

}

private TypeSpec.Builder childTypeBuilder() {
return TypeSpec.classBuilder("Child")
.addMethod(MethodSpec.methodBuilder("optionalString")
Expand Down
4 changes: 4 additions & 0 deletions src/test/java/com/squareup/javapoet/Nest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package com.squareup.javapoet;

public class Nest {
}