Fix JDK compatibility issue in LombokHandler and introduce AstHelpers…

…Backports (#795) Fixes #794. Since the JDK interface change was made for JDK 17, I think even if NullAway was built on and targeted JDK 11, we would still have this problem. I added some hacky unit test coverage for this code so we'd have a way to test for regressions in the future. (The new unit test fails the `:nullaway:testJdk8` task without this change.) We missed this problem earlier since we suppressed a warning from `AstHelpersSuggestions`. Even though we cannot accept some of the suggestions from that check (since we may not require a recent enough version of Error Prone), the issues it flags can be serious. So, this PR removes all other suppressions of that checker and fixes the warnings. We introduce an `AstHelpersBackports` class to contain any backported logic from `AstHelpers` to enable the fixes.
uber · Jul 31, 2023 · 8e4a36a · 8e4a36a
1 parent 48772af
commit 8e4a36a
Show file tree

Hide file tree

Showing 10 changed files with 84 additions and 45 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java b/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java
@@ -0,0 +1,38 @@
+package com.uber.nullaway;
+
+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.
+ */
+public class ASTHelpersBackports {
+
+  private ASTHelpersBackports() {}
+
+  /**
+   * Returns true if the symbol is static. Returns {@code false} for module symbols. Remove once we
+   * require Error Prone 2.16.0 or higher.
+   */
+  @SuppressWarnings("ASTHelpersSuggestions")
+  public static boolean isStatic(Symbol symbol) {
+    if (symbol.getKind().name().equals("MODULE")) {
+      return false;
+    }
+    return symbol.isStatic();
+  }
+
+  /**
+   * A wrapper for {@link Symbol#getEnclosedElements} to avoid binary compatibility issues for
+   * covariant overrides in subtypes of {@link Symbol}.
+   *
+   * <p>Same as this ASTHelpers method in Error Prone:
+   * https://github.com/google/error-prone/blame/a1318e4b0da4347dff7508108835d77c470a7198/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java#L1148
+   * TODO: delete this method and switch to ASTHelpers once we can require Error Prone 2.20.0
+   */
+  public static List<Symbol> getEnclosedElements(Symbol symbol) {
+    return symbol.getEnclosedElements();
+  }
+}
diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
@@ -22,6 +22,7 @@
 
 package com.uber.nullaway;
 
+import static com.uber.nullaway.ASTHelpersBackports.isStatic;
 import static com.uber.nullaway.ErrorMessage.MessageTypes.FIELD_NO_INIT;
 import static com.uber.nullaway.ErrorMessage.MessageTypes.GET_ON_EMPTY_OPTIONAL;
 import static com.uber.nullaway.ErrorMessage.MessageTypes.METHOD_NO_INIT;
@@ -479,9 +480,7 @@ void reportInitErrorOnField(Symbol symbol, VisitorState state, Description.Build
       fieldName = flatName.substring(index) + "." + fieldName;
     }
 
-    @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater
-    boolean isStatic = symbol.isStatic();
-    if (isStatic) {
+    if (isStatic(symbol)) {
       state.reportMatch(
           createErrorDescription(
               new ErrorMessage(

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.isStatic;
 import static com.uber.nullaway.ErrorBuilder.errMsgForInitializer;
 import static com.uber.nullaway.NullabilityUtil.castToNonNull;
 
@@ -1005,9 +1006,7 @@ private Description checkForReadBeforeInit(ExpressionTree tree, VisitorState sta
     }
 
     // for static fields, make sure the enclosing init is a static method or block
-    @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater
-    boolean isStatic = symbol.isStatic();
-    if (isStatic) {
+    if (isStatic(symbol)) {
       Tree enclosing = enclosingBlockPath.getLeaf();
       if (enclosing instanceof MethodTree
           && !ASTHelpers.getSymbol((MethodTree) enclosing).isStatic()) {
@@ -1116,9 +1115,7 @@ private boolean fieldAlwaysInitializedBeforeRead(
       Symbol symbol, TreePath pathToRead, VisitorState state, TreePath enclosingBlockPath) {
     AccessPathNullnessAnalysis nullnessAnalysis = getNullnessAnalysis(state);
     Set<Element> nonnullFields;
-    @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater
-    boolean isStatic = symbol.isStatic();
-    if (isStatic) {
+    if (isStatic(symbol)) {
       nonnullFields = nullnessAnalysis.getNonnullStaticFieldsBefore(pathToRead, state.context);
     } else {
       nonnullFields = new LinkedHashSet<>();
@@ -2102,9 +2099,7 @@ private FieldInitEntities collectEntities(ClassTree tree, VisitorState state) {
             // matchVariable()
             continue;
           }
-          @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater
-          boolean fieldIsStatic = fieldSymbol.isStatic();
-          if (fieldIsStatic) {
+          if (isStatic(fieldSymbol)) {
             nonnullStaticFields.add(fieldSymbol);
           } else {
             nonnullInstanceFields.add(fieldSymbol);

diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
@@ -22,9 +22,6 @@
 
 package com.uber.nullaway;
 
-import static com.sun.tools.javac.code.TypeAnnotationPosition.TypePathEntryKind.ARRAY;
-import static com.sun.tools.javac.code.TypeAnnotationPosition.TypePathEntryKind.INNER_TYPE;
-
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.VisitorState;
@@ -176,18 +173,6 @@ public static TreePath findEnclosingMethodOrLambdaOrInitializer(TreePath path) {
     return findEnclosingMethodOrLambdaOrInitializer(path, ImmutableSet.of());
   }
 
-  /**
-   * A wrapper for {@link Symbol#getEnclosedElements} to avoid binary compatibility issues for
-   * covariant overrides in subtypes of {@link Symbol}.
-   *
-   * <p>Same as this ASTHelpers method in Error Prone:
-   * https://github.com/google/error-prone/blame/a1318e4b0da4347dff7508108835d77c470a7198/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java#L1148
-   * TODO: delete this method and switch to ASTHelpers once we can require Error Prone 2.20.0
-   */
-  public static List<Symbol> getEnclosedElements(Symbol symbol) {
-    return symbol.getEnclosedElements();
-  }
-
   /**
    * NOTE: this method does not work for getting all annotations of parameters of methods from class
    * files. For that case, use {@link #getAllAnnotationsForParameter(Symbol.MethodSymbol, int)}

diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java
@@ -16,6 +16,7 @@
 package com.uber.nullaway.dataflow;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.uber.nullaway.ASTHelpersBackports.isStatic;
 import static com.uber.nullaway.NullabilityUtil.castToNonNull;
 import static com.uber.nullaway.Nullness.BOTTOM;
 import static com.uber.nullaway.Nullness.NONNULL;
@@ -761,10 +762,9 @@ private CodeAnnotationInfo getCodeAnnotationInfo(VisitorState state) {
     return codeAnnotationInfo;
   }
 
-  @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater
   private void setReceiverNonnull(
       AccessPathNullnessPropagation.ReadableUpdates updates, Node receiver, Symbol symbol) {
-    if (symbol != null && !symbol.isStatic()) {
+    if ((symbol != null) && !isStatic(symbol)) {
       setNonnullIfAnalyzeable(updates, receiver);
     }
   }

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/AbstractFieldContractHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/AbstractFieldContractHandler.java
@@ -22,6 +22,7 @@
 
 package com.uber.nullaway.handlers;
 
+import static com.uber.nullaway.ASTHelpersBackports.getEnclosedElements;
 import static com.uber.nullaway.NullabilityUtil.castToNonNull;
 
 import com.google.common.base.Preconditions;
@@ -222,7 +223,7 @@ protected boolean validateAnnotationSyntax(
   public static @Nullable VariableElement getInstanceFieldOfClass(
       Symbol.ClassSymbol classSymbol, String name) {
     Preconditions.checkNotNull(classSymbol);
-    for (Element member : NullabilityUtil.getEnclosedElements(classSymbol)) {
+    for (Element member : getEnclosedElements(classSymbol)) {
       if (member.getKind().isField() && !member.getModifiers().contains(Modifier.STATIC)) {
         if (member.getSimpleName().toString().equals(name)) {
           return (VariableElement) member;

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java
@@ -21,6 +21,8 @@
  */
 package com.uber.nullaway.handlers;
 
+import static com.uber.nullaway.ASTHelpersBackports.getEnclosedElements;
+
 import com.google.common.base.Preconditions;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.suppliers.Supplier;
@@ -30,7 +32,6 @@
 import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.code.Types;
 import com.uber.nullaway.NullAway;
-import com.uber.nullaway.NullabilityUtil;
 import com.uber.nullaway.Nullness;
 import com.uber.nullaway.dataflow.AccessPath;
 import com.uber.nullaway.dataflow.AccessPathNullnessPropagation;
@@ -143,7 +144,7 @@ private FieldAndGetterElements getFieldAndGetterForProperty(
     Element getter = null;
     String fieldName = decapitalize(capPropName);
     String getterName = "get" + capPropName;
-    for (Symbol elem : NullabilityUtil.getEnclosedElements(symbol.owner)) {
+    for (Symbol elem : getEnclosedElements(symbol.owner)) {
       if (elem.getKind().isField() && elem.getSimpleName().toString().equals(fieldName)) {
         if (field != null) {
           throw new RuntimeException("already found field " + fieldName);

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java
@@ -21,6 +21,7 @@
  */
 package com.uber.nullaway.handlers;
 
+import static com.uber.nullaway.ASTHelpersBackports.getEnclosedElements;
 import static com.uber.nullaway.NullabilityUtil.castToNonNull;
 
 import com.google.common.base.Preconditions;
@@ -35,7 +36,6 @@
 import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.code.Types;
 import com.uber.nullaway.NullAway;
-import com.uber.nullaway.NullabilityUtil;
 import com.uber.nullaway.Nullness;
 import com.uber.nullaway.dataflow.AccessPath;
 import com.uber.nullaway.dataflow.AccessPathNullnessPropagation;
@@ -123,7 +123,7 @@ public ImmutableSet<String> onRegisterImmutableTypes() {
   private Symbol.MethodSymbol getGetterForMetadataSubtype(
       Symbol.ClassSymbol classSymbol, Types types) {
     // Is there a better way than iteration?
-    for (Symbol elem : NullabilityUtil.getEnclosedElements(classSymbol)) {
+    for (Symbol elem : getEnclosedElements(classSymbol)) {
       if (elem.getKind().equals(ElementKind.METHOD)) {
         Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) elem;
         if (grpcIsMetadataGetCall(methodSymbol, types)) {

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java
@@ -31,7 +31,6 @@ public LombokHandler(Config config) {
     this.config = config;
   }
 
-  @SuppressWarnings("ASTHelpersSuggestions") // Suggested API doesn't exist in EP 2.4.0
   private boolean isLombokMethodWithMissingNullableAnnotation(
       Symbol.MethodSymbol methodSymbol, VisitorState state) {
     if (!ASTHelpers.hasAnnotation(methodSymbol, LOMBOK_GENERATED_ANNOTATION_NAME, state)) {
@@ -44,16 +43,11 @@ private boolean isLombokMethodWithMissingNullableAnnotation(
     String originalFieldName =
         methodNameString.substring(LOMBOK_BUILDER_DEFAULT_METHOD_PREFIX.length());
     ImmutableList<Symbol> matchingMembers =
-        StreamSupport.stream(
-                methodSymbol
-                    .enclClass()
-                    .members()
-                    .getSymbols(
-                        sym ->
-                            sym.name.contentEquals(originalFieldName)
-                                && sym.getKind().equals(ElementKind.FIELD))
-                    .spliterator(),
-                false)
+        StreamSupport.stream(methodSymbol.enclClass().members().getSymbols().spliterator(), false)
+            .filter(
+                sym ->
+                    sym.name.contentEquals(originalFieldName)
+                        && sym.getKind().equals(ElementKind.FIELD))
             .collect(ImmutableList.toImmutableList());
     Preconditions.checkArgument(
         matchingMembers.size() == 1,

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayFrameworkTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayFrameworkTests.java
@@ -372,6 +372,32 @@ public void testLombokBuilderWithoutGeneratedAsUnannotated() {
         .doTest();
   }
 
+  /**
+   * This test is solely to check if we can run through some of the {@link
+   * com.uber.nullaway.handlers.LombokHandler} logic without crashing. It does not check that the
+   * logic is correct.
+   */
+  @Test
+  public void lombokHandlerRunsWithoutCrashing() {
+    makeTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "class Test {",
+            "  @Nullable Object test;",
+            "  @lombok.Generated",
+            "  Object $default$test() {",
+            "    return new Object();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void systemConsoleNullable() {
     defaultCompilationHelper