FabLabsMC / fiber

A configuration system. Maintained by @zeroeightysix and @Pyrofab
Apache License 2.0
32 stars 8 forks source link

Library used for nullable, etc. annotations #30

Open zeroeightysix opened 4 years ago

zeroeightysix commented 4 years ago

In https://github.com/FabricMC/fabric/issues/499 the library used for the @Deprecated is being discussed. I'd like to use the same library for the @Nullable annotations (and variants).

Note IDEA currently provides the following warning for all usages of javax annotations:

Not 'javax.annotation.Nullable' but 'org.jetbrains.annotations.Nullable' would be used for code generation. 

This issue serves as a tracker until https://github.com/FabricMC/fabric/issues/499 is solved.

liach commented 4 years ago

Should we use jetbrains or javax? javax annotations imo are not sustainable in long term as we might have things like nested nullable types

zeroeightysix commented 4 years ago

I prefer jetbrains annotations as well. Fiber currently uses javax solely because it's in the standard lib.

liach commented 4 years ago

With jetbrain annotations/other type annotations and some tweaks to https://github.com/FabLabsMC/fiber/blob/9096f8160b17a882bc3bab3ac7ba0c822dff6948/src/main/java/me/zeroeightsix/fiber/annotation/magic/TypeMagic.java#L34, it will be able to handle the nullability of nested types properly

Pyrofab commented 4 years ago

I've been dying to use Jetbrains annotations everywhere in my PRs. With that said, would Fabric Loader accept a compile-time dependency on those ?

liach commented 4 years ago

Yep, but a problem with jetbrain annotations is that they are class-level retention (i.e. aren't exposed at runtime so config's type resolution cannot perform nullable checks unless using asm to grab those annotations)

i509VCB commented 4 years ago

Does checker preserve annotations into runtime?

liach commented 4 years ago

Does checker preserve annotations into runtime?

https://github.com/typetools/checker-framework/blob/6cf4e4f2cb829bb39a3bc33f3a11b547f3afe3dd/checker/src/main/java/org/checkerframework/checker/nullness/qual/Nullable.java#L27 It does

Pyrofab commented 4 years ago

I did not consider using nullness annotations for runtime checks. That is an excellent idea, it could use a new NonNullConstraint.

ghost commented 4 years ago

I'd vote for Jetbrains. The Javax annotations cause build problems when I'm using CurseGradle on Github Actions (and the error log leads to this project). I've had to manually add the Javax annotations as an implementation even though I personally use Jetbrains.

RemainingToast commented 4 years ago

Jetbrains ftw