diff --git a/src/main/java/com/google/api/generator/engine/ast/AstNodeVisitor.java b/src/main/java/com/google/api/generator/engine/ast/AstNodeVisitor.java index 14f75eb452..b13213a311 100644 --- a/src/main/java/com/google/api/generator/engine/ast/AstNodeVisitor.java +++ b/src/main/java/com/google/api/generator/engine/ast/AstNodeVisitor.java @@ -24,6 +24,10 @@ public interface AstNodeVisitor { public void visit(AnnotationNode annotation); + public void visit(ConcreteReference reference); + + public void visit(VaporReference reference); + /** =============================== EXPRESSIONS =============================== */ public void visit(ValueExpr valueExpr); diff --git a/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java b/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java index 64f34d1927..4cf5728648 100644 --- a/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java +++ b/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java @@ -37,6 +37,11 @@ public abstract class ConcreteReference implements Reference { // Private. abstract Class clazz(); + @Override + public void accept(AstNodeVisitor visitor) { + visitor.visit(this); + } + @Nullable @Override public abstract Reference wildcardUpperBound(); @@ -188,6 +193,10 @@ public Reference copyAndSetGenerics(List generics) { return toBuilder().setGenerics(generics).build(); } + public String simpleName() { + return clazz().getSimpleName(); + } + public static ConcreteReference withClazz(Class clazz) { return builder().setClazz(clazz).build(); } diff --git a/src/main/java/com/google/api/generator/engine/ast/Reference.java b/src/main/java/com/google/api/generator/engine/ast/Reference.java index 7031b5cbe6..a0b9325f5d 100644 --- a/src/main/java/com/google/api/generator/engine/ast/Reference.java +++ b/src/main/java/com/google/api/generator/engine/ast/Reference.java @@ -18,7 +18,9 @@ import java.util.List; import javax.annotation.Nullable; -public interface Reference { +public interface Reference extends AstNode { + void accept(AstNodeVisitor visitor); + ImmutableList generics(); String name(); diff --git a/src/main/java/com/google/api/generator/engine/ast/VaporReference.java b/src/main/java/com/google/api/generator/engine/ast/VaporReference.java index 0461fabab4..a522582734 100644 --- a/src/main/java/com/google/api/generator/engine/ast/VaporReference.java +++ b/src/main/java/com/google/api/generator/engine/ast/VaporReference.java @@ -29,6 +29,11 @@ public abstract class VaporReference implements Reference { private static final String RIGHT_ANGLE = ">"; private static final String COMMA = ", "; + @Override + public void accept(AstNodeVisitor visitor) { + visitor.visit(this); + } + @Override public abstract ImmutableList generics(); diff --git a/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java b/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java index 557c43c447..a6a5b1bc8f 100644 --- a/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java +++ b/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java @@ -26,6 +26,7 @@ import com.google.api.generator.engine.ast.CastExpr; import com.google.api.generator.engine.ast.ClassDefinition; import com.google.api.generator.engine.ast.CommentStatement; +import com.google.api.generator.engine.ast.ConcreteReference; import com.google.api.generator.engine.ast.EmptyLineStatement; import com.google.api.generator.engine.ast.EnumRefExpr; import com.google.api.generator.engine.ast.Expr; @@ -55,6 +56,7 @@ import com.google.api.generator.engine.ast.TypeNode; import com.google.api.generator.engine.ast.UnaryOperationExpr; import com.google.api.generator.engine.ast.ValueExpr; +import com.google.api.generator.engine.ast.VaporReference; import com.google.api.generator.engine.ast.VariableExpr; import com.google.api.generator.engine.ast.WhileStatement; import com.google.common.base.Preconditions; @@ -65,6 +67,7 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; +import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -76,6 +79,9 @@ public class ImportWriterVisitor implements AstNodeVisitor { private final Set staticImports = new TreeSet<>(); private final Set imports = new TreeSet<>(); + // Cache the list of short names, since it will be used relatively often. + private final Set importShortNames = new TreeSet<>(); + private String currentPackage; @Nullable private String currentClassName; @@ -109,6 +115,24 @@ public String write() { return sb.toString(); } + public boolean collidesWithImport(String pakkage, String shortName) { + // This is a sufficiently-good heuristic since it's unlikely that the AST structure has changed + // if the size is the same. + if (importShortNames.size() != imports.size()) { + importShortNames.clear(); + importShortNames.addAll( + imports.stream() + .map(s -> s.substring(s.lastIndexOf(DOT) + 1)) + .collect(Collectors.toSet())); + } + return importShortNames.contains(shortName) + && imports.stream() + .filter(s -> s.equals(String.format("%s.%s", pakkage, shortName))) + .findFirst() + .orElse("") + .isEmpty(); + } + @Override public void visit(IdentifierNode identifier) { // Nothing to do. @@ -127,6 +151,16 @@ public void visit(TypeNode type) { references(refs); } + @Override + public void visit(ConcreteReference reference) { + handleReference(reference); + } + + @Override + public void visit(VaporReference reference) { + handleReference(reference); + } + @Override public void visit(ScopeNode scope) { // Nothing to do. @@ -410,44 +444,49 @@ private void variableExpressions(List expressions) { } } - private void references(List refs) { - for (Reference ref : refs) { - // Don't need to import this. - if (ref.useFullName()) { - continue; - } - if (!ref.isStaticImport() - && (ref.isFromPackage(PKG_JAVA_LANG) || ref.isFromPackage(currentPackage))) { - continue; - } + private void handleReference(Reference reference) { + // Don't need to import this. + if (reference.useFullName()) { + return; + } + if (!reference.isStaticImport() + && (reference.isFromPackage(PKG_JAVA_LANG) || reference.isFromPackage(currentPackage))) { + return; + } - if (ref.isWildcard()) { - if (ref.wildcardUpperBound() != null) { - references(Arrays.asList(ref.wildcardUpperBound())); - } - continue; + if (reference.isWildcard()) { + if (reference.wildcardUpperBound() != null) { + references(Arrays.asList(reference.wildcardUpperBound())); } + return; + } - if (ref.isStaticImport() - && !Strings.isNullOrEmpty(currentClassName) - && !ref.enclosingClassNames().isEmpty() - && ref.enclosingClassNames().contains(currentClassName)) { - continue; - } + if (reference.isStaticImport() + && !Strings.isNullOrEmpty(currentClassName) + && !reference.enclosingClassNames().isEmpty() + && reference.enclosingClassNames().contains(currentClassName)) { + return; + } - if (ref.isStaticImport()) { - // This is a static import. - staticImports.add(ref.fullName()); + if (reference.isStaticImport()) { + // This is a static import. + staticImports.add(reference.fullName()); + } else { + if (reference.hasEnclosingClass()) { + imports.add( + String.format( + "%s.%s", reference.pakkage(), String.join(DOT, reference.enclosingClassNames()))); } else { - if (ref.hasEnclosingClass()) { - imports.add( - String.format("%s.%s", ref.pakkage(), String.join(DOT, ref.enclosingClassNames()))); - } else { - imports.add(ref.fullName()); - } + imports.add(reference.fullName()); } + } - references(ref.generics()); + references(reference.generics()); + } + + private void references(List refs) { + for (Reference ref : refs) { + ref.accept(this); } } diff --git a/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java b/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java index 77801d69cd..f2984f36ad 100644 --- a/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java +++ b/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java @@ -26,6 +26,7 @@ import com.google.api.generator.engine.ast.CastExpr; import com.google.api.generator.engine.ast.ClassDefinition; import com.google.api.generator.engine.ast.CommentStatement; +import com.google.api.generator.engine.ast.ConcreteReference; import com.google.api.generator.engine.ast.EmptyLineStatement; import com.google.api.generator.engine.ast.EnumRefExpr; import com.google.api.generator.engine.ast.Expr; @@ -43,6 +44,7 @@ import com.google.api.generator.engine.ast.NewObjectExpr; import com.google.api.generator.engine.ast.OperatorKind; import com.google.api.generator.engine.ast.PackageInfoDefinition; +import com.google.api.generator.engine.ast.Reference; import com.google.api.generator.engine.ast.ReferenceConstructorExpr; import com.google.api.generator.engine.ast.RelationalOperationExpr; import com.google.api.generator.engine.ast.ReturnExpr; @@ -56,6 +58,7 @@ import com.google.api.generator.engine.ast.TypeNode.TypeKind; import com.google.api.generator.engine.ast.UnaryOperationExpr; import com.google.api.generator.engine.ast.ValueExpr; +import com.google.api.generator.engine.ast.VaporReference; import com.google.api.generator.engine.ast.Variable; import com.google.api.generator.engine.ast.VariableExpr; import com.google.api.generator.engine.ast.WhileStatement; @@ -145,23 +148,15 @@ public void visit(IdentifierNode identifier) { @Override public void visit(TypeNode type) { TypeKind typeKind = type.typeKind(); - StringBuilder generatedCodeBuilder = new StringBuilder(); if (type.isPrimitiveType()) { - generatedCodeBuilder.append(typeKind.toString().toLowerCase()); + buffer.append(typeKind.toString().toLowerCase()); } else { - if (type.reference().useFullName()) { - generatedCodeBuilder.append(type.reference().pakkage()); - generatedCodeBuilder.append(DOT); - } - // A null pointer exception will be thrown if reference is null, which is WAI. - generatedCodeBuilder.append(type.reference().name()); + type.reference().accept(this); } if (type.isArray()) { - generatedCodeBuilder.append("[]"); + buffer.append("[]"); } - - buffer.append(generatedCodeBuilder.toString()); } @Override @@ -181,6 +176,68 @@ public void visit(AnnotationNode annotation) { newline(); } + @Override + public void visit(ConcreteReference reference) { + if (reference.isWildcard()) { + buffer.append(QUESTION_MARK); + if (reference.wildcardUpperBound() != null) { + // Handle the upper bound. + buffer.append(SPACE); + buffer.append(EXTENDS); + buffer.append(SPACE); + reference.wildcardUpperBound().accept(this); + } + return; + } + String pakkage = reference.pakkage(); + String shortName = reference.name(); + if (reference.useFullName() || importWriterVisitor.collidesWithImport(pakkage, shortName)) { + buffer.append(pakkage); + buffer.append(DOT); + } + + if (reference.hasEnclosingClass() && !reference.isStaticImport()) { + buffer.append(String.join(DOT, reference.enclosingClassNames())); + buffer.append(DOT); + } + + buffer.append(reference.simpleName()); + + if (!reference.generics().isEmpty()) { + buffer.append(LEFT_ANGLE); + for (int i = 0; i < reference.generics().size(); i++) { + Reference r = reference.generics().get(i); + r.accept(this); + if (i < reference.generics().size() - 1) { + buffer.append(COMMA); + buffer.append(SPACE); + } + } + buffer.append(RIGHT_ANGLE); + } + } + + @Override + public void visit(VaporReference reference) { + // This implementation should be more extensive, but there are no existing use cases that + // exercise the edge cases. + // TODO(miraleung): Give this behavioral parity with ConcreteReference. + String pakkage = reference.pakkage(); + String shortName = reference.name(); + + if (reference.useFullName() || importWriterVisitor.collidesWithImport(pakkage, shortName)) { + buffer.append(pakkage); + buffer.append(DOT); + if (reference.hasEnclosingClass()) { + buffer.append(String.join(DOT, reference.enclosingClassNames())); + buffer.append(DOT); + } + } + + // A null pointer exception will be thrown if reference is null, which is WAI. + buffer.append(shortName); + } + /** =============================== EXPRESSIONS =============================== */ @Override public void visit(ValueExpr valueExpr) { @@ -798,6 +855,7 @@ public void visit(ClassDefinition classDefinition) { newline(); } + // This must go first, so that we can check for type collisions. classDefinition.accept(importWriterVisitor); if (!classDefinition.isNested()) { buffer.append(importWriterVisitor.write()); diff --git a/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden b/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden index f1bdaeeff7..cae284d0c9 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden @@ -281,8 +281,8 @@ public class TestingClientTest { @Test public void getTestTest() throws Exception { - Test expectedResponse = - Test.newBuilder() + com.google.showcase.v1beta1.Test expectedResponse = + com.google.showcase.v1beta1.Test.newBuilder() .setName(TestName.of("[SESSION]", "[SHARD_ID]", "[TEST_ID]").toString()) .setDescription("description-1724546052") .build(); @@ -290,7 +290,7 @@ public class TestingClientTest { TestName name = TestName.of("[SESSION]", "[SHARD_ID]", "[TEST_ID]"); - Test actualResponse = client.getTest(name); + com.google.showcase.v1beta1.Test actualResponse = client.getTest(name); Assert.assertEquals(expectedResponse, actualResponse); List actualRequests = mockTesting.getRequests(); @@ -320,8 +320,8 @@ public class TestingClientTest { @Test public void getTestTest2() throws Exception { - Test expectedResponse = - Test.newBuilder() + com.google.showcase.v1beta1.Test expectedResponse = + com.google.showcase.v1beta1.Test.newBuilder() .setName(TestName.of("[SESSION]", "[SHARD_ID]", "[TEST_ID]").toString()) .setDescription("description-1724546052") .build(); @@ -329,7 +329,7 @@ public class TestingClientTest { String name = "name3373707"; - Test actualResponse = client.getTest(name); + com.google.showcase.v1beta1.Test actualResponse = client.getTest(name); Assert.assertEquals(expectedResponse, actualResponse); List actualRequests = mockTesting.getRequests(); @@ -359,7 +359,8 @@ public class TestingClientTest { @Test public void listTestsTest() throws Exception { - Test responsesElement = Test.newBuilder().build(); + com.google.showcase.v1beta1.Test responsesElement = + com.google.showcase.v1beta1.Test.newBuilder().build(); ListTestsResponse expectedResponse = ListTestsResponse.newBuilder() .setNextPageToken("") @@ -376,7 +377,8 @@ public class TestingClientTest { ListTestsPagedResponse pagedListResponse = client.listTests(request); - List resources = Lists.newArrayList(pagedListResponse.iterateAll()); + List resources = + Lists.newArrayList(pagedListResponse.iterateAll()); Assert.assertEquals(1, resources.size()); Assert.assertEquals(expectedResponse.getTestsList().get(0), resources.get(0));