Issue #15769: fix false-positive indentation violations for block codes #16515

mohitsatr · 2025-03-10T03:15:29Z

Diff Regression config: https://gist.githubusercontent.com/mohitsatr/d55485b980e5c7ae9c883ba8f79dcbd6/raw/c824102d90368dbd060f41a1f6bda249cdcf0a5b/master_google_style.xml
Diff Regression patch 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-03-10T03:16:57Z

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

github-actions · 2025-03-10T03:32:22Z

Failed to download or process the specified configuration(s).
Error details:

Please ensure you've used one of the following commands correctly:

'GitHub, generate report for <config_bundle_path>'
'GitHub, generate report by config from <file_path>'
'GitHub, generate report for configs in PR description'

And check that all specified configs or files exist and are valid.

Link: https://github.com/checkstyle/checkstyle/actions/runs/13756086125

mohitsatr · 2025-03-10T05:19:31Z

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

github-actions · 2025-03-10T06:10:42Z

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

romani · 2025-03-12T15:46:36Z

@Zopsss , please do first review

Zopsss

item:

Zopsss · 2025-03-12T18:06:44Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/BlockParentHandler.java

+                && getParent() instanceof BlockParentHandler
+                && getParent().getParent() instanceof BlockParentHandler;


why getParent().getParent() is required?

this condition is important for handling switch-cases blocks present in InputIndentationSwitchCasesAndEnums.java

