Issue #1833: Fixed redundant modifier false positive for abstract classes in interfaces.

src/main/java/com/puppycrawl/tools/checkstyle/checks/modifier/RedundantModifierCheck.java

@@ -29,15 +29,40 @@ import com.puppycrawl.tools.checkstyle.api.DetailAST;
 import com.puppycrawl.tools.checkstyle.api.TokenTypes;
 
 /**
- * Checks for redundant modifiers in interface and annotation definitions.
- * Checks for non public class constructor and enum constructor redundant modifier.
- * Checks for redundant final modifiers on methods of final classes.
- * Checks for redundant static modifiers on nested enums.
+ * Checks for redundant modifiers in interface and annotation definitions,
+ * final modifier on methods of final classes, inner <code>interface</code>
+ * declarations that are declared as <code>static</code>, non public class
+ * constructors and enum constructors, nested enum definitions that are declared
+ * as <code>static</code>.
  *
- * <p>Examples:</p>
+ * <p>Interfaces by definition are abstract so the <code>abstract</code>
+ * modifier on the interface is redundant.
+ *
+ * <p>Classes inside of interfaces by definition are public and static,
+ * so the <code>public</code> and <code>static</code> modifiers
+ * on the inner classes are redundant. On the other hand, classes
+ * inside of interfaces can be abstract or non abstract.
+ * So, <code>abstract</code> modifier is allowed.
+ *
+ * <p>Fields in interfaces and annotations are automatically
+ * public, static and final, so these modifiers are redundant as
+ * well.</p>
+ *
+ * <p>As annotations are a form of interface, their fields are also
+ * automatically public, static and final just as their
+ * annotation fields are automatically public and abstract.</p>
+ *
+ * <p>Enums by definition are static implicit subclasses of java.lang.Enum&#60;E&#62;.
+ * So, the <code>static</code> modifier on the enums is redundant. In addition,
+ * if enum is inside of interface, <code>public</code> modifier is also redundant.
+ *
+ * <p>Final classes by definition cannot be extended so the <code>final</code>
+ * modifier on the method of a final class is redundant.
+ *
+ * <p>Public modifier for constructors in non-public non-protected classes
+ * is always obsolete: </p>
  *
  * <pre>
- * {@code
  * public class PublicClass {
  *     public PublicClass() {} // OK
  * }
@@ -45,36 +70,30 @@ import com.puppycrawl.tools.checkstyle.api.TokenTypes;
  * class PackagePrivateClass {
  *     public PackagePrivateClass() {} // violation expected
  * }
- * }
  * </pre>
  *
+ * <p>There is no violation in the following example,
+ * because removing public modifier from ProtectedInnerClass
+ * constructor will make this code not compiling: </p>
+ *
  * <pre>
- * {@code
  * package a;
- * public class ClassWithProtectedInnerClass {
- *     protected class ProtectedClass {
- *         public ProtectedClass () {} // OK
+ * public class ClassExample {
+ *     protected class ProtectedInnerClass {
+ *         public ProtectedInnerClass () {}
  *     }
  * }
- * }
- * </pre>
- * <p>
- * in this example is no violation because removing public from
- * ProtectedClass constructor modifier will make this example
- * not compiling:
- * </p>
- * <pre>
- * {@code
+ *
  * package b;
- * import a.ClassWithProtectedInnerClass;
- * public class ClassExtending extends ClassWithProtectedInnerClass {
- *     ProtectedClass pc = new ProtectedClass();
- * }
+ * import a.ClassExample;
+ * public class ClassExtending extends ClassExample {
+ *     ProtectedInnerClass pc = new ProtectedInnerClass();
  * }
  * </pre>
  *
  * @author lkuehne
  * @author <a href="mailto:piotr.listkiewicz@gmail.com">liscju</a>
+ * @author <a href="mailto:andreyselkin@gmail.com">Andrei Selkin</a>
  * @author Vladislav Lisetskiy
  */
 public class RedundantModifierCheck
@@ -208,6 +227,7 @@ public class RedundantModifierCheck
                 || type == TokenTypes.LITERAL_STATIC
                         && ast.getType() != TokenTypes.METHOD_DEF
                 || type == TokenTypes.ABSTRACT
