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

Document the nullness of special APIs on enum and annotation types #509

Open
cpovirk opened this issue Apr 29, 2024 · 4 comments
Open

Document the nullness of special APIs on enum and annotation types #509

cpovirk opened this issue Apr 29, 2024 · 4 comments

Comments

@cpovirk
Copy link
Collaborator

cpovirk commented Apr 29, 2024

  • all enum constants are non-nullable
  • SomeEnum.values() returns a non-nullable array of non-nullable values
  • SomeEnum.valueOf(String) returns non-null (which may be a useful fact for tools that want to warn about unnecessary null comparisons, like https://errorprone.info/bugpattern/ImpossibleNullComparison)
  • annotation element methods return non-nullable values

I may be forgetting other examples, as well as other autogenerated methods with known nullnesses. I guess we already have some posts about equals methods of record classes, as distinct from equals methods as a whole. (That first link mentions the alternative of considering all such methods to be null-unmarked. But I doubt there's much upside to that.)

We could discuss where this information belongs. For now, I wanted to jot some notes so that I can link to them from elsewhere.

@cpovirk
Copy link
Collaborator Author

cpovirk commented May 5, 2024

There are really 2-3 separate classes of APIs here, which we could choose to handle differently:

  • APIs that are generated by the compiler (values(), valueOf(String)): These are somewhat similar to generated equals methods on record types, which might someday be identified as "mandated" (ACC_MANDATED). In this case, as in the equals case, there is no way for users to annotate the APIs directly. However, unlike in the equals case, a lack of annotations is completely correct, at least inside @NullMarked code, since values() and valueOf(String) have non-nullable parameter and return types. JSpecify and/or tool authors could still choose to recognize those APIs as non-nullable even in null-unmarked code. But because @NullMarked does the right thing if users apply it, further handling is more of a nice-to-have.
  • Enum constants: These are their own special thing: They're declared in source code but not using normal field syntax. As a result, there's no place to put a @NonNull annotation or @NullMarked annotation. (@NonNull might be allowed by javac, but even if so, it's not allowed by JSpecify.) Beyond that, they're similar to the previous bullet: A lack of annotations is completely correct in @NullMarked code, so we're discussing only nice-to-have additional handling for null-unmarked code. And since we're talking about the types of read-only fields, the situation is similar to that of the return types of the non-overridable methods values() and valueOf(String): Knowing that the result of a usage is non-nullable doesn't change things much for users, since most tools behave similarly for a non-nullable result as for an unannotated result. (The main actual win for the previous bullet is the ability to reject a null argument to valueOf(String).) Still, at least in Kotlin, we've seen that it's bad for a local variable to be inferred to have a platform type: That lets code later assign a nullable value to it but then dereference it!
  • Annotation elements: Unlike the previous two bullets, these can be annotated normally. But that distinction isn't as relevant as it could be: They fall into the same category as the previous two: A lack of annotations is completely correct in @NullMarked code. I guess that the only difference here is that tools could complain to users who try to annotate a return type as @Nullable.

(I think I confirmed recently that Kotlin recognizes the non-nullness of all these APIs already.)

@cpovirk
Copy link
Collaborator Author

cpovirk commented May 5, 2024

(To be clear, I don't think that any of this matters a ton. I'm just continuing to write notes as they pop into my head.)

In practice, what matters most is probably special handling for valueOf(String) and values(), including the return types to avoid a problem like Kotlin has with platform types in general (but which it avoids for those methods by recognizing their non-nullness). It might also help users who write something like if (Foo.valueOf(bar) == null), since checkers could recognize that as an unnecessary check. (We have seen mixups like that in practice.)

In theory, what matters most is probably the special handling for annotation elements: No one should be able to implement an annotation with nullable return types (either by declaring the annotation itself with nullable return types or by overriding methods inherited from a null-unmarked annotation type), since there is already a contract that annotation elements should have non-null values.

@cpovirk cpovirk changed the title Document the nullness of special APIs on enum and annotation type Document the nullness of special APIs on enum and annotation types May 5, 2024
@kevinb9n
Copy link
Collaborator

kevinb9n commented May 8, 2024

Fortunately I don't see anything too controversial here, so I agree with it just being filed as a docs to-do.

@cpovirk
Copy link
Collaborator Author

cpovirk commented May 22, 2024

In theory, this could become part of the spec, similar to #301 (comment) (mentioned in the original post). But I don't see that as a 1.0 blocker, since tools are always welcome to add their own knowledge to what JSpecify provides. (#301 is arguably more important than this one, since JSpecify rules there can give the "wrong" answer ("equals requires a non-nullable argument") as opposed to mostly just leaving something unspecified in the case of this issue.)

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

No branches or pull requests

2 participants