From 5f68bb50fd191fec409f5edb90755a18fce3e0ac Mon Sep 17 00:00:00 2001 From: alexkravin Date: Fri, 27 Feb 2015 13:58:13 +0400 Subject: [PATCH] Visibility Modifier Check, ignore annotated fields --- .../design/VisibilityModifierCheck.java | 172 +++++++++++++++++- .../design/VisibilityModifierCheckTest.java | 81 ++++++++- .../tools/checkstyle/AnnotatedVisibility.java | 56 ++++++ .../AnnotatedVisibilitySameTypeName.java | 11 ++ .../tools/checkstyle/LocalAnnotations.java | 8 + src/xdocs/config_design.xml | 86 ++++++++- 6 files changed, 401 insertions(+), 13 deletions(-) create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/AnnotatedVisibility.java create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/AnnotatedVisibilitySameTypeName.java create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/LocalAnnotations.java diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheck.java index 855373668..948b35c46 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheck.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheck.java @@ -31,14 +31,17 @@ import org.apache.commons.beanutils.ConversionException; import antlr.collections.AST; import com.google.common.collect.ImmutableList; +import com.puppycrawl.tools.checkstyle.api.AnnotationUtility; import com.puppycrawl.tools.checkstyle.api.Check; import com.puppycrawl.tools.checkstyle.api.DetailAST; +import com.puppycrawl.tools.checkstyle.api.FullIdent; import com.puppycrawl.tools.checkstyle.api.ScopeUtils; import com.puppycrawl.tools.checkstyle.api.TokenTypes; import com.puppycrawl.tools.checkstyle.api.Utils; /** - * Checks visibility of class members. Only static final or immutable members may be public, + * Checks visibility of class members. Only static final, immutable or annotated + * by specified annotation members may be public, * other class members must be private unless allowProtected/Package is set. *

* Public members are not flagged if the name matches the public @@ -47,7 +50,27 @@ import com.puppycrawl.tools.checkstyle.api.Utils; *

* Rationale: Enforce encapsulation. *

- * Check also has an option making it less strict: + * Check also has options making it less strict: + *

+ *

+ * ignoreAnnotationCanonicalNames - the list of annotations canonical names + * which ignore variables in consideration, if user will provide short annotation name + * that type will match to any named the same type without consideration of package, + * list by default: + *

+ *

+ *

+ * For example such public field will be skipped by default value of list above: + *

+ *

+ * + * 

 @org.junit.Rule
+ * public TemporaryFolder publicJUnitRule = new TemporaryFolder();
+ *
+ * *

*

* allowPublicImmutableFields - which allows immutable fields be @@ -159,6 +182,50 @@ import com.puppycrawl.tools.checkstyle.api.Utils; * * *

+ *

+ * To configure the Check passing fields annotated with + * 

@com.annotation.CustomAnnotation
: + *

+ *

+ * <module name="VisibilityModifier"> + * <property name="ignoreAnnotationCanonicalNames" value=" + * com.annotation.CustomAnnotation"/> + * </module> + *

+ *

+ * + * 

 @com.annotation.CustomAnnotation
+ * String customAnnotated; // No warning
+ *
+ * 
 @CustomAnnotation
+ * String shortCustomAnnotated; // No warning
+ *
+ * + *

+ *

+ * To configure the Check passing fields annotated with short annotation name + * 

@CustomAnnotation
: + *

+ *

+ * <module name="VisibilityModifier"> + * <property name="ignoreAnnotationCanonicalNames" + * value="CustomAnnotation"/> + * </module> + *

+ *

+ * + * 

 @CustomAnnotation
+ * String customAnnotated; // No warning
+ *
+ * 
 @com.annotation.CustomAnnotation
+ * String customAnnotated1; // No warning
+ *
+ * 
 @mypackage.annotation.CustomAnnotation
+ * String customAnnotatedAnotherPackage; // another package but short name matches
+ *                                       // so no violation
+ *
+ * + *

* * @author Aleksey Nesterenko */ @@ -190,6 +257,14 @@ public class VisibilityModifierCheck /** regexp for public members that should be ignored */ private Pattern publicMemberPattern = Pattern.compile(publicMemberFormat); + /** List of ignore annotations canonical names. */ + private List ignoreAnnotationCanonicalNames = + new ArrayList<>(DEFAULT_IGNORE_ANNOTATIONS); + + /** List of ignore annotations short names. */ + private List ignoreAnnotationShortNames = + getClassShortNames(DEFAULT_IGNORE_ANNOTATIONS); + /** Allows immutable fields to be declared as public. */ private boolean allowPublicImmutableFields = true; @@ -224,6 +299,12 @@ public class VisibilityModifierCheck "java.net.InetSocketAddress" ); + /** Default ignore annotations canonical names. */ + private static final List DEFAULT_IGNORE_ANNOTATIONS = ImmutableList.of( + "org.junit.Rule", + "com.google.common.annotations.VisibleForTesting" + ); + /** contains explicit access modifiers. */ private static final String[] EXPLICIT_MODS = {"public", "private", "protected"}; @@ -233,6 +314,15 @@ public class VisibilityModifierCheck return protectedAllowed; } + /** + * Set the list of ignore annotations. + * @param annotationNames array of ignore annotations canonical names. + */ + public void setIgnoreAnnotationCanonicalNames(String[] annotationNames) + { + ignoreAnnotationCanonicalNames = Arrays.asList(annotationNames); + } + /** * Set whether protected members are allowed. * @param protectedAllowed whether protected members are allowed @@ -291,7 +381,7 @@ public class VisibilityModifierCheck /** * Set the list of immutable classes types names. - * @param classNames array of immutable types short names. + * @param classNames array of immutable types canonical names. */ public void setImmutableClassCanonicalNames(String[] classNames) { @@ -321,8 +411,14 @@ public class VisibilityModifierCheck public void beginTree(DetailAST rootAst) { immutableClassShortNames.clear(); - final List shortNames = getClassShortNames(immutableClassCanonicalNames); - immutableClassShortNames.addAll(shortNames); + final List classShortNames = + getClassShortNames(immutableClassCanonicalNames); + immutableClassShortNames.addAll(classShortNames); + + ignoreAnnotationShortNames.clear(); + final List annotationShortNames = + getClassShortNames(ignoreAnnotationCanonicalNames); + ignoreAnnotationShortNames.addAll(annotationShortNames); } @Override @@ -363,7 +459,7 @@ public class VisibilityModifierCheck final boolean inInterfaceOrAnnotationBlock = ScopeUtils.inInterfaceOrAnnotationBlock(variableDef); - if (!inInterfaceOrAnnotationBlock) { + if (!inInterfaceOrAnnotationBlock && !hasIgnoreAnnotation(variableDef)) { final DetailAST varNameAST = getVarNameAST(variableDef); final String varName = varNameAST.getText(); if (!hasProperAccessModifier(variableDef, varName)) { @@ -373,6 +469,18 @@ public class VisibilityModifierCheck } } + /** + * Checks if variable def has ignore annotation. + * @param variableDef {@link TokenTypes#VARIABLE_DEF VARIABLE_DEF} + * @return true if variable def has ignore annotation. + */ + private boolean hasIgnoreAnnotation(DetailAST variableDef) + { + final DetailAST firstIgnoreAnnotation = + containsMatchingAnnotation(variableDef); + return firstIgnoreAnnotation != null; + } + /** * Checks imported type. If type's canonical name was not specified in * immutableClassCanonicalNames, but it's short name collides with one from @@ -394,6 +502,11 @@ public class VisibilityModifierCheck { immutableClassShortNames.remove(shortName); } + if (!ignoreAnnotationCanonicalNames.contains(canonicalName) + && ignoreAnnotationShortNames.contains(shortName)) + { + ignoreAnnotationShortNames.remove(shortName); + } } } @@ -651,4 +764,51 @@ public class VisibilityModifierCheck return shortClassName; } + /** + * Checks whether the AST is annotated with + * an annotation containing the passed in regular + * expression and return the AST representing that + * annotation. + * + *

+ * This method will not look for imports or package + * statements to detect the passed in annotation. + *

+ * + *

+ * To check if an AST contains a passed in annotation + * taking into account fully-qualified names + * (ex: java.lang.Override, Override) + * this method will need to be called twice. Once for each + * name given. + *

+ * + * @param variableDef {@link TokenTypes#VARIABLE_DEF variable def node}. + * @return the AST representing the first such annotation or null if + * no such annotation was found + */ + private DetailAST containsMatchingAnnotation(DetailAST variableDef) + { + DetailAST matchingAnnotation = null; + + final DetailAST holder = AnnotationUtility.getAnnotationHolder(variableDef); + + for (DetailAST child = holder.getFirstChild(); + child != null; child = child.getNextSibling()) + { + if (child.getType() == TokenTypes.ANNOTATION) { + final DetailAST at = child.getFirstChild(); + final String name = + FullIdent.createFullIdent(at.getNextSibling()).getText(); + if (ignoreAnnotationCanonicalNames.contains(name) + || ignoreAnnotationShortNames.contains(name)) + { + matchingAnnotation = child; + break; + } + } + } + + return matchingAnnotation; + } } diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheckTest.java index 2d9304232..f6446da8b 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheckTest.java +++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheckTest.java @@ -18,12 +18,13 @@ //////////////////////////////////////////////////////////////////////////////// package com.puppycrawl.tools.checkstyle.checks.design; +import static com.puppycrawl.tools.checkstyle.checks.design.VisibilityModifierCheck.MSG_KEY; + +import org.junit.Test; + import com.puppycrawl.tools.checkstyle.BaseCheckTestSupport; import com.puppycrawl.tools.checkstyle.Checker; import com.puppycrawl.tools.checkstyle.DefaultConfiguration; -import org.junit.Test; - -import static com.puppycrawl.tools.checkstyle.checks.design.VisibilityModifierCheck.MSG_KEY; public class VisibilityModifierCheckTest extends BaseCheckTestSupport @@ -180,4 +181,78 @@ public class VisibilityModifierCheckTest verify(checkConfig, getPath("InputImmutableStarImport2.java"), expected); } + @Test + public void testDefaultAnnotationPatterns() throws Exception + { + final DefaultConfiguration checkConfig = + createCheckConfig(VisibilityModifierCheck.class); + final String[] expected = { + "39:19: " + getCheckMessage(MSG_KEY, "customAnnotatedPublic"), + "42:12: " + getCheckMessage(MSG_KEY, "customAnnotatedPackage"), + "45:22: " + getCheckMessage(MSG_KEY, "customAnnotatedProtected"), + "47:19: " + getCheckMessage(MSG_KEY, "unannotatedPublic"), + "48:12: " + getCheckMessage(MSG_KEY, "unannotatedPackage"), + "49:22: " + getCheckMessage(MSG_KEY, "unannotatedProtected"), + }; + verify(checkConfig, getPath("AnnotatedVisibility.java"), expected); + } + + @Test + public void testCustomAnnotationPatterns() throws Exception + { + final DefaultConfiguration checkConfig = + createCheckConfig(VisibilityModifierCheck.class); + checkConfig.addAttribute("ignoreAnnotationCanonicalNames", + "com.puppycrawl.tools.checkstyle.AnnotatedVisibility.CustomAnnotation"); + final String[] expected = { + "15:28: " + getCheckMessage(MSG_KEY, "publicJUnitRule"), + "18:28: " + getCheckMessage(MSG_KEY, "fqPublicJUnitRule"), + "21:19: " + getCheckMessage(MSG_KEY, "googleCommonsAnnotatedPublic"), + "24:12: " + getCheckMessage(MSG_KEY, "googleCommonsAnnotatedPackage"), + "27:22: " + getCheckMessage(MSG_KEY, "googleCommonsAnnotatedProtected"), + "30:19: " + getCheckMessage(MSG_KEY, "fqGoogleCommonsAnnotatedPublic"), + "33:12: " + getCheckMessage(MSG_KEY, "fqGoogleCommonsAnnotatedPackage"), + "36:22: " + getCheckMessage(MSG_KEY, "fqGoogleCommonsAnnotatedProtected"), + "47:19: " + getCheckMessage(MSG_KEY, "unannotatedPublic"), + "48:12: " + getCheckMessage(MSG_KEY, "unannotatedPackage"), + "49:22: " + getCheckMessage(MSG_KEY, "unannotatedProtected"), + }; + verify(checkConfig, getPath("AnnotatedVisibility.java"), expected); + } + + @Test + public void testIgnoreAnnotationNoPattern() throws Exception + { + final DefaultConfiguration checkConfig = + createCheckConfig(VisibilityModifierCheck.class); + checkConfig.addAttribute("ignoreAnnotationCanonicalNames", ""); + final String[] expected = { + "15:28: " + getCheckMessage(MSG_KEY, "publicJUnitRule"), + "18:28: " + getCheckMessage(MSG_KEY, "fqPublicJUnitRule"), + "21:19: " + getCheckMessage(MSG_KEY, "googleCommonsAnnotatedPublic"), + "24:12: " + getCheckMessage(MSG_KEY, "googleCommonsAnnotatedPackage"), + "27:22: " + getCheckMessage(MSG_KEY, "googleCommonsAnnotatedProtected"), + "30:19: " + getCheckMessage(MSG_KEY, "fqGoogleCommonsAnnotatedPublic"), + "33:12: " + getCheckMessage(MSG_KEY, "fqGoogleCommonsAnnotatedPackage"), + "36:22: " + getCheckMessage(MSG_KEY, "fqGoogleCommonsAnnotatedProtected"), + "39:19: " + getCheckMessage(MSG_KEY, "customAnnotatedPublic"), + "42:12: " + getCheckMessage(MSG_KEY, "customAnnotatedPackage"), + "45:22: " + getCheckMessage(MSG_KEY, "customAnnotatedProtected"), + "47:19: " + getCheckMessage(MSG_KEY, "unannotatedPublic"), + "48:12: " + getCheckMessage(MSG_KEY, "unannotatedPackage"), + "49:22: " + getCheckMessage(MSG_KEY, "unannotatedProtected"), + }; + verify(checkConfig, getPath("AnnotatedVisibility.java"), expected); + } + + @Test + public void testIgnoreAnnotationSameName() throws Exception + { + final DefaultConfiguration checkConfig = + createCheckConfig(VisibilityModifierCheck.class); + final String[] expected = { + "10:28: " + getCheckMessage(MSG_KEY, "publicJUnitRule"), + }; + verify(checkConfig, getPath("AnnotatedVisibilitySameTypeName.java"), expected); + } } diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/AnnotatedVisibility.java b/src/test/resources/com/puppycrawl/tools/checkstyle/AnnotatedVisibility.java new file mode 100644 index 000000000..4064dd6d8 --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/AnnotatedVisibility.java @@ -0,0 +1,56 @@ +package com.puppycrawl.tools.checkstyle; + +import com.google.common.annotations.VisibleForTesting; + +import org.junit.Rule; +import org.junit.rules.TemporaryFolder; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +public class AnnotatedVisibility { + @Rule + public TemporaryFolder publicJUnitRule = new TemporaryFolder(); + + @org.junit.Rule + public TemporaryFolder fqPublicJUnitRule = new TemporaryFolder(); + + @VisibleForTesting + public String googleCommonsAnnotatedPublic; + + @VisibleForTesting + String googleCommonsAnnotatedPackage; + + @VisibleForTesting + protected String googleCommonsAnnotatedProtected; + + @com.google.common.annotations.VisibleForTesting + public String fqGoogleCommonsAnnotatedPublic; + + @com.google.common.annotations.VisibleForTesting + String fqGoogleCommonsAnnotatedPackage; + + @com.google.common.annotations.VisibleForTesting + protected String fqGoogleCommonsAnnotatedProtected; + + @CustomAnnotation + public String customAnnotatedPublic; + + @CustomAnnotation + String customAnnotatedPackage; + + @CustomAnnotation + protected String customAnnotatedProtected; + + public String unannotatedPublic; + String unannotatedPackage; + protected String unannotatedProtected; + private String unannotatedPrivate; + + @Retention(value=RetentionPolicy.RUNTIME) + @Target(value={ElementType.FIELD}) + public @interface CustomAnnotation { + } +} \ No newline at end of file diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/AnnotatedVisibilitySameTypeName.java b/src/test/resources/com/puppycrawl/tools/checkstyle/AnnotatedVisibilitySameTypeName.java new file mode 100644 index 000000000..900731836 --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/AnnotatedVisibilitySameTypeName.java @@ -0,0 +1,11 @@ +package com.puppycrawl.tools.checkstyle; + +import org.junit.rules.TemporaryFolder; + +import com.puppycrawl.tools.checkstyle.LocalAnnotations.Rule; + +public class AnnotatedVisibilitySameTypeName +{ + @Rule + public TemporaryFolder publicJUnitRule = new TemporaryFolder(); +} diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/LocalAnnotations.java b/src/test/resources/com/puppycrawl/tools/checkstyle/LocalAnnotations.java new file mode 100644 index 000000000..8ec685357 --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/LocalAnnotations.java @@ -0,0 +1,8 @@ +package com.puppycrawl.tools.checkstyle; + +public class LocalAnnotations +{ + public @interface Rule { + + } +} diff --git a/src/xdocs/config_design.xml b/src/xdocs/config_design.xml index 88559cd9a..389191940 100644 --- a/src/xdocs/config_design.xml +++ b/src/xdocs/config_design.xml @@ -13,9 +13,9 @@

- Checks visibility of class members. Only static final or immutable members - may be public; other class members must be private unless the - property protectedAllowed or packageAllowed is set. + Checks visibility of class members. Only static final, immutable or annotated + by specified annotation members may be public; other class members must be private + unless the property protectedAllowed or packageAllowed is set.

@@ -33,7 +33,12 @@ Rationale: Enforce encapsulation.

- Check also has an option making it less strict: + Check also has options making it less strict: +

+

+ ignoreAnnotationCanonicalNames - the list of annotations which ignore variables + in consideration. If user will provide short annotation name that type will match to any + named the same type without consideration of package

allowPublicImmutableFields - which allows immutable fields be declared as @@ -108,6 +113,12 @@ java.util.Locale, java.util.UUID, java.net.URL, java.net.URI, java.net.Inet4Address, java.net.Inet6Address, java.net.InetSocketAddress, + + ignoreAnnotationCanonicalNames + ignore annotations canonical names + String Set + org.junit.Rule, com.google.common.annotations.VisibleForTesting + @@ -194,6 +205,73 @@ public class ImmutableClass this.notes = notes; this.someValue = someValue; } +} + + +

