[BUG] `@NonNull` on array elements doesn't work

Yury-Fridlyand commented 6 months ago

Describe the bug

private void test (@NonNull String @NonNull [] args)

Such double annotation allows null strings in the array.

To Reproduce Define test functions

  private void t1 (@NonNull String[] args) {}
  private void t2 (String @NonNull [] args) {}
  private void t3 (@NonNull String @NonNull [] args) {}

Then call all 3

      try {
        t1(null);
      } catch (Exception e) {
        System.out.println("t1(null) error");
      }

      try {
        t1(new String[] { null });
      } catch (Exception e) {
        System.out.println("t1(new String[] { null }) error");
      }

      try {
        t2(null);
      } catch (Exception e) {
        System.out.println("t2(null) error");
      }

      try {
        t2(new String[] { null });
      } catch (Exception e) {
        System.out.println("t2(new String[] { null }) error");
      }

      try {
        t3(null);
      } catch (Exception e) {
        System.out.println("t3(null) error");
      }

      try {
        t3(new String[] { null });
      } catch (Exception e) {
        System.out.println("t3(new String[] { null }) error");
      }

Expected output:

t1(new String[] { null }) error
t2(null) error
t3(null) error
t3(new String[] { null }) error

Actual output:

t1(null) error
t2(null) error
t3(null) error

Expected behavior

private void test (@NonNull String[] args)

Such annotation should prohibit null strings in array.

private void test (String @NonNull [] args)

Such annotation should prohibit null array. (works now)

private void test (@NonNull String @NonNull [] args)

Such double annotation should prohibit null array and null strings in array both.

Version info (please complete the following information):

javac 17.0.10 from java-17-openjdk-amd64
lombok 1.18.30
gradle lombok plugin 6.4.0

Additional context Meanwhile, IDEA is smart enough to properly warn on annotations:

ShashankGarg24 commented 6 months ago

Hi, is this issue still open for contribution? if yes, then shall I pick it up?

Rawi01 commented 6 months ago

Probably a feature request, not a bug. It was never mentioned anywhere that @NonNull checks array elements.

A similar request (#2271) was rejected a few years ago.

rzwitserloot commented 6 months ago

@Yury-Fridlyand wrote:

I think yes, contributions are always welcome!

I think it's rather misleading to insinuate that a contribution is useful here when you aren't in the maintenance team. Please refrain from such comments. I have deleted it to avoid confusion.

rzwitserloot commented 6 months ago

The TL;DR: No, and we would deny PRs. Read on for details.

The semantic purpose of `@NonNull`

@NonNull is a weird concept that does two completely unrelated things, but, in practice, it ends up doing 3 mostly unrelated things:

It is documentary - Part of the purpose of this annotation is simply to exist as a tool to comment things in a way that tools can understand; it's better than attempting to AI-analyse your way through a javadoc comment. For example, you can make:

public @lombok.NonNull String getName() { ... }

To indicate that the getName() method will never return null. This annotation is purely documentary here: It does nothing. Lombok does not instrument all your return statements in this method to add 'throw an NPE instead of returning if you are about to return null'. It could, but it doesn't - also for good reasons.¹

However, @NonNull isn't solely documentary. If it is applied to a parameter, then lombok will scan the first few statements of your method and look for an explicit null-check on this parameter whose body is either throw, or a block that starts with throw. If it finds one, great, lombok does nothing. If it does not, lombok generates an if (param == null) throw new SomeException (exception type configurable; it is NPE by default).
This isn't directly @NonNull's own purpose, but a side-effect of how things like @RequiredArgsConstructor and @Setter interact with documentary-purpose non-null annotations: They are aware of them and react accordingly. It doesn't matter which non-null annotation you use, any non-null annotation that lombok is aware of has this effect, and @lombok.NonNull is, naturally, one such annotation that lombok knows about. Therefore, @NonNull on a field does, on its own, nothing² - but it does modify the behaviour of @RequiredArgsConstructor.

Yes, this does mean that @lombok.NonNull and something like @jakarta.NonNull are interchangible when applied to fields, but only @lombok.NonNull caused lombok to explicitly inject nullchecks if applied to a parameter. This is intentional.

So what about `void foo(@NonNull String @NonNull [] arr) {}`?

What are you asking for? This is a more general rule with feature requests - it would all become so much clearer if you explicitly write out: "THIS code with lombok stuff turns into THIS code without it" - I don't really care how obvious it seems to you. It never is.

Taking a wild stab in the dark, I bet that everybody reading this issue thread thinks that this is the obvious thing being asked, once you think about the question a bit:

void foo(String[] arr) {
  if (arr == null) throw new NullPointerException("arr");
  for (int i = 0; i < arr.length; i++) if (arr[i] == null) {
    throw new NullPointerException("arr[" + i + "]");
  }
  // actual code here

So, that simply innocuous little annotation might cause a million statements to be executed, if you pass in an array of a million elements. If the body of the method loops through the entire array and dereferences each item (that seems very likely to me), that was a big waste of time!

It's not about whether this argument convinces you. I explain the issue so you know what you have to do if you still want this feature: You have to convince us that the above analysis is incorrect or incomplete.

But, that's not the half of it, there's a much bigger issue. Imagine this method:

void foo(List<@NonNull String> args) {
  code();
}

It is very much an aggressive 'heck no!' denial for any feature request that says that in the array case, lombok checks each and every item in the array, but for the above code, that @NonNull does nothing. Because that is ridiculously unexpected. However, how could lombok possibly implement this? Only by hardcoding knowledge of how to perform this check for the java.util.List type. Which is highly problematic - what if some third party library has, say, Pair (Pair classes are a bad idea. Not relevant here - just pointing out this is not to be taken as an endorsement of that idea) - how do we ensure lombok knows what that means and knows what nullchecks to inject if you write void foo(Pair<@NonNull String, @NonNull Integer> param)?

Any answer is such a convoluted frameworky bonanza that the mental load on a dev team is massive. Easily outweighs any benefit it would bring.

The conclusion

Hence, no. Feature request denied. Do not send PRs; you'd be wasting your time. Unless it's a PR for adding a note in the docs to explain this (it should be in the smallprint, if anywhere).

[1] the reason we denied the PR of 'make @NonNull on a method add instrumenting code to all return statements inside this method to throw NPE if attempting to return null is an explicit, reasoned choice: Because then everybody is null-checking all the things at all times and that's a ridiculous amount of nullchecks. There is a key difference between checking preconditions stipulated in your API docs for a public/protected method, and checking 'private' conditions (where the author who determines what the conditions are is necessarily the same author that is to adhere to them. 'same java file' here implies, in our book, 'same author' for the purposes of this maxim).

[2] Think about it: If it did something, what would it do? Find every line of code in this source file that attempts to assign to this field and add a nullcheck to it? Find every line of code in this source file that attempts to read this field and add a nullcheck? Let's say it did. Take a moment to start counting. You'll find that generally then 90% of all generated bytecode ends up being null checks. And nevermind that fields can be public - should lombok now start modifying code that has no lombok annotations in it at all? You see why this is a big 'no' from us - it is clear what one would want, and it is clear lombok cannot possibly deliver. Trying to deliver a fraction of what one would want/expect is confusing. Better to be clear and deliver nothing.

projectlombok / lombok