How to handle weird methods like `requireNonNull(Object)` about jspecify HOT 24 CLOSED

I feel like there shouldn't be many methods like this, just the ones in the JDK and Guava [edit: oh, and test assertion methods], but wishing doesn't make it so. Generally it's super weird to know you will never return successfully if null is passed, yet want to accept null anyway. Right? [Edit: however, to be clear, it IS exactly what this very special category of methods want to do.]

from jspecify.

There's some discussion of methods like requireNonNull in the CF manual, which advocates for annotating this parameter as @NotNull: https://checkerframework.org/manual/#annotate-normal-behavior

from jspecify.

Note that test frameworks often provide methods like assertNotNull with various signatures like assertNotNull(String message, Object obj) or assertThat(obj, not(isNull())) or assertThat(obj).isNotNull(), etc. After that people expect to be able to use the passed object without additional nullability warnings.

In IntelliJ IDEA we have a contract annotation which allows us to express contract in simple cases like requireNonNull or assertNotNull using something like @Contract("null -> fail") annotation. This solves the problem for us. We also hardcoded methods like assertThat which are hardly expressible via any kind of contract language. These methods could be considered as !! operator in Kotlin which means "I know what I'm doing". Somehow it's like an explicit type downcast (from nullable to not-null).

That said, I don't agree with CF manual. requireNonNull call is an explicit way to say that complexity of called method contract is beyond the practical capabilities of static analyzer. Producing a warning on requireNonNull call makes this method useless if you are going to dereference the value anyway.

Here's the example of IntelliJ IDEA API where I use requireNonNull often.

interface PsiReferenceExpression {
  // yes, a reference could be unqualified and in general this should be checked
  @Nullable PsiExpression getQualifierExpression(); 
}

...
PsiExpression expr = factory.createExpressionFromText("foo.bar", context);
// I've just created an expression from the text constant, so I'm pretty sure it's qualified here
// And I bet no existing static analyzer is sophisticated enough to understand this
PsiExpression qualifier = requireNonNull(((PsiReferenceExpression)expr).getQualifierExpression());
qualifier.something(...);

from jspecify.

Yeah, the CF doc does not even recognize that methods like requireNonNull are in a special category of their own which is very different from cases like Double.valueOf. I, like amaembo, do not consider their reasoning to apply to this case.

Thanks for reminding me that test assertions are another valid example. Whether an assert statement counts, I can definitely see both sides on.

from jspecify.

So at this point, I still believe:

1. the signature should probably look like static <T> @NotNull T requireNonNull(@Nullable T obj) (one of the annotations could be done as a type parameter bound instead, but I don't see the point).

2. we need a way to recognize this very special case, I think via a very-rarely-used annotation (which will sadly get overapplied, but what can we do). I can't see much alternative to this; having tools share hardcoded lists would be strange.

from jspecify.

[EDIT: I recanted the idea in this post.]

This special category of methods seem like the appropriate choices to use for "casting" (so to speak) from nullable to not-null. This is perfect for a case such as if you assign to a @NotNull field then return without actually using that field.

On the other hand, it doesn't feel like a good fit in this case: checkNotNull(obj).doSomething(). The intention is to capture the developer's assertion that obj cannot be null in the circumstances, but the result is redundant checking at runtime. Not the worst thing in the world, but feels wrong. In fact, it feels VERY wrong when you get a lot of it (paraphrased from real code we found):

checkNotNull(checkNotNull(checkNotNull(checkNotNull(pager).getAdapter())).getTitle(index)).toString()

Supporting this case via a cast would seem to make sense but is actually a big step backward in usability:

((@NotNull RedundantlyWriteTheTypeHere) obj).doSomething()

We could add a new method like

<T> @NotNull T trustNotNull(@Nullable T obj) {}

... which does nothing. The analyzer would want to make sure that a runtime check is happening somewhere else -- i.e. it should not allow

  private @NotNull Foo foo;
  void setFoo(@Nullable Foo foo) {
    this.foo = trustNotNull(foo);
  }

... but should continue to demand use of checkNotNull or equivalent in this case.

It may seem odd to bother offering two different methods for this just so that one can be a real no-op. But another point I'd make is that with only one method, you can't actually distinguish between the "I'm worried this might be null so please fail now if so" case and the "I know this can't be null so please let me dereference it" case. They both get represented the same way.

Is it worth it? The alternative, perhaps, is just to say

'A few redundant runtime checks are not really so bad, but if you're doing a lot of them, see whether the libraries you're interacting with can be improved to produce @NotNull types more often, or use @SuppressWarnings("nullness").'

We could potentially even consider creating a synonym for @SuppressWarnings("nullness"), like @TrustNullness or something.

from jspecify.

Btw, when I talk about what runtime methods are "offered" I do not mean that they would be released as part of this project. Presumably they will exist in various places and nullness analyzers just need some way to recognize them for what they are.

(One could propose the creation of a small runtime library as part of this effort, we have just been planning not to.)

from jspecify.

I've broached the idea of a trustNotNull method that doesn't actually do anything at runtime, existing only for the signal that it gives to static analyzers. But that still doesn't make this code lovable:

@NotNull String s = trustNotNull(trustNotNull(trustNotNull(trustNotNull(pager).getAdapter())).getTitle(index)).toString()

Look how much nicer suppression is:

@SuppressWarnings("nullness") // I'm convinced this can't NPE in practice
@NotNull String s = pager.getAdapter().getTitle(index).toString();

I think this is grounds to dismiss the idea.

from jspecify.

(@kevinb9n , you might want to peek at internal bugs 123925743 and 129155385, which are related to this.)

(Sorry if I'm missing context, using out-of-date syntax, etc.)

I think it would be helpful to lay out the various use cases we have. I can see a world in which there are 2 different methods for different use cases.

(1) I'm writing a constructor that needs a non-null value for a parameter (but wouldn't naturally fail immediately if given null, since it's just assigning it to a field).

I declare that parameter without @Nullable, so it's enforced (soundiness-ly). But I know that I have callers that don't use nullness checkers, so I still want to perform a check. The most sensible method for me to call is probably:

public static <T> T requireNonNull(T obj) {
  // throw NPE if obj is null, otherwise return obj
}

Nullness checkers will ensure that my call to this method can never throw NullPointerException when called by null-checked code.

(2) I have a value that I know, for reasons not provable by the nullness checker, cannot be null, and the nullness checker is telling me about the potential null dereference.

(Possibly the method that I got the value from should have been @LessEnforced, but maybe it's a case in which enforcement makes sense 99% of the time. (Or maybe @LessEnforced "gets lost" in a complex type or something, like when I unpack the elements of a List<Future<@Nullable String>>? Whether that's even possible would depend on the details of @LessEnforced.)

@kevinb9n was advocating (I believe) for a no-op method like this:

public static <T> T trustNotNull(@Nullable T obj) {}

I would probably advocate for having the method perform a null check. It just feel safer. (And that behavior also works for (3) below.)

But I think it's also fair to say that @SuppressWarning("nullness") may be the better tool here.

...at least if I really do trust myself that the value can never be null. Since I might be wrong, I would probably rather have the checking in place.

That said, in extreme cases, I would prefer the suppression, maybe with one final null check to ensure that any NPE happens locally and not later on.

(3) I'm in a situation like (2), but the nullness checker isn't requiring a null check.

(because I got the value from an unannotated or @LessEnforced API)

In this case, the only reason I'd want a check is to confirm that I'm actually right to "know" that the value is never null (as discussed under (2)). Typically I'd get such a check "for free" when dereferencing the value, but maybe I want to eagerly check the value before saving it for later, similar to the constructor example above.

In this case, it doesn't matter what the method signature requires of the parameter (since the nullness checker won't be checking it, anyway). Still, I would lean toward a method like the one in (2) to emphasize that NPE could happen here if there's a bug in this code, rather than only if there's a bug in a caller. But maybe I'm overemphasizing the importance of that distinction.

(4) (Hmm, I should think more about the test-assertion case. Maybe I can consider that to be part of (2) and (3)?)

But is having 2 methods going to be more confusing than it's worth? Certainly naming would be tricky.

from jspecify.

2. we need a way to recognize this very special case, I think via a very-rarely-used annotation (which will sadly get overapplied, but what can we do). I can't see much alternative to this; having tools share hardcoded lists would be strange.

Is releasing the One True Method in a tiny runtime library out of the question? (Maybe users need multiple such methods for some reason, and we can't provide them all?)

Is requireNonNull in particular not good enough?

Of course I am possibly advocating for 2 methods, in which case requireNonNull won't cut it, but a library with the Two True Methods could work. But maybe people really don't like depending on anything?

Hmm, I'm thinking maybe everyone who writes such a method should just suppress?

from jspecify.

Oh, I see: some of the "weird methods" would be in various test assertion libraries, for example :) Still, probably enough of an edge case that they can suppress?

from jspecify.

It's all the users of these libraries who end up getting suboptimal behavior because the analyzer doesn't realize things that it should about their code.

Releasing a small runtime library of our own is a possibility, but I think it wouldn't change the fact that people would want other libraries they had already chosen to be recognized in the same way so we're left with the same problem anyway.

from jspecify.

(1) I'm writing a constructor that needs a non-null value for a parameter (but wouldn't naturally fail immediately if given null, since it's just assigning it to a field).

I declare that parameter without @Nullable, so it's enforced (soundiness-ly). But I know that I have callers that don't use nullness checkers, so I still want to perform a check. The most sensible method for me to call is probably:

public static <T> T requireNonNull(T obj) {
  // throw NPE if obj is null, otherwise return obj
}

I believe our assumption in this scenario is that we're in a @DefaultNotNull context (not legacy), and that this default does apply to type parameters, such that <T> means by default <T extends @NotNull Object>. If so then I agree with this statement (but see a question about case (1) a little further down).

(2) I have a value that I know, for reasons not provable by the nullness checker, cannot be null, and the nullness checker is telling me about the potential null dereference.

@kevinb9n was advocating (I believe) for a no-op method like this:

public static <T> T trustNotNull(@Nullable T obj) {}

I withdraw that advocacy. I think it's probably inferior to suppression.

...at least if I really do trust myself that the value can never be null. Since I might be wrong, I would probably rather have the checking in place.

Then this calls for the signature shown in the very first comment, right?

public static <T> @NotNull T whatever(@Nullable T obj) { ... }

And this signature functions well enough for case (1) as well, right?

(3) I'm in a situation like (2), but the nullness checker isn't requiring a null check.

(because I got the value from an unannotated or @LessEnforced API)

In this case, the only reason I'd want a check is to confirm that I'm actually right to "know" that the value is never null (as discussed under (2)). Typically I'd get such a check "for free" when dereferencing the value, but maybe I want to eagerly check the value before saving it for later, similar to the constructor example above.

In this case, it doesn't matter what the method signature requires of the parameter (since the nullness checker won't be checking it, anyway). Still, I would lean toward a method like the one in (2) to emphasize that NPE could happen here if there's a bug in this code, rather than only if there's a bug in a caller. But maybe I'm overemphasizing the importance of that distinction.