+ To configure the Check passing fields annotated with @com.annotation.CustomAnnotation: +

+ +<module name="VisibilityModifier"> + <property name="ignoreAnnotationCanonicalNames" value= + "com.annotation.CustomAnnotation"/> +</module> + +

+ Example of allowed field: +

+ +class SomeClass +{ + @com.annotation.CustomAnnotation + String annotatedString; // no warning + @CustomAnnotation + String shortCustomAnnotated; // no warning +} + + +

+ To configure the Check passing fields annotated with @org.junit.Rule and + @com.google.common.annotations.VisibleForTesting annotations: +

+ +<module name="VisibilityModifier"/> + +

+ Example of allowed fields: +

+ +class SomeClass +{ + @org.junit.Rule + public TemporaryFolder publicJUnitRule = new TemporaryFolder(); // no warning + @com.google.common.annotations.VisibleForTesting + public String testString = ""; // no warning +} + + +

+ To configure the Check passing fields annotated with short annotation name: +

+ +<module name="VisibilityModifier"> + <property name="ignoreAnnotationCanonicalNames" + value="CustomAnnotation"/> +</module> + +

+ Example of allowed fields: +

+ +class SomeClass +{ + @CustomAnnotation + String customAnnotated; // no warning + @com.annotation.CustomAnnotation + String customAnnotated1; // no warning + @mypackage.annotation.CustomAnnotation + String customAnnotatedAnotherPackage; // another package but short name matches + // so no violation }