Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RedundantModifier should allow redundant modifiers in interfaces #5756

Closed
jodastephen opened this issue Apr 25, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@jodastephen
Copy link
Contributor

commented Apr 25, 2018

This is a request for a new check or a variation of RedundantModifier.

In Java 8, developers are allowed to have static and default methods on interfaces. In Java 9, developers can write private methods on interfaces. As part of this change, my Java 8 best practices talk slide 75
image

recommends that all methods on interfaces should be declared public static, public abstract or public default (plus private in Java 9).

In Java 9, the following combinations of modifiers are allowed on interfaces
https://bugs.openjdk.java.net/browse/JDK-8071453

First set:
"public static"
"public abstract"
"public default"
"private static"
"private"

Second set:
"static" - same as "public static"
"abstract" - same as "public abstract"
"public" - same as "public abstract"
"" - same as "public abstract"
"default" - same as "public default"

Since Java 8, I've argued that the existing coding standards for
interfaces are now wrong.
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-April/015803.html
http://blog.joda.org/2016/09/private-methods-in-interfaces-in-java-9.html
https://www.jfokus.se/jfokus17/preso/Java-SE-8-best-practices.pdf slide 75

What should be clear from the above list is that the first set of
modfiers are consistent, and match modifiers on classes. The second
set of modifiers are inconsistent, and require the developer to
remember that an absence of a modifier means package-scoped on a
class, but public on an interface. At this point, the different
meanings for an absence of modifier are just a confusing anachronism
in Java. So, this is not about methods lining up in a column, this is
about consistency across classes and interfaces, now that interfaces
are much more like classes.

Currently RedundantModifier is applied everywhere - on interfaces and classes, no way for customization. The change I want to control what happens separately on interfaces and classes.

No violations on following interface are expected:

$ java -version
java version "1.8.0_172"

$ javac TestClass.java

$ cat TestClass.java
public interface TestClass { 
  public static final int CONSTANT1 = 1;  // VIOLATION now
  public static int CONSTANT2 = 1;  // VIOLATION now
  public final int CONSTANT3 = 1;  // VIOLATION now
  static final int CONSTANT4 = 1;  // VIOLATION now
  final int CONSTANT5 = 1;  // VIOLATION now
  static int CONSTANT6 = 1;  // VIOLATION now
  public int CONSTANT7 = 1;  // VIOLATION now
  int CONSTANT8 = 1;

  public static boolean create1() { return true;}  // VIOLATION now
  static boolean create2() { return true;}

  public abstract void execute1();  // VIOLATION now
  abstract void execute2();  // VIOLATION now
  public void execute3();  // VIOLATION now

  public default void execute4() {} // VIOLATION now
  default void execute5() {} 

  //private static void execute6() {}  // jdk9 only
  //private void execute7() {}  // jdk9 only
}

$ cat TestConfig.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
        <module name="RedundantModifier">
        </module>
    </module>
</module>


$ java -jar checkstyle-8.10-all.jar -c TestConfig.xml TestClass.java 
Starting audit...
[ERROR] TestClass.java:2:3: Redundant 'public' modifier. [RedundantModifier]
[ERROR] TestClass.java:3:3: Redundant 'public' modifier. [RedundantModifier]
[ERROR] TestClass.java:4:3: Redundant 'public' modifier. [RedundantModifier]
[ERROR] TestClass.java:5:3: Redundant 'static' modifier. [RedundantModifier]
[ERROR] TestClass.java:6:3: Redundant 'final' modifier. [RedundantModifier]
[ERROR] TestClass.java:7:3: Redundant 'static' modifier. [RedundantModifier]
[ERROR] TestClass.java:8:3: Redundant 'public' modifier. [RedundantModifier]
[ERROR] TestClass.java:11:3: Redundant 'public' modifier. [RedundantModifier]
[ERROR] TestClass.java:14:3: Redundant 'public' modifier. [RedundantModifier]
[ERROR] TestClass.java:15:3: Redundant 'abstract' modifier. [RedundantModifier]
[ERROR] TestClass.java:16:3: Redundant 'public' modifier. [RedundantModifier]
[ERROR] TestClass.java:18:3: Redundant 'public' modifier. [RedundantModifier]
Audit done.
Checkstyle ends with 12 errors.

I don't really want to constrain how this is implemented since I don't know the codebase. However one possible way would be to add a new property to RedundantModifier that specifies a list of contexts to check - CLASS, INTERFACE, ENUM, ANNOTATION (allowing interfaces to be excluded from RedundantModifier).

Conversation was started at https://groups.google.com/forum/#!topic/checkstyle/KYF-9EEzsbs

@romani romani changed the title Require modifiers in interfaces RedundantModifier should allow redundant modifiers in interfaces May 9, 2018

@romani

This comment has been minimized.

Copy link
Member

commented May 10, 2018

@rnveach , I updated issue to be more focused on single change.

I doubt it will be good to make one more property for target type. We can just make boolean property skipInterfaces to skip validation completely for Interfaces.

@jodastephen ,
Sorry for delay, and almost forgotten reply in mail-list, it was not forgotten, just tried to postpone as too much stuff is moving in project.

The change I want to control what happens separately on interfaces and classes.

You want to make redundant rules for classes and interfaces separately configurable.
Do you see what modifiers still left redundant for interface members in your style ? If nothing, we will just do new property skipInterfaces.

@jodastephen

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2018

As I mentioned in #5783 I don't think skipInterfaces is sufficient. While it would meet my immediate needs, it wouldn't if I had rules around enums. And future changes for value types and records are likely to add more edge cases where style checks might be needed.

@rnveach

This comment has been minimized.

Copy link
Member

commented May 15, 2018

@romani

We can just make boolean property skipInterfaces to skip validation completely for Interfaces.

Eventually we will have xpath suppression filter supporting all TreeWalker checks. When this happens, will there be a time when we stop creating property suppressions and tell user to write xpath to suppress? Is this issue one of those cases?

@jodastephen

I don't think skipInterfaces is sufficient... it wouldn't if I had rules around enums

Please provide us all details for areas you don't want RedundantModifier to give a violation so we can make an accurate decision.

@romani

This comment has been minimized.

Copy link
Member

commented May 28, 2018

@jodastephen ,

And future changes for value types and records are likely to add more edge cases where style checks might be needed.

Please share links to specification or discussion or ... to let me take a look how it might affect Modifiers style in classes/interfaces/enums/annotations.
We usually do not plan to make Check too general for no very good reason. It make problems for users to understand how to set it up to for their case, additionally complicate implementation, on conner cases result in bunch of issues, so maintenance will be complicated, no one is happy.

in latest release 8.10.1 we added support of RedundantModifierCheck to SuppressionXpathFilter http://checkstyle.sourceforge.net/config_filters.html#SuppressionXpathFilter , so you can suppress easily all violations from this Check over interfaces only.

/var/tmp$ cat TestConfig.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <module name="TreeWalker">

        <module name="RedundantModifier" />
        <module name="SuppressionXpathFilter">
            <property name="file" value="suppressions.xml"/>
        </module>

    </module>
</module>

/var/tmp$ cat suppressions.xml 
<?xml version="1.0"?>
<!DOCTYPE suppressions PUBLIC
    "-//Puppy Crawl//DTD Suppressions Xpath Experimental 1.2//EN"
    "http://checkstyle.sourceforge.net/dtds/suppressions_1_2_xpath_experimental.dtd">

<suppressions>
    <suppress-xpath checks="RedundantModifier" query="/INTERFACE_DEF//*"/>
</suppressions>

/var/tmp$ cat Test.java 
public interface TestClass {
  public static final int CONSTANT1 = 1;  // VIOLATION now
}

/var/tmp$ java -jar checkstyle-8.10.1-all.jar -c TestConfig.xml Test.java 
Starting audit...
Audit done.

It would be good compromise while we collecting knowledge and best practices for new style demands for modifiers in different types(class/interface/enum/....).
It even looks like persistent solution without changes to Check. If there will be more requests for this style, we will add some support in Check to work out-of-the-box, but with more requests we will be more sure in that is really users what to control and how.

Does it work for you ?

@jodastephen

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

Thanks for the changes and update.

The good news is that the concept seems to basically work - it allows RedundantModifier to be used but suppressed on interfaces.

The bad news is that I can't actually use it in practice. The combination of Maven and Eclipse-CS conspire to make separate files, suppression and header, not work. In multi-module projects, a separate "build-config" jar file is the only way to go, but that just can't cope with the extra files - the additional file cannot be located (bearing in mind it needs to work in Eclipse, IntelliJ, Maven from project root and Maven from any pom.xml subfolder).

I also discovered that in an ideal world, I would want to not suppress warnings about using final on method parameters in interfaces. But I could definitely live without that.

One option would be if the suppress xpath concept could be inlined into checkstyle.xml like headers can.

@romani

This comment has been minimized.

Copy link
Member

commented May 31, 2018

sad ....

extra files is known problem, very old issue - #65 (filters are very similar in design, so share same problems). Checkstyle config in under DTD, so no that much freedom to inline extra stuff.
Extending Checks with suppression Xpath is also a problem, as not all Checks can be suppressed, due several internal/legacy problems in checkstyle. Xpath support is in progress.

@rnveach

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

One option would be if the suppress xpath concept could be inlined into checkstyle.xml like headers can.

@jodastephen I am closing this issue.
Please use SuppressionXpathSingleFilter to embed suppression into configuration file as this was just released.
See http://checkstyle.sourceforge.net/config_filters.html#SuppressionXpathSingleFilter , examples section provide example on how to configure such filter for this case.

@rnveach rnveach closed this Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.