Checkstyle: create concept of inheritance/override and compositions/extension of configs

[romani]

create concept of inheritance/override and compositions of configs to support main config and custom suppression and additional changes to certain Checks without copy-paste of configs.

[jochenvdv]

I gave this some thought and propose the following solution. Please give feedback.

Something we would have to deal with is the fact that modules can be configured multiple times.
A backwards-compatible solution would be to create an id property for modules and a new property on Checker to inherit from an existing configuration file. To override a module, the same id should be used, else it is added as an extra check. Modules without an id are never overriden.

We should also add an attribute disabled to modules so that a child configuration can disable an inherited module.

When a module is overriden, all properties are lost and have to be configured again, unless we introduce a way to inherit specific property values, for example an attribute inherit. Properties with multiples values that are inherited could be appended to.

For example:

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
     "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
     "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<!-- parent.xml -->
<module name="Checker">
    <module name="TreeWalker">
        <module name="EqualsAvoidNull" id="equals-avoid-null"/>

        <module name="FinalLocalVariable" id="final-local-variable"/>

        <module name="ImportControl" id="import-control">
            <property name="file" value="path/to/import-control.xml"/>
            <property name="path" value="^.*[\\/]src[\\/]main[\\/].*$"/>
        </module>

        <module name="LineLength" id="line-length">
            <property name="max" value="100"/>
            <property name="ignorePattern" value="^ *\* *[^ ]+$"/>
        </module>

        <module name="NoWhitespaceAfter" id="no-whitespace-after">
            <property name="tokens" value="ARRAY_INIT"/>
            <property name="tokens" value="BNOT"/>
            <property name="tokens" value="DEC"/>
        </module>
    </module>
</module>

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
     "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
     "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<!-- child.xml -->
<module name="Checker">
    <!-- This configuration inherits from parent.xml -->
    <property name="parent" value="path/to/parent.xml"/>

    <module name="TreeWalker">
        <!-- EqualsAvoidNull check is inherited, with all properties set in parent -->

        <!-- FinalLocalVariable check is overriden because id matched, but is disabled in child --> 
        <module name="FinalLocalVariable" id="final-local-variable" disabled="true"/>

        <!-- This ImportControl check is NOT overriding the parent because id didn't match. -->
        <module name="ImportControl" id="import-control-other">
            <property name="file" value="path/to/import-control-other.xml"/>
        </module>

        <!-- This ImportControl check is overriden because id matched. -->
        <!-- The 'file' property is changed and 'path' property is lost -->
        <module name="ImportControl" id="import-control">
            <property name="file" value="path/to/import-control.xml"/>
        </module>

        <!-- LineLength is overriden -->
        <!-- 'ignorePattern' property is inherited, 'max' property is changed -->
        <module name="LineLength" id="line-length">
            <property name="ignorePattern" inherit="true"/>
            <property name="max" value="150"/>
        </module>

        <!-- NoWhiteSpaceAfter is overriden -->
        <!-- 'tokens' property is appended -->
        <module name="NoWhitespaceAfter" id="no-whitespace-after">
            <property name="tokens" inherit="true"/>
            <property name="tokens" value="LNOT"/>
            <property name="tokens" value="DOT"/>
        </module>
    </module>
</module>

[rnveach]

When a module is overriden, all properties are lost and have to be configured again
<property name="ignorePattern" inherit="true"/>

Shouldn't the default be all properties are inherited?
If a module is a class and properties are methods (which they actually are), when I extend a class I expect to inherit all the methods of the that class. I don't expect to have to hand write them over again.
What happens if we add a new property defined in the parent config but it is not written in the child? Is the parent property ignored and only default value is used?

<!-- This ImportControl check is NOT overriding the parent because id didn't match. -->

What happens then? Is it ignored or is it added as a new check instance?
You also do not provide an example of how to add a new check that the parent doesn't have. I expect most people will want to add additional checks and not just change the original ones.

Parent: <module name="TreeWalker">

Same as previous. What happens that there is no inherit on TreeWalker but there are inherits on checks.

Parent: <module name="Checker">
Child: <module name="Checker">

Root module can be changed by user, how will you handle if parent and child request different root modules?

