Issue #14805: Remove support for string templates syntax by mahfouz72 · Pull Request #15212 · checkstyle/checkstyle

mahfouz72 · 2024-07-08T15:50:53Z

closes #14805

Reports

Projects file: projects-to-test-on.properties
Patch branch: pull-15212

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/pull-15212/2024-07-10-T-00-20-42/part1/index.html

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/pull-15212/2024-07-10-T-00-20-42/part4/index.html

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/pull-15212/2024-07-10-T-00-20-42/part5/index.html

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/pull-15212/2024-07-10-T-00-20-42/part3/index.html

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/pull-15212/2024-07-10-T-00-20-42/part2/index.html

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/pull-15212/2024-07-10-T-00-20-42/part6/index.html

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/pull-15212/2024-07-10-T-00-20-42/sevntu-check-regression_part_1/index.html

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/pull-15212/2024-07-10-T-00-20-42/sevntu-check-regression_part_2/index.html

All previous reports are good. differences are:

removal of violations in files that have string template
line numbers in exception messages (VarargsAndReceiver file)

https://checkstyle-reports.s3.us-east-2.amazonaws.com/reports/pull-15212/2024-07-10-T-00-20-42/antlr-report-checkstyle/index.html

Report is good difference are:

String templates trees are removed and printing exception message instead
line numbers in exception messages

I just don't understand this. Is this expected? it is still stackoverflow but the difference in messages is not just line numbers

mahfouz72 · 2024-07-08T15:55:17Z

    STRING_TEMPLATE_CONTENT, EMBEDDED_EXPRESSION_BEGIN, EMBEDDED_EXPRESSION,
    EMBEDDED_EXPRESSION_END,



had to leave string templates token in the lexer due to:

[ERROR] GeneratedJavaTokenTypesTest.testTokenNumbering:741 A token's number has changed. Please open 'GeneratedJavaTokenTypesTest' and confirm which token is at fault. Token numbers must not change or else they will create a conflict with users. See Issue: https://github.com/checkstyle/checkstyle/issues/505

If we remove string template tokens from the lexer the last two tokens' numbers will change and we have a test that ensures that the tokens' numbers don't change.

accordingly, coverage for javaLnaguageLexer.java will drop

@nrmancuso please help in this how to remove the tokens and pass GeneratedJavaTokenTypesTest.testTokenNumbering ?

nrmancuso

Please do:

Update coverage numbers to make mvn clean verify happy (generated files are not a big concern on this front, and it is obvious why it is dropping in this case)
Add file filter modules to openjdk21 no-exceptions execution, see link at the top of this file if you want to do it via script
prefix all string template tokens in the lexer with DEPRECATED_
Leave a comment in the lexer about why we did item 3.

Sadly, we consider renumbering of tokens in our API as a breaking change, so we need to keep these tokens unless we can agree that this case is exceptional.

@rnveach @romani please share your thoughts on item 3.

romani · 2024-07-08T18:32:58Z

What if we put __ to make it clear it is not for use at all ? Or something like this.
Deprecated might be considered as part of language terminology.

mahfouz72 · 2024-07-09T02:28:23Z

I had to use DEPRECATED_ as a prefix instead of __
because it is a syntax error if name starts with underscore

[ERROR] error(50): JavaLanguageLexer.g4:119:4: syntax error: '_' came as a complete surprise to me

romani · 2024-07-09T02:49:04Z

+    // are no longer used, but we need to keep them to preserve token numbering,
+    // so that we are not breaking change with older versions.
+    // We used 'DEPRECATED' as prefix to indicate that they are not used.
+    DEPRECATED_STRING_TEMPLATE_BEGIN,DEPRECATED_STRING_TEMPLATE_MID, DEPRECATED_STRING_TEMPLATE_END,


__Not__For_usage1__, __Not__For_usage2__
To completely forget why it was added.
No reason in comment. No confusion in future.

I open for better names.

good
@nrmancuso what do you think

We can do NOT_FOR_USAGE_1 with comment that these are placeholders to maintain token ordering, we can skip mentioning string templates