I'm getting a little lost, but I do suspect users are not going to care enough about all these distinctions, so if a method with a signature like we've been discussing exists they will just use it - and it will do the right thing, right?

The hope is that everything is suitably addressed by a choice between suppression or a single checkNotNull-type method. Hopefully that is so.

If so, well, the original issue with the original description at top still continues to need a solution.

from jspecify.

It's all the users of these libraries who end up getting suboptimal behavior because the analyzer doesn't realize things that it should about their code.

Releasing a small runtime library of our own is a possibility, but I think it wouldn't change the fact that people would want other libraries they had already chosen to be recognized in the same way so we're left with the same problem anyway.

Yes, sorry, I lost sight of the original point of this thread after seeing the discussion about @Nullable/@NotNull on the checkNotNull/verifyNonNull/trustNotNull methods. I've been focused on that instead.

from jspecify.

Agreed, the method in the first comment...

public static <T> T whatever(@Nullable T obj) { ... }

(I still dislike including a @NotNull annotation in our offerings at all, but that ship has probably sailed?)

...could be used for all these cases. I'd just anticipate a lot of calls to the method when the nullness checker "knows" the input can't be null (again, the case of null-checking a @NotNull parameter from a caller who might not be using the nullness checker). That doesn't have to be a problem, but:

(1) It's possible that some users would like to know where the "real" null checks in their code are, the ones that could actually throw NPE when called from null-checked code. Maybe they'd like to be able to be able to identify those checks by looking for calls to a separate method. However:

• They could always write their own method.
• Code is likely to be full of other places where NPE is technically still possible, thanks to @LessEnforced cases.
• So identifying the "real" null checks might be best done by static analysis.

(2) Error Prone already has static analysis for various "redundant" null checks, and I assume that other tools do, too. There has already been talk of extending that to more cases, and theoretically we could flag all cases identifiable by the nullness analysis. That would mean a bunch of warnings/robocomments on the kind of null check described above. Or it would mean giving up on the analysis, or it would mean tracking 4-state(?) nullness: nullable, not null, uncertain, not null but permissible to null check again.

(Given that requireNonNull exists and is surely used for all the cases above, it does seem likely that we'd use the signature above for it, so this discussion is more about the legitimacy of differently annotated methods.)

from jspecify.

For reference, here's how Kotlin handles this and other similar cases through contracts: https://github.com/Kotlin/KEEP/blob/master/proposals/kotlin-contracts.md

from jspecify.

We are comfortable letting the recognition of these special methods be checker-specific for now.

from jspecify.

For those who are torn between wanting and not wanting runtime checks: doesn't assert provide exactly that? Defer the decision to deploy time (or even launch time). Run tests with -ea and run without it in production. Checkers should already be able to understand assert v != null;

Or is it the lack of support for method chaining, that makes this an inferior solution?

from jspecify.

Method chaining is indeed a problem. In this case (where the result of a nullable method call is dereferenced) we actually suggest wrapping the qualifier with requireNonNull (and, of course, we understand the semantics of requireNonNull due to our Contract annotation).

from jspecify.

Random new thought: so we're talking about this very rare category of methods like Objects.requireNonNull and Guava's Preconditions.checkNotNull, and Verify.verifyNotNull and their fairly-direct analogues in other libraries. (But not much else.)

It occurred to me that the Preconditions one is slightly different from the rest. Like them it CAN accept a nullable input*, however, probably 99% of its callers are passing some parameter value directly to it, and we would really want them to mark that parameter @NotNull. For these 99% they're kind of better served by an API that does require @NotNull because it forces them to pass that requirement on to their own callers.

However, @Nullable as planned above would be friendlier to unannotated callers. We can stick with that, and simply recommend that checkers should probably suggest users mark such parameters @Nullable even though the nullness analyzer isn't otherwise forcing them to based just on the annotations.

*It seems very strange to say that a method can accept a null input when it will always throw in that case. However, what makes this method so unusual is that it is not throwing to indicate failure; it is throwing on behalf of the caller user as a service to that caller. That's always going to be an odd duck.

from jspecify.

Coincidentally, I was just thinking about this again today, too.

This has some overlap with the case I was trying to describe above (case "(1)" in this comment). The idea, as you say, is: If checkNotNull is defined to require a @NotNull input, then libraries that use it (and run the nullness checker) can guarantee that they won't throw NullPointerException when called by code that also runs the nullness checker.

But as you said (earlier and again now): Users might not want to get into all this, so we're likely to just use @Nullable.

(It's possible that, for widely used libraries like Guava, we'll respond by creating package-private helper methods that merely delegate to checkNotNull but that tighten their signatures to require @NotNull. [edit: But it's unfortunate that we'd lose out on all the performance-optimized overloads of checkNotNull unless we clone the whole set!] And then we'll generalize the static analysis we have, which makes sure we don't call checkNotNull("foo", foo) instead of checkNotNull(foo, "foo"), to recognize the new methods :) [edit: The easier solution might be for us to provide stricter stubs for checkNotNull that we apply only when compiling our own code.])

from jspecify.

This bug is theoretically mostly about what we know about foo after a call to requireNonNull(foo).

However, it has also included some discussion of whether requireNonNull(foo) is even allowed if foo might be null.

It just occurred to me that we could hedge our bets by making its parameter type @NullnessUnspecified. I've noted this in #71.

That said, it remains controversial whether APIs should use @NullnessUnspecified in this way at all :)

from jspecify.

I'm making this one the dup just because it's more outdated.

from jspecify.

Duplicate of #176

from jspecify.