Checkstyle config review

Development
1

Hi devs,

In the context of Loading... , I’ve done a quick review of our checkstyle config with the help of a LLM, to review:

  • The status of the commented out checks we have in our config file
  • To see if we could add some more

In both cases, I’ve asked the LLM to run the mvn command and report how many failures found to help us choose low hanging fruits.

Here’s the result:

Complete Table: All 35 Tested Checkstyle Checks Across XWiki Projects

Check Name Category xwiki-commons xwiki-rendering xwiki-platform Currently in Config Notes
COMMENTED-OUT CHECKS
AvoidInlineConditionals Coding 5 0 116 Commented Ternary operators
ArrayTrailingComma Coding 0 0 0 Commented :white_check_mark: Safe to enable across all projects
ClassDataAbstractionCoupling Metrics 25 0 6 Commented (lines 113-116) High coupling in commons crypto/xml
DesignForExtension Design 32 0 954 Commented Major effort for platform
FinalLocalVariable Coding 114 0 1,675 Commented Very high effort, especially platform
FinalParameters Misc 153 0 3,276 Commented Highest violation count on platform
IllegalCatch Coding 8 0 95 Commented Catching Exception/Throwable
InterfaceIsType Design 0 0 0 Commented :white_check_mark: Safe to enable across all projects
MagicNumber Coding 1 0 129 Commented Low in commons, high in platform
MissingCtor Coding 14 0 231 Commented Moderate effort for platform
UnnecessaryParentheses Coding 3 0 55 Commented Low/moderate violations
SuperClone Coding 0 0 2 Commented Minimal violations
ThrowsCount Design 0 0 0 Commented :white_check_mark: Safe to enable across all projects
ImportOrder Imports 35 0 1,143 Commented Major effort for platform
RequireThis Coding 0 0 0 Commented :white_check_mark: Safe to enable across all projects
JavadocPackage Javadoc 30 0 0 Commented Only commons has violations (8 tool packages)
StrictDuplicateCode N/A N/A N/A N/A Commented (lines 53-55) :warning: Invalid check - remove from config
NEW CHECKS - GENERAL
EmptyCatchBlock Blocks 0 0 0 Not in config :white_check_mark: Safe to enable across all projects
EqualsAvoidNull Coding 0 0 14 Not in config Low violations on platform only
OneStatementPerLine Coding 0 0 0 Not in config :white_check_mark: Safe to enable across all projects
OverloadMethodsDeclarationOrder Coding 0 0 28 Not in config Low violations on platform only
UnusedLocalVariable Coding 0 0 1 Not in config Minimal violation
OneTopLevelClass Design 0 0 2 Not in config Minimal violation
InnerTypeLast Design 0 0 97 Not in config Moderate violations on platform
EmptyLineSeparator Whitespace 0 0 498 Not in config High violations on platform
NoLineWrap Whitespace 0 0 0 Not in config :white_check_mark: Safe to enable across all projects
AvoidDoubleBraceInitialization Coding 0 0 4 Not in config Minimal violation
LambdaParameterName Naming 0 0 0 Not in config :white_check_mark: Safe to enable across all projects
IllegalIdentifierName Naming 0 0 0 Not in config :white_check_mark: Safe to enable across all projects
NEW CHECKS - MODERN
AtclauseOrder Javadoc 1 0 0 Not in config Minimal violation in commons only
JavadocBlockTagLocation Javadoc 0 0 0 Not in config :white_check_mark: Safe to enable across all projects
InvalidJavadocPosition Javadoc 0 0 0 Not in config :white_check_mark: Safe to enable across all projects
AvoidNoArgumentSuperConstructorCall Coding 0 0 13 Not in config Low violations on platform only
CatchParameterName Naming 0 0 1 Not in config Minimal violation
NoWhitespaceBeforeCaseDefaultColon Whitespace 0 0 0 Not in config :white_check_mark: Safe to enable across all projects

Summary Statistics Across All Projects

Checks with Zero Violations on ALL Projects (14 checks): :white_check_mark:

  • ArrayTrailingComma
  • InterfaceIsType
  • ThrowsCount
  • RequireThis
  • EmptyCatchBlock
  • OneStatementPerLine
  • NoLineWrap
  • LambdaParameterName
  • IllegalIdentifierName
  • JavadocBlockTagLocation
  • InvalidJavadocPosition
  • NoWhitespaceBeforeCaseDefaultColon

