google_checks.xml not allowing eol left curly for switch statement with lambda-like construct #15831

bradleylarrick · 2024-10-29T17:53:48Z

I'm getting the warning '(extension) LeftCurlyNl: '{' at column 20 should be on a new line' on a switch statement using lambda-like construct (line 7 below):

  for (int i = 0; i < buff.length(); i++) {
    final var ch = buff.charAt(i);
    switch (ch) {
      case 'e' -> buff.setCharAt(i, '3');
      case 'i' -> buff.setCharAt(i, '1');
      case 'o' -> buff.setCharAt(i, '0');
      default -> {
      }
    }
  }

Using the latest checkstyle-plugin and checkstyle versions:

<plugin>
	<groupId>org.apache.maven.plugins</groupId>
	<artifactId>maven-checkstyle-plugin</artifactId>
	<version>3.6.0</version>
	<dependencies>
		<dependency>
			<groupId>com.puppycrawl.tools</groupId>
			<artifactId>checkstyle</artifactId>
			<version>10.19.0</version>
		</dependency>
	</dependencies>
	<configuration>
		<configLocation>google_checks.xml</configLocation>
		<consoleOutput>true</consoleOutput>
		<failsOnError>true</failsOnError>
		<failOnViolation>true</failOnViolation>
		<linkXRef>false</linkXRef>
	</configuration>
</plugin>

The text was updated successfully, but these errors were encountered:

romani · 2024-10-29T18:02:30Z

@bradleylarrick, please reproduce by CLI it helps us a lot.

@Zopsss , please take a look

bradleylarrick · 2024-10-29T19:36:58Z

Ran

java -jar checkstyle-10.19.0-all.jar -c google_checks.xml FooBar.java

against this source file:

package org.natuna.tools;

/**
 * Test class.
 */
public class FooBar {

  /**
   * Constructor.
   */
  public FooBar() {
  }

  /**
   * Method.
   *
   * @return translated string
   */
  public String getPassword() {
    final var buff = new StringBuilder(32);
    buff.append("Password");

    for (int i = 0; i < buff.length(); i++) {
      final var ch = buff.charAt(i);
      switch (ch) {
        case 'e' -> buff.setCharAt(i, '3');
        case 'i' -> buff.setCharAt(i, '1');
        case 'o' -> buff.setCharAt(i, '0');
        default -> { 
        }
        // violation 2 lines above 
      }
    }

    return buff.toString();
  }
}

Returned:

Starting audit...
[WARN] C:\Users\bradley\temp\FooBar.java:29:20: '{' at column 20 should be on a new line. [LeftCurlyNl]
Audit done.

romani · 2024-10-29T20:22:46Z

This was fix 5b301c7#diff-8e4bfd0f5535171a324423e3cc959fc50f3c7759555973e6084110dae5f1fd20R22

This is intentional behavior and should matching to google-java-format.

@bradleylarrick , please recheck how https://github.com/google/google-java-format behave on your code. Is supposed to put { on new line. It is rule of Google style, we previously had defect in checkstyle on it, as it was added in style guide few years ago.

bradleylarrick · 2024-10-29T22:24:27Z

I ran the test class through google-java-format-1.24.0 and got this:

     1  package org.natuna.tools;
     2
     3  /** Test class. */
     4  public class FooBarNew {
     5
     6    /** Constructor. */
     7    public FooBarNew() {}
     8
     9    /**
    10     * Method.
    11     *
    12     * @return translated string
    13     */
    14    public String getPassword() {
    15      final var buff = new StringBuilder(32);
    16      buff.append("Password");
    17
    18      for (int i = 0; i < buff.length(); i++) {
    19        final var ch = buff.charAt(i);
    20        switch (ch) {
    21          case 'e' -> buff.setCharAt(i, '3');
    22          case 'i' -> buff.setCharAt(i, '1');
    23          case 'o' -> buff.setCharAt(i, '0');
    24          default -> {}
    25        }
    26      }
    27
    28      return buff.toString();
    29    }
    30  }

Running the audit on this new file returned this:

Starting audit...
[WARN] C:\Users\bradley\temp\FooBarNew.java:24:20: WhitespaceAround: '{' is not followed by whitespace. Empty blocks                may only be repres
ented as {} when not part of a multi-block statement (4.1.3) [WhitespaceAround]
[WARN] C:\Users\bradley\temp\FooBarNew.java:24:21: WhitespaceAround: '}' is not preceded with whitespace. [WhitespaceAround]
Audit done.

romani · 2024-10-30T05:25:36Z

