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

Add nullability annotations to main API and extension points #215

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

Conversation

berrueta
Copy link
Contributor

@berrueta berrueta commented Jan 7, 2019

This PR adds nullability annotations (JSR 305) to the main API and extension points. For conciseness and safety, the annotations are not comprehensive. There is a lot of room to continue annotating, but in this PR I have selected the methods that are likely to have a bigger impact.

These annotations provide a richer API contract that allows compilers, IDEs and static analysis tools to do a better job at helping developers and preventing bugs.

For example, before this PR, if a developer wanted to implement a new Gen in Kotlin, the IDE would automatically generate the following skeleton:

class MyGenerator : Gen<String> {
    override fun generate(sourceOfRandomness: SourceOfRandomness?, generationStatus: GenerationStatus?): String {
        TODO("implementation goes here")
    }
}

The question marks after the parameter types indicate that the parameters are nullable, because the IDE does not know better and makes the safest choice on behalf of the developer (this is called a "platform type" in Kotlin). The developer then has two choices:

  • keep the question marks and explicitly handle the nullable value, if the developer doesn't know better either and wants to be defensive, or
  • remove the question marks to indicate to the compiler that the developer is sure the values are non-nullable. That requires the developer to understand the intention of the library creator.

With the changes in this PR, the two parameters are now annotated with @Nonnull in the interface. The IDE will generate the following skeleton:

class MyGenerator : Gen<String> {
    override fun generate(sourceOfRandomness: SourceOfRandomness, generationStatus: GenerationStatus): String {
        TODO("implementation goes here")
    }
}

Note the absence of question marks. The developer does not need to be defensive and handle a hypothetical null, and the type checker can guarantee that the implementation of the method is NPE-free.

The drawback of this change is that it may cause compilation errors in some applications. Going back to the first example, if the developer decided to keep the question marks, this PR will cause a compilation error, because the generate method in the application no longer overrides the generate method in the interface (they have different signatures). The fix is simple: just remove the question marks to match the signature of the interface. This problem may affect some Kotlin users, and potentially also Java applications using static analysis tools like Findbugs.

@pholser
Copy link
Owner

pholser commented Jan 9, 2019

👀

<dependency>
<groupId>com.google.code.findbugs</groupId>
<artifactId>jsr305</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you add this dependency? It doesn't seem to be used anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dependency provides the javax.annotation.Nonnull and javax.annotation.Nullable annotations from JSR-305.

Alternatively we could use some other vendor-specific annotations with similar purpose, for example https://github.com/JetBrains/java-annotations

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.

None yet

3 participants