Add the new option for Checkstyle CLI to generate the basic suppression xpath

[MEZk]

We need to add the new option for Checkstyle CLI to generate basic suppression xpath for the Checkstyle violations.

1. Identify the node which causes the violation (blocked by Add token type to LocalizedMessage #4419).
2. Map Xpath to AST tree (blocked by Mapper of XPath expressions onto AST nodes #4369).
3. Generate full xpath starting from the root of the tree to refere to the AST node which caused the violation. For example,

source file

public class TestClass {
    void foo() {
    }
}

AST

CLASS_DEF -> CLASS_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|  `--LITERAL_PUBLIC -> public [1:0]
|--LITERAL_CLASS -> class [1:7]
|--IDENT -> TestClass [1:13]
`--OBJBLOCK -> OBJBLOCK [1:23]
    |--LCURLY -> { [1:23]
    |--METHOD_DEF -> METHOD_DEF [2:4]
    |  |--MODIFIERS -> MODIFIERS [2:4]
    |  |--TYPE -> TYPE [2:4]
    |  |  `--LITERAL_VOID -> void [2:4]
    |  |--IDENT -> foo [2:9]
    |  |--LPAREN -> ( [2:12]
    |  |--PARAMETERS -> PARAMETERS [2:13]
    |  |--RPAREN -> ) [2:13]
    |  `--SLIST -> { [2:15]
    |      `--RCURLY -> } [3:4]
    `--RCURLY -> } [4:0]

node which caused the violation

|--METHOD_DEF -> METHOD_DEF [2:4]

Possible example of usage:

java -jar checkstyle-7.6-all.jar -t Test.java -supp "[2:4]"

Expected result

/CLASS_DEF//OBJBLOCK//METHOD_DEF[@text='foo']

[rnveach]

Checkstyle CLI to generate basic suppression xpath

We are not going to support GUI or will this be a different issue?

If I use checkstyle in maven, how will I generate suppression(s) as I don't have standalone all jar (as it is not a maven depedency)? Should we not be concerned with maven and force them to come up their own thing in the next version?

-t Test.java -supp "[2:4]"

Why do we need print ast option (-t) to also get suppression? Can't we do suppression by itself?
Should we support simply formats like 2:4 (no brackets)?

"[2:4]"
/CLASS_DEF//OBJBLOCK//METHOD_DEF[@text='foo']

What if my intention was to suppress the TYPE/LITERAL_VOID which is in the same line/column as METHOD_DEF as it got a violation from indentation (as example)? Both have the same column/line.
Will I be restricted to only certain places for xpath by line/column?
What if violation is only for a line and user isn't told column number? Will this behavior change for all Java checks so all violations have line and column?

/CLASS_DEF//OBJBLOCK//METHOD_DEF[@text='foo']

Shouldn't it be /CLASS_DEF[@text='TestClass']/..., as you can have multiple methods named foo in different classes?
If this class was in a package, would the package be auto-appended too to the xpath?

[romani]

@MEZk ,

-supp "[2:4]"

expected output should be all tokens for "[2:4]"

/CLASS_DEF[@text='TestClass']//OBJBLOCK//METHOD_DEF[@text='foo']
/CLASS_DEF[@text='TestClass']//OBJBLOCK//MODIFIERS
/CLASS_DEF[@text='TestClass']//OBJBLOCK//TYPE
/CLASS_DEF[@text='TestClass']//OBJBLOCK//LITERAL_VOID

we could think about approach like -supp "METHOD_DEF[2:4]", but I think it is over complication.
Attention to LITERAL_VOID, it could be LITERAL_VOID[@text='void'], but I think we do not need that text.

[romani]

We are not going to support GUI or will this be a different issue?

we will do GUI, but looks like CLI will be more convenient for first steps. I started to use CLI tree printing almost all the time. CLI is mostly for our convenience.

Why do we need print ast option (-t) to also get suppression?

agree, no need.

Should we support simply formats like 2:4 (no brackets)?

agree.

Both have the same column/line. Will I be restricted to only certain places for xpath by line/column?

agree, see my comment above.

What if violation is only for a line and user isn't told column number?

@MEZk , please investigate and give us report, with all cases.

Will this behavior change for all Java checks so all violations have line and column?

no changes in Checks are expected.

Shouldn't it be /CLASS_DEF[@text='TestClass']/..., as you can have multiple methods named foo in different classes?

CLI should use all text fields if it smth valuable (not a literal, has the same content as token name, ....)

If this class was in a package, would the package be auto-appended too to the xpath?

package node are siblings, I do not think we need them, we can do this after GSOC if that become required.

[MEZk]

@romani @rnveach

expected output should be all tokens for "[2:4]"

Yes. We need to print all Xpath expressions. In general, we should avoid specifying of token type. User does not need to remember the token type if he wants to generate suppression xpath.

we could think about approach like -supp "METHOD_DEF[2:4]", but I think it is over complication.

Yes, you are right.

Should we support simply formats like 2:4 (no brackets)?

Yes.

Both have the same column/line. Will I be restricted to only certain places for xpath by line/column?

All xpath expressions should be printed for the specified column and line number.

[romani]

@MEZk

Please update issue description to specify all details, I will remove all our comments to keep issue clear

[MEZk]

@MEZk , please investigate and give us report, with all cases.

@romani
These checks logs only line number:

1. AnnotationLocationCheck.

InputAnnotationLocationIncorrect.java:70: Annotation 'MyAnnotation2' have incorrect indentation level 7, expected level should be 4.

2. AbbreviationAsWordInNameCheck

InputAbbreviationAsWordInNameType.java:1: Abbreviation in name 'FactoryWithBADNAme' must contain no more than '4' consecutive capital letters.

3. AnnotationUseStyleCheck

InputAnnotationUseStyleDifferentStyles.java:12: Annotation must have closing parenthesis.

4. ArrayTrailingCommaCheck

InputArrayTrailingComma.java:117: Array should contain trailing comma.

5. AtclauseOrderCheck

InputIncorrectAtClauseOrder.java:91: At-clauses have to appear in the order '[@author, @version, @param, @return, @throws, @exception, @see, @since, @serial, @serialField, @serialData, @deprecated]'.

6. AvoidEscapedUnicodeCharactersCheck

InputAvoidEscapedUnicodeCharacters.java:71: Unicode escape(s) usage should be avoided.

7. AvoidStarImportCheck

InputAvoidStarImportDefault.java:71: Using the '.*' form of import should be avoided - com.puppycrawl.tools.checkstyle.checks.imports.*.

8. AvoidStaticImportCheck

InputAvoidStaticImportDefault.java:213: Using a static member import should be avoided - java.io.File.listRoots.

9. CommentsIndentationCheck

InputCommentsIndentationCommentIsAtTheEndOfBlock.java:181: Comment has incorrect indentation level 25, expected is 8, indentation should be the same level as line 17.

10. CommentsIndentationCheck

InputCommentsIndentationCommentIsAtTheEndOfBlock.java:118: Comment has incorrect indentation level 25, expected is 8, indentation should be the same level as line 17.

11. EmptyCatchBlockCheck

InputEmptyCatchBlockDefault.java:351: Empty catch block.

12. EmptyLineSeparatorCheck

InputEmptyLineSeparator.java:211: 'import' should be separated from previous statement.

13. FileLengthCheck

InputFileLength.java:11: File length is 225 lines (max allowed is 20).

14. FinalClassCheck

InputFinalClass.java:71: Class InputFinalClass should be declared as final.

15. HeaderCheck

InputHeader.java:11: Missing a header - not enough lines in file.

16. ImportOrderCheck

InputImportOrder.java:15: Wrong order for 'java.awt.Dialog' import.

17. IndentationCheck

InputIndentationMethodCStyle.java:210: 'int' has incorrect indentation level 29, expected level should be 12.

18. InterfaceIsTypeCheck

InputInterfaceIsType.java:251: interfaces should describe a type and hence have methods.

19. JavadocMethodCheck

InputTags.java:241: Expected an @return tag.

20. JavadocPackageCheck

InputBadCls.java:1: Missing package-info.java file.

21. JavadocParagraphCheck

InputIncorrectJavaDocParagraph.java:71: <p> tag should be placed immediately before the first word, with no space after.

22. JavadocStyleCheck

InputJavadocStyle.java:201: First sentence should end with a period.

23. JavadocTagContinuationIndentationCheck

InputJavaDocTagContinuationIndentation.java:471: Line continuation have incorrect indentation level, expected level should be 4.

24. JavadocTypeCheck

InputTags.java:81: Missing a Javadoc comment.

25. LineLengthCheck

InputLineLength.java:51: Line is longer than 100 characters (found 112).

26. MethodCountCheck

InputMethodCount.java:31: Number of private methods is 5 (max allowed is 3).

27. MissingCtorCheck

InputMissingCtor.java:13: Class should define a constructor.

28. MissingDeprecatedCheck

InputMissingDeprecatedBadDeprecated.java:17: Must include both @java.lang.Deprecated annotation and @deprecated Javadoc tag with description.

29. MissingOverrideCheck

InputMissingOverrideBadOverrideFromObject.java:81: Must include @java.lang.Override annotation when {@inheritDoc} Javadoc tag exists.

30. MissingSwitchDefaultCheck

InputMissingSwitchDefault.java:171: switch without "default" clause.

31. NeedBracesCheck

InputNeedBraces.java:219: 'do' construct must use '{}'s.

32. NewlineAtEndOfFileCheck

InputNoNewlineAtEndOfFile.java:10: File does not end with a newline.

33. NoCloneCheck

InputNoClone.java:101: Avoid using clone method.

34. NoFinalizerCheck

InputNoFinalizerHasFinalizer.java:15: Avoid using finalizer method.

35. NoLineWrapCheck

InputNoLineWrapBad.java:11: package statement should not be line-wrapped.

36. NonEmptyAtclauseDescriptionCheck

InputNonEmptyAtclauseDescription.java:216: At-clause should have a non-empty description.

37. OneTopLevelClassCheck

InputOneTopLevelClassNoPublic.java:18: Top-level class InputOneTopLevelClassNoPublic2 has to reside in its own source file.

38. OuterTypeFilenameCheck

InputOuterTypeFilename5.java:41: The name of the outer type and the file do not match.

39. OverloadMethodsDeclarationOrderCheck

InputOverloadMethodsDeclarationOrder.java:128: Overload methods should not be split. Previous overloaded method located at line '17'.

and many more.

The above mentioned report shows that there are many checks which logs only line number.

no changes in Checks are expected.

We need to pass TokenType through log method to LocalizedMessage. So it will be required to update all checks.

[romani]

The above mentioned report shows that there are many checks which logs only line number.

true to say , it was done only because authors were lazy to put violation on certain token(name or ......).
It even causes problems to implement "Quick fix" feature in eclipse-cs and other plugins as IDE do not know exact place of violation (too much of code in one line).

We need to pass TokenType through log method to LocalizedMessage. So it will be required to update all checks.

Yes, confirmed. In this case you can report a column too. To make it requirement to report 3 elements (lineNo, colNo, tokenType). It might be reasonable for Check to pass to log(....) DetailAST element, and log method do know what to take from DetailAST to make LocalizedMessage. In this case updates to Checks will be minimal as most of them do "log(ast.getLineno(),. ......);" so it will become "log(ast,......)".

@MEZk , please investigate this, and confirm that it is reasoble.
please also provide me/student a table what DetailAST to use to report issue. To let me confirm before code changes started and let student quickly to refactoring.

[MEZk]

@romani @timurt @rnveach
Split the issue into two parts:

1. Implement xpath query generator. Implement xpath query generator #4901
2. The current issue. The main idea is to add the new option to use the implemented generator and print the results to the console.

[MEZk]

@romani @rnveach
Can we move parseFile method from TestUtils to AstUtils in main src area and use the method in Main to get DetailAST in order to pass it to XpathQueryGenerator or can we use parseWithComments static method of TreeWalker class? XpathQueryGenerator will receive ast, columnNo and lineNo in order to generate xpath query. The generator is implemented in #4911 .

[rnveach]

move parseFile method from TestUtils to AstUtils

Instead of duplicating this method, why don't we make an existing utility one public?
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/AstTreeStringPrinter.java#L234

[MEZk]

Is is a good idea to invoke method from the class which name ends with *StringPrinter?

[rnveach]

Since that method isn't printing anything, should we just move it out to a different utility class?
You basically want the same method, right?

[MEZk]

Yes, we can create a new utility class since now we have only TokenUtils class which is somehow connected with the tree.

[romani]

@MEZk ,

Can we move parseFile method from TestUtils to AstUtils in main src area

you can if you need, it is located in test only because it was required in test area only. What ever moving around utils methods is fine.

[MEZk]

@timurt
I think that you can start the implementation. You have already implemented Xpath Generator, now you should connect it to CLI in your local branch. When we merge #4911, you are able to send PR immediately. Should you have any questions, please ask.

[timurt]

@MEZk
So I should add new option to Main.java to support xpath query generator
And create additional utility class and move parse file to AST logic to this class.
Should AstTreeStringPrinter use this util method?

[romani]

Should AstTreeStringPrinter use this util method?

Yes

java -jar checkstyle-7.6-all.jar -t Test.java -supp "[2:4]"

It might be better to do it like:

java -jar checkstyle-7.6-all.jar -s "2:4" Test.java
java -jar checkstyle-7.6-all.jar --xpath-suppression "2:4" Test.java

@MEZk , please confirm that proposal.

[MEZk]

@romani

It might be better to do it like:

Yes, parentheses are not required.

[MEZk]

@timurt
You are free to work on this issue.

[romani]

@MEZk , @timurt attention, I removed -t too. Suppression generation should not depend on tree printing. It is suppression printing.

[MEZk]

@timurt
After discussion with @romani we came up to a conclusion that CLI should have 2 additional options:

1. -s "2:4" - to specify lineNo and columnNo for the suppression
2. -tabWith - to specify the length of the tab character. For example, if -tabWith is set to 4 it means that '\t' should be treated as 4 characters. This option should help us to support different IDEs. The option should be passed to XPath generator. I will explain the meaning of the option in separate issue, but please be aware of this fact.

[MEZk]

I will explain the meaning of the option in separate issue, but please be aware of this fact.

Issues:
#4998
#4999

[rnveach]

Fix was merged