Checkstyle ignores to be defined in source code

Development
1

Hi devs,

Right now when we want to delay fixing checkstyle errors (not good, but sometimes it’s needed temporarily…) or if there are false positives (I don’t think we have that with checkstyle, do we?), we use a checkstyle-suppressions.xml file.

For example:

<!DOCTYPE suppressions PUBLIC
    "-//Puppy Crawl//DTD Suppressions 1.0//EN"
    "http://www.puppycrawl.com/dtds/suppressions_1_0.dtd">

<suppressions>
  <suppress checks="BooleanExpressionComplexity" files="Base64BinaryStringEncoder.java" lines="59-60"/>
  <suppress checks="BooleanExpressionComplexity" files="HexBinaryStringEncoder.java" lines="58-59"/>
  <suppress checks="BooleanExpressionComplexity" files="UrlBase64BinaryStringEncoder" lines="46-47"/>
</suppressions>

After discussing with other devs on matrix, I’d like to propose that we change this practice to use exclusions inside the source files themselves and drop the usage of checkstyle-suppressions.xml files.

Here are the rules I’m proposing:

  • If the issue affects the whole class/interface/etc, use @SuppressWarnings("checkstyle:<rule name here>") on the class/interface/etc

  • If the issue affects a whole method, use @SuppressWarnings("checkstyle:<rule name here>") on the method

  • If the issue affects a variable declaration, use @SuppressWarnings("checkstyle:<rule name here>") on the variable declaration

  • If the issue affects a line (basically whenever an annotation is not allowed), use // CHECKSTYLE IGNORE <rule name> or // CHECKSTYLE IGNORE <rule name> FOR <n> LINES.

    Example:

      @Override
  InternalBinaryStringEncoder getEncoder()
  {
      return new AbstractBouncyCastleInternalBinaryStringEncoder(new HexEncoder(), BLOCK_SIZE, CHAR_SIZE)
      {
          @Override
          public boolean isValidEncoding(byte b)
          {
              // CHECKSTYLE IGNORE BooleanExpressionComplexity
              return ((b >= 0x2f && b <= 0x39) || (b >= 0x41 && b <= 0x46) || (b >= 0x61 && b <= 0x66));
          }
      };
  }

This proposal would be a must for new code and a best effort to fix existing excludes and move from the file usage to the code usage.

WDYT?

Thanks

PS1: I’ll later propose something for SonarCloud too.
PS2: We can configure the wording for the comment excludes. For now I’ve configured checkstyle with:

    <module name="SuppressWithNearbyCommentFilter">
      <property name="commentFormat" value="CHECKSTYLE IGNORE (\w+)"/>
      <property name="checkFormat" value="$1"/>
      <property name="influenceFormat" value="1"/>
    </module>

    <module name="SuppressWithNearbyCommentFilter">
      <property name="commentFormat" value="CHECKSTYLE IGNORE (\w+) FOR (\d+) LINES"/>
      <property name="checkFormat" value="$1"/>
      <property name="influenceFormat" value="$2"/>
    </module>
2

Note that we can also optionally add an explanation.

For example the following works:

@Override
public boolean isValidEncoding(byte b)
{
    // CHECKSTYLE IGNORE BooleanExpressionComplexity FOR 2 LINES
    //   Cryptography requires complex expressions, allow a few such expressions
    return (b == 0x2b || b == 0x3d || (b >= 0x2f && b <= 0x39)
        || (b >= 0x41 && b <= 0x5a) || (b >= 0x61 && b <= 0x7a));
}

Maybe there’s a cleaner way to achieve this. Let’s discuss.

3

+1 in general for moving as much as we can to the source

I think I would prefer a syntax which looks a bit more technical to make it even more obvious that it’s not a documentation comment, like

// CSIGNORE:BooleanExpressionComplexity

(also shorter because I’m lazy).

4

The syntax is to be discussed, we could also use:

  • // CHECKSTYLE:OFF <rule name>
  • // CHECKSTYLE:OFF <rule name> FOR <n> LINES
  • // CHECKSTYLE:OFF(n) <rule name>
  • etc.
5

+1, thanks

6

If we don’t care about being explicit, I’d prefer:

  • // CHECKSTYLE:BooleanExpressionComplexity
  • // CHECKSTYLE:BooleanExpressionComplexity(n)

EDIT: But I don’t mind, what you proposed it ok for me too!

EDIT2: using // CHECKSTYLE:<rule id> is a bit more in line with @SuppressWarnings("checkstyle:<rule id>"). We could also use // @SuppressWarnings("checkstyle:<rule id>") if we wanted to use the same syntax but extend it to be valid on any line.

7

Honestly, as long as it looks technical, and it’s not too long, it’s fine with me.

Another possibility (probably a bit too extreme):

// CS:<rule id>

Yes, I though about this too. It’s not short (but compensated by the IDE completion, before putting the // in front), but it’s consistent and it allows copy/pasting the same syntax whatever the context (easier to remember). So it’s fine with me too.

8

globally +1 with the proposal

sounds good to me: I prefer that we keep the full word CHECKSTYLE than something shorter to help newcomers understanding what it’s about.

9

+1 for that one as well, thanks.

10

Sounds good to me too, +1

11

Thanks all, this best practice has now been agreed and I’ve documented it at https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/#HQualitycheckignores

It’s been implemented in [Misc] Allow using @SuppressWarnings annotations + comments to ignore… · xwiki/xwiki-commons@0c70b11 · GitHub (with an example of migrating some existing ignores to the code).

1 Like
12

One potential downside of this best practice is that it’ll make it easier to ignore checkstyle rules (and we’re not supposed to ignore any checkstyle rules, we’re supposed to always fix them). We also need a strategy to fix the temporary ignores…

It was easier to review a single checkstyle-suppressions.xml file than searching for scattered comments or @SuppressWarnings annotations.

13

I forgot one thing: In addition to the annotation or comment, we also need to add a comment to explain WHY it’s ignored.

Examples:

With Annotation:

// Utility class with lots of features
@SuppressWarnings("checkstyle:ClassFanOutComplexity")
public final class XMLUtils
{
...

With comments:

            @Override
            public boolean isValidEncoding(byte b)
            {
                // CHECKSTYLE:BooleanExpressionComplexity(2)
                //   Cryptography requires complex expressions, allow a few such expressions
                return (b == 0x2b || b == 0x3d || (b >= 0x2f && b <= 0x39)
                    || (b >= 0x41 && b <= 0x5a) || (b >= 0x61 && b <= 0x7a));
            }

WDYT?

14

+1

15

I’ve now updated the doc with the comment format, see https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/#HCheckstyle