Add/Enforce Null Annotations

In it’s current state the API does not easily state which API Objects can be nulled and which cannot. This applies to parameters and results.

It would be a lot of work, so if someone has a good idea for how we should tackle this (i.e. start with major classes, do it only whenever someone touches a file), bring it on.

I would recommend it for one specific reason:
It has no runtime cost as it’s purely annotations. Modern IDEs can provide help for dumb mistakes in that regard, but even for the text-editor and browser people, the annotations will still be in the javadoc.

I’d like to hear your opinions (please provide some reasoning).

Another subject for discussion would be what annotation lib to use. JavaX vs. JetBrains. I think I’ve heared that JetBrains has problems with guice, but JavaX cannot express the correct nullability for Array Objects (i.e. Vector3f[] foo(): Does foo return null or does the array contain null values?).

2 Likes

So we would have to suck in an additional dependency just to annotate these parameters?

1 Like

Follow up question… assuming the answer is yes, how big (compiled JAR size), and what license?

1 Like

Yes, but I guess we don’t need them to be transitive at least.

Then we also have “quals checkerframework” somewhere halfway implemented which would be one of the possible deps.

javax.annotation would be The Java Community Process(SM) Program - JSRs: Java Specification Requests - detail JSR# 305 (JSR 305), but it seems this has problems (violating the oracle license, it won’t become standard, details)

But in general I guess more dependencies wouldn’t hurt some times (guava and stuff), especially because the jar size is no concern for typical jme applications, I guess. AND I guess we have a lot of other options to slim jme down (more modularization, strange sail-navigation code in jme3-desktop, e.g.).

Since this is only annotations, it will only be ~ 30kIB under various licenses:
Maven Repository: com.google.code.findbugs » jsr305 » 3.0.2 (19kiB, Apache)
Maven Repository: org.jetbrains » annotations » 20.1.0 (25kiB, Apache)
Maven Repository: org.checkerframework » checker-qual » 3.6.1 (okay this one is 200kiB but MIT).

But: These annotations are not required in a jme application, it will probably only be unused annotations. You only need these if you want to continue the annotation.

Edit: And jme3-testdata already depends on the findbugs jsr305 implcitly, as nifty itself does depend on it

3 Likes

It’s just with all of the other random stuff Java has thrown in over the years, somehow this has not become a standard annotation… when plenty of other standard annotations have crept in.

That could mean a couple things.

  • it’s not that important. After all, null arguments are likely to fail immediately at runtime.
  • there isn’t a particularly best way to do it.
  • java will come along and standardize this one day and we have to fix it all
  • or even more likely, we’ll end up with three different kinds of nullable annotations by then.

Personally, for me it seems like a lot of nearly-unnecessary work for almost no gain. Users would do better to just assume that nothing is nullable unless they’ve checked the javadoc or source code (which is one click away in the SDK… and should be only a couple clicks away otherwise).

In the end, “nulls where they shouldn’t be” is the single easiest kind of error to find.

4 Likes

I have to agree. If something is not intended to be null, it will be caught at runtime with a exception that tells you almost exactly what the problem is, especially in java 14. User code is where nulls come from, and users should take care to read the documentation, or even the jme source code as I do in many cases. It is a general rule of thumb to never pass a null into a library, and most user code will likely be passing a null in unintentionally, in which case a exception will get thrown either way that will tell the developer where the issue is.

3 Likes

based on your comments looks like the advantage is small when comparing to disadvantages.

Nifty depends on it, but well, no need use it.

so im also against using it.

I also don’t see a large value in using these in jME - I tend to assume that any reference typed parameters can’t be null, and I see the value of using these sorts of annotations more in enterprise software (signalling model types that can be null as part of the domain model) than in something like a game engine where most of the time nulls aren’t a valid parameter value.

I do see the value in clearly indicating cases where null parameters are valid, but I’d suggest doing that with documentation rather than annotations.

2 Likes

