WICKET-7145: added @Nonnull to parameters

[jstuyts]

Added @Nonnull to parameters that are checked for null. Also included some method overloads/variants.

In addition found some bugs in null checks.

See https://issues.apache.org/jira/browse/WICKET-7145 for details.

[mattrpav]

I'm about 1/2 through the review a couple of comments:

1. 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.

2. Please see the code style notes around import static and end-of-file new lines.

[mattrpav]

static imports usually come before non-static?

[mattrpav]

Does the Wicket code style call for newline or no newline at end of file?

[mattrpav]

Code style EOF new line?

[mattrpav]

Code style EOF new line?

[mattrpav]

Code style EOF new line?

[mattrpav]

Code style EOF new line?

[mattrpav]

static imports first?

[mattrpav]

Good time to fix the wicket-native-websocket-core/pom.xml formatting?

[mattrpav]

Code style EOF new line

[mattrpav]

Double check for unintended whitespace

[mattrpav]

Good time to fix pom formatting?

[jstuyts]

I'm about 1/2 through the review a couple of comments:

Thanks, I'll fix everything once you're done.

1. 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.

There is only 1 in production code: OriginResourceIsolationPolicy (search for Checks.notNull). The other 2 are in test code: BaseWicketTester and WicketTesterTest (search for assertNull). If you want, I can create a separate issue and PRs for backporting those.

2. Please see the code style notes around import static and end-of-file new lines.

I'll check the notes and will make sure all changed files comply with them.

[theigl]

Is there a reason you went with jakarta-annotations and not with JSpecify?

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 @NullMarked on the package level und marking @Nullable parameters.

[mattrpav]

There is only 1 in production code: OriginResourceIsolationPolicy (search for Checks.notNull). The other 2 are in test code: BaseWicketTester and WicketTesterTest (search for assertNull). If you want, I can create a separate issue and PRs for backporting those.

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.

[mattrpav]

JSpecify is intended to be the defacto standard and Spring Framework recently migrated all their annotations to it.

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.

[theigl]

JSpecify is intended to be the defacto standard and Spring Framework recently migrated all their annotations to it.

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 @Nullable instead.

This is not a veto from me, just my opinion.

[jstuyts]

Is there a reason you went with jakarta-annotations and not with JSpecify?

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 @NullMarked on the package level und marking @Nullable parameters.

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:

• Something that is not annotated will indeed have an ambiguous meaning: it is nullable, or it has not been checked for non-nullability yet. Specifying at some higher level that all of the API is nullable if marked (or the other way around) won't help here: you still need to add an annotation to each variable or method that is nullable (or not) before it matches how the code behaves. Until that has been done, the other variables and methods are ambiguous too.
• About using @Nonnull: in Java the default is that a reference is nullable, so for me it is more logical to state that you cannot assume the standard Java semantics.

[mattrpav]

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.

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.

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.

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:
ref: jakartaee/common-annotations-api#145

[theigl]

If something isn't marked, it should be presumed to be 'nullable', since that is the language default.
IMO, less noise is better.

Marking with @Nullable produces significantly less noise in my experience. While it is true that at the language level almost everything is nullable, in a typical project, the majority of values are never null. I recently annotated hundreds of thousands of LOC in my main project and I would have needed at least 3-5x the amount of annotations if I went with @Nonnull.
But marking as @Nullable only makes sense together with a @Nullmarked or @ParametersAreNonnullByDefault annotation. Otherwise it doesn't provide much value. So if we stick with jakarta.annotation I think that only the current @Nonnull approach makes sense.

@jstuyts: Are you consuming Wicket APIs from Kotlin? If so, does adding these @Nonnulls already help with null-safety in Kotlin?

[jstuyts]

@jstuyts: Are you consuming Wicket APIs from Kotlin? If so, does adding these @Nonnulls already help with null-safety in Kotlin?

@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 @NonNull everywhere where the Jakarta annotation used to be. The compiler trips over non-simple names: fully-qualified names and nested types. For example: @NonNull WebResponse.CacheScope scope results in a compile error. I had to remove annotations because of this.

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.

[theigl]

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:

WebResponse.@NonNull CacheScope scope

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.

[jstuyts]

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.

I only found the issues in 9.x. In 8.x the methods are used correctly: https://issues.apache.org/jira/browse/WICKET-7147

[jstuyts]

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.

Got it. I have created a branch using JSpecify: https://github.com/jstuyts/wicket/tree/add-non-null-to-parameters-jspecify

The advantages are:

• It seems like the Java ecosystem is moving to JSpecify as the semantics are more precise and there is more tooling support.
• The Kotlin compiler understands the annotations of JSpecify.

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.

[theigl]

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.

I prefer the JSpecify branch because of the advantages you mentioned!

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.

What I did in my project is this:

1. Mark a package as @NullMarked in package-info.java
2. For each class in the package:
   • Mark all parameters as @Nullable that IntelliJ reported as null being passed or that are null-checked inside the method
   • Mark all return values as @Nullable that IntelliJ reported as possibly returning null or where the return value is null-checked
3. Repeat until almost all packages are marked and IntelliJ shows no more warnings
4. Optional: Remove all package-info.java files and place a single @NullMarked annotation in the module-info.java.

This is basically the incremental strategy that JSpecify recommends.

What approach we take will mostly depend on how much time we want to spend.

[jstuyts]

This is basically the incremental strategy that JSpecify recommends.

Yes, but Wicket is a public project. It is undesirable to have things in a state that is incomplete, I think, as this would result in a lot of errors/warnings for users of Wicket.

Doing everything at once would result in a giant PR. I think this one is already stretching the limit. I have a few more coming if this (or the JSpecify variant) is merged, and will make sure those PRs are smaller.

So I think that higher-level declarations of (non-)nullability is not an option for now.

[theigl]

So I think that higher-level declarations of (non-)nullability is not an option for now.

Agreed. That only makes sense if we completely annotate a whole module or at least a whole package.

I think we can go ahead with your current JSpecify PR and continue improving the annotations in future work.

The only minor concern I still have with this PR is that we are adding @NonNull. From my experience (and also what JSpecify recommends and large projects like Spring are doing) @Nullable makes more sense in the long run. At some point, we will have to remove all these @NonNull annotations. But this should be a trivial change.

And then there is the question of how we continue with this and what we require in future PRs. Is our goal to completely annotate the whole Wicket codebase? Do we add nullability annotations whenever we touch existing code? Do we require them in PRs? Or is this a one-time effort to slightly improve the developer experience?

[jstuyts]

I think we can go ahead with your current JSpecify PR and continue improving the annotations in future work.

I'll wait for what the final decision will be: Jakarta or JSpecify.

The only minor concern I still have with this PR is that we are adding @NonNull. From my experience (and also what JSpecify recommends and large projects like Spring are doing) @Nullable makes more sense in the long run. At some point, we will have to remove all these @NonNull annotations. But this should be a trivial change.

Like I said before: once everything is annotated, it is easy to decide which of the 2 annotations can stay.

And then there is the question of how we continue with this and what we require in future PRs. Is our goal to completely annotate the whole Wicket codebase? Do we add nullability annotations whenever we touch existing code? Do we require them in PRs? Or is this a one-time effort to slightly improve the developer experience?

A recommendation to add the annotations when adding or modifying code is the least that can be done. Maybe split PRs: first a PR that adds the annotations to existing code, and then a PR that contains clearer code for the modifications because of the added annotations. This should make reviews easier.

Annotating everything would be best, but there are some tricky situations, that force clients that can never see a null to work with nullable variables. Here are a few I found so far:

• Theoretically getComponent() of a behavior can return null (for example, call it before binding the behavior), but I think that practically getComponent() is never called when the behavior is unbound. There are a couple of options here, but I want to discuss those when I create an issue for behaviors.
• The IValidatable passed to IValidator will not have a null value, unless the validator implements INullAcceptingValidator.

[martin-g]

I'll wait for what the final decision will be: Jakarta or JSpecify.

For me both are meh.

• The annotations will add a lot of noise
• The changes will make cherry-picking harder, unless someone volunteers to backport them to 9.x too
• I am not coding Java since several years (happy user of std::result::Result<T, E>!) but in my past experience the lack of JSR-305 usage and NPEs caused by Wicket APIs was not a (big) problem

But if others decide to use it in Wicket then I'd vote for JSpecify.

[theigl]

I'll wait for what the final decision will be: Jakarta or JSpecify.

For me both are meh.

• The annotations will add a lot of noise
• The changes will make cherry-picking harder, unless someone volunteers to backport them to 9.x too

Yeah, both of these concerns are valid.

In my experience, when a whole package is marked as @NullMarked and only @Nullable is used, the noise is limited and the safety gained from it is worth it (and no more unnecessary if-not-null-checks). Also when new code or files are added to the package, the IDE will automatically show warnings if new code is not null-marked correctly.

I'm also unsure about proceeding with this if we don't have a concrete and useful final result in mind.

[jstuyts]

I'm also unsure about proceeding with this if we don't have a concrete and useful final result in mind.

The desired end result would be that for every variable and method in Wicket it is known whether it is nullable or not. This may take a very long time to achieve. And may even be unsatisfactory: for some situations a client is guaranteed that the value will never be null, but as there are other situations where it can be null the variable or method has to be marked as nullable.

So the perfect state where you know exactly what is nullable will likely never be achieved. Even so, even adding nullable information to the most-used types of Wicket will greatly improve the developer experience. Yes, it is too bad that nobody took the time to add the annotations to less-used parts. And yes, it is unfortunate that a value is marked as nullable that will never be null because of the way you set things up.

Both issues can be solved: more effort by people who want to contribute, and new, more strict APIs and backward-incompatible changes to contracts of existing APIs (if acceptable for Wicket users).

