Issue #17223: New Check TextBlockGoogleStyleFormattingCheck by mohitsatr · Pull Request #17329 · checkstyle/checkstyle

mohitsatr · 2025-07-08T05:17:37Z

New module config: https://gist.githubusercontent.com/mohitsatr/0e04070f847926ea44bf7eda88706807/raw/f9470db307d730a651e9c93cad776d6b592839e8/textblockgooglestyleformatting.xml

Contribution repo: checkstyle/contribution#955

Zopsss

Item:

Zopsss · 2025-07-13T03:28:46Z

I think Indentation checking should be easy too. We first check if both quotes are at the start of line or not. And then check if both quotes are vertically aligned with tabs expanded. Get the starting column number of the line, for both - opening quotes & closing quotes. Check their starting column number value and see if it is same or not. If it is not same then log the violation.

mohitsatr · 2025-07-14T12:34:12Z

@Zopsss there can be a case where quotes are vertically aligned but at wrong Indentation value.
For checking indentation, we will first find the indentation of a opening quotes and indentation of it's parent element (anything that's one line above it). if the difference between the two values is 0 or 4 (lineWrappingIndentation), then it's is at correct position.

Zopsss · 2025-07-15T06:01:29Z

we will first find the indentation of a opening quotes and indentation of it's parent element (anything that's one line above it). if the difference between the two values is 0 or 4 (lineWrappingIndentation), then it's is at correct position.

This should be part of the indentation check itself. In this check we should only focus on whether the quotes are vertically aligned or not.

The JavadocLeadingAsteriskAlign does something similar:

The Check ensures that leading asterisks are aligned vertically under the first asterisk ( * ) of opening Javadoc tag. The alignment of closing Javadoc tag ( */ ) is also checked.

It ensures that the leading asterisks in Javadoc comments are vertically aligned under the first asterisk (*) of the opening Javadoc tag. It also verifies the alignment of the closing tag (*/).

This check does not enforce correct indentation. For example, if the entire Javadoc block is misaligned (e.g., starts at column 5 instead of 4), but the asterisks are properly aligned vertically, the check will still pass.

Similarly, this new check should only be concerned with the vertical alignment of the quote characters. Whether the quotes are placed at the correct indentation level is outside its scope and should be handled by the indentation check.

Zopsss · 2025-07-27T15:10:49Z

+    /**
+     * Determines if the given expression is at the start of a line.
+     *
+     * @param ast the ast to check
+     * @return true if it is, false otherwise
+     */
+    private boolean isStartOfLine(DetailAST ast) {
+        final int tabWidth = getTabWidth();
+        final String line = getLine(ast.getLineNo() - 1);
+
+        int index = 0;
+        while (Character.isWhitespace(line.charAt(index))) {
+            index++;
+        }
+        return CommonUtil.lengthExpandedTabs(line, index, tabWidth)
+            == CommonUtil.lengthExpandedTabs(line, ast.getColumnNo(), tabWidth);
+    }


Can we just get the text of the token and then verify if it is the first thing in the line or not. Regex can help us to verify if the quotes are the first thing present in the line or not. It'll be much simpler and more optimized.

@Zopsss @romani
yes we can try using Regex, but don't we always push for more AST based approach in general?

what do you mean by "simpler"? if you mean less lines of code, then we can make this code much simpler. this is only first draft and I can already notice there is lot of duplicate code that can be reduced. We will have much simpler version of this code by the time this PR is ready for merging.

We prefer AST based solutions.
But regexp some time are ok, as you might noticed with Todo. When regexp is simple, it might be ok
Your call to fall to AST based.

I think Regex approach will be more suitable, it will reduce calls to other methods so it'll be a little more optimized.

method to get content of line is deprecated, we try all to not use it, as you allow to use it for one reason, second reason will be even more easier to justify. But it is proven to be hell to maintain, but yes AST is more complicated in implementaton but more robust.. We should operate by AST.

is this conversation resolved? how to proceed further here?

We should go with the AST base approach. @mohitsatr

mohitsatr · 2025-08-18T08:30:50Z

+                !TokenUtil.areOnSameLine(ast, superParent);
+        };
+    }
+


Ast based approach. more complex

Zopsss

We're on the right track!

pass 1:

Zopsss · 2025-08-21T02:21:24Z

