Repository: checkstyle/checkstyle

Problem statement:
Pitest: Kill all surviving mutations
This will be main tracker issue for killing surviving mutations before we begin work in https://github.com/checkstyle/checkstyle/issues/11719

Now that we have diff report generation as github action, let's modify the procedure from  https://github.com/checkstyle/checkstyle/issues/7797#issue-577522554 a bit to speed this up:

1. Disable all CI first as separate commit(See example at https://github.com/checkstyle/checkstyle/pull/11721). This will help to not waste credits.
2. Pick one single surviving mutation from pitest.sh.
3. Send PR with a single hardcoded mutation only.
4. Generate diff report with hardcoded mutation.
5. Share link to initial commit with hardcoded mutation and initial diff report in PR description. This is in lieu of separate branch/ etc. in previous issue at #7797. 

HARDCODED MUTATION WILL NOT BE IN FINAL PR.

We will have one of three outcomes for each mutation (in order of preference):
1. Find a case that kills the mutation via diff report, add new test in existing PR
2. Safely (no regressions) modify code to be covered, in existing PR
3. Safely (no regressions) remove code to be covered, in existing PR

Case (2) and (3) required a detailed analysis and discussion in issue before making any changes.

-----------------------------------

**We will not be pursuing surviving mutations in IndentationCheck or gui package .**



Hints:
Once we approve this issue, we should close https://github.com/checkstyle/checkstyle/issues/7797 and any child/ associated issues.
approved.

it would be good to see html report(s) where ALL is activated to see visually scope of work. REpost should be on github.io to let mentors review it and discuss at any time and from phones. 

we need to have commit that disables all CIs. to make sure we will not eat all credits for CI provided, and we are not banned from CI usage due to misbehavior (overusage). 
so PRs for search of killer will have two commits: 
1) disable all CIs jobs 
2) hardcoded survival mutation

PR should have no CI jobs started or all of them do nothing (skip all jobs).

the only activity happens by comment to trigger Github Action.
@rnveach @nick-mancuso 
The new pitest method only examines `mutations.xml` which isn't appropriate to determine lines that are uncovered and are not mutated (due to the absence of some mutator). Such lines can only be taken from the HTML file.

An example of such lines is present over [here](https://vyom-yadav.github.io/DiffReport/pit-blocks-mod-1/index.html) (modified a bit for demonstrating).

Though uncovered lines will result in low line coverage, it is rounded up in certain cases ([example](https://vyom-yadav.github.io/DiffReport/pit-blocks-mod-single-line/index.html)) and the line coverage becomes 100% :( https://github.com/hcoles/pitest/issues/375 

The final resort is to create some custom method, we can either check line coverage `212/213` in this sense or we can, do what we used to do, add suppression, but in that case, our suppression model needs to be different because current one operates on mutations. 

We can create a new class named `UncoveredLines` and add that to the suppression model, ideas are welcome :)
Missing code coverage would also impact jacoco, which is also checking code coverage. Is Jacoco reporting similar missing code coverage is pitest only alone?
This seems like an improvement to me, as long as coverage is properly reported by Jacoco. Do we currently have suppressions in pitest.sh like this?
> This seems like an improvement to me, as long as coverage is properly reported by Jacoco.

@nick-mancuso Yes, coverage is supported by jacoco, where pitest rounds off, jacoco doesn't so the result is more accurate. See [this](https://vyom-yadav.github.io/DiffReport/jacoco-coverage/index.html) report, see `blocks` package, it shows 99% whereas [pitest](https://vyom-yadav.github.io/DiffReport/pit-blocks-mod-single-line/index.html) turns it to 100%.

> Do we currently have suppressions in pitest.sh like this?

We do but that suppression is a bug, see https://github.com/hcoles/pitest/issues/688
https://github.com/checkstyle/checkstyle/blob/e8b63a1b1ce4d9f40d0ce9dc8bc70ea8549a77bf/.ci/pitest.sh#L61-L67
 
@Vyom-Yadav let's do this: since we have separated execution of pitest and generation of pitest report out of your groovy script, let's also remove pitest execution and report generation from pitest.sh, and have both scripts run for duration of GSOC as an ongoing "regression" test for your new script.
@nick-mancuso , do we need this mega issue ?
I think it is better to create more precise issue with more clear scope and target.
We will allow #11972 to close this issue, and open more granular issues as we need to.

Patch:
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/VariableDeclarationUsageDistanceCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/VariableDeclarationUsageDistanceCheck.java
--- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/VariableDeclarationUsageDistanceCheck.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/VariableDeclarationUsageDistanceCheck.java
@@ -761,37 +761,26 @@ private static DetailAST getFirstNodeInsideIfBlock(
         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);
-                }
+            final Optional<DetailAST> slistToken = TokenUtil
+                .findFirstTokenByPredicate(block, token -> token.getType() == TokenTypes.SLIST);
+            final DetailAST lastNode = block.getLastChild();
+            DetailAST previousNode = lastNode.getPreviousSibling();
 
-                // Looking into ELSE block, get its first child and analyze it.
-                currentNode = currentNode.getFirstChild();
+            if (slistToken.isEmpty()
+                && lastNode.getType() == TokenTypes.LITERAL_ELSE) {
 
-                if (currentNode.getType() == TokenTypes.LITERAL_IF) {
-                    currentNode = currentNode.getLastChild();
-                }
-                else if (isChild(currentNode, variable)) {
-                    variableUsageExpressions.add(currentNode);
-                    currentNode = null;
-                }
+                // Is if statement without '{}' and has a following else branch,
+                // then change previousNode to the if statement body.
+                previousNode = previousNode.getPreviousSibling();
+            }
+
+            final List<DetailAST> variableUsageExpressions = new ArrayList<>();
+            if (isChild(previousNode, variable)) {
+                variableUsageExpressions.add(previousNode);
             }
 
-            // 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 (isChild(lastNode, variable)) {
+                variableUsageExpressions.add(lastNode);
             }
 
             // If variable usage exists in several related blocks, then


