Skip to content

Commit 0488280

Browse files
wilkinsonawilkinsona
committed
Check that annotation attribute values are concise
When the value of an annotation's attribute references something defined by an inner type of the annotation, the inner type should be imported and referenced directly rather than through the containing type. Consider the following: @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) The repetition of SpringBootTest in both the annotation itself and the value of one of its attributes is overly verbose. Instead, we prefer to import WebEnvironment and refer to it directly: @SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT) This commit adds a check that enforces the style described above. In the event of there being an existing import for a different WebEnvironment type, no error will be reported as the more verbose style is necessary to avoid referencing the wrong WebEnvironment. Closes gh-452
1 parent b77b526 commit 0488280

File tree

18 files changed

+308
-2
lines changed

18 files changed

+308
-2
lines changed

‎spring-javaformat/spring-javaformat-checkstyle/src/main/java/io/spring/javaformat/checkstyle/check/SpringAnnotationAttributeConciseValueCheck.java‎

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
/*
2+
* Copyright 2017-present the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package io.spring.javaformat.checkstyle.check;
18+
19+
import java.util.ArrayList;
20+
import java.util.Collections;
21+
import java.util.List;
22+
23+
import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
24+
import com.puppycrawl.tools.checkstyle.api.DetailAST;
25+
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
26+
27+
/**
28+
* Spring-specific check of the concision of an annotation's attribute values.
29+
*
30+
* @author Andy Wilkinson
31+
*/
32+
public class SpringAnnotationAttributeConciseValueCheck extends AbstractCheck {
33+
34+
private final List<ImportStatement> imports = new ArrayList<>();
35+
36+
@Override
37+
public int[] getDefaultTokens() {
38+
return getRequiredTokens();
39+
}
40+
41+
@Override
42+
public int[] getAcceptableTokens() {
43+
return getRequiredTokens();
44+
}
45+
46+
@Override
47+
public int[] getRequiredTokens() {
48+
return new int[] { TokenTypes.ANNOTATION, TokenTypes.IMPORT };
49+
}
50+
51+
@Override
52+
public void init() {
53+
this.imports.clear();
54+
}
55+
56+
@Override
57+
public void visitToken(DetailAST ast) {
58+
if (ast.getType() == TokenTypes.IMPORT) {
59+
visitImport(ast);
60+
}
61+
else if (ast.getType() == TokenTypes.ANNOTATION) {
62+
visitAnnotation(ast);
63+
}
64+
}
65+
66+
private void visitImport(DetailAST importNode) {
67+
List<String> components = dotSeparatedComponents(importNode.getFirstChild());
68+
if (components != null) {
69+
this.imports.add(new ImportStatement(components));
70+
}
71+
}
72+
73+
private void visitAnnotation(DetailAST annotation) {
74+
int valuePairCount = annotation.getChildCount(TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR);
75+
if (valuePairCount == 0) {
76+
DetailAST valueExpression = annotation.findFirstToken(TokenTypes.EXPR);
77+
visitValueExpression(valueExpression, annotation, "value");
78+
}
79+
else {
80+
DetailAST candidate = annotation.getFirstChild();
81+
while (candidate != null) {
82+
if (candidate.getType() == TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) {
83+
visitMemberValuePair(candidate);
84+
}
85+
candidate = candidate.getNextSibling();
86+
}
87+
}
88+
}
89+
90+
private void visitMemberValuePair(DetailAST annotationValue) {
91+
DetailAST annotation = annotationValue.getParent();
92+
DetailAST attribute = annotationValue.findFirstToken(TokenTypes.IDENT);
93+
DetailAST valueExpression = annotationValue.findFirstToken(TokenTypes.EXPR);
94+
visitValueExpression(valueExpression, annotation, attribute.getText());
95+
}
96+
97+
private void visitValueExpression(DetailAST valueExpression, DetailAST annotation, String attributeName) {
98+
if (valueExpression != null && valueExpression.getChildCount() == 1) {
99+
List<String> expressionComponents = dotSeparatedComponents(valueExpression.getFirstChild());
100+
if (expressionComponents != null && expressionComponents.size() > 2) {
101+
String outerTypeName = expressionComponents.get(0);
102+
String annotationName = annotation.findFirstToken(TokenTypes.IDENT).getText();
103+
if (outerTypeName.equals(annotationName)) {
104+
String innerTypeName = expressionComponents.get(1);
105+
if (!existingClashingImport(outerTypeName, innerTypeName)) {
106+
String toImport = outerTypeName + "." + innerTypeName;
107+
String replacement = String.join(".", expressionComponents.subList(1, expressionComponents.size()));
108+
log(valueExpression.getLineNo(), valueExpression.getColumnNo(),
109+
"annotation.attribute.overlyVerboseValue", attributeName, toImport, replacement);
110+
}
111+
}
112+
}
113+
}
114+
}
115+
116+
private List<String> dotSeparatedComponents(DetailAST ast) {
117+
if (ast.getType() == TokenTypes.IDENT) {
118+
return Collections.singletonList(ast.getText());
119+
}
120+
if (ast.getType() == TokenTypes.DOT) {
121+
List<String> left = dotSeparatedComponents(ast.getFirstChild());
122+
List<String> right = dotSeparatedComponents(ast.getLastChild());
123+
if (left != null && right != null) {
124+
List<String> components = new ArrayList<>();
125+
components.addAll(left);
126+
components.addAll(right);
127+
return components;
128+
}
129+
}
130+
return null;
131+
}
132+
133+
private boolean existingClashingImport(String outer, String inner) {
134+
return this.imports.stream().filter((imported) -> imported.clashesWith(outer, inner)).findFirst().isPresent();
135+
}
136+
137+
static class ImportStatement {
138+
139+
private final String imported;
140+
141+
ImportStatement(List<String> components) {
142+
this.imported = String.join(".", components);
143+
}
144+
145+
boolean clashesWith(String outer, String inner) {
146+
return this.imported.endsWith("." + inner) && !this.imported.endsWith("." + outer + "." + inner);
147+
}
148+
149+
}
150+
151+
}

