Issue #3095: Add COMPILATION_UNIT token in Ast Tree, remove EOF token

[nrmancuso]

Closes #3095.

Check Regression Reports

https://nmancus1.github.io/issue-3095-b_check_diff_reports_2021_08_11/diff_checks-nonjavadoc-error/index.html

• All diffs are in exception message, example: https://www.diffchecker.com/efLWFs9j

---

https://nmancus1.github.io/issue-3095-b_check_diff_reports_2021_08_11/diff_checks-only-javadoc-error/index.html

• All diffs are in exception message, example: https://www.diffchecker.com/efLWFs9j

---

https://nmancus1.github.io/issue-3095-b_check_diff_reports_2021_08_11/diff_part1/index.html

• Clean

---

https://nmancus1.github.io/issue-3095-b_check_diff_reports_2021_08_11/diff_part2/index.html

• Clean

---

https://nmancus1.github.io/issue-3095-b_check_diff_reports_2021_08_11/diff_part3/index.html

• Clean

---

https://nmancus1.github.io/issue-3095-b_check_diff_reports_2021_08_11/diff_part4/index.html

• Clean

---

https://nmancus1.github.io/issue-3095-b_check_diff_reports_2021_08_11/diff_part5/index.html

• Clean

---

https://nmancus1.github.io/issue-3095-b_check_diff_reports_2021_08_11/diff_part6/index.html

• Clean

---

https://nmancus1.github.io/issue-3095-b_check_diff_reports_2021_08_11/diff_sevntu-check-regression_part_1/index.html