Test patch:
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/VariableDeclarationUsageDistanceCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/VariableDeclarationUsageDistanceCheckTest.java
--- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/VariableDeclarationUsageDistanceCheckTest.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/VariableDeclarationUsageDistanceCheckTest.java
@@ -91,6 +91,23 @@ public void testGeneralLogic2() throws Exception {
                 getPath("InputVariableDeclarationUsageDistanceGeneral2.java"), expected);
     }
 
+    @Test
+    public void testIfStatements() throws Exception {
+        final String[] expected = {
+            "18:9: " + getCheckMessage(MSG_KEY, "a", 4, 1),
+            "28:9: " + getCheckMessage(MSG_KEY, "a", 2, 1),
+            "32:9: " + getCheckMessage(MSG_KEY, "b", 2, 1),
+            "38:9: " + getCheckMessage(MSG_KEY, "c", 3, 1),
+            "49:9: " + getCheckMessage(MSG_KEY, "b", 2, 1),
+            "50:9: " + getCheckMessage(MSG_KEY, "c", 3, 1),
+            "51:9: " + getCheckMessage(MSG_KEY, "d", 4, 1),
+            "63:9: " + getCheckMessage(MSG_KEY, "a", 4, 1),
+
+        };
+        verifyWithInlineConfigParser(
+            getPath("InputVariableDeclarationUsageDistanceIfStatements.java"), expected);
+    }
+
     @Test
     public void testDistance() throws Exception {
         final String[] expected = {
diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/variabledeclarationusagedistance/InputVariableDeclarationUsageDistanceIfStatements.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/variabledeclarationusagedistance/InputVariableDeclarationUsageDistanceIfStatements.java
new file mode 100644
--- /dev/null
+++ b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/variabledeclarationusagedistance/InputVariableDeclarationUsageDistanceIfStatements.java
@@ -0,0 +1,72 @@
+/*
+VariableDeclarationUsageDistance
+allowedDistance = 1
+ignoreVariablePattern = (default)
+validateBetweenScopes = true
+ignoreFinal = false
+
+
+*/
+
+package com.puppycrawl.tools.checkstyle.checks.coding.variabledeclarationusagedistance;
+
+import java.util.HashSet;
+import java.util.Set;
+
+public class InputVariableDeclarationUsageDistanceIfStatements {
+    void method2() {
+        int a = 12; // violation
+        if (true) {
+            method2();
+            checkIfStatementWithoutParen();
+            method2();
+            a++;
+        }
+    }
+
+    void checkIfStatementWithoutParen() {
+        int a = 12; // violation
+        method2();
+        if (true)
+            a++;
+        int b = 12; // violation
+        method2();
+        if (false)
+            method2();
+        else if(true)
+            b++;
+        int c = 12; // violation
+        method2();
+        checkIfStatementWithoutParen();
+        if (true)
+            c++;
+        else
+            method2();
+    }
+
+    void testConsecutiveIfStatements() {
+        int a = 12; // ok
+        int b = 13; // violation
+        int c = 14; // violation
+        int d = 15; // violation
+        if (true)
+            a++;
+        if (true)
+            b++;
+        if (false)
+            c++;
+        if (true)
+            d++;
+    }
+
+    int testReturnStatement() {
+        int a = 1; // violation
+        testConsecutiveIfStatements();
+        testConsecutiveIfStatements();
+        testConsecutiveIfStatements();
+        if (true)
+            return a;
+
+        return 0;
+    }
+}