-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
How to handle weird methods like requireNonNull(Object)
#29
Comments
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.] |
There's some discussion of methods like |
Note that test frameworks often provide methods like In IntelliJ IDEA we have a contract annotation which allows us to express contract in simple cases like That said, I don't agree with CF manual. Here's the example of IntelliJ IDEA API where I use 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(...); |
Yeah, the CF doc does not even recognize that methods like Thanks for reminding me that test assertions are another valid example. Whether an |
So at this point, I still believe:
|
[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:
Supporting this case via a cast would seem to make sense but is actually a big step backward in usability:
We could add a new method like
... which does nothing. The analyzer would want to make sure that a runtime check is happening somewhere else -- i.e. it should not allow
... but should continue to demand use of 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 We could potentially even consider creating a synonym for |
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.) |
I've broached the idea of a
Look how much nicer suppression is:
I think this is grounds to dismiss the idea. |
(@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 I declare that parameter without 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 (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 @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 ...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 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. |
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 Of course I am possibly advocating for 2 methods, in which case Hmm, I'm thinking maybe everyone who writes such a method should just suppress? |
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? |
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. |
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
I withdraw that advocacy. I think it's probably inferior to suppression.
Then this calls for the signature shown in the very first comment, right?
And this signature functions well enough for case (1) as well, right?
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. |
Yes, sorry, I lost sight of the original point of this thread after seeing the discussion about |
Agreed, the method in the first comment... public static <T> T whatever(@Nullable T obj) { ... } (I still dislike including a ...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 (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:
(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 |
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 |
We are comfortable letting the recognition of these special methods be checker-specific for now. |
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 Or is it the lack of support for method chaining, that makes this an inferior solution? |
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). |
Random new thought: so we're talking about this very rare category of methods like It occurred to me that the However, *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. |
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 But as you said (earlier and again now): Users might not want to get into all this, so we're likely to just use (It's possible that, for widely used libraries like Guava, we'll respond by creating package-private helper methods that merely delegate to |
This bug is theoretically mostly about what we know about However, it has also included some discussion of whether It just occurred to me that we could hedge our bets by making its parameter type That said, it remains controversial whether APIs should use |
I'm making this one the dup just because it's more outdated. |
Duplicate of #176 |
This method currently looks like so:
A method like this is supposed to be used when you have a
@UnknownNullness Foo
or a@Nullable Foo
and you want to get a@NotNull Foo
instead. (Users need this; we can't allow them to just cast because this would require bytecode munging.)A user with an expression that is either nullable or of-unknown-nullness needs to be able to call this. If the expression is already non-nullable, it should probably work as well, and tools can offer a separately configurable "you probably don't need to do this" warning.
And of course, what they get back should be non-nullable.
So would the signature look like this?
Assuming that (or something like it) fulfills the requirements, then there's still another question.
What's very unusual about this method is that if it succeeds then a variable passed in for
obj
can automatically be presumed by the inferencer to be non-nullable itself, whether the return value is used or not. A tool should view it the same as it would an explicit if-throw pattern.We wouldn't want every tool to have to keep its own hardcoded list of methods that have this property, so do we need to provide a special annotation to cover this case?
The text was updated successfully, but these errors were encountered: