jstuyts commented on PR #1093:
URL: https://github.com/apache/wicket/pull/1093#issuecomment-2650218140
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
