Fix backwards-incompatible calls to ASTHelpers.hasDirectAnnotationWit…

…hSimpleName (#894) Error Prone 2.24.0 introduces new overloads of `ASTHelpers.hasDirectionAnnotationWithSimpleName`; see google/error-prone@aa6e3e7. 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.
uber · Jan 14, 2024 · 9ff44a7 · 9ff44a7
1 parent abff73e
commit 9ff44a7
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 28 deletions.
diff --git 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
@@ -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
@@ -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);
+  }
 }
diff --git 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,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);
             }
           });
     }

diff --git 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.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;
     }