rnveach · 2024-07-09T18:13:20Z

we consider renumbering of tokens in our API as a breaking change, so we need to keep these tokens unless we can agree that this case is exceptional

Isn't this whole PR a breaking change? We are removing what we once supported. Issue is already labeled as this.

I am good to remove all existence of the old tokens and change numbers. I prefer a clean break over leaving crazyness in our code.

I think this should be why we shouldn't support any preview features until they get out of the preview state.

nrmancuso · 2024-07-09T20:01:41Z

I am good to remove all existence of the old tokens and change numbers. I prefer a clean break over leaving crazyness in our code.

I am good, @romani?

romani · 2024-07-09T20:09:55Z

Yes, it is breaking changes.
But there is more problems, such int values are inclined during compilation to others third party and not changing. If some plugin/extension in compiled with 10.17.0 and executed with some 11.x where we will have same numbers, it will be disaster to figure out a problem.

We already did this ones in a past :). Users were upset. Let's not deal with them. Managing API is hard , and we need to do it in hard way. Fortunately this deprecation is easy.
Such numbers will never be used, custom Checks will not be called :), no Check execution, no violations, easy to detect anomalies.

nrmancuso

minor item:

nrmancuso · 2024-07-09T22:20:28Z

I am prepared to approve after #15212 (review) is completed, and reports at https://github.com/nrmancuso/checkstyle-diff-report-generator/actions/runs/9864962646 look good.

nrmancuso · 2024-07-09T22:23:22Z

To be clear, nothing blocks us from making a new issue to remove the placeholder tokens and breaking compatibility, @rnveach feel free to open such an issue and we can continue the discussion there. We need to get this PR across the finish line.

nrmancuso · 2024-07-09T22:25:04Z

@romani please proceed with review

romani

Ok to merge

nrmancuso · 2024-07-10T03:42:37Z

@mahfouz72 I have added the reports to the PR description, please analyze them and share results for each under each report.

nrmancuso

Approved conditionally based on report results.

nrmancuso · 2024-07-10T13:53:52Z

Reports and analysis looks good, the stack overflow causes undefined behavior which can manifest on our side or ANTLR’s side.

mahfouz72 · 2024-07-10T17:26:35Z

Execution Time Baseline: 415.72 s
Average Execution Time: 384.02 s
============================================================
Execution Time Difference: -7.6200%

interesting statistics, but why is there a huge difference compared to other PR? Does parsing have such high time complexity?

rnveach · 2024-07-10T17:52:05Z

I am good to approve with no concerns but this has to go at the same time as #15204 . I am not sure if it is safe to merge as release still hasn't happened and @romani was suppose to be looking into the issue already.

mahfouz72 · 2024-07-10T18:22:48Z

Will we have 10.18.0 this month? I thought it would be postponed for next month due to the release issues

I am not sure if it is safe to merge as release still hasn't happened

why not? why does it have to be after release?

nrmancuso · 2024-07-10T18:44:11Z

Does parsing have such high time complexity?

Yes, this is our MOST performance sensitive area.

nrmancuso · 2024-07-10T18:45:31Z

I am good to approve with no concerns but this has to go at the same time as #15204 .

To be clear, #15204 should go first.

rnveach · 2024-07-10T20:21:59Z

Will we have 10.18.0 this month? I thought it would be postponed for next month due to the release issues

The last message we got from @romani was he would come back to the release issue in a week (already passed) and he would continue the release. He hasn't said it has been moved to the end of the month.

why not? why does it have to be after release?

We want this and other PR to go together or as @nrmancuso says for the other to go first. I don't want to merge this first before the other and then find out a surprise release happened.

romani · 2024-07-11T02:35:01Z

Too much activity in projects, I deprioritized release in favor of unblocking activity in project.
Should I change priority?

romani · 2024-07-11T02:38:00Z

It is safe to merge.
Don't forget to downgrade value in performance threshold.

nrmancuso · 2024-07-12T19:20:13Z

#15241

mahfouz72 commented Jul 8, 2024

View reviewed changes

