skinny85 / jilt

Java annotation processor library for auto-generating Builder (including Staged Builder) pattern classes
Other
240 stars 13 forks source link

Support `@Nullable` to mark field as optional #11

Closed maciejwalkowiak closed 7 months ago

maciejwalkowiak commented 10 months ago

In addition to @Opt it would be great if fields annotated with @Nullable were considered optional out of the box.

The tricky part is that there is no standard @Nullable annotation (jspecify is emerging but it will take years till it is really considered a standard), so either jilt would have to support all of the most popular ones, or it would have to be configurable.

Ones I can think of:

skinny85 commented 9 months ago

Hi @maciejwalkowiak, thanks for opening the issue!

I think this is a great idea, and makes a lot of sense.

The situation with the various annotation standards is indeed pretty complex, as this StackOverflow question details. The only saving grace for us seems to be the fact that all of the null-permitting annotations are called @Nullable (which is not the case for the null-forbidding ones, which have a few variants like @Nonnull, @NotNull, @NonNull, etc.).

So, I'm thinking about implementing the following behavior: any field/parameter annotated with a @Nullable annotation makes that property optional, regardless of what Java package that annotation comes from. That should cover all of the existing libraries, and also jspecify, as it also uses @Nullable.

Thoughts on that approach?

Thanks, Adam

maciejwalkowiak commented 9 months ago

Sounds great!

skinny85 commented 9 months ago

OK, this is now done on the develop branch. Here's an example value class, and here's a test using it.

In addition to making the property optional, I also added code to propagate the @Nullable annotation into the setter methods arguments - I'd figured it makes sense to have them there, otherwise the IDE might show warnings when using them.

I'll include this in the next release, along with #5 - after I'm done with #10 and #12.

skinny85 commented 9 months ago

This has been fixed in the latest Jilt release (1.4).

I'm closing this issue as "Resolved", please comment if you run into any more problems related to this area, and I'll reopen the issue.

alexandrenavarro commented 7 months ago

Hi,

I’m trying to migrate from jakarta.annotation.Nullable to org.jspecify.annotations.Nullable and from @lombok.Builder(toBuilder = true) to org.jilt.@Builder(style = BuilderStyle.STAGED) on real project and try to use errorprone / nullaway / spotbug / checker to avoid as much as possible NullPointerException at runtime and detect null potential problems at compiletime.

I discovered a weird bug.

The staged builder works well when you annotated a field with jakarta.annotation.jakarta.annotation.Nullable, org.jetbrains.annotations.Nullable, org.springframework.lang.Nullable, edu.umd.cs.findbugs.annotations.Nullable, javax.annotation.Nullable but does not seem to work with org.jspecify.annotations.Nullable.

I checked quickly the code, it seems ok at first look, I suspect the values set in @Target in the different annotations impacted the retrieval of in AbstractBuilderGenerator / annotation.getAnnotationType().asElement().getSimpleName().toString().

Don’t hesitate if you need more informations to reproduce, I can have a look if needed.

Thanks in advance.

skinny85 commented 7 months ago

Hey @alexandrenavarro,

thanks for re-opening the issue. This is surprising, since we actually have a unit test that uses javax.annotation.Nullable here: https://github.com/skinny85/jilt/blob/master/src/test/java/org/jilt/test/data/nullable/FullName.java.

Can you share a small reproduction of the issue? Maybe in a separate repo?

alexandrenavarro commented 7 months ago

Yes I will do it and send you the link.

alexandrenavarro commented 7 months ago

I did a sample to show the problem

https://github.com/alexandrenavarro/nullcheck- sample/blob/main/src/main/java/com/github/alexandrenavarro/Person.java

It seems not working only on org.jspecify.annotations.Nullable (I retested on javax.annotiation.Nullable it works and the others I tested works). I suspect the @Target({ElementType.TYPE_USE}) of org.jspecify.annotations.Nullable .

Don't hesitate if it is not clear.

alexandrenavarro commented 7 months ago

I just modified your FullName with org.jspecify.annotations.Nullable instead of javax.annotation.Nullable and the compilation of FullNameTest failed.

skinny85 commented 7 months ago

Thanks, confirming I was able to reproduce the problem. I'll dig in what the issue might be.

skinny85 commented 7 months ago

Yes, there is something wrong with JSpecify.

Here's the field annotated with @org.jspecify.annotations.Nullable as Jilt sees it:

jspecify-attribute

And here's a field annotated with @jakarta.annotation.Nullable:

jakarta-attribute

So, Jilt doesn't even see the annotation on the field.

Unfortunately, that means you'll need to annotate your field or parameter (I tried annotating a constructor parameter with @org.jspecify.annotations.Nullable, same result) with an explicit @Opt annotation if you want to make it optional.

I'm not sure what exactly JSpecify is doing to not be able to find these annotations, but I don't see what Jilt can do in this situation 😕.

skinny85 commented 7 months ago

Nvm, I figured it out 😛. Fuck me, these annotation processor APIs are weird.

I'll have a fix out with the next version of Jilt.

skinny85 commented 7 months ago

This has been fixed in Jilt release 1.5.

I'm resolving this issue, please comment if you run into this problem again, and I'll be happy to reopen.