From 2a57c544463666dc092f225a381ba6f39c249939 Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Wed, 31 May 2023 16:08:30 -0400 Subject: [PATCH] Fix error inside Lombok generated code when dealing with @Nullable @Builder.Default fields --- .../main/java/com/uber/nullaway/NullAway.java | 13 ++- .../com/uber/nullaway/handlers/Handlers.java | 1 + .../handlers/LibraryModelsHandler.java | 4 +- .../uber/nullaway/handlers/LombokHandler.java | 92 +++++++++++++++++++ .../NullAwayCustomLibraryModelsTests.java | 14 +-- .../main/java/com/uber/lombok/LombokDTO.java | 1 + .../testlibrarymodels/TestLibraryModels.java | 2 +- 7 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index a4e50a328d..ed75f7ec17 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -822,8 +822,17 @@ private Description checkReturnExpression( // support) return Description.NO_MATCH; } - if (codeAnnotationInfo.isSymbolUnannotated(methodSymbol, config) - || Nullness.hasNullableAnnotation(methodSymbol, config)) { + final boolean isContainingMethodAnnotated = + !codeAnnotationInfo.isSymbolUnannotated(methodSymbol, config); + Nullness containingMethodReturnNullness = + Nullness.NULLABLE; // Permissive default for unannotated code. + if (isContainingMethodAnnotated && !Nullness.hasNullableAnnotation(methodSymbol, config)) { + containingMethodReturnNullness = Nullness.NONNULL; + } + containingMethodReturnNullness = + handler.onOverrideMethodInvocationReturnNullability( + methodSymbol, state, isContainingMethodAnnotated, containingMethodReturnNullness); + if (containingMethodReturnNullness.equals(Nullness.NULLABLE)) { return Description.NO_MATCH; } if (mayBeNullExpr(state, retExpr)) { diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java index ba46360a87..85e10e644a 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java +++ b/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()); } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java index 954651bccc..fff29af3e0 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -116,7 +116,9 @@ public Nullness onOverrideMethodInvocationReturnNullability( 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; } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java new file mode 100644 index 0000000000..91d3ea2a64 --- /dev/null +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java @@ -0,0 +1,92 @@ +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. + * + *

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; + } + + 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 matchingMembers = + StreamSupport.stream( + ASTHelpers.scope(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 onOverrideMethodInvocationReturnNullability( + Symbol.MethodSymbol methodSymbol, + VisitorState state, + boolean isAnnotated, + Nullness returnNullness) { + if (isLombokMethodWithMissingNullableAnnotation(methodSymbol, state)) { + return Nullness.NULLABLE; + } + return returnNullness; + } +} diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java index 0685a6566e..937752c0bc 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java +++ b/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;", " }", 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 index 37bedb4c14..44d6b50709 100644 --- 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,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; } 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 index cd6aad1d40..3b84b8f8d5 100644 --- 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 nullImpliesNullParameters() { @Override public ImmutableSet 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()")); }