In my opinion adding nullability information to even a small number of places, makes Wicket easier to use. This will hopefully help convince developers that Wicket still is a good choice for front end development.

Note that knowing what is nullable is one of the reason I love working with Kotlin: no more going through incomplete documentation, and then through multiple layers of code to figure out whether or not I should handle nulls. It saves me so much time. This time saving can also be (partially) realized for Wicket developers.

[keesvandieren]

For seamless Kotlin integration, having proper @nullable and @NotNull annotations is extremely helpful.

We're using Kotlin with Wicket, and one challenge we've encountered is the need to manually remove nullability when overriding methods. This adds unnecessary friction and makes the combination of Kotlin and Wicket feel less fluent. Improving support for nullability annotations would greatly enhance the developer experience.

[theigl]

I thought about this some more over the weekend: It took me about 30 hours to fully annotate our ~350KLOC codebase by using the approach described above. Wicket is much smaller than that and we probably only need to annotate the most important packages in wicket-core and wicket-request to get immediate benefits for Kotlin users.

I would suggest the following approach:

1. Create a PR that adds JSpecify but does not annotate anything and merge it
2. Create PRs that annotate a single package or a small group of packages with @NullMarked and @Nullable

That way individual PRs should be relatively small and easy to review and by using only @Nullable the noise should be kept to a minimum.

Ideally, these changes would be driven by someone who actually uses Wicket with Kotlin and can verify the improvements.

[jstuyts]

@theigl I tried your approach, but I think it is more hassle than adding @NonNull. I think the Wicket codebase is not suitable for @NullMarked because in a number of cases the nullability depends on the state of the object. For example: late initialization, attach/detach, and bind/unbind. Here is what I did.

I added @NullMarked to package org.apache.wicket so I have multiple source code files to work with for finding out what the consequences are. It could also go on a type so there is less code in a patch.

I made a copy of default inspection profile of IntelliJ with any inspection not related to null disabled, so I can see how many warnings appear.

• Application: 61 warnings related to null. There are some warnings that say the same thing, so there actually are less than 61 warnings. Let's see what it takes to resolve the warnings related to 1 field:

  • sessionStoreProvider: warning that @Nullable must be added as the field is not initialized in the constructor. In Kotlin you would just slap lateinit on it, but Java and JSpecify have no such construct. That leaves 2 possible solutions:

    • Mark sessionStoreProvider with @Nullable.

      • Now getSessionStoreProvider() has a warning that a non-nullable getter returns a nullable value.
      • Let's try Checks.notNullShort(sessionStoreProvider, "Session store provider") before the return: no, still a warning (plus additional overhead of a method call that was not there before, and an extra string allocation).
      • Inline the invocation of notNullShort(...) and end up with Checks.notNull(sessionStoreProvider, "Session store provider may not be null."). That removes the warning (and still additional overhead).
      • In validateInit() there are warnings about the getters never returning null. This can be resolved by inlining the call to the getter (which is final).

    • Suppress the warning for sessionStoreProvider: @SuppressWarnings("NotNullFieldNotInitialized"). A comment could be added to clarify the warning is suppressed because of lazy initialization, but unfortunately this comment cannot be part of the annotation that suppresses the warning.

      • Again warnings in validateInit() about the getters never returning null. So again inline the call to the getter.

• RequestCycle: 6 warnings because implemented interface IMetadataContext is in org.apache.wicket.

  • The second type parameter to interface IMetadataContext has a warning that a non-null type argument is expected. To resolve it @NonNull can be added.

So using @NullMarked results in extra code (in some cases), a (tiny) bit of overhead, and does not prevent the need to use @NonNull.

Using @NonNull, the impact is smaller. For the 2 examples above, the changes for @NonNull would be:

• Application.sessionStoreProvider:

  • Add @NonNull to the getter.
  • Inline the getter in validateInit().

• RequestCycle: nothing.

So let's go back to my original goal: make it more pleasant to work with Wicket (primarily for Wicket users). I prefer a pragmatic approach here:

• If a value is effectively not nullable if you use Wicket correctly, mark it with @NonNull. It may be possible to get a method to return null or for a parameter to accept null in some cases, so the semantics are not exactly what the annotation is intended for. But if those situations cannot occur during regular use, why not relieve the users from having to deal with impossible situations?
• Be able to work incrementally. If somebody figures out that some value cannot be null, make it easy to create a patch without forcing them to fix more warnings related to null. Will you end up with a situation where everything is marked correctly? Most probably not, because it is very likely that annotations will only be added for variables and methods people actually use.

If it is decided that @NullMarked is the way to go, chances are I will not provide the patches. There is additional work to be done without an additional benefit over @NonNull, plus I cannot work on a sub set of variables and methods.

If the position changes to: any @NonNull for an effectively non-nullable method or variable helps our users, so let's add them if someone contributes a patch, I will likely contribute multiple patches this year.

Possibly, we will maintain the @NonNull patches ourselves, create custom builds of Wicket with different coordinates, and add a rule to Gradle to replace any Wicket dependency with our custom built one.