From d9bd21f3f7e589bfce3177a654f38a2e33ddf375 Mon Sep 17 00:00:00 2001 From: Oliver Burn Date: Sat, 9 Oct 2010 16:26:15 +1100 Subject: [PATCH] Patch #1939775 for star imports (first draft) --- .../checks/imports/AvoidStarImportCheck.java | 156 ++++++++++++++---- .../tools/checkstyle/api/ScopeTest.java | 5 +- .../checkstyle/api/SeverityLevelTest.java | 4 +- .../checks/imports/AvoidStarImportTest.java | 23 ++- 4 files changed, 153 insertions(+), 35 deletions(-) diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/AvoidStarImportCheck.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/AvoidStarImportCheck.java index a162181a5..8cdd6bf3b 100755 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/AvoidStarImportCheck.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/AvoidStarImportCheck.java @@ -27,73 +27,171 @@ import com.puppycrawl.tools.checkstyle.api.TokenTypes; *

* Check that finds import statements that use the * notation. *

- *

Rationale: Importing all classes from a package leads to tight coupling - * between packages and might lead to problems when a new version of a library - * introduces name clashes. + *

+ * Rationale: Importing all classes from a package or static + * members from a class leads to tight coupling between packages + * or classes and might lead to problems when a new version of a + * library introduces name clashes. *

*

* An example of how to configure the check is: *

* 
  * <module name="AvoidStarImport">
- *   <property name="excludes" value="java.io,java.net"/>
+ *   <property name="excludes" value="java.io,java.net,java.lang.Math"/>
+ *   <property name="allowClassImports" value="false"/>
+ *   <property name="allowStaticMemberImports" value="false"/>
  * </module>
  *
