Skip to content

Commit

Permalink
Issue #11973: Fix NPE on empty switch statements in VariableDeclarati…
Browse files Browse the repository at this point in the history
…onUsageDistance
  • Loading branch information
stoyanK7 committed Sep 15, 2022
1 parent d02f5e7 commit ded47a1
Show file tree
Hide file tree
Showing 9 changed files with 203 additions and 27 deletions.
1 change: 1 addition & 0 deletions .ci/jsoref-spellchecker/whitelist.words
Original file line number Diff line number Diff line change
Expand Up @@ -1349,6 +1349,7 @@ todocomment
tokenizer
tokennizer
tokentype
tokenutil
tolist
Toolbar
TOSTRING
Expand Down
9 changes: 0 additions & 9 deletions .ci/pitest-suppressions/pitest-coding-1-suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,4 @@
<description>replaced call to com/puppycrawl/tools/checkstyle/api/DetailAST::getNextSibling with receiver</description>
<lineContent>DetailAST exprBetweenBrackets = openingBracket.getNextSibling();</lineContent>
</mutation>

<mutation unstable="false">
<sourceFile>VariableDeclarationUsageDistanceCheck.java</sourceFile>
<mutatedClass>com.puppycrawl.tools.checkstyle.checks.coding.VariableDeclarationUsageDistanceCheck</mutatedClass>
<mutatedMethod>lambda$getFirstCaseGroupOrSwitchRule$2</mutatedMethod>
<mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.NakedReceiverMutator</mutator>
<description>replaced call to com/puppycrawl/tools/checkstyle/api/DetailAST::findFirstToken with receiver</description>
<lineContent>.orElseGet(() -&gt; block.findFirstToken(TokenTypes.SWITCH_RULE));</lineContent>
</mutation>
</suppressedMutations>
3 changes: 3 additions & 0 deletions config/checkstyle_input_suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
<suppress id="StyleValidationCommentInInputs"
files="utils[\\/]checkutil[\\/]InputCheckUtilTest.java"/>

<suppress id="StyleValidationCommentInInputs"
files="utils[\\/]tokenutil[\\/]InputTokenUtilTestForEachChildMultipleTypes.java"/>

