unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
122 stars 42 forks source link

Please add NonNull/Nullable annotations #248

Closed remal closed 3 years ago

remal commented 5 years ago

I'd like to use the library with Kotlin language, which can handle NonNull/Nullable annotations during compile-time.

Please add these annotations in the library. If you want, I can do it myself by sending a PR.

keilw commented 5 years ago

Which ones? there are almost too many such annotations, and several of those projects are dead or have very little activity.

remal commented 5 years ago

I'd choose between JSR305 (com.google.code.findbugs:jsr305 for example), or Jetbrains' ones: org.jetbrains:annotations.

The full list of annotations supported by Kotlin lang can be found here: https://github.com/JetBrains/kotlin/blob/master/core/descriptors.jvm/src/org/jetbrains/kotlin/load/java/JvmAnnotationNames.kt

keilw commented 5 years ago

According to https://jcp.org/en/jsr/detail?id=305 JSR 305 is dormant for over 7 years now. While javax.annotation.Nullable sounds best, it is part of that dormant JSR and worse, the package name is shared with Jakarta Common Annotations, an absolute No-go unless that annotation became part of Jakarta Annotations some day (package-splitting is prohibited by Jigsaw) These https://github.com/JetBrains/java-annotations by JetBrains look decent, most of the others are either defunct or come with to much overhead (eg. RXJava does not have a separate annotation module) or target Android mostly.

keilw commented 5 years ago

This is also mentioned in https://github.com/eclipse-ee4j/common-annotations-api/issues/50, so in a future version we might simply use Jakarta Annotations (which is already used by SI Units 2.x) for both purposes.

teobais commented 5 years ago

For the reasons mentioned by @keilw in the above two comments, I would also not investigate such a possibility at the moment.

keilw commented 5 years ago

@remal What would the annotations bring in particular, and have you checked https://github.com/Tenkiv/Physikal for use with Kotlin?

remal commented 5 years ago

@keilw

  1. There are some static analysis tools that rely on NonNull/Nullable annotations. They help to avoid NullPointerException bugs
  2. No, I haven't checked this project, I'll take a close look, thanks! It looks like only a list extensions...
keilw commented 5 years ago

Well 2.0 is already done, but we could include it in a follow up version.

remal commented 5 years ago

Just to be on the same page. @keilw, you're going to add @NonNull/Nullable annotations to one of the next versions, right?

keilw commented 5 years ago

@remal If there is a more stable spec like Jakarta Annotations including such annotations, we might do that I see no reason against it, especially @Priority is already used, so we would get it for free. Until then marking it as deferred.

keilw commented 4 years ago

We won't do that until Jakarta Annotations included the currently defunct JSR 305 annotations: https://github.com/eclipse-ee4j/common-annotations-api/issues/50

keilw commented 3 years ago

@remal @thodorisbais after looking at this again, quite frankly I would leave it to application code to apply a @NonNull or any annotation of their choice. Although it may lead to undesired side-effects there are good reasons why e.g. the symbol inside a Unit implementation or a Quantity value could be null, e.g. SPI base types like Range where we evaluated Optional, but unfortunately that is not very well-designed for nested generics either and it caused problems, therefore nullness is allowed to represent a missing upper or lower-bound. Almost everywhere else internally there are null-checks, unless a null-value is intentionally allowed, so there is not a real value to use those annotations inside the RI, while it is up to developers to combine it with Bean Validation or other annotations in their own code.

remal commented 3 years ago

@keilw Basically, this issue is not about runtime checks inside the library itself - it's mainly about static analysis. Indeed, Java developers won't benefit much from it (only from static analysis perspective), but Kotlin developers will, as Kotlin handles nullity annotations and behaves accordingly.

keilw commented 3 years ago

@remal Unfortunately there is no accepted standard for that, see https://stackoverflow.com/questions/4963300/which-notnull-java-annotation-should-i-use. So please keep an eye on Jakarta Annotations, if they ever pick up the "Walking Dead" of JSR 305 we might consider that (as we already support Jakarta Annotations even all the way down to the API) feel free to raise a new ticket, but in the meantime it makes no sense.