Skip to content

Issue #17052: Add support for Flexible constructor bodies #17982

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Issue #17052: Add support for Flexible constructor bodies #17982

wants to merge 1 commit into from

Conversation

@mohitsatr
Copy link
Member

@mohitsatr mohitsatr commented Oct 23, 2025

resolves #17052

very WIP

Notes:

Additional info:

@romani
Copy link
Member

romani commented Oct 23, 2025

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

@mahfouz72
Copy link
Member

mahfouz72 commented Oct 24, 2025

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
Copy link
Member

romani commented Oct 24, 2025

Restarted

@mohitsatr
Copy link
Member Author

mohitsatr commented Oct 25, 2025

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
Copy link
Member Author

mohitsatr commented Oct 25, 2025

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
mahfouz72 reviewed Oct 25, 2025
View reviewed changes
src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageParser.g4
Comment on lines 492 to 497
constructorBlock
: LCURLY
blockStatement*
explicitConstructorInvocation?
blockStatement*
RCURLY
Copy link
Member

@mahfouz72 mahfouz72 Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mahfouz72
Copy link
Member

mahfouz72 commented Oct 25, 2025

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
Copy link
Member Author

mohitsatr commented Nov 4, 2025 •
edited
Loading

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mahfouz72 mahfouz72 mahfouz72 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for flexible constructor bodies (JEP 513) targeted for JDK25

3 participants

@mohitsatr @romani @mahfouz72