Random @Nullable annotations: Thread and AbstractPreferences.

[cpovirk]

I notice that we're inconsistent about whether to use @Nullable on the
Runnable parameter of Thread. I can see an argument that an
inconsistency is pragmatic from the perspective of a subclass that
overrides run():

• There's no need for such a subclass to call call super((Runnable) null) when it could just call super().
• There is a need for it to pass a null Runnable to one of the
  "longer" constructors that provides a way to set things like
  stackSize, since there's no other way(?) to do that.

However, in this PR, I've added it universally.

(The Checker Framework would of course have a different perspective on
all of this.)

[wmdietl]

(The Checker Framework would of course have a different perspective on
all of this.)

I'm not so sure... the Javadoc for all these parameters say that null is a legal value and under no condition an NPE would be raised if null is passed.
Here, it's probably not so much about being conservative, but maybe "helpful": why would you create a Thread with a null Runnable? But on the helpfulness-side, JSpecify and CF don't need to vary much.

[wmdietl]

Looking through all the constructors:

• jdk/src/java.base/share/classes/java/lang/Thread.java

  Line 497 in 09f934f

  public Thread(Runnable target) {

  could be @Nullable, but how useful is a Thread that does nothing?

• jdk/src/java.base/share/classes/java/lang/Thread.java

  Line 585 in 09f934f

  public Thread(Runnable target, String name) {

  could also be @Nullable, but I'm not sure whether If {@code null}, this thread's run method is invoked. means anything different from does nothing. I guess subclasses of Thread could override run to do something different even with a null Runnable.

• jdk/src/java.base/share/classes/java/lang/Thread.java

  Line 633 in 09f934f

  public Thread(@Nullable ThreadGroup group, Runnable target, String name) {

  same as previous.

Should we add a comments to these or also make them @Nullable?

[cpovirk]

Thanks for the nudge. I've had this thread starred in my mail for a while, but I keep not getting back to it.

I had indeed misremembered how Thread.run() worked, and apparently it never dawned on me to check my memory. I had been envisioning it as unconditionally calling target.run() where it in fact no-ops if target is null, as you say.

So I'm with you now this is just about helpfulness, not NPEs.

I suspect that the behavior across constructors is meant to be presented as identical, even though the doc phrasing varies. We could still decide that a different annotation is more helpful for one constructor than for another.

I don't actually know what the CF or JSpecify position on helpfulness is :) My personal position has been that we have some design wiggle room except that we don't want for JSpecify annotations to call a type non-nullable when real users pass null in practice. That would probably suggest changing all of the target parameters to @Nullable, perhaps especially when the danger is "just" a silent no-op, not NPE.

(I'm assuming that someone passes null to each of the constructors, even though I found little evidence of that just now, aside from one project whose tests create non-functional threads to pass to Thread-accepting debug methods. I guess that, if nothing else, the constructors call one another with null arguments :))

I'd also add that anyone can always call the public no-arg Thread constructor (which is meant for subclasses) and get the same behavior. So I might actually prefer new Thread(/* target= */ null) if you actually really truly do want to do that (like for the tests I discussed above).

I've updated the PR to use @Nullable Runnable target. But I still don't have strong opinions here, so let me know if you've developed any :)

[wmdietl]

I'm fine with using @Nullable for these.

Are the GitHub Actions supposed to be green?

[cpovirk]

Thanks for checking on the CI. I've filed #99.