<module name="FinalLocalVariable" id="final-local-variable" disabled="true"/>

Disabled is like setting the severity to ignore. Should we require users to do that instead of adding this additional property?

Parent: <module name="EqualsAvoidNull" id="equals-avoid-null"/>
Modules without an id are never overriden.

This is the main issue with this example. It requires IDs on all modules in the parent config. Most users do not write their config this way. We only started this in google and sun configs and only to allow suppressions on specific check when multiples are defined. I wouldn't expect most people to make this kind of drastic change anytime soon and what kind of name would they give to the most of generic checks.

@jochenvdv To keep this condensed, how about you update your original post with the more details I mentioned?

---

Most of the example by @jochenvdv is what I was thinking of too, but with some slight differences.

Here is an example of my idea. Names can be changed.
````xml

<!-- Disable check by name and position. Order should be guaranteed by XML layout. 
  In this example this is the same as 'RightCurlyAlone' ID in parent. In terms of grandparents, 
  order is first grandparent, and then parent. Nothing happens if check not found. -->
<disable module="RightCurly" position="1"/>

<!-- Disable check by ID. IDs must be unique. If they are not unique, then outcome 
  is unpredictable. Nothing happens if id not found. -->
<disable id="RightCurlySame"/>

<!-- Enable check by ID that was previously disabled. This should be the same 
  as setting 'severity' to the configuration's default 'severity'. IDs must be unique. 
  If they are not unique, then outcome is unpredictable. Nothing happens if id not 
  found. -->
<enable id="RightCurlySame"/>
<!-- module and position are valid attributes for enable -->

<!-- Change property for check by name. Doesn't matter if property is defined 
  in parent or not. Nothing happens if check not found. -->
<override module="EmptyLineSeparator" property="allowNoEmptyLineBetweenFields"
  value="false"/>
<!-- if an invalid property is defined, default behavior should happen (Exception) -->
<!-- position is valid attribute for override -->

<!-- Add new multiples to an existing property for a check. Doesn't matter if 
  property is defined in parent or not, defaults to the same as an override. Nothing 
  happens if check not found. -->
<append module="NonExistantCheckName" property="tokens" value="STAR"/>
<!-- if an invalid property is defined, default behavior should happen (Exception) -->
<!-- id and position are valid attributes for append -->

<!-- If check is not mentioned, it is used 'as is' from parent. -->

````
We can change design if needed.
We might want a CLI option to print out current config without inheritance so users can be clear on final result and for our debugging purposes.

[jochenvdv]

@rnveach I agree with all of your remarks and I like your example a lot. It requires more work for implementation and documentation but it solves all the issues with my proposed solution.

You say that duplicate id's will lead to unpredictable outcome but can't we simply throw an error if we detect this while reading the configuration files?

I think we should also add re-enabling grandparent modules that were disabled in parent config (with enable element)

[rnveach]

duplicate id's will lead to unpredictable outcome but can't we simply throw an error if we detect this while reading the configuration files?

Part of me was thinking of JavaScript/HTML when writing it, as it too has unpredictable results when duplicate ids are defined. Also the 'no exception' result went with the other non-errors situations like 'disabling a check that doesn't exist'. I didn't really want users to be negatively affected if owner of parent config decides to make changes in future revisions.

But it is mostly just an example, and yes we can change it to throw an exception or have some other predictable result.

I think we should also add re-enabling grandparent modules that were disabled in parent config

If we set disabled check's severity to ignore, then it can be enabled by just changing the property to a different severity. enable can be added too, to set it to the default config severity which we can't define otherwise.

I'll add it to my example.

[rnveach]

@romani Do you have an opinion of my presentation on how to allow inheritance in configurations?

[sanyarnd]

If anyone wants to avoid copy-paste, here's solution I came up with.

First add the standard execution for sun_checks.xml (or google_checks.xml) with suppression filter:
pom.xml:

<execution>
    <id>sun-validate</id>
    <phase>validate</phase>
    <goals>
        <goal>check</goal>
    </goals>
    <configuration>
        <configLocation>sun_checks.xml</configLocation>
        <suppressionsLocation>checkstyle/sun_suppressions.xml</suppressionsLocation>
    </configuration>
</execution>

