Issue #18438: Fix false positive in EmptyLineSeparator check by Praveen7294 · Pull Request #18545 · checkstyle/checkstyle

Praveen7294 · 2026-01-07T15:26:13Z

Praveen7294 · 2026-01-10T04:45:20Z

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

github-actions · 2026-01-10T06:03:19Z

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

Praveen7294 · 2026-01-10T07:25:46Z

@Zopsss @romani i reviewed diff report everything looks fine.

Zopsss · 2026-01-12T10:32:29Z

+        DetailAST reqAST = result;
+        if (reqAST.getNextSibling() != null) {
+            final Optional<DetailAST> postFixNode = getPostFixNode(reqAST.getNextSibling());


what is reqAST?

reqAST is a temporary candidate node used during postfix handling.
It initially points to the current result, then allows the method to traverse into postfix-related AST structures (for example, method calls or increment expressions) without immediately overwriting result.

After the recursive traversal, reqAST is validated against the line constraint (reqAST.getLineNo() <= line). Only if it satisfies this condition is it promoted to result.

In short, reqAST acts as a safe checkpoint that holds a potential replacement for result while exploring postfix nodes, ensuring the final returned AST node still lies on or before the target line.

can you explain it with AST?

Also the reqAST name doesn't make much sense, you need to rename it to something more meaningful.

can you explain it with AST?

| |--SEMI -> ; [19:63] | |--EXPR -> EXPR [23:13] | | `--METHOD_CALL -> ( [23:13] | | |--DOT -> . [23:9] | | | |--SINGLE_LINE_COMMENT -> // [22:8] | | | | `--COMMENT_CONTENT -> violation 2 lines below 'There is more than 1 empty line after this line.'\r\n [22:10] | | | |--IDENT -> t [23:8] | | | `--IDENT -> foo [23:10] | | |--ELIST -> ELIST [23:14] | | | `--EXPR -> EXPR [23:14] | | | `--NUM_INT -> 10 [23:14] | | `--RPAREN -> ) [23:16] | |--SEMI -> ; [23:17]

Looking at the AST, there are two empty lines between the first SEMI token (; [19:63]) and the next EXPR token (EXPR [23:13]). Therefore, according to the method’s intent, the SEMI at line 19 should be identified as the last AST element before the empty lines.

