WICKET-7145: added @Nonnull to parameters
#1093
Conversation
There was a problem hiding this comment.
I'm about 1/2 through the review a couple of comments:
-
If there are null check fixes, it would be good if they were in a separate commit so those changes could potentially be back-ported.
-
Please see the code style notes around import static and end-of-file new lines.
| @@ -47,6 +45,10 @@ | |||
| import org.junit.jupiter.api.BeforeEach; | |||
| import org.junit.jupiter.api.Test; | |||
|
|
|||
| import static org.assertj.core.api.Assertions.assertThat; | |||
| import static org.junit.jupiter.api.Assertions.assertEquals; | |||
| import static org.junit.jupiter.api.Assertions.assertNull; | |||
There was a problem hiding this comment.
static imports usually come before non-static?
| } |
There was a problem hiding this comment.
Does the Wicket code style call for newline or no newline at end of file?
| } |
| } |
| } |
| } |
| import javax.annotation.Nonnull; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; |
| <dependency> | ||
| <groupId>jakarta.annotation</groupId> | ||
| <artifactId>jakarta.annotation-api</artifactId> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Good time to fix the wicket-native-websocket-core/pom.xml formatting?
| } |
| public FilenameWithVersionResourceCachingStrategy(String versionPrefix, | ||
| IResourceVersion resourceVersion) | ||
| public FilenameWithVersionResourceCachingStrategy(@Nonnull String versionPrefix, | ||
| @Nonnull IResourceVersion resourceVersion) |
There was a problem hiding this comment.
Double check for unintended whitespace
| <dependency> | ||
| <groupId>jakarta.annotation</groupId> | ||
| <artifactId>jakarta.annotation-api</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Good time to fix pom formatting?
Thanks, I'll fix everything once you're done.
There is only 1 in production code:
I'll check the notes and will make sure all changed files comply with them. |
|
Is there a reason you went with JSpecify is intended to be the defacto standard and Spring Framework recently migrated all their annotations to it. I recently annotated our main project with it but I went the other way and used JSpecify's recommended strategy of applying |
That would be great, please do. I think there will be users on Wicket 9.x for a while since it is the javax.* based release. |
If JSpecify's approach becomes the standard, it will be in an updated release of the jakarta.annotation API. I do not see any reason to bring in a non-standard dependency when the standardized Jakarta-based solution is sufficient. |
The whole nullability discussion has been going on for years. There are findbugs annotations, checker framework annotations, jetbrains, nullaway, javax/jakarta and many more. All of them have their advantages and disadvantages. JSpecify is an attempt (by Google, Facebook, Uber, etc) to standardize these annotations and define their exact semantics. The jakarta.annotation package is simply not enough. There is no way using only these annotations to define the default nullability of a module/package/class. What does it mean now when a parameter is not annotated? Does it mean it is nullable? Or is its nullability unspecified? JSpecify solves these ambiguities. Having said that, annotating with jakarta.annotation is probably still better than nothing, though I would have preferred annotating with This is not a veto from me, just my opinion. |
I have been a Kotlin developer for years, so I have no knowledge of recent annotation libraries. I chose the Jakarta annotations because the library was already in the BOM. But the annotations for nullability had not been used anywhere yet. As for the other concerns:
|
There is also a huge graveyard of dead projects from large organizations that take passes at solving for short comings. I agree, that the spec could be improved. I agree the the Jakarta Annotation Team should take this up-- if a spec doesn't meet the needs of users, it won't be viable.
If something isn't marked, it should be presumed to be 'nullable', since that is the language default. IMO, less noise is better. Nulls are a feature, not a bug. Null values don't use memory and do not require object allocations that need to be garbage collected. Annotating everything and throwing Optionals everywhere creates non-zero side effects and performance impacts. Related Jakarta Annotation update requested: |
Marking with @jstuyts: Are you consuming Wicket APIs from Kotlin? If so, does adding these |
@theigl Yes, I do. I just tested, and no, they unfortunately don't. Of the many options, I chose the one that is not supported by Kotlin (Jakarta annotations are not listed on this page and not in the code linked to): https://kotlinlang.org/docs/java-interop.html#nullability-annotations I switched to JSpecify, and although that does result in warnings in Kotlin, it won't allow me to specify So it seems to me that JSpecify is not in a usable state yet. Is there a preference for one of the other supported annotations? I don't see a good candidate as most of them are part of a library that is not specifically meant to strengthen contracts, is vendor-specific or is (as good as) obsolete. |
JSpecify is perfectly usable already but it has a couple of non-intuitive annotation usages. Your example is one of them. See https://jspecify.dev/docs/user-guide/#some-subtler-details. You have to either use static imports or write it like this:
Another example is with arrays. I always have to think twice where to put the annotation depending on if the array itself is nullable or its elements are. |
I only found the issues in 9.x. In 8.x the methods are used correctly: https://issues.apache.org/jira/browse/WICKET-7147 |
Got it. I have created a branch using JSpecify: https://github.com/jstuyts/wicket/tree/add-non-null-to-parameters-jspecify The advantages are:
Let me know if you decide you would like to use JSpecify instead of the Jakarta annotations. I will close this PR and create a new one. Another remark about which annotations to use. I think the annotations should be added incrementally, and both annotations (nullable and not nullable) must be added. Once everything is annotated (which can take quite some time), the decision to switch to package-level (or some other level) annotations can be made. It is then also easy to determine which of the 2 annotations results in the least amount of code. The most occurring annotation can be changed to higher-level annotations. |
Added
@Nonnullto parameters that are checked fornull. Also included some method overloads/variants.In addition found some bugs in
nullchecks.See https://issues.apache.org/jira/browse/WICKET-7145 for details.