TouchInstinct
/
checkstyle
307
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Visibility Modifier Check, ignore annotated fields

This commit is contained in:
alexkravin 2015-02-27 13:58:13 +04:00 committed by Roman Ivanov
parent 244eb9d0a9
commit 5f68bb50fd
6 changed files with 401 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

172
src/main/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheck.java
View File

@ -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.
* <p>
* Public members are not flagged if the name matches the public
@ -47,7 +50,27 @@ import com.puppycrawl.tools.checkstyle.api.Utils;
* </p>
* Rationale: Enforce encapsulation.
* <p>
* Check also has an option making it less strict:
* Check also has options making it less strict:
* </p>
* <p>
* <b>ignoreAnnotationCanonicalNames</b> - 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:
* <ul>
* <li>org.junit.Rule</li>
* <li>com.google.common.annotations.VisibleForTesting</li>
* </ul>
* </p>
* <p>
* For example such public field will be skipped by default value of list above:
* </p>
* <p>
* <code>
* <pre> @org.junit.Rule
* public TemporaryFolder publicJUnitRule = new TemporaryFolder();
* </pre>
* </code>
* </p>
* <p>
* <b>allowPublicImmutableFields</b> - which allows immutable fields be
@ -159,6 +182,50 @@ import com.puppycrawl.tools.checkstyle.api.Utils;
* </code>
* </pre>
* </p>
* <p>
* To configure the Check passing fields annotated with
* <pre>@com.annotation.CustomAnnotation</pre>:
* </p>
* <p>
* &lt;module name=&quot;VisibilityModifier&quot;&gt;
* &lt;property name=&quot;ignoreAnnotationCanonicalNames&quot; value=&quot;
* com.annotation.CustomAnnotation&quot;/&gt;
* &lt;/module&gt;
* </p>
* <p>
* <code>
* <pre> @com.annotation.CustomAnnotation
* String customAnnotated; // No warning
* </pre>
* <pre> @CustomAnnotation
* String shortCustomAnnotated; // No warning
* </pre>
* </code>
* </p>
* <p>
* To configure the Check passing fields annotated with short annotation name
* <pre>@CustomAnnotation</pre>:
* </p>
* <p>
* &lt;module name=&quot;VisibilityModifier&quot;&gt;
* &lt;property name=&quot;ignoreAnnotationCanonicalNames&quot;
* value=&quot;CustomAnnotation&quot;/&gt;
* &lt;/module&gt;
* </p>
* <p>
* <code>
* <pre> @CustomAnnotation
* String customAnnotated; // No warning
* </pre>
* <pre> @com.annotation.CustomAnnotation
* String customAnnotated1; // No warning
* </pre>
* <pre> @mypackage.annotation.CustomAnnotation
* String customAnnotatedAnotherPackage; // another package but short name matches
* // so no violation
* </pre>
* </code>
* </p>
*
* @author <a href="mailto:nesterenko-aleksey@list.ru">Aleksey Nesterenko</a>
*/
@ -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<String> ignoreAnnotationCanonicalNames =
new ArrayList<>(DEFAULT_IGNORE_ANNOTATIONS);
/** List of ignore annotations short names. */
private List<String> 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<String> 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<String> shortNames = getClassShortNames(immutableClassCanonicalNames);
immutableClassShortNames.addAll(shortNames);
final List<String> classShortNames =
getClassShortNames(immutableClassCanonicalNames);
immutableClassShortNames.addAll(classShortNames);
ignoreAnnotationShortNames.clear();
final List<String> 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
* <b>immutableClassCanonicalNames</b>, 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.
*
* <p>
* This method will not look for imports or package
* statements to detect the passed in annotation.
* </p>
*
* <p>
* 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.
* </p>
*
* @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;
}
}

81
src/test/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheckTest.java
View File

@ -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);
}
}

56
src/test/resources/com/puppycrawl/tools/checkstyle/AnnotatedVisibility.java Normal file
View File

@ -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 {
}
}

11
src/test/resources/com/puppycrawl/tools/checkstyle/AnnotatedVisibilitySameTypeName.java Normal file
View File

@ -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();
}

8
src/test/resources/com/puppycrawl/tools/checkstyle/LocalAnnotations.java Normal file
View File

@ -0,0 +1,8 @@
package com.puppycrawl.tools.checkstyle;
public class LocalAnnotations
{
public @interface Rule {
}
}

86
src/xdocs/config_design.xml
View File

@ -13,9 +13,9 @@
<section name="VisibilityModifier">
<subsection name="Description">
<p>
Checks visibility of class members. Only static final or immutable members
may be public; other class members must be private unless the
property <code>protectedAllowed</code> or <code>packageAllowed</code> 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 <code>protectedAllowed</code> or <code>packageAllowed</code> is set.
</p>
<p>
@ -33,7 +33,12 @@
Rationale: Enforce encapsulation.
</p>
<p>
Check also has an option making it less strict:
Check also has options making it less strict:
</p>
<p>
<b>ignoreAnnotationCanonicalNames</b> - 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
</p>
<p>
<b>allowPublicImmutableFields</b> - 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,</td>
</tr>
<tr>
<td>ignoreAnnotationCanonicalNames</td>
<td>ignore annotations canonical names</td>
<td><a href="property_types.html#stringSet">String Set</a></td>
<td>org.junit.Rule, com.google.common.annotations.VisibleForTesting</td>
</tr>
</table>
</subsection>
@ -194,6 +205,73 @@ public class ImmutableClass
this.notes = notes;
this.someValue = someValue;
}
}
</source>
<p>
To configure the Check passing fields annotated with @com.annotation.CustomAnnotation:
</p>
<source>
&lt;module name=&quot;VisibilityModifier&quot;&gt;
&lt;property name=&quot;ignoreAnnotationCanonicalNames&quot; value=
&quot;com.annotation.CustomAnnotation&quot;/&gt;
&lt;/module&gt;
</source>
<p>
Example of allowed field:
</p>
<source>
class SomeClass
{
@com.annotation.CustomAnnotation
String annotatedString; // no warning
@CustomAnnotation
String shortCustomAnnotated; // no warning
}
</source>
<p>
To configure the Check passing fields annotated with @org.junit.Rule and
@com.google.common.annotations.VisibleForTesting annotations:
</p>
<source>
&lt;module name=&quot;VisibilityModifier&quot;/&gt;
</source>
<p>
Example of allowed fields:
</p>
<source>
class SomeClass
{
@org.junit.Rule
public TemporaryFolder publicJUnitRule = new TemporaryFolder(); // no warning
@com.google.common.annotations.VisibleForTesting
public String testString = ""; // no warning
}
</source>
<p>
To configure the Check passing fields annotated with short annotation name:
</p>
<source>
&lt;module name=&quot;VisibilityModifier&quot;&gt;
&lt;property name=&quot;ignoreAnnotationCanonicalNames&quot;
value=&quot;CustomAnnotation&quot;/&gt;
&lt;/module&gt;
</source>
<p>
Example of allowed fields:
</p>
<source>
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
}
</source>
</subsection>