-
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
Address the "findViewById
problem" (generally nullable, but in *common* specific cases not)
#71
Comments
I just want to reassert that I think condoning the use of I think all these type usages are nullable. Then we've done everything we can do via the avenue of a type system to prevent errors. Analyzers can always do more. |
Even |
I would not describe myself as ever having been "in favor" of it, so much as against defining a fifth kind of nullness. And musing aloud on the question of how much we should try to account for the fact that even if we declare |
Oops, sorry, thanks. I think my belief was based on remembering that you'd closed the But it sounds like your position is that |
When you talk about |
One method that I think should be added to the list for this issue is I guess that |
Since |
I think there's been some miscommunication. Android, AFAIK, does indeed declare There are some interesting Android-specific examples, such as the return type of |
Back to the |
Naturally, as he also points out:
But I think this is overall yet more encouragement to make I don't think we're in danger of making any kind of binding decision here, but I would say that, if people feel strongly the other way, it would be good to hear about it at their convenience. If we end up using |
We have a small internal discussion going about |
Yes, we don't annotate |
I was talking to @kevin1e100 yesterday, and he mentioned a trick that might apply here: using aliases. Instead of making I have a couple follow-up thoughts, but I'll put them in separate posts. [edit: Update: In my second follow-up thought, I end up not liking this idea much anymore.] [*]: The use case for aliases that kevin1e100 and I were discussing was migration (#26, Kotlin support): When library owners adopt jspecify annotations, they might choose to annotate their code with a custom annotation like |
Follow-up thought 1: As I hinted in my previous message, the aliasing approach lets us define different "kinds" of unspecified nullness, each of which users may handle as strictly as they like. So we could have:
Then each team could decide which should be errors for their codebase, given the kinds of operations they tend to perform and their risk tolerance. However: Would we really want to provide n annotations in our artifact (or even in a separate artifact)?
So maybe users who want this level of control should just write stub files. (We could even host stub files that are commonly requested, similar to what the Checker Framework does. However, this would require that we settle on a stub format that all tools are required to recognize.) |
(This next post is long and definitely skippable -- not that you need my permission to skip anything I post :)) Follow-up thought 2:
My thinking so far:
The easiest way to see the problem is by comparing Return types: Consider Parameter types: Consider But wait: These two are backward from one another! A user who wants "strict" checking wants to treat To get universally strict (or universally lenient) handling:
So: Making I do think that, in practice, (Users might still be able to take this aliasing approach themselves if they want to, but we would probably recommend against it.) [edit: Forgot to add: We could consider having 2 separate |
Here is an issue related to having different types of Consider a method like A sound verification tool needs this method to be annotated as
The tool issues no false negative warnings (as a sound tool must) because it issues a warning whenever the argument might be null. However, it does issue false positive warnings. A bug-finding tool that aims to avoid false positive warnings might prefer the annotation
Put another way, a bug-finding tool might want to treat these methods differently:
The bug-finding tool might want to issue a warning whenever |
Yep, that is one of the cases we have in mind. (I don't think I'd see it defined that simply before, so thanks.) Your case is about parameter types. The other main case is with return types, and I don't think it's even possible to define precisely. It's more of a case-by-case question: If we force callers of this method to null-check the result before dereferencing it, what percentage of the time will that catch a bug? Our main example here is So we consider annotating the return type as
I have one other thing I've been meaning to say about |
Some thoughts about One way that we could make things easier on users is to have at least one method like Aside: So they have to suppress the nullness warning. That's the best I can imagine doing in that case. It's just also a small argument that Fortunately, most "normal code" won't encounter this issue often. We did see it in Guava occasionally, but Guava is weird. (We would have seen it more in Guava, but the Checker Framework's |
The name Can we think of a better name? Here are some suggestions. |
I dug up a past thread on the topic of naming, and I've quoted you and replied there. |
@kevin1e100 pointed out today that, for Kotlin users, parameters really need to err on the side of being Suppose that This heightens the conflict between Checker Framework users' desire for sound annotations and Kotlin users' desire for flexible ones. And as everyone reading this thread is probably already aware, we have conversations on this topic underway. |
Also on the subject of Kotlin: I was starting to worry about how hard it was to convince Kotlin that a It turns out to be easy: @Suppress("UNCHECKED_CAST")
class Foo<T> {
fun go(t : T?) : T {
return t as T
}
} |
I want to sort of "insist" :-) that we please retitle and reframe this issue. I think we need to remember very solidly that:
I want to push back on the idea I've been hearing that there are "kinds of nullness that are too complex for our annotations to be able to express". I think that's not the right mindset. A (non-type-variable) type usage is either nullable or not (well, or legitimately unspecified), just like an integral type is either int or long. The type must include the union of all values that ever might come through. Now there can be any manner of additional knowledge someone could code up about which values of that type it can actually take on in real life and in which circumstances. Maybe someone will even bring "refinement types" to Java (shudder). But this really shouldn't be our concern in this project. Post-1.0, however far off that is, sure, we could always talk about this again. Can everyone live with this? If there's an objection that's sticking in your mind that won't go away easily, then sure, let's have that out. |
I had 2 small realizations around this. They may be old news to people:
|
findViewById
problem" (generally nullable; in *common* specific cases not)
findViewById
problem" (generally nullable; in *common* specific cases not)findViewById
problem" (generally nullable; in *common* specific cases not)
The feeling has continued to gnaw at me that we will have to have some reasonable approach to this case by 1.0 (but not by 0.3). It could be just advice for users that is almost satisfying while being better than "leave it unspec". If such a stance exists. My concern is that without this, users will have irritating experiences that will get disproportionate attention. |
findViewById
problem" (generally nullable; in *common* specific cases not)findViewById
problem" (generally nullable, but in *common* specific cases not) [working decision: no]
Hello from 18 months later. I think it's now fair to assume we won't have anything special for this case in 1.0. I'm moving to post-1.0, and filing a separate doc bug for what to do in the meantime. |
findViewById
problem" (generally nullable, but in *common* specific cases not) [working decision: no]findViewById
problem" (generally nullable, but in *common* specific cases not)
Current decision: JSpecify 1.0 will not explicitly address this problem. Proposal for 1.0: Finalize the current decision. If you agree, please add a thumbs-up emoji (👍) to this comment. If you disagree, please add a thumbs-down emoji (👎) to this comment and briefly explain your disagreement. Please only add a thumbs-down if you feel you can make a strong case why this decision will be materially worse for users or tool providers than an alternative. |
We've discussed using
@NullUnspecified
as a way of saying something like "a smarter tool would be able to tell you when this is safe" -- or just "this method is used commonly enough that some users/owners are unwilling to tolerate null checks on every call size." See this earlier discussion from when we discussed using a@LessEnforced
annotation for this and this even earlier discussion.There are a number of places that this could be useful. Rather than dumping these into #32 (as I did recently), I'll start a new bug with a list, carrying over items from past discussions:
Possible usages for
@NullnessUnspecified
on return types:Map.get
Activity.findViewById
(rough stats on how often callers perform a null check on the result)Context.getSystemService
Fragment.getContext
Throwable.getCause
ResourceLoader.getClassLoader
Thread.getUncaughtExceptionHandler
HttpMethod.resolve
DateTimeFormatterFactoryBean.getObject
Map.Entry.getValue
: technically "undefined" if the entry has been removed, but some implementations returnnull
in that case. Checker Framework leaves it returningV
, but we could switch to@NullUnspecified V
if we wanted to be paranoid.Matcher.group(int)
Injector.getInstance()
Possible usages for
@NullnessUnspecified
on parameter types:Joiner.join
MoreObjects.firstNonNull
? (Or maybe we'd consider this likecheckNotNull
, for which we might allownull
inputs that might result in aNullPointerException
?)Collection.contains
?requireNonNull
/checkNotNull
?Another category of use cases is
@PolyNull
, whose status is up in the air. I've compiled a list of@PolyNull
methods.(I got to wondering if an evil way to support
@PolyNull
without@PolyNull
would be to allow stubs to contain 2 versions of a method -- so one could include@Nullable
and the other not. This might work, but it would probably require special rules, and anyway, it would require stub files for a library, while we want code to be annotated inline -- and we especially wouldn't want it to be mostly annotated inline but with magic elsewhere.)The text was updated successfully, but these errors were encountered: