Skip to content

Commit

Permalink
Issue checkstyle#3335: prevented static variables being checked for R…
Browse files Browse the repository at this point in the history 
…equireThis
  • Loading branch information
@rnveach
rnveach committed Jul 17, 2016
1 parent 137c0b2 commit ae59bf3
Show file tree
Hide file tree
Showing 5 changed files with 169 additions and 101 deletions.
172 changes: 98 additions & 74 deletions src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java
View file
Expand Up @@ -305,7 +305,11 @@ private AbstractFrame getFieldWithoutThis(DetailAST ast, int parentType) {
&& !methodNameInMethodCall
&& !typeName
&& !isDeclarationToken(parentType)) {
frame = getClassFrameWhereViolationIsFound(ast);
final AbstractFrame fieldFrame = findClassFrame(ast, false);

if (fieldFrame != null && ((ClassFrame) fieldFrame).hasInstanceMember(ast)) {
frame = getClassFrameWhereViolationIsFound(ast);
}
}
return frame;
}
Expand Down Expand Up @@ -412,57 +416,54 @@ private void endCollectingDeclarations(Queue<AbstractFrame> frameStack, DetailAS
private AbstractFrame getClassFrameWhereViolationIsFound(DetailAST ast) {
AbstractFrame frameWhereViolationIsFound = null;
final AbstractFrame variableDeclarationFrame = findFrame(ast, false);
if (variableDeclarationFrame != null) {
final FrameType variableDeclarationFrameType = variableDeclarationFrame.getType();
final DetailAST prevSibling = ast.getPreviousSibling();
if (variableDeclarationFrameType == FrameType.CLASS_FRAME
&& !validateOnlyOverlapping
&& prevSibling == null
&& !ScopeUtils.isInInterfaceBlock(ast)
&& canBeReferencedFromStaticContext(ast)) {
frameWhereViolationIsFound = variableDeclarationFrame;
}
else if (variableDeclarationFrameType == FrameType.METHOD_FRAME) {
if (isOverlappingByArgument(ast)) {
if (!isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)
&& !isReturnedVariable(variableDeclarationFrame, ast)
&& canBeReferencedFromStaticContext(ast)
&& canAssignValueToClassField(ast)) {
frameWhereViolationIsFound = findFrame(ast, true);
}
}
else if (!validateOnlyOverlapping
&& prevSibling == null
&& isAssignToken(ast.getParent().getType())
&& !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)
&& canBeReferencedFromStaticContext(ast)
&& canAssignValueToClassField(ast)) {
final FrameType variableDeclarationFrameType = variableDeclarationFrame.getType();
final DetailAST prevSibling = ast.getPreviousSibling();
if (variableDeclarationFrameType == FrameType.CLASS_FRAME
&& !validateOnlyOverlapping
&& prevSibling == null
&& canBeReferencedFromStaticContext(ast)) {
frameWhereViolationIsFound = variableDeclarationFrame;
}
else if (variableDeclarationFrameType == FrameType.METHOD_FRAME) {
if (isOverlappingByArgument(ast)) {
if (!isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)
&& !isReturnedVariable(variableDeclarationFrame, ast)
&& canBeReferencedFromStaticContext(ast)
&& canAssignValueToClassField(ast)) {
frameWhereViolationIsFound = findFrame(ast, true);

}
}
else if (variableDeclarationFrameType == FrameType.CTOR_FRAME
&& isOverlappingByArgument(ast)
&& !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)) {
else if (!validateOnlyOverlapping
&& prevSibling == null
&& isAssignToken(ast.getParent().getType())
&& !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)
&& canBeReferencedFromStaticContext(ast)
&& canAssignValueToClassField(ast)) {
frameWhereViolationIsFound = findFrame(ast, true);

}
else if (variableDeclarationFrameType == FrameType.BLOCK_FRAME) {
if (isOverlappingByLocalVariable(ast)) {
if (canAssignValueToClassField(ast)
&& !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)
&& !isReturnedVariable(variableDeclarationFrame, ast)
&& canBeReferencedFromStaticContext(ast)) {
frameWhereViolationIsFound = findFrame(ast, true);
}
}
else if (!validateOnlyOverlapping
&& prevSibling == null
&& isAssignToken(ast.getParent().getType())
&& !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)
&& canBeReferencedFromStaticContext(ast)) {
}
else if (variableDeclarationFrameType == FrameType.CTOR_FRAME
&& isOverlappingByArgument(ast)
&& !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)) {
frameWhereViolationIsFound = findFrame(ast, true);
}
else if (variableDeclarationFrameType == FrameType.BLOCK_FRAME) {
if (isOverlappingByLocalVariable(ast)) {
if (canAssignValueToClassField(ast)
&& !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)
&& !isReturnedVariable(variableDeclarationFrame, ast)
&& canBeReferencedFromStaticContext(ast)) {
frameWhereViolationIsFound = findFrame(ast, true);
}
}
else if (!validateOnlyOverlapping
&& prevSibling == null
&& isAssignToken(ast.getParent().getType())
&& !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)
&& canBeReferencedFromStaticContext(ast)) {
frameWhereViolationIsFound = findFrame(ast, true);
}
}
return frameWhereViolationIsFound;
}
Expand All @@ -483,17 +484,15 @@ private static boolean isUserDefinedArrangementOfThis(AbstractFrame currentFrame

boolean userDefinedArrangementOfThis = false;

if (blockEndToken != null) {
final Set<DetailAST> variableUsagesInsideBlock =
getAllTokensWhichAreEqualToCurrent(definitionToken, ident,
blockEndToken.getLineNo());
final Set<DetailAST> variableUsagesInsideBlock =
getAllTokensWhichAreEqualToCurrent(definitionToken, ident,
blockEndToken.getLineNo());

for (DetailAST variableUsage : variableUsagesInsideBlock) {
final DetailAST prevSibling = variableUsage.getPreviousSibling();
if (prevSibling != null
&& prevSibling.getType() == TokenTypes.LITERAL_THIS) {
userDefinedArrangementOfThis = true;
}
for (DetailAST variableUsage : variableUsagesInsideBlock) {
final DetailAST prevSibling = variableUsage.getPreviousSibling();
if (prevSibling != null
&& prevSibling.getType() == TokenTypes.LITERAL_THIS) {
userDefinedArrangementOfThis = true;
}
}
return userDefinedArrangementOfThis;
Expand Down Expand Up @@ -611,10 +610,7 @@ private boolean canAssignValueToClassField(DetailAST ast) {
final boolean fieldUsageInConstructor = isInsideConstructorFrame(fieldUsageFrame);

final AbstractFrame declarationFrame = findFrame(ast, true);
boolean finalField = false;
if (declarationFrame != null) {
finalField = ((ClassFrame) declarationFrame).hasFinalField(ast);
}
final boolean finalField = ((ClassFrame) declarationFrame).hasFinalField(ast);

return fieldUsageInConstructor || !finalField;
}
Expand Down Expand Up @@ -649,14 +645,12 @@ private boolean isOverlappingByArgument(DetailAST ast) {
final DetailAST sibling = ast.getNextSibling();
if (sibling != null && isAssignToken(parent.getType())) {
final ClassFrame classFrame = (ClassFrame) findFrame(ast, true);
if (classFrame != null) {
final Set<DetailAST> exprIdents = getAllTokensOfType(sibling, TokenTypes.IDENT);
if (isCompoundAssignToken(parent.getType())) {
overlapping = true;
}
else {
overlapping = classFrame.containsFieldOrVariableDef(exprIdents, ast);
}
final Set<DetailAST> exprIdents = getAllTokensOfType(sibling, TokenTypes.IDENT);
if (isCompoundAssignToken(parent.getType())) {
overlapping = true;
}
else {
overlapping = classFrame.containsFieldOrVariableDef(exprIdents, ast);
}
}
return overlapping;
Expand All @@ -673,12 +667,8 @@ private boolean isOverlappingByLocalVariable(DetailAST ast) {
final DetailAST sibling = ast.getNextSibling();
if (sibling != null && isAssignToken(parent.getType())) {
final ClassFrame classFrame = (ClassFrame) findFrame(ast, true);
if (classFrame != null) {
final Set<DetailAST> exprIdents = getAllTokensOfType(sibling, TokenTypes.IDENT);
if (classFrame.hasInstanceMember(ast)) {
overlapping = classFrame.containsFieldOrVariableDef(exprIdents, ast);
}
}
final Set<DetailAST> exprIdents = getAllTokensOfType(sibling, TokenTypes.IDENT);
overlapping = classFrame.containsFieldOrVariableDef(exprIdents, ast);
}
return overlapping;
}
Expand Down Expand Up @@ -793,19 +783,53 @@ private AbstractFrame getMethodWithoutThis(DetailAST ast) {
return result;
}

/**
* Find the class frame containing declaration.
* @param name IDENT ast of the declaration to find.
* @param lookForMethod whether we are looking for a method name.
* @return AbstractFrame containing declaration or null.
*/
private AbstractFrame findClassFrame(DetailAST name, boolean lookForMethod) {
AbstractFrame frame = current;

while (true) {
frame = findFrame(frame, name, lookForMethod);

if (frame == null || frame instanceof ClassFrame) {
break;
}

frame = frame.getParent();
}

return frame;
}

/**
* Find frame containing declaration.
* @param name IDENT ast of the declaration to find.
* @param lookForMethod whether we are looking for a method name.
* @return AbstractFrame containing declaration or null.
*/
private AbstractFrame findFrame(DetailAST name, boolean lookForMethod) {
return findFrame(current, name, lookForMethod);
}

/**
* Find frame containing declaration.
* @param frame The parent frame to searching in.
* @param name IDENT ast of the declaration to find.
* @param lookForMethod whether we are looking for a method name.
* @return AbstractFrame containing declaration or null.
*/
private static AbstractFrame findFrame(AbstractFrame frame, DetailAST name,
boolean lookForMethod) {
final AbstractFrame result;
if (current == null) {
if (frame == null) {
result = null;
}
else {
result = current.getIfContains(name, lookForMethod);
result = frame.getIfContains(name, lookForMethod);
}
return result;
}
Expand Down
23 changes: 8 additions & 15 deletions src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java
View file
Expand Up @@ -53,7 +53,6 @@ public void testIt() throws Exception {
"31:9: " + getCheckMessage(MSG_VARIABLE, "i", ""),
"49:13: " + getCheckMessage(MSG_VARIABLE, "z", ""),
"56:9: " + getCheckMessage(MSG_VARIABLE, "z", ""),
"86:16: " + getCheckMessage(MSG_VARIABLE, "CONST", ""),
"113:9: " + getCheckMessage(MSG_VARIABLE, "i", ""),
"114:9: " + getCheckMessage(MSG_VARIABLE, "i", ""),
"115:9: " + getCheckMessage(MSG_METHOD, "instanceMethod", ""),
Expand Down Expand Up @@ -94,7 +93,6 @@ public void testFieldsOnly() throws Exception {
"31:9: " + getCheckMessage(MSG_VARIABLE, "i", ""),
"49:13: " + getCheckMessage(MSG_VARIABLE, "z", ""),
"56:9: " + getCheckMessage(MSG_VARIABLE, "z", ""),
"86:16: " + getCheckMessage(MSG_VARIABLE, "CONST", ""),
"113:9: " + getCheckMessage(MSG_VARIABLE, "i", ""),
"114:9: " + getCheckMessage(MSG_VARIABLE, "i", ""),
"122:13: " + getCheckMessage(MSG_VARIABLE, "i", "Issue2240."),
Expand Down Expand Up @@ -175,7 +173,6 @@ public void testValidateOnlyOverlappingFalse() throws Exception {
"60:9: " + getCheckMessage(MSG_VARIABLE, "fieldFinal1", ""),
"61:9: " + getCheckMessage(MSG_VARIABLE, "fieldFinal2", ""),
"80:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
"84:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),
"119:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
"128:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
"132:9: " + getCheckMessage(MSG_METHOD, "method1", ""),
Expand All @@ -201,18 +198,10 @@ public void testValidateOnlyOverlappingFalse() throws Exception {
"275:18: " + getCheckMessage(MSG_METHOD, "addSuffixToField", ""),
"301:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
"340:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
"360:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),
"374:40: " + getCheckMessage(MSG_METHOD, "getServletRelativeAction", ""),
"376:20: " + getCheckMessage(MSG_METHOD, "processAction", ""),
"383:9: " + getCheckMessage(MSG_VARIABLE, "servletRelativeAction", ""),
"384:16: " + getCheckMessage(MSG_METHOD, "processAction", ""),
"443:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),
"447:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),
"451:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),
"455:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),
"459:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),
"463:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),

};
verify(checkConfig, getPath("InputValidateOnlyOverlappingFalse.java"), expected);
}
Expand All @@ -233,10 +222,6 @@ public void testValidateOnlyOverlappingTrue() throws Exception {
"275:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
"301:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
"339:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
"359:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),
"442:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),
"450:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),
"454:9: " + getCheckMessage(MSG_VARIABLE, "fieldStatic", ""),
};
verify(checkConfig, getPath("InputValidateOnlyOverlappingTrue.java"), expected);
}
Expand All @@ -255,4 +240,12 @@ public void testBraceAlone() throws Exception {
final String[] expected = CommonUtils.EMPTY_STRING_ARRAY;
verify(checkConfig, getPath("InputRequireThisBraceAlone.java"), expected);
}

@Test
public void testStatic() throws Exception {
final DefaultConfiguration checkConfig = createCheckConfig(RequireThisCheck.class);
checkConfig.addAttribute("validateOnlyOverlapping", "false");
final String[] expected = CommonUtils.EMPTY_STRING_ARRAY;
verify(checkConfig, getPath("InputRequireThisStatic.java"), expected);
}
}
51 changes: 51 additions & 0 deletions src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/InputRequireThisStatic.java
View file
@@ -0,0 +1,51 @@
package com.puppycrawl.tools.checkstyle.checks.coding;

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;

public final class InputRequireThisStatic {
public static String staticField1 = "";

public static String staticField2 = new String(staticField1);

public String instanceField1;
public BufferedReader instanceField2;

static {
try (BufferedReader instanceField2 = new BufferedReader(new FileReader(""))) {
instanceField2.readLine();
}
catch (FileNotFoundException e) {
}
catch (IOException e) {
}
}

public String get() {
return staticField1;
}

void foo50(String staticField1) {
staticField1 = staticField1;
}

void foo52(String staticField1) {
staticField1 += staticField1;
}

static void test(String instanceField1) {
if (instanceField1 == null) {
instanceField1 = staticField1;
}
}

static void test2() {
try (BufferedReader instanceField2 = new BufferedReader(new FileReader(""))) {
instanceField2.readLine();
}
catch (IOException e) {
}
}
}

0 comments on commit ae59bf3

Please sign in to comment.