Nullability annotations are trivial to remove. I’ve added some basic aliases for them in main. As it stands, they’re copies of the four JSpecify annotations with those annotations applied to them along with equivalent JSR 305 meta-annotations to make the annotations function the same in existing tooling. The annotations are in log4j-api (in a new package that hasn’t been published before), and the JSR and JSpecify annotations are explicitly excluded from the OSGi and JPMS module imports to ensure they’re optional (being annotations after all). I’ve annotated a few things, but before bothering with more widespread updates, I figure we should discuss what to do there before spending a lot of time updating things. The annotation dependencies are all “provided” scope (we have some of these already there for SpotBugs-related things). If we want to reuse the same thing across components, though, then we’d likely want a separate jar for this (or to directly use JSpecify as said jar).
I personally like and use nullability annotations wherever possible in Spinnaker as we use a combination of Java, Kotlin, and other languages, and Kotlin has built-in support for various nullability annotations to bridge over to its nullable-based type system (i.e., Kotlin can differentiate between types like `List<T>?`, `List<T?>`, and `List<T?>?`, which would correspond to Java versions like `@Nullable List<T>`, `List<@Nullable T>`, and `@Nullable List<@Nullable T>` respectively). By adopting the JSpecify convention, it’s easy to mark either a package or class as `@NullUnmarked` which means everything in scope is non-null unless otherwise annotated as `@Nullable`. The branch approach to adding this is extremely likely to end up in merge hell after short order. I’ve had this happen to my own branches where I tried to incrementally add nullability annotations. What I’d suggest is ensuring everything in log4j-api is properly annotated first, then the major plugin and core APIs/SPIs, and then the rest. Then we can take advantage of the static analysis frameworks as a regular check to ensure no new NPEs are introduced. > On Jan 2, 2024, at 4:17 AM, Piotr P. Karwasz <piotr.karw...@gmail.com> wrote: > > Hi all, > > Matt made an interesting proposal to use JSpecify nullability > annotations in Log4j: > > https://github.com/apache/logging-parent/issues/88 > > I am a big fan of nullability annotations but in my professional > experience they are worthless, unless the whole team agrees to use > them and how to introduce them. > > IMHO we should: > > * create a branch that will differ from `main` only for the present > of nullability annotations, > * depend directly (scope `provided`) on JSpecify, although this might > cause issue reports like this one [1], > * only merge the changes of one module at a time (starting with `log4j-api`), > * start by marking all fields and return types as `@Nullable` and all > parameters as `@NonNull`, > * only merge the change if a tool (NullAway/Checker Framework) > guarantees that the code is NPE-free. > * target 3.1.x as the first release with nullability annotations. > > What do you think? > > Piotr > > [1] https://github.com/apache/logging-log4j2/issues/2144
