Google style has improper enforcement of horizontal whitespace

[shashwatj07]

Leftover from #7988 Reference: https://github.com/checkstyle/checkstyle/pull/7988#discussion_r410706795

Add all tokens left to be added and make google_checks adhere to Google style's chapter 4.6.2 https://google.github.io/styleguide/javaguide.html#s4.6.2-horizontal-whitespace

the colon (:) in an enhanced for ("foreach") statement

Between any content and a double slash (//) which begins a comment. Multiple spaces are allowed.

The following code should show violations wherever pointed.

class A {
    public static void main(String args[]) {
        for (int i:ll) { // violation as per 4.6.2.4
            int k;
        }
        List<String>list; // violation as per 4.6.2.7
        String g = "Google"; //google
        // the line above: violation as per 4.6.2.6, should be "// google", space after "//"
    }
}

C:\Users\Shashwat\Documents>java -jar checkstyle-8.31-all.jar -c config.xml Test.java
Starting audit...
Audit done.

[shashwatj07]

I've listed down the list of stuff we need to do.

Point 1: This if for for (int i:ll) { // violation as per 4.6.2.4 We can tweak WhitespaceAround to consider COLON that are a part of for each.

Point 2: This is for List<String>list; // violation as per 4.6.2.7 This extends to another case like int[]arr; We can add token support for RBRACK and GENERIC_END to WhitespaceAfter to tackle this. A point to note here will be that we don't require the check to log cases like this when the tokens are followed by comma. https://github.com/checkstyle/checkstyle/blob/5f86c897ee5619588f91bd10925cf7c4d0fd06db/src/test/resources/com/puppycrawl/tools/checkstyle/checks/whitespace/whitespaceafter/InputWhitespaceAfterGenerics.java#L6 and like this when the token is followed by [ https://github.com/checkstyle/checkstyle/blob/5f86c897ee5619588f91bd10925cf7c4d0fd06db/src/test/resources/com/puppycrawl/tools/checkstyle/checks/whitespace/whitespaceafter/InputWhitespaceAfterAround.java#L26

Point 3. This is for //google We can solve this by adding SINGLE_LINE_COMMENT token to WhitespaceAround. I've tried this in the linked PR #8165 , but I'm getting build errors due to NullPointerException when it is unable to read the input test files. Please help me how to fix this.

Please provide feedback and suggest better ways to resolve this issue if possible. @romani @rnveach @strkkk

[shashwatj07]

We can solve this by adding SINGLE_LINE_COMMENT token to WhitespaceAround.

Since WhitespaceAround is not for comments, will it be okay to solve this issue using RegexpSingleline? I'm suggesting this:

    <module name="RegexpSingleline">
        <property name="id" value="whitespaceAroundDoubleSlash"/>
        <property name="format" value="(.*[^ ]\/\/[^ ])|(.*\/\/[^ ])|(.*[^ ]\/\/)"/>
        <property name="minimum" value="0"/>
        <property name="maximum" value="0"/>
        <prperty name="ignoreCase" value="false"/>
        <prperty name="fileExtensions" value="java"/>
        <property name ="message" value="'//' that begins an end-of-line comment must have whitespace on both sides."/>
    </module>

@romani @rnveach

[shashwatj07]

@romani @rnveach ping

[strkkk]

Fix for enhanced for is merged

[shashwatj07]

@strkkk please respond to https://github.com/checkstyle/checkstyle/issues/8122#issuecomment-619896869

[strkkk]

@shashwatj07 Probably singleline regexp will work, but it is very error-prone approach, as for me. May be it is better to extend list of tokens with single line comment token.

[shashwatj07]

@strkkk But the check whitespacearound is not meant for comments, right? Could you please look at the linked draft PR?

[strkkk]

@shashwatj07 as for me, it looks good. The only thing that probably this should be excluded from default tokens set.

[romani]

<property name="id" value="whitespaceAroundDoubleSlash"/>

we should demand space after "//", we do this alredy in our codebase: https://github.com/checkstyle/checkstyle/blob/dfec36a68198f335475976c47ca7c4f2dd553603/config/checkstyle_checks.xml#L639-L644 it will resolve not all cases, but most of them, unresolved is leading space before //

https://github.com/checkstyle/checkstyle/issues/8137

[mohitsatr]

import java.util.ArrayList;

/** some javadoc. */
public class SwitchesIn {

  /** some javadoc. */
  public static void main(String[] args) {
    int[] ll = {1, 3, 2, 2};
    for (int num:ll) {                                         // OK
      System.out.println(num);
    }
    ArrayList<String>listOfNames = new ArrayList<String>();    // OK
    String google = "Google"; //google                         // EXPECTED VIOLATION
  }
}


$ java -jar testing/checkstyle-10.23.0-all.jar -c testing/google_checks.xml SwitchesIn.java 
Starting audit...
[WARN] /mnt/5D92528E6B945467/test/SwitchesIn.java:9:17: WhitespaceAround: ':' is not followed by whitespace. Empty blocks                may only be represented as {} when not part of a multi-block statement (4.1.3) [WhitespaceAround]
[WARN] /mnt/5D92528E6B945467/test/SwitchesIn.java:9:17: WhitespaceAround: ':' is not preceded with whitespace. [WhitespaceAround]
[WARN] /mnt/5D92528E6B945467/test/SwitchesIn.java:12:21: GenericWhitespace '>' should followed by whitespace. [GenericWhitespace]
Audit done.

Checkstyle now shows proper violation on the cases for which the issue is raised except for the last case. //google we expect a space after //.

[mohitsatr]

@romani @Zopsss how should we demand space after double slashes ? new check or regexp https://github.com/checkstyle/checkstyle/issues/8122#issuecomment-715949223

[Zopsss]

TodoComment would not be much of use here, I think we can accomplish this with MatchXpath

[Zopsss]

On the second thought, TodoComment maybe useful, first let's try using it and use custom violation message instead of the default one. Use a regex which checks there's no space after //, if regex matches we log the violation.

If TodoComment is not able to do it then we'll use MatchXpath as second approach.

[romani]

@mohitsatr , please create separate issues for each case/point in this issue, each of them will be handled separately it is not same fix for all of them. Please use CLI of formatter to prove or show if formatter might conflict with checkstyle

[mohitsatr]

@Zopsss for some reason TodoComment won't show violations even when pattern matched, So I tried using it with RegexpMultiline https://github.com/checkstyle/checkstyle/pull/17206

[mohitsatr]

@romani we can close this issue. all the cases are covered

https://github.com/checkstyle/checkstyle/issues/8122#issuecomment-2924072112

https://github.com/checkstyle/checkstyle/issues/17193

[romani]

fixed in some release before or at 10.23.0