<suppress id="StyleValidationCommentInInputs"
files="suppresswarningsholder[\\/].*.java"/>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -809,18 +809,8 @@ private static DetailAST getFirstNodeInsideIfBlock(
*/
private static DetailAST getFirstNodeInsideSwitchBlock(
DetailAST block, DetailAST variable) {
final DetailAST currentNode = getFirstCaseGroupOrSwitchRule(block);
final List<DetailAST> variableUsageExpressions =
new ArrayList<>();

// Checking variable usage inside all CASE_GROUP and SWITCH_RULE ast's.
TokenUtil.forEachChild(block, currentNode.getType(), node -> {
final DetailAST lastNodeInCaseGroup =
node.getLastChild();
if (isChild(lastNodeInCaseGroup, variable)) {
variableUsageExpressions.add(lastNodeInCaseGroup);
}
});
getVariableUsageExpressionsInsideSwitchBlock(block, variable);

// If variable usage exists in several related blocks, then
// firstNodeInsideBlock = null, otherwise if variable usage exists
Expand All @@ -835,15 +825,26 @@ private static DetailAST getFirstNodeInsideSwitchBlock(
}

/**
* Helper method for getFirstNodeInsideSwitchBlock to return the first CASE_GROUP or
* SWITCH_RULE ast.
* Helper method for getFirstNodeInsideSwitchBlock to return all variable
* usage expressions inside a given switch block.
*
* @param block the switch block to check.
* @return DetailAST of the first CASE_GROUP or SWITCH_RULE.
* @param variable variable which is checked for in switch block.
* @return List of usages or empty list if none are found.
*/
private static DetailAST getFirstCaseGroupOrSwitchRule(DetailAST block) {
return Optional.ofNullable(block.findFirstToken(TokenTypes.CASE_GROUP))
.orElseGet(() -> block.findFirstToken(TokenTypes.SWITCH_RULE));
private static List<DetailAST> getVariableUsageExpressionsInsideSwitchBlock(DetailAST block,
DetailAST variable) {
final List<DetailAST> variableUsageExpressions = new ArrayList<>();
final List<Integer> tokens = List.of(TokenTypes.CASE_GROUP, TokenTypes.SWITCH_RULE);

TokenUtil.forEachChild(block, tokens, child -> {
final DetailAST lastNodeInCaseGroup = child.getLastChild();
if (isChild(lastNodeInCaseGroup, variable)) {
variableUsageExpressions.add(lastNodeInCaseGroup);
}
});

return variableUsageExpressions;
}

/**
Expand Down
18 changes: 18 additions & 0 deletions src/main/java/com/puppycrawl/tools/checkstyle/utils/TokenUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.lang.reflect.Modifier;
import java.util.Arrays;
import java.util.BitSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -246,6 +247,23 @@ public static void forEachChild(DetailAST root, int type, Consumer<DetailAST> ac
}
}

/**
* Performs an action for each child of {@link DetailAST} root node
* which matches any of the given token types.
*
* @param root root node.
* @param types token types to match.
* @param action action to perform on the nodes.
*/
public static void forEachChild(DetailAST root,
List<Integer> types, Consumer<DetailAST> action) {
for (DetailAST ast = root.getFirstChild(); ast != null; ast = ast.getNextSibling()) {
if (types.contains(ast.getType())) {
action.accept(ast);
}
}
}

/**
* Determines if two ASTs are on the same line.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,4 +309,15 @@ public void testVariableDeclarationUsageDistanceSwitchExpressions() throws Excep
verifyWithInlineConfigParser(getNonCompilablePath(filename), expected);
}

@Test
public void testVariableDeclarationUsageDistanceSwitchExpressions2() throws Exception {
final int maxDistance = 1;
final String[] expected = {
"16:9: " + getCheckMessage(MSG_KEY, "i", 2, maxDistance),
};

final String filename = "InputVariableDeclarationUsageDistanceCheckSwitchExpressions2.java";
verifyWithInlineConfigParser(getNonCompilablePath(filename), expected);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
package com.puppycrawl.tools.checkstyle.utils;

import static com.google.common.truth.Truth.assertWithMessage;
import static com.puppycrawl.tools.checkstyle.internal.utils.TestUtil.findTokenInAstByPredicate;
import static com.puppycrawl.tools.checkstyle.internal.utils.TestUtil.isUtilsClassHasPrivateConstructor;

import java.io.File;
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.Arrays;
Expand All @@ -32,11 +34,18 @@

import org.junit.jupiter.api.Test;

import com.puppycrawl.tools.checkstyle.AbstractPathTestSupport;
import com.puppycrawl.tools.checkstyle.DetailAstImpl;
import com.puppycrawl.tools.checkstyle.JavaParser;
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;

public class TokenUtilTest {
public class TokenUtilTest extends AbstractPathTestSupport {

@Override
protected String getPackageLocation() {
return "com/puppycrawl/tools/checkstyle/utils/tokenutil";
}

@Test
public void testIsProperUtilsClass() throws ReflectiveOperationException {
Expand Down Expand Up @@ -321,6 +330,31 @@ public void testForEachChild() {
.isEqualTo(secondSibling);
}

@Test
public void testForEachChildMultipleTypes() throws Exception {
final DetailAST astForTest = getNodeFromFile(TokenTypes.OBJBLOCK,
"InputTokenUtilTestForEachChildMultipleTypes.java");
final List<String> children = new ArrayList<>(3);
final List<Integer> tokensToMatch = List.of(TokenTypes.CLASS_DEF, TokenTypes.VARIABLE_DEF);

TokenUtil.forEachChild(astForTest, tokensToMatch, child -> {
children.add(child.toString());
});

final List<String> expectedChildren = List.of(
"VARIABLE_DEF[5x4]",
"CLASS_DEF[8x4]",
"VARIABLE_DEF[9x4]"
);

assertWithMessage("Must match three children")
.that(children)
.hasSize(3);
assertWithMessage("Mismatched child node(s)")
.that(children)
.isEqualTo(expectedChildren);
}

@Test
public void testIsTypeDeclaration() {
assertWithMessage("Should return true when valid type passed")
Expand Down Expand Up @@ -395,4 +429,19 @@ public void testIsBooleanLiteralType() {
.isFalse();
}

private DetailAST getNodeFromFile(int type, String filename) throws Exception {
return getNode(JavaParser.parseFile(new File(getPath(filename)),
JavaParser.Options.WITH_COMMENTS), type);
}

private static DetailAST getNode(DetailAST root, int type) {
final Optional<DetailAST> node = findTokenInAstByPredicate(root,
ast -> ast.getType() == type);

assertWithMessage("Cannot find node of specified type: %s", type)
.that(node.isPresent())
.isTrue();

return node.get();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/*
VariableDeclarationUsageDistance
allowedDistance = 1
ignoreVariablePattern = (default)
validateBetweenScopes = true
ignoreFinal = false
*/

//non-compiled with javac: Compilable with Java14
package com.puppycrawl.tools.checkstyle.checks.coding.variabledeclarationusagedistance;

public class InputVariableDeclarationUsageDistanceCheckSwitchExpressions2 {
void issue11973() {
int i = -1; // violation 'Distance between.*'i'.*and.*first usage is 2, but allowed 1.'
int x = -1; // ok
switch (i) {
case 1 -> {
x++;
i++;
}
};

int a = -1; // ok
int a123 = switch (a) {
case 1 -> {
yield 2;
}
default -> {
yield a;
}
};

int b = -1; // ok
int b123 = switch (b) {
case 1 -> {
yield b;
}
default -> {
yield 1;
}
};

int day = 1; // ok
int c = -1;
int c123 = switch (c) {
case 1:
case 2:
default:
c++;
c--;
yield day++;
};

int day1 = 1; // ok
int cc = -1;
int d123 = switch (cc) {
case 1 -> {
cc++;
cc--;
yield day1;
}
default -> {
yield 0;
}
};

int day2 = 1; // ok
int ccc = -1;
int e123 = switch (ccc) {
case 1 -> {
ccc++;
ccc--;
yield ccc;
}
case 2 -> {
yield 2;
}
default -> {
ccc++;
ccc--;
ccc++;
day2++;
yield 3;
}
};

int u = 0;
switch (u) { };
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.puppycrawl.tools.checkstyle.utils.tokenutil;

public class InputTokenUtilTestForEachChildMultipleTypes {
void method1() { }
private int a;
enum Days { }
void method2() { }
class Inner1 { }
private int b;
}

0 comments on commit ded47a1

Please sign in to comment.