Fix backwards-incompatible calls to ASTHelpers.hasDirectAnnotationWithSimpleName

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",

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 {

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 static boolean isStatic(Symbol symbol) {
   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);
+  }
 }

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,15 +82,15 @@ private static boolean fromAnnotatedPackage(
     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
       return false;
     }
     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 boolean isGenerated(Symbol symbol, Config config) {
       return false;
     }
     Symbol.ClassSymbol outermostClassSymbol = get(classSymbol, config).outermostClassSymbol;
-    return ASTHelpers.hasDirectAnnotationWithSimpleName(outermostClassSymbol, "Generated");
+    return hasDirectAnnotationWithSimpleName(outermostClassSymbol, "Generated");
   }
 
   /**
@@ -216,10 +217,10 @@ private ClassCacheRecord get(Symbol.ClassSymbol classSymbol, Config config) {
         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 @@ private boolean shouldTreatAsUnannotated(Symbol.ClassSymbol classSymbol, Config
       // 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 @@ private boolean shouldTreatAsUnannotated(Symbol.ClassSymbol classSymbol, Config
 
   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 boolean isMethodNullnessAnnotated(Symbol.MethodSymbol methodSymbol) {
       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);
             }
           });
     }

nullaway/src/main/java/com/uber/nullaway/NullAway.java

@@ -28,6 +28,7 @@
 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 @@ private void checkForMethodNullMarkedness(MethodTree tree, VisitorState state) {
     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 Description matchVariable(VariableTree tree, VisitorState state) {
    */
   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 @@ private boolean isConstructor(MethodTree methodTree) {
   }
 
   private boolean isInitializerMethod(VisitorState state, Symbol.MethodSymbol symbol) {
-    if (ASTHelpers.hasDirectAnnotationWithSimpleName(symbol, "Initializer")
+    if (hasDirectAnnotationWithSimpleName(symbol, "Initializer")
         || config.isKnownInitializerMethod(symbol)) {
       return true;
     }