* * The optional "excludes" property allows for certain packages like - * java.io or java.net to be exempted from the rule. Note that the excludes - * property is not recursive, subpackages of excluded packages are not - * automatically excluded. + * java.io or java.net to be exempted from the rule. It also is used to + * allow certain classes like java.lang.Math or java.io.File to be + * excluded in order to support static member imports. * - * Compatible with Java 1.5 source. + * The optional "allowClassImports" when set to true, will allow starred + * class imports but will not affect static member imports. + * + * The optional "allowStaticMemberImports" when set to true will allow + * starred static member imports but will not affect class imports. * * @author Oliver Burn * @author Bill Schneider - * @version 1.0 + * @author Travis Schneeberger + * @version 2.0 */ public class AvoidStarImportCheck extends Check { - /** the packages to exempt from this check. */ + /** the packages/classes to exempt from this check. */ private String[] mExcludes = new String[0]; + /** whether to allow all class imports */ + private boolean mAllowClassImports; + + /** whether to allow all static member imports */ + private boolean mAllowStaticMemberImports; + @Override public int[] getDefaultTokens() { - return new int[] {TokenTypes.IMPORT}; + return new int[] {TokenTypes.IMPORT, TokenTypes.STATIC_IMPORT}; } /** - * Sets the list of packages to exempt from the check. - * @param aExcludes a list of package names where star imports are ok + * Sets the list of packages or classes to be exempt from the check. + * The excludes can contain a .* or not. + * @param aExcludes a list of package names/fully-qualifies class names + * where star imports are ok */ public void setExcludes(String[] aExcludes) { - mExcludes = new String[aExcludes.length]; + mExcludes = appendDotStar(aExcludes); + } + + /** + * Sets whether or not to allow all non-static class imports. + * @param aAllow true to allow false to disallow + */ + public void setAllowClassImports(boolean aAllow) + { + mAllowClassImports = aAllow; + } + + /** + * Sets whether or not to allow all static member imports. + * @param aAllow true to allow false to disallow + */ + public void setAllowStaticMemberImports(boolean aAllow) + { + mAllowStaticMemberImports = aAllow; + } + + @Override + public void visitToken(final DetailAST aAST) + { + if (!mAllowClassImports + && aAST.getType() == TokenTypes.IMPORT) + { + final DetailAST startingDot = + aAST.getFirstChild(); + logsStarredImportViolation(startingDot, mExcludes); + } + else if (!mAllowStaticMemberImports + && aAST.getType() == TokenTypes.STATIC_IMPORT) + { + //must navigate past the static keyword + final DetailAST startingDot = + aAST.getFirstChild().getNextSibling(); + logsStarredImportViolation(startingDot, mExcludes); + } + } + + /** + * Appends a .* to the end of the string in a given + * array of excludes. + * @param aExcludes array of excludes (either package or class) + * @return array of excludes with .* + */ + private String[] appendDotStar(String[] aExcludes) + { + final String[] excludes = new String[aExcludes.length]; for (int i = 0; i < aExcludes.length; i++) { - mExcludes[i] = aExcludes[i]; - if (!mExcludes[i].endsWith(".*")) { + excludes[i] = aExcludes[i]; + if (!excludes[i].endsWith(".*")) { // force all package names to end with ".*" to disambiguate // "java.io" - mExcludes[i] = mExcludes[i] + ".*"; + excludes[i] += ".*"; + } + } + return excludes; + } + + /** + * Gets the full import identifier. If the import is a starred import and + * it's not excluded then a violation is logged. + * @param aStartingDot the starting dot for the import statement + * @param aExcludes an array of excludes + */ + private void logsStarredImportViolation(DetailAST aStartingDot, + String[] aExcludes) + { + final FullIdent name = FullIdent.createFullIdent(aStartingDot); + + if (isStaredImport(name)) { + if (!isExempt(name.getText(), aExcludes)) { + log(aStartingDot.getLineNo(), + "import.avoidStar", name.getText()); } } } - @Override - public void visitToken(DetailAST aAST) + /** + * Checks is an import is a stared import. + * @param aImportIdent the full import identifier + * @return true if a start import false if not + */ + private boolean isStaredImport(FullIdent aImportIdent) { - final FullIdent name = FullIdent.createFullIdentBelow(aAST); - if ((name != null) && name.getText().endsWith(".*")) { - boolean exempt = false; - for (int i = 0; (i < mExcludes.length) && !exempt; i++) { - if (name.getText().equals(mExcludes[i])) { - exempt = true; - } - } - if (!exempt) { - log(aAST.getLineNo(), "import.avoidStar", name.getText()); + if ((aImportIdent != null) && aImportIdent.getText().endsWith(".*")) { + return true; + } + return false; + } + + /** + * Checks if a class of package is exempt from a give array of excludes. + * @param aClassOrPackage the class or package + * @param aExcludes an array of excludes + * @return true if except false if not + */ + private boolean isExempt(String aClassOrPackage, String[] aExcludes) + { + for (final String exclude : aExcludes) { + if (aClassOrPackage.equals(exclude)) { + return true; } } + return false; } } diff --git a/src/tests/com/puppycrawl/tools/checkstyle/api/ScopeTest.java b/src/tests/com/puppycrawl/tools/checkstyle/api/ScopeTest.java index 61f95c612..da0da7cd1 100755 --- a/src/tests/com/puppycrawl/tools/checkstyle/api/ScopeTest.java +++ b/src/tests/com/puppycrawl/tools/checkstyle/api/ScopeTest.java @@ -18,8 +18,9 @@ //////////////////////////////////////////////////////////////////////////////// package com.puppycrawl.tools.checkstyle.api; -import static org.junit.Assert.*; - +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import org.junit.Test; public class ScopeTest diff --git a/src/tests/com/puppycrawl/tools/checkstyle/api/SeverityLevelTest.java b/src/tests/com/puppycrawl/tools/checkstyle/api/SeverityLevelTest.java index c08a02d35..56508436b 100755 --- a/src/tests/com/puppycrawl/tools/checkstyle/api/SeverityLevelTest.java +++ b/src/tests/com/puppycrawl/tools/checkstyle/api/SeverityLevelTest.java @@ -18,8 +18,8 @@ //////////////////////////////////////////////////////////////////////////////// package com.puppycrawl.tools.checkstyle.api; -import static org.junit.Assert.*; - +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import org.junit.Test; public class SeverityLevelTest diff --git a/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/AvoidStarImportTest.java b/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/AvoidStarImportTest.java index 65625c271..f238b6a52 100755 --- a/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/AvoidStarImportTest.java +++ b/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/AvoidStarImportTest.java @@ -36,6 +36,9 @@ public class AvoidStarImportTest "7: Using the '.*' form of import should be avoided - com.puppycrawl.tools.checkstyle.imports.*.", "9: Using the '.*' form of import should be avoided - java.io.*.", "10: Using the '.*' form of import should be avoided - java.lang.*.", + "25: Using the '.*' form of import should be avoided - javax.swing.WindowConstants.*.", + "26: Using the '.*' form of import should be avoided - javax.swing.WindowConstants.*.", + "28: Using the '.*' form of import should be avoided - sun.net.ftpclient.FtpClient.*.", }; verify(checkConfig, getPath("imports" + File.separator + "InputImport.java"), expected); @@ -47,11 +50,27 @@ public class AvoidStarImportTest { final DefaultConfiguration checkConfig = createCheckConfig(AvoidStarImportCheck.class); - checkConfig.addAttribute("excludes", "java.io,java.lang"); - // allow the java.io/java.lang star imports + checkConfig.addAttribute("excludes", + "java.io,java.lang,javax.swing.WindowConstants.*, javax.swing.WindowConstants"); + // allow the java.io/java.lang,javax.swing.WindowConstants star imports final String[] expected2 = new String[] { "7: Using the '.*' form of import should be avoided - com.puppycrawl.tools.checkstyle.imports.*.", + "28: Using the '.*' form of import should be avoided - sun.net.ftpclient.FtpClient.*.", }; verify(checkConfig, getPath("imports" + File.separator + "InputImport.java"), expected2); } + + @Test + public void testAllowClassImports() throws Exception + { + final DefaultConfiguration checkConfig = createCheckConfig(AvoidStarImportCheck.class); + checkConfig.addAttribute("allowClassImports", "true"); + // allow all class star imports + final String[] expected2 = new String[] { + "25: Using the '.*' form of import should be avoided - javax.swing.WindowConstants.*.", + "26: Using the '.*' form of import should be avoided - javax.swing.WindowConstants.*.", + "28: Using the '.*' form of import should be avoided - sun.net.ftpclient.FtpClient.*.", }; + verify(checkConfig, getPath("imports" + File.separator + + "InputImport.java"), expected2); + } }