Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Issue #14747: fix ParenPad to not flag unsupported Tokens #14792

Closed
wants to merge 1 commit into from
Closed

Issue #14747: fix ParenPad to not flag unsupported Tokens #14792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 0 additions & 11 deletions config/checker-framework-suppressions/checker-index-suppressions.xml
View file
Open in desktop
Expand Up @@ -2344,17 +2344,6 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/ParenPadCheck.java</fileName>
<specifier>argument</specifier>
<message>incompatible argument for parameter bitIndex of BitSet.get.</message>
<lineContent>return acceptableTokens.get(ast.getType());</lineContent>
<details>
found : int
required: @NonNegative int
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/SeparatorWrapCheck.java</fileName>
<specifier>argument</specifier>
Expand Down
14 changes: 0 additions & 14 deletions config/checker-framework-suppressions/checker-nullness-optional-interning-suppressions.xml
View file
Open in desktop
Expand Up @@ -4549,20 +4549,6 @@
<lineContent>result = latestDefinition == methodDef.getParent().getParent();</lineContent>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/ParenPadCheck.java</fileName>
<specifier>not.interned</specifier>
<message>attempting to use a non-@Interned comparison operand</message>
<lineContent>while (currentNode.getNextSibling() == null &amp;&amp; currentNode.getParent() != ast) {</lineContent>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/ParenPadCheck.java</fileName>
<specifier>not.interned</specifier>
<message>attempting to use a non-@Interned comparison operand</message>
<lineContent>while (currentNode.getNextSibling() == null &amp;&amp; currentNode.getParent() != ast) {</lineContent>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/TypecastParenPadCheck.java</fileName>
<specifier>not.interned</specifier>
Expand Down
9 changes: 9 additions & 0 deletions config/linkcheck-suppressions.txt
View file
Open in desktop
Expand Up @@ -1345,3 +1345,12 @@
<a href="apidocs/com/puppycrawl/tools/checkstyle/grammar/CompositeLexerContextCache.html#%3Cinit%3E(org.antlr.v4.runtime.Lexer)">#%3Cinit%3E(org.antlr.v4.runtime.Lexer)</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/grammar/CompositeLexerContextCache.StringTemplateContext.html#%3Cinit%3E(int,int)">#%3Cinit%3E(int,int)</a>: doesn't exist.
<a href="com/puppycrawl/tools/checkstyle/grammar/CompositeLexerContextCache.StringTemplateContext.html#%3Cinit%3E(int,int)">com/puppycrawl/tools/checkstyle/grammar/CompositeLexerContextCache.StringTemplateContext.html#%3Cinit%3E(int,int)</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#DIV">../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#DIV</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LAND">../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LAND</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LNOT">../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LNOT</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LOR">../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LOR</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MINUS">../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MINUS</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MOD">../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MOD</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#PLUS">../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#PLUS</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#STAR">../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#STAR</a>: doesn't exist.
<a href="apidocs/com/puppycrawl/tools/checkstyle/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#TYPECAST">../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#TYPECAST</a>: doesn't exist.
3 changes: 2 additions & 1 deletion config/pmd.xml
View file
Open in desktop
Expand Up @@ -288,7 +288,8 @@
| //ClassOrInterfaceDeclaration[@SimpleName='SarifLogger']//MethodDeclaration[@Name='escape']
| //ClassOrInterfaceDeclaration[@SimpleName='LeftCurlyCheck'
or @SimpleName='FinalLocalVariableCheck'
or @SimpleName='NPathComplexityCheck']
or @SimpleName='NPathComplexityCheck'
or @SimpleName='ParenPadCheck']
//MethodDeclaration[@Name='visitToken']
| //ClassOrInterfaceDeclaration[@SimpleName='FallThroughCheck']
//MethodDeclaration[@Name='isTerminated']
Expand Down
79 changes: 41 additions & 38 deletions src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/ParenPadCheck.java
View file
Open in desktop
Expand Up @@ -19,8 +19,7 @@

package com.puppycrawl.tools.checkstyle.checks.whitespace;

