Fix backwards-incompatible calls to ASTHelpers.hasDirectAnnotationWithSimpleName (#894)

Error Prone 2.24.0 introduces new overloads of
`ASTHelpers.hasDirectionAnnotationWithSimpleName`; see
https://github.com/google/error-prone/commit/aa6e3e7be223e5f8319609281b229d4db4219654.
This causes backward compatibility issues when compiling against EP
2.24.0+ and then running on an older version of Error Prone. We
introduce a wrapper method to avoid this issue. We also modify our
`testJdk8` tasks to properly test the scenario of compiling against the
newest supported Error Prone version and then running on the oldest
supported version, which would have caught this issue.

Ideally we would have our own EP check preventing direct calls to
`ASTHelpers.hasDirectionAnnotationWithSimpleName`, but that can be done
in a follow-up.

5 files changed, 54 insertions, 28 deletions

diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle
index 01bbe70..5962a7c 100755
--- a/gradle/dependencies.gradle
+++ b/gradle/dependencies.gradle
@@ -67,10 +67,12 @@ def build = [
     asm                     : "org.ow2.asm:asm:${versions.asm}",
     asmTree                 : "org.ow2.asm:asm-tree:${versions.asm}",
     errorProneCheckApi      : "com.google.errorprone:error_prone_check_api:${versions.errorProneApi}",
+    errorProneCheckApiOld   : "com.google.errorprone:error_prone_check_api:${oldestErrorProneVersion}",
     errorProneCore          : "com.google.errorprone:error_prone_core:${versions.errorProne}",
     errorProneCoreForApi    : "com.google.errorprone:error_prone_core:${versions.errorProneApi}",
     errorProneJavac         : "com.google.errorprone:javac:9+181-r4173-1",
     errorProneTestHelpers   : "com.google.errorprone:error_prone_test_helpers:${versions.errorProneApi}",
+    errorProneTestHelpersOld: "com.google.errorprone:error_prone_test_helpers:${oldestErrorProneVersion}",
     checkerDataflow         : "org.checkerframework:dataflow-nullaway:${versions.checkerFramework}",
     guava                   : "com.google.guava:guava:30.1-jre",
     javaxValidation         : "javax.validation:validation-api:2.0.1.Final",
diff --git a/nullaway/build.gradle b/nullaway/build.gradle
index 829917f..7d39adc 100644
--- a/nullaway/build.gradle
+++ b/nullaway/build.gradle
@@ -21,7 +21,8 @@ plugins {
 }
 
 configurations {
-    nullawayJar
+    // A configuration holding the jars for the oldest supported version of Error Prone, to use with tests
+    errorProneOldest
 }
 
 dependencies {
@@ -64,6 +65,11 @@ dependencies {
     testImplementation deps.test.mockito
     testImplementation deps.test.javaxAnnotationApi
     testImplementation deps.test.assertJ
+
+    errorProneOldest deps.build.errorProneCheckApiOld
+    errorProneOldest(deps.build.errorProneTestHelpersOld) {
+        exclude group: "junit", module: "junit"
+    }
 }
 
 javadoc {
@@ -93,12 +99,10 @@ apply plugin: 'com.vanniktech.maven.publish'
 //    }
 
 // Create a task to test on JDK 8
+// NOTE: even when we drop JDK 8 support, we will still need a test task similar to this one for testing building
+// against a recent JDK and Error Prone version but then running on the oldest supported JDK and Error Prone version,
+// to check for binary compatibility issues.
 def jdk8Test = tasks.register("testJdk8", Test) {
-    onlyIf {
-        // Only if we are using a version of Error Prone compatible with JDK 8
-        deps.versions.errorProneApi == "2.10.0"
-    }
-
     javaLauncher = javaToolchains.launcherFor {
         languageVersion = JavaLanguageVersion.of(8)
     }
@@ -108,7 +112,11 @@ def jdk8Test = tasks.register("testJdk8", Test) {
 
     // Copy inputs from normal Test task.
     def testTask = tasks.getByName("test")
-    classpath = testTask.classpath
+    // A bit of a hack: we add the dependencies of the oldest supported Error Prone version to the _beginning_ of the
+    // classpath, so that they are used instead of the latest version.  This exercises the scenario of building
+    // NullAway against the latest supported Error Prone version but then running on the oldest supported version.
+    classpath = configurations.errorProneOldest + testTask.classpath
+
     testClassesDirs = testTask.testClassesDirs
     jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}"
     filter {
diff --git a/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java b/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java
index 06a91f9..47dc99f 100644
--- a/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java
+++ b/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java
@@ -1,12 +1,15 @@
 package com.uber.nullaway;
 
+import com.google.errorprone.util.ASTHelpers;
 import com.sun.tools.javac.code.Symbol;
 import java.util.List;
 
 /**
  * Methods backported from {@link com.google.errorprone.util.ASTHelpers} since we do not yet require
  * a recent-enough Error Prone version. The methods should be removed once we bump our minimum Error
- * Prone version accordingly.
+ * Prone version accordingly. We also include methods where new overloads have been added in recent
+ * Error Prone versions, to ensure binary compatibility when compiling against a new version but
+ * running on an old version.
  */
 public class ASTHelpersBackports {
 
@@ -36,4 +39,19 @@ public class ASTHelpersBackports {
   public static List<Symbol> getEnclosedElements(Symbol symbol) {
     return symbol.getEnclosedElements();
   }
+
+  /**
+   * A wrapper for {@link ASTHelpers#hasDirectAnnotationWithSimpleName(Symbol, String)} to avoid
+   * binary compatibility issues with new overloads in recent Error Prone versions. NullAway code
+   * should only use this method and not call the corresponding ASTHelpers methods directly.
+   *
+   * <p>TODO: delete this method and switch to ASTHelpers once we can require Error Prone 2.24.0
+   *
+   * @param sym the symbol
+   * @param simpleName the simple name
+   * @return {@code true} iff the symbol has a direct annotation with the given simple name
+   */
+  public static boolean hasDirectAnnotationWithSimpleName(Symbol sym, String simpleName) {
+    return ASTHelpers.hasDirectAnnotationWithSimpleName(sym, simpleName);
+  }
 }
diff --git a/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java b/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java
index 013e8f4..37fcc03 100644
--- a/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java
+++ b/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java
@@ -22,6 +22,7 @@
 
 package com.uber.nullaway;
 
+import static com.uber.nullaway.ASTHelpersBackports.hasDirectAnnotationWithSimpleName;
 import static com.uber.nullaway.NullabilityUtil.castToNonNull;
 
 import com.google.common.base.Preconditions;
@@ -81,7 +82,7 @@ public final class CodeAnnotationInfo {
     Symbol.PackageSymbol enclosingPackage = ASTHelpers.enclosingPackage(outermostClassSymbol);
     if (!config.fromExplicitlyAnnotatedPackage(className)
         && !(enclosingPackage != null
-            && ASTHelpers.hasDirectAnnotationWithSimpleName(
+            && hasDirectAnnotationWithSimpleName(
                 enclosingPackage, NullabilityUtil.NULLMARKED_SIMPLE_NAME))) {
       // By default, unknown code is unannotated unless @NullMarked or configured as annotated by
       // package name
@@ -89,7 +90,7 @@ public final class CodeAnnotationInfo {
     }
     if (config.fromExplicitlyUnannotatedPackage(className)
         || (enclosingPackage != null
-            && ASTHelpers.hasDirectAnnotationWithSimpleName(
+            && hasDirectAnnotationWithSimpleName(
                 enclosingPackage, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME))) {
       // Any code explicitly marked as unannotated in our configuration is unannotated, no matter
       // what. Similarly, any package annotated as @NullUnmarked is unannotated, even if
@@ -120,7 +121,7 @@ public final class CodeAnnotationInfo {
       return false;
     }
     Symbol.ClassSymbol outermostClassSymbol = get(classSymbol, config).outermostClassSymbol;
-    return ASTHelpers.hasDirectAnnotationWithSimpleName(outermostClassSymbol, "Generated");
+    return hasDirectAnnotationWithSimpleName(outermostClassSymbol, "Generated");
   }
 
   /**
@@ -216,10 +217,10 @@ public final class CodeAnnotationInfo {
         if (enclosingMethod != null) {
           isAnnotated = recordForEnclosing.isMethodNullnessAnnotated(enclosingMethod);
         }
-        if (ASTHelpers.hasDirectAnnotationWithSimpleName(
+        if (hasDirectAnnotationWithSimpleName(
             classSymbol, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) {
           isAnnotated = false;
-        } else if (ASTHelpers.hasDirectAnnotationWithSimpleName(
+        } else if (hasDirectAnnotationWithSimpleName(
             classSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME)) {
           isAnnotated = true;
         }
@@ -244,7 +245,7 @@ public final class CodeAnnotationInfo {
       // Generated code is or isn't excluded, depending on configuration
       // Note: In the future, we might want finer grain controls to distinguish code that is
       // generated with nullability info and without.
-      if (ASTHelpers.hasDirectAnnotationWithSimpleName(classSymbol, "Generated")) {
+      if (hasDirectAnnotationWithSimpleName(classSymbol, "Generated")) {
         return true;
       }
       ImmutableSet<String> generatedCodeAnnotations = config.getGeneratedCodeAnnotations();
@@ -259,13 +260,11 @@ public final class CodeAnnotationInfo {
 
   private boolean isAnnotatedTopLevelClass(Symbol.ClassSymbol classSymbol, Config config) {
     // First, check for an explicitly @NullUnmarked top level class
-    if (ASTHelpers.hasDirectAnnotationWithSimpleName(
-        classSymbol, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) {
+    if (hasDirectAnnotationWithSimpleName(classSymbol, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) {
       return false;
     }
     // Then, check if the class has a @NullMarked annotation or comes from an annotated package
-    if ((ASTHelpers.hasDirectAnnotationWithSimpleName(
-            classSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME)
+    if ((hasDirectAnnotationWithSimpleName(classSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME)
         || fromAnnotatedPackage(classSymbol, config))) {
       // make sure it's not explicitly configured as unannotated
       return !shouldTreatAsUnannotated(classSymbol, config);
@@ -295,14 +294,12 @@ public final class CodeAnnotationInfo {
       return methodNullnessCache.computeIfAbsent(
           methodSymbol,
           m -> {
-            if (ASTHelpers.hasDirectAnnotationWithSimpleName(
-                m, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) {
+            if (hasDirectAnnotationWithSimpleName(m, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) {
               return false;
             } else if (this.isNullnessAnnotated) {
               return true;
             } else {
-              return ASTHelpers.hasDirectAnnotationWithSimpleName(
-                  m, NullabilityUtil.NULLMARKED_SIMPLE_NAME);
+              return hasDirectAnnotationWithSimpleName(m, NullabilityUtil.NULLMARKED_SIMPLE_NAME);
             }
           });
     }
diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
index 1ab3b12..3bf0a85 100644
--- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java
+++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -28,6 +28,7 @@ import static com.sun.source.tree.Tree.Kind.IDENTIFIER;
 import static com.sun.source.tree.Tree.Kind.OTHER;
 import static com.sun.source.tree.Tree.Kind.PARENTHESIZED;
 import static com.sun.source.tree.Tree.Kind.TYPE_CAST;
+import static com.uber.nullaway.ASTHelpersBackports.hasDirectAnnotationWithSimpleName;
 import static com.uber.nullaway.ASTHelpersBackports.isStatic;
 import static com.uber.nullaway.ErrorBuilder.errMsgForInitializer;
 import static com.uber.nullaway.NullabilityUtil.castToNonNull;
@@ -578,20 +579,20 @@ public class NullAway extends BugChecker
     Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree);
     switch (nullMarkingForTopLevelClass) {
       case FULLY_MARKED:
-        if (ASTHelpers.hasDirectAnnotationWithSimpleName(
+        if (hasDirectAnnotationWithSimpleName(
             methodSymbol, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) {
           nullMarkingForTopLevelClass = NullMarking.PARTIALLY_MARKED;
         }
         break;
       case FULLY_UNMARKED:
-        if (ASTHelpers.hasDirectAnnotationWithSimpleName(
+        if (hasDirectAnnotationWithSimpleName(
             methodSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME)) {
           nullMarkingForTopLevelClass = NullMarking.PARTIALLY_MARKED;
           markedMethodInUnmarkedContext = true;
         }
         break;
       case PARTIALLY_MARKED:
-        if (ASTHelpers.hasDirectAnnotationWithSimpleName(
+        if (hasDirectAnnotationWithSimpleName(
             methodSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME)) {
           // We still care here if this is a transition between @NullUnmarked and @NullMarked code,
           // within partially marked code, see checks below for markedMethodInUnmarkedContext.
@@ -1467,10 +1468,10 @@ public class NullAway extends BugChecker
    */
   private boolean classAnnotationIntroducesPartialMarking(Symbol.ClassSymbol classSymbol) {
     return (nullMarkingForTopLevelClass == NullMarking.FULLY_UNMARKED
-            && ASTHelpers.hasDirectAnnotationWithSimpleName(
+            && hasDirectAnnotationWithSimpleName(
                 classSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME))
         || (nullMarkingForTopLevelClass == NullMarking.FULLY_MARKED
-            && ASTHelpers.hasDirectAnnotationWithSimpleName(
+            && hasDirectAnnotationWithSimpleName(
                 classSymbol, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME));
   }
 
@@ -2240,7 +2241,7 @@ public class NullAway extends BugChecker
   }
 
   private boolean isInitializerMethod(VisitorState state, Symbol.MethodSymbol symbol) {
-    if (ASTHelpers.hasDirectAnnotationWithSimpleName(symbol, "Initializer")
+    if (hasDirectAnnotationWithSimpleName(symbol, "Initializer")
         || config.isKnownInitializerMethod(symbol)) {
       return true;
     }