Skip to content
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

Treat @PolyNull like @Nullable. #206

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Treat @PolyNull like @Nullable. #206

wants to merge 1 commit into from

Conversation

cpovirk
Copy link
Collaborator

@cpovirk cpovirk commented Sep 19, 2024

This seems like something that we would have discussed previously, but I can't find any evidence of that.

  • Maybe the hope was that we'd soon implement full support from scratch based on forthcoming JSpecify rules for @PolyNull?
  • Maybe the hope was that we'd be able to make @PolyNull "imply" @Nullable soon?
  • Maybe the plan was to tell Checker Framework users to write @Nullable @PolyNull?
  • Maybe we once had a principle of recognizing only annotations that were "universally" recognized (or at least recognized by Kotlin), while @PolyNull is not? But if so, we moved away from that long ago.

For now, it seems useful to at least recognize it. I could see us backtracking if we were to fall back to a policy of (at least optionally) recognizing only JSpecify annotations or only "universally" recognized annotations. But if we do so, that will involve a larger change than just reverting back this PR.

(My immediate motivation is that J2CL is looking at @PolyNull support (@kevinoconnor7 FYI), and we've been trying to keep our lists of annotations in sync with theirs.)

Hmm, and you know, only now that I'm writing this am I realizing that making @PolyNull imply @Nullable would not actually be sound, at least for the class definition of @PolyNull. I'll leave a note on jspecify/jspecify#79 and maybe in a doc or two that we have going about @Implies, assuming that I don't convince myself that I'm wrong by then. If I'm right, then it would actually make conceptual sense to not merge this PR, either....

This seems like something that we would have discussed previously, but I can't find any evidence of that.

- Maybe the hope was that we'd soon implement _full_ support from scratch based on forthcoming JSpecify rules for `@PolyNull`?
- Maybe the hope was that we'd be able to [make `@PolyNull` "imply" `@Nullable`](jspecify/jspecify#307 (comment)) soon?
- Maybe the plan was to tell Checker Framework users to write [`@Nullable @PolyNull`](jspecify/jspecify#35)?
- Maybe we once had a principle of recognizing only annotations that were "universally" recognized (or at least recognized by Kotlin), while `@PolyNull` is _not_? But if so, we moved away from that long ago.

For now, it seems useful to at least recognize it. I could see us backtracking if we were to fall back to a policy of (at least optionally) recognizing _only_ JSpecify annotations or _only_ "universally" recognized annotations. But if we do so, that will involve a larger change than just reverting back this PR.

(My immediate motivation is that J2CL is looking at `@PolyNull` support (@kevinoconnor7 FYI), and we've been trying to keep our lists of annotations in sync with theirs.)

Hmm, and you know, only now that I'm writing this am I realizing that making `@PolyNull` imply `@Nullable` would not actually be sound, at least for the class definition of `@PolyNull`. I'll leave a note on jspecify/jspecify#79 and maybe in a doc or two that we have going about `@Implies`, assuming that I don't convince myself that I'm wrong by then.
@cpovirk
Copy link
Collaborator Author

cpovirk commented Sep 19, 2024

Hmm, and you know, only now that I'm writing this am I realizing that making @PolyNull imply @Nullable would not actually be sound, at least for the class definition of @PolyNull.

Eh, nope, I already have to take that back :) I feel better about this PR. But to try to write down what I have been thinking since my last post:

We already know that a checker that treats @PolyNull only like @Nullable will be unsound:

@PolyNull Object foo(@PolyNull Object o) {
  return null; // should be an error but is actually allowed
}

What had caught my attention was a more elaborate case that involved generics:

@NullMarked
class Bar<T extends @Nullable Object> {
  @PolyNull T foo(@PolyNull T t) {
    ...
  }
}

Bar<@Nullable String> bar;
bar.foo(...);

In that case, I was envisioning... I'm not entirely sure what, but it was going to be related to a point that I've already harped on before, which is that the existing @PolyNull means "@Nullable or @NonNull," rather than "@Nullalble or NO_CHANGE." So, while it might seem as if the foo call above would always be dealing with a @Nullable type (thanks to the @Nullable String type argument), it should in theory also consider the possibility of a non-null type. Somehow I was thinking that that would then lead to new trouble. But every time I try to put my finger on how, it slips away :)

In short, I don't think my new example fundamentally different from the simple, known example at the beginning of this post.

It is still possible that there's something interesting to be said about other generic cases, such as with a type like List<@PolyNull String>: While plain @Nullable String is more general than @NonNull String, List<@Nullable String> is not strictly more general than List<@NonNull String>. Even then, though, I'm not sure that treating @PolyNull like @Nullable is any worse than not treating it like @Nullable. And there are plenty of cases in which not treating is like @Nullable is much worse.

So if there is any takeaway here, it's that treating @PolyNull like @Nullable may be technically incorrect in ways that may or may not actually matter. That could be interesting to think further about, but we already know of big practical problems that that treatment would cause, like in the handling of Optional.orElse and Class.cast.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants