Issue sevntu-checkstyle#464: Check for trailing commas on enums

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.
 

eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/coding/checkstyle-metadata.xml

@@ -556,5 +556,12 @@
       <message-key key="require.fail"/>
     </rule-metadata>
 
+    <rule-metadata name="%EnumTrailingCommaCheck.name" internal-name="EnumTrailingCommaCheck" parent="TreeWalker">
+      <alternative-name internal-name="com.github.sevntu.checkstyle.checks.coding.EnumTrailingCommaCheck"/>
+      <description>%EnumTrailingCommaCheck.desc</description>
+
+      <message-key key="enum.trailing.comma"/>
+    </rule-metadata>
+
   </rule-group-metadata>
 </checkstyle-metadata>

sevntu-checks/sevntu-checks.xml

@@ -157,6 +157,7 @@
     <module name="com.github.sevntu.checkstyle.checks.coding.UselessSuperCtorCallCheck"/>
     <module name="com.github.sevntu.checkstyle.checks.coding.UselessSingleCatchCheck"/>
     <module name="com.github.sevntu.checkstyle.checks.coding.WhitespaceBeforeArrayInitializerCheck"/>
+    <module name="com.github.sevntu.checkstyle.checks.coding.EnumTrailingCommaCheck"/>
     <module name="com.github.sevntu.checkstyle.checks.coding.MoveVariableInsideIfCheck" />
     <module name="com.github.sevntu.checkstyle.checks.coding.RequireFailForTryCatchInJunitCheck" />
 

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.
+ *
+ * <p>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
+ * <a href="https://checkstyle.org/config_coding.html#ArrayTrailingComma">ArrayTrailingComma</a>.
+ *
+ * <p>The following is a normal enum type declaration:
+ * <pre>
+ * enum Type {
+ *     ALPHA,
+ *     BETA,
+ *     GAMMA
+ * }
+ * </pre>
+ *
+ * <p>However, if you want to append something to the list, you would need to change the line
+ * containing the last enum constant:
+ *
+ * <pre>
+ * enum Type {
+ *     ALPHA,
+ *     BETA,
+ *     GAMMA,   // changed due to the ','
+ *     DELTA    // new line
+ * }
+ * </pre>
+ *
+ * <p>This check makes sure that also the last enum constant has a trailing comma, which is
+ * valid according to the Java Spec (see <a
+ * href="http://docs.oracle.com/javase/specs/jls/se8/html/jls-8.html#jls-8.9.1">Enum Constants</a>)
+ *
+ * <pre>
+ * enum Type {
+ *     ALPHA,
+ *     BETA,
+ *     GAMMA,
+ *     DELTA, // removing this comma will result in a violation with the check activated
+ * }
+ * </pre>
+ *
+ * <p>However, you could also add a semicolon behind that comma on the same line, which would raise
+ * a violation
+ *
+ * <pre>
+ * enum Type {
+ *     ALPHA,
+ *     BETA,
+ *     GAMMA,
+ *     DELTA,; // violation
+ * }
+ * </pre>
+ * 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.
+ *
+ * <p>An example of how to configure the check is:
+ * <pre>
+ * &lt;module name="EnumTrailingComma"/&gt;
+ * </pre>
+ *
+ * <p>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 <a href="[email protected]">Kariem Hussein</a>
+ */
+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;
+    }
+}

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,
 
     }
 

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().

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

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<String> ADD_CHECK_SUPPRESSIONS = Arrays
-            .asList("ReturnCountExtendedCheck");
+            .asList(
+                    // conflicting with NoWhitespaceBefore, SeparatorWrap
+                    "EnumTrailingCommaCheck",
+
+                    "ReturnCountExtendedCheck"
+            );
 
     // -@cs[CyclomaticComplexity] Can't split
     @Test

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,
 
     }
 

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,;
+    }
+}

sevntu-checkstyle-sonar-plugin/src/main/resources/com/github/sevntu/checkstyle/sonar/checkstyle-extensions.xml

@@ -897,6 +897,14 @@
     <configKey>Checker/TreeWalker/com.github.sevntu.checkstyle.checks.coding.WhitespaceBeforeArrayInitializerCheck</configKey>
   </rule>
 
+  <rule>
+    <key>com.github.sevntu.checkstyle.checks.coding.EnumTrailingCommaCheck</key>
+    <name>Enum Trailing Comma</name>
+    <category name="coding"/>
+    <description>This checks enforces a trailing comma at the end of a line in an enum constant definition.</description>
+    <configKey>Checker/TreeWalker/com.github.sevntu.checkstyle.checks.coding.EnumTrailingCommaCheck</configKey>
+  </rule>
+
   <rule>
     <key>com.github.sevntu.checkstyle.checks.design.InnerClassCheck</key>
     <name>Inner Class</name>