Enforce @NonNull on method return and locals

[Maaartinus]

As @lombok.NonNull (unlike related annotations) really means "never ever null", it should be enforced not only in generated methods and on fields, but everywhere. I find it disappointing to see

@NonNull Object fail() {
    @NonNull final Object result = new HashMap<>().get("");
    return result;
}

returning null. Actually, I can't see any reason for placing @NonNull on methods or locals, other then enforcing the check.

I'm afraid that for compatibility reasons (i.e. for people happily returning null from such methods), there need to be a corresponding configuration key.

[rzwitserloot]

So, you want lombok to silently inject: if (result != null) throw new NullPointerException("Erm, what do we put here then?");? I'm not sure that's particularly useful; presumably your method is going to blow up all by itself soon. Well, it might not. Still, there's a pretty big difference between @NonNull on a param and on your internal code (that is, a local var or the return type): Params can always be null, at any time, due to changes in code or because other libraries etc call you with weird values. But, what you set your own locals to, and what you return, that's within your control.

Your IDE should just generate the warnings; the right move is NOT to generate an actual at-runtime check. Runtime checks are of dubious value here; you really want static code analysis.

We have 3 options here:

(A) Disallow the use of @NonNull on methods / method return types and local variables because we have no intent on generating any code for this.
(B) Carry on as normal: Acknowledge that @NonNull also has a documentary purpose and might be a part of your IDE's/analyser's configuration of static code analysis. It's got a retention policy of 'class' for this reason.
(C) Write an analyser. Because if we don't do the same deep and resolution-requiring analysis that IDEs do, then your static analyser is going to whine about the lombok-generated null check!. Presumably, anything marked @NonNull is usually already determinable as never null by analysis, so you'd run into that, let's say 80%+ of all the stuff you marked.

C is impossible. A is backwards incompatible. B is the choice we're going with.

[mbjelac]

Yes, but what about cases where I get a property from a bean and just want to validate it's non-nullness?

I don't like Objects.nonNull because it throws NullPointerException when really I want IllegalArgumentException plus the name of the field/variable, which Lombok's @NonNull provides.

See this gist for an example test which I would like to pass.

[aplatypus]

One approach I've used before on similar situations is to have a small utility method to check my post-conditionss/ constraints. Something like ...

<code ...>
assertExitConstraints(
    (result    != null),
    (counter <  MAX_TRIES),
);

Where the helper method assertExitConstraints( boolean... ) throws the Exception you want to see. In my case I have a Constraint Error. It would also be possible to pass the type of exception or exception(s) as parameters if that were desirable.

Personally, if the error was in the parameters that is when I expect an: IllegalArgumentException. Not on the return value. It sounds like the null check we're discussing is a proxy for not validating the input constraints / pre-conditions. For that case I use the same pattern wth assertEntryConstraints(...).

I find these most useful in Spock specifications and JUnit tests.

[TreffnonX]

Despite this issue being dead for about 5 years, I would ask to reassert it's validity. If not by default, at least a configuration to assert objects to not be null on return would be very helpful. Usually, if a method has only one return statements, it is relatively easy to throw on value == null, however, if for any reason your code has multiple return statements (for example within a switch clause), then asserting explicit non-nullity might become cumbersome.

Params can always be null, at any time, due to changes in code or because other libraries etc call you with weird values.

This is in fact the best possible argument for @NonNull on method return. If an underlying API changes, or if the input becomes faulty, or if any state of a related object cannot be handled, but the unterlying API does not throw, but instead return null, then I want that to 'fail fast', so it blows up the first time I run my test. If not, it might be carried to production and fail there later on.

As an example, if I have a wrapper method which passes one of many incoming parameters to an underlying API, and that value (while not being null) is faulty for some reason, and the API returns null, then I will only notice, once I call on the value, or if I explicitly check for that, which might be hours, days, or months later, if the value was since persisted to a database e.g..

It is a lengthy discussion on programming paradigms when and where null is acceptable, and not every project will be handled the same way, and underly the same paradigms. I respect that, which is why I believe this should be configurable. And I really believe, that the following would be relatively slim to implement:

public @NonNull String exampleString() {
    return null;
}

becoming

public String exampleString() {
    return Lombok.nonNull(null);
}

with a Lombok.nonNull looking akin to this:

public static <T> T nonNull(T value) {
    if (value == null) {
        throw new NullPointerException...
    }
    return value;
}

[Maaartinus]

@TreffnonX I'm afraid, Lombok.nonNull is impossible (Lombok is purely compile-time), but Objects.requireNonNull does exactly this (and Lombok can do it already #1197 (comment)).

This feature would surely require a specific configuration. A fool-proof setting would be to use assert someValue != null : "someValue was null", which would do the check in my tests (running with assertion on), but would do nothing in production (that's a good thing, as activating the checks afterwards is a bit risky).

Concerning the static analysis which would make this superfluous - somehow I still don't get it running smoothly.

[TreffnonX]

Ah, I was falsely assuming that Lombok preferrably used it's own utility methods for things like this, but Objects.requireNonNull is just perfekt for this.

(that's a good thing, as activating the checks afterwards is a bit risky).

TBH. I would rather have my production throw errors on me than search for data inconsistencies. But I believe that to be dependant upon personal preference, and of course the project's context.

[rzwitserloot]

Null is far more complex than this; I'm done trying to explain the garbage fire that is nullity annotations and null checks in general on an issue per issue basis because it is taking up tons of time. I regret that I can't get into it more, but these kinds of issues shall remain closed, and any debate on why or how lombok can be of use can be held in person at conferences or if you're anywhere around Delft, The Netherlands, contact us and we'll meet. Make sure you free up an hour or 3 on your calendar, because it is that complex.

What's left for me to say is: Oh, I have thought about this. Lots of time spent thinking about it, in fact. It's not this simple, this won't work well. I may be wrong, but going a few rounds on an issue tracker is not an acceptable use of time.

[TreffnonX]

Sorry, I was very much unaware of the deep implications of my question and apologize for it. It won't Happen again. I might take you up on your offer though, when in the netherlands again.