Issue #8965: fix false-negative on new keyword's children's Indentation #17181

mohitsatr · 2025-06-07T10:21:19Z

Diff Regression config: https://gist.githubusercontent.com/mohitsatr/60d8725143067033c0ee8c8b8c6902c0/raw/e585f76bf1fe45a3eab04329b4f3ae01766aa394/google_checks_codeblock.xml

Diff Regression projects: https://gist.githubusercontent.com/mohitsatr/68b0817ac52ff7773d35585ee12fc1cc/raw/f9c7774ca78df1f48f34bf30799bd3952abd9e2a/list-of-projects.properties

mohitsatr · 2025-06-07T10:22:46Z

.../tools/checkstyle/checks/indentation/indentation/InputIndentationCorrectIfAndParameter1.java

+                                                SecondClassLongNam7(getString(100000, "Long" //indent:48 exp:48
+                                                    + "Foo><"))) || conNoArg() //indent:52 exp:52
+                                                        || conNoArg() || //indent:56 exp:56
+                                                            conNoArg() || conNoArg()) {} //indent:60 exp:60


@Zopsss these two files go out of length limit.

is there way to change variables to be shorter and less crazy ?
if all such long names matters, we can add Input to suppression, but I think we should try shortening of Input, such creazy Input code is not readable and nobody can say what is reasonable indentation should be. It might be copy pasted from other Input.

Yup I also think this is very crazy input

Done. and it was not easy. 🥲

Zopsss · 2025-06-14T17:37:32Z

@mohitsatr any update on this?

mohitsatr · 2025-06-17T12:04:36Z

Github, generate report

github-actions · 2025-06-17T12:22:09Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/493aa59_2025121853/reports/diff/index.html

romani · 2025-06-18T13:27:16Z

@mohitsatr , can we make progress on this ?
we are about to release hot fix release for indentation, one update is already merged.

From checkstyle/checkstyle#17181

From: checkstyle/checkstyle#17181

mohitsatr · 2025-06-19T08:02:53Z

@romani
regression report is good. we can merge this as soon as these other PRs get merged.

mohitsatr · 2025-06-19T08:03:36Z

@Zopsss should I exclude and make InputFormatted.. file in this case too. ?

Zopsss · 2025-06-19T13:01:41Z

Yes. If we need violation and formatter complains for it then we need to exclude that file and make InputFormatted file for it so formatter can be happy.

From checkstyle/checkstyle#17181

From: checkstyle/checkstyle#17181

Zopsss · 2025-06-21T12:21:06Z

We should fix error for pgjdbc @mohitsatr

https://app.circleci.com/pipelines/github/checkstyle/checkstyle/34516/workflows/55b21dae-ed5c-457a-8f14-d5b149bfb09f/jobs/973041

mohitsatr · 2025-06-21T12:23:33Z

@Zopsss I fixed it pgjdbc/pgjdbc#3682.
CI needs to be restarted.

Zopsss · 2025-06-21T12:24:59Z

@romani can you restart CI?

romani · 2025-06-21T13:37:20Z

No need to restart, it is CS violations:

> Task :postgresql:checkstyleMain
[ant:checkstyle] [ERROR] /home/circleci/project/.ci-temp/pgjdbc/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java:74:9: 'new' child has incorrect indentation level 8, expected level should be 10. [Indentation]
[ant:checkstyle] [ERROR] /home/circleci/project/.ci-temp/pgjdbc/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java:75:9: 'new' child has incorrect indentation level 8, expected level should be 10. [Indentation]

> Task :postgresql:checkstyleMain FAILED

  static {
    try {
      Class.forName("org.postgresql.Driver");
    } catch (ClassNotFoundException e) {
      throw new IllegalStateException(
          "BaseDataSource is unable to load org.postgresql.Driver. Please check if you have proper PostgreSQL JDBC Driver jar on the classpath",
          e);
      // violation above and 2 lines above 
    }
  }

and

[INFO] --- antrun:3.1.0:run (ant-phase-verify) @ sevntu-checks --- 02:36
[INFO] Executing tasks 02:36
[INFO]      [echo] Checkstyle started (../../../config/checkstyle-checks.xml): 20/06/2025 03:30:47 AM 02:36
[INFO] [checkstyle] Running Checkstyle 10.25.1-SNAPSHOT on 141 files 02:38
[ERROR] [checkstyle] [ERROR] /home/semaphore/checkstyle/.ci-temp/sevntu.checkstyle/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/ChecksTest.java:88:29: 'new' child has incorrect indentation level 28, expected level should be 32. [Indentation] 02:48

https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/ca6d8c536254c0762e38ecac2e015578dd596a0c/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/ChecksTest.java#L85-L88

            validateEclipseCsMetaXmlFile(
                    new File(getEclipseCsPath(packageName
                            + "/checkstyle-metadata.xml")), packageName, new HashSet<>(
                            pkgModules));  // violation

I think all of them are false positives.

mohitsatr · 2025-06-22T05:18:59Z

I have added both the cases to the inputs.