sun_suppressions.xml:

<?xml version="1.0"?>
<!DOCTYPE suppressions PUBLIC "-//Puppy Crawl//DTD Suppressions 1.0//EN"
    "http://www.puppycrawl.com/dtds/suppressions_1_0.dtd">
<suppressions>
    <suppress checks="LineLength" files="."/>
</suppressions>

Suppress all unwanted standard checks.

Then create (if required) overridden versions along with your own checks in custom checkstyle.xml:

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN" 
    "https://checkstyle.org/dtds/configuration_1_3.dtd">
<!--
    Additional style checks, which are not present in sun_checks.xml
-->
<module name="Checker">
        <module name="LineLength">
            <property name="max" value="150"/>
        </module>
</module>

Add the second execution point:

<execution>
    <id>custom-validate</id>
    <phase>validate</phase>
    <goals>
        <goal>check</goal>
    </goals>
    <configuration>
        <configLocation>checkstyle/checkstyle.xml</configLocation>
    </configuration>
</execution>

Complete pom.xml:

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-checkstyle-plugin</artifactId>
    <version>${maven-checkstyle-plugin.version}</version>
    <dependencies>
        <dependency>
            <groupId>com.puppycrawl.tools</groupId>
            <artifactId>checkstyle</artifactId>
            <version>${checkstyle.version}</version>
        </dependency>
    </dependencies>
    <executions>
        <execution>
            <id>sun-validate</id>
            <phase>validate</phase>
            <goals>
                <goal>check</goal>
            </goals>
            <configuration>
                <configLocation>sun_checks.xml</configLocation>
                <suppressionsLocation>checkstyle/sun_suppressions.xml</suppressionsLocation>
            </configuration>
        </execution>
        <execution>
            <id>custom-validate</id>
            <phase>validate</phase>
            <goals>
                <goal>check</goal>
            </goals>
            <configuration>
                <configLocation>checkstyle/checkstyle.xml</configLocation>
            </configuration>
        </execution>
    </executions>
</plugin>

The only serious drawback if checkstyle error == compilation error is that your own checks won't be visible (because execution won't be executed) until you fix all standard checks or visa versa (depends on execution order).

[romani]

@rnveach ,
you variant is too big update for XML structure and will affect all modules, as all is module in Checkstyle.

I am more leaning to solution of @jochenvdv with modification

<module name="Checker">
    <!-- This configuration inherits from parent.xml -->
    <property name="parent" value="path/to/parent.xml"/>
    <!-- it is same as "disable" -->
    <property name="parent-modules-to-exclude" value="final-local-variable,RightCurlySame"/>

and all modules are merging from parent to current config AND inherited modules loose if matching by name or id module present in config.
Concept: in parent config you can disable whole module or override whole module.

If user want to modify single property, it has to override whole module config (no ability to override certain property).
I think this will resolve most users needs, and in future, if users express more interest, we can think on more granular override with bigger design modification for config.

Problem: what to do with TreeWalker (composite module) ?
Solutions: we can ask user to keep copy of such composite properties in sub-config. All Child modules of composite-module are keep merging.

[romani]

@pbludov , @strkkk , @esilkensen please share your ideas on this.
@rdiachenko , please share your experience of Google style override/customization.

[rdiachenko]

@romani
Nothing interesting. Here's the only difference at the moment between our custom config and the original one:
original

    <module name="MissingJavadocMethod">
      <property name="scope" value="public"/>
      <property name="minLineCount" value="2"/>
      <property name="allowedAnnotations" value="Override, Test"/>
      <property name="tokens" value="METHOD_DEF, CTOR_DEF, ANNOTATION_FIELD_DEF,
                                   COMPACT_CTOR_DEF"/>
    </module>

custom

    <!-- Disabled due to ridiculous number of violations. The team is not ready for this check -->
    <!--
    <module name="MissingJavadocMethod">
      <property name="scope" value="public"/>
      <property name="minLineCount" value="2"/>
      <property name="allowedAnnotations" value="Override, Test"/>
      <property name="tokens" value="METHOD_DEF, CTOR_DEF, ANNOTATION_FIELD_DEF,
                                   COMPACT_CTOR_DEF"/>
    </module>
    -->