Fully qualify types in generic synthetic method return types (#341)

Previously, when a synthetic method with a generic return type had a
type parameter in the same package as the target method, it would not
keep its fully qualified name in its synthetic definition, leading to
compilation errors seen in na-791b.

For example, `ImmutableSet.copyOf` returned an `ImmutableSet<Feature>`,
but in its synthetic definition, `copyOf` returned
`com.google.common.collect.ImmutableSet<Feature>` instead of
`com.google.common.collect.ImmutableSet<com.github.benmanes.caffeine.cache.Feature>`.

src/main/java/org/checkerframework/specimin/JavaParserUtil.java

@@ -85,6 +85,16 @@ public static ResolvedReferenceType classOrInterfaceTypeToResolvedReferenceType(
     return type.resolve().asReferenceType();
   }
 
+  /**
+   * Erases type arguments from a method signature string.
+   *
+   * @param signature the signature
+   * @return the same signature without type arguments
+   */
+  public static String erase(String signature) {
+    return signature.replaceAll("<.*>", "");
+  }
+
   /**
    * Returns the corresponding type name for an Expression within an annotation.
    *

src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java

@@ -277,12 +277,12 @@ private boolean overridesAnInterfaceMethodImpl(
       // (i.e., a raw type). Note though that this doesn't mean there are no type variables in the
       // signature:
       // add(E) is still add(E).
-      targetSignature = erase(targetSignature);
+      targetSignature = JavaParserUtil.erase(targetSignature);
 
       for (ResolvedMethodDeclaration methodInInterface :
           resolvedInterface.getAllMethodsVisibleToInheritors()) {
         try {
-          if (erase(methodInInterface.getSignature()).equals(targetSignature)) {
+          if (JavaParserUtil.erase(methodInInterface.getSignature()).equals(targetSignature)) {
             if (methodInInterface.isAbstract()) {
               // once we've found the correct method, we return to whether we
               // control it or not. If we don't, it must be preserved. If we do, then we only
@@ -366,16 +366,6 @@ private static Collection<ResolvedReferenceType> getAllImplementations(
     return qualifiedNameToType.values();
   }
 
-  /**
-   * Erases type arguments from a method signature string.
-   *
-   * @param signature the signature
-   * @return the same signature without type arguments
-   */
-  private static String erase(String signature) {
-    return signature.replaceAll("<.*>", "");
-  }
-
   /**
    * Given a MethodDeclaration, this method returns the method that it overrides, if one exists in
    * one of its super classes. If one does not exist, it returns null.

src/main/java/org/checkerframework/specimin/UnsolvedSymbolVisitor.java

@@ -3598,18 +3598,83 @@ private String lookupFQNs(String javacType) {
             new ModifierVisitor<Void>() {
               @Override
               public Visitable visit(ClassOrInterfaceType type, Void p) {
-                if (classAndPackageMap.containsKey(type.asString())) {
-                  return new ClassOrInterfaceType(
-                      classAndPackageMap.get(type.asString()) + "." + type.asString());
-                } else {
+                StringBuilder fullyQualifiedName = new StringBuilder();
+                if (classAndPackageMap.containsKey(JavaParserUtil.erase(type.asString()))) {
+                  fullyQualifiedName.append(classAndPackageMap.get(type.asString()));
+                  fullyQualifiedName.append(".");
+                } else if (!type.getTypeArguments().isPresent()) {
+                  // This type is in the same package and doesn't contain any type parameters
                   return super.visit(type, p);
                 }
+
+                NodeList<Type> typeArguments = type.getTypeArguments().orElse(null);
+
+                if (typeArguments != null) {
+                  fullyQualifiedName.append(
+                      type.asString().substring(0, type.asString().indexOf('<') + 1));
+
+                  for (int i = 0; i < typeArguments.size(); i++) {
+                    Type typeArgument = typeArguments.get(i);
+                    lookupTypeArgumentFQN(fullyQualifiedName, typeArgument);
+
+                    if (i < typeArguments.size() - 1) {
+                      fullyQualifiedName.append(", ");
+                    }
+                  }
+                  fullyQualifiedName.append(">");
+                } else {
+                  fullyQualifiedName.append(type.asString());
+                }
+
+                return StaticJavaParser.parseClassOrInterfaceType(fullyQualifiedName.toString());
               }
             },
             null);
     return typeVarDecl + parsedJavac.toString();
   }
 
+  /**
+   * Helper method for lookupFQNs which adds the fully qualified type argument to
+   * fullyQualifiedName.
+   *
+   * @param fullyQualifiedName the fully qualified name to build
+   * @param typeArgument the type argument to lookup
+   */
+  private void lookupTypeArgumentFQN(StringBuilder fullyQualifiedName, Type typeArgument) {
+    String erased = JavaParserUtil.erase(typeArgument.asString());
+    if (classAndPackageMap.containsKey(erased)) {
+      fullyQualifiedName
+          .append(classAndPackageMap.get(erased))
+          .append(".")
+          .append(typeArgument.asString());
+    } else if (JavaLangUtils.isJavaLangName(erased)) {
+      // Keep java.lang type arguments as is (Integer, String, etc.)
+      fullyQualifiedName.append(typeArgument.asString());
+    } else if (typeArgument.isWildcardType()) {
+      WildcardType asWildcardType = typeArgument.asWildcardType();
+
+      if (asWildcardType.getSuperType().isPresent()) {
+        fullyQualifiedName.append("? super ");
+        lookupTypeArgumentFQN(fullyQualifiedName, asWildcardType.getSuperType().get());
+      } else if (asWildcardType.getExtendedType().isPresent()) {
+        fullyQualifiedName.append("? extends ");
+        lookupTypeArgumentFQN(fullyQualifiedName, asWildcardType.getExtendedType().get());
+      }
+    } else if (isAClassPath(erased)) {
+      // If it's already a fully qualified name, don't do anything
+      fullyQualifiedName.append(typeArgument.asString());
+    } else {
+      // If it's not imported, it's probably in the same package
+      // TODO: handle already fully qualified generic type arguments. Right now JavaTypeCorrect
+      // only outputs the simple class name, so there is no way to get its fully qualified name
+      // here without ambiguity (i.e. org.example.Foo and com.example.Foo have different signatures)
+      // with the same simple class name
+      // Check MethodReturnFullyQualifiedGenericTest for more details; the current return type in
+      // Bar.java is com.example.InOtherPackage2 rather than com.foo.InOtherPackage2 (expected)
+      fullyQualifiedName.append(currentPackage).append(".").append(typeArgument.toString());
+    }
+  }
+
   /**
    * If and only if synthetic classes exist for both input type names, this method migrates all the
    * content of the sythetic class for the incorrect type name to the synthetic class for the

src/main/resources/min_program_compile_status.json

@@ -28,5 +28,5 @@
   "na-389": "PASS",
   "na-705": "PASS",
   "na-791a": "PASS",
-  "na-791b": "FAIL"
+  "na-791b": "PASS"
 }

src/test/java/org/checkerframework/specimin/MethodReturnFullyQualifiedGenericTest.java

@@ -0,0 +1,21 @@
+package org.checkerframework.specimin;
+
+import java.io.IOException;
+import org.junit.Test;
+
+/**
+ * This test checks that Specimin includes the fully qualified class name in generics when
+ * generating a synthetic method. For example, if a return type is Bar<com.foo.Foo>, the actual
+ * synthetic return type should be com.qualified.Bar<com.foo.Foo>. This test encodes Specimin's
+ * current behavior, which doesn't produce compilable output. If you break this test, it might 
+ * not be a bad thing.
+ */
+public class MethodReturnFullyQualifiedGenericTest {
+  @Test
+  public void runTest() throws IOException {
+    SpeciminTestExecutor.runTestWithoutJarPaths(
+        "methodreturnfullyqualifiedgeneric",
+        new String[] {"com/example/Simple.java"},
+        new String[] {"com.example.Simple#alreadyQualified()"});
+  }
+}

src/test/java/org/checkerframework/specimin/MethodReturnGenericTest.java

@@ -0,0 +1,23 @@
+package org.checkerframework.specimin;
+
+import java.io.IOException;
+import org.junit.Test;
+
+/**
+ * This test checks that Specimin includes the fully qualified class name in generics when
+ * generating a synthetic method. For example, if a return type is Foo<Bar>, Specimin should always
+ * generate the synthetic method return type as com.example.Foo<com.other.Bar>.
+ */
+public class MethodReturnGenericTest {
+  @Test
+  public void runTest() throws IOException {
+    SpeciminTestExecutor.runTestWithoutJarPaths(
+        "methodreturngeneric",
+        new String[] {"com/example/Simple.java"},
+        new String[] {
+          "com.example.Simple#foo()",
+          "com.example.Simple#bar()",
+          "com.example.Simple#baz()"
+        });
+  }
+}

src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/bar/Bar.java

@@ -0,0 +1,8 @@
+package com.bar;
+
+public class Bar<T> {
+
+    public static com.bar.Bar<com.example.InOtherPackage2> getOtherPackage2() {
+        throw new Error();
+    }
+}

src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/example/Simple.java

@@ -0,0 +1,10 @@
+package com.example;
+
+import com.bar.Bar;
+
+public class Simple {
+
+    public Bar<com.foo.InOtherPackage2> alreadyQualified() {
+        return Bar.getOtherPackage2();
+    }
+}

src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/foo/InOtherPackage2.java

@@ -0,0 +1,4 @@
+package com.foo;
+
+public class InOtherPackage2 {
+}

src/test/resources/methodreturnfullyqualifiedgeneric/input/com/example/Simple.java

@@ -0,0 +1,9 @@
+package com.example;
+
+import com.bar.Bar;
+
+public class Simple {
+    public Bar<com.foo.InOtherPackage2> alreadyQualified() {
+        return Bar.getOtherPackage2();
+    }
+}

src/test/resources/methodreturngeneric/expected/com/bar/Bar.java

@@ -0,0 +1,16 @@
+package com.bar;
+
+public class Bar<T> {
+
+    public static com.bar.Bar<com.foo.InOtherPackage> getOtherPackage() {
+        throw new Error();
+    }
+
+    public static com.bar.Bar<com.example.InSamePackage> getSamePackage() {
+        throw new Error();
+    }
+
+    public static com.bar.Bar<Integer> getJavaLang() {
+        throw new Error();
+    }
+}

src/test/resources/methodreturngeneric/expected/com/example/InSamePackage.java

@@ -0,0 +1,4 @@
+package com.example;
+
+public class InSamePackage {
+}

src/test/resources/methodreturngeneric/expected/com/example/Simple.java

@@ -0,0 +1,19 @@
+package com.example;
+
+import com.bar.Bar;
+import com.foo.InOtherPackage;
+
+public class Simple {
+
+    public Bar<InOtherPackage> foo() {
+        return Bar.getOtherPackage();
+    }
+
+    public Bar<InSamePackage> bar() {
+        return Bar.getSamePackage();
+    }
+
+    public Bar<Integer> baz() {
+        return Bar.getJavaLang();
+    }
+}

src/test/resources/methodreturngeneric/expected/com/foo/InOtherPackage.java

@@ -0,0 +1,4 @@
+package com.foo;
+
+public class InOtherPackage {
+}

src/test/resources/methodreturngeneric/input/com/example/Simple.java

@@ -0,0 +1,18 @@
+package com.example;
+
+import com.bar.Bar;
+import com.foo.InOtherPackage;
+
+public class Simple {
+    public Bar<InOtherPackage> foo() {
+        return Bar.getOtherPackage();
+    }
+
+    public Bar<InSamePackage> bar() {
+        return Bar.getSamePackage();
+    }
+
+    public Bar<Integer> baz() {
+        return Bar.getJavaLang();
+    }
+}

typecheck_one_test.sh

@@ -14,6 +14,8 @@ if [ "${testcase}" = "superinterfaceextends" ]; then exit 0; fi
 # incomplete handling of method references: https://github.com/njit-jerse/specimin/issues/291
 # this test exists to check that no crash occurs, not that Specimin produces the correct output
 if [ "${testcase}" = "methodref2" ]; then exit 0; fi
+# this test will not compile right now; this is a TODO in UnsolvedSymbolVisitor#lookupTypeArgumentFQN
+if [ "${testcase}" = "methodreturnfullyqualifiedgeneric" ]; then exit 0; fi
 cd "${testcase}/expected/" || exit 2
 # javac relies on word splitting
 # shellcheck disable=SC2046

typecheck_test_outputs.bat

@@ -20,6 +20,8 @@ for /d %%t in (*) do (
   rem incomplete handling of method references: https://github.com/njit-jerse/specimin/issues/291
   rem this test exists to check that no crash occurs, not that Specimin produces the correct output
   if "%%t"=="methodref2" set continue=1
+  rem this test will not compile right now; this is a TODO in UnsolvedSymbolVisitor#lookupTypeArgumentFQN
+  if "%%t"=="methodreturnfullyqualifiedgeneric" set continue=1
   if !continue!==0 (
     cd "%%t/expected/" || exit 1
     rem javac relies on word splitting