‎spring-javaformat/spring-javaformat-checkstyle/src/main/resources/io/spring/javaformat/checkstyle/check/messages.properties‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
annotation.attribute.overlyVerboseValue=Value of ''{0}'' attribute is overly verbose. Import ''{1}'' and use ''{2}'' instead.
12
annotation.location=Annotation ''{0}'' have incorrect indentation level {1}, expected level should be {2}.
23
annotation.location.alone=Annotation ''{0}'' should be alone on line.
34
catch.singleLetter=Single letter catch variable (use "ex" instead).

‎spring-javaformat/spring-javaformat-checkstyle/src/main/resources/io/spring/javaformat/checkstyle/spring-checkstyle.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
<module name="com.puppycrawl.tools.checkstyle.checks.annotation.MissingDeprecatedCheck" />
2626
<module name="com.puppycrawl.tools.checkstyle.checks.annotation.PackageAnnotationCheck" />
2727
<module name="io.spring.javaformat.checkstyle.check.SpringAnnotationLocationCheck" />
28+
<module name="io.spring.javaformat.checkstyle.check.SpringAnnotationAttributeConciseValueCheck" />
2829

2930
<!-- Block Checks -->
3031
<module name="com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck">

‎spring-javaformat/spring-javaformat-checkstyle/src/test/java/io/spring/javaformat/checkstyle/SpringConfigurationLoaderTests.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ public void loadShouldLoadChecks() {
5050
assertThat(checks).hasSize(5);
5151
TreeWalker treeWalker = (TreeWalker) checks.toArray()[4];
5252
Set<?> ordinaryChecks = (Set<?>) Extractors.byName("ordinaryChecks").extract(treeWalker);
53-
assertThat(ordinaryChecks).hasSize(61);
53+
assertThat(ordinaryChecks).hasSize(62);
5454
Set<?> commentChecks = (Set<?>) Extractors.byName("commentChecks").extract(treeWalker);
5555
assertThat(commentChecks).hasSize(6);
5656
}
@@ -64,7 +64,7 @@ public void loadWithExcludeShouldExcludeChecks() {
6464
assertThat(checks).hasSize(5);
6565
TreeWalker treeWalker = (TreeWalker) checks.toArray()[4];
6666
Set<?> ordinaryChecks = (Set<?>) Extractors.byName("ordinaryChecks").extract(treeWalker);
67-
assertThat(ordinaryChecks).hasSize(60);
67+
assertThat(ordinaryChecks).hasSize(61);
6868
Set<?> commentChecks = (Set<?>) Extractors.byName("commentChecks").extract(treeWalker);
6969
assertThat(commentChecks).hasSize(5);
7070
}

‎spring-javaformat/spring-javaformat-checkstyle/src/test/resources/check/AnnotationAttributeWithValueThatHasConciseReferenceToContainedEnumValue.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
+0 errors

‎spring-javaformat/spring-javaformat-checkstyle/src/test/resources/check/AnnotationAttributeWithValueThatHasReferenceToContainedConstant.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
+0 errors

‎spring-javaformat/spring-javaformat-checkstyle/src/test/resources/check/AnnotationAttributeWithValueThatHasVerboseReferenceToContainedEnumValue.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
+AnnotationAttributeWithValueThatHasVerboseReferenceToContainedEnumValue.java:17:84: Value of 'value' attribute is overly verbose. Import 'ConditionalOnWebApplicationType.WebApplicationType' and use 'WebApplicationType.SERVLET' instead. [SpringAnnotationAttributeConciseValue]
2+
+1 error

‎spring-javaformat/spring-javaformat-checkstyle/src/test/resources/check/AnnotationNamedAttributeWithValueThatHasConciseReferenceToContainedEnumValue.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
+0 errors

‎spring-javaformat/spring-javaformat-checkstyle/src/test/resources/check/AnnotationNamedAttributeWithValueThatHasNecessarilyVerboseReferenceToContainedEnumValue.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
+0 errors

‎spring-javaformat/spring-javaformat-checkstyle/src/test/resources/check/AnnotationNamedAttributeWithValueThatHasReferenceToContainedConstant.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
+0 errors

0 commit comments

Comments
 (0)