Merge 28ac48b into c1741b3

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

@@ -808,6 +808,21 @@ static Trees getTreesInstance(VisitorState state) {
     return Trees.instance(JavacProcessingEnvironment.instance(state.context));
   }
 
+  private Nullness getMethodReturnNullness(
+      Symbol.MethodSymbol methodSymbol, VisitorState state, Nullness defaultForUnannotated) {
+    final boolean isMethodAnnotated = !codeAnnotationInfo.isSymbolUnannotated(methodSymbol, config);
+    Nullness methodReturnNullness =
+        defaultForUnannotated; // Permissive default for unannotated code.
+    if (isMethodAnnotated) {
+      methodReturnNullness =
+          Nullness.hasNullableAnnotation(methodSymbol, config)
+              ? Nullness.NULLABLE
+              : Nullness.NONNULL;
+    }
+    return handler.onOverrideMethodReturnNullability(
+        methodSymbol, state, isMethodAnnotated, methodReturnNullness);
+  }
+
   private Description checkReturnExpression(
       Tree tree, ExpressionTree retExpr, Symbol.MethodSymbol methodSymbol, VisitorState state) {
     Type returnType = methodSymbol.getReturnType();
@@ -822,8 +837,7 @@ private Description checkReturnExpression(
       // support)
       return Description.NO_MATCH;
     }
-    if (codeAnnotationInfo.isSymbolUnannotated(methodSymbol, config)
-        || Nullness.hasNullableAnnotation(methodSymbol, config)) {
+    if (getMethodReturnNullness(methodSymbol, state, Nullness.NULLABLE).equals(Nullness.NULLABLE)) {
       return Description.NO_MATCH;
     }
     if (mayBeNullExpr(state, retExpr)) {
@@ -908,63 +922,41 @@ private Description checkOverriding(
       Symbol.MethodSymbol overridingMethod,
       @Nullable MemberReferenceTree memberReferenceTree,
       VisitorState state) {
-    final boolean isOverriddenMethodAnnotated =
-        !codeAnnotationInfo.isSymbolUnannotated(overriddenMethod, config);
-    Nullness overriddenMethodReturnNullness =
-        Nullness.NULLABLE; // Permissive default for unannotated code.
-    if (isOverriddenMethodAnnotated && !Nullness.hasNullableAnnotation(overriddenMethod, config)) {
-      overriddenMethodReturnNullness = Nullness.NONNULL;
-    }
-    overriddenMethodReturnNullness =
-        handler.onOverrideMethodInvocationReturnNullability(
-            overriddenMethod, state, isOverriddenMethodAnnotated, overriddenMethodReturnNullness);
-    // if the super method returns nonnull,
-    // overriding method better not return nullable
-    if (overriddenMethodReturnNullness.equals(Nullness.NONNULL)) {
-      final boolean isOverridingMethodAnnotated =
-          !codeAnnotationInfo.isSymbolUnannotated(overridingMethod, config);
-      // Note that, for the overriding method, the permissive default is non-null.
-      Nullness overridingMethodReturnNullness = Nullness.NONNULL;
-      if (isOverridingMethodAnnotated && Nullness.hasNullableAnnotation(overridingMethod, config)) {
-        overridingMethodReturnNullness = Nullness.NULLABLE;
-      }
-      // We must once again check the handler chain, to allow it to update nullability of the
-      // overriding method
-      // (e.g. through AcknowledgeRestrictiveAnnotations=true)
-      overridingMethodReturnNullness =
-          handler.onOverrideMethodInvocationReturnNullability(
-              overridingMethod, state, isOverridingMethodAnnotated, overridingMethodReturnNullness);
-      if (overridingMethodReturnNullness.equals(Nullness.NULLABLE)
-          && (memberReferenceTree == null
-              || getComputedNullness(memberReferenceTree).equals(Nullness.NULLABLE))) {
-        String message;
-        if (memberReferenceTree != null) {
-          message =
-              "referenced method returns @Nullable, but functional interface method "
-                  + ASTHelpers.enclosingClass(overriddenMethod)
-                  + "."
-                  + overriddenMethod.toString()
-                  + " returns @NonNull";
-
-        } else {
-          message =
-              "method returns @Nullable, but superclass method "
-                  + ASTHelpers.enclosingClass(overriddenMethod)
-                  + "."
-                  + overriddenMethod.toString()
-                  + " returns @NonNull";
-        }
+    // if the super method returns nonnull, overriding method better not return nullable
+    // Note that, for the overriding method, the permissive default is non-null,
+    // but it's nullable for the overridden one.
+    if (getMethodReturnNullness(overriddenMethod, state, Nullness.NULLABLE).equals(Nullness.NONNULL)
+        && getMethodReturnNullness(overridingMethod, state, Nullness.NONNULL)
+            .equals(Nullness.NULLABLE)
+        && (memberReferenceTree == null
+            || getComputedNullness(memberReferenceTree).equals(Nullness.NULLABLE))) {
+      String message;
+      if (memberReferenceTree != null) {
+        message =
+            "referenced method returns @Nullable, but functional interface method "
+                + ASTHelpers.enclosingClass(overriddenMethod)
+                + "."
+                + overriddenMethod.toString()
+                + " returns @NonNull";
 
-        Tree errorTree =
-            memberReferenceTree != null
-                ? memberReferenceTree
-                : getTreesInstance(state).getTree(overridingMethod);
-        return errorBuilder.createErrorDescription(
-            new ErrorMessage(MessageTypes.WRONG_OVERRIDE_RETURN, message),
-            buildDescription(errorTree),
-            state,
-            overriddenMethod);
+      } else {
+        message =
+            "method returns @Nullable, but superclass method "
+                + ASTHelpers.enclosingClass(overriddenMethod)
+                + "."
+                + overriddenMethod.toString()
+                + " returns @NonNull";
       }
+
+      Tree errorTree =
+          memberReferenceTree != null
+              ? memberReferenceTree
+              : getTreesInstance(state).getTree(overridingMethod);
+      return errorBuilder.createErrorDescription(
+          new ErrorMessage(MessageTypes.WRONG_OVERRIDE_RETURN, message),
+          buildDescription(errorTree),
+          state,
+          overriddenMethod);
     }
     // if any parameter in the super method is annotated @Nullable,
     // overriding method cannot assume @Nonnull

nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java

@@ -109,7 +109,7 @@ public void onMatchReturn(NullAway analysis, ReturnTree tree, VisitorState state
   }
 
   @Override
-  public Nullness onOverrideMethodInvocationReturnNullability(
+  public Nullness onOverrideMethodReturnNullability(
       Symbol.MethodSymbol methodSymbol,
       VisitorState state,
       boolean isAnnotated,

nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java

@@ -123,15 +123,14 @@ public void onMatchReturn(NullAway analysis, ReturnTree tree, VisitorState state
   }
 
   @Override
-  public Nullness onOverrideMethodInvocationReturnNullability(
+  public Nullness onOverrideMethodReturnNullability(
       Symbol.MethodSymbol methodSymbol,
       VisitorState state,
       boolean isAnnotated,
       Nullness returnNullness) {
     for (Handler h : handlers) {
       returnNullness =
-          h.onOverrideMethodInvocationReturnNullability(
-              methodSymbol, state, isAnnotated, returnNullness);
+          h.onOverrideMethodReturnNullability(methodSymbol, state, isAnnotated, returnNullness);
     }
     return returnNullness;
   }

nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

@@ -162,12 +162,12 @@ boolean onOverrideMayBeNullExpr(
    * @param methodSymbol The method symbol for the method in question.
    * @param state The current visitor state.
    * @param isAnnotated A boolean flag indicating whether the called method is considered to be
-   *     within isAnnotated or unannotated code, used to avoid querying for this information
-   *     multiple times within the same handler chain.
+   *     within annotated or unannotated code, used to avoid querying for this information multiple
+   *     times within the same handler chain.
    * @param returnNullness return nullness computed by upstream handlers or NullAway core.
    * @return Updated return nullability computed by this handler.
    */
-  Nullness onOverrideMethodInvocationReturnNullability(
+  Nullness onOverrideMethodReturnNullability(
       Symbol.MethodSymbol methodSymbol,
       VisitorState state,
       boolean isAnnotated,

nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java

@@ -74,6 +74,7 @@ public static Handler buildDefault(Config config) {
     if (config.checkContracts()) {
       handlerListBuilder.add(new ContractCheckHandler(config));
     }
+    handlerListBuilder.add(new LombokHandler(config));
 
     return new CompositeHandler(handlerListBuilder.build());
   }

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java

@@ -109,14 +109,16 @@ public Nullness[] onOverrideMethodInvocationParametersNullability(
   }
 
   @Override
-  public Nullness onOverrideMethodInvocationReturnNullability(
+  public Nullness onOverrideMethodReturnNullability(
       Symbol.MethodSymbol methodSymbol,
       VisitorState state,
       boolean isAnnotated,
       Nullness returnNullness) {
     OptimizedLibraryModels optLibraryModels = getOptLibraryModels(state.context);
     if (optLibraryModels.hasNonNullReturn(methodSymbol, state.getTypes(), !isAnnotated)) {
-      return Nullness.NONNULL;
+      return NONNULL;
+    } else if (optLibraryModels.hasNullableReturn(methodSymbol, state.getTypes(), !isAnnotated)) {
+      return NULLABLE;
     }
     return returnNullness;
   }

nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java

@@ -0,0 +1,95 @@
+package com.uber.nullaway.handlers;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.Tree;
+import com.sun.tools.javac.code.Symbol;
+import com.uber.nullaway.Config;
+import com.uber.nullaway.NullAway;
+import com.uber.nullaway.Nullness;
+import java.util.stream.StreamSupport;
+import javax.annotation.Nullable;
+import javax.lang.model.element.ElementKind;
+
+/**
+ * A general handler for Lombok generated code and its internal semantics.
+ *
+ * <p>Currently used to propagate @Nullable in cases where the Lombok annotation processor fails to
+ * do so consistently.
+ */
+public class LombokHandler extends BaseNoOpHandler {
+
+  private static String LOMBOK_GENERATED_ANNOTATION_NAME = "lombok.Generated";
+  private static String LOMBOK_BUILDER_DEFAULT_METHOD_PREFIX = "$default$";
+
+  private final Config config;
+
+  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)) {
+      return false;
+    }
+    String methodNameString = methodSymbol.name.toString();
+    if (!methodNameString.startsWith(LOMBOK_BUILDER_DEFAULT_METHOD_PREFIX)) {
+      return false;
+    }
+    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)
+            .collect(ImmutableList.toImmutableList());
+    Preconditions.checkArgument(
+        matchingMembers.size() == 1,
+        String.format(
+            "Found %d fields matching Lombok generated builder default method %s",
+            matchingMembers.size(), methodNameString));
+    return Nullness.hasNullableAnnotation(matchingMembers.get(0), config);
+  }
+
+  @Override
+  public boolean onOverrideMayBeNullExpr(
+      NullAway analysis,
+      ExpressionTree expr,
+      @Nullable Symbol exprSymbol,
+      VisitorState state,
+      boolean exprMayBeNull) {
+    if (exprMayBeNull) {
+      return true;
+    }
+    Tree.Kind exprKind = expr.getKind();
+    if (exprSymbol != null && exprKind == Tree.Kind.METHOD_INVOCATION) {
+      Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) exprSymbol;
+      return isLombokMethodWithMissingNullableAnnotation(methodSymbol, state);
+    }
+    return false;
+  }
+
+  @Override
+  public Nullness onOverrideMethodReturnNullability(
+      Symbol.MethodSymbol methodSymbol,
+      VisitorState state,
+      boolean isAnnotated,
+      Nullness returnNullness) {
+    if (isLombokMethodWithMissingNullableAnnotation(methodSymbol, state)) {
+      return Nullness.NULLABLE;
+    }
+    return returnNullness;
+  }
+}

nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java

@@ -133,7 +133,7 @@ public Nullness[] onOverrideMethodInvocationParametersNullability(
   }
 
   @Override
-  public Nullness onOverrideMethodInvocationReturnNullability(
+  public Nullness onOverrideMethodReturnNullability(
       Symbol.MethodSymbol methodSymbol,
       VisitorState state,
       boolean isAnnotated,

nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java

@@ -53,20 +53,22 @@ public void allowLibraryModelsOverrideAnnotations() {
                 temporaryFolder.getRoot().getAbsolutePath(),
                 "-XepOpt:NullAway:AnnotatedPackages=com.uber"))
         .addSourceLines(
-            "Foo.java",
+            "AnnotatedWithModels.java",
             "package com.uber;",
-            "public class Foo {",
+            "public class AnnotatedWithModels {",
             "   Object field = new Object();",
-            "   Object bar() {",
-            "      return new Object();",
+            "   // implicitly @Nullable due to library model",
+            "   Object returnsNullFromModel() {",
+            "      // null is valid here only because of the library model",
+            "      return null;",
             "   }",
             "   Object nullableReturn() {",
             "       // BUG: Diagnostic contains: returning @Nullable",
-            "       return bar();",
+            "       return returnsNullFromModel();",
             "   }",
             "   void run() {",
             "       // just to make sure, flow analysis is also impacted by library models information",
-            "      Object temp = bar();",
+            "      Object temp = returnsNullFromModel();",
             "       // BUG: Diagnostic contains: assigning @Nullable",
             "      this.field = temp;",
             "   }",

test-java-lib-lombok/src/main/java/com/uber/lombok/LombokDTO.java

@@ -36,4 +36,5 @@ public class LombokDTO {
   private String field;
   @Builder.Default private String fieldWithDefault = "Default";
   @Nullable private String nullableField;
+  @Nullable @Builder.Default private String fieldWithNullDefault = null;
 }

test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java

@@ -66,7 +66,7 @@ public ImmutableSetMultimap<MethodRef, Integer> nullImpliesNullParameters() {
   @Override
   public ImmutableSet<MethodRef> nullableReturns() {
     return ImmutableSet.of(
-        methodRef("com.uber.Foo", "bar()"),
+        methodRef("com.uber.AnnotatedWithModels", "returnsNullFromModel()"),
         methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated()"),
         methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated2()"));
   }