Skip to content

FEATURE PROPOSAL: null checking @Singular

Jump to bottom
Reinier Zwitserloot edited this page Jan 17, 2020 · 3 revisions

Current behaviour

Currently, @Singular List<String> names; generates the following method:

    public BuilderType names(Collection<? extends String> names) {
        if (this.names == null) this.names = new ArrayList<String>();
        this.names.addAll(names);
        return this;
    }

As you can see, this means that if you pass in null as argument, you get a NullPointerException with no message. This is... suboptimal.

use-cases

  • API runtime – When users of the builder pass in a null, it'd be a lot nicer if they get an exception with a message explaining what the problem is here. For example, add if (names == null) throw new NullPointerException("names may not be null"); to the top of that generated method.
  • API write-time – If using nullity status annotations (and there are so many.. here is a list of common non-null annotations) it would be nice if lombok annotated the parameter to help you out at write time.
  • Serialization – If using jackson or other deserialization tools, and the serialized input data (for example, your JSON) contains an explicit null value for the collection, then the desired behaviour is either that the object that builder will make ends up with an actual nullref for the collection, or, that the object that builder will make ends up having an immutable empty collection. Whilst making an explicit nullref would most closely mirror the serialized form, the notion of 'perfect deserialization' is weird in the first place, and stylistically I just cannot bear to write code that would result in a collection variable holding either null or an empty collection somewhat arbitrarily, so that use case (The collection ends up being null) is right out, but there's room to say that the collection ends up being an immutable empty list. Currently, you'd get a NullPointerException.
  • Legacy API – Sometimes, making that collectionref be null is required. We have decided not to cater to this directly in any case – If the collection must be null instead of empty, you can always make a constructor, annotate the parameter with @Singular, annotate the constructor with @Builder, and explicitly write this.collection = collection.isEmpty() ? null : collection to support your legacy stuff.

Proposal

Singular gets a parameter to configure the behaviour, with 'throw a null pointer exception with a nice message' becoming the default. The options are:

  • Throw NullPointerException with a message.
  • Throw IllegalArgumentException with a message.
  • Use JDK to do it (Objects.requireNonNull).
  • Use Guava to do it (it has a similar method).
  • do nothing if null is passed.

These options mostly mirror the configurable behaviour for @NonNull, except this time you CAN configure it on the parameter (because I can foresee that the desired behaviour is different per usage of @Singular, which isn't the case for putting @NonNull on parameters). You can also configure it in lombok.config.

In addition, assuming you do NOT pick the 'ignore' option, AND the @Singular-annotated field has a known non-null or nullable annotation, OR a known non-null or nullable annotation or star-import of a relevant package is in your import list, lombok adds this non-null annotation to the parameter. (It will add all known non-null annotations found with this algorithm).

If choosing the ignore option, the same algorithm applies, except the nullable variant is used instead.

There is no way to configure this; lombok does it automatically. Note that if you use a package/module-level 'everything is non null by default' annotation, lombok won't find that, but that should be allright, as that very package-level 'it is non-null' will automatically apply. The one problem is that combined with the ignore-null behaviour, but that sounds like an extremely exotic situation (who is that aggressive about eliminating null from the code-base whilst still supporting legacy/deserialization null stuff?) – and you can STILL address that in a roundabout way, by having the Nullable annotation in your import list.

Impact on existing code

Assuming a lombok user doesn't know about this update, doesn't change anything in their codebase, and updates their lombok dependency, then their code gets a free upgrade; instead of getting an NPE with no message, they now get an NPE with a message, and potentially their collection param is festooned with an annotation. This is theoretically backwards incompatible if they subclass their builder, but that is so incredibly exotic I'm okay with not catering to this.

It is a bit annoying that the behaviour of their code will slightly change depending on who last compiled the file if others in their team haven't updated their lombok yet, but I don't think it's worth making this an opt-in feature to cater to this scenario either.

I don't think it is possible that we actively break / change the behaviour of a program that already compiled and ran correctly before this change (any code that looks at the NPE's getMessage() would notice, but java itself changes these messages).

It IS possible that unit tests actually inspect this message, but I'm okay with forcing the authors of such thorough and specific tests into writing a slight update to their tests. Regrettable, but the JVM itself is in the business of changing exception detail messages and considering this 'backwards compatible' already, so clearly it is acceptable to follow suit.

Relevant discussions

See issue #2221 which inspired this proposal.

Clone this wiki locally