Issue #17052: Add support for Flexible constructor bodies by mohitsatr · Pull Request #17982 · checkstyle/checkstyle

mohitsatr · 2025-10-23T05:11:18Z

resolves #17052

romani · 2025-10-23T13:18:33Z

@mahfouz72, please assist in this PR.
your knowledge is in high demand

mahfouz72 · 2025-10-24T08:30:28Z

Grammar changes look good to me at this point, but performance seems to have been heavily affected. Can we rerun the job to confirm whether this slowdown is real?

===================== BENCHMARK SUMMARY ====================
Execution Time Baseline: 390.7 s
Average Execution Time: 527.54 s
============================================================
Execution Time Difference: 35.0200%
Difference exceeds the maximum allowed difference (35.0200%      > 10%)!

romani · 2025-10-24T12:12:09Z

Restarted

mohitsatr · 2025-10-25T09:25:31Z

Still bad!

 ===================== BENCHMARK SUMMARY ====================
Execution Time Baseline: 390.7 s
Average Execution Time: 505.37 s
============================================================
Execution Time Difference: 29.3400%
Difference exceeds the maximum allowed difference (29.3400%      > 10%)!

mohitsatr · 2025-10-25T11:58:52Z

code:

public Invalid(int arg) {
new InputIndentationCtorCall().super(
arg
+ 1L);
}

Old AST:

        |       |   `--SLIST -> { [39:28]
        |       |       |--SUPER_CTOR_CALL -> super [40:35]
        |       |       |   |--LITERAL_NEW -> new [40:4]
        |       |       |   |   |--IDENT -> InputIndentationCtorCall [40:8]
        |       |       |   |   |--LPAREN -> ( [40:32]
        |       |       |   |   |--ELIST -> ELIST [40:33]
        |       |       |   |   `--RPAREN -> ) [40:33]
        |       |       |   |--DOT -> . [40:34]
        |       |       |   |--LPAREN -> ( [40:40]
        |       |       |   |--ELIST -> ELIST [42:4]
        |       |       |   |   `--EXPR -> EXPR [42:4]
        |       |       |   |       `--PLUS -> + [42:4]
        |       |       |   |           |--IDENT -> arg [41:4]
        |       |       |   |           `--NUM_LONG -> 1L [42:6]
        |       |       |   |--RPAREN -> ) [42:8]
        |       |       |   `--SEMI -> ; [42:9]

New AST:

        |       |   `--SLIST -> { [39:28]
        |       |       |--EXPR -> EXPR [40:34]
        |       |       |   `--DOT -> . [40:34]
        |       |       |       |--LITERAL_NEW -> new [40:4]
        |       |       |       |   |--IDENT -> InputIndentationCtorCall [40:8]
        |       |       |       |   |--LPAREN -> ( [40:32]
        |       |       |       |   |--ELIST -> ELIST [40:33]
        |       |       |       |   `--RPAREN -> ) [40:33]
        |       |       |       `--LITERAL_SUPER -> super [40:35]
        |       |       |--SEMI -> ; [42:9]
        |       |       `--RCURLY -> } [43:4]

@mahfouz72, I'm resolving failing Indentation Tests because of this change. I noticed in new AST SUPER_CTOR_CALL token has been reduced to mere LITERAL_SUPER. It's children are also missing from the AST, is it expected?

mahfouz72 · 2025-10-25T11:59:35Z

 constructorBlock
    : LCURLY
+      blockStatement*
      explicitConstructorInvocation?
      blockStatement*
      RCURLY


The klenee star (*) could add a lot of lookahead because now the parser needs to match an arbitrary number of blockStatements before and after the constructor call.

I suggest that we can add hints to help the parser immediately decide.

for example:

(blockStatement { !_input.LT(1).equals("LITERAL_SUPER")}?)*

Another solution is that we can unify under a single statement rule. To move explicitConstructorInvocation into the same rule as blockStatement to eliminate nested stars

constructorBlockStatement : blockStatement | explicitConstructorInvocation ; constructorBlock : LCURLY constructorBlockStatement* RCURLY ;

The second solution will probably need changes in the JavaASTVisitor layer

Done. Little tweaking with second approach resovled all the conflicts.

@mohitsatr Great!

mahfouz72 · 2025-10-25T12:03:34Z

I noticed in new AST SUPER_CTOR_CALL token has been reduced to mere LITERAL_SUPER. It's children are also missing from the AST, is it expected?

No, you need to debug JavaASTVisitor to see why this happens. This change is not expected in this PR. You may need to update one of the visitors related to the ctor constructs.

Please read this doc: https://github.com/checkstyle/checkstyle/blob/master/docs/GRAMMAR_UPDATES.md

mohitsatr · 2025-11-04T04:22:57Z

@mahfouz72 while I'm figuring it out, here's some findings so far.

With current grammer, the following code get evaluated in explicitConstructorInvocation block under #primaryCtorCall alternative.

Example:

    public Derived(int arg) {

    new Outer().super(
    arg
    + 1L);
    }

All the token in constructor's code are the children of SUPER_CTOR_CALL Token:

|--SUPER_CTOR_CALL -> super [50:16]
|   |--LITERAL_NEW -> new [50:4]
|   |   |--IDENT -> Outer [50:8]
|   |   |--LPAREN -> ( [50:13]
|   |   |--ELIST -> ELIST [50:14]
|   |   `--RPAREN -> ) [50:14]
|   |--DOT -> . [50:15]
|   |--LPAREN -> ( [50:21]
|   |--ELIST -> ELIST [52:4]
|   |   `--EXPR -> EXPR [52:4]
|   |       `--PLUS -> + [52:4]
|   |           |--IDENT -> arg [51:4]
|   |            `--NUM_LONG -> 1L [52:6]
|   |--RPAREN -> ) [52:8]
|   |   `--SEMI -> ; [52:9]
|    `--RCURLY -> } [53:4]

but after the change in constuctorBlock, now the same code gets evaluated in blockStatement under #superExp label

constructorBlock
    : ...
      (blockStatement { !_input.LT(1).equals("LITERAL_SUPER")}?)*   <----
      explicitConstructorInvocation?
      ...
      ...
    ;

explicitConstructorInvocation
    : ...
    | expr DOT typeArguments? LITERAL_SUPER arguments SEMI                 #primaryCtorCall

blockStatement
    : ...
    | statement                                                            #stat
    ...
    ;
statement
    : ...
    | statementExpression=expression SEMI                                  #expStat

expression
    : expr 
    ;

expr
     :
    | expr bop=DOT nonWildcardTypeArguments?
      LITERAL_SUPER superSuffix?                                          `#superExp`

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/JavaAstVisitor.java

Lines 1337 to 1341 in 9eceb67

    
           @Override 
        
           public DetailAstImpl visitSuperExp(JavaLanguageParser.SuperExpContext ctx) { 
        
               final DetailAstImpl bop = create(ctx.bop); 
        
               bop.addChild(visit(ctx.expr())); 
        
               bop.addChild(create(ctx.LITERAL_SUPER()));

visitSuperExp method is responsible for ASTs which have SUPER_LITERAL expression.
Example:

    Outer() {}

    public int size() { return super.size(); }

    public boolean isEmpty() { return Outer.super.isEmpty(); }

AST:

|--LITERAL_RETURN -> return [12:24]
|   |--EXPR -> EXPR [12:41]
|   |   `--METHOD_CALL -> ( [12:41]
|   |       |--DOT -> . [12:36]
|   |       |   |--LITERAL_SUPER -> super [12:31]
|   |       |   `--IDENT -> size [12:37]
|   |       |--ELIST -> ELIST [12:42]
|   |       `--RPAREN -> ) [12:42]
|   `--SEMI -> ; [12:43]
 `--RCURLY -> } [12:45]
....
|--LITERAL_RETURN -> return [14:31]
|   |--EXPR -> EXPR [14:77]
|   |   `--METHOD_CALL -> ( [14:77]
|   |       |--DOT -> . [14:69]
|   |       |   |--DOT -> . [14:63]
|   |       |   |   |--IDENT -> InputRegressionJavaClass2 [14:38]
|   |       |   |    `--LITERAL_SUPER -> super [14:64]
|   |       |    `--IDENT -> isEmpty [14:70]
|   |       |--ELIST -> ELIST [14:78]
|   |        `--RPAREN -> ) [14:78]
|    `--SEMI -> ; [14:79]
 `--RCURLY -> } [14:81]

I think it is not a good idea to handle these both types of ASTs from a single visit.. method?

mohitsatr · 2025-11-08T05:26:18Z

Now I'm working on adding more test cases and a description

romani · 2025-11-18T13:54:10Z

@mohitsatr , we appreciate your help on this.
our test suite was migrate from openjdk21 to openjdk25 so you can easily test by diff report on such new syntax.

but to clearly see that all is resolved we need to run report on local , as explained at #18079 (comment) as CI version of diff report are focused on ignoring parser problems.

mohitsatr · 2025-11-19T01:42:19Z

@romani yes I understand. I'm just waiting for #17329 to finish, then I'll resume working on this one.

Adrijaned · 2025-12-16T07:48:07Z

@mohitsatr Hello, may I ask what the progress on this is looking like/is there something I might be able to help with to move this forward when I get some spare time?

mohitsatr · 2025-12-16T14:15:35Z

@Adrijaned, this Pr is back on top priority now.

Just in case you haven't noticed, there's a temporary workaround.

mohitsatr · 2025-12-17T05:18:13Z

@stoyanK7 @mahfouz72
I've generated regression report locally, how can I host it so everyone can see ?

stoyanK7 · 2025-12-17T05:27:15Z

@mohitsatr Check this out: https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/README_MANUAL_EXECUTION.md#deploying-report

mohitsatr · 2025-12-17T13:22:07Z

@stoyanK7 It is here: https://mohitsatr.github.io/diff/index.html

But I'm not sure what to make of it?

mahfouz72 · 2025-12-17T22:14:31Z

https://mohitsatr.github.io/diff/my-checkstyle/index.html#A32

We got an exception, this is not expected, we should only see a difference in AST in files that use the new feature (flexible constructor bodies)

mohitsatr · 2025-12-18T12:34:15Z

@mahfouz72, do you mean there was a mistake in the report generation process?

mohitsatr · 2025-12-27T12:17:21Z

@stoyanK7 I'm currently having time constraints, so if you like, feel free to take this one.

vad0 · 2026-02-01T16:36:53Z

Guys, pls check this out: #18821. The build is green now.

mahfouz72 reviewed Oct 25, 2025

View reviewed changes

mohitsatr force-pushed the flexible-ctor-body branch from 77d6696 to 61271c0 Compare November 4, 2025 04:34

Issue checkstyle#17052: Add support for Flexible constructor bodies

ceb4316

mohitsatr force-pushed the flexible-ctor-body branch from 61271c0 to ceb4316 Compare November 8, 2025 05:25

mohitsatr marked this pull request as ready for review November 8, 2025 05:25

This was referenced Nov 11, 2025

Issue #17919: Add support for Java25's 'import module' #18079

Merged

doc: Add extra section about 'Performance Regressions' #18082

Merged

vad0 mentioned this pull request Feb 1, 2026

Issue #17052: Fix CI failures for Flexible constructor bodies #18821

Closed

4 tasks

mohitsatr closed this Apr 3, 2026

Uh oh!

Conversation

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

Uh oh!

Uh oh!

romani commented Oct 23, 2025

Uh oh!

mahfouz72 commented Oct 24, 2025

Uh oh!

romani commented Oct 24, 2025

Uh oh!

mohitsatr commented Oct 25, 2025

Uh oh!

mohitsatr commented Oct 25, 2025

Uh oh!

mahfouz72 Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

mohitsatr Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

mahfouz72 Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

mahfouz72 commented Oct 25, 2025

Uh oh!

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

Uh oh!

Uh oh!

mohitsatr commented Nov 8, 2025

Uh oh!

romani commented Nov 18, 2025

Uh oh!

mohitsatr commented Nov 19, 2025

Uh oh!

Adrijaned commented Dec 16, 2025

Uh oh!

mohitsatr commented Dec 16, 2025

Uh oh!

mohitsatr commented Dec 17, 2025

Uh oh!

stoyanK7 commented Dec 17, 2025

Uh oh!

mohitsatr commented Dec 17, 2025

Uh oh!

mahfouz72 commented Dec 17, 2025

Uh oh!

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

Uh oh!

Uh oh!

mohitsatr commented Dec 27, 2025

Uh oh!

vad0 commented Feb 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mohitsatr commented Oct 23, 2025 •

edited

Loading

mohitsatr commented Nov 4, 2025 •

edited

Loading

mohitsatr commented Dec 18, 2025 •

edited

Loading