Skip to content

Commit

Permalink
Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
Browse files Browse the repository at this point in the history 
…ionLocation check, not local variables
  • Loading branch information
@Vampire
Vampire committed May 15, 2019
1 parent bb6d12f commit ead01c2
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 106 deletions.
1 change: 1 addition & 0 deletions config/checkstyle_checks.xml
View file
Expand Up @@ -147,6 +147,7 @@
<property name="tokens" value="ANNOTATION_FIELD_DEF"/>
<property name="tokens" value="PACKAGE_DEF"/>
<property name="tokens" value="ENUM_CONSTANT_DEF"/>
<property name="tokens" value="VARIABLE_DEF"/>
<property name="allowSamelineSingleParameterlessAnnotation" value="false"/>
</module>
<module name="AnnotationOnSameLine">
Expand Down
96 changes: 19 additions & 77 deletions src/main/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationLocationCheck.java
View file
Expand Up @@ -31,13 +31,20 @@
* Check location of annotation on language elements.
* By default, Check enforce to locate annotations immediately after
* documentation block and before target element, annotation should be located
* on separate line from target element.
* on separate line from target element. This check also verifies that the annotations
* are on the same indenting level as the annotated element if they are not on the same line.
* </p>
* <p>
* Attention: Elements that cannot have JavaDoc comments like local variables are not in the
* scope of this check even though a token type like {@code VARIABLE_DEF} would match them.
* </p>
* <p>
* Attention: Annotations among modifiers are ignored (looks like false-negative)
* as there might be a problem with annotations for return types:
* </p>
* <pre>public @Nullable Long getStartTimeOrNull() { ... }</pre>
* <pre>
* public @Nullable Long getStartTimeOrNull() { ... }
* </pre>
* <p>
* Such annotations are better to keep close to type.
* Due to limitations, Checkstyle can not examine the target of an annotation.
Expand Down Expand Up @@ -140,34 +147,6 @@
* &lt;property name=&quot;allowSamelineParameterizedAnnotation&quot; value=&quot;true&quot;/&gt;
* &lt;/module&gt;
* </pre>
* <p>
* The following example demonstrates how the check validates annotations of
* for-each and for-loop variable definitions.
* </p>
* <p>
* Configuration:
* </p>
* <pre>
* &lt;module name=&quot;AnnotationLocation&quot;&gt;
* &lt;property name=&quot;allowSamelineMultipleAnnotations&quot; value=&quot;false&quot;/&gt;
* &lt;property name=&quot;allowSamelineSingleParameterlessAnnotation&quot;
* value=&quot;false&quot;/&gt;
* &lt;property name=&quot;allowSamelineParameterizedAnnotation&quot; value=&quot;false&quot;/&gt;
* &lt;property name=&quot;tokens&quot; value=&quot;VARIABLE_DEF&quot;/&gt;
* &lt;/module&gt;
* </pre>
* <p>
* Code example:
* </p>
* <pre>
* public void test(String s) {
* ...
* for (&#64;MyAnnotation char c : s.toCharArray()) { ... } // OK
* ...
* for (&#64;MyAnnotation int i = 0; i &lt; 10; i++) { ... } // OK
* ...
* }
* </pre>
*
* @since 6.0
*/
Expand All @@ -186,10 +165,6 @@ public class AnnotationLocationCheck extends AbstractCheck {
*/
public static final String MSG_KEY_ANNOTATION_LOCATION = "annotation.location";

/** Array of single line annotation parents. */
private static final int[] SINGLELINE_ANNOTATION_PARENTS = {TokenTypes.FOR_EACH_CLAUSE,
TokenTypes.FOR_INIT, };

/**
* Allow single parameterless annotation to be located on the same line as
* target element.
Expand Down Expand Up @@ -272,11 +247,15 @@ public int[] getRequiredTokens() {

@Override
public void visitToken(DetailAST ast) {
DetailAST node = ast.findFirstToken(TokenTypes.MODIFIERS);
if (node == null) {
node = ast.findFirstToken(TokenTypes.ANNOTATIONS);
// ignore variable def tokens that are not field definitions
if (ast.getType() != TokenTypes.VARIABLE_DEF
|| ast.getParent().getType() == TokenTypes.OBJBLOCK) {
DetailAST node = ast.findFirstToken(TokenTypes.MODIFIERS);
if (node == null) {
node = ast.findFirstToken(TokenTypes.ANNOTATIONS);
}
checkAnnotations(node, getExpectedAnnotationIndentation(node));
}
checkAnnotations(node, getExpectedAnnotationIndentation(node));
}

/**
Expand Down Expand Up @@ -348,8 +327,7 @@ private static String getAnnotationName(DetailAST annotation) {
* the value of {@link AnnotationLocationCheck#allowSamelineParameterizedAnnotation};
* 2) checks parameterless annotation location considering
* the value of {@link AnnotationLocationCheck#allowSamelineSingleParameterlessAnnotation};
* 3) checks annotation location considering the elements
* of {@link AnnotationLocationCheck#SINGLELINE_ANNOTATION_PARENTS};
* 3) checks annotation location;
* @param annotation annotation node.
* @param hasParams whether an annotation has parameters.
* @return true if the annotation has a correct location.
Expand All @@ -365,8 +343,7 @@ private boolean isCorrectLocation(DetailAST annotation, boolean hasParams) {
}
return allowSamelineMultipleAnnotations
|| allowingCondition && !hasNodeBefore(annotation)
|| !allowingCondition && (!hasNodeBeside(annotation)
|| isAllowedPosition(annotation, SINGLELINE_ANNOTATION_PARENTS));
|| !hasNodeBeside(annotation);
}

/**
Expand Down Expand Up @@ -406,39 +383,4 @@ private static boolean hasNodeAfter(DetailAST annotation) {
return annotationLineNo == nextNode.getLineNo();
}

/**
* Checks whether position of annotation is allowed.
* @param annotation annotation token.
* @param allowedPositions an array of allowed annotation positions.
* @return true if position of annotation is allowed.
*/
private static boolean isAllowedPosition(DetailAST annotation, int... allowedPositions) {
boolean allowed = false;
for (int position : allowedPositions) {
if (isInSpecificCodeBlock(annotation, position)) {
allowed = true;
break;
}
}
return allowed;
}

/**
* Checks whether the scope of a node is restricted to a specific code block.
* @param node node.
* @param blockType block type.
* @return true if the scope of a node is restricted to a specific code block.
*/
private static boolean isInSpecificCodeBlock(DetailAST node, int blockType) {
boolean returnValue = false;
for (DetailAST token = node.getParent(); token != null; token = token.getParent()) {
final int type = token.getType();
if (type == blockType) {
returnValue = true;
break;
}
}
return returnValue;
}

}
2 changes: 0 additions & 2 deletions ...t/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationLocationCheckTest.java
View file
Expand Up @@ -306,8 +306,6 @@ public void testAnnotationSingleParameterless() throws Exception {
"23: " + getCheckMessage(MSG_KEY_ANNOTATION_LOCATION_ALONE, "Annotation"),
"25: " + getCheckMessage(MSG_KEY_ANNOTATION_LOCATION_ALONE, "Annotation"),
"27: " + getCheckMessage(MSG_KEY_ANNOTATION_LOCATION_ALONE, "Annotation"),
"31: " + getCheckMessage(MSG_KEY_ANNOTATION_LOCATION_ALONE, "Annotation"),
"33: " + getCheckMessage(MSG_KEY_ANNOTATION_LOCATION_ALONE, "Annotation"),
};
verify(checkConfig, getPath("InputAnnotationLocationSingleParameterless.java"), expected);
}
Expand Down
4 changes: 2 additions & 2 deletions ...tyle/checks/annotation/annotationlocation/InputAnnotationLocationSingleParameterless.java
View file
Expand Up @@ -28,9 +28,9 @@ class InputAnnotationLocationSingleParameterless {

void parameterlessSamelineInForEach() {
for (@Annotation Object o : new Object[0]) break; //ok
for (@Annotation @Annotation Object o : new Object[0]) break; //warn
for (@Annotation @Annotation Object o : new Object[0]) break; //ok
for (@Annotation Object o;;) break; // ok
for (@Annotation @Annotation Object o;;) break; //warn
for (@Annotation @Annotation Object o;;) break; //ok
}

@Repeatable(Annotations.class)
Expand Down
32 changes: 7 additions & 25 deletions src/xdocs/config_annotation.xml
View file
Expand Up @@ -29,7 +29,13 @@
Check location of annotation on language elements.
By default, Check enforce to locate annotations immediately after documentation block
and before target element, annotation should be located on separate line from target
element.
element. This check also verifies that the annotations are on the same indenting level as
the annotated element if they are not on the same line.
</p>
<p>
Attention: Elements that cannot have JavaDoc comments like local variables are not in the
scope of this check even though a token type like <code>VARIABLE_DEF</code> would match
them.
</p>
<p>
Attention: Annotations among modifiers are ignored (looks like false-negative)
Expand Down Expand Up @@ -186,30 +192,6 @@ public String getNameIfPresent() { ... }
&lt;property name=&quot;allowSamelineParameterizedAnnotation&quot; value=&quot;true&quot;/&gt;
&lt;/module&gt;
</source>
<p>
The following example demonstrates how the check validates annotations of
for-each and for-loop variable definitions.
</p>
<p>Configuration:</p>
<source>
&lt;module name=&quot;AnnotationLocation&quot;&gt;
&lt;property name=&quot;allowSamelineMultipleAnnotations&quot; value=&quot;false&quot;/&gt;
&lt;property name=&quot;allowSamelineSingleParameterlessAnnotation&quot;
value=&quot;false&quot;/&gt;
&lt;property name=&quot;allowSamelineParameterizedAnnotation&quot; value=&quot;false&quot;/&gt;
&lt;property name=&quot;tokens&quot; value=&quot;VARIABLE_DEF&quot;/&gt;
&lt;/module&gt;
</source>
<p>Code example:</p>
<source>
public void test(String s) {
...
for (&#64;MyAnnotation char c : s.toCharArray()) { ... } // OK
...
for (&#64;MyAnnotation int i = 0; i &lt; 10; i++) { ... } // OK
...
}
</source>
</subsection>

<subsection name="Example of Usage" id="AnnotationLocation_Example_of_Usage">
Expand Down

0 comments on commit ead01c2

Please sign in to comment.