+                        && ast.getType() != TokenTypes.CLASS_DEF
                 || type == TokenTypes.FINAL) {
                 log(modifier.getLineNo(), modifier.getColumnNo(),
                         MSG_KEY, modifier.getText());

src/test/java/com/puppycrawl/tools/checkstyle/checks/modifier/RedundantModifierTest.java

@@ -33,6 +33,20 @@ import com.puppycrawl.tools.checkstyle.api.TokenTypes;
 
 public class RedundantModifierTest
     extends BaseCheckTestSupport {
+
+    @Test
+    public void testClassesInsideOfInterfaces() throws Exception {
+        final DefaultConfiguration checkConfig =
+            createCheckConfig(RedundantModifierCheck.class);
+        final String[] expected = {
+            "11:5: " + getCheckMessage(MSG_KEY, "static"),
+            "17:5: " + getCheckMessage(MSG_KEY, "public"),
+            "20:5: " + getCheckMessage(MSG_KEY, "public"),
+            "26:5: " + getCheckMessage(MSG_KEY, "static"),
+        };
+        verify(checkConfig, getPath("InputModifierClassesInsideOfInterfaces.java"), expected);
+    }
+
     @Test
     public void testIt() throws Exception {
         final DefaultConfiguration checkConfig =

src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/InputFinalInDefaultMethods.java

@@ -1,6 +1,6 @@
 //Compilable with Java8
 public interface MyInterface {
-	final int k = 5; //WARNING
+	final int k = 5; // violation
 	
     default int defaultMethod(final int x) {
     	if (k == 5) {

src/test/resources/com/puppycrawl/tools/checkstyle/InputModifier.java

@@ -51,32 +51,32 @@ strictfp final class InputModifier // illegal order of modifiers for class
     }
 
     /** holder for redundant 'public' modifier check. */
-    public static interface InputRedundantPublicModifier
+    public static interface InputRedundantPublicModifier // violation
     {
         /** redundant 'public' modifier */
-        public void a();
+        public void a(); // violation
 
         /** all OK */
         void b();
 
         /** redundant abstract modifier */
-        abstract void c();
+        abstract void c(); // violation
 
         /** redundant 'public' modifier */
-        public float PI_PUBLIC = (float) 3.14;
+        public float PI_PUBLIC = (float) 3.14; // violation
 
         /** redundant 'abstract' modifier (field can not be abstract) */
 //        abstract float PI_ABSTRACT = (float) 3.14;
 
         /** redundant 'final' modifier */
-        final float PI_FINAL = (float) 3.14;
+        final float PI_FINAL = (float) 3.14; // violation
 
         /** all OK */
         float PI_OK = (float) 3.14;
     }
 
     /** redundant 'final' modifier */
-    private final void method()
+    private final void method() // violation
     {
     }
 }
@@ -85,7 +85,7 @@ strictfp final class InputModifier // illegal order of modifiers for class
 final class RedundantFinalClass
 {
     /** redundant 'final' modifier */
-    public final void finalMethod()
+    public final void finalMethod() // violation
     {
     }
 
@@ -96,7 +96,7 @@ final class RedundantFinalClass
 }
 
 /** Holder for redundant modifiers of inner implementation */
-abstract interface InnerImplementation
+abstract interface InnerImplementation // violation
 {
     InnerImplementation inner =
         new InnerImplementation()
@@ -113,12 +113,12 @@ abstract interface InnerImplementation
 /** Holder for redundant modifiers of annotation fields/variables */
 @interface Annotation
 {
-    public String s1 = "";
-    final String s2 = "";
-    static String s3 = "";
+    public String s1 = ""; // violation
+    final String s2 = ""; // violation
+    static String s3 = ""; // violation
     String s4 = "";
-    public String blah();
-    abstract String blah2();
+    public String blah(); // violation
+    abstract String blah2(); // violation
 }
 
 @interface MyAnnotation2 {

src/test/resources/com/puppycrawl/tools/checkstyle/InputModifierClassesInsideOfInterfaces.java

@@ -0,0 +1,31 @@
+package com.puppycrawl.tools.checkstyle;
+
+public interface InputModifierClassesInsideOfInterfaces {
+
+    // Class inside of interface can be abstract and non abstract, but always public static.
+    abstract class A {}
+
+    class B {}
+
+    // All classes inside interfaces are public static classes, and static modifier is redundant.
+    static class C { // violation
+        public static boolean verifyState( A a ) {
+            return true;
+        }
+    }
+
+    public class E {} // violation
+
+    // Enums are static implicit subclasses of Enum class.
+    public enum Role1 { // violation
+        ADMIN,
+        EDITOR,
+        VANILLA;
+    }
+
+    static enum Role2 { // violation
+        ADMIN,
+        EDITOR,
+        VANILLA;
+    }
+}

src/test/resources/com/puppycrawl/tools/checkstyle/InputRedundantConstructorModifier.java

@@ -7,7 +7,7 @@ package com.puppycrawl.tools.checkstyle;
 public enum InputRedundantConstructorModifier {
     VAL1, VAL2;
 
-    private InputRedundantConstructorModifier() { }
+    private InputRedundantConstructorModifier() { } // violation
 
     InputRedundantConstructorModifier(int i) { }
 

src/test/resources/com/puppycrawl/tools/checkstyle/InputRedundantStaticModifierInInnerTypeOfInterface.java

@@ -5,11 +5,11 @@
 package com.puppycrawl.tools.checkstyle;
 
 public interface InputRedundantStaticModifierInInnerTypeOfInterface {
-    static class MyInnerClass { }
+    static class MyInnerClass { } // violation
 
     class MyInnerClass2 { }
 
-    static enum MyInnerEnum { }
+    static enum MyInnerEnum { } // violation
 
     enum MyInnerEnum2 { }
 }

src/xdocs/checks.xml

@@ -617,7 +617,11 @@
       </tr>
       <tr>
         <td><a href="config_modifier.html#RedundantModifier">RedundantModifier</a></td>
-        <td>Checks for redundant modifiers in interface and annotation definitions.</td>
+        <td>Checks for redundant modifiers in interface and annotation definitions,
+          final modifier on methods of final classes, inner <code>interface</code>
+          declarations that are declared as <code>static</code>, non public class
+          constructors and enum constructors, nested enum definitions that are declared
+          as <code>static</code>.</td>
       </tr>
       <tr>
         <td><a href="config_regexp.html#Regexp">Regexp</a></td>

src/xdocs/config_modifier.xml

@@ -122,7 +122,15 @@
         </p>
 
         <p>
-          Variables in interfaces and annotations are automatically
+          Classes inside of interfaces by definition are public and static,
+          so the <code>public</code> and <code>static</code> modifiers
+          on the inner classes are redundant. On the other hand, classes
+          inside of interfaces can be abstract or non abstract.
+          So, <code>abstract</code> modifier is allowed.
+        </p>
+
+        <p>
+          Fields in interfaces and annotations are automatically
           public, static and final, so these modifiers are redundant as
           well.
         </p>
@@ -134,14 +142,20 @@
         </p>
 
         <p>
-          Final classes by definition cannot be extended so the <code>final</code>
-          modifier on the method of a final class is redundant.
+          Enums by definition are static implicit subclasses of java.lang.Enum&#60;E&#62;.
+          So, the <code>static</code> modifier on the enums is redundant. In addition,
+          if enum is inside of interface, <code>public</code> modifier is also redundant.
         </p>
 
         <p>
           Nested <code>enum</code> types are always static by default.
         </p>
 
+        <p>
+          Final classes by definition cannot be extended so the <code>final</code>
+          modifier on the method of a final class is redundant.
+        </p>
+
         <p>
           Public modifier for constructors in non-public non-protected classes
           is always obsolete:
@@ -157,22 +171,23 @@
           }
         </source>
 
-        <p>For non-public protected classes public modifier is not obsolete,
-          removing public from this example will make code not compiling:
+        <p>There is no violation in the following example,
+          because removing public modifier from ProtectedInnerClass
+          constructor will make this code not compiling:
         </p>
 
         <source>
           package a;
-          public class ClassWithProtectedInnerClass {
-            protected class ProtectedClass {
-              public ProtectedClass () {}
+          public class ClassExample {
+            protected class ProtectedInnerClass {
+              public ProtectedInnerClass () {}
             }
           }
 
           package b;
-          import a.ClassWithProtectedInnerClass;
-          public class ClassExtending extends ClassWithProtectedInnerClass {
-            ProtectedClass pc = new ProtectedClass();
+          import a.ClassExample;
+          public class ClassExtending extends ClassExample {
+            ProtectedInnerClass pc = new ProtectedInnerClass();
           }
         </source>
       </subsection>