case ONE: (CaseHandler) { //indent:8 exp:8 System.out.println("One"); //indent:10 exp:10 break; //indent:10 exp:10 } //indent:8 exp:8

|--CASE_GROUP -> CASE_GROUP [24:6] | | |--LITERAL_CASE -> case [24:6] | | | |--EXPR -> EXPR [24:11] | | | | `--IDENT -> ONE [24:11] | | | `--COLON -> : [24:14] | | `--SLIST -> SLIST [25:8] | | `--SLIST -> { [25:8] | | |--EXPR -> EXPR [26:28]

without an extra check, checkifBlockCode returns True, causing a wrong indentation value. I just put this condition and it worked on all tests. In regression reports , we might find problems with it.

there is one weird behaviour. (not introduced by change in this PR)

Notice that 8 also happens to be valid indentation for 26 (System.out..) line . but isn't it wrong ? only 10 is valid, right ?

In regression reports , we might find problems with it.

yeah let's see how this performs in regression

but isn't it wrong ? only 10 is valid, right ?

yup, this seems like a bug. Nice catch, please create a new issue for it.

Zopsss · 2025-03-12T18:10:09Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/BlockParentHandler.java

-        if (!isOnStartOfLine(lcurly)
-            || lcurly.getParent().getType() == TokenTypes.INSTANCE_INIT) {
+        if (!isOnStartOfLine(lcurly) || checkIfCodeBlock()) {


is removing lcurly.getParent().getType() == TokenTypes.INSTANCE_INIT safe? can you tell me why was it used before?

it worked only for Instance code blocks like #16515 (comment) won't work for cases like-when a code block is inside a function body.
yes it is safe to remove it as new logic covers the above cases

Zopsss · 2025-03-12T18:12:08Z

.../puppycrawl/tools/checkstyle/checks/indentation/indentation/InputIndentationCodeBlocks1.java

+
+package com.puppycrawl.tools.checkstyle.checks.indentation.indentation; //indent:0 exp:0
+
+public class InputIndentationCodeBlocks1 { //indent:0 exp:0


please add following cases right after class begins:

{ //indent:2 exp:2 System.out.println("True"); //indent:4 exp:4 } //indent:2 exp:2 { //indent:4 exp:2 warn System.out.println("False"); //indent:6 exp:4 warn } //indent:4 exp:2 warn

mohitsatr · 2025-03-13T11:08:13Z

Github, generate report

github-actions · 2025-03-13T11:11:54Z

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

mohitsatr · 2025-03-13T11:13:48Z

first, we need to check regression on the project suggested here #14294 (comment)

report is ready #16515 (comment)

mohitsatr · 2025-03-14T06:42:39Z

@Zopsss in the regression config, we need to specify the google_style config as this issue is related to google-style right?

Zopsss · 2025-03-14T15:41:25Z

Yes, find diff between current master branch's google_checks.xml config and your modified google_checks.xml config. Check the generated report, if false positives/negatives are introduced then report them here and try to fix them

mohitsatr · 2025-03-15T10:04:09Z

Github, generate report

mohitsatr · 2025-03-16T03:22:27Z

@Zopsss report generation exceeded 6 hours limit and got cancelled twice https://github.com/checkstyle/checkstyle/actions/runs/13871818479 ??

Zopsss · 2025-03-16T06:53:16Z

Sorry, even I'm seeing this error for the first time.

@romani can you check what's the issue here?

mohitsatr · 2025-03-17T08:52:20Z

Github, generate report

romani · 2025-03-21T04:49:19Z

Please do not run on whole Google style config, just use intendtation module from it, nothing else.

mohitsatr · 2025-03-21T05:16:39Z

Github, generate report

github-actions · 2025-03-21T05:22:24Z

Report generation failed on phase "make_report",
step "Generate report".
Link: https://github.com/checkstyle/checkstyle/actions/runs/13985147713

mohitsatr · 2025-03-21T07:18:01Z

Github, generate report

github-actions · 2025-03-21T07:23:38Z

Report generation failed on phase "make_report",
step "Generate report".
Link: https://github.com/checkstyle/checkstyle/actions/runs/13986614683

mohitsatr · 2025-03-21T07:42:46Z

Error : Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.12.1:site (default-site) on project sample: Error generating maven-checkstyle-plugin:3.1.1:checkstyle report: Failed during checkstyle configuration: Exception was thrown while processing /home/runner/work/checkstyle/checkstyle/.ci-temp/contribution/checkstyle-tester/src/main/java/openjdk21/src/jdk.jlink/share/classes/module-info.java: IllegalStateException occurred while parsing file /home/runner/work/checkstyle/checkstyle/.ci-temp/contribution/checkstyle-tester/src/main/java/openjdk21/src/jdk.jlink/share/classes/module-info.java. 54:0: no viable alternative at input 'module': NoViableAltException -> [Help 1]

romani · 2025-03-21T13:10:03Z

module-info.java

please use https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/my_check.xml but module should one one , attention to 

mohitsatr · 2025-03-22T01:22:34Z

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name = "Checker">
    <property name="charset" value="UTF-8"/>

    <!-- do not change severity to 'error', as that will hide errors caused by exceptions -->
    <property name="severity" value="warning"/>

    <!-- haltOnException is required for exception fixes and reporting of all exceptions -->
    <property name="haltOnException" value="false"/>

    <!-- BeforeExecutionFileFilters is required for sources of java9 -->
    <module name="BeforeExecutionExclusionFileFilter">
        <property name="fileNamePattern" value="module\-info\.java$" />
    </module>

    <module name="TreeWalker">
         <!-- as we run on regression even on non-compiled files we need to skip exceptions on them -->
         <property name="skipFileOnJavaParseException" value="true"/>
         <property name="javaParseExceptionSeverity" value="ignore"/>

    
         
         <module name="Indentation">
             <property name="basicOffset" value="2"/>
             <property name="braceAdjustment" value="2"/>
             <property name="caseIndent" value="2"/>
             <property name="throwsIndent" value="4"/>
             <property name="lineWrappingIndentation" value="4"/>
             <property name="arrayInitIndent" value="2"/>
        </module>
         <!-- Suppression for block code until we find a way to detect them properly
         until https://github.com/checkstyle/checkstyle/issues/15769 -->
        <module name="SuppressionXpathSingleFilter">
           <property name="checks" value="Indentation"/>
           <property name="query" value="//SLIST[not(parent::CASE_GROUP)]/SLIST
                                   | //SLIST[not(parent::CASE_GROUP)]/SLIST/RCURLY"/>
        </module>

         <!-- suppress javadoc parsing errors, as we test Check not a parser -->
         <module name="SuppressionXpathSingleFilter">
            <property name="message" value="Javadoc comment at column \d+ has parse error"/>
         </module>
    </module>

</module>

@romani like this ?

romani · 2025-03-22T04:48:13Z

Yes

mohitsatr · 2025-03-22T11:58:40Z

Github, generate report

github-actions · 2025-03-22T16:10:40Z

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

romani · 2025-03-22T22:10:33Z

diff is huge, please start copy all code from diff report to Input to have all distinct cases that we missed in Inputs.
Ideally we should have all in Inputs, and depend less on diff report.

mohitsatr · 2025-03-23T03:32:55Z

report is weird. there are diffs in switch and case blocks. this report is based on google config and not every project uses it. so obviously there will be diffs.
only Hbase and camunda use google-style

mohitsatr · 2025-03-23T04:02:25Z

we have already checked the regression using default indentation config #16515 (comment)

Zopsss · 2025-03-23T10:16:55Z

only Hbase and camunda use google-style

Please go through their diff and add missing examples to our inputs. We can ignore other projects.

Also if you're going to generate more regression reports then only include projects which uses google-style, other projects will not help us to find edge cases.

mohitsatr · 2025-03-24T03:05:02Z

Github, generate report

github-actions · 2025-03-24T03:15:48Z

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

mohitsatr · 2025-03-24T03:37:02Z

Github, generate report

github-actions · 2025-03-24T03:56:04Z

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

mohitsatr · 2025-03-24T04:29:01Z

@Zopsss in camunda project, violations on switch expression are fixed in #16418. these diffs got removed that PR's regression report #16418 (comment)
As for the Hbase project, there are not any new violations. diff is due to change in violation message.

… block codes

mohitsatr · 2025-03-24T06:24:06Z

Github, generate report

github-actions · 2025-03-24T06:45:54Z

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

mohitsatr · 2025-03-25T02:37:29Z

like I said , there is no violation change in Hbase project, do I still need to add examples from it to our input ??

Zopsss · 2025-03-25T02:44:01Z

in camunda project, violations on switch expression are fixed in #16418. these diffs got removed that PR's regression report #16418 (comment)

Sorry I'm not able to understand why diff got removed. Were the violations in diff was false positives? And your PR fixed them?

As for the Hbase project, there are not any new violations. diff is due to change in violation message.

Can you point out the PR where the violation message was changed?

Zopsss · 2025-03-25T02:47:02Z

like I said , there is no violation change in Hbase project, do I still need to add examples from it to our input ??

Did we fixed some bug that's why the violation message was changed? Or is it something else?

If the diff doesn't have any false-postivie/negative and if you don't find an example which is not in our input files then you don't need to add any new example. We'll be good to go

romani · 2025-03-27T13:13:55Z

@mohitsatr , please fix compilation problem https://app.circleci.com/pipelines/github/checkstyle/checkstyle/31749/workflows/70da813c-4d76-4d3a-a16f-5e33572194d9/jobs/850139

mohitsatr force-pushed the codeblock-indentation branch from ffcf264 to fbfa4b1 Compare March 10, 2025 03:41

mohitsatr marked this pull request as ready for review March 12, 2025 03:24

Zopsss suggested changes Mar 12, 2025

View reviewed changes

Issue checkstyle#15769: fix false-positive indentation violations for…

78e1fb8

… block codes

mohitsatr force-pushed the codeblock-indentation branch from fbfa4b1 to 78e1fb8 Compare March 24, 2025 06:22

		&& getParent() instanceof BlockParentHandler
		&& getParent().getParent() instanceof BlockParentHandler;


		package com.puppycrawl.tools.checkstyle.checks.indentation.indentation; //indent:0 exp:0

		public class InputIndentationCodeBlocks1 { //indent:0 exp:0

Issue #15769: fix false-positive indentation violations for block codes #16515

Are you sure you want to change the base?

Issue #15769: fix false-positive indentation violations for block codes #16515

Conversation

mohitsatr commented Mar 10, 2025 • edited Loading

mohitsatr commented Mar 10, 2025 • edited Loading

github-actions bot commented Mar 10, 2025

mohitsatr commented Mar 10, 2025

github-actions bot commented Mar 10, 2025

romani commented Mar 12, 2025

Zopsss left a comment

Choose a reason for hiding this comment

Zopsss Mar 12, 2025

Choose a reason for hiding this comment

mohitsatr Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

mohitsatr Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

Zopsss Mar 14, 2025

Choose a reason for hiding this comment

Zopsss Mar 14, 2025

Choose a reason for hiding this comment

Zopsss Mar 12, 2025

Choose a reason for hiding this comment

mohitsatr Mar 25, 2025

Choose a reason for hiding this comment

Zopsss Mar 12, 2025

Choose a reason for hiding this comment

mohitsatr Mar 25, 2025

Choose a reason for hiding this comment

mohitsatr commented Mar 13, 2025

github-actions bot commented Mar 13, 2025

mohitsatr commented Mar 13, 2025 • edited Loading

mohitsatr commented Mar 14, 2025

Zopsss commented Mar 14, 2025 • edited Loading

mohitsatr commented Mar 15, 2025

mohitsatr commented Mar 16, 2025

Zopsss commented Mar 16, 2025

mohitsatr commented Mar 17, 2025

romani commented Mar 21, 2025

mohitsatr commented Mar 21, 2025

github-actions bot commented Mar 21, 2025

mohitsatr commented Mar 21, 2025

github-actions bot commented Mar 21, 2025

mohitsatr commented Mar 21, 2025

romani commented Mar 21, 2025

mohitsatr commented Mar 22, 2025

romani commented Mar 22, 2025

mohitsatr commented Mar 22, 2025

github-actions bot commented Mar 22, 2025

romani commented Mar 22, 2025

mohitsatr commented Mar 23, 2025 • edited Loading

mohitsatr commented Mar 23, 2025

Zopsss commented Mar 23, 2025

mohitsatr commented Mar 24, 2025

github-actions bot commented Mar 24, 2025

mohitsatr commented Mar 24, 2025

github-actions bot commented Mar 24, 2025

mohitsatr commented Mar 24, 2025 • edited Loading

mohitsatr commented Mar 24, 2025

github-actions bot commented Mar 24, 2025

mohitsatr commented Mar 25, 2025

Zopsss commented Mar 25, 2025

Zopsss commented Mar 25, 2025

romani commented Mar 27, 2025

mohitsatr commented Mar 10, 2025 •

edited

Loading

mohitsatr commented Mar 10, 2025 •

edited

Loading

mohitsatr Mar 14, 2025 •

edited

Loading

mohitsatr Mar 14, 2025 •

edited

Loading

mohitsatr commented Mar 13, 2025 •

edited

Loading

Zopsss commented Mar 14, 2025 •

edited

Loading

mohitsatr commented Mar 23, 2025 •

edited

Loading

mohitsatr commented Mar 24, 2025 •

edited

Loading