Note: StrictDuplicateCode is invalid and should be removed.

Plus on commons/rendering only:

  • AtclauseOrder (only 1 violation in commons)

Checks with Minimal Violations (1-14 total across all projects - 10 checks):

  • AtclauseOrder (1)
  • SuperClone (2)
  • OneTopLevelClass (2)
  • UnusedLocalVariable (1)
  • CatchParameterName (1)
  • AvoidDoubleBraceInitialization (4)
  • EqualsAvoidNull (14)
  • AvoidNoArgumentSuperConstructorCall (13)

Breakdown by Project

xwiki-commons (Current Project)

  • Zero violations: 23 checks (66%)
  • 1-10 violations: 5 checks (14%)
  • 11-50 violations: 5 checks (14%)
  • 100+ violations: 2 checks (6%)

xwiki-rendering

  • Zero violations: 34 checks (97%)
  • Notes: Rendering maintains very strict code quality; many files excluded from checkstyle

xwiki-platform (Largest Codebase)

  • Zero violations: 14 checks (40%)
  • 1-100 violations: 11 checks (31%)
  • 100-1000 violations: 6 checks (17%)
  • 1000+ violations: 4 checks (11%)

Platform’s Highest Violations:

  1. FinalParameters: 3,276 violations
  2. FinalLocalVariable: 1,675 violations
  3. ImportOrder: 1,143 violations
  4. DesignForExtension: 954 violations
  5. EmptyLineSeparator: 498 violations

Final Recommendations (Prioritized Across All Projects)

Tier 1: Enable Immediately Across All Projects :white_check_mark: (14 checks, 0 violations everywhere)

These can be enabled with zero code changes across all three projects:

<!-- Add to TreeWalker section in checkstyle.xml -->
<module name="EmptyCatchBlock"/>
<module name="OneStatementPerLine"/>
<module name="NoLineWrap"/>
<module name="LambdaParameterName"/>
<module name="IllegalIdentifierName"/>
<module name="JavadocBlockTagLocation"/>
<module name="InvalidJavadocPosition"/>
<module name="NoWhitespaceBeforeCaseDefaultColon"/>

<!-- Uncomment existing checks -->
<module name="ArrayTrailingComma"/>
<module name="InterfaceIsType"/>
<module name="ThrowsCount"/>
<module name="RequireThis">
  <property name="checkMethods" value="false"/>
</module>

Tier 2: Enable After Minimal Fixes (1-14 violations across all projects - 10 checks)

Priority Order:

  1. AtclauseOrder (1 violation) - Fix 1 Javadoc in commons
  2. CatchParameterName (1 violation) - Fix 1 parameter name in platform
  3. UnusedLocalVariable (1 violation) - Remove 1 unused variable in platform
  4. SuperClone (2 violations) - Fix 2 clone methods in platform
  5. OneTopLevelClass (2 violations) - Move 2 classes in platform
  6. AvoidDoubleBraceInitialization (4 violations) - Refactor 4 instances in platform
  7. AvoidNoArgumentSuperConstructorCall (13 violations) - Remove 13 unnecessary super() calls in platform
  8. EqualsAvoidNull (14 violations) - Reverse 14 string comparisons in platform

Tier 3: Enable Per-Project with Moderate Planning

Commons-specific (low effort):

  • JavadocPackage (30) - Add package-info.java to 8 tool packages
  • ClassDataAbstractionCoupling (25) - Refactor 6 complex classes

Platform-specific (moderate-high effort):

  • OverloadMethodsDeclarationOrder (28)
  • UnnecessaryParentheses (55)
  • InnerTypeLast (97)
  • IllegalCatch (95)
  • AvoidInlineConditionals (116)
  • MagicNumber (129)
  • MissingCtor (231)
  • EmptyLineSeparator (498)

Tier 4: Requires Major Cross-Project Effort (Defer or Phased Approach)

These require significant refactoring across multiple projects:

  • DesignForExtension (commons: 32, platform: 954)
  • ImportOrder (commons: 35, platform: 1,143) - Requires import style configuration
  • FinalLocalVariable (commons: 114, platform: 1,675)
  • FinalParameters (commons: 153, platform: 3,276)

