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 #43

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Add nullability annotations #43

merged 5 commits into from
Nov 29, 2023

Conversation

freya022
Copy link
Contributor

No description provided.

@felldo
Copy link
Owner

felldo commented Nov 28, 2023

I think it's better to add a dependency on compileOnly("org.jspecify:jspecify:0.3.0").
It is pretty widely supported and saves us pretty much all annotations. While in your case you could use @ParametersAreNonnullByDefault on the package-info file, it doesn't help with return types and afaik it is not possible to mark generic parameters an non null.

Therefore I suggest to add the mentioned dependency instead and add a package-info.java file which contains the following:

@NullMarked
package net.fellbaum.jemoji;

import org.jspecify.annotations.NullMarked;

With this all types should be declared a NonNull if not otherwise marked as Nullable

@freya022
Copy link
Contributor Author

freya022 commented Nov 28, 2023

I don't like implicit non-null, I'd rather have either NotNull/Nonnull, or Nullable on the returned types and parameters.

As for generic types, they are supported by annotations with TYPE_PARAMETER or TYPE_USE targets.
The JetBrains annotations do support it, but the java compiler has a bug which makes annotations satisfying more than 1 scope, be duplicated in the javadocs, which is why I am not using it here

@freya022
Copy link
Contributor Author

At least jspecify annotations works with Kotlin so, alright, but it's quite weird how you can use them on generics, but they are still seen as platform types. Though using @NullMarked on the package does help for generics too

@freya022
Copy link
Contributor Author

Last question though: Do we keep the dependency compileOnly? When looking at the sources, the annotation is not present in the classpath, and so, creates errors.

I think adding a 3 KB dependency isn't too much

@felldo
Copy link
Owner

felldo commented Nov 28, 2023

I don't like implicit non-null, I'd rather have either NotNull/Nonnull, or Nullable on the returned types and parameters.

I don't really agree with you on that, but imo null annotations can clutter the code quite a bit in a language thats already very verbose. This is a completely null free library and if a null value is returned, it is a bug. For that reason I don't think it is a big deal to just specify that everything returns NonNull in the package info.
Btw. why did you add a @Nullable to the parameters? Even if the null case is handled, it is not really a "valid" argument to pass as you will never get something different than an empty string/collection/optional an therefore shouldn't be Nullable.

Last question though: Do we keep the dependency compileOnly? When looking at the sources, the annotation is not present in the classpath, and so, creates errors.

I think adding a 3 KB dependency isn't too much

For that case I think we can add the dependency as compileOnlyApi though I am not 100% sure if this is the right thing to do. You can test if it resolves this issue, otherwise I would keep it as compileOnly.

@freya022
Copy link
Contributor Author

Issues solved

Copy link
Owner

@felldo felldo left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@felldo felldo merged commit 504c1c2 into felldo:master Nov 29, 2023
1 of 2 checks passed
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