import java.util.BitSet;

import com.puppycrawl.tools.checkstyle.StatelessCheck;
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
import com.puppycrawl.tools.checkstyle.utils.CommonUtil;
Expand Down Expand Up @@ -97,7 +96,26 @@
* <a href="https://checkstyle.org/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LAMBDA">
* LAMBDA</a>,
* <a href="https://checkstyle.org/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#RECORD_DEF">
* RECORD_DEF</a>.
* RECORD_DEF</a>,
* <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#TYPECAST">
* TYPECAST</a>,
* <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#STAR">
* STAR</a>,
* <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#PLUS">
* PLUS</a>,
* <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MINUS">
* MINUS</a>,
* <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#DIV">
* DIV</a>,
* <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MOD">
* MOD</a>,
* <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LAND">
* LAND</a>,
* <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LOR">
* LOR</a>,
* <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LNOT">
* LNOT</a>
* .
* </li>
* </ul>
* <p>
Expand All @@ -123,20 +141,9 @@
*
* @since 3.0
*/
@StatelessCheck
public class ParenPadCheck extends AbstractParenPadCheck {

/**
* Tokens that this check handles.
*/
private final BitSet acceptableTokens;

/**
* Initializes acceptableTokens.
*/
public ParenPadCheck() {
acceptableTokens = TokenUtil.asBitSet(makeAcceptableTokens());
}

@Override
public int[] getDefaultTokens() {
return makeAcceptableTokens();
Expand All @@ -162,6 +169,15 @@ public void visitToken(DetailAST ast) {
case TokenTypes.DOT:
case TokenTypes.EXPR:
case TokenTypes.QUESTION:
case TokenTypes.TYPECAST:
case TokenTypes.STAR:
case TokenTypes.PLUS:
case TokenTypes.MINUS:
case TokenTypes.DIV:
case TokenTypes.MOD:
case TokenTypes.LAND:
case TokenTypes.LOR:
case TokenTypes.LNOT:
processExpression(ast);
break;
case TokenTypes.LITERAL_FOR:
Expand Down Expand Up @@ -239,8 +255,7 @@ private void visitLiteralFor(DetailAST ast) {
}

/**
* Checks parens inside {@link TokenTypes#EXPR}, {@link TokenTypes#QUESTION}
* and {@link TokenTypes#METHOD_CALL}.
* Checks all parens that are children of a given Token.
*
* @param ast the token to check.
*/
Expand All @@ -253,31 +268,10 @@ private void processExpression(DetailAST ast) {
else if (currentNode.getType() == TokenTypes.RPAREN && !isInTypecast(currentNode)) {
processRight(currentNode);
}
else if (currentNode.hasChildren() && !isAcceptableToken(currentNode)) {
// Traverse all subtree tokens which will never be configured
// to be launched in visitToken()
currentNode = currentNode.getFirstChild();
continue;
}

// Go up after processing the last child
while (currentNode.getNextSibling() == null && currentNode.getParent() != ast) {
currentNode = currentNode.getParent();
}
currentNode = currentNode.getNextSibling();
}
Comment on lines 270 to 272
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the deep scan and just check the first child and all of its siblings

PS D:\test> cat src/Test3.java

public class Test3 {
    int i =  (( (5*4) + ( 4 + 2)));
}
PS D:\test> java  -jar checkstyle-10.14.2-all.jar -t  src/Test3.java
COMPILATION_UNIT -> COMPILATION_UNIT [2:0]
`--CLASS_DEF -> CLASS_DEF [2:0]
    |--MODIFIERS -> MODIFIERS [2:0]
    |   `--LITERAL_PUBLIC -> public [2:0]
    |--LITERAL_CLASS -> class [2:7]
    |--IDENT -> Test3 [2:13]
    `--OBJBLOCK -> OBJBLOCK [2:19]
        |--LCURLY -> { [2:19]
        |--VARIABLE_DEF -> VARIABLE_DEF [3:4]
        |   |--MODIFIERS -> MODIFIERS [3:4]
        |   |--TYPE -> TYPE [3:4]
        |   |   `--LITERAL_INT -> int [3:4]
        |   |--IDENT -> i [3:8]
        |   |--ASSIGN -> = [3:10]
        |   |   `--EXPR -> EXPR [3:13]
        |   |       |--LPAREN -> ( [3:13]         <-- this and all its siblings only checked 
        |   |       |--LPAREN -> ( [3:14]
        |   |       |--PLUS -> + [3:22]
        |   |       |   |--LPAREN -> ( [3:16]          <-- no deep scan so this will not be picked
        |   |       |   |--STAR -> * [3:18]
        |   |       |   |   |--NUM_INT -> 5 [3:17]
        |   |       |   |   `--NUM_INT -> 4 [3:19]
        |   |       |   |--RPAREN -> ) [3:20]
        |   |       |   |--LPAREN -> ( [3:24]
        |   |       |   |--PLUS -> + [3:28]
        |   |       |   |   |--NUM_INT -> 4 [3:26]
        |   |       |   |   `--NUM_INT -> 2 [3:30]
        |   |       |   `--RPAREN -> ) [3:31]
        |   |       |--RPAREN -> ) [3:32]
        |   |       `--RPAREN -> ) [3:33]
        |   `--SEMI -> ; [3:34]
        `--RCURLY -> } [4:0]

}

/**
* Checks whether AcceptableTokens contains the given ast.
*
* @param ast the token to check.
* @return true if the ast is in AcceptableTokens.
*/
private boolean isAcceptableToken(DetailAST ast) {
return acceptableTokens.get(ast.getType());
}

Comment on lines -276 to -280
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this anymore we needed it to avoid checking the same node twice. while doing a deep scan when we found a subtree that is acceptableToken we don't visit it because it will be picked up with visitToken() later

now there is no deep scan for tokens so we don't need this check. I removed this method, the related unnecessary class fields and the test corresponding to it

/**
* Returns array of acceptable tokens.
*
Expand Down Expand Up @@ -306,6 +300,15 @@ private static int[] makeAcceptableTokens() {
TokenTypes.SUPER_CTOR_CALL,
TokenTypes.LAMBDA,
TokenTypes.RECORD_DEF,
TokenTypes.TYPECAST,
TokenTypes.STAR,
TokenTypes.PLUS,
TokenTypes.MINUS,
TokenTypes.DIV,
TokenTypes.MOD,
TokenTypes.LAND,
TokenTypes.LOR,
TokenTypes.LNOT,
Comment on lines +303 to +311
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some tokens that got missed due to having no deep scan (we may need to add some more tokens but those were the obvious one from the test and input files)

};
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/whitespace/ParenPadCheck.xml
View file
Open in desktop
Expand Up @@ -26,7 +26,7 @@
type="com.puppycrawl.tools.checkstyle.checks.whitespace.PadOption">
<description>Specify policy on how to pad parentheses.</description>
</property>
<property default-value="ANNOTATION,ANNOTATION_FIELD_DEF,CTOR_CALL,CTOR_DEF,DOT,ENUM_CONSTANT_DEF,EXPR,LITERAL_CATCH,LITERAL_DO,LITERAL_FOR,LITERAL_IF,LITERAL_NEW,LITERAL_SWITCH,LITERAL_SYNCHRONIZED,LITERAL_WHILE,METHOD_CALL,METHOD_DEF,QUESTION,RESOURCE_SPECIFICATION,SUPER_CTOR_CALL,LAMBDA,RECORD_DEF"
<property default-value="ANNOTATION,ANNOTATION_FIELD_DEF,CTOR_CALL,CTOR_DEF,DOT,ENUM_CONSTANT_DEF,EXPR,LITERAL_CATCH,LITERAL_DO,LITERAL_FOR,LITERAL_IF,LITERAL_NEW,LITERAL_SWITCH,LITERAL_SYNCHRONIZED,LITERAL_WHILE,METHOD_CALL,METHOD_DEF,QUESTION,RESOURCE_SPECIFICATION,SUPER_CTOR_CALL,LAMBDA,RECORD_DEF,TYPECAST,STAR,PLUS,MINUS,DIV,MOD,LAND,LOR,LNOT"
name="tokens"
type="java.lang.String[]"
validation-type="tokenSet">
Expand Down
2 changes: 1 addition & 1 deletion src/main/resources/google_checks.xml
View file
Open in desktop
Expand Up @@ -292,7 +292,7 @@
EXPR, LITERAL_CATCH, LITERAL_DO, LITERAL_FOR, LITERAL_IF, LITERAL_NEW,
LITERAL_SWITCH, LITERAL_SYNCHRONIZED, LITERAL_WHILE, METHOD_CALL,
METHOD_DEF, QUESTION, RESOURCE_SPECIFICATION, SUPER_CTOR_CALL, LAMBDA,
RECORD_DEF"/>
RECORD_DEF, TYPECAST, STAR, PLUS, MINUS, DIV, MOD, LAND, LOR, LNOT"/>
</module>
<module name="OperatorWrap">
<property name="option" value="NL"/>
Expand Down
27 changes: 0 additions & 27 deletions src/test/java/com/puppycrawl/tools/checkstyle/checks/whitespace/ParenPadCheckTest.java
View file
Open in desktop
Expand Up @@ -28,11 +28,8 @@
import org.junit.jupiter.api.Test;

import com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport;
import com.puppycrawl.tools.checkstyle.DetailAstImpl;
import com.puppycrawl.tools.checkstyle.api.CheckstyleException;
import com.puppycrawl.tools.checkstyle.internal.utils.TestUtil;
import com.puppycrawl.tools.checkstyle.utils.CommonUtil;
import com.puppycrawl.tools.checkstyle.utils.TokenUtil;

public class ParenPadCheckTest
extends AbstractModuleTestSupport {
Expand Down Expand Up @@ -543,28 +540,4 @@ public void testParenPadForEnum() throws Exception {
getPath("InputParenPadForEnum.java"), expected);
}

/**
* Pitest requires us to specify more concrete lower bound for condition for
* ParenPadCheck#isAcceptableToken as nodes of first several types like CTOR_DEF,
* METHOD_DEF will never reach this method. It is hard to recreate conditions for
* all tokens to go through this method. We do not want to change main code to have
* this set ok tokens more exact, because it will not be ease to understand.
* So we have to use reflection to be sure all
* acceptable tokens pass that check.
*/
@Test
public void testIsAcceptableToken() throws Exception {
final ParenPadCheck check = new ParenPadCheck();
final DetailAstImpl ast = new DetailAstImpl();
final String message = "Expected that all acceptable tokens will pass isAcceptableToken "
+ "method, but some token don't: ";

for (int token : check.getAcceptableTokens()) {
ast.setType(token);
assertWithMessage(message + TokenUtil.getTokenName(token))
.that(TestUtil.<Boolean>invokeMethod(check, "isAcceptableToken", ast))
.isTrue();
}
}

}
36 changes: 36 additions & 0 deletions src/xdocs/checks/whitespace/parenpad.xml
View file
Open in desktop
Expand Up @@ -92,6 +92,24 @@
LAMBDA</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#RECORD_DEF">
RECORD_DEF</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#TYPECAST">
TYPECAST</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#STAR">
STAR</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#PLUS">
PLUS</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MINUS">
MINUS</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#DIV">
DIV</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MOD">
MOD</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LAND">
LAND</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LOR">
LOR</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LNOT">
LNOT</a>
.
</td>
<td>
Expand Down Expand Up @@ -139,6 +157,24 @@
LAMBDA</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#RECORD_DEF">
RECORD_DEF</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#TYPECAST">
TYPECAST</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#STAR">
STAR</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#PLUS">
PLUS</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MINUS">
MINUS</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#DIV">
DIV</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MOD">
MOD</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LAND">
LAND</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LOR">
LOR</a>
, <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LNOT">
LNOT</a>
.
</td>
<td>3.0</td>
Expand Down