New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mapper of XPath expressions onto AST nodes #4369

Closed
MEZk opened this Issue May 22, 2017 · 29 comments

Comments

@MEZk
Contributor

MEZk commented May 22, 2017

We need to investigate how to map XPath expressions onto AST nodes. To begin with, it is worth designing a model of mapper which will allow to map XPath expressions onto AST nodes. The main idea of the mapper is to connect XPath expressions which will be based on the values of TokenTypes enum onto the nodes of DetailAST.

For example, if we have the following code fragment:

public class TestClass {
    void method() {
    }
}

the following tree structure:

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 -> method [2:9]
    |  |--LPAREN -> ( [2:15]
    |  |--PARAMETERS -> PARAMETERS [2:16]
    |  |--RPAREN -> ) [2:16]
    |  `--SLIST -> { [2:18]
    |      `--RCURLY -> } [3:4]
    `--RCURLY -> } [4:0]

and the following XPath expression (example, format should be discussed):

//ClassDef[@Text='TestClass']

than the parser should connect the XPath expression elements with the AST nodes in this way:

DetailAST (type: CLASS_DEF, text: "TestClass") <- XpathExpression(ClassDef, text: "TestClass")

The base idea can be taken from PMD. They implemented both Saxon and Jaxen mappers. From my point of view, Saxon mapper is easier to reimplement for our needs. Saxon XPath rule mapper maps each XPath node onto AST node. Their algorithm is based on the tree traversal algorithm. They map each node of the AST starting with root onto XPath element.

Thus, what should be done in the scope of this issue:

  1. Design XPath expression format for the new Checkstyle Suppression Model;
  2. Investigate what library (Saxon, Jaxen, or native Java) should be used to implement mapper;
  3. Provide base details of mapper model implementation (UML diagram of classes can be a good example).

@MEZk MEZk changed the title from Investigate how to map XPath expression onto AST nodes to Investigate how to map XPath expressions onto AST nodes May 22, 2017

@MEZk MEZk added the question label May 22, 2017

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt May 31, 2017

Collaborator

@MEZk
I am agree with you, I have tried both Jaxen and Saxon and noticed some moments.

  1. Both implementations can work on DOM, XOM, DOM4j and JDOM, but Saxon also supports JDOM2 and AXIOM tree models
  2. Jaxen project is probably not active now, last changes on Git repository were about 2 years ago (https://github.com/jaxen-xpath/jaxenand official site is unavailable (http://jaxen.codehaus.org/, www.jaxen.org/), while Saxon's last release was in 5 April 2017.
  3. Jaxen does not support XPATH 2.0, while Saxon does
  4. PMD uses Jaxen for own suppression model. They also allow to select Jaxen/Saxon for writing custom rules.
    I think Saxon library is more reliable and has more functionalities comparing to Jaxen.
Collaborator

timurt commented May 31, 2017

@MEZk
I am agree with you, I have tried both Jaxen and Saxon and noticed some moments.

  1. Both implementations can work on DOM, XOM, DOM4j and JDOM, but Saxon also supports JDOM2 and AXIOM tree models
  2. Jaxen project is probably not active now, last changes on Git repository were about 2 years ago (https://github.com/jaxen-xpath/jaxenand official site is unavailable (http://jaxen.codehaus.org/, www.jaxen.org/), while Saxon's last release was in 5 April 2017.
  3. Jaxen does not support XPATH 2.0, while Saxon does
  4. PMD uses Jaxen for own suppression model. They also allow to select Jaxen/Saxon for writing custom rules.
    I think Saxon library is more reliable and has more functionalities comparing to Jaxen.
@vanniktech

This comment has been minimized.

Show comment
Hide comment
@vanniktech

vanniktech May 31, 2017

How about not mapping it to AST and instead map it to UAST (Universal Abstract Syntax Tree)? That way also adding support for Kotlin, which is an official thing for Android now, can be supported.

vanniktech commented May 31, 2017

How about not mapping it to AST and instead map it to UAST (Universal Abstract Syntax Tree)? That way also adding support for Kotlin, which is an official thing for Android now, can be supported.

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt May 31, 2017

Collaborator

@MEZk

Design XPath expression format for the new Checkstyle Suppression Model;

I compared AST components between PMD and Checkstyle . I think using constants from TokenTypes as parts for XPath queries is not good idea. For example CTOR_DEF is not so obvious for Constructor declaration and LITERAL_while contains underscore,lowercase and uppercase characters which is difficult for user. I like PMD format, all AST components names consist of description and category: ASTAnnotationMethodDeclaration, ASTPreIncrementExpression, ASTBreakStatement and etc.

Having full names in camel-case format instead of shorten tokens will make Xpath queries more understandable I guess. Here some mapping examples:
CTOR_DEF -> ConstructorDeclaration
METHOD_DEF -> MethodDeclaration
VARIABLE_DEF -> VariableDeclaration
LITERAL_if -> IfStatement
LITERAL_while -> WhileStatement

Here some queries examples:
//CLASS_DEF[@Text='TestClass']/METHOD_DEF/SLIST/LITERAL_if
vs
//ClassDeclaration[@Text='TestClass']/MethodDeclaration/StatementsList/IfStatement

Collaborator

timurt commented May 31, 2017

@MEZk

Design XPath expression format for the new Checkstyle Suppression Model;

I compared AST components between PMD and Checkstyle . I think using constants from TokenTypes as parts for XPath queries is not good idea. For example CTOR_DEF is not so obvious for Constructor declaration and LITERAL_while contains underscore,lowercase and uppercase characters which is difficult for user. I like PMD format, all AST components names consist of description and category: ASTAnnotationMethodDeclaration, ASTPreIncrementExpression, ASTBreakStatement and etc.

Having full names in camel-case format instead of shorten tokens will make Xpath queries more understandable I guess. Here some mapping examples:
CTOR_DEF -> ConstructorDeclaration
METHOD_DEF -> MethodDeclaration
VARIABLE_DEF -> VariableDeclaration
LITERAL_if -> IfStatement
LITERAL_while -> WhileStatement

Here some queries examples:
//CLASS_DEF[@Text='TestClass']/METHOD_DEF/SLIST/LITERAL_if
vs
//ClassDeclaration[@Text='TestClass']/MethodDeclaration/StatementsList/IfStatement

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 3, 2017

Contributor

@timurt

  1. LITERAL_while is not a constant from TokenTypes enum.
  2. ClassDeclaration, MethodDeclaration, StatementsList, are not attributes of DetailAST, but TokenTypes are. In order to connect them to DetailAST we will need to implement converter to bind them to one of the attributes of DetailAST.
  3. PMD uses the names as their tree nodes are called in that way. They have an hierarchy of AST nodes, we don't. The nodes in our AST can be identified by token types.

I have already created the basic implementation of the mapper. I'll share it with you soon.

Contributor

MEZk commented Jun 3, 2017

@timurt

  1. LITERAL_while is not a constant from TokenTypes enum.
  2. ClassDeclaration, MethodDeclaration, StatementsList, are not attributes of DetailAST, but TokenTypes are. In order to connect them to DetailAST we will need to implement converter to bind them to one of the attributes of DetailAST.
  3. PMD uses the names as their tree nodes are called in that way. They have an hierarchy of AST nodes, we don't. The nodes in our AST can be identified by token types.

I have already created the basic implementation of the mapper. I'll share it with you soon.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 3, 2017

Contributor

@vanniktech

How about not mapping it to AST and instead map it to UAST (Universal Abstract Syntax Tree)? That way also adding support for Kotlin, which is an official thing for Android now, can be supported.

Checkstyle works only with AST as we depend on ANTLR and java grammar.

Contributor

MEZk commented Jun 3, 2017

@vanniktech

How about not mapping it to AST and instead map it to UAST (Universal Abstract Syntax Tree)? That way also adding support for Kotlin, which is an official thing for Android now, can be supported.

Checkstyle works only with AST as we depend on ANTLR and java grammar.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 5, 2017

Contributor
  • The main idea of the mapper is to connect XPath expressions which will be based on the values of TokenTypes enum onto the nodes of DetailAST.
  • We can use Saxon library to work with XPath expressions. XPath expressions consists of document nodes. Each node can be mapped to corresponding AST node. Thus, we need to implement our own XPath-AST document-based tree model. Saxon offers an interface NodeInfo which represents a node in Saxon's implementation of the XPath 2.0 data model. This is the primary interface for accessing trees in Saxon, and it forms part of the public Saxon API. In our case, we need to design our own implementation which will combine the information about XPath and AST nodes. We also need the implementation of the root (document) node which will contain the reference to the root of XPath-AST document-based tree.
  • The class XPathExpression from Saxon represents XPath expression which can be executed, returning the result as a List, whose members will be instances of the class which implements NodeInfo interface.
  • To perform AST tree traversal with XPath expression we need to extend the Navigator class from Saxon in order to apply its algorithms to XPath-AST document-based tree. The Navigator class provides helper classes for navigating a tree, irrespective of its implementation.
  • Each AST node has its attributes (methods which access fields information). XPath expression can use the attributes to refer to AST node attributes. To iterate over the attributes the Navigator subclass is required.

Documentation:
http://www.saxonica.com/html/documentation9.6/expressions/range.html
http://saxon.sourceforge.net/saxon7.6/api-guide.html

Contributor

MEZk commented Jun 5, 2017

  • The main idea of the mapper is to connect XPath expressions which will be based on the values of TokenTypes enum onto the nodes of DetailAST.
  • We can use Saxon library to work with XPath expressions. XPath expressions consists of document nodes. Each node can be mapped to corresponding AST node. Thus, we need to implement our own XPath-AST document-based tree model. Saxon offers an interface NodeInfo which represents a node in Saxon's implementation of the XPath 2.0 data model. This is the primary interface for accessing trees in Saxon, and it forms part of the public Saxon API. In our case, we need to design our own implementation which will combine the information about XPath and AST nodes. We also need the implementation of the root (document) node which will contain the reference to the root of XPath-AST document-based tree.
  • The class XPathExpression from Saxon represents XPath expression which can be executed, returning the result as a List, whose members will be instances of the class which implements NodeInfo interface.
  • To perform AST tree traversal with XPath expression we need to extend the Navigator class from Saxon in order to apply its algorithms to XPath-AST document-based tree. The Navigator class provides helper classes for navigating a tree, irrespective of its implementation.
  • Each AST node has its attributes (methods which access fields information). XPath expression can use the attributes to refer to AST node attributes. To iterate over the attributes the Navigator subclass is required.

Documentation:
http://www.saxonica.com/html/documentation9.6/expressions/range.html
http://saxon.sourceforge.net/saxon7.6/api-guide.html

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 8, 2017

Member

and the following XPath expression (example, format should be discussed): //ClassDef[@Text='TestClass']

It should be //CLASS_DEF[@text='TestClass'] , TokenType is used as name ,all attributes are properties from DetailAST.

Jaxen project is probably not active now, last changes on Git repository were about 2 years ago

yes, lets avoid it.

LITERAL_while contains underscore,lowercase and uppercase characters which is difficult for use

it is public static final int LITERAL_WHILE = ... - code

I like PMD format, all AST components names consist of description and category: ASTAnnotationMethodDeclaration, ASTPreIncrementExpression, ASTBreakStatement and etc.

It is there AST structure and name - so they do not invent new names too, what is used in development that is used in xpath.
it is verbose, and verbose is good when smb read this text on daily basis, or need to create expression without help of any tool. Creation of suppression in PMD in very hard process. I still a problem for to make new suppression for PMD and I always use existing examples of their tree tool to collect name of nodes.
They do have problems with names too - @Image
PMDs suppressions are long - https://github.com/checkstyle/checkstyle/blob/master/config/pmd.xml#L33 , additional pain is too have several path is one string - very hard to maintain. It will be very good if user can make several suppress lines in his config to keep long xpath separately and easy to read.

Our user already aware of AST tokens, they configure Checks by them, they already know about AST , they do not know exact structure (nobody in the world can predict AST tree), so we need a tool that will generate xpath for user to copy paste in configuration.

If DetailAST extend, we do not need to change our xpath processor/matcher, as it will just for with it as with java bean.

Member

romani commented Jun 8, 2017

and the following XPath expression (example, format should be discussed): //ClassDef[@Text='TestClass']

It should be //CLASS_DEF[@text='TestClass'] , TokenType is used as name ,all attributes are properties from DetailAST.

Jaxen project is probably not active now, last changes on Git repository were about 2 years ago

yes, lets avoid it.

LITERAL_while contains underscore,lowercase and uppercase characters which is difficult for use

it is public static final int LITERAL_WHILE = ... - code

I like PMD format, all AST components names consist of description and category: ASTAnnotationMethodDeclaration, ASTPreIncrementExpression, ASTBreakStatement and etc.

It is there AST structure and name - so they do not invent new names too, what is used in development that is used in xpath.
it is verbose, and verbose is good when smb read this text on daily basis, or need to create expression without help of any tool. Creation of suppression in PMD in very hard process. I still a problem for to make new suppression for PMD and I always use existing examples of their tree tool to collect name of nodes.
They do have problems with names too - @Image
PMDs suppressions are long - https://github.com/checkstyle/checkstyle/blob/master/config/pmd.xml#L33 , additional pain is too have several path is one string - very hard to maintain. It will be very good if user can make several suppress lines in his config to keep long xpath separately and easy to read.

Our user already aware of AST tokens, they configure Checks by them, they already know about AST , they do not know exact structure (nobody in the world can predict AST tree), so we need a tool that will generate xpath for user to copy paste in configuration.

If DetailAST extend, we do not need to change our xpath processor/matcher, as it will just for with it as with java bean.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 8, 2017

Contributor

@romani

It should be //CLASS_DEF[@text='TestClass'] , TokenType is used as name ,all attributes are properties from DetailAST.

Yes, your are right. It was my first attempt to show the format. The Xpath will be constructed based on TokenType enum values.

it is public static final int LITERAL_WHILE = ... - code

Yes. Should be LITERAL_WHILE.

They do have problems with names too - @image

It was their design mistake, as for me, to save node name in the field with name "image".

additional pain is too have several path is one string - very hard to maintain. It will be very good if user can make several suppress lines in his config to keep long xpath separately and easy to read

We can allow to have as many suppression-xpath elements as user wants.

Our user already aware of AST tokens, they configure Checks by them, they already know about AST

That was my main point.

Our user already aware of AST tokens, they configure Checks by them, they already know about AST

Yes. We will have problems only if the AST structure is changed. For example, if METHOD_DEF will become the parent of CLASS_DEF :), etc.

Contributor

MEZk commented Jun 8, 2017

@romani

It should be //CLASS_DEF[@text='TestClass'] , TokenType is used as name ,all attributes are properties from DetailAST.

Yes, your are right. It was my first attempt to show the format. The Xpath will be constructed based on TokenType enum values.

it is public static final int LITERAL_WHILE = ... - code

Yes. Should be LITERAL_WHILE.

They do have problems with names too - @image

It was their design mistake, as for me, to save node name in the field with name "image".

additional pain is too have several path is one string - very hard to maintain. It will be very good if user can make several suppress lines in his config to keep long xpath separately and easy to read

We can allow to have as many suppression-xpath elements as user wants.

Our user already aware of AST tokens, they configure Checks by them, they already know about AST

That was my main point.

Our user already aware of AST tokens, they configure Checks by them, they already know about AST

Yes. We will have problems only if the AST structure is changed. For example, if METHOD_DEF will become the parent of CLASS_DEF :), etc.

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jun 15, 2017

Collaborator

@MEZk @rnveach
Sorry for delay, was struggling with Saxon Navigator/NodeInfo implementation.
Here is My commit where I have created simple Saxon xpath implementation with tests.

The realization is pretty simple:
Node - general interface
AbstractNode - abstract class implements Node
DocumentNode - actually this is not node, it is info about document
RootNode - class represents root node, extends AbstractNode
ElementNode - class represents ClassDefinition node

Is this correct way?

Collaborator

timurt commented Jun 15, 2017

@MEZk @rnveach
Sorry for delay, was struggling with Saxon Navigator/NodeInfo implementation.
Here is My commit where I have created simple Saxon xpath implementation with tests.

The realization is pretty simple:
Node - general interface
AbstractNode - abstract class implements Node
DocumentNode - actually this is not node, it is info about document
RootNode - class represents root node, extends AbstractNode
ElementNode - class represents ClassDefinition node

Is this correct way?

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 18, 2017

Contributor

@timurt
How are you going to connect the nodes with AST nodes? Just TokenType is not enough.

Contributor

MEZk commented Jun 18, 2017

@timurt
How are you going to connect the nodes with AST nodes? Just TokenType is not enough.

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jun 19, 2017

Collaborator

@MEZk
Added DetailNode and DetailAst fields to node
Implemented [@Image='SomeClass']
Added javadoc

Collaborator

timurt commented Jun 19, 2017

@MEZk
Added DetailNode and DetailAst fields to node
Implemented [@Image='SomeClass']
Added javadoc

timurt added a commit to timurt/checkstyle that referenced this issue Jun 20, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jun 22, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jun 22, 2017

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 24, 2017

Contributor

@timurt
First implementation can support only direct xpath-expressions. For example, there might be two ways of suppressing a violation for the method 'foo':

public class TestClass {
    void foo() {
    }
}
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:15]
    |  |--PARAMETERS -> PARAMETERS [2:16]
    |  |--RPAREN -> ) [2:16]
    |  `--SLIST -> { [2:18]
    |      `--RCURLY -> } [3:4]
    `--RCURLY -> } [4:0]
  1. /CLASS_DEF//OBJBLOCK//METHOD_DEF[@text='foo']
  2. METHOD_DEF[@text='foo']

First implementation of suppression model can handle only case 1. We will add support for case 2 later.

Contributor

MEZk commented Jun 24, 2017

@timurt
First implementation can support only direct xpath-expressions. For example, there might be two ways of suppressing a violation for the method 'foo':

public class TestClass {
    void foo() {
    }
}
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:15]
    |  |--PARAMETERS -> PARAMETERS [2:16]
    |  |--RPAREN -> ) [2:16]
    |  `--SLIST -> { [2:18]
    |      `--RCURLY -> } [3:4]
    `--RCURLY -> } [4:0]
  1. /CLASS_DEF//OBJBLOCK//METHOD_DEF[@text='foo']
  2. METHOD_DEF[@text='foo']

First implementation of suppression model can handle only case 1. We will add support for case 2 later.

timurt added a commit to timurt/checkstyle that referenced this issue Jun 25, 2017

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jun 25, 2017

Collaborator

@MEZk
I maybe wrong but I am not agree with the second one

  1. /CLASS_DEF//OBJBLOCK//METHOD_DEF[@text='foo']
  • we can query it from any node because it has full path from the root
  1. METHOD_DEF[@text='foo']
  • context-based query, so we should query from OBJBLOCK node, if we query from the root it should return empty array, no?
  1. //METHOD_DEF[@text='foo'] - this query will get all possible method definitions
Collaborator

timurt commented Jun 25, 2017

@MEZk
I maybe wrong but I am not agree with the second one

  1. /CLASS_DEF//OBJBLOCK//METHOD_DEF[@text='foo']
  • we can query it from any node because it has full path from the root
  1. METHOD_DEF[@text='foo']
  • context-based query, so we should query from OBJBLOCK node, if we query from the root it should return empty array, no?
  1. //METHOD_DEF[@text='foo'] - this query will get all possible method definitions
@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jun 25, 2017

Collaborator

@MEZk
Here is my corrected commit 4369

If I am not wrong only thing to implement left, is attribute iterator, to make queries like this
//CLASS_DEF[@*] or *[@text]

Collaborator

timurt commented Jun 25, 2017

@MEZk
Here is my corrected commit 4369

If I am not wrong only thing to implement left, is attribute iterator, to make queries like this
//CLASS_DEF[@*] or *[@text]

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 26, 2017

Contributor

@timurt
Good step forward. I will review the changes ASAP.

Contributor

MEZk commented Jun 26, 2017

@timurt
Good step forward. I will review the changes ASAP.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 27, 2017

Contributor

@timurt
Reviewed the changes.

Contributor

MEZk commented Jun 27, 2017

@timurt
Reviewed the changes.

timurt added a commit to timurt/checkstyle that referenced this issue Jul 5, 2017

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jul 5, 2017

Collaborator

@MEZk
New commit 4369
resolved bugs and moments you mentioned

Additionally

  1. added attribute iterator
  2. resolved problem with querying symbolic characters like TokenTypes.RCURLY, TokenTypes.LCURLY, TokenTypes.SLIST and etc.
  3. Added UTs to test Attribute query, Parent query, And query and etc
Collaborator

timurt commented Jul 5, 2017

@MEZk
New commit 4369
resolved bugs and moments you mentioned

Additionally

  1. added attribute iterator
  2. resolved problem with querying symbolic characters like TokenTypes.RCURLY, TokenTypes.LCURLY, TokenTypes.SLIST and etc.
  3. Added UTs to test Attribute query, Parent query, And query and etc
@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 5, 2017

Contributor

@timurt
Good, need some time to review.

Contributor

MEZk commented Jul 5, 2017

@timurt
Good, need some time to review.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 6, 2017

Contributor

@timurt
I reviewed changes. You did a good job.

  1. Please, fix/answer the points mentioned.

  2. Please, prepare the commit for the PR, fix checkstyle/PMD/... violations. Use mvn clean verify on your local.

  3. Check that your changes satisfy coverage threshold. Use mvn clean cobertura:check.

  4. When you are ready, please notify me here, I will do the final review and you can send PR.

  5. Please, also add examples for ENUM_DEF and INTERFACE_DEF. See #4415 (comment)

Contributor

MEZk commented Jul 6, 2017

@timurt
I reviewed changes. You did a good job.

  1. Please, fix/answer the points mentioned.

  2. Please, prepare the commit for the PR, fix checkstyle/PMD/... violations. Use mvn clean verify on your local.

  3. Check that your changes satisfy coverage threshold. Use mvn clean cobertura:check.

  4. When you are ready, please notify me here, I will do the final review and you can send PR.

  5. Please, also add examples for ENUM_DEF and INTERFACE_DEF. See #4415 (comment)

timurt added a commit to timurt/checkstyle that referenced this issue Jul 14, 2017

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jul 15, 2017

Collaborator

@MEZk
Here is my commit

It passed mvn verification and cobertura checks.

I have question regarding DetailAst, how can I get string representation of token type, for example
TokenTypes.BLOCK_COMMENT_BEGIN -> "BLOCK_COMMENT_BEGIN",
DetailAst.getText() sometimes returns actual value, for example
STRING_LITERAL -> "HelloWorld" or ARRAY_DECLARATOR -> [
We can write switch statements for every token type, but is there more simple another way?

Collaborator

timurt commented Jul 15, 2017

@MEZk
Here is my commit

It passed mvn verification and cobertura checks.

I have question regarding DetailAst, how can I get string representation of token type, for example
TokenTypes.BLOCK_COMMENT_BEGIN -> "BLOCK_COMMENT_BEGIN",
DetailAst.getText() sometimes returns actual value, for example
STRING_LITERAL -> "HelloWorld" or ARRAY_DECLARATOR -> [
We can write switch statements for every token type, but is there more simple another way?

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 15, 2017

Contributor

@timurt
Unfortunately, TokenType is not an enum, it is just a class with lots of constants. We cannot use valueOf, etc methods. We have two options here:

  1. Implement a logic which will convert string representation (getText) to the TokenType. Similar to switch-case.
  2. Use reflection to get the name of the field.

@romani
What we can do in this situation? For some token types getText method returns the string representation of the token (RCURLY -> }, SLIST -> }, etc). We use getText method to refer to the node attribute text, for example CLASS_DEF[@text='MyClass']. It's a pity taht TokenTypes is not an enum.

Contributor

MEZk commented Jul 15, 2017

@timurt
Unfortunately, TokenType is not an enum, it is just a class with lots of constants. We cannot use valueOf, etc methods. We have two options here:

  1. Implement a logic which will convert string representation (getText) to the TokenType. Similar to switch-case.
  2. Use reflection to get the name of the field.

@romani
What we can do in this situation? For some token types getText method returns the string representation of the token (RCURLY -> }, SLIST -> }, etc). We use getText method to refer to the node attribute text, for example CLASS_DEF[@text='MyClass']. It's a pity taht TokenTypes is not an enum.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 15, 2017

Member

Use reflection to get the name of the field.

Lets try this approach if we could do this ones only, to create a Map<int,String> to map integer value to string.

Member

romani commented Jul 15, 2017

Use reflection to get the name of the field.

Lets try this approach if we could do this ones only, to create a Map<int,String> to map integer value to string.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 15, 2017

Contributor

@timurt
Please, try to use reflection to get the name of the field by token type.

Contributor

MEZk commented Jul 15, 2017

@timurt
Please, try to use reflection to get the name of the field by token type.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 17, 2017

Contributor

@timurt
Ping

Contributor

MEZk commented Jul 17, 2017

@timurt
Ping

timurt added a commit to timurt/checkstyle that referenced this issue Jul 18, 2017

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jul 18, 2017

Collaborator

@MEZk
Here is my commit with reflection

I am trying to suppress PMD violations for AbstractNode, but when I use @SuppressWarnings annotation I get this message:

AbstractNode.java:52:19: The warning 'PMD' cannot be suppressed at this location. If you try to suppress IntelliJ IDEA inspection, please use javadoc block tag @noinspection [SuppressWarnings]

What can be the problem?

Collaborator

timurt commented Jul 18, 2017

@MEZk
Here is my commit with reflection

I am trying to suppress PMD violations for AbstractNode, but when I use @SuppressWarnings annotation I get this message:

AbstractNode.java:52:19: The warning 'PMD' cannot be suppressed at this location. If you try to suppress IntelliJ IDEA inspection, please use javadoc block tag @noinspection [SuppressWarnings]

What can be the problem?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 18, 2017

Member

What can be the problem?

@SuppressWarnings is not allowed in our code as it is pollute code and all nuances/problems of incpection/checks/rules/ ...... should be configured outside of sources. IDEA inspections do not have such feature but they do have suppression in javadoc, that is preferable.
PMD suppressions are stored in PMD config files by xpath expressions.

Member

romani commented Jul 18, 2017

What can be the problem?

@SuppressWarnings is not allowed in our code as it is pollute code and all nuances/problems of incpection/checks/rules/ ...... should be configured outside of sources. IDEA inspections do not have such feature but they do have suppression in javadoc, that is preferable.
PMD suppressions are stored in PMD config files by xpath expressions.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 18, 2017

Contributor

@timurt

Here is my commit with reflection

I did the final review.

  1. Please, fix new points.
  2. Please, prepare your changes for PR. They should pass at least mvn clean verify on your local.
  3. Send PR.
Contributor

MEZk commented Jul 18, 2017

@timurt

Here is my commit with reflection

I did the final review.

  1. Please, fix new points.
  2. Please, prepare your changes for PR. They should pass at least mvn clean verify on your local.
  3. Send PR.

timurt added a commit to timurt/checkstyle that referenced this issue Jul 24, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 25, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 25, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 25, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 25, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 26, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 26, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 27, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 4, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 4, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 4, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 5, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 5, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 5, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 5, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 5, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 6, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 6, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 7, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 7, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 8, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 8, 2017

rnveach added a commit that referenced this issue Aug 9, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 9, 2017

Member

Fix was merged

Member

rnveach commented Aug 9, 2017

Fix was merged

@rnveach rnveach closed this Aug 9, 2017

@rnveach rnveach added this to the 8.2 milestone Aug 9, 2017

@MEZk MEZk moved this from In Progress to Done in Flexible Suppression Model Aug 9, 2017

@romani romani changed the title from Investigate how to map XPath expressions onto AST nodes to Mapper of XPath expressions onto AST nodes Aug 10, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 10, 2017

Member

I marked issue as "misc" as feature for full functional Xpath Filter is not ready to be viewed by users.

Member

romani commented Aug 10, 2017

I marked issue as "misc" as feature for full functional Xpath Filter is not ready to be viewed by users.

soon added a commit to soon/checkstyle that referenced this issue Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment