From 1b8739896bd25b20d496882f8bcedce7a20869bb Mon Sep 17 00:00:00 2001 From: Kariem Hussein Date: Fri, 29 Jul 2016 15:10:56 +0200 Subject: [PATCH] Issue #464: Check for trailing commas on enums --- .../coding/checkstyle-metadata.properties | 3 + .../checks/coding/checkstyle-metadata.xml | 7 + sevntu-checks/sevntu-checks.xml | 1 + .../checks/coding/EnumTrailingCommaCheck.java | 165 ++++++++++++++++++ .../NumericLiteralNeedsUnderscoreCheck.java | 2 +- .../checks/coding/messages.properties | 3 +- .../coding/EnumTrailingCommaCheckTest.java | 56 ++++++ .../internal/CheckstyleRegressionTest.java | 7 +- .../internal/CommitValidationTest.java | 3 +- .../coding/InputEnumTrailingCommaCheck.java | 61 +++++++ .../sonar/checkstyle-extensions.xml | 8 + 11 files changed, 312 insertions(+), 4 deletions(-) create mode 100644 sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/EnumTrailingCommaCheck.java create mode 100644 sevntu-checks/src/test/java/com/github/sevntu/checkstyle/checks/coding/EnumTrailingCommaCheckTest.java create mode 100644 sevntu-checks/src/test/resources/com/github/sevntu/checkstyle/checks/coding/InputEnumTrailingCommaCheck.java diff --git a/eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/coding/checkstyle-metadata.properties b/eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/coding/checkstyle-metadata.properties index e7efd317dd..1d9fbf9418 100755 --- a/eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/coding/checkstyle-metadata.properties +++ b/eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/coding/checkstyle-metadata.properties @@ -183,6 +183,9 @@ EitherLogOrThrowCheck.loggingMethodNames = Logging method names separated with c WhitespaceBeforeArrayInitializerCheck.name = Whitespace Before Array Initializer WhitespaceBeforeArrayInitializerCheck.desc = This checks enforces whitespace before array initializer. +EnumTrailingCommaCheck.name = Enum Trailing Comma +EnumTrailingCommaCheck.desc = This checks enforces a trailing comma at the end of a line in an enum constant definition. + MoveVariableInsideIfCheck.name = Move Variable Inside If Check MoveVariableInsideIfCheck.desc = Checks if a variable is only used inside if statements and asks for it's declaration to be moved there too. diff --git a/eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/coding/checkstyle-metadata.xml b/eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/coding/checkstyle-metadata.xml index 3392c3bec1..3b4263dcaa 100644 --- a/eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/coding/checkstyle-metadata.xml +++ b/eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/coding/checkstyle-metadata.xml @@ -556,5 +556,12 @@ + + + %EnumTrailingCommaCheck.desc + + + + diff --git a/sevntu-checks/sevntu-checks.xml b/sevntu-checks/sevntu-checks.xml index d4c7612e18..d22927d095 100644 --- a/sevntu-checks/sevntu-checks.xml +++ b/sevntu-checks/sevntu-checks.xml @@ -157,6 +157,7 @@ + diff --git a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/EnumTrailingCommaCheck.java b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/EnumTrailingCommaCheck.java new file mode 100644 index 0000000000..d7486d44fe --- /dev/null +++ b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/EnumTrailingCommaCheck.java @@ -0,0 +1,165 @@ +//////////////////////////////////////////////////////////////////////////////// +// checkstyle: Checks Java source code for adherence to a set of rules. +// Copyright (C) 2001-2018 the original author or authors. +// +// This library is free software; you can redistribute it and/or +// modify it under the terms of the GNU Lesser General Public +// License as published by the Free Software Foundation; either +// version 2.1 of the License, or (at your option) any later version. +// +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +// Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public +// License along with this library; if not, write to the Free Software +// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +//////////////////////////////////////////////////////////////////////////////// + +package com.github.sevntu.checkstyle.checks.coding; + +import com.puppycrawl.tools.checkstyle.api.AbstractCheck; +import com.puppycrawl.tools.checkstyle.api.DetailAST; +import com.puppycrawl.tools.checkstyle.api.TokenTypes; + +/** + * Checks if enum constant contains an optional trailing comma. + * + *

