Support all varieties of Null handling

[Randgalt]

Yes that will probably be good for all. However I can't keep working on #106 or the other nullable annotation bugs till some decisions are made. Which is why I have been so back and forth on this as there are some serious previous design decisions that are impacting me.

So I'm fine with holding off indefinitely till some decisions are made (like lets copy what immutables does).

And I'm sorry for coming off rough and flooding the inbox but I have already sunken some time on this (a lot more than just changing some call from of to ofNullable) as I sort of felt obligated given previous discussions on reddit and whatnot.

I'll stop work and commenting till the dust clears.

Originally posted by @agentgt in https://github.com/Randgalt/record-builder/issues/107#issuecomment-1104038505

[Randgalt]

I'm opening this new issue to deal with any Null issues in Record-Builder. I'd like to stipulate a few things to keep in mind for this discussion and any solutions:

• Record-Builder is a targeted at a wide audience. That audience will contain users that are mostly concerned with utility over correctness or compliance with a spec
• Record-Builder has a well defined options/customization facility that can be used to generate builders and ancillary classes that are very specialized

Given this - what what needs to be done here. I disagree that the current implementation is wrong in any way. We can use the options to generate classes as needed. So, what should the generation be for the purposes of this issue?

cc @agentgt

[agentgt]

See my comment here: https://github.com/Randgalt/record-builder/issues/106#issuecomment-1120409923

Pasted below here for others:

EDIT the below is not for runtime support (e.g. requireNonNull but to aid static analysis tools like checker and eclipse and whatever jspecify comes up with). Runtime null handling is a different issue than what I was addressing.

---

@Randgalt I'm sorry I just don't have time anymore. One of the reasons I was "flooding the inbox" was that was my window to work on your project. That was why I was being urgent.

As for production code... you have never seen or used?

• jOOQ
• Guava
• Checker framework
• https://javadoc.io/doc/org.jetbrains/annotations/latest/org/jetbrains/annotations/Nullable.html

Anyway it doesn't really matter because I think the best thing to do is something like what Immutables does and provide your own annotation say @RecordBuilderNullable (and NonNull) (of target type RECORD_COMPONENT. You can also make that annotation TYPE_USE as well as all the other targets like Jetbrains (see link which is why I suspect you think you haven't seen them).

Then wherever you like to do configuration (perhaps package level annotations) users can specify what annotation they would like propagated. This is somewhat how jOOQ works.

For example

record MyRecord( @MyNullable /* the previous annotation will not be read by record builder */ @RecordBuilderNullable String field) {}

Then in configuration you say RecordBuilderNullable = MyNullable.class.

Then when you generate any method you just bring along MyNullable.class.

The only issue at that point is pure TYPE_USE annotations (as in no other @Target) like JSpecify and Eclipses annotations. For those when you make your "withers" you will need print the type different if you print the FQN.

Going back to the example if @MyNullable is TYPE_USE which you can figure out much easier than if a type has that annotation is do this for the wither:

MyRecordBuilder with(java.lang. @MyNullable String field);

instead of

MyRecordBuilder with(@MyNullable java.lang.String field);

I think but am not sure some generators get around this issue by importing instead of FQN but that will eventually fail.

The above practice will avoid the nasty issue of pulling TYPE_USE annotations from fields which I admit is difficult given the bugs of both the JDK and JPoet.

Anyway I don't have anymore time for now for this project but wish you best of luck. I'll try to answer anymore questions you have.

[Randgalt]

Thanks for the description - I'll see what can be done

[agentgt]

I forgot to add that there are three "use cases" of null handling you should consider:

