Add guardrails to prevent and correct wrong output from JavaTypeCorre…

…ct (#339)

~~Incorporates #338, so don't review until that is merged.~~

~~I'm not sure if we want to merge this. It avoids some bad behavior on
the NA-176 test, and we could probably expand it in the future for
other, similar kinds of problems. However, it definitely does not
address the root cause: why is Specimin putting itself in a situation
where javac insists that a synthetic class must be compatible with
String?~~

~~I'm going to continue my investigation, but I'm leaving this PR here
because I may want to merge it later if the fix for the root cause is
too complex and/or impracticle.~~

This PR makes a series of changes to how our javac type correction
algorithm (in the `JavaTypeCorrect` class) handles complex cases. That
algorithm is fundamentally imprecise, so the changes in this PR should
be thought of as incremental improvements to the existing design that
avoid some of its worst pathologies. In particular, this PR:
* prevents JavaTypeCorrect from ever attempting to make a class extend
`java.lang.String`, which is final;
* makes it generally more difficult for the `SyntheticUnconstrainedType`
type variable to propagate beyond the return type to which it is
applied; and,
* replaces parameters whose types are synthetic return types that get
replaced by `SyntheticUnconstrainedType` with `java.lang.Object`.
Previously, these types would hang around and make the output
non-compilable, since they'd been marked as unused.

Together, these changes make NullAway-176 reproducible, based on my
local testing.

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

@@ -43,6 +43,12 @@ private JavaLangUtils() {
   /** Internal set for the names of the primitive types. */
   private static final Set<String> primitives = new HashSet<>(8);
 
+  /**
+   * This is an (incomplete) list of classes that we know are final in the JDK. The idea is that
+   * Specimin should never try to extend a class in this list.
+   */
+  private static final Set<String> knownFinalJdkTypes = new HashSet<>();
+
   static {
     primitives.add("int");
     primitives.add("short");
@@ -178,6 +184,36 @@ private JavaLangUtils() {
     javaLangClassesAndInterfaces.add("VirtualMachineError");
     javaLangClassesAndInterfaces.add("Void");
     javaLangClassesAndInterfaces.add("WrongThreadException");
+
+    // I made this list by going through the members of
+    // java.lang and checking which were final classes. We
+    // can do the same for other packages as needed.
+    knownFinalJdkTypes.add("String");
+    knownFinalJdkTypes.add("Class");
+    knownFinalJdkTypes.add("Integer");
+    knownFinalJdkTypes.add("Byte");
+    knownFinalJdkTypes.add("Short");
+    knownFinalJdkTypes.add("Long");
+    knownFinalJdkTypes.add("Double");
+    knownFinalJdkTypes.add("Float");
+    knownFinalJdkTypes.add("Character");
+    knownFinalJdkTypes.add("Character.UnicodeBlock");
+    knownFinalJdkTypes.add("Boolean");
+    knownFinalJdkTypes.add("Compiler");
+    knownFinalJdkTypes.add("Math");
+    knownFinalJdkTypes.add("ProcessBuilder");
+    knownFinalJdkTypes.add("RuntimePermission");
+    knownFinalJdkTypes.add("StackTraceElement");
+    knownFinalJdkTypes.add("StrictMath");
+    knownFinalJdkTypes.add("StringBuffer");
+    knownFinalJdkTypes.add("StringBuilder");
+    knownFinalJdkTypes.add("System");
+    knownFinalJdkTypes.add("Void");
+    Set<String> withJavaLang = new HashSet<>(knownFinalJdkTypes.size());
+    for (String s : knownFinalJdkTypes) {
+      withJavaLang.add("java.lang." + s);
+    }
+    knownFinalJdkTypes.addAll(withJavaLang);
   }
 
   /** The integral primitives. */
@@ -296,4 +332,14 @@ public static boolean inJdkPackage(String qualifiedName) {
         || qualifiedName.startsWith("com.sun.")
         || qualifiedName.startsWith("jdk.");
   }
+
+  /**
+   * Could the given name be a final class from the JDK, like String?
+   *
+   * @param name a simple or fully-qualified name
+   * @return true if the input might be a final JDK class
+   */
+  public static boolean isFinalJdkClass(String name) {
+    return knownFinalJdkTypes.contains(name);
+  }
 }

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

@@ -61,6 +61,9 @@ class JavaTypeCorrect {
    */
   private Map<String, String> classAndUnresolvedInterface = new HashMap<>();
 
+  /** The name used for a synthetic, unconstrained type variable. */
+  public static final String SYNTHETIC_UNCONSTRAINED_TYPE = "SyntheticUnconstrainedType";
+
   /**
    * Create a new JavaTypeCorrect instance. The directories of files in fileNameList are relative to
    * rootDirectory, and rootDirectory is an absolute path
@@ -104,6 +107,23 @@ public Map<String, String> getClassAndUnresolvedInterface() {
    * @return the map described above.
    */
   public Map<String, String> getExtendedTypes() {
+    // Before returning, purge any entries that are obviously bad according to
+    // the following simple heuristic(s):
+    // * don't extend known-final classes from the JDK, like java.lang.String.
+    // * don't add change types to "SyntheticUnconstrainedType"
+    Set<String> toRemove = new HashSet<>(0);
+    for (Map.Entry<String, String> entry : extendedTypes.entrySet()) {
+      if (JavaLangUtils.isFinalJdkClass(entry.getValue())) {
+        toRemove.add(entry.getKey());
+      }
+      // Don't let errors related sythetic unconstrained types added by Specimin propagate.
+      if (entry.getValue().equals(SYNTHETIC_UNCONSTRAINED_TYPE)) {
+        toRemove.add(entry.getKey());
+      }
+    }
+    for (String s : toRemove) {
+      extendedTypes.remove(s);
+    }
     return extendedTypes;
   }
 
@@ -455,12 +475,13 @@ private void changeType(String incorrectType, String correctType) {
           // continue with our main fix strategy
           return;
         } else {
-          // we require a GLB: that is, this synthetic return type needs to _used_ in
+          // we require a GLB: that is, this synthetic return type needs to be _used_ in
           // two different contexts: one where correctType is required, and another
           // where otherCorrectType is required. Instead of worrying about making a correct GLB,
           // instead just use an unconstrained type variable.
           typeToChange.put(
-              incorrectType, "<SyntheticUnconstrainedType> SyntheticUnconstrainedType");
+              incorrectType,
+              "<" + SYNTHETIC_UNCONSTRAINED_TYPE + "> " + SYNTHETIC_UNCONSTRAINED_TYPE);
           return;
         }
       }

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

@@ -324,19 +324,18 @@ public Visitable visit(EnumConstantDeclaration enumConstantDeclaration, Void p)
 
   @Override
   public Visitable visit(MethodDeclaration methodDecl, Void p) {
+    String signature;
     try {
       // resolved() will only check if the return type is solvable
       // getQualifiedSignature() will also check if the parameters are solvable
-      methodDecl.resolve().getQualifiedSignature();
+      signature = methodDecl.resolve().getQualifiedSignature();
     } catch (UnsolvedSymbolException e) {
       // The current class is employed by the target methods, although not all of its members are
       // utilized. It's not surprising for unused members to remain unresolved.
       methodDecl.remove();
       return methodDecl;
     }
 
-    ResolvedMethodDeclaration resolved = methodDecl.resolve();
-    String signature = resolved.getQualifiedSignature();
     if (targetMethods.contains(signature)) {
       return super.visit(methodDecl, p);
     }

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

@@ -503,17 +503,25 @@ public Visitable visit(MethodCallExpr call, Void p) {
             resolvedYetStuckMethodCall.add(
                 this.currentClassQualifiedName + "." + call.getNameAsString());
           } else {
-            // Use the scope instead. There are two cases: the scope is an FQN (e.g., in
-            // a call to a fully-qualified static method) or the scope is a simple name.
-            // In the simple name case, append the current package to the front, since
-            // if it had been imported we wouldn't be in this situation.
-            if (UnsolvedSymbolVisitor.isAClassPath(scopeAsString)) {
-              resolvedYetStuckMethodCall.add(scopeAsString + "." + call.getNameAsString());
-              usedTypeElements.add(scopeAsString);
-            } else {
-              resolvedYetStuckMethodCall.add(
-                  getCurrentPackage() + "." + scopeAsString + "." + call.getNameAsString());
-              usedTypeElements.add(getCurrentPackage() + "." + scopeAsString);
+            // Use the scope instead. First, check if it's resolvable. If it is, great -
+            // just use that. If not, then we need to use some heuristics as fallbacks.
+            try {
+              ResolvedType scopeType = scope.calculateResolvedType();
+              resolvedYetStuckMethodCall.add(scopeType.describe() + "." + call.getNameAsString());
+              usedTypeElements.add(scopeType.describe());
+            } catch (Exception e1) {
+              // There are two fallback cases: the scope is an FQN (e.g., in
+              // a call to a fully-qualified static method) or the scope is a simple name.
+              // In the simple name case, append the current package to the front, since
+              // if it had been imported we wouldn't be in this situation.
+              if (UnsolvedSymbolVisitor.isAClassPath(scopeAsString)) {
+                resolvedYetStuckMethodCall.add(scopeAsString + "." + call.getNameAsString());
+                usedTypeElements.add(scopeAsString);
+              } else {
+                resolvedYetStuckMethodCall.add(
+                    getCurrentPackage() + "." + scopeAsString + "." + call.getNameAsString());
+                usedTypeElements.add(getCurrentPackage() + "." + scopeAsString);
+              }
             }
           }
         } else {

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

@@ -164,6 +164,24 @@ public List<String> getParameterList() {
     return Collections.unmodifiableList(parameterList);
   }
 
+  /**
+   * Attempts to replace any parameters with the given name with java.lang.Object. If a parameter is
+   * successfully replaced, returns true. Otherwise, returns false.
+   *
+   * @param incorrectTypeName the type name to replace
+   * @return true if the name was replaced, false if not
+   */
+  public boolean replaceParamWithObject(String incorrectTypeName) {
+    boolean result = false;
+    for (int i = 0; i < parameterList.size(); i++) {
+      if (parameterList.get(i).equals(incorrectTypeName)) {
+        parameterList.set(i, "java.lang.Object");
+        result = true;
+      }
+    }
+    return result;
+  }
+
   /** Set isStatic to true */
   public void setStatic() {
     isStatic = true;

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

@@ -1990,12 +1990,17 @@ public void updateUnsolvedClassOrInterfaceWithMethod(
         listOfExceptions.add(exceptionTypeAsString);
       }
     }
+    if (listOfParameters.contains(JavaTypeCorrect.SYNTHETIC_UNCONSTRAINED_TYPE)) {
+      // return early: this method is an artifact of JavaTypeCorrect and won't be needed.
+      return;
+    }
     String returnType = "";
     if (desiredReturnType.equals("")) {
       returnType = returnNameForMethod(methodName);
     } else {
       returnType = desiredReturnType;
     }
+
     UnsolvedMethod thisMethod =
         new UnsolvedMethod(
             methodName,
@@ -3532,7 +3537,9 @@ public boolean updateTypeForSyntheticClasses(
       String correctTypeName) {
     // Make sure that correctTypeName is fully qualified, so that we don't need to
     // add an import to the synthetic class.
-    correctTypeName = lookupFQNs(correctTypeName);
+    if (!correctTypeName.contains(JavaTypeCorrect.SYNTHETIC_UNCONSTRAINED_TYPE)) {
+      correctTypeName = lookupFQNs(correctTypeName);
+    }
     boolean updatedSuccessfully = false;
     UnsolvedClassOrInterface classToSearch = new UnsolvedClassOrInterface(className, packageName);
     Iterator<UnsolvedClassOrInterface> iterator = missingClass.iterator();
@@ -3616,6 +3623,17 @@ private void migrateType(String incorrectTypeName, String correctTypeName) {
     // This one may or may not be present. If it is not, exit early and do nothing.
     UnsolvedClassOrInterface correctType = getMissingClassWithQualifiedName(correctTypeName);
     if (correctType == null) {
+      if (correctTypeName.contains(JavaTypeCorrect.SYNTHETIC_UNCONSTRAINED_TYPE)) {
+        // Special case: if the new type name is the synthetic unconstrained type name
+        // placeholder, there is one more thing to do: replace any and all parameter types
+        // in synthetic classes that use this (soon to be deleted) synthetic type name
+        // with java.lang.Object.
+        for (UnsolvedClassOrInterface unsolvedClass : missingClass) {
+          for (UnsolvedMethod m : unsolvedClass.getMethods()) {
+            m.replaceParamWithObject(incorrectTypeName);
+          }
+        }
+      }
       return;
     }
 

src/main/resources/min_program_compile_status.json

@@ -22,7 +22,7 @@
   "na-97": "PASS",
   "na-102": "PASS",
   "na-103": "PASS",
-  "na-176": "FAIL",
+  "na-176": "PASS",
   "na-323": "PASS",
   "na-347": "PASS",
   "na-389": "PASS",

src/main/resources/preservation_status.json

@@ -23,7 +23,7 @@
   "na-97": "PASS",
   "na-102": "PASS",
   "na-103": "PASS",
-  "na-176": "FAIL",
+  "na-176": "PASS",
   "na-323": "FAIL",
   "na-347": "PASS",
   "na-389": "PASS",

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

@@ -0,0 +1,18 @@
+package org.checkerframework.specimin;
+
+import java.io.IOException;
+import org.junit.Test;
+
+/**
+ * This test checks that typically-styled test code doesn't cause Specimin to lose a method. Based
+ * on a bug observed in na-176.
+ */
+public class WhenThenTest {
+  @Test
+  public void runTest() throws IOException {
+    SpeciminTestExecutor.runTestWithoutJarPaths(
+        "whenthen",
+        new String[] {"com/example/Simple.java"},
+        new String[] {"com.example.Simple#bar()"});
+  }
+}

src/test/resources/whenthen/expected/com/example/Banana.java

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

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

@@ -0,0 +1,16 @@
+package com.example;
+
+import static org.example.TestUtil.when;
+import static org.example.TestUtil.mock;
+
+import com.example.Banana;
+
+class Simple {
+
+    void bar() {
+        Integer s = mock(2);
+        Banana b = mock(3);
+        when(5).then("Foo");
+        when("bar").then(mock(1));
+    }
+}

src/test/resources/whenthen/expected/org/example/OrgExampleTestUtilWhenReturnType.java

@@ -0,0 +1,12 @@
+package org.example;
+
+public class OrgExampleTestUtilWhenReturnType {
+
+    public ThenReturnType then(java.lang.String parameter0) {
+        throw new Error();
+    }
+
+    public ThenReturnType then(java.lang.Object parameter0) {
+        throw new Error();
+    }
+}

src/test/resources/whenthen/expected/org/example/TestUtil.java

@@ -0,0 +1,16 @@
+package org.example;
+
+public class TestUtil {
+
+    public static <SyntheticUnconstrainedType> SyntheticUnconstrainedType mock(int parameter0) {
+        throw new Error();
+    }
+
+    public static OrgExampleTestUtilWhenReturnType when(int parameter0) {
+        throw new Error();
+    }
+
+    public static OrgExampleTestUtilWhenReturnType when(java.lang.String parameter0) {
+        throw new Error();
+    }
+}

src/test/resources/whenthen/expected/org/example/ThenReturnType.java

@@ -0,0 +1,4 @@
+package org.example;
+
+public class ThenReturnType {
+}

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

@@ -0,0 +1,16 @@
+package com.example;
+
+import static org.example.TestUtil.when;
+import static org.example.TestUtil.mock;
+
+import com.example.Banana;
+
+class Simple {
+
+    void bar() {
+        Integer s = mock(2);
+        Banana b = mock(3);
+        when(5).then("Foo");
+        when("bar").then(mock(1));
+    }
+}