@bradleylarrick, do you agree that we should fix false positives of WhitespaceAround? That we see in checkstyle execution after auto formatting.

Zopsss · 2024-10-30T06:05:58Z

Formatter converted

default -> { 
}

into

default -> {}

this might be a bug in the formatter, as it is not obeying style guide's rule

https://google.github.io/styleguide/javaguide.html#s4.1.2-blocks-k-r-style

Exception: In places where these rules allow a single statement ending with a semicolon (;), a block of statements can appear, and the opening brace of this block is preceded by a line break.

from our side, the LeftCurlyNl violation was correct.

Zopsss · 2024-10-30T06:22:56Z

@bradleylarrick, do you agree that we should fix false positives of WhitespaceAround? That we see in checkstyle execution after auto formatting.

We should fix this, this is a false-positive and we were unable to detect it at #15338

Zopsss · 2024-10-30T06:36:45Z

We have allowEmptyLambdas set to true

checkstyle/src/main/resources/google_checks.xml

Lines 127 to 130 in ca693c7

    
           <module name="WhitespaceAround"> 
        
             <property name="allowEmptyConstructors" value="true"/> 
        
             <property name="allowEmptyLambdas" value="true"/> 
        
             <property name="allowEmptyMethods" value="true"/>

but it is still giving error for it.

After modifying WhitespaceAround's suppression and adding LITERAL_DEFAULT as following:

$ diff -u og_suppression.xml modified_suppression.xml
--- og_suppression.xml  2024-10-30 12:03:24.085742000 +0530
+++ modified_suppression.xml    2024-10-30 12:03:38.686326100 +0530
@@ -1,7 +1,7 @@
 <module name="SuppressionXpathSingleFilter">
     <property name="checks" value="WhitespaceAround"/>
     <property name="query" value="//*[self::LITERAL_IF or self::LITERAL_ELSE or self::STATIC_INIT
