From ead01c2c56b7e04425cd1ff88bca6a340c634285 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Kautler?= Date: Thu, 7 Mar 2019 19:16:39 +0100 Subject: [PATCH] Issue #6463: Only handle fields for VARIABLE_DEF in AnnotationLocation check, not local variables --- config/checkstyle_checks.xml | 1 + .../annotation/AnnotationLocationCheck.java | 96 ++++--------------- .../AnnotationLocationCheckTest.java | 2 - ...AnnotationLocationSingleParameterless.java | 4 +- src/xdocs/config_annotation.xml | 32 ++----- 5 files changed, 29 insertions(+), 106 deletions(-) diff --git a/config/checkstyle_checks.xml b/config/checkstyle_checks.xml index 4f049592d33..8f78e1914f1 100644 --- a/config/checkstyle_checks.xml +++ b/config/checkstyle_checks.xml @@ -147,6 +147,7 @@ + diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationLocationCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationLocationCheck.java index 215e1311be2..2b953456245 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationLocationCheck.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationLocationCheck.java @@ -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. + *

+ *

+ * 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. *

*

* Attention: Annotations among modifiers are ignored (looks like false-negative) * as there might be a problem with annotations for return types: *

- * 
public @Nullable Long getStartTimeOrNull() { ... }
+ * 
+ * public @Nullable Long getStartTimeOrNull() { ... }
+ *
*

* Such annotations are better to keep close to type. * Due to limitations, Checkstyle can not examine the target of an annotation. @@ -140,34 +147,6 @@ * <property name="allowSamelineParameterizedAnnotation" value="true"/> * </module> * - *

- * The following example demonstrates how the check validates annotations of - * for-each and for-loop variable definitions. - *

- *

- * Configuration: - *

- * 
- * <module name="AnnotationLocation">
- *   <property name="allowSamelineMultipleAnnotations" value="false"/>
- *   <property name="allowSamelineSingleParameterlessAnnotation"
- *     value="false"/>
- *   <property name="allowSamelineParameterizedAnnotation" value="false"/>
- *   <property name="tokens" value="VARIABLE_DEF"/>
- *  </module>
- *
- *

- * Code example: - *

- * 
- * public void test(String s) {
- *   ...
- *   for (@MyAnnotation char c : s.toCharArray()) { ... }  // OK
- *   ...
- *   for (@MyAnnotation int i = 0; i < 10; i++) { ... } // OK
- *   ...
- * }
- *
* * @since 6.0 */ @@ -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. @@ -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)); } /** @@ -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. @@ -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); } /** @@ -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; - } - } diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationLocationCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationLocationCheckTest.java index 70207c1ff39..f47aa3231f0 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationLocationCheckTest.java +++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationLocationCheckTest.java @@ -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); } diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/annotation/annotationlocation/InputAnnotationLocationSingleParameterless.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/annotation/annotationlocation/InputAnnotationLocationSingleParameterless.java index 1a88f2816a8..6bd5a660023 100644 --- a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/annotation/annotationlocation/InputAnnotationLocationSingleParameterless.java +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/annotation/annotationlocation/InputAnnotationLocationSingleParameterless.java @@ -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) diff --git a/src/xdocs/config_annotation.xml b/src/xdocs/config_annotation.xml index 0fe156565f8..78c278f5262 100644 --- a/src/xdocs/config_annotation.xml +++ b/src/xdocs/config_annotation.xml @@ -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. +

+

+ Attention: Elements that cannot have JavaDoc comments like local variables are not in the + scope of this check even though a token type like VARIABLE_DEF would match + them.

Attention: Annotations among modifiers are ignored (looks like false-negative) @@ -186,30 +192,6 @@ public String getNameIfPresent() { ... } <property name="allowSamelineParameterizedAnnotation" value="true"/> </module> -

- The following example demonstrates how the check validates annotations of - for-each and for-loop variable definitions. -

-

Configuration:

- -<module name="AnnotationLocation"> - <property name="allowSamelineMultipleAnnotations" value="false"/> - <property name="allowSamelineSingleParameterlessAnnotation" - value="false"/> - <property name="allowSamelineParameterizedAnnotation" value="false"/> - <property name="tokens" value="VARIABLE_DEF"/> - </module> - -

Code example:

- -public void test(String s) { - ... - for (@MyAnnotation char c : s.toCharArray()) { ... } // OK - ... - for (@MyAnnotation int i = 0; i < 10; i++) { ... } // OK - ... -} -