• Regression detected in InnerClassCheck, LineLengthExtendedCheck, and PublicReferenceToPrivateTypeCheck
• PR opened at minor: update checks (related to checkstyle#3095) sevntu-checkstyle/sevntu.checkstyle#858

---

https://nmancus1.github.io/issue-3095-b_check_diff_reports_2021_08_11/diff_sevntu-check-regression_part_2/index.html

• Regression detected in AvoidDefaultSerializableInInnerClasses
• PR opened at minor: update checks (related to checkstyle#3095) sevntu-checkstyle/sevntu.checkstyle#858

---

AST Regression Reports

It will be very challenging to share or view AST regresion report for this PR:

File size of report folder is 4.5 GB; every line of printed AST in the entire repo will have a diff, since we are adding a root node.

[romani]

@nmancus1 , please resolve conflict

[romani]

Items

[romani]

I did not expect updates in Checks a lot, but we have affect on a lot of them.
Lets double check that introduction of root token have enough benefits, as whole world of thirdparty checks might be affected in same way.

most interesting update:

✔ ~/java/github/romani/checkstyle [nmancus1/issue-3095-b L|✔] 
$ git show --stat | grep -v "XpathReg" | grep -v ".txt" | grep -v ".xml" | grep -v "gression" | grep -v "Input" | grep ".../"
 .../tools/checkstyle/JavaAstVisitor.java           |   15 +-
 .../puppycrawl/tools/checkstyle/JavaParser.java    |   10 +-
 .../checkstyle/JavadocPropertiesGenerator.java     |    2 +-
 .../tools/checkstyle/api/TokenTypes.java           |   50 +-
 .../checkstyle/checks/OuterTypeFilenameCheck.java  |    2 +-
 .../checks/annotation/MissingDeprecatedCheck.java  |    2 +-
 .../checkstyle/checks/blocks/LeftCurlyCheck.java   |    9 +-
 .../checks/coding/OneStatementPerLineCheck.java    |    7 +-
 .../checks/design/InnerTypeLastCheck.java          |    2 +-
 .../checks/design/OneTopLevelClassCheck.java       |   14 +-
 .../checks/imports/UnusedImportsCheck.java         |    3 +-
 .../indentation/CommentsIndentationCheck.java      |    6 +-
 .../checks/javadoc/AtclauseOrderCheck.java         |   17 +-
 .../checks/modifier/RedundantModifierCheck.java    |    7 +-
 .../checks/whitespace/GenericWhitespaceCheck.java  |    3 +-
 .../checkstyle/filters/SuppressionXpathFilter.java |   10 +-
 .../filters/SuppressionXpathSingleFilter.java      |   22 +-
 .../tools/checkstyle/gui/ParseTreeTableModel.java  |    2 +-
 .../checkstyle/gui/ParseTreeTablePresentation.java |   28 +-
 .../checkstyle/utils/BlockCommentPosition.java     |   31 +-
 .../tools/checkstyle/utils/CheckUtil.java          |    2 +-
 .../tools/checkstyle/xpath/RootNode.java           |    2 +-
 .../checkstyle/xpath/XpathQueryGenerator.java      |    6 +-
 .../tools/checkstyle/grammar/java/JavaLexer.g4     |    7 +-
 .../tools/checkstyle/AstTreeStringPrinterTest.java |   13 +-
 .../tools/checkstyle/JavaAstVisitorTest.java       |    2 +-
 .../checkstyle/JavadocDetailNodeParserTest.java    |    2 +-
 .../com/puppycrawl/tools/checkstyle/MainTest.java  |  253 +-
 .../checkstyle/SuppressionsStringPrinterTest.java  |   19 +-
 .../XpathFileGeneratorAstFilterTest.java           |    7 +-
 .../XpathFileGeneratorAuditListenerTest.java       |   12 +-
 .../tools/checkstyle/api/FullIdentTest.java        |   18 +-
 .../checks/design/FinalClassCheckTest.java         |    2 +-
 .../checks/design/OneTopLevelClassCheckTest.java   |   24 +-
 .../checks/design/VisibilityModifierCheckTest.java |    2 +-
 .../checks/javadoc/JavadocMethodCheckTest.java     |    8 +
 .../filters/SuppressionXpathSingleFilterTest.java  |   15 +-
 .../checkstyle/filters/SuppressionsLoaderTest.java |    4 +-
 .../checkstyle/filters/XpathFilterElementTest.java |   15 +-
 .../grammar/GeneratedJavaTokenTypesTest.java       |    3 +-
 .../gui/ParseTreeTablePresentationTest.java        |   14 +-
 .../tools/checkstyle/utils/TokenUtilTest.java      |   11 +-
 .../tools/checkstyle/utils/XpathUtilTest.java      |   55 +-
 .../tools/checkstyle/xpath/ElementNodeTest.java    |    2 +-
 .../tools/checkstyle/xpath/RootNodeTest.java       |    2 +-
 .../tools/checkstyle/xpath/XpathMapperTest.java    |  152 +-
 .../checkstyle/xpath/XpathQueryGeneratorTest.java  |  150 +-

[rnveach]

Since this is for the 9.0 version, I more for breaking compatibility.

I am not very sure on the benefit part, but its a change that makes sense. It is weird to be given a node that is not a root and having to do next sibling on it to get the rest of the CU when dealing with the beginTree/endTree methods.

I don't see a reason not to do this.

[nrmancuso]

I really did not see a great deal of value in this issue when I started on this, but, after working on check updates, I think that adding a root node makes a lot of sense:

1. It helps to maintain consistency in API; when writing checks, one of the first considerations should be which tokens to check. This change would allow top level checks to now specify a token.

2. Having a check traverse the tree itself is discouraged. From https://checkstyle.sourceforge.io/writingchecks.html#Understanding_the_visitor_pattern:

It is important to understand that the individual Checks do not drive the AST traversal (it possible to traverse itself, but not recommended).

3. As you can see from the changes to existing checks (all are minor), it is more informative to write if (ast.getType() == TokenTypes.COMPILATION_UNIT) than if (ast == null) or while (parent.getType() != TokenTypes.COMPILATION_UNIT) instead of while (parent != null). This helps the code to be more self-documenting, because we are explicitly stating that we want to check to see if this is a top level node in the AST.
4. As @rnveach states above, it seems nonsensical to have a tree with no "root".

as whole world of thirdparty checks might be affected in same way.

Yes, but update is simple: either get the first child of COMPILATION_UNIT in beginTree()and let the check function as it did previously, or add the COMPILATION_UNIT token to your check and treat it like other checks now (with visitToken()). Also, we have the isRootNode() method to check for top level nodes now when ascending to the top of the tree.

[romani]

items:

[romani]

OkOk to merge

[romani]

@nmancus1 , please resolve conflict

[rnveach]

Has any regression been done to sevntu? This will also be a breaking change for it.
EDIT: There is a blocked PR for that in sevntu.