-                                or self::LITERAL_TRY or self::LITERAL_CATCH]/SLIST[count(./*)=1]
-                                | //*[self::STATIC_INIT or self::LITERAL_TRY or self::LITERAL_IF]
+                                or self::LITERAL_TRY or self::LITERAL_CATCH or LITERAL_DEFAULT]/SLIST[count(./*)=1]
+                                | //*[self::STATIC_INIT or self::LITERAL_TRY or self::LITERAL_IF or LITERAL_DEFAULT]
                                 //*[self::RCURLY][parent::SLIST[count(./*)=1]]"/>
 </module>

The false-positive violation got suppressed.

/** Test. */
public class Test {
  /** Test. */
  public static void main(String[] args) {
    switch (ch) {
      default -> {}
    }
  }
}

$ java -jar .\checkstyle-10.19.0-all.jar -c .\google_checks.xml .\Test.java
Starting audit...
Audit done.

bradleylarrick · 2024-10-30T11:24:42Z

Agree on fixing false positive around WhitespaceAround.

romani · 2024-10-30T13:31:30Z

#9540

Looks like we need to activate this new property that we just released

romani · 2024-10-30T15:19:27Z

@bradleylarrick, can you try to change config on your local and run over all your sources to let us catch all use cases.

bradleylarrick · 2024-10-31T11:03:18Z

I ran google-java-format-1.24.0 on another projects, and the ran checkstyle-10.19.0. This code:

   310      public String prefix(final Calendar date) {
   311        int year = date.get(Calendar.YEAR);
   312        int month = date.get(Calendar.MONTH) + 1;
   313        int day = date.get(Calendar.DAY_OF_MONTH);
   314
   315        final var claimPrefix = new StringBuilder(pattern.length());
   316        for (int i = 0; i < pattern.length(); i++) {
   317          switch (pattern.charAt(i)) {
   318            case '\\' -> claimPrefix.append(pattern.charAt(++i)); // NOPMD - skipping the escape char
   319            case 'D' -> {
   320              claimPrefix.append((char) ('0' + (day % 10)));
   321              day /= 10;
   322            }
   323            case 'M' -> {
   324              claimPrefix.append((char) ('0' + (month % 10)));
   325              month /= 10;
   326            }
   327            case 'Y' -> {
   328              claimPrefix.append((char) ('0' + (year % 10)));
   329              year /= 10;
   330            }
   331            default -> {}
   332          }
   333        }
   334
   335        // Save the formatted prefix value
   336        currPrefix = claimPrefix.reverse().toString();
   337        return currPrefix;
   338      }

generated the following warnings:

[WARN] C:\projects\claim-service\src\main\java\org\natuna\claim\service\ClaimNumberFormatter.java:319:23: '{' at column 23 should be on a new line. [L
eftCurlyNl]
[WARN] C:\projects\claim-service\src\main\java\org\natuna\claim\service\ClaimNumberFormatter.java:323:23: '{' at column 23 should be on a new line. [L
eftCurlyNl]
[WARN] C:\projects\claim-service\src\main\java\org\natuna\claim\service\ClaimNumberFormatter.java:327:23: '{' at column 23 should be on a new line. [L
eftCurlyNl]
[WARN] C:\projects\claim-service\src\main\java\org\natuna\claim\service\ClaimNumberFormatter.java:331:22: WhitespaceAround: '{' is not followed by whi
tespace. Empty blocks                may only be represented as {} when not part of a multi-block statement (4.1.3) [WhitespaceAround]
[WARN] C:\projects\claim-service\src\main\java\org\natuna\claim\service\ClaimNumberFormatter.java:331:23: WhitespaceAround: '}' is not preceded with w
hitespace. [WhitespaceAround]

Zopsss · 2024-10-31T11:31:11Z

Line 319, 323, 327 violation seems correct to me.

Line 331 violations are false positives, I've provided solution for it in above comment or as @romani said in above comments, we can solve it by enabling allowEmptySwitchBlockStatements property

romani · 2024-10-31T13:25:09Z

we can solve it by enabling allowEmptySwitchBlockStatements property

If someone around keyboard and can send PR with fix, I can release it right after merging.

…rty of WhitespaceAround in google_checks.xml

Zopsss · 2024-10-31T14:24:12Z

@romani #15839

…rty of WhitespaceAround in google_checks.xml

…tespaceAround in google_checks.xml

romani · 2024-10-31T15:48:44Z

Thanks a lot @Zopsss , for quick fix.

@mahfouz72 , your feature is not only life but now in high usage.

@bradleylarrick , fix is released 10.20.0 , by same day delivery.
We appreciate it you use it how it works on your code base.

bradleylarrick · 2024-11-04T13:09:00Z

I found a couple more false positives.

   123      if (result.length > rpos) {
   124        switch (modulus) {
   125          case 1 -> { // one octet remaining; add two bits and load 2 quintets
   126            workarea = workarea << 2;
   127            rpos = loadCodes(workarea, result, rpos, 2);
   128          }
   129          case 2 -> { // two octet remaining; add four bits and load 4 quintets
   130            workarea = workarea << 4;
   131            rpos = loadCodes(workarea, result, rpos, 4);
   132          }
   133          case 3 -> { // three octets remaining; add one bit and load 5 quintets
   134            workarea = workarea << 1;
   135            rpos = loadCodes(workarea, result, rpos, 5);
   136          }
   137          case 4 -> { // four octets remaining; add 3 bits and load 7 quintets
   138            workarea = workarea << 3;
   139            rpos = loadCodes(workarea, result, rpos, 7);
   140          }
   141          default -> {} // Will never get here
   142        }
   143      }

Warnings were:

[WARN] C:\projects\commons\src\main\java\org\natuna\commons\Base32.java:125:19: '{' at column 19 should be on a new line. [LeftCurlyNl]
[WARN] C:\projects\commons\src\main\java\org\natuna\commons\Base32.java:129:19: '{' at column 19 should be on a new line. [LeftCurlyNl]
[WARN] C:\projects\commons\src\main\java\org\natuna\commons\Base32.java:133:19: '{' at column 19 should be on a new line. [LeftCurlyNl]
[WARN] C:\projects\commons\src\main\java\org\natuna\commons\Base32.java:137:19: '{' at column 19 should be on a new line. [LeftCurlyNl]

And:

   246      switch (modulus) {
   247        case 0 -> { // no leftovers
   248        }
   249        case 1, // 5 bits remaining; shouldn't occur from valid encoding, so ignore
   250                2 -> // 10 bits remaining; drop two and output one octet
   251            result[pos++] = (byte) ((workarea >> 2) & MASK_8BITS);
   252        case 3 -> // 15 bits remaining; shouldn't occur from valid encoding
   253            result[pos++] = (byte) ((workarea >> 7) & MASK_8BITS);
   254        case 4 -> // 20 bits remaining; drop the last four and load 2 bytes
   255            pos = loadBytes(workarea >> 4, result, pos, 2);
   256        case 5 -> // 25 bits remaining; drop the last bit and load 3 bytes
   257            pos = loadBytes(workarea >> 1, result, pos, 3);
   258        case 6 -> // 30 bits remaining; shouldn't occur from valid encoding
   259            pos = loadBytes(workarea >> 6, result, pos, 3);
   260        case 7 -> // 35 bits remaining; drop last three and load 4 bytes
   261            pos = loadBytes(workarea >> 3, result, pos, 4);
   262        default -> { // Will never get here
   263        }
   264      }

Generated:

[WARN] C:\projects\commons\src\main\java\org\natuna\commons\Base32.java:247:17: '{' at column 17 should be on a new line. [LeftCurlyNl]
[WARN] C:\projects\commons\src\main\java\org\natuna\commons\Base32.java:262:18: '{' at column 18 should be on a new line. [LeftCurlyNl]

romani · 2024-11-04T13:18:48Z

@bradleylarrick, please create new issue.
Please use CLI of checkstyle and Google java format.
In style guide Wording of { for cases/defaults is not very firm, but Google refers to their as source of truth formatter for all edge cases.

Zopsss added a commit to Zopsss/checkstyle that referenced this issue Oct 31, 2024

Issue checkstyle#15831: enabled allowEmptySwitchBlockStatements prope…

8c78fe9

…rty of WhitespaceAround in google_checks.xml

Zopsss mentioned this issue Oct 31, 2024

Issue #15831: enabled allowEmptySwitchBlockStatements property of WhitespaceAround in google_checks.xml #15839

Merged

romani changed the title ~~LeftCurly rule in google_checks.xml not allowing eol left curly for switch statement with lambda-like construct~~ google_checks.xml not allowing eol left curly for switch statement with lambda-like construct Oct 31, 2024

romani added approved bug labels Oct 31, 2024

Zopsss added a commit to Zopsss/checkstyle that referenced this issue Oct 31, 2024

Issue checkstyle#15831: enabled allowEmptySwitchBlockStatements prope…

60ab7cf

…rty of WhitespaceAround in google_checks.xml

romani pushed a commit that referenced this issue Oct 31, 2024

Issue #15831: enabled allowEmptySwitchBlockStatements property of Whi…

4c67922

…tespaceAround in google_checks.xml

romani closed this as completed in #15839 Oct 31, 2024

github-actions bot added this to the 10.20.0 milestone Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

google_checks.xml not allowing eol left curly for switch statement with lambda-like construct #15831

google_checks.xml not allowing eol left curly for switch statement with lambda-like construct #15831

bradleylarrick commented Oct 29, 2024

romani commented Oct 29, 2024

bradleylarrick commented Oct 29, 2024 •

edited by romani

Loading

romani commented Oct 29, 2024

bradleylarrick commented Oct 29, 2024

romani commented Oct 30, 2024

Zopsss commented Oct 30, 2024

Zopsss commented Oct 30, 2024

Zopsss commented Oct 30, 2024 •

edited

Loading

bradleylarrick commented Oct 30, 2024

romani commented Oct 30, 2024

romani commented Oct 30, 2024

bradleylarrick commented Oct 31, 2024

Zopsss commented Oct 31, 2024 •

edited

Loading

romani commented Oct 31, 2024

Zopsss commented Oct 31, 2024

romani commented Oct 31, 2024 •

edited

Loading

bradleylarrick commented Nov 4, 2024

romani commented Nov 4, 2024 •

edited

Loading

google_checks.xml not allowing eol left curly for switch statement with lambda-like construct #15831

google_checks.xml not allowing eol left curly for switch statement with lambda-like construct #15831

Comments

bradleylarrick commented Oct 29, 2024

romani commented Oct 29, 2024

bradleylarrick commented Oct 29, 2024 • edited by romani Loading

romani commented Oct 29, 2024

bradleylarrick commented Oct 29, 2024

romani commented Oct 30, 2024

Zopsss commented Oct 30, 2024

Zopsss commented Oct 30, 2024

Zopsss commented Oct 30, 2024 • edited Loading

bradleylarrick commented Oct 30, 2024

romani commented Oct 30, 2024

romani commented Oct 30, 2024

bradleylarrick commented Oct 31, 2024

Zopsss commented Oct 31, 2024 • edited Loading

romani commented Oct 31, 2024

Zopsss commented Oct 31, 2024

romani commented Oct 31, 2024 • edited Loading

bradleylarrick commented Nov 4, 2024

romani commented Nov 4, 2024 • edited Loading

bradleylarrick commented Oct 29, 2024 •

edited by romani

Loading

Zopsss commented Oct 30, 2024 •

edited

Loading

Zopsss commented Oct 31, 2024 •

edited

Loading

romani commented Oct 31, 2024 •

edited

Loading

romani commented Nov 4, 2024 •

edited

Loading