mahfouz72 marked this pull request as draft July 8, 2024 15:55

nrmancuso suggested changes Jul 8, 2024

View reviewed changes

mahfouz72 force-pushed the remove-string-templates branch from 3534ec9 to bdfdf43 Compare July 9, 2024 00:21

mahfouz72 force-pushed the remove-string-templates branch from bdfdf43 to 003cc3d Compare July 9, 2024 02:45

romani reviewed Jul 9, 2024

View reviewed changes

mahfouz72 marked this pull request as ready for review July 9, 2024 03:03

mahfouz72 force-pushed the remove-string-templates branch from 003cc3d to b763422 Compare July 9, 2024 16:35

mahfouz72 force-pushed the remove-string-templates branch from b763422 to f5c492a Compare July 9, 2024 21:28

nrmancuso suggested changes Jul 9, 2024

View reviewed changes

Comment thread src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageLexer.g4 Outdated

nrmancuso self-assigned this Jul 9, 2024

nrmancuso requested a review from romani July 9, 2024 22:24

nrmancuso assigned romani Jul 9, 2024

Issue checkstyle#14805: Remove support for string templates syntax

ef5cf61

mahfouz72 force-pushed the remove-string-templates branch from f5c492a to ef5cf61 Compare July 9, 2024 22:54

romani approved these changes Jul 10, 2024

View reviewed changes

romani removed their assignment Jul 10, 2024

nrmancuso approved these changes Jul 10, 2024

View reviewed changes

nrmancuso assigned rnveach and unassigned nrmancuso Jul 10, 2024

nrmancuso requested a review from rnveach July 10, 2024 03:43

nrmancuso mentioned this pull request Jul 12, 2024

Reduce performance threshold value #15241

Closed

nrmancuso merged commit 15de92a into checkstyle:master Jul 12, 2024

nrmancuso mentioned this pull request Jul 12, 2024

Remove Support for String Template Syntax #14805

Closed

mahfouz72 mentioned this pull request Jul 13, 2024

Issue #15241: Reduce baseline execution time value #15245

Merged

mahfouz72 deleted the remove-string-templates branch May 9, 2025 09:36

		STRING_TEMPLATE_CONTENT, EMBEDDED_EXPRESSION_BEGIN, EMBEDDED_EXPRESSION,
		EMBEDDED_EXPRESSION_END,

Uh oh!

Conversation

mahfouz72 commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reports

Uh oh!

mahfouz72 Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

nrmancuso left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romani commented Jul 8, 2024

Uh oh!

mahfouz72 commented Jul 9, 2024

Uh oh!

romani Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahfouz72 Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

nrmancuso Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

mahfouz72 Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

rnveach commented Jul 9, 2024

Uh oh!

nrmancuso commented Jul 9, 2024

Uh oh!

romani commented Jul 9, 2024

Uh oh!

nrmancuso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nrmancuso commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrmancuso commented Jul 9, 2024

Uh oh!

nrmancuso commented Jul 9, 2024

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

nrmancuso commented Jul 10, 2024

Uh oh!

nrmancuso left a comment

Choose a reason for hiding this comment

Uh oh!

nrmancuso commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahfouz72 commented Jul 10, 2024

Uh oh!

rnveach commented Jul 10, 2024

Uh oh!

mahfouz72 commented Jul 10, 2024

Uh oh!

nrmancuso commented Jul 10, 2024

Uh oh!

nrmancuso commented Jul 10, 2024

Uh oh!

rnveach commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Jul 11, 2024

Uh oh!

romani commented Jul 11, 2024

Uh oh!

nrmancuso commented Jul 12, 2024

Uh oh!

Reviewers

Assignees

Labels

mahfouz72 commented Jul 8, 2024 •

edited

Loading

nrmancuso left a comment •

edited

Loading

romani Jul 9, 2024 •

edited

Loading

nrmancuso commented Jul 9, 2024 •

edited

Loading

nrmancuso commented Jul 10, 2024 •

edited

Loading

rnveach commented Jul 10, 2024 •

edited

Loading