Fix error inside Lombok generated code for @Nullable @Builder.Default #765

lazaroclapp · 2023-05-31T20:20:27Z

When given code such as:

@Builder
public class LombokDTO {
  @Nullable @Builder.Default private String fieldWithNullDefault = null;
}

Lombok internally generates the following method:

@java.lang.SuppressWarnings(value = "all")
@lombok.Generated()
private static String $default$fieldWithNullDefault() {
    return null;
}

which does not propagate @Nullable to the method's return type!

While this method is marked as @Generated code and @SuppressWarnings("all"), that does not suppress NullAway under all configurations. In fact, we sometimes want to check generated code (setters/getters for auto-annotation, for example), so just counting all Lombok Generated code as unannotated is not always the desired behavior (it's optional behavior, enabled by the TreatGeneratedAsUnannotated=true flag).

Instead, we want to internally and implicitly propagate the @Nullable annotation from fieldWithNullDefault to the generated $default$fieldWithNullDefault().

We do this in two steps:

We modify our checking of return statements to allow handlers' existing overriding of method return nullability to be taken into account when deciding if return [nullable expression]; should result in a NullAway error.
We add a new handler for fixing the Lombok nullability propagation, by detecting these $default$foo() methods and looking at the nullability of foo to determine if the method should also be @Nullable.

In addition to this, we can suggest a fix upstream in Lombok to propagate @Nullable to these $default$ methods when present on the field, but this would not obviate the need for this PR, since we are keeping compatibility with multiple older Lombok releases internally.

Edit: Also, note that coverage on LombokHandler is bad in terms of unit tests, but that's because coveralls doesn't count test-java-lib-lombok/ as a test case. We need to build with Lombok to exercise most of this handler, so we can't really exercise it in unit tests (vs integration) without significantly artificial workarounds (i.e. manually replicating the Lombok-generated code).

…uilder.Default fields

lazaroclapp · 2023-05-31T20:23:37Z

@msridhar Let me know if you want me to split this between steps 1 (LibraryModelsHandler and core NullAway.java changes) and 2 (adding the LombokHandler and corresponding test case). Also, I am 50/50 on whether or not we should always ignore @lombok.Generated, instead of optionally handling it like we do for @Generated thrift code, etc. I do feel that, specially with auto-fixing and our need to check at least the public generated methods, we might not want to just default to always skipping the implementations, but the alternative might be playing wack'a'mole using this handler.

coveralls · 2023-05-31T20:25:46Z

Pull Request Test Coverage Report for Build #1097

0 of 0 changed or added relevant lines in 0 files are covered.
31 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.2%) to 92.718%

Files with Coverage Reduction	New Missed Lines	%
../nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java	1	96.3%
../nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java	8	97.79%
../nullaway/src/main/java/com/uber/nullaway/NullAway.java	22	95.83%

Totals
Change from base Build #1093:	-0.2%
Covered Lines:	5551
Relevant Lines:	5987

💛 - Coveralls

lazaroclapp · 2023-05-31T20:38:55Z

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()"),


Renamed this to avoid clashing with other tests using the common name Foo.bar()

msridhar

Mostly LGTM! Just a couple comments / questions. In particular I'm not sure I understand the changes to library model handling.

Also:

In addition to this, we can suggest a fix upstream in Lombok to propagate @nullable to these $default$ methods when present on the field, but this would not obviate the need for this PR, since we are keeping compatibility with multiple older Lombok releases internally.

I think we should do this; I'll bet the Lombok maintainers would want to fix.

msridhar · 2023-05-31T20:57:30Z

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

+              ? Nullness.NULLABLE
+              : Nullness.NONNULL;
+    }
+    return handler.onOverrideMethodInvocationReturnNullability(


Small thought: I wonder if we should rename the handler method onOverrideMethodReturnNullability? Seems like we are using now at more than just invocations.

msridhar · 2023-05-31T20:58:54Z

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

+    } else if (optLibraryModels.hasNullableReturn(methodSymbol, state.getTypes(), !isAnnotated)) {
+      return NULLABLE;


Is this fixing a separate bug unrelated to Lombok?

For consistency. Without this change library models specifying an annotated method is @Nullable will be ignored while analyzing the return statements inside such methods. In theory this also changes behavior for overriding, but the error will manifest only in the case where we have a @Nullable model for a method which overriding either a) a method in annotated code or, b) a third-party method with a @NonNull return as a library model. Guess this is probably worth some internal testing just in case I am missing some subtlety about our existing library models...

msridhar · 2023-05-31T21:03:53Z

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;


So before this PR, this line would result in a NullAway error? Just want to make sure we have the right NullAway config to test the fix here

Yes, though the error would be reported at the class level, since it actually happens in a new generated method of LombokDTO:

/test-java-lib-lombok/src/main/java/com/uber/lombok/LombokDTO.java:29: error: [NullAway] returning @Nullable expression from method with @NonNull return type @Builder ^ (see http://t.uber.com/nullaway )

Without the Lombok handler.

Yikes, glad we're getting rid of this awful error message!

msridhar

Now LGTM

msridhar · 2023-05-31T23:00:48Z

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;


Yikes, glad we're getting rid of this awful error message!

…tFuture (#771) We need this due to our incomplete support for generics, and the fact that #765 fixes a previous false negative which would cause our `@NonNull` model for Guava's `Function` to be ignored when determining the correct overriding of the functional interface by a lambda. After the fix, however, code such as: ``` FluentFuture .from(...) .transform(s -> { if(...) {...} else { return null; } }, executor); ``` will fail with an error about `Function::apply` having a `@NonNull` result. Where nullability should actually depend on the parameter `T` of the `ListenableFuture<T>` in question. Unfortunately, usage of this futures API is common internally for us, and our generics support is far from fully supporting this in a sound manner, so this diff introduces a (hopefully temporary) workaround, in the form of handler for the Futures/FluentFuture Guava APIs: This works by special casing the return nullability of `com.google.common.base.Function` and `com.google.common.util.concurrent.AsyncFunction` to be e.g. `Function<@nullable T>` whenever these functional interfaces are implemented as a lambda expression passed to a list of specific methods of `com.google.common.util.concurrent.FluentFuture` or `com.google.common.util.concurrent.Futures`. This is unsound, but permits the code example above to pass NullAway checking, while strictly reducing the safety hole of NullAway versions prior to PR #765.

….Default (uber#765)" This reverts commit d09ff9b.

Fix error inside Lombok generated code when dealing with @nullable @B…

2a57c54

…uilder.Default fields

lazaroclapp requested a review from msridhar May 31, 2023 20:20

Compatibility fix for EP 2.4.0

50f9fc0

lazaroclapp commented May 31, 2023

View reviewed changes

lazaroclapp added 2 commits May 31, 2023 16:55

Refactor to reduce code duplication.

3a212ef

And one more simplification

94d9829

msridhar reviewed May 31, 2023

View reviewed changes

Rename handler entrypoint

28ac48b

msridhar reviewed May 31, 2023

View reviewed changes

msridhar approved these changes Jun 1, 2023

View reviewed changes

lazaroclapp merged commit d09ff9b into master Jun 1, 2023

lazaroclapp deleted the lazaro_lombok_builder_default_test branch June 1, 2023 16:28

lazaroclapp mentioned this pull request Jun 21, 2023

Introduce FluentFutureHandler as a workaround for Guava Futures/FluentFuture #771

Merged

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023

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

b256598

….Default (uber#765)" This reverts commit d09ff9b.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023

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

b748c9e

….Default (uber#765)" This reverts commit d09ff9b.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023

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

38011df

….Default (uber#765)" This reverts commit d09ff9b.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023

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

b7c68a4

….Default (uber#765)" This reverts commit d09ff9b.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023

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

fd5ec12

….Default (uber#765)" This reverts commit d09ff9b.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error inside Lombok generated code for @Nullable @Builder.Default #765

Fix error inside Lombok generated code for @Nullable @Builder.Default #765

lazaroclapp commented May 31, 2023 •

edited

Loading

lazaroclapp commented May 31, 2023

coveralls commented May 31, 2023 •

edited

Loading

lazaroclapp May 31, 2023

msridhar left a comment

msridhar May 31, 2023

msridhar May 31, 2023

lazaroclapp May 31, 2023

msridhar May 31, 2023

lazaroclapp May 31, 2023 •

edited

Loading

msridhar May 31, 2023

msridhar left a comment

msridhar May 31, 2023

		} else if (optLibraryModels.hasNullableReturn(methodSymbol, state.getTypes(), !isAnnotated)) {
		return NULLABLE;

Fix error inside Lombok generated code for @Nullable @Builder.Default #765

Fix error inside Lombok generated code for @Nullable @Builder.Default #765

Conversation

lazaroclapp commented May 31, 2023 • edited Loading

lazaroclapp commented May 31, 2023

coveralls commented May 31, 2023 • edited Loading

Pull Request Test Coverage Report for Build #1097

💛 - Coveralls

lazaroclapp May 31, 2023

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

msridhar May 31, 2023

Choose a reason for hiding this comment

msridhar May 31, 2023

Choose a reason for hiding this comment

lazaroclapp May 31, 2023

Choose a reason for hiding this comment

msridhar May 31, 2023

Choose a reason for hiding this comment

lazaroclapp May 31, 2023 • edited Loading

Choose a reason for hiding this comment

msridhar May 31, 2023

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

msridhar May 31, 2023

Choose a reason for hiding this comment

lazaroclapp commented May 31, 2023 •

edited

Loading

coveralls commented May 31, 2023 •

edited

Loading

lazaroclapp May 31, 2023 •

edited

Loading