CODEWITHSUNDEEP
javanullable
kennyg
kennyg

Reputation: 1059

Is it proper to mark a param as @Nullable and then throw an IllegalArgumentException when a null is passed in?

So I just drilled into my colleague that he should not be doing as the title states. But I fought so strongly for my stance that I want to make sure that I am actually right (the worst thing I want to do is enforce an incorrect opinion on someone else).

So basically I'm trying to verify that the following is indeed bad practice:

public void methodA(@Nullable String value) {
  if (value == null) throw new IllegalArgumentException();
  ...
}

His argument was that @Nullable simply states that you can pass a null in here, but it doesn't guarantee that the result of doing so will be "nice".

Whereas my argument is that doing so breaks the contract that @Nullable implies that a null value will be properly handled and not treated as exceptional.

The problem is I can't find any documentation actually verifying my claim.

Ultimately I was able to convince him of my stance, but now I'm concerned that perhaps I was wrong.

Ah, I am missing some context here. So we are using @NonNullApi from Spring, which by default puts a @Nonnull on all the params. So if you want to support a null param, you need to override this with a @Nullable annotation.

Upvotes: 2

Views: 1826

Answers (2)

mernst
mernst

Reputation: 8137

The Checker Framework manual, section "Annotations indicate non-exceptional behavior", supports your point of view. It states:

You should use annotations to specify normal behavior. The annotations indicate all the values that you want to flow to a reference — not every value that might possibly flow there if your program has a bug.

Under your colleague's reasoning:

His argument was that @Nullable simply states that you can pass a null in here, but it doesn't guarantee that the result of doing so will be "nice".

every reference type could be annotated as @Nullable, and thus @Nullable would have no meaning.

Upvotes: 3

Sweeper
Sweeper

Reputation: 272760

The problem is I can't find any documentation actually verifying my claim.

Here's some documentation from the JetBrains Annotations library:

Nullable:

An element annotated with Nullable claims null value is perfectly valid to return (for methods), pass to (parameters) or hold in (local variables and fields).

NotNull:

An element annotated with NotNull claims null value is forbidden to return (for methods), pass to (parameters) and hold (local variables and fields).

I would say a contract such as "I will definitely throw an exception if you pass a null to this parameter!" would make null a "forbidden" value to pass to that parameter. You should use @NotNull (or similar) to annotate it instead. It's certainly not a "perfectly valid" value to pass to that parameter.

Of course, your colleague could still argue that since passing null compiles, it's "perfectly valid" :) but that's just sophistry IMO.

I understand that you are using Spring's annotations, but the wordings in Spring's documentation isn't as strong as the JetBrains' ones, which is why I chose the latter to make a stronger argument. Hopefully you would agree that both pairs of annotations are designed to solve the same problems, and have the same semantics. It would be absurd to have a situation where you should mark something as org.springframework.lang.Nonnull, but not org.jetbrains.annotations.NotNull.

Upvotes: 4

Related Questions