checkstyle
Give more clarity on xdocs for AnnotationLocation
Link: https://checkstyle.sourceforge.io/checks/annotation/annotationlocation.html
I am quoting the documentation for AnnotationLocation below to highlight the perceived inconsistency.
Use the following configuration to allow multiple annotations on the same line:
<module name="AnnotationLocation">
<property name="allowSamelineMultipleAnnotations" value="true"/>
<property name="allowSamelineSingleParameterlessAnnotation"
value="false"/>
<property name="allowSamelineParameterizedAnnotation" value="false"/>
</module>
Example to allow any location multiple annotations:
@NotNull private boolean field1; //ok
@Override public int hashCode() { return 1; } //ok
@NotNull //ok
private boolean field2;
@Override //ok
public boolean equals(Object obj) { return true; }
@Mock DataLoader loader; //ok
@SuppressWarnings("deprecation") DataLoader loader; //ok
@SuppressWarnings("deprecation") public int foo() { return 1; } //ok
@NotNull @Mock DataLoader loader; //ok
Use the following configuration to allow only one and only parameterized annotation on the same line:
<module name="AnnotationLocation">
<property name="allowSamelineMultipleAnnotations" value="false"/>
<property name="allowSamelineSingleParameterlessAnnotation"
value="false"/>
<property name="allowSamelineParameterizedAnnotation" value="true"/>
</module>
Example to allow only one and only parameterized annotation on the same line:
@NotNull private boolean field1; //violation
@Override public int hashCode() { return 1; } //violation
@NotNull //ok
private boolean field2;
@Override //ok
public boolean equals(Object obj) { return true; }
@Mock DataLoader loader; //violation
@SuppressWarnings("deprecation") DataLoader loader; //ok
@SuppressWarnings("deprecation") public int foo() { return 1; } //ok
@NotNull @Mock DataLoader loader; //violation
end of doc quote.
But, the below config line which is same for above examples mentioned, however presents inconsistent scenarios.
<property name="allowSamelineSingleParameterlessAnnotation" value="false"/>
For "Example to allow any location multiple annotations":
@Override public int hashCode() { return 1; } //ok
For "Example to allow only one and only parameterized annotation on the same line":
@Override public int hashCode() { return 1; } //violation
Upon insepcting the codebase for AnnotationLocationCheck.java, I came across the constant and comments i.e.
/**
* Allow annotation(s) to be located on the same line as
* target element.
*/
private boolean allowSamelineMultipleAnnotations;
And, when i played around the AnnotationLocationExamplesTest's method testExample2(), I realised that this config shadows other configs like allowSamelineMultipleAnnotations and allowSamelineSingleParameterlessAnnotation.
As the comments suggest that it can allow annotation/annotations (also hold true for single annotation) on same line, it ignores even if the allowSamelineMultipleAnnotations and allowSamelineSingleParameterlessAnnotation are set to false in the below module config.
<module name="AnnotationLocation">
<property name="allowSamelineMultipleAnnotations" value="false"/>
<property name="allowSamelineSingleParameterlessAnnotation"
value="false"/>
<property name="allowSamelineParameterizedAnnotation" value="true"/>
</module>
Therefore, it fails to register @Override public int hashCode() { return 1; } as a violation. But, if i set allowSamelineMultipleAnnotations to false (visualised below), it recognises the above code as a violation.
<module name="AnnotationLocation">
<property name="allowSamelineMultipleAnnotations" value="false"/>
<property name="allowSamelineSingleParameterlessAnnotation"
value="false"/>
<property name="allowSamelineParameterizedAnnotation" value="false"/>
</module>
Hence, if i have understood the issue right, I sugges to have allowSamelineParameterizedAnnotation strictly for multiple annotations otherwise in special cases as above, other configs become redundant.
Confusion is due to fact that allowSamelineMultipleAnnotations is overlapping two other properties.
So any true from property is prevailing, so if code is allowed by some property, no violation.
Looks like when we created such Check we found that people completely ok to have single annotation on same line, but if there are two they prefer to keep them on separate lines. That is why we have allowSamelineMultipleAnnotations that affects all others properties.
allowSamelineParameterizedAnnotation supposed to be named allowSamelineSingleParameterizedAnnotation.
It is too late to change a name, as it is breaking changes. And we have in description of property:
allowSamelineParameterizedAnnotation Allow one and only parameterized annotation to be located on the same line as target element.
I reviewed the file changes, and it appears the PR only updates the documentation.
The main logic hasn't been altered in AnnotationLocationCheck.java. I am on it.
Logic is not expected to be changed, only documentation
Thanks for the clarification, that makes sense. I'm gonna make the documentation clearer 👍
I have opened a pr which fixes this issue and done changes in the documentation required but the some CI runs fails. I dont know why. Maybe you can guide if i am doing something wrong.