if (result.getNextSibling() != null) { final Optional<DetailAST> postFixNode = getPostFixNode(result.getNextSibling()); if (postFixNode.isPresent()) { // A post fix AST will always have a sibling METHOD CALL // METHOD CALL will at least have two children // The first child is DOT in case of POSTFIX which have at least 2 children // First child of DOT again puts us back to normal AST tree which will // recurse down below from here final DetailAST firstChildAfterPostFix = postFixNode.orElseThrow(); result = getLastElementBeforeEmptyLines(firstChildAfterPostFix, line); } }

The postfix-handling logic inspects result.getNextSibling() and detects a method call expression. Because of this, it recursively descends into the method call AST and updates result to the SEMI at line 23 (; [23:17]). This overrides the correct result, even though that node appears after the empty lines, which leads to a false positive.

To avoid this, reqAST is introduced as a temporary candidate that initially refers to the current result. If a possible postfix or method call expression is found, traversal continues and reqAST is updated accordingly. Before promoting it to the final result, its line number is validated against the target line:

if (reqAST.getLineNo() <= line) { result = reqAST; }

This ensures that postfix traversal does not incorrectly cross empty-line boundaries and that the returned AST node truly lies on or before the required line.

Also the reqAST name doesn't make much sense, you need to rename it to something more meaningful.

Is candidateAST an appropriate name? If not, could you please suggest a more meaningful one?

can code

DetailAST reqAST = result; if (reqAST.getNextSibling() != null) { final Optional<DetailAST> postFixNode = getPostFixNode(reqAST.getNextSibling()); if (postFixNode.isPresent()) { // A post fix AST will always have a sibling METHOD CALL // METHOD CALL will at least have two children // The first child is DOT in case of POSTFIX which have at least 2 children // First child of DOT again puts us back to normal AST tree which will // recurse down below from here final DetailAST firstChildAfterPostFix = postFixNode.orElseThrow(); reqAST = getLastElementBeforeEmptyLines(firstChildAfterPostFix, line); } } if (reqAST.getLineNo() <= line) { result = reqAST; }

be written like:

result = positionToPotentialPostFixNode(result);

in this case comment can be moved to javadoc and logic will be a bit more structred.

@romani Thanks for the help, it is now more structred.

Look good now

Praveen7294 · 2026-01-20T13:00:43Z

@Zopsss please review, i explained as you asked.

romani · 2026-01-30T08:19:38Z

@Praveen7294 , please se my comment above.
Please rebase , sorry that we lost tracking of this PR.

please consider #18438 (comment) also.

romani · 2026-01-30T08:23:30Z

why we have this violation ?
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7ec4b2bad97c4c11059d33f0e846ca4ef5d6492a_2026053909/reports/diff/spring-framework/index.html#A1

is this different defect in same Check?

Praveen7294 · 2026-01-30T22:35:30Z

please se my comment above.
Please rebase , sorry that we lost tracking of this PR.

Done.

please consider #18438 (comment) also.

This false positive occurs when we add below property in EmptyLineSeparatorCheck, caught during #18311 :

<property name="allowMultipleEmptyLinesInsideClassMembers" value="false"/>

This property is not added in google_checks.xml. So there is no need to add inputs for google style.

Praveen7294 · 2026-01-31T09:26:08Z

why we have this violation ?
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7ec4b2bad97c4c11059d33f0e846ca4ef5d6492a_2026053909/reports/diff/spring-framework/index.html#A1

is this different defect in same Check?

@romani The red one is for the base, and the green one is for the patch. Both are the same defect, but in the base code the violation references the wrong line, while in the patch code the violation references the correct line.

romani · 2026-02-03T01:33:18Z

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

github-actions · 2026-02-03T02:18:54Z

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

romani · 2026-02-03T03:14:54Z

Ok, I checking violations all good.
But

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/96b8b50bbe62f142d0d4c1a228476862a61ea839_2026021830/reports/diff/spoon/index.html#A3 this looks like existing other problem.
Please check our issue tracker for it, if nothing, please create it.

romani · 2026-02-03T03:49:40Z

I reviewed all from bottom until Orekit, please go though top to Orekit to spot unusual case, and let me know when you finished

Praveen7294 · 2026-02-04T10:58:20Z

I reviewed all from bottom until Orekit, please go though top to Orekit to spot unusual case, and let me know when you finished

I reviewed from top to openjdk25, there is no unusual violation.

Praveen7294 · 2026-02-04T11:09:18Z

@romani

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/96b8b50bbe62f142d0d4c1a228476862a61ea839_2026021830/reports/diff/spoon/index.html#A3 this looks like existing other problem.

i reviewed and found there is no handling case of anonymous class with method chaining.

Below example:

    public static void foo1() {
        Stack<Integer> stack = new Stack<>();
        stack.push(5);
 	    new EarlyTerminatingScanner<Void>() { // violation 'There is more than one empty line after this line.'



 	    }
 	    .setVisitCompilationUnitContent(true)
 	    .scan(element.getRoleInParent(), element);
    }

Please check our issue tracker for it, if nothing, please create it.

This is a case of chained method call and the issue is about method call, so it's better to solve in this PR.

Sorry for the delay. I was working on this problem to identify the main cause.

romani · 2026-02-04T20:43:03Z

Please create issue, we can improve in separate issue/PR.

Praveen7294 · 2026-02-04T23:01:08Z

Please create issue, we can improve in separate issue/PR.

Done.

romani · 2026-02-05T00:49:19Z

But this #18545 (comment) is just comment that Check consider as empty line
Check is comment aware

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

Line 135 in c7da3ee

return true;

Not clear why it is new violation

Praveen7294 · 2026-02-05T14:41:06Z

But thishttps://github.com/#18545 (comment) is just comment that Check consider as empty line
Check is comment aware

Yes, you are right. The check is comment-aware, but comments tokens are skipped(not consider as empty line) during AST traversal in the visitToken() method.

This one is to skip comment before current token

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

Line 169 in a6594e0

checkComments(ast);

This one is to skip comment after current token

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

Line 180 in a6594e0

while (nextToken != null && TokenUtil.isCommentType(nextToken.getType())) {

but in one case comment tokens are not skipped when EmptyLineSeparatorCheck have to check empty line inside class member block, comment can occur inside class member block also. see below example for clarification:

Without comment:

import java.util.Stack;

public class Test {

    public static void foo1() {
        Stack<Integer> stack = new Stack<>();
        stack.push(5);
 	    new EarlyTerminatingScanner() {
             @Override
             public void visit(Object role, Object element) {
             
             }     // line 12
             



 	    }
            .scan();
    }
}

cli:

$ java -jar checkstyle-13.1.0-all.jar -c config_check.xml Test.java
Starting audit...
[ERROR] F:\GitHub\headhtmltagname\Test.java:12:14: '}' has more than 1 empty lines after. [EmptyLineSeparator]
[ERROR] F:\GitHub\headhtmltagname\Test.java:12:14: There is more than 1 empty line after this line. [EmptyLineSeparator]
Audit done.
Checkstyle ends with 2 errors.

With comment:

import java.util.Stack;

public class Test {

    public static void foo1() {
        Stack<Integer> stack = new Stack<>();
        stack.push(5);
 	    new EarlyTerminatingScanner() {
             @Override
             public void visit(Object role, Object element) {
             
             }        // line 12
             // test



 	    }
            .scan();
    }
}

cli:

$ java -jar checkstyle-13.1.0-all.jar -c config_check.xml Test.java
Starting audit...
[ERROR] F:\GitHub\headhtmltagname\Test.java:12:14: '}' has more than 1 empty lines after. [EmptyLineSeparator]
[ERROR] F:\GitHub\headhtmltagname\Test.java:13:16: There is more than 1 empty line after this line. [EmptyLineSeparator]
Audit done.
Checkstyle ends with 2 errors.

you can see after adding comment difference in second violation. This clearly shows comment is not skipped inside class member block. This explanation is only for clearing confusion around comment.

Not clear why it is new violation

This violation is due to anonymous class with chained method call. if you want i can explain with ast why this violation related to anonymous class with chained method call (not related to comment).

romani · 2026-02-05T21:35:47Z

EmptyLineSeparatorCheck#example7 There is more than 1 empty line after this line. 146

public static ElementSourceFragment createSourceFragmentsFrom(CtElement element) {
144 		ElementSourceFragment rootFragment = new ElementSourceFragment(element, null);
145 		Deque<ElementSourceFragment> parents = new ArrayDeque<>();
146 		parents.push(rootFragment);
147 		/*
148 		 * scan all children of `element` and build tree of SourceFragments
149 		 * Note: we cannot skip implicit elements,
150 		 * because CtBlock can be implicit but contains non implicit elements, which has to be processed.
151 		 */
152 		new EarlyTerminatingScanner<Void>() {

I am human, and I don't understand why checkstyle thinks that after 146 lines there is more than 1 empty lines. It is not true for sure

Praveen7294 · 2026-02-05T22:59:53Z

I am human, and I don't understand why checkstyle thinks that after 146 lines there is more than 1 empty lines. It is not true for sure

see line 160. After this line, there are more than one empty lines. Due to this, the check tries to find the last token before the empty line. However, because of the chained method call, it is unable to determine the correct last token before the empty line.

Praveen7294 · 2026-02-18T16:31:28Z

@romani can we move forward this PR, remaining false positive cases will resolve in #18858 issue. if you are ok with my clarification.

romani · 2026-03-25T15:17:53Z

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

github-actions · 2026-03-25T16:32:29Z

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

Praveen7294 · 2026-04-01T06:42:26Z

@romani ping

romani · 2026-04-11T00:44:53Z

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

github-actions · 2026-04-11T02:01:59Z

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

romani

thanks a lot for this update.
All good.

Zopsss

LGTM!

Praveen7294 · 2026-04-28T16:02:48Z

@romani and @Zopsss, since you both approved my changes, is it okay to merge? I have rebased my PR branch.

romani · 2026-04-29T04:53:00Z

Thanks a lot , sorry that we lost this PR is tracking

Praveen7294 force-pushed the false branch from 4743df8 to 7ec4b2b Compare January 7, 2026 15:26

Zopsss reviewed Jan 12, 2026

View reviewed changes

Praveen7294 force-pushed the false branch 2 times, most recently from de133b2 to 96b8b50 Compare January 30, 2026 22:05

Praveen7294 mentioned this pull request Jan 30, 2026

False positive: EmptyLineSeparator reports violation on method call #18438

Closed

Praveen7294 requested review from Zopsss and romani January 31, 2026 15:06

Praveen7294 force-pushed the false branch 2 times, most recently from 15d9449 to 1504705 Compare February 4, 2026 22:05

Praveen7294 mentioned this pull request Feb 4, 2026

False positive: EmptyLineSeparator report wrong violation in the case of anonymous class #18858

Open

Praveen7294 force-pushed the false branch from 1504705 to 125c7de Compare February 5, 2026 14:47

Praveen7294 force-pushed the false branch from 125c7de to bb39660 Compare February 18, 2026 16:40

Praveen7294 force-pushed the false branch 2 times, most recently from a061750 to 15d2caf Compare March 19, 2026 11:56

Praveen7294 force-pushed the false branch 2 times, most recently from bdc820f to 9e6fad1 Compare March 25, 2026 14:43

Praveen7294 force-pushed the false branch 2 times, most recently from 3f09e03 to 2ec001b Compare March 31, 2026 15:18

romani approved these changes Apr 18, 2026

View reviewed changes

romani assigned Zopsss Apr 18, 2026

Zopsss approved these changes Apr 21, 2026

View reviewed changes

Praveen7294 force-pushed the false branch from 2ec001b to e25b893 Compare April 28, 2026 15:58

Issue checkstyle#18438: Fix false positive in EmptyLineSeparator check

ba03139

romani force-pushed the false branch from e25b893 to ba03139 Compare April 29, 2026 02:29

romani merged commit b884b9a into checkstyle:master Apr 29, 2026
123 checks passed

Praveen7294 deleted the false branch May 9, 2026 05:31

Uh oh!

Conversation

Praveen7294 commented Jan 7, 2026

Uh oh!

Praveen7294 commented Jan 10, 2026

Uh oh!

github-actions Bot commented Jan 10, 2026

Uh oh!

Praveen7294 commented Jan 10, 2026

Uh oh!

Zopsss Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Praveen7294 Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Zopsss Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Praveen7294 Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

romani Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Praveen7294 Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

romani Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Praveen7294 commented Jan 20, 2026

Uh oh!

romani commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Jan 30, 2026

Uh oh!

Praveen7294 commented Jan 30, 2026

Uh oh!

Praveen7294 commented Jan 31, 2026

Uh oh!

romani commented Feb 3, 2026

Uh oh!

github-actions Bot commented Feb 3, 2026

Uh oh!

romani commented Feb 3, 2026

Uh oh!

romani commented Feb 3, 2026

Uh oh!

Praveen7294 commented Feb 4, 2026

Uh oh!

Praveen7294 commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Feb 4, 2026

Uh oh!

Praveen7294 commented Feb 4, 2026

Uh oh!

romani commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Praveen7294 commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Feb 5, 2026

Uh oh!

Praveen7294 commented Feb 5, 2026

Uh oh!

Praveen7294 commented Feb 18, 2026

Uh oh!

romani commented Mar 25, 2026

Uh oh!

github-actions Bot commented Mar 25, 2026

Uh oh!

Praveen7294 commented Apr 1, 2026

Uh oh!

romani commented Apr 11, 2026

Uh oh!

github-actions Bot commented Apr 11, 2026

romani commented Jan 30, 2026 •

edited

Loading

Praveen7294 commented Feb 4, 2026 •

edited

Loading

romani commented Feb 5, 2026 •

edited

Loading

Praveen7294 commented Feb 5, 2026 •

edited

Loading