3

I have an interface with a method like this:

public interface MyInterface {
    void myMethod(@Nonnull String userEmail);
}

The interface has the following implementation:

public class MyInterfaceImpl implements MyInterface {
    @Override
    public void myMethod(@Nonnull String userEmail) {
        if ((userEmail== null) || !userEmail.endsWith("something")) {
            throw new SomeException("...");
        }
        ...
    }
}

The compiler is raising me a warning saying that Condition 'userEmail == null' is always 'false', but that doesn't look right.

As for my understanding, the annotation javax.annotation.Nonnull will warn the compiler in case someone calls my method with a null value, but does not prevent the code to compile if someone passes a null value to it. So yes, my code can be called with a null value at some point:

Compiler warning


Debugging with a null value

Note that I get the same warning if I compile on command line with the option -Xlint:all (so it does not look like just a bug in my IDE).

Does anyone have any idea how can I get rid of this warning and why is it occurring?

Disclaimer: the example I showed is just an example, the actual code does some stuff before to get to that condition, but nothing that can make userEmail == null always false (as it's proven by the debugger screenshot that I attached).

Matteo NNZ
  • 11,930
  • 12
  • 52
  • 89
  • @ruakh Java version: 11.0.2-BellSoft, vendor: BellSoft, runtime: C:\jdk-11.0.2 – Matteo NNZ Jun 04 '21 at 07:50
  • 2
    Which `@Nonnull` is that? I thought source-level annotations like that (like the one given by lombok) actually generate an `if` block that checks for null in the code. If that's the case, then the compiler is right. You might need to decompile your class file to see exactly what code ended up there. – ernest_k Jun 04 '21 at 07:54
  • @ernest_k: Lombok's annotation processor only does that for Lombok's own @NonNull. `javax.annotation.Nonnull` (which the OP is using) doesn't have that behavior. – ruakh Jun 04 '21 at 08:00
  • @ernest_k the decompiled code does not show any if block checking for null. In any case, I managed to screenshot a breakpoint on that same line with a null value, so I don't think that's the reason :) – Matteo NNZ Jun 04 '21 at 08:08
  • 1
    @Nonnull doesn't do what you think. It tells the compiler that the argument it applies to cannot be null. So your `== null` test cannot be true. – user207421 Jun 04 '21 at 08:21
  • @user207421 but if that's true, how can I compile a test where I explicitly pass null to the method? Shouldn't that throw a compilation error? – Matteo NNZ Jun 04 '21 at 08:26
  • Exactly. It should give a compliation error. So you can't compile it. So you don't need to write that test. That's the whole idea. See [here](https://checkerframework.org/manual/#nullness-checks). – user207421 Jun 04 '21 at 08:30
  • @user207421 That's not builtin to Java. Look for "Optional Type Annotations are not a substitute for runtime validation" in https://blogs.oracle.com/java-platform-group/java-8s-new-type-annotations. – kabanus Jun 04 '21 at 08:43
  • @user207421 I see your point, the problem is that I can compile my code passing null... – Matteo NNZ Jun 04 '21 at 08:45
  • @Matteo it looks like this is a linter warning, perhaps you have linting enabled or some plugin such as the checker framerwork user207421 links to? The Linter can guarantee no null values will reach that piece of code, assuming it is not used externally. Of course, as you mention, there is no problem compiling something that breaks that. – kabanus Jun 04 '21 at 08:45
  • I assumed you used Eclipse, but I mean whichever IDE (including compiling with a linter). – kabanus Jun 04 '21 at 08:46
  • @kabanus it is a compiler warning, I get the same when I compile the code on command line. By the way, it is the javax.annotations.Nonnull - I wrote it in the question but maybe it was not visible. I use IntelliJ but I don't think it makes the difference since I get the warning while compiling on command line as well. – Matteo NNZ Jun 04 '21 at 08:47
  • 1
    Because you turned on the linter, as mentioned. That will guarantee the correctness, and make the `if` redundant. – kabanus Jun 04 '21 at 08:47

2 Answers2

2

As a lot of people mentioned in the comments already. It is behaving as it is intended. The explanation from https://checkerframework.org/manual/#nullness-checker clearly states what is going on here:

The checker issues a warning in these cases: When an expression of @NonNull type might become null, because it is a misuse of the type: the null value could flow to a dereference that the checker does not warn about.

The above warning only shows up when you pass -Alint=redundantNullComparison to the compiler and is by default turned off. As you are compiling with -Xlint:all even this warning is being enabled.

If you don't want to see this warning on itellij you can update your settings:

Settings (Ctrl+Alt+S / ) > Editor > Inspections > Java > Declaration redundancy > Null-check method is called with obviously non-null argument.

or

Settings (Ctrl+Alt+S / ) > Build, Execution, Deployment > Compiler > Add runtime assertions for not null annotated methods and parameters

or

Settings (Ctrl+Alt+S / ) > Editor > Inspections > Java > Probable bugs

Also if you are expecting a null value, it doesn't seem right to use that annotation in the first place.

