[ggj][ast] fix: avoid type collisions - refactor {Concrete,Vapor}Ref into AstNodes

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);
 

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<Reference> generics) {
     return toBuilder().setGenerics(generics).build();
   }
 
+  public String simpleName() {
+    return clazz().getSimpleName();
+  }
+
   public static ConcreteReference withClazz(Class clazz) {
     return builder().setClazz(clazz).build();
   }

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<Reference> generics();
 
   String name();

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<Reference> generics();
 

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<String> staticImports = new TreeSet<>();
   private final Set<String> imports = new TreeSet<>();
 
+  // Cache the list of short names, since it will be used relatively often.
+  private final Set<String> 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<VariableExpr> expressions) {
     }
   }
 
-  private void references(List<Reference> 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<Reference> refs) {
+    for (Reference ref : refs) {
+      ref.accept(this);
     }
   }
 

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());