+        final DetailAST closingQuotes = getClosingQuotes(ast);
+
+        if (!areOpeningQuotesOnCorrectPosition(ast)) {
+            log(ast, MSG_OPEN_QUOTES_ERROR);
+        }
+
+        if (!areClosingQuotesOnCorrectPosition(getClosingQuotes(ast))) {
+            log(closingQuotes, MSG_CLOSE_QUOTES_ERROR);
+        }


Suggested change

final DetailAST closingQuotes = getClosingQuotes(ast);

if (!areOpeningQuotesOnCorrectPosition(ast)) {

log(ast, MSG_OPEN_QUOTES_ERROR);

}

if (!areClosingQuotesOnCorrectPosition(getClosingQuotes(ast))) {

log(closingQuotes, MSG_CLOSE_QUOTES_ERROR);

}

if (!areOpeningQuotesOnCorrectPosition(ast)) {

log(ast, MSG_OPEN_QUOTES_ERROR);

}

final DetailAST closingQuotes = getClosingQuotes(ast);

if (!areClosingQuotesOnCorrectPosition(closingQuotes)) {

log(closingQuotes, MSG_CLOSE_QUOTES_ERROR);

}

Zopsss · 2025-08-21T02:23:10Z

+     * @return true if the opening quotes are on the new line.
+     */
+    private static boolean areOpeningQuotesOnCorrectPosition(DetailAST openingQuotes) {
+        DetailAST superParent = openingQuotes;


DetailAST parent = openingQuotes;

Zopsss · 2025-08-21T02:25:22Z

+        while (!TokenUtil.isOfType(superParent.getType(), TokenTypes.LITERAL_RETURN,
+            TokenTypes.ASSIGN, TokenTypes.LITERAL_RETURN, TokenTypes.METHOD_CALL,
+            TokenTypes.PLUS)) {
+
+            superParent = superParent.getParent();
+
+            if (superParent.getType() == TokenTypes.PLUS
+                    && plusIsBetweenTwoTextBlocks(openingQuotes, superParent)) {
+                superParent = superParent.getParent();
+            }
+        }


while iterating over the parents, can we check if the opening quotes and parent are on same line? if they're not then we can break the loop as the opening quotes will be on new line so no need to check anything further.

Done.

I redid the entire logic based on your suggestion. It's more cleaner and easier to understand now. I really feel new logic is more robust

Zopsss · 2025-08-21T02:29:27Z

+        if (superParent.getType() == TokenTypes.METHOD_CALL) {
+            result = checkMethodCall(openingQuotes, superParent);
+        }
+        else {
+            result = !TokenUtil.areOnSameLine(openingQuotes, superParent);
+        }
+        return result;


Suggested change

if (superParent.getType() == TokenTypes.METHOD_CALL) {

result = checkMethodCall(openingQuotes, superParent);

}

else {

result = !TokenUtil.areOnSameLine(openingQuotes, superParent);

}

return result;

result = !TokenUtil.areOnSameLine(openingQuotes, superParent);

if (superParent.getType() == TokenTypes.METHOD_CALL && !result) {

result = checkMethodCall(openingQuotes, superParent);

}

return result;

first check if opening quotes and parent are on the same line or not. If not then return true. If the parent is type of Method Call and parent & opening quotes are not on same line then invoke the further logic.

Zopsss · 2025-08-21T02:30:00Z

+     * @param methodCall the methodCall
+     * @return true
+     */
+    private static boolean checkMethodCall(DetailAST ast, DetailAST methodCall) {


(DetailAST openingQuotes, DetailAST methodCall)

Zopsss · 2025-08-21T02:30:24Z

+     * @return true
+     */
+    private static boolean checkMethodCall(DetailAST ast, DetailAST methodCall) {
+        DetailAST tmp = methodCall.getFirstChild();


what is tmp? Please use better name

Zopsss · 2025-08-21T02:30:50Z

+        }
+
+        else {
+            DetailAST expList = methodCall.findFirstToken(TokenTypes.ELIST);


expList -> expressionList

Zopsss · 2025-08-21T02:31:24Z

+        boolean result = !TokenUtil.areOnSameLine(ast, tmp);
+
+        if (tmp.getType() == TokenTypes.DOT && tmp.getFirstChild() != ast) {
+            result = !TokenUtil.areOnSameLine(ast, tmp);
+        }


we're repeating the same logic here. I don't think this if statement is needed.

Zopsss · 2025-08-21T02:32:24Z

+        while (text.charAt(index) == ' ') {
+            index--;
+        }
+        char test = text.charAt(index);
+        return text.charAt(index) == '\n';


instead of using while loop, can we directly check if the 0th index is \n or not?

Im not sure I follow, 0th index?

We are looking for \n from the last index. after eliminating whitespaces, if we reach a '\n' then we know Closing quotes are on the new line. but if we reach any other character, then we place violation for closing quotes not being on new line.

--TEXT_BLOCK_LITERAL_BEGIN -> """ [108:18] |--TEXT_BLOCK_CONTENT -> \n some String\n [108:21] `--TEXT_BLOCK_LITERAL_END -> """ [110:12]

ah sorry my bad, I misunderstood the code.

Zopsss · 2025-08-21T02:33:49Z

+        //   'Text-block quotes are not vertically aligned.'
+        getData("""
+            first string
+            """ + """


is opening quotes allowed to be preceded by +? How does formatter tackle this code?

No. violation comment for that is on 114 line.

Formatter fails here. It made the opening quotes preceded by +.

--- TextBlocksOnLine.java 2025-08-27 12:03:51.928243400 +0530 +++ InputFormattedTextBlockGoogleStyleTree.java 2025-08-27 12:03:55.051129600 +0530 @@ -1,15 +1,16 @@ public class TextBlocksOnLine { void method1() { - getData(""" + getData( + """ first string - """ + """ - some String - """, """ - second string + + """ + some String + """, """ - ); - } + second string + """); + }

nice catch. Report it to their github repository.

romani

items:

romani · 2025-08-28T12:41:28Z

+ * </li>
+ * </ul>
+ *
+ * @since 11.0.0


romani · 2025-08-28T12:43:40Z

    }

-    @Test
-    public void testCommitMessageHasProperStructure() throws Exception {


nevermind those changes. we are still in testing phrase and this test was bothering me a lot in the build process

Zopsss · 2025-08-28T12:53:16Z

-        final String message = """
+        final String message =
+                """


why this was changed? Similar changes are made to other files too.

I started getting build errors from this Check

@romani I haven't even added this check to checkstyle-checks.xml, why have we started getting errors from it already?

Zopsss · 2025-08-30T12:46:37Z

@mohitsatr please start generating regression reports.

Zopsss · 2025-09-02T04:19:46Z

@mohitsatr ping

mohitsatr · 2025-09-02T07:15:08Z

Github, generate report

github-actions · 2025-09-02T07:45:40Z

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

Zopsss · 2025-09-03T05:36:14Z

checker gave an exception at: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7007ab5_2025074153/reports/diff/apache-struts/index.html#A17

@mohitsatr can you investigate it? Exception was thrown at following line:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/TextBlockGoogleStyleFormattingCheck.java

Line 198 in 7007ab5

final DetailAST node = expressionList.getFirstChild().getFirstChild();

But it was for for a multi-line comment, so it maybe not due to this check. Maybe something else caused it:

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7007ab5_2025074153/reports/diff/apache-struts/xref/home/runner/work/checkstyle/checkstyle/.ci-temp/contribution/checkstyle-tester/repositories/apache-struts/plugins/convention/src/main/java/org/apache/struts2/convention/DefaultResultMapBuilder.java.html#L1

Zopsss · 2025-09-03T06:17:43Z

False-negative. Opening quotes are not violated:

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7007ab5_2025074153/reports/diff/camunda/index.html#A421

Case:

   private static String createMigrationRejectionDueConcurrentModificationReason(
       final long processInstanceKey) {
     return """
         Expected to migrate process instance '%d' \
         but a concurrent command was executed on the process instance. \
         Please retry the migration."""
         .formatted(processInstanceKey);
   }

Edit: similar case: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7007ab5_2025074153/reports/diff/camunda/index.html#A423

Zopsss · 2025-09-03T06:31:55Z

Found similar case again, in Checkstyle codebase:

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7007ab5_2025074153/reports/diff/checkstyle/index.html#A3

57th line is not marked as violation.

Another one: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7007ab5_2025074153/reports/diff/checkstyle/index.html#A101

41st line is not violated.

Zopsss · 2025-09-03T06:57:59Z

False positive: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7007ab5_2025074153/reports/diff/spring-integration/index.html#A24

mohitsatr · 2025-09-04T07:36:37Z

Github, generate report

romani

last:

Zopsss

Thank you @mohitsatr for all the hard work! This check will help us to cover a huge gap in our style guide implementation! :D

romani · 2025-12-02T04:37:14Z

This error is stable https://app.circleci.com/pipelines/github/checkstyle/checkstyle/38176/workflows/5720fa60-65a7-4ca4-bcd4-634027ec0b74/jobs/1149262

romani

one more:

romani · 2025-12-03T03:24:02Z

Ok , final challenge is to pass CI

Should be simple to fix from semaphore:
List of missing files in pitest profiles: src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/TextBlockGoogleStyleFormattingCheck.java

Circleci:
./.ci/validation.sh release-dry-run hmmmm not very clear

romani · 2025-12-04T15:33:39Z

ERROR] XdocsPagesTest.testAllXmlExamples:701->getTagSourcesNode:481 src/site/xdoc/checks/coding/textblockgooglestyleformatting.xml has invalid xml (The element type "p" must be terminated by the matching end-tag "</p>".): <?xml version="1.0" encoding="UTF-8"?>

So it conflicts with release dry run

romani · 2025-12-05T02:01:13Z

+  <mutation unstable="false">
+    <sourceFile>TextBlockGoogleStyleFormattingCheck.java</sourceFile>
+    <mutatedClass>com.puppycrawl.tools.checkstyle.checks.coding.TextBlockGoogleStyleFormattingCheck</mutatedClass>
+    <mutatedMethod>closingQuotesAreAloneOnTheLine</mutatedMethod>
+    <mutator>org.pitest.mutationtest.engine.gregor.mutators.NonVoidMethodCallMutator</mutator>
+    <description>removed call to java/lang/String::charAt</description>
+    <lineContent>while (text.charAt(index) == &apos; &apos;) {</lineContent>
+  </mutation>
+
+  <mutation unstable="false">
+    <sourceFile>TextBlockGoogleStyleFormattingCheck.java</sourceFile>
+    <mutatedClass>com.puppycrawl.tools.checkstyle.checks.coding.TextBlockGoogleStyleFormattingCheck</mutatedClass>
+    <mutatedMethod>closingQuotesAreAloneOnTheLine</mutatedMethod>
+    <mutator>org.pitest.mutationtest.engine.gregor.mutators.RemoveConditionalMutator_EQUAL_ELSE</mutator>
+    <description>removed conditional - replaced equality check with false</description>
+    <lineContent>while (text.charAt(index) == &apos; &apos;) {</lineContent>
+  </mutation>
+
+  <mutation unstable="false">
+    <sourceFile>TextBlockGoogleStyleFormattingCheck.java</sourceFile>
+    <mutatedClass>com.puppycrawl.tools.checkstyle.checks.coding.TextBlockGoogleStyleFormattingCheck</mutatedClass>
+    <mutatedMethod>quotesArePrecededWithComma</mutatedMethod>
+    <mutator>org.pitest.mutationtest.engine.gregor.mutators.RemoveConditionalMutator_EQUAL_IF</mutator>
+    <description>removed conditional - replaced equality check with true</description>
+    <lineContent>return expression.getType() == TokenTypes.EXPR</lineContent>
+  </mutation>

@mohitsatr , I pushed fixes for all previous problems.
Here are new , pitest was not executed before. Please fix mutations

Before you proceed, pull branch from remote yo your local

mohitsatr · 2025-12-06T01:46:51Z

I pushed fixes for all previous problems.

thanks!

@romani By the way, how did you find out the main issue? Did you run the release_dry locally?

mohitsatr · 2025-12-10T01:41:42Z

Github, generate report

github-actions · 2025-12-10T02:08:18Z

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

romani · 2025-12-10T02:36:41Z

Did you run the release_dry locally?

Yes, all commands of CI are executable on Linux local.
All CI prints a command they executed.

This PR is hanging for so long I decided to use my rare laptop time to move this PR across finish line, but :) faced more blockers.

mohitsatr · 2025-12-11T02:36:29Z

Github, generate report

github-actions · 2025-12-11T03:11:14Z

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

romani

It was huge effort. Great JOB !!!

Even I impressed how complicated it is to make new Check in Checkstyle. But it works and hope there will be no problems for users.

Thanks a lot to all participants

Zopsss suggested changes Jul 9, 2025

View reviewed changes

mohitsatr force-pushed the textblockgooglestyle branch 2 times, most recently from 28c8773 to 2060baa Compare July 12, 2025 11:15

Zopsss suggested changes Jul 13, 2025

View reviewed changes

Comment thread .../java/com/puppycrawl/tools/checkstyle/checks/coding/TextBlockGoogleStyleFormattingCheck.java Outdated

mohitsatr force-pushed the textblockgooglestyle branch from 2060baa to bc499b0 Compare July 14, 2025 10:22

mohitsatr force-pushed the textblockgooglestyle branch 2 times, most recently from 7ca514c to 4099ebb Compare July 15, 2025 04:11

This was referenced Jul 27, 2025

Config: Introduced new check TextBlockGoogleStyleFormatting mohitsatr/contribution#1

Closed

Config: Introduced new check TextBlockGoogleStyleFormatting checkstyle/contribution#955

Merged

mohitsatr force-pushed the textblockgooglestyle branch from 4099ebb to 5f76ebf Compare July 27, 2025 04:03

Zopsss reviewed Jul 27, 2025

View reviewed changes

mohitsatr force-pushed the textblockgooglestyle branch from 5f76ebf to 0d8e0dc Compare August 18, 2025 08:29

mohitsatr commented Aug 18, 2025

View reviewed changes

Zopsss suggested changes Aug 21, 2025

View reviewed changes

mohitsatr force-pushed the textblockgooglestyle branch from c8f7ea3 to 7007ab5 Compare August 27, 2025 13:47

romani requested changes Aug 28, 2025

View reviewed changes

Zopsss reviewed Aug 28, 2025

View reviewed changes

mohitsatr force-pushed the textblockgooglestyle branch from 7007ab5 to 093a01b Compare September 4, 2025 07:35

mohitsatr force-pushed the textblockgooglestyle branch from d34fb63 to d90a7a6 Compare November 28, 2025 02:05

romani requested changes Nov 30, 2025

View reviewed changes

Comment thread .../java/com/puppycrawl/tools/checkstyle/checks/coding/TextBlockGoogleStyleFormattingCheck.java Outdated

Comment thread src/site/xdoc/checks/coding/textblockgooglestyleformatting.xml.template Outdated

romani mentioned this pull request Nov 30, 2025

Xdoc marco to put version in xdoc from 'since' javadoc tag on Check #18218

Closed

Zopsss approved these changes Dec 1, 2025

View reviewed changes

This was referenced Dec 2, 2025

Extend TextBlockGoogleStyleFormatting to check indentation of each line in the blocks #18227

Closed

False-positive: Suppress indentation check when quotes start at the left margin #18228

Closed

romani force-pushed the textblockgooglestyle branch from d90a7a6 to 52a16bb Compare December 2, 2025 12:23

romani requested changes Dec 2, 2025

View reviewed changes

Comment thread src/site/xdoc/checks/coding/textblockgooglestyleformatting.xml.template Outdated

mohitsatr force-pushed the textblockgooglestyle branch from 52a16bb to d74722a Compare December 3, 2025 02:46

romani force-pushed the textblockgooglestyle branch from d74722a to 904f111 Compare December 4, 2025 15:22

romani force-pushed the textblockgooglestyle branch 3 times, most recently from 09a2e86 to 309b608 Compare December 5, 2025 00:46

mohitsatr force-pushed the textblockgooglestyle branch from 309b608 to 20d9764 Compare December 11, 2025 02:35

Issue checkstyle#17223: New Check TextBlockGoogleStyleFormattingCheck

5b69f8e

mohitsatr force-pushed the textblockgooglestyle branch from 20d9764 to 5b69f8e Compare December 11, 2025 04:33

romani approved these changes Dec 11, 2025

View reviewed changes

romani merged commit 43c7ae4 into checkstyle:master Dec 11, 2025
123 checks passed

mohitsatr deleted the textblockgooglestyle branch December 13, 2025 01:20

mohitsatr mentioned this pull request Dec 16, 2025

Issue #18064: added ArrayBracketNoWhitespace check #18240

Open

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zopsss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Zopsss commented Jul 13, 2025

Uh oh!

mohitsatr commented Jul 14, 2025

Uh oh!

Zopsss commented Jul 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohitsatr Aug 12, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zopsss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohitsatr Aug 25, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mohitsatr commented Jul 8, 2025 •

edited

Loading

mohitsatr Aug 12, 2025 •

edited

Loading

mohitsatr Aug 25, 2025 •

edited

Loading