for sevntu case,
I think there is mismatch in config values.
Notice, this file uses lineWrappingIndentation value of 8,

        for (String packageName : packages) {
            Assertions.assertTrue(new File(
                    getEclipseCsPath(packageName)).exists(), "folder " + packageName 
                    + " must exist in eclipsecs");   


            final Set<Class<?>> pkgModules = CheckUtil.getModulesInPackage(modules, packageName);


            validateEclipseCsMetaXmlFile(
                    new File(getEclipseCsPath(packageName
                            + "/checkstyle-metadata.xml")), packageName, new HashSet<>(
                            pkgModules));

but it is getting tested on lineWrappingIndentation of 4 from checkstyle-checks.xml

[INFO]      [echo] Checkstyle started (../../../config/checkstyle-checks.xml): 20/06/2025 03:30:47 AM 02:36  
                                                         ^^^
[INFO] [checkstyle] Running Checkstyle 10.25.1-SNAPSHOT on 141 files 02:38
[ERROR] [checkstyle] [ERROR] /home/semaphore/checkstyle/.ci-temp/sevntu.checkstyle/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/ChecksTest.java:88:29: 'new' child has incorrect indentation level 28, expected level should be 32. [Indentation] 02:48

lineWrappingIndentation has default value of 4.

checkstyle/config/checkstyle-checks.xml

Lines 813 to 818 in 512dfb4

    
           <module name="Indentation"> 
        
             <property name="basicOffset" value="4"/> 
        
             <property name="braceAdjustment" value="0"/> 
        
             <property name="caseIndent" value="4"/> 
        
             <property name="throwsIndent" value="8"/> 
        
           </module>

romani · 2025-06-22T05:34:18Z

Github, generate report

romani · 2025-06-22T05:35:51Z

Github, generate report for Indentation/all-examples-in-one

github-actions · 2025-06-22T05:41:00Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/adec420_2025054046/reports/diff/index.html

mohitsatr · 2025-06-22T06:08:14Z

no change in report.

github-actions · 2025-06-22T06:27:58Z

Report for Indentation/all-examples-in-one:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/adec420f62d6623a0db14d414ea13c0b86129121_2025062740/reports/diff/index.html

romani · 2025-06-22T19:00:02Z

@mohitsatr ,

Please add this simple code snippet to input.
We have crazy inputs, but we lack simple real code that slightly not correct.

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/adec420f62d6623a0db14d414ea13c0b86129121_2025062740/reports/diff/spoon/index.html#A1

romani

Minor test extension is required, but in general ready for merge

romani

I see it was added
https://github.com/checkstyle/checkstyle/pull/17181/files#diff-9fc42d7028c358e2830adba34b7df66af36bab1a72ef7b62f520660a08953bcfR62

mohitsatr · 2025-06-23T04:52:24Z

@romani was my reasoning right at #17181 (comment) ?

Zopsss · 2025-06-23T18:08:50Z

...e/checkstyle/test/chapter4formatting/rule4841indentation/InputNewKeywordChildrenCorrect.java

+  public Object foo() {
+    return Optional.empty()
+        .orElseThrow(
+            () ->
+                new IllegalArgumentException(
+                    "Something wrong 1, something wrong 2, something wrong 3"));
+  }


Can you also add a similar case like following:

public Object foo() { return Optional.empty() .orElseThrow( () -> new IllegalArgumentException( "Something wrong 1, something wrong 2, something wrong 3")); }

Sorry I'm commenting from phone so indentation might be wrong.

Also try to add a violation case with incorrect indentation for the last line.

https://google.github.io/styleguide/javaguide.html#s4.5.1-line-wrapping-where-to-break

A line is never broken adjacent to the arrow in a lambda or a switch rule, except that a break may come immediately after the arrow if the text following it consists of a single unbraced expression

It says a break "may" come, so I think it is not compulsory to have a break after -> so my suggested case should not give violation.

Also we should add this case to main indentation inputs too

Zopsss · 2025-06-24T04:46:38Z

...m/google/checkstyle/test/chapter4formatting/rule4841indentation/InputNewKeywordChildren.java

+            () -> new IllegalArgumentException(
+                "something wrong 1, something wrong 2, something wrong 3"));


Is the Indentation wrong here? "Something wrong 1.... shouldn't have 2 more spaces?

No, it is correct. lineWrappingIndentation is 4.
line 65 has indentation of 12, so line 66 have indentation of 16

Zopsss · 2025-06-24T04:47:25Z

...m/google/checkstyle/test/chapter4formatting/rule4841indentation/InputNewKeywordChildren.java

+  /** some data. */
+  public Object foo4(int data) {
+    return Optional.empty()
+        .orElseThrow(
+            () -> new IllegalArgumentException(
+                "something wrong 1, something wrong 2, something wrong 3"));
+  }
+
+  /** some data. */
+  public Object foo5(int data) {
+    return Optional.empty()
+        .orElseThrow(
+            () -> new IllegalArgumentException(
+        "something wrong 1, something wrong 2, something wrong 3"));
+    // violation above '.* incorrect indentation level 8, expected .* 16.'
+  }


