Revert "Fix error inside Lombok generated code for @nullable @Builder…

….Default (uber#765)" This reverts commit d09ff9b.
msridhar · Jul 19, 2023 · fd5ec12 · fd5ec12
1 parent eaab0a1
commit fd5ec12
Show file tree

Hide file tree

Showing 11 changed files with 74 additions and 166 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -808,21 +808,6 @@ 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();
@@ -837,7 +822,8 @@ private Description checkReturnExpression(
       // support)
       return Description.NO_MATCH;
     }
-    if (getMethodReturnNullness(methodSymbol, state, Nullness.NULLABLE).equals(Nullness.NULLABLE)) {
+    if (codeAnnotationInfo.isSymbolUnannotated(methodSymbol, config)
+        || Nullness.hasNullableAnnotation(methodSymbol, config)) {
       return Description.NO_MATCH;
     }
     if (mayBeNullExpr(state, retExpr)) {
@@ -922,41 +908,63 @@ private Description checkOverriding(
       Symbol.MethodSymbol overridingMethod,
       @Nullable MemberReferenceTree memberReferenceTree,
       VisitorState state) {
-    // 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";
-
-      } else {
-        message =
-            "method returns @Nullable, but superclass method "
-                + ASTHelpers.enclosingClass(overriddenMethod)
-                + "."
-                + overriddenMethod.toString()
-                + " returns @NonNull";
+    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";
 
-      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

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java b/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 onOverrideMethodReturnNullability(
+  public Nullness onOverrideMethodInvocationReturnNullability(
       Symbol.MethodSymbol methodSymbol,
       VisitorState state,
       boolean isAnnotated,

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java
@@ -123,14 +123,15 @@ public void onMatchReturn(NullAway analysis, ReturnTree tree, VisitorState state
   }
 
   @Override
-  public Nullness onOverrideMethodReturnNullability(
+  public Nullness onOverrideMethodInvocationReturnNullability(
       Symbol.MethodSymbol methodSymbol,
       VisitorState state,
       boolean isAnnotated,
       Nullness returnNullness) {
     for (Handler h : handlers) {
       returnNullness =
-          h.onOverrideMethodReturnNullability(methodSymbol, state, isAnnotated, returnNullness);
+          h.onOverrideMethodInvocationReturnNullability(
+              methodSymbol, state, isAnnotated, returnNullness);
     }
     return returnNullness;
   }

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java b/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 annotated or unannotated code, used to avoid querying for this information multiple
-   *     times within the same handler chain.
+   *     within isAnnotated 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 onOverrideMethodReturnNullability(
+  Nullness onOverrideMethodInvocationReturnNullability(
       Symbol.MethodSymbol methodSymbol,
       VisitorState state,
       boolean isAnnotated,

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java
@@ -74,7 +74,6 @@ public static Handler buildDefault(Config config) {
     if (config.checkContracts()) {
       handlerListBuilder.add(new ContractCheckHandler(config));
     }
-    handlerListBuilder.add(new LombokHandler(config));
 
     return new CompositeHandler(handlerListBuilder.build());
   }

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
@@ -109,16 +109,14 @@ public Nullness[] onOverrideMethodInvocationParametersNullability(
   }
 
   @Override
-  public Nullness onOverrideMethodReturnNullability(
+  public Nullness onOverrideMethodInvocationReturnNullability(
       Symbol.MethodSymbol methodSymbol,
       VisitorState state,
       boolean isAnnotated,
       Nullness returnNullness) {
     OptimizedLibraryModels optLibraryModels = getOptLibraryModels(state.context);
     if (optLibraryModels.hasNonNullReturn(methodSymbol, state.getTypes(), !isAnnotated)) {
-      return NONNULL;
-    } else if (optLibraryModels.hasNullableReturn(methodSymbol, state.getTypes(), !isAnnotated)) {
-      return NULLABLE;
+      return Nullness.NONNULL;
     }
     return returnNullness;
   }

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java
diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java
@@ -133,7 +133,7 @@ public Nullness[] onOverrideMethodInvocationParametersNullability(
   }
 
   @Override
-  public Nullness onOverrideMethodReturnNullability(
+  public Nullness onOverrideMethodInvocationReturnNullability(
       Symbol.MethodSymbol methodSymbol,
       VisitorState state,
       boolean isAnnotated,

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

diff --git a/test-java-lib-lombok/src/main/java/com/uber/lombok/LombokDTO.java b/test-java-lib-lombok/src/main/java/com/uber/lombok/LombokDTO.java
@@ -36,5 +36,4 @@ public class LombokDTO {
   private String field;
   @Builder.Default private String fieldWithDefault = "Default";
   @Nullable private String nullableField;
-  @Nullable @Builder.Default private String fieldWithNullDefault = null;
 }
diff --git a/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java b/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.AnnotatedWithModels", "returnsNullFromModel()"),
+        methodRef("com.uber.Foo", "bar()"),
         methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated()"),
         methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated2()"));
   }