Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The combination of JSpecify + Lombok does not seem to work well #917

Closed
kjim opened this issue Feb 17, 2024 · 6 comments
Closed

The combination of JSpecify + Lombok does not seem to work well #917

kjim opened this issue Feb 17, 2024 · 6 comments

Comments

@kjim
Copy link
Contributor

kjim commented Feb 17, 2024

Background

Originally, I was using JSR 305 for @Nullable annotation and was recently trying to migrate to JSpecify, but with JSpecify's @Nullable annotation, Lombok handling of NullAway written in the README does not seem to work well. I thought it was quite possible that this was intentional JSpecify support in NullAway, but it makes the transition to JSpecify for projects using NullAway very difficult.

So I would like to know about the following:

  • Is this behavior of JSpecify intentional? or bug?
  • Is there a workaround for the combination of NullAway + JSpecify + Lombok?

Reproducer

Below is a sample code that I have tried. (The following code is committed to my github repo as reproducer)

public final class NullableWithLombok {

    public void jsr305() {
        new JSR305NullableValue(null);
    }

    public void jakartaAnnotation() {
        new JakartaAnnotationNullableValue(null);
    }

    public void jetbrains() {
        new JetbrainsNullableValue(null);
    }

    public void checkerframework() {
        new CheckerframeworkNullableValue(null);
    }

    public void jspecify() {
        new JSpecifyNullableValue(null);
    }

    @lombok.Value
    private static class JSR305NullableValue {
        @javax.annotation.Nullable
        Object value;
    }

    @lombok.Value
    private static class JakartaAnnotationNullableValue {
        @jakarta.annotation.Nullable
        Object value;
    }

    @lombok.Value
    private static class JetbrainsNullableValue {
        @org.jetbrains.annotations.Nullable
        Object value;
    }

    @lombok.Value
    private static class CheckerframeworkNullableValue {
        @org.checkerframework.checker.nullness.qual.Nullable
        Object value;
    }

    @lombok.Value
    private static class JSpecifyNullableValue {
        @org.jspecify.annotations.Nullable
        Object value;
    }

This code will make the . /gradlew check will report an error due to NullAway as shown below.

$ ./g check

> Task :nullaway-lombok:compileJava FAILED
/Users/keiji.muraishi/workspace/smartnews/kjim/nullaway-example/nullaway-lombok/src/main/java/org/example/NullableWithLombok.java:22: error: [NullAway] passing @Nullable parameter 'null' where @NonNull is required
        new JSpecifyNullableValue(null);
                                  ^
    (see http://t.uber.com/nullaway )
1 error

FAILURE: Build failed with an exception.
@msridhar
Copy link
Collaborator

Thanks for this report! This is not intentional behavior; it's quite surprising in fact. Let me dig a bit to see if I can fix.

@msridhar
Copy link
Collaborator

Based on projectlombok/lombok#3346 I figured out the right config required here:

lombok.copyableAnnotations+=org.jspecify.annotations.NonNull
lombok.copyableAnnotations+=org.jspecify.annotations.Nullable

If you want Lombok to use JSpecify annotations for generated code you can also do:

lombok.addNullAnnotations = CUSTOM:TYPE_USE:org.jspecify.annotations.NonNull:org.jspecify.annotations.Nullable

To avoid needing any extra config, I think these two lines in Lombok need to be updated to use the latest JSpecify package names and to refer to @NonNull instead of @NullnessUnspecified:

https://github.com/projectlombok/lombok/blob/8bea25107de8177596546574c02ed204366e1281/src/core/lombok/core/handlers/HandlerUtil.java#L188-L189

(@cpovirk should @NullMarked / @NullUnmarked be in that list as well?)

In any case, let me know if the above config works for you. I'll leave this issue open until we can open an appropriate PR / issue on Lombok.

@kjim
Copy link
Contributor Author

kjim commented Feb 18, 2024

Thanks for the info. I tried your suggestion on https://github.com/kjim/nullaway-example/pull/1/files and the issue has been gone.

In my case, simply adding the following one settings seems to solve the problem (since I don’t use @NonNull).

lombok.copyableAnnotations+=org.jspecify.annotations.Nullable

This workaround seems to work well. Until jspecify annotation is added (or changed?) on lombok side, I can proceed with the migration by adding this setting. Thanks!

@msridhar
Copy link
Collaborator

I opened a PR to fix this upstream in Lombok: projectlombok/lombok#3613

@cpovirk
Copy link

cpovirk commented Feb 22, 2024

Thanks for the report and the fix.

As for @NullMarked and @NullUnmarked: My read is that Lombok's lists are for copying annotations from fields only. If so, then we wouldn't have reason to list those annotations, as they aren't applicable to fields (only to methods/classes/packages/modules).

@msridhar
Copy link
Collaborator

msridhar commented Mar 3, 2024

Fixed upstream in projectlombok/lombok#3608

@msridhar msridhar closed this as completed Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants