Define best practice regarding position of @NonNull annotations

[mxtartaglia-sl]

Review comment on PR:

Originally posted by @hendrikebbers in #10332 (comment)

[mxtartaglia-sl]

Regarding the position of NonNull, Nullable (and other type) annotation

Context

We detected an inconsistent pattern during recent code reviews when using annotations and the final keyword, especially for method parameters. Our style includes both of the following patterns:

String example(final @NonNull BigDecimal bigDecimal) {...}

String example(@NonNull final BigDecimal bigDecimal) {...}

This is also present:

@NonNull
final String prefix;

public final @NonNull Distributor distributor;

We can discover the same inconsistency in the use of @Nullable, too.

@Nullable
static List<String> createList(@Nullable final String rawValue) {...}

@Override
public void commit(final @Nullable SigImpactHistorian observer) {...}

Our hedera code style is not explicit for this case, but it does mention:

4.8.5 Annotations

4.8.5.1 Type-use annotations

Type-use annotations appear immediately before the annotated type. An annotation is a type-use annotation if it is meta-annotated with @Target(ElementType.TYPE_USE). Example:

final @Nullable String name;

public @Nullable Person getPersonByName(String name);

4.8.5.4 Field annotations

Annotations applying to a field also appear immediately after the documentation block, but in this case, multiple annotations (possibly parameterized) may be listed on the same line; for example:

@Partial @Mock DataLoader loader;

4.8.5.5 Parameter and local variable annotations

There are no specific rules for formatting annotations on parameters or local variables (except, of course, when the annotation is a type-use annotation).

Following the idea that the declaration statement for a field and the declaration of a method parameter should be consistent, we could infer that the suggested approach regarding method parameters is:

private final @NonNull BigDecimal bigDecimal;

String example(final @NonNull BigDecimal bigDecimal) {...}

It is worth mentioning that spotbugs @NonNull annotation would not be a Type annotation (it does not declare ElementType.TYPE_USE), and @Nullable isn't either.
But IMO, they behave as such, so the hedera style should apply.

Things to consider

Our code-base

As mentioned above, we have occurrences for both styles, but there is a remarkable preference towards the pattern: (?:private\s+)?+@\w+\s*+(?:private\s+)?\bfinal\b\s+\w+\s+\w (private @Annotation final Type varName or @Annotation private final Type varName or @Annotation final Type varName) of about 10400+ occurrences of that convention against 270+ for (?:private\s+)?+\bfinal\b\s+@\w+\w+\s+\w+\s*

IntelliJ inconsistency

Wrong order of method parameter annotations vs. final modifier
When intelliJ is configured to automatically add final keyword to generated fields and parameters (Settings/Preferences -> Editor -> Code Style -> Java -> Code Generation and enable "Make generated local variables final" and "Make generated parameters final" in the "Final modifier") is known to perform these operations using different styles in each case.

For example, Given a Type annotation

creating a constructor for a field annotated with @TypeAnnotation.

Uses the style @Annotation final Type name
While implementing an interface that receives an annotated parameter

Generates:

Given that @NonNull is not a Type annotation per se, the generated code for that case is always the same and follows the pattern @Annotation final Type varName.

Spotless

formatAnnotations()

Spotless does not currently reorder annotations. It does not move annotations around modifiers. Spotless also does not reorder annotations of a particular variety (say, putting all declaration annotations in a canonical order, such as alphabetical order.
Still, as it is configured in hedera, even if the annotation is a type annotation and there are no modifiers in between, for fields, it will force annotations on a new line.

One can use formatAnnotations spotless/plugin-gradle/README.md at main to enforce the rule "type annotations should appear immediately before the type they annotate" but only when writing the annotations in the correct places (before/after modifiers such as public/final)
For example:

 @Override
 @Deprecated
 @Nullable
 @Interned
 String s;

It will be formatted as follows:

@Override
@Deprecated
@Nullable @Interned String s;

The final modifier can't be reordered despite adding NonNull to recognized type annotations (that are not automatically recognized by formatAnnotations).

A word from other tools (checker framework)

The Checker Framework Manual: Custom pluggable types for Java

When possible, you should use type annotations rather than declaration annotations.
Users who are using old Java 5–7 declaration annotations (for instance, from the FindBugs tool) can use annotations in package org.checkerframework.checker.nullness.compatqual to avoid name conflicts. Once the users are ready to upgrade to Java 8+ type annotations, those compatibility annotations are no longer necessary.

How should type annotations be formatted in source code? Where should I write type annotations?
The Java Language Specification says: "It is customary, though not required, to write declaration annotations before all other modifiers, and type annotations immediately before the type to which they apply."
A type annotation should be written immediately before the type it qualifies, on the same line. There should not be any modifiers (such as public) between them. This emphasizes that the type qualifier plus the Java base type is a logical unit, which aids people reading the code.
// Wrong formatting
@Positive
public int numItems;
// Right
public @Positive int numItems;

// Wrong formatting
@Nullable
public Object getThing() {
...
}

// Right
public @Nullable Object getThing() {
...
}

By contrast, declaration annotations are conventionally written on their own line.

// Wrong formatting
@Deprecated URL toURL() {
...
}

// Right
@Deprecated
URL toURL() {
...
}

The preferred order of modifiers is:

1. Declaration annotations
2. Keyword modifiers such as public and strictfp
3. Type annotations
4. Type

@Deprecated
public @Interned class Level {
...
}

Conclusion

It's a gray area. Even though Palantir, Google, and Headera code-style documents expressed in a way what is the desired style when using type annotations, the NonNull and Nullable annotations we are using from Spotbugs ( which is known not to have support for type annotations) are masked declaration annotations. We don't have any other custom type annotations.
Our code shows mixed patterns, but the preferred patterns lean toward the opposite direction to what is suggested.
Spotless can't be used to reorder or enforce the annotation position if modifiers are between the annotated type and the annotation. Even IntelliJ is known to have these patterns mixed up when generating code.

A possible solution to enforce the rule is to implement our spotless formatting step that would apply the desired style. We already implemented a custom step for handling the license: StripOldLicenseFormatterStep; we might have to deal with parsing issues and multiple line cases that we might be unable to solve.

References

Type Annotations and Pluggable Type Systems (The Java™ Tutorials > Learning the Java Language > Annotations)
formatAnnotations() contradicts Google Java style · Issue #1366 · diffplug/spotless · GitHub
Nonnull annotations on final field to satisfy FindBugs and IntelliJ IDEA - Stack Overflow
Type Annotations and Pluggable Type Systems (The Java™ Tutorials > Learning the Java Language > Annotations)
https://help.eclipse.org/latest/topic/org.eclipse.jdt.doc.user/tasks/task-using_null_type_annotations.htm?cp=1_3_9_1
The Checker Framework Manual: Custom pluggable types for Java

[jsync-swirlds]

For what it's worth, the modular services code has been (informally at least) preferring final @NonNull Type variable style (that is, final always comes before the annotation, if both are present). I don't think I've seen a PR with the order reversed in some months.

[mxtartaglia-sl]

After a conversation with @rbair23 the following was defined:

Both formats seem acceptable. If we want to pick one, we'll choose the most prevalent pattern and update Spotless accordingly so it will rearrange the text automatically to be the correct format.

There is no strong motivation to force only one pattern, so closing the ticket and leaving it for future reference.