1. Compile time Static Analysis (what #106 was originally about)
2. Runtime Assertion (requireNonNull) aka defensive programming
3. Runtime Validation (Java Bean Validation API aka javax.constraints)

I know you talked about not really complying to a spec but about utility and the above are three use cases when dealing with null.

As for:

I disagree that the current implementation is wrong in any way.

When I said the library was "wrong" it was because you or someone who did the NotNull for "Full Record support" made number 3... aka Validation do all three of those by accident. This was especially bad because static analysis and runtime assertion can be a direct contradiction of what javax.constraint.validation api needs.

That is a very common use case is to have:

@checkerframework.Nullable @javax.constraint.validation.NotNull String someField; 

(replace checkerframework with any static analysis annotation)

However the generated code does a:

withSomeField(@javax.constraint.validation.NotNull /* <- that validation annotation should not be there and the nullable one is missing because its TYPE_USE */ String someField) {
Objects.requireNonNull(someField); // and this is wrong because we expect it to be null
}

How can you ever validate an object if you cannot make it because NPE is thrown? That is @javax.constraint.validation.NotNull does not mean never null... it means the opposite. It means we expect it to be null and we will tell the user. Furthermore I believe javax.constraint.validation in some cases doesn't even want the annotations propagated (in fact it used to fail if you annotated a getter and a field)

The irony is if you do use static analysis (you need far less to almost zero defensive programming with static analysis) and java validation the only time a requireNonNull would ever be needed is if you wanted to support Optional (which is exactly what immutables does).

I explained this extensively over and over again in #107 . See this comment validation api: https://github.com/Randgalt/record-builder/issues/107#issuecomment-1102624501

So it's not "wrong" in terms of it doesn't crash. In fact it arguable other than not supporting TYPE_USE did indeed propagate the annotation. However it certainly doesn't do out of the box what most folks who know what the validation api is supposed to do. It certainly doesn't do what Spring expects in its @Valid support (because you would blow up before the object made it to the controller). This all because the default regex on NotNull annotations. (However don't forget you did show the example with javax.validation w/ your gist).

So since you don't want to comply to spec I recommend the best solution is to do what Immutables does and make your own annotations for the various null handling. And while Immutables isn't your goal in my opinion it currently does a better job than this library because the exact scenario I had with validation/static analysis contradiction didn't happen on my first stab at the library (it also has semver... not that I love semver it does give me some idea of the backward compatibility).

It is not about being "wrong" or "right". I should not have said that. That wasn't my point. It was about making people use the other libraries you plan on making "specialized" classes for correctly as those do have specs and designs. It's about being aware of a specific portion of annotations that has been around since Java 8. Static analysis loves code generation (since reflection cannot be verified) and annotation processing. Its about knowing validation needs nulls. It's also about planning things correctly from the beginning so you don't break peoples code later on every upgrade or make a trillion little switches of configuration.

You advertise the library frequently on reddit so imagine I won't be the only once with some opinion that what the libraries is not exactly the "right" way.

So it is not "wrong" but can we agree it needs to be improved?

[Randgalt]

As I said, I've never encountered a TYPE_USE annotation and thus don't know what to do. I'm happy to it make it work properly but I need to understand what is wrong and what can be improved. I hope we can just focus on the problem and eliminate any superfluous comments. For example, in #106 you say:

Let us assume @Nullable is a TYPE_USE like it is for JSpecify.

    @RecordBuilder
    public record MyRecord(@Nullable String name, String required) {}

The record builder should copy the type java.lang. @Nullable String for wherever name is used.

I searched around and the docs for TYPE_USE (even that JDK bug report) are opaque. Wouldn't it also be correct to just copy the TYPE_USE @Nullable without the FQPN? e.g.

// in builder...
public void name(@Nullable String name) {
   ...
}

My read of the spec says that the @Nullable would bind to the type String. Record-Builder doesn't use FQPNs.

It's a 2 line code change to get the Record Builder processor to recognize the TYPE_USE annotations. Let me know if this is correct for numbers:

Given:

@Documented
@Retention(value = RUNTIME)
@Target(value = {TYPE_USE, RECORD_COMPONENT})
public @interface TypeUseNullable {
}

@RecordBuilderFull
public record FullRecord(@TypeUseNullable List<Number> numbers, @NotNull Map<Number, FullRecord> fullRecords, @NotNull String justAString) {
}

    @Generated("io.soabase.recordbuilder.core.RecordBuilder")
    public FullRecordBuilder numbers(
            @TypeUseNullable Collection<? extends Number> numbers) {
        this.numbers = (numbers != null) ? new ArrayList<>(numbers) : null;
        return this;
    }

    /**
     * Return the current value for the {@code numbers} record component in the builder
     */
    @Generated("io.soabase.recordbuilder.core.RecordBuilder")
    @TypeUseNullable
    public List<Number> numbers() {
        return numbers;
    }

[agentgt]

What happens when you have name conflicts? A dumb example is com.stuff.String and java.lang.String. At some point I assume you will need to print the FQN?

No it will not work for all cases. You also need to worry static inner classes, arrays and generics.

For example you can have a record like:

record MyRecord(
    Map<String, @Nullable String> model, 
    @NonNull String @Nullable [] ) {
}

You basically need to print the type exactly as it is and that is what the JDK bug is about. You should be able to just call toString on the TypeMirror so you do not have to use a crazy complicated Visitor in the annotation processor to recreate it (as well as deal with name conflicts).

In fact you probably do not even need to try to interpret the meaning of the TYPE_USE annotations but just propagate them. That is all you have to do.

However your solution might just be good enough (e.g. 80-20) till the JDK makes it easier.

---

Apologies on the superfluous comments but it is very complicated subject and is extremely frustrating when someone is dismissive with "I am not wrong" or the implementation "is not wrong". That makes me feel like I have to go out of my way to show its problems.

[Randgalt]

OK - that example helps - there's definitely more work to do. I'll also add some conflicting names to would surface FQPNs. Currently, I have:

@RecordBuilderFull
public record MyRecord(
    Map<String, @TypeUseNullable String> model,
    @TypeUseNonNull String @TypeUseNullable [] values) {
}

I'll make something that has a duplicate name for it.

So, that's the TYPE_USE issue WIP. I'll move to some of the other nullable/nonnull handling soon.

[Randgalt]

FYI - I did a lot of playing around/investigating. In order to get a complete solution to the various possible positions of TYPE_USE annotations I'll need to make some changes to JavaPoet. JavaPoet appears to be abandoned by Square so I could fork it. I did some testing with a fork and was able to solve most of the issues. However, I feel that this is a step too far. We can get an 80% solution that solves how most people use Checker framework's null annotation with a few fixes in Record-Builder. I'll have a PR with that soon. Afterwards, we can consider forking JavaPoet or some other alternative.

JavaPoet appears to be abandoned by Square

Update: maybe not - there has been some recent activity ¯\(ツ)/¯

[brainbytes42]

Hi, really cool tool - and maybe it's already possible, but I didn't find it: How can I make may complete record null-safe without annotation each and every single parameter or using other frameworks? I searched for something like interpretNotNulls in RecordBuilder.Options, in my case maybe "requireNonNull" or even better "defaultRequireNonNull" to allow for some explicitly annotated nullable values... (If I need to annotate each and every field, this might be errorprone and repetitive; and for the default-non-null, it would be more obvious which values are nullable, other than when all parameters are annotated and look visually almost the same.)

[NicklasWallgren]

Are there any annotation similar to lomboks NonNull which adds Objects.requireNonNull() or similar?

[Randgalt]

Are there any annotation similar to lomboks NonNull which adds Objects.requireNonNull() or similar?

I played around with some ideas but they all required bytecode modifications and I'd like to stay away from that. See here: https://github.com/Randgalt/record-builder/discussions/120