Issue #11720: Kill surviving mutation in VariableDeclarationUsageDistanceCheck in getFirstNodeInsideIfBlock

[Vyom-Yadav]

Closes #11720

Check documentation: https://checkstyle.sourceforge.io/config_coding.html#VariableDeclarationUsageDistance

Diff Reports (Contains diff, Issue for NPE at #11973):

• validateBetweenScopesFalse: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/78d2c37_2022195745/reports/diff/index.html
• validateBetweenScopesTrue: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/3fe931a_2022125544/reports/diff/index.html

Rationale

There is no need to loop and check all the statements in the if-else chain at once, just check the current statement, the next statement (if present), and the outer method i.e. calculateDistanceBetweenScopes will iterate over the remaining branches, we just add the head of the remaining branches to variableUsageExpressions for it to be analyzed in the next iteration.

Also support for if statements without {} was added, it rationale is present below.
Previous Logic:

private static DetailAST getFirstNodeInsideIfBlock(
        DetailAST block, DetailAST variable) {
    DetailAST firstNodeInsideBlock = null;


    if (!isVariableInOperatorExpr(block, variable)) {
        DetailAST currentNode = block.getLastChild();
        final List<DetailAST> variableUsageExpressions =
                new ArrayList<>();


        while (currentNode != null
                && currentNode.getType() == TokenTypes.LITERAL_ELSE) {
            final DetailAST previousNode =
                    currentNode.getPreviousSibling();


            // Checking variable usage inside IF block.
            if (isChild(previousNode, variable)) {
                variableUsageExpressions.add(previousNode);
            }


            // Looking into ELSE block, get its first child and analyze it.
            currentNode = currentNode.getFirstChild();


            if (currentNode.getType() == TokenTypes.LITERAL_IF) {
                currentNode = currentNode.getLastChild();
            }
            else if (isChild(currentNode, variable)) {
                variableUsageExpressions.add(currentNode);
                currentNode = null;
            }
        }


        // If IF block doesn't include ELSE then analyze variable usage
        // only inside IF block.
        if (currentNode != null
                && isChild(currentNode, variable)) {
            variableUsageExpressions.add(currentNode);
        }


        // If variable usage exists in several related blocks, then
        // firstNodeInsideBlock = null, otherwise if variable usage exists
        // only inside one block, then get node from
        // variableUsageExpressions.
        if (variableUsageExpressions.size() == 1) {
            firstNodeInsideBlock = variableUsageExpressions.get(0);
        }
    }


    return firstNodeInsideBlock;
}

If we see the initial logic, we get the if statements last-child, for if statements without {}, it will be:

if (true)    
    method();

AST-

LITERAL_IF -> if 
   |--LPAREN -> ( 
   |--EXPR -> EXPR 
   |   `--LITERAL_TRUE -> true 
   |--RPAREN -> ) 
   |--EXPR -> EXPR 
   |   `--METHOD_CALL -> ( 
   |       |--IDENT -> method 
   |       |--ELIST -> ELIST 
   |       `--RPAREN -> ) 
   `--SEMI -> ; 

Now the last child is SEMI and the according to the logic, it was supposed to be if statements body.
The logic was built for if statements with {} where the last child is SLIST i.e. if statement's body.

LITERAL_IF -> if 
  |--LPAREN -> ( 
  |--EXPR -> EXPR 
  |   `--LITERAL_TRUE -> true 
  |--RPAREN -> ) 
  `--SLIST -> { 
      |--EXPR -> EXPR 
      |   `--METHOD_CALL -> ( 
      |       |--IDENT -> method 
      |       |--ELIST -> ELIST 
      |       `--RPAREN -> ) 
      |--SEMI -> ; 
      `--RCURLY -> } 

Cases like this in combination with else were missing, support for that was added.

---

Generating reports again:

Diff Regression config: https://gist.githubusercontent.com/Vyom-Yadav/98dceb63a79f4833e85fff9b2e1464a6/raw/a06fa3c8f525c4466a67c1ee057d4ed843db2301/my_checks.xml
Report label: validateBetweenScopesTrue

[Vyom-Yadav]

Github, generate report

[Vyom-Yadav]

Github, generate report

[github-actions]

validateBetweenScopesFalse: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/143b6e0_2022191919/reports/diff/index.html

[github-actions]

validateBetweenScopesFalse: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/78d2c37_2022195745/reports/diff/index.html

[Vyom-Yadav]

Github, generate report

[github-actions]

validateBetweenScopesTrue: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/78d2c37_2022043220/reports/diff/index.html

[nrmancuso]

@Vyom-Yadav are we able to cleanly separate changes here into two PRs?

[Vyom-Yadav]

@Vyom-Yadav are we able to cleanly separate changes here into two PRs?

We can, but it is not recommended, the if conditions are related to each other, just removing one branch at a time can be done but it doesn't make any sense to do so, having complete correct logic that is related to each other in one PR is better IMO, but still if needed, I can split this into two :)

[Vyom-Yadav]

Github, generate report

[github-actions]

validateBetweenScopesTrue: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/3fe931a_2022125544/reports/diff/index.html

[nrmancuso]

@Vyom-Yadav why such a large discrepancy between reports?

[Vyom-Yadav]

@Vyom-Yadav why such a large discrepancy between reports?

I added support for if statements without { }, that's why there is this difference, in many cases, the distance is increased by one as earlier if statements without { } were not supported.

[nrmancuso]

Item:

[nrmancuso]

Item:

[baratali]

can you please rename it to lastNode (since there is no loop anymore and it has only one value)?

[Vyom-Yadav]

Done.

[baratali]

When return is the first statement in if the last child is not SEMI:

if (true) return a;

LITERAL_IF -> if [3:8]
|--LPAREN -> ( [3:11]
|--EXPR -> EXPR [3:12]
|  `--LITERAL_TRUE -> true [3:12]
|--RPAREN -> ) [3:16]
`--LITERAL_RETURN -> return [3:18]
    |--EXPR -> EXPR [3:25]
    |  `--IDENT -> a [3:25]
    `--SEMI -> ; [3:26]

And it doesn't catch a violation in such code:

    String testReturn() {
        int a = 1; // violation
        System.out.println();
        System.out.println();
        System.out.println();
        if (true)
            return String.valueOf(a);

        return null;
    }

Can you please add it to the inputs too and test?

[Vyom-Yadav]

I tested it, it won't give a violation, the if statement evaluations works correctly, but this is considered by check logic as an initialization sequence
(isInitializationSequence = true), so it does not give a violation finally at:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/VariableDeclarationUsageDistanceCheck.java

Lines 417 to 418 in f4ab4aa

	if (dist > allowedDistance
	&& !isInitializationSequence(variableUsageAst, variable.getText())) {

[Vyom-Yadav]

I added a similar example as proof of working on current changes.