Please add these cases to src/test/resources/com/puppycrawl/tools/checkstyle/checks/indentation/indentation/InputIndentationNewChildren.java as well

already did:

checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/indentation/indentation/InputIndentationNewChildren.java

Line 47 in ffeeb06

public Object foo4(int data) { //indent:2 exp:2

Please add correct case also

… Indentation

Zopsss · 2025-06-24T06:27:15Z

@romani ok to merge if ci passes

mohitsatr commented Jun 7, 2025

View reviewed changes

mohitsatr force-pushed the new-indent branch 3 times, most recently from ffd119e to 493aa59 Compare June 17, 2025 12:01

mohitsatr added a commit to mohitsatr/sevntu.checkstyle that referenced this pull request Jun 19, 2025

minor: update indentation of new's child

7612a59

From checkstyle/checkstyle#17181

mohitsatr mentioned this pull request Jun 19, 2025

supplemental: update indentation of new's child sevntu-checkstyle/sevntu.checkstyle#1043

Merged

mohitsatr added a commit to mohitsatr/pgjdbc that referenced this pull request Jun 19, 2025

fix indentation to let CI pass

f2750b0

From: checkstyle/checkstyle#17181

mohitsatr mentioned this pull request Jun 19, 2025

fix indentation to let CI pass pgjdbc/pgjdbc#3682

Merged

mohitsatr force-pushed the new-indent branch from 493aa59 to cc411a0 Compare June 19, 2025 08:01

mohitsatr added a commit to mohitsatr/sevntu.checkstyle that referenced this pull request Jun 20, 2025

supplemental: update indentation of new's child

e3dd585

From checkstyle/checkstyle#17181

mohitsatr force-pushed the new-indent branch 2 times, most recently from 8895496 to 7b9aecd Compare June 20, 2025 03:26

mohitsatr mentioned this pull request Jun 20, 2025

supplemental: adding inputs for 4.6.2 single whitespace around -> #17236

Merged

davecramer pushed a commit to pgjdbc/pgjdbc that referenced this pull request Jun 20, 2025

fix indentation to let CI pass (#3682)

6daee7d

From: checkstyle/checkstyle#17181

mohitsatr force-pushed the new-indent branch from 7b9aecd to adec420 Compare June 22, 2025 05:06

romani approved these changes Jun 22, 2025

View reviewed changes

mohitsatr force-pushed the new-indent branch 2 times, most recently from f1aedea to ec79307 Compare June 23, 2025 03:28

romani approved these changes Jun 23, 2025

View reviewed changes

romani assigned Zopsss Jun 23, 2025

Zopsss suggested changes Jun 23, 2025

View reviewed changes

mohitsatr force-pushed the new-indent branch 2 times, most recently from f187405 to ffeeb06 Compare June 24, 2025 04:08

Zopsss suggested changes Jun 24, 2025

View reviewed changes

Issue checkstyle#8965: fix false-negative on new keyword's children's…

94535b7

… Indentation

mohitsatr force-pushed the new-indent branch from ffeeb06 to 94535b7 Compare June 24, 2025 06:01

Zopsss approved these changes Jun 24, 2025

View reviewed changes

romani merged commit 846d71b into checkstyle:master Jun 24, 2025
119 checks passed

		() -> new IllegalArgumentException(
		"something wrong 1, something wrong 2, something wrong 3"));

Uh oh!

Issue #8965: fix false-negative on new keyword's children's Indentation #17181

Issue #8965: fix false-negative on new keyword's children's Indentation #17181

Uh oh!

Conversation

mohitsatr commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zopsss commented Jun 14, 2025

Uh oh!

mohitsatr commented Jun 17, 2025

Uh oh!

github-actions bot commented Jun 17, 2025

Uh oh!

romani commented Jun 18, 2025

Uh oh!

mohitsatr commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mohitsatr commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zopsss commented Jun 19, 2025

Uh oh!

Zopsss commented Jun 21, 2025

Uh oh!

mohitsatr commented Jun 21, 2025

Uh oh!

Zopsss commented Jun 21, 2025

Uh oh!

romani commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mohitsatr commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Jun 22, 2025

Uh oh!

romani commented Jun 22, 2025

Uh oh!

github-actions bot commented Jun 22, 2025

Uh oh!

mohitsatr commented Jun 22, 2025

Uh oh!

github-actions bot commented Jun 22, 2025

Uh oh!

romani commented Jun 22, 2025

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

mohitsatr commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zopsss commented Jun 24, 2025

mohitsatr commented Jun 7, 2025 •

edited

Loading

mohitsatr commented Jun 19, 2025 •

edited

Loading

mohitsatr commented Jun 19, 2025 •

edited

Loading

romani commented Jun 21, 2025 •

edited

Loading

mohitsatr commented Jun 22, 2025 •

edited

Loading

mohitsatr commented Jun 23, 2025 •

edited

Loading