Inconsistent annotation formatting

[talios]

I was wondering if anyone could explain the inconsistent formatting of annotations I see here between an abstract class member and a private field:

-    @Value.Parameter
-    abstract int page();
+  @Value.Parameter
+  abstract int page();

and

-  @Reference
-  private SomeService someService;
+  @Reference private SomeService someService;

The style guide mentions that multiple annotations may go on a single line - but my reading would be that it could imply "the annotations remain on a separate line to the field, but the annotations may be wrapped onto one line".

It further says there's no specific rules about types, parameters and local variables.

For a measure of consistency - I think sticking to separate lines would be better, except for parameters - which already often wrap to their own line.

Thoughts? What are the rules which the formatter currently uses?

[tbroyer]

See fieldAnnotationDirection in the code: it currently uses horizontal direction if all annotations are marker annotations. @Value.Parameter could have arguments, so it triggers vertical annotation direction.

[JakeWharton]

Somewhat related: #342.

[cushon]

Right, some of the discussion in #342 applies. The style guide doesn't have a lot of hard-and-fast rules about annotations, and we want the formatter to make consistent decisions that generally ignore existing formatting.

I've paged out what went in to this specific discussion, but we usually prioritized consistency over saving vertical space, and this seems counter to that. It's possible that we found there was a lot of existing code where the marker annotations were on the same line as the rest of a field declaration, and we wanted to avoid churn.

If we wanted to revisit this, I'd try making the change and see how much existing code is affected.

[talios]

Just following up on this as it looks like both linked issues were just kinda left hanging. Looking at the code related to fieldAnnotationDirection to me, it seems the implementation doesn't really match the comment associated with it:

/**
 * Should a field with a set of modifiers be declared with horizontal annotations? This is
 * currently true if all annotations are marker annotations.
 */

The comment talks about marker annotations, but the code deals with annotation instances since getAnnotations is returning the expression tree of the code use, thus:

@Inject
@Inject(true)

would yield the first as a marker, the second not, and then you'd when up with formatted code like:

@Inject SomeThing something;

@Inject(true)
SomethingOptionalThing someOptionalThing;

which is.... inconsistent.

Should not fieldAnnotationDirection look at the class definition of the annotations in question, so seeing what arguments are available. That way - @tbroyer's comment above would be correct for all annotations, I suspect it works for @Value.Parameter in my instance, because the annotation has default values, which I assume may be being returned from getArguments?

[talios]

Any comments above @cushon - seems consistency isn't really being applied here. Or at least, the wrong consistency.

[cushon]

The comment should say 'parameterless' annotation, not 'marker' annotation, I'll fix it.

For fields, the style guide also talks about parameterless annotations: "multiple annotations (possibly parameterized) may be listed on the same line".

One consideration here is that field declarations are usually shorter than method signatures, so keeping the annotations in-line saves some vertical space without hurting readability. It also maintains consistency between field declarations, even if it's sacrificing some global consistency between annotations in general.

If we revisit this, we'd want to do a survey of field declarations that would be affected by the change, and see if keeping the annotations one-per-line looked like an improvement.

[talios]

So we're closing, and leaving the inconsistent formatting? Lovely.

so keeping the annotations in-line saves some vertical space without hurting readability

But this doesn't keep them inline. In one source file the same annotation will be inlined, or not inlined, based on its usage - that's... inconsistent.

[cushon]

based on its usage

By usage, are you referring to whether or not it's a parameterized annotation?

[talios]

By usage, are you referring to whether or not it's a parameterized annotation?

Yes. Going back to my original example:

@Inject SomeThing something;

@Inject(true)
SomethingOptionalThing someOptionalThing;

"a parameterized annotation" to my mind, talks about the parameterization of the annotation itself - CAN it be parameterized, not "a parameterized invocation/usage.

In the above, depending on how its used you end up with inconsistent line breaks for the new @Inject usages, if one was making a commit change from one to the other, rather than a simple 1 line change, we've now got 2 lines.

I was thinking of submitting a PR with a change to look at the available parameter count on an annotation to make that choice, which would yield consistency for a given annotation.

[lowasser]

@talios, https://github.com/google/google-java-format/wiki/FAQ#what-do-you-mean-its-not-a-compiler would seem to imply that it is not possible to look up the annotation to determine its parameter count.

[cushon]

Right, as @lowasser says we don't have a way to look at the declaration of an annotation. There's related discussion in #5, where we'd like to make a distinction between type and declaration annotations.

If we always formatted annotations on fields one-per-line that would be consistent with methods, but inconsistent with parameters and locals. Minimizing the lines changed (e.g. when replacing @Inject with @Inject(true)) is also consideration, but not the only one, and there are trade-offs.

I prototyped the change to always format annotations one-per-line, and for the examples I surveyed it doesn't seem like an improvement--the field takes up more vertical space, and there weren't nearby fields with different parameterizations of the same annotation that were made more consistent.

Do you have additional perspective or data (such as real-world examples of code that would be affected by the change) that suggests it would be an improvement overall?

[Stephan202]

Another annotation for which the effect of the current implementation is visible is Spring Boot's @MockBean, which optionally accepts a Mockito answer. Anonymized example from an internal code base (the non-anonmized line stays under 100 characters):

@MockBean private SpringBean1 springBean1;
@MockBean private SpringBean2 springBean2;
@MockBean private SpringBean3 springBean3;
@MockBean private SpringBean4 springBean4;

@MockBean(answer = RETURNS_MOCKS)
private SpringBean5 springBean5;

@MockBean private SpringBean6 springBean6;
@MockBean private SpringBean7 springBean7;
@Autowired private SpringBean8 springBean8;

In some other cases we have several beans that require a custom answer, amplifying the effect. In the extreme case a list such as the one above would take up 7 * 4 = 28 lines. In my experience this makes it significantly harder to parse those lines (subjectively, the result appears more "chaotic" than it really is).

That said, I don't have many concrete improvement suggestions. One thing I do suspect will improve the situation is to not enforce the extra two empty lines around multi-line statements. I suspect that for field and variable declarations a compact representation works well enough, as in the "99% case" their complexity is very limited.