AEM
  • 1,354
  • 8
  • 20
  • 30
deepakchethan
  • 5,240
  • 1
  • 23
  • 33
  • Thanks a lot for taking the time to write the answer. As I stated in the body of my question and in the comments, I'm using `javax.annotations.Nonnull`, and not `org.checkerframework.checker.nullness.qual.NonNull` which is the one you're mentioning in your answer. – Matteo NNZ Jun 04 '21 at 10:04
  • Also, I'm not expecting `null` values (and that's why I annotate with `@Nonnull` so that the client of my service will see a compiler warning if they pass a `null`), but I can't be sure that someone will be sending it anyway and if that's the case, I want to send a clear exception back stating why I didn't accept their request (that's why I check for `null` in my code). – Matteo NNZ Jun 04 '21 at 10:05
  • @MatteoNNZ this annotation is solely being used for linting and warning. If you want to throw exceptions you can just use lambok's notNull or Objects.requireNonNull(object); – deepakchethan Jun 04 '21 at 10:19
  • My input string needs validation and I need to call endsWith() on it. Hence if i am called with a null string, I risk a null pointer exception. I would like to catch null strings/invalid strings together and send a specific exception, not just a NPE, that's why I wrote the code with the if null condition first. Anyway, it seems nobody sees it the same way than me :) – Matteo NNZ Jun 04 '21 at 19:21
1

You have the annotation on the method parameter. @Nonnull String userEmail

So it expects that userEmail== null will always evaluate to false and hence no reason to be there in that if check.

Check the following answer regarding the @Nonnull which belongs to JSR 305

305 is about new annotations where you can already put them, which can help provide programmatic visibility into a design by contract system. So that if a certain method is supposed to not return null, or if a certain method is supposed to never receive a null parameter

So if you as a programmer have the confidence to annotate that parameter as nonNull, why shouldn't it report that the check userEmail == null has no meaning? With the annotation is like you inform the compiler that you have the confidence that it will never get called with null.

Annotation @Nonnull is like describing a contract. You don't make contracts which you know could be broken. If you are not sure then remove the annotation. If you are sure then remove the check userEmail == null

Panagiotis Bougioukos
  • 15,955
  • 2
  • 30
  • 47
  • It seems I am the only one seeing like that but I really don't get one point. Annotating a parameter as @Nonnull will make the compiler warn anyone calling my method with a null value that they are passing a null into a method which doesn't expect a null. That's great. But it does not actually _refrain_ someone to send a null value anyway. The call chain can be complex, a value may become null by hazard so yes, the method *can* receive a null value by mistake (proof is I can debug with null input) and if that was the case, I want to be able to test it and throw back if it happened to be null. – Matteo NNZ Jun 04 '21 at 10:12
  • 1
    That's the way JSR 305 was intended to be used. Is like API description where you set a contract. It is not making sure that this contract exists. Otherwise look for validation annotations that throw exception if the contract is broken. Lombok nonNull will do that . – Panagiotis Bougioukos Jun 04 '21 at 10:15
  • The one you use would normally be used in a local application where only you provide calls to that method, and you as a programmer make sure that you don't provide any null parameter. Then you set that contract, so that other programmers coming after you in that application know what you have coded, and be confident that no one will call the method with a null parameter. That is how it was intended to be used, so that other programmers after you receive some information without having to reverse engineer the whole system. – Panagiotis Bougioukos Jun 04 '21 at 10:18
  • I agree about the usage. I don't want to send a NPE (that's what the tools you mention would do), I want to test the string with endsWith and to do that I need to make sure it's not null first, that's why I wrote the code like that and also why I need to keep it that way. I disagree with the warning, it's not true that the input cannot be null, it can be and I have proven it by debugging with a null value. Anyway, it seems everyone agrees with your vision except me, I guess I need to accept that it's like that :) – Matteo NNZ Jun 04 '21 at 19:24
  • @MatteoNNZ Well, I think the message "is always 'false'" is indeed not correct, as you have shown the opposite. The message "Condition 'userEmail == null' is always 'false'" should instead be something like "Condition 'userEmail == null' is expected to be always 'false'" or something. – MC Emperor Jun 06 '21 at 11:50
  • @MCEmperor yes, that's what I think. In any case, expected or not, the warning is annoying because I am forced to test for null (hence I can crash right after when I call .endsWith()), but I will have to keep my code with a warning which will be visible on code analysis tools... – Matteo NNZ Jun 07 '21 at 07:49
  • I want to do a contract, and tell the users that it shouldn't be null. But the runtime cannot be trusted, so its not the user, maybe its the bug that would cause a NPE. So I don't agree with this. I would like to keep both @Nonnull and null check for careless or mallicious code. – yerlilbilgin Dec 29 '21 at 08:24
  • @yerlilbilgin you can always remove `@Nonnull` and add the null check in your function and then just add in the description of your method that no null should be passed in – Panagiotis Bougioukos Dec 29 '21 at 08:26