Rationale: Putting this comma in makes is easier to change the order of the elements or add + * new elements at the end of the list. This is similar to + * ArrayTrailingComma. + * + *

The following is a normal enum type declaration: + * 

+ * enum Type {
+ *     ALPHA,
+ *     BETA,
+ *     GAMMA
+ * }
+ *
+ * + *

However, if you want to append something to the list, you would need to change the line + * containing the last enum constant: + * + * 

+ * enum Type {
+ *     ALPHA,
+ *     BETA,
+ *     GAMMA,   // changed due to the ','
+ *     DELTA    // new line
+ * }
+ *
+ * + *

This check makes sure that also the last enum constant has a trailing comma, which is + * valid according to the Java Spec (see Enum Constants) + * + * 

+ * enum Type {
+ *     ALPHA,
+ *     BETA,
+ *     GAMMA,
+ *     DELTA, // removing this comma will result in a violation with the check activated
+ * }
+ *
+ * + *

However, you could also add a semicolon behind that comma on the same line, which would raise + * a violation + * + * 

+ * enum Type {
+ *     ALPHA,
+ *     BETA,
+ *     GAMMA,
+ *     DELTA,; // violation
+ * }
+ *
+ * In this case the semicolon should be removed. However, if there is more in the enum body, the + * semicolon should be placed on a line by itself. + * + *

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

+ * <module name="EnumTrailingComma"/>
+ *
+ * + *