With kotlin, you get nullable types built into the type system :wink:

one of the benefits of Kotlin I love :slight_smile:

1 Like

For once, i agree, this would be kinda nice.
But if this has to be done with annotations, it should be something that doesn’t break the code when the build environment doesn’t support it, unlike lombok for example, if the build environment doesn’t support these nonstandard annotations it should just compile without nullchecks. That’s my only request.
Alternatively we can just put assertions everywhere.

3 Likes

@RiccardoBlb iirc aren’t assertions disabled by default? Or has that changed?
If assertions are disabled by default, the issue with using them is no one will pass the flags to enable them to the java command.

I do like the idea of using a built in mechanism, but I guess I do not see the advantage of throwing the error due to a null check verses throwing a npe. I am still on the side of the fence that it would be best not to add anything at this point.

Yes, they usually are. But that’s not an issue… if you want the feature you enable it, if you don’t then you don’t. Besides assertions are very useful and you should always use and enable them when you develop code.

1 Like

But if you aren’t going to see the assertion until runtime… null pointers are already a case where you are pretty much guaranteed to see an exception at runtime already.

JME could use more assertions for sure… but that’s not really related to this conversation in my opinion.

2 Likes

Yes, but sometimes you get the npe downstream and it is not immediately clear what is causing it. Having a
assert mySpatial!=null : "mySpatial cannot be null"
at the beginning of the method would make things a little bit clearer.
It also helps someone looking at the code by taking some conditions out of the way immediately.

Guys this is NOT about Assertions but Annotations.
Let’s take Vector3f#toArray:

Here, floats can be null, actually in most cases it might be null.
All I ask for is to change the method header like this:

public @NonNull float[] toArray(@Nullable float[] floats) {

That way, the Users IDE knows that the result from toArray is guaranteed to be NonNull and the input is allowed to be null. Yes it already is in the javadoc, but javadoc is not machine parseable and I wouldn’t read the javadoc for something as intuitive as toArray.

Regards the complaint that NPEs throw up at runtime: That’s like saying we could change all our Types to Object and cast them at runtime. It’s about compile time safety and general guidance. Sometimes you didn’t even think about what you passed was nullable and so it will tell you, so you can add a null check. This especially comes into play when also receiving values from the API, which could be nullable.

Yes, it doesn’t make much sense because most everything already is not null.
Btw according to Riccardo, Checkers provide “derive” annotations, so that we could only mark Nullable types and everything else is inferred as NonNull.

Speaking of Oracle: I guess they declined that, because they introduced the Optional<T> type for “nullable types”, which is however much more opinionated and can’t be slammed on an existing codebase easily.

Talking about concerns regarding annotations: Essentially that’s just a few classes provided via Maven, like a regular dependency. In itself the compiler will ignore them. The IDE for projects which depend on JME however or the compiler may support/understand this annotations. If not, they are just dead annotations (i.e. I also use annotations in my game and they are just evaluated at runtime).

I guess it depends, JSR 305 might be the only annotation that javac understands, but it might indeed throw a warning or an error when there are nullability errors.

And whoever says every “null-issue” is easily found and will never happen because we’re professionals: JME has around ~50 of these issues, some more obvious, others less obvious. We will get to them next week.

There’s something from Google as well, Preconditions.validate() or something.

But I understand if you think the gain is not worth the effort to add such annotations. Might also heavily depend on the project, some are more null than others :slight_smile:

2 Likes

I wouldn’t go through all the code to add those annotations, but i would agree to adding them in a progressive way.

1 Like

I think most IDEs donot accept non standard notations , and it wont add something new anyway , so working on the SDK core is better , so

assert object !=null;
 

OR

Objects.requireNonNull(obj);

Would be the alternative
But if you need to add @NonNull I think you need to look for an example like how Android studio auto add these annotations in Android …it Adds these annotations in special cases like a Fragment for example that the user can ignore basically so in jme it should exist in some specific context if it will , like loading models must not be null