JDT is not capable to detect that Map.getOrDefault can’t return a null value and it gives a warning.
And if I remove the @Nullable from the map then the second method complains about the else if is dead code because can’t be true.
How do you proceed in these cases? Just put a @SuppressWarnings(“null”) is fine?
I suppose that I can introduce a getOrDefault method in my class to get ride of the warning, but it seems an antipattern for me to create more code to solve limitations of the static null analysis.
Looks fine. But better style would be to chose a different name (e.g. dbClient, so you are sure not to confuse them). The local variable can also be final.
It’s annoying but the warning about fields is a totally valid one if you consider concurrency.
Imagine two threads call disconnect() at the same time.
Thread 1 evaluates client != null is true but then suspends before evaluating client.close().
Thread 2 then evaluates all of disconnect() and sets client to null at the end.
Thread 1 resumes and tries to evaluate client.close() but client is now null so you get a NullPointerException.
If I remember correctly the null checker will respect assert x != null if you have some local variable that is definitely not null but you are hitting some limitation of the checker.
I understand the concurrent use case.
But a lot of classes are not intended to be used concurrently and prior code can be safe due to its invariants. And the ones that support concurrent usage and have state, normally will need some synchronization logic to don’t corrupt internal state or break invariants. And you can easily get to the same situation because in these cases, null safety is only a little part of the photo.
An annotation to declare that the state can’t be concurrently updated will possibly help in these situations, but looking at the doc doesn’t seem to be available.
I will go for the local variable trick then, I don’t like to modify code only to make tools happy, but I understand the value of standardization.
Wearing my pedant’s hat I could make the argument that any use of a class that is not explicitly disallowed is implicitly allowed. So not making a class thread safe and NPE proof is a bug because how is the next person (or you in the future) who uses your code meant to know about your unenforced invariants. Ah… for a world were all java is annotated with JML and the behaviour of everything is perfectly defined…
Taking the hat off again, yeah it is kind of annoying. Not everything needs to be “perfect” and static analysis tends to be hard so tools are often over cautious.
But… unintended NPEs are a really common bug so is it not worth occasionally doing a little dance for the null checker to prevent lots of bugs that are easy to catch?
I have to say since we last talked I have become less of a fan of null checking in this project because I have noticed the implementation used is not just over conservative but seems to actually be pretty broken in some cases.
I guess @SuppressWarnings is not working because it is marked as an error, not a warning.
It looks like this error is because we are not using an annotated JDK it does not know Optional#orElse(@NonNull T) will allways return non null.
Unfortunately I think this would make any use of Optional#orElse(...) very messy (you would have to do a null check to prove to it you know it is not null).
It’s probably simpler to just do something like this:
if (configuration.isAddCategoryTag()) {
@Nullable String tag = item.getCategory();
if (tag == null) {
tag = "N/A";
}
point.withTag(TAG_CATEGORY_NAME, tag);
}
I suppose you could also try putting @NonNullByDefault({}) on the method and see if that does anything for a hacky fix.
My opinion is that I like the null static analysis, but that from JDT is problematic in some cases.
Especially managing some new APIs like Optional.ofNullable .orElse. and field usage with classes that are not designed to be used concurrently.
Having to change clear code to a more noisy one is a smell for me. It’s possibly a fair price to pay for the strict null checkings, but not a nice thing.
I still like null static analysis in general, just am not too fond of the implementation used in this project.
In the past I’ve used it with an annotated JDK so you didn’t have any errors using methods like Optional<T>#orElse(T).
Also if you use something like Checker Framework you can use annotations like @MonotonicNonNull which solves your problem with fields.
I have noticed there is a project providing external annotations for use with the eclipse JDT:
If we could integrate that into the build it might make things nicer.
I didn’t like the annotations at first but nowadays I do like them.
Sometimes you have to help the compiler a bit by changing your code because the analysis has some limitations.
There’s also some helpful documentation about how to use them, their limitations and explanation of error messages at Eclipse, see Using null annotations.
Using the EEAs has been on our wishlist for some time now, see:
That’s interesting. To my understand the only problem to be able to use it is in IDE Eclipse support?
Because for CI and other IDEs that are more mvn based it works with @maggu2810 tests, isn’t it?
There has been a possible solution for the Eclipse IDE setup years ago as the code base had been part of Eclipse SmartHome:
IIRC after the migration from PDE to Pure Maven projects it should be enough to add the LastNPE plugin to the Eclipse IDE (as I written in the topic @wborn comment above).
I used the EEA in the Eclipse IDE for some other projects, so in general it should work.
There’s also an issue in som cases where the compiler and SAT doesn’t agree, e.g. when using nested classes. If the main class is annotated with @NonNullByDefault, this should apply to the nested classes as well, but the SAT still complains that “Classes/Interfaces should be annotated with @NonNullByDefault”, but if I do that, the compiler warns that it’s unnecessary. Which one should I listen to?
I’ve just experimented by myself based in your commit from the EEA issue and it works nicely:
With that simple commit I’ve been able to remove the @SupressWarnings(“null”) and be able to use Map.getOrDefault()
What I’ve also done is manually clone LastNPE repository and build it because dependencies were not correctly found in jfrog.
Also as the project seems very outdated it hasn’t declared null annotations for Map.getOrDefault() adding this to Map.eaa:
If I’m not missing something it will be easy to start using it if there is interest, as it seems from the issue.
I think that it will be nice to be able to use effectively new methods for null managing in new JDK versions like Optional.orElse, Map.getOrDefault,…
