Add RequireNonNullMessageChecker

[ksobolew]

This is a check to find some common cases of malformed requireNonNull calls. See the documentation for more details.

Based on trinodb/trino#10473 but is slightly more restrictive than that one (it always disallows the case when the part before the " is null" part is not the required identifier, while the other check only disallows it in constructors).

[ksobolew]

Why am I allowed to merge it without any approvals? 🤔

[kokosing]

It should not be the case:

Can you please check it again?

[ksobolew]

Ok, now it's greyed out :)

[nevillelyh]

Allowing multiple patterns with regex makes it hard to reason about what's allowed and how it matches. What about being more opinionated and matching only 1 pattern e.g. "{PARAMETER} is null", and making the pattern a config string?

I can't really think of a case where X is null must be expressed in some other form. Like empty/none/missing/required should mean something different from null, and the subject is always singular (1 parameter, even if the name is plural) and present, i.e. is.

Then we can possibly (but probably overkill) generalize it to other checks like Preconditions.checkArgument with a mapping:

• requireNonNull(param) -> {PARAM} is null
• checkArgument(!collection.isEmpty()) -> {PARAM} is empty
• ...

Although requireNonNull seems to be mostly developer facing with X is null like messages while checkArgument et al are more user facing with far more descriptive messages.

[ksobolew]

All of these different variants are a result of trying this checker on the Trino codebase. So they do appear in the wild, although I assume that this is mostly in older code, before the "X is null" became the prevailing idiom. I added them as "alternative" pattern just to avoid generating even more churn :)

[nevillelyh]

Grepping through Trino code base, there're ~11.8K requireNonNull(s, ~11.5K with is null"), the ones that are becoming less relevant.

Among the remaining 3K, only ~140 matches requireNonNull(IDENTIFIER, STRING_LITERAL). Most can be changed to X is null but could trigger the same debate in trinodb/trino#10473.

[ksobolew]

is slightly more restrictive than that one

I actually have qualms about this part. It makes sense on first sight, but I'm afraid that it will generate a torrent of irrelevant reports.

[nevillelyh]

I'm a little on the fence after reading through trinodb/trino#10473

Seems requireNonNull is used for 2 things

1. User facing e.g. "Required property X not set"
2. Internal e.g. "X {is/must no be/cannot be} null"

IMO for 2. everything should be "X is null" or no message. The problem is we can't easily distinguish user vs internal facing checks. Many requireNonNull(config.getFlag(), ...) or requireNonNull(System.getProperty(...), ...) are allowed by chance.
And even for 2. there are a few debatable cases like
list.forEach(x -> requireNonNull(x, "one of list is null"))
vs
list.forEach(x -> requireNonNull(x, "x is null"))
So we'll end up with complex regex or lots of SuppressWarnings either way.

[Laonel]

I have similar reservations about this. It looks like there is really no limit to how complicated regexes can become once we decide to support different forms of error messages, and if we decide to require x is null there aren't a lot of places that we could validate, like @nevillelyh pointed out.

But I do see a value in this check, as we are getting a lot of PR comments about requireNonNull.

Maybe we could start by limiting the scope of check only to usages in constructor with a constructor arg as the first param? In that scope I think we can be confident enough to require is null as the suffix and nothing else.