Recommendation: Consider if these style-only checks justify the massive effort, or implement gradually with suppression for existing code.

I propose implementing Tier 1 right now and then do Tier 2.

WDYT?

Thanks

2

I think an important question that the LLM didn’t answer is why the checks are failing and what would be necessary to fix them.

For example, I don’t understand why we should have so many import order violations as we do configure this in IDEs. Maybe Checkstyle doesn’t know about our import ordering? From what I see, it can be configured. If we have indeed many files with wrong import order, it should be very easy and safe to fix them automatically - if there is no existing tool, I’m sure an LLM can easily write us a quick tool. If we can ensure that a) the tool just re-orders lines and b) the file still compiles afterward, there is basically no risk for any regression. I think it would be important to enable import order validation, in particular with the use of coding agents it is easy to end up with wrong import order and having a Checkstyle rule for it would completely eliminate that problem (as Checkstyle is executed during the build and the coding agent would then fix any errors). We also frequently saw wrong import orders on PRs of new contributors.

If a rule is only violated in a few classes, it should be easy to enable it globally and disable it on these classes. A very safe change and still a win.

Also, for other violations, I think at least for some violations, coding agents completely change how easy or hard it is to apply something. If we agree that FinalParameters are something we want, I assume it shouldn’t be too difficult to just let a coding agent run and fix all violations unless we really modify parameter values.

While I don’t mind enabling rules that we’re currently not violating, I think for every rule, we as humans should review if we actually want the rule or not. Are all those rules in the list suggested by the Checkstyle project to be enabled by default?

3

That would be my main comment. We should enable rules we actually want, not rules for which we seem to not have that many violations.

4

ok let me ask it differently.

Are you ok that I add these to start with, using the default config for all the checks (I’ve checked them and I don’t see any reason to not use them):

Once agreed, I’ll suggest other tags.

You can check their meanings at Checks – checkstyle

Thanks

5

Looks good.

6

+1

Thanks,
Marius

7

@tmortagne / @mflorea I’m fixing quite a few InterfaceIsType – checkstyle checkstyle errors by using the following:

// Old interface not describing a type, hard to remove for backward-compatibility reasons.
@SuppressWarnings("checkstyle:InterfaceIsType")

I’d like to ensure that we’re really ok with this check. It means that in the future we should avoid creating interfaces to hold only constants. This has been used quite in a few places in the past, hence why I ask. I’m personally fine with it as I believe it means that we haven’t identified a proper Object/Type (ie with methods) on which we would put these constants.

Thanks

8

I’m fine with that too, never been a fan of this kind of interfaces (but It’s possible I added more constant to already existing ones).

9

ok good. FWIW SonarQube is also complaining with:

Move constants defined in this interfaces to another class or enum.

For ex: SonarQube Cloud

10

Applied in:

I’m now going to propose a new list of checks to add.

1 Like
11

WDYT about adding:

    <module name="AtclauseOrder">
      <property name="tagOrder" value="@param, @return, @throws, @exception,
        @see, @serial, @serialField, @serialData, @version, @since, @deprecated"/>
    </module>

This is about consistency.

Example:

/**
 * Represent a component.
 *
 * @param <T> the type of the component role
 * @version $Id: 0a9a9a1ee2706d396ac65b2713a48fe571c146e7 $
 * @since 1.7M1
 */

?

12

Note: I’m applying the UnusedLocalVariable check since nobody could be against that :slight_smile:

1 Like
13

+1, what happens to tags that are not part of the list?

14

Looks good. Not sure I ever seen any use of @serial, @serialField or @serialData.

15

Are you ok to use AvoidDoubleBraceInitialization? See the 2 articles linked to understand the problem.

I’ve tried it and fixed a few places in platform.

16

+1

17

Sounds good.

18

Done in:

Would be nice to have a validation of the changes brought in XWIKI-24148: Review and improve our checkstyle.xml config · xwiki/xwiki-platform@d1146be · GitHub

19

I’m also applying OneTopLevelClass which I feel is a no brainer.

20

Are you ok to apply EqualsAvoidNull?