Update AbstractChecks to log DetailAST (part3)

[MEZk]

Checkstyle has recently started to update old checks from logging on the line/column, to log on the AST node. This is needed to support https://checkstyle.org/config_filters.html#SuppressionXpathFilter which needs the AST of the violation to match it to the user supplied XPath.

As PR is started for each check, links from master ( Ex: https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/EmptyBlockCheck.java#L201 ) must be posted to show each log call found in the check. An additional note must be made of the number of logs actually modified. This will greatly help admins ensure the full check is now xpath compliant and nothing is missed.

All checks need to be reviewed to ensure:

1. All their calls to log pass the AST and not the line/column
2. Each log have a respective IT test to show that it supports xpath at https://github.com/checkstyle/checkstyle/tree/master/src/it/java/org/checkstyle/suppressionxpathfilter
3. Documentation of https://checkstyle.org/config_filters.html#SuppressionXpathFilter_Description shows the check is now supported.
4. Since this is modifying the functionality of a check, regression will be required for the PR. See https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-tester

1: We don't expect drastic change in current UTs when making the switch. If any items change the line/column drastically, please point them out to be clear to admins in the PR. Adding a column number when no was there before is ok and an expected change.
2: Must have a UT for each log in the check. If the check has 3 separate log calls, then it must have 3 separate UTs.
4: In report for each project, Number of unique base messages reported below and Number of unique path messages reported below should match otherwise it is saying there is an uneven amount of changes, either new violations or violations removed. We aren't expecting this type of change in these PRs unless you can account for it. One example where it would be acceptable is if duplicate violations on the same line were being dropped as we only report 1 violation per line/column/message/check.

---

[romani]

Just use first or more related AST and report violation on it.
Message need be rechecked and could be updated to be reasonable. We will see what to do in each particular case.

There are about 135 usages of this method in our code:

Most of them just need to be converted from log(aAst.getLineNo,... to log(aAst,... .

The only Checks that should stay reporting by line number is FilesetChecks (non Treewalker's Checks):

[rnveach]

How do we want to report handling a violation on a comment's specific line number?
Example: https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/TodoCommentCheck.java#L102

There is no AST inside the comment for a specific line. The AST will point to the start of the comment and the TODO could be 3 lines after that. Do we still want to print a violation on the AST?

---

Also how do we want to handle checks that don't even use the AST? I made an issue before that they should be changed to FileSet checks, but that was denied.
Example: https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/regexp/RegexpCheck.java#L280

[rnveach]

28 checks fit into the easy category where we just do log(ast.getLineNo(), MSG_....

[romani]

How do we want to report handling a violation on a comment's specific line number?

I do not know for now. It is better to postpone comment related Checks to most end of this project.

Do we still want to print a violation on the AST?

I would not doing this for now. Violation on AST will help extensions to do autofixes. Comment related violation is still unclear area. Let's postpone such Checks.

Also how do we want to handle checks that don't even use the AST? I

No AST no Xpath. They should continue to use any other way of suppression.

[rnveach]

No AST no Xpath. They should continue to use any other way of suppression.

We can't reconsider changing to a FileSet? We broke configurations when we did the same for Filters.

[romani]

I made an issue before that they should be changed to FileSet checks, but that was denied.

do you remember in what issue I rejected such proposal? We need to think about this one more time.
In most worse scenario we can take first token on this line and report violation by it.
damaging(braking compatibility) on users config is most unwanted scenario at least for now.

So we can update such Check to report violation on first AST in this line now ..... and in separate issue discuss reasons of Check to be under Treewalker.

[rnveach]

do you remember in what issue I rejected such proposal?

#2116 . Different check then mentioned here, but still same issue.

[romani]

conversion to FileSet is kind of reasonable but should be done with special attention and separately from Xpath project.

[rnveach]

PR #6199 is abandoned but anyone can take the code and finish it. Latest commit was rnveach@128bd56 .

What still needs to be done is we need a bunch of regression tests to show that xpath suppression works before we merge these changes. These tests will be located at https://github.com/checkstyle/checkstyle/tree/master/src/it/java/org/checkstyle/suppressionxpathfilter. This is similar to Issue #6207 but we need regression for the checks specifically modified in this issue.

[rnveach]

This issue is being closed as it was split into its children