Randgalt / record-builder

Record builder generator for Java records
Apache License 2.0
758 stars 55 forks source link

Fix NPE for uninitialized non-null collection #92

Closed cykl closed 2 years ago

cykl commented 2 years ago

Closes #91

This is a tentative patch for #91. I'm not familiar with record-builder code base, so I prefer to share an early draft and get feedback before trying to polish it.

The idea is to move collection shims invocation before null checks.

Before:

    public Removeme.R build() {
        Objects.requireNonNull(l, "l is required");
        return new Removeme.R(__list(l));
    }

After:

    public Removeme.R build() {
        l = __list(l);
        Objects.requireNonNull(l, "l is required");
        return new Removeme.R(l);
    }

Shortcomings:

Randgalt commented 2 years ago

@cykl thanks for the PR. I'm thinking that the Objects.requireNonNull calls are not needed actually. The values will never be null right? Wouldn't a simpler solution be to not emit the Objects.requireNonNull calls when useImmutableCollections is true?

cykl commented 2 years ago

If think that your are right. Objects.requireNonNull is not required for collection members when useImmutableCollections is true since the shim ensure that it is not. However, it is still required for non collection members.

I still have to check what happen if caller pass null to the builder (I expect the "setter" to catch that but haven't checked).

Because I'm not yet comfortable with all the features and code of record-builder, my approach was to write the safest & dumbest code. It should compose nicely with any other feature at the expense of prettiness of generated code. I'm not too worried about performances as I think JIT will eliminate all those useless checks and assignments.

If we agree on the goal and you think there are no obvious pitfalls to avoid, I can try to improve the patch.

Randgalt commented 2 years ago

With useImmutableCollections passing nulls to the builders is fine. It will get converted to an empty collection.

Randgalt commented 2 years ago

Closing in favor of #98