Please note that using this check together with {@code NoWhitespaceBefore} or + * {@code SeparatorWrap} may create conflicts with enums that contain a body: + * {@code EnumTrailingComma} enforces the semicolon on a separate line while + * {@code NoWhiteSpaceBefore} does not allow the semicolon to be preceded with whitespace and + * {@code SeparatorWrap} expects the semicolon to be on the same line as the last enum constant. + * + * @author Kariem Hussein + */ +public class EnumTrailingCommaCheck extends AbstractCheck { + + /** Key for warning message text in "messages.properties" file. */ + public static final String MSG_KEY = "enum.trailing.comma"; + + @Override + public int[] getDefaultTokens() { + return getAcceptableTokens(); + } + + @Override + public int[] getAcceptableTokens() { + return new int[] {TokenTypes.ENUM_DEF}; + } + + @Override + public int[] getRequiredTokens() { + return getAcceptableTokens(); + } + + @Override + public void visitToken(DetailAST enumDef) { + final DetailAST enumConstBlock = enumDef.findFirstToken(TokenTypes.OBJBLOCK); + + final DetailAST enumConstLeft = enumConstBlock.findFirstToken(TokenTypes.LCURLY); + final DetailAST enumConstRight = enumConstBlock.findFirstToken(TokenTypes.RCURLY); + + // Only check, if block is multi-line and there are more than one enum constants + if (enumConstLeft.getLineNo() != enumConstRight.getLineNo() + && enumConstBlock.getChildCount(TokenTypes.ENUM_CONSTANT_DEF) > 1) { + final DetailAST constant = enumConstBlock.findFirstToken(TokenTypes.ENUM_CONSTANT_DEF); + final DetailAST lastComma = getLastComma(constant); + + final DetailAST nextAst = lastComma.getNextSibling(); + if (isIllegalTokenAfterComma(lastComma, nextAst)) { + log(nextAst, MSG_KEY); + } + } + } + + /** + * Check whether there is an illegal token after the last comma token. + * + * @param lastComma the AST containing the last comma + * @param nextAst the next AST + * @return {@code true} if there is an illegal token after the last comma, + * {@code false} otherwise + */ + private boolean isIllegalTokenAfterComma(DetailAST lastComma, DetailAST nextAst) { + final int nextAstType = nextAst.getType(); + + // semi on the same line as last comma, or followed by enum constant + return (nextAstType == TokenTypes.SEMI && nextAst.getLineNo() == lastComma.getLineNo()) + || nextAstType == TokenTypes.ENUM_CONSTANT_DEF; + } + + /** + * Get the last comma in a series of siblings. + * + * @param start the first sibling + * @return the AST containing the last comma + */ + private static DetailAST getLastComma(DetailAST start) { + DetailAST comma = null; + for (DetailAST ast = start; ast != null; ast = ast.getNextSibling()) { + if (ast.getType() == TokenTypes.COMMA) { + comma = ast; + } + } + return comma; + } +} diff --git a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/NumericLiteralNeedsUnderscoreCheck.java b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/NumericLiteralNeedsUnderscoreCheck.java index be23a6fdf3..a3821a152e 100644 --- a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/NumericLiteralNeedsUnderscoreCheck.java +++ b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/NumericLiteralNeedsUnderscoreCheck.java @@ -166,7 +166,7 @@ protected enum NumericType { /** * Denotes a binary literal. For example, 0b0011 */ - BINARY; + BINARY, } diff --git a/sevntu-checks/src/main/resources/com/github/sevntu/checkstyle/checks/coding/messages.properties b/sevntu-checks/src/main/resources/com/github/sevntu/checkstyle/checks/coding/messages.properties index 04ee0a0026..9427dcc5ad 100644 --- a/sevntu-checks/src/main/resources/com/github/sevntu/checkstyle/checks/coding/messages.properties +++ b/sevntu-checks/src/main/resources/com/github/sevntu/checkstyle/checks/coding/messages.properties @@ -12,8 +12,9 @@ custom.declaration.order.interface=Interface definition in wrong order. Expected custom.declaration.order.enum=Enum definition in wrong order. Expected ''{0}'' then ''{1}''. custom.declaration.order.invalid.setter=Setter ''{0}'' is in wrong order. Should be right after ''{1}''. diamond.operator.for.variable.definition = Diamond operator expected. -empty.public.ctor=This empty public constructor is useless. either.log.or.throw=Either log or throw exception. +empty.public.ctor=This empty public constructor is useless. +enum.trailing.comma=Enum constant should be followed by only a trailing comma in the line. finalize.implementation.missed.try.finally = finalize() method should contain try-finally block with super.finalize() call inside finally block. finalize.implementation.public = finalize() method should have a "protected" visibility. finalize.implementation.useless = finalize() method is useless: it does nothing except for calling super.finalize(). diff --git a/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/checks/coding/EnumTrailingCommaCheckTest.java b/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/checks/coding/EnumTrailingCommaCheckTest.java new file mode 100644 index 0000000000..64ea45514e --- /dev/null +++ b/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/checks/coding/EnumTrailingCommaCheckTest.java @@ -0,0 +1,56 @@ +//////////////////////////////////////////////////////////////////////////////// +// checkstyle: Checks Java source code for adherence to a set of rules. +// Copyright (C) 2001-2018 the original author or authors. +// +// This library is free software; you can redistribute it and/or +// modify it under the terms of the GNU Lesser General Public +// License as published by the Free Software Foundation; either +// version 2.1 of the License, or (at your option) any later version. +// +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +// Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public +// License along with this library; if not, write to the Free Software +// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +//////////////////////////////////////////////////////////////////////////////// + +package com.github.sevntu.checkstyle.checks.coding; + +import static com.github.sevntu.checkstyle.checks.coding.EnumTrailingCommaCheck.MSG_KEY; + +import org.junit.Assert; +import org.junit.Test; + +import com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport; +import com.puppycrawl.tools.checkstyle.DefaultConfiguration; + +public class EnumTrailingCommaCheckTest extends AbstractModuleTestSupport { + + @Override + protected String getPackageLocation() { + return "com/github/sevntu/checkstyle/checks/coding"; + } + + @Test + public void testDefault() throws Exception { + final DefaultConfiguration checkConfig = + createModuleConfig(EnumTrailingCommaCheck.class); + final String[] expected = { + "14:9: " + getCheckMessage(MSG_KEY), + "20:9: " + getCheckMessage(MSG_KEY), + "26:15: " + getCheckMessage(MSG_KEY), + }; + verify(checkConfig, getPath("InputEnumTrailingCommaCheck.java"), expected); + } + + @Test + public void testTokensNotNull() { + final EnumTrailingCommaCheck check = new EnumTrailingCommaCheck(); + Assert.assertNotNull(check.getAcceptableTokens()); + Assert.assertNotNull(check.getDefaultTokens()); + Assert.assertNotNull(check.getRequiredTokens()); + } +} diff --git a/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CheckstyleRegressionTest.java b/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CheckstyleRegressionTest.java index f4d25eeef8..e053cdb7f1 100644 --- a/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CheckstyleRegressionTest.java +++ b/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CheckstyleRegressionTest.java @@ -36,7 +36,12 @@ public class CheckstyleRegressionTest { /** List of checks to suppress if we dynamically add it to the configuration. */ private static final List ADD_CHECK_SUPPRESSIONS = Arrays - .asList("ReturnCountExtendedCheck"); + .asList( + // conflicting with NoWhitespaceBefore, SeparatorWrap + "EnumTrailingCommaCheck", + + "ReturnCountExtendedCheck" + ); // -@cs[CyclomaticComplexity] Can't split @Test diff --git a/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CommitValidationTest.java b/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CommitValidationTest.java index 38221cadb9..786f98911a 100644 --- a/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CommitValidationTest.java +++ b/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CommitValidationTest.java @@ -291,7 +291,8 @@ private static String getInvalidCommitMessageFormattingError(String commitId, private enum CommitsResolutionMode { - BY_COUNTER, BY_LAST_COMMIT_AUTHOR + BY_COUNTER, + BY_LAST_COMMIT_AUTHOR, } diff --git a/sevntu-checks/src/test/resources/com/github/sevntu/checkstyle/checks/coding/InputEnumTrailingCommaCheck.java b/sevntu-checks/src/test/resources/com/github/sevntu/checkstyle/checks/coding/InputEnumTrailingCommaCheck.java new file mode 100644 index 0000000000..ba398ec1f7 --- /dev/null +++ b/sevntu-checks/src/test/resources/com/github/sevntu/checkstyle/checks/coding/InputEnumTrailingCommaCheck.java @@ -0,0 +1,61 @@ +package com.github.sevntu.checkstyle.checks.coding; + +public interface InputEnumTrailingCommaCheck +{ + enum E1 { + ONE, + TWO, + THREE, + } + + enum E2 { + ONE, + TWO, + THREE // violation + } + + enum E3 { + ONE, + TWO, + THREE; // violation + } + + enum E4 { + ONE, + TWO, + THREE,; // violation + } + + enum E5 { + ONE, + TWO, + THREE, + ; + } + + // enums below are ignored by the check, but were added for completenes + // Please don't remove, they are necessary for full cobertura branch coverage + + // empty + enum E6 {} + + // single enum const, single-line block + enum E7_1 { ONE } + enum E7_2 { ONE; } + enum E7_3 { ONE, } + enum E7_4 { ONE,; } + + // single enum const, multi-line block + enum E8_1 { + ONE + } + enum E8_2 { + ONE; + } + enum E8_3 { + ONE, + } + enum E8_4 { + ONE,; + } +} diff --git a/sevntu-checkstyle-sonar-plugin/src/main/resources/com/github/sevntu/checkstyle/sonar/checkstyle-extensions.xml b/sevntu-checkstyle-sonar-plugin/src/main/resources/com/github/sevntu/checkstyle/sonar/checkstyle-extensions.xml index 083323a6c7..5cf63e7eb0 100644 --- a/sevntu-checkstyle-sonar-plugin/src/main/resources/com/github/sevntu/checkstyle/sonar/checkstyle-extensions.xml +++ b/sevntu-checkstyle-sonar-plugin/src/main/resources/com/github/sevntu/checkstyle/sonar/checkstyle-extensions.xml @@ -897,6 +897,14 @@ Checker/TreeWalker/com.github.sevntu.checkstyle.checks.coding.WhitespaceBeforeArrayInitializerCheck + + com.github.sevntu.checkstyle.checks.coding.EnumTrailingCommaCheck + Enum Trailing Comma + + This checks enforces a trailing comma at the end of a line in an enum constant definition. + Checker/TreeWalker/com.github.sevntu.checkstyle.checks.coding.EnumTrailingCommaCheck + + com.github.sevntu.checkstyle.checks.design.InnerClassCheck Inner Class