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

DetailAst class unclear logic in addChilld method #3491

Closed
romani opened this Issue Oct 2, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@romani
Member

romani commented Oct 2, 2016

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/DetailAST.java

    @Override
    public void addChild(AST ast) {
        clearBranchTokenTypes();
        clearChildCountCache(this);
        super.addChild(ast);
        if (ast != null) {
            ((DetailAST) ast).setParent(this);
            getFirstChild().setParent(this);
        }
}

it is not clear why we need to do `getFirstChild().setParent(this);
If not required, need to be removed.

@romani romani added the approved label Oct 2, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 3, 2016

Member

Removing the specified code causes DetailASTTest.testTreeStructure to fail
java.lang.AssertionError: Bad prev node=;[7x79] prev==[7x36] parent=VARIABLE_DEF[7x4] filename=M:\checkstyleWorkspaceEclipse\checkstyle\src\test\resources\com\puppycrawl\tools\checkstyle\ant\InputCheckstyleAntTaskError.java root=package[1x0] expected:<=[7x36]> but was:<null>
and causes OneStatementPerLineCheckTest.testWithMultilineStatements to fail
org.junit.ComparisonFailure: error message 0 expected:<[M:\checkstyleWorkspaceEclipse\checkstyle\src\test\resources\com\puppycrawl\tools\checkstyle\checks\coding\InputOneStatementPerLine2.java:135:39: Only one statement per line allowed].> but was:<[Audit done].>

Member

rnveach commented Oct 3, 2016

Removing the specified code causes DetailASTTest.testTreeStructure to fail
java.lang.AssertionError: Bad prev node=;[7x79] prev==[7x36] parent=VARIABLE_DEF[7x4] filename=M:\checkstyleWorkspaceEclipse\checkstyle\src\test\resources\com\puppycrawl\tools\checkstyle\ant\InputCheckstyleAntTaskError.java root=package[1x0] expected:<=[7x36]> but was:<null>
and causes OneStatementPerLineCheckTest.testWithMultilineStatements to fail
org.junit.ComparisonFailure: error message 0 expected:<[M:\checkstyleWorkspaceEclipse\checkstyle\src\test\resources\com\puppycrawl\tools\checkstyle\checks\coding\InputOneStatementPerLine2.java:135:39: Only one statement per line allowed].> but was:<[Audit done].>

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 4, 2016

Member

In looking at InputCheckstyleAntTaskError failure with DetailASTTest, with the code in question removed, the previous sibling of ; in the following input is null, which is totally incorrect:
private static final String FOO = "This line is longer then 70 characters.";
Here is the complete AST tree:

    |--VARIABLE_DEF -> VARIABLE_DEF [7:4]
    |  |--MODIFIERS -> MODIFIERS [7:4]
    |  |  |--LITERAL_PRIVATE -> private [7:4]
    |  |  |--LITERAL_STATIC -> static [7:12]
    |  |  `--FINAL -> final [7:19]
    |  |--TYPE -> TYPE [7:25]
    |  |  `--IDENT -> String [7:25]
    |  |--IDENT -> FOO [7:32]
    |  |--ASSIGN -> = [7:36]
    |  |  `--EXPR -> EXPR [7:38]
    |  |      `--STRING_LITERAL -> "This line is longer then 70 characters." [7:38]
    |  `--SEMI -> ; [7:79] <--- no previous sibling found when code in question is removed

The act of calling getFirstChild().setParent(this); tells all the siblings to set their parent and set their previous siblings.

Without this code, addChild is adding a node to the tree without setting the previous sibling of that node. It isn't set by default, so that is why it is null.

@romani If we want to change it to something that will make more sense when reading it, the following will do:

    public void addChild(AST ast) {
        clearBranchTokenTypes();
        clearChildCountCache(this);
        if (ast != null) {
            ((DetailAST) ast).setParent(this);
            ((DetailAST)ast).previousSibling = getLastChild(); // new code
        }
        super.addChild(ast);
    }

Otherwise, we can just add a comment to explain it and leave the code alone.

Member

rnveach commented Oct 4, 2016

In looking at InputCheckstyleAntTaskError failure with DetailASTTest, with the code in question removed, the previous sibling of ; in the following input is null, which is totally incorrect:
private static final String FOO = "This line is longer then 70 characters.";
Here is the complete AST tree:

    |--VARIABLE_DEF -> VARIABLE_DEF [7:4]
    |  |--MODIFIERS -> MODIFIERS [7:4]
    |  |  |--LITERAL_PRIVATE -> private [7:4]
    |  |  |--LITERAL_STATIC -> static [7:12]
    |  |  `--FINAL -> final [7:19]
    |  |--TYPE -> TYPE [7:25]
    |  |  `--IDENT -> String [7:25]
    |  |--IDENT -> FOO [7:32]
    |  |--ASSIGN -> = [7:36]
    |  |  `--EXPR -> EXPR [7:38]
    |  |      `--STRING_LITERAL -> "This line is longer then 70 characters." [7:38]
    |  `--SEMI -> ; [7:79] <--- no previous sibling found when code in question is removed

The act of calling getFirstChild().setParent(this); tells all the siblings to set their parent and set their previous siblings.

Without this code, addChild is adding a node to the tree without setting the previous sibling of that node. It isn't set by default, so that is why it is null.

@romani If we want to change it to something that will make more sense when reading it, the following will do:

    public void addChild(AST ast) {
        clearBranchTokenTypes();
        clearChildCountCache(this);
        if (ast != null) {
            ((DetailAST) ast).setParent(this);
            ((DetailAST)ast).previousSibling = getLastChild(); // new code
        }
        super.addChild(ast);
    }

Otherwise, we can just add a comment to explain it and leave the code alone.

@rnveach rnveach added the easy label Mar 2, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 17, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 17, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 17, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 17, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 18, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 18, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 18, 2017

Issue #3491: DetailAst class unclear logic in addChilld method
Issue #3491: DetailAst class unclear logic in addChilld method

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 19, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 19, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 19, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 19, 2017

rnveach added a commit that referenced this issue Mar 19, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 19, 2017

Member

Fix is merged

Member

rnveach commented Mar 19, 2017

Fix is merged

@rnveach rnveach closed this Mar 19, 2017

@rnveach rnveach added this to the 7.7 milestone Mar 19, 2017

SergeyDzyuba added a commit to SergeyDzyuba/checkstyle that referenced this issue Mar 22, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 26, 2017

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