Skip to content

Commit

Permalink
Added functionality that recognizes when there is an adjacent parenth…
Browse files Browse the repository at this point in the history 
…esized expression that contains the same binary operator, and merges them instead of adding another layer of parenthesis.

RELNOTES: Added to OperatorPrecedence check so it does not add unnecessary parenthesis

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=201697769
  • Loading branch information
@cushon
seibelsabrina authored and cushon committed Jun 29, 2018
1 parent 51f90ff commit c01d43f
Show file tree
Hide file tree
Showing 2 changed files with 179 additions and 6 deletions.
63 changes: 57 additions & 6 deletions core/src/main/java/com/google/errorprone/bugpatterns/OperatorPrecedence.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,12 @@
import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.BinaryTree;
import com.sun.source.tree.ParenthesizedTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind;
import com.sun.tools.javac.tree.JCTree;
import com.sun.tools.javac.tree.JCTree.JCBinary;
import com.sun.tools.javac.tree.TreeInfo;
import java.util.EnumSet;
Expand All @@ -53,6 +56,9 @@ public class OperatorPrecedence extends BugChecker implements BinaryTreeMatcher
private static final EnumSet<Kind> ARITHMETIC =
EnumSet.of(Kind.PLUS, Kind.MULTIPLY, Kind.DIVIDE, Kind.MINUS);

private static final EnumSet<Kind> SAFE_ASSOCIATIVE_OPERATORS =
EnumSet.of(Kind.CONDITIONAL_AND, Kind.CONDITIONAL_OR, Kind.PLUS);

@Override
public Description matchBinary(BinaryTree tree, VisitorState state) {
Tree parent = state.getPath().getParentPath().getLeaf();
Expand All @@ -66,18 +72,63 @@ public Description matchBinary(BinaryTree tree, VisitorState state) {
if (!isConfusing(tree.getKind(), parent.getKind())) {
return NO_MATCH;
}
return describeMatch(
tree, SuggestedFix.builder().prefixWith(tree, "(").postfixWith(tree, ")").build());
return createAppropriateFix(tree, state);
}

private boolean isConfusing(Kind thisKind, Kind parentKind) {
if (CONDITIONAL.contains(thisKind) && CONDITIONAL.contains(parentKind)) {
return true;
}
if ((SHIFT.contains(thisKind) && ARITHMETIC.contains(parentKind))
|| (SHIFT.contains(parentKind) && ARITHMETIC.contains(thisKind))) {
return true;
return (SHIFT.contains(thisKind) && ARITHMETIC.contains(parentKind))
|| (SHIFT.contains(parentKind) && ARITHMETIC.contains(thisKind));
}

private Description createAppropriateFix(BinaryTree tree, VisitorState state) {
// If our expression is like: (A && B) && C, replace it with (A && B && C) instead of
// ((A && B) && C)
return SAFE_ASSOCIATIVE_OPERATORS.contains(tree.getKind())
&& tree.getLeftOperand() instanceof ParenthesizedTree
!= tree.getRightOperand() instanceof ParenthesizedTree
&& parenthesizedChildHasKind(tree, tree.getKind())
? leftOrRightFix(tree, state)
: basicFix(tree);
}

private Description leftOrRightFix(BinaryTree tree, VisitorState state) {
int startPos;
int endPos;
String prefix = "(";
String postfix = ")";
if (tree.getRightOperand() instanceof ParenthesizedTree) {
startPos = ((JCTree) tree.getRightOperand()).getStartPosition();
endPos = ((JCTree) tree.getRightOperand()).getStartPosition() + 1;
postfix = "";
} else {
startPos = state.getEndPosition(tree.getLeftOperand()) - 1;
endPos = state.getEndPosition(tree.getLeftOperand());
prefix = "";
}
return false;
return describeMatch(
tree,
SuggestedFix.builder()
.prefixWith(tree, prefix)
.replace(startPos, endPos, "")
.postfixWith(tree, postfix)
.build());
}

private Description basicFix(BinaryTree tree) {
return describeMatch(
tree, SuggestedFix.builder().prefixWith(tree, "(").postfixWith(tree, ")").build());
}

private static boolean parenthesizedChildHasKind(BinaryTree tree, Kind kind) {
Kind childKind =
ASTHelpers.stripParentheses(
tree.getLeftOperand() instanceof ParenthesizedTree
? tree.getLeftOperand()
: tree.getRightOperand())
.getKind();
return childKind == kind;
}
}
122 changes: 122 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/OperatorPrecedenceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.google.errorprone.bugpatterns;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -28,6 +29,9 @@ public class OperatorPrecedenceTest {
private final CompilationTestHelper compilationTestHelper =
CompilationTestHelper.newInstance(OperatorPrecedence.class, getClass());

private final BugCheckerRefactoringTestHelper helper =
BugCheckerRefactoringTestHelper.newInstance(new OperatorPrecedence(), getClass());

@Test
public void positive() {
compilationTestHelper
Expand Down Expand Up @@ -69,4 +73,122 @@ public void negative() {
"}")
.doTest();
}

@Test
public void positiveNotSpecialParenthesisCase() throws Exception {
helper
.addInputLines(
"Test.java",
"class Test {",
" boolean f(boolean a, boolean b, boolean c, boolean d, boolean e) {",
" boolean r = a || (b && c) && (d && e);",
" return r;",
" }",
" int f2(int a, int b, int c, int d) {",
" int e = a << (b + c) + d;",
" return e;",
" }",
" boolean f3(boolean a, boolean b, boolean c, boolean d, boolean e) {",
" boolean r = a || b && c;",
" return r;",
" }",
"}")
.addOutputLines(
"Test.java",
"class Test {",
" boolean f(boolean a, boolean b, boolean c, boolean d, boolean e) {",
" boolean r = a || ((b && c) && (d && e));",
" return r;",
" }",
" int f2(int a, int b, int c, int d) {",
" int e = a << (b + c + d);",
" return e;",
" }",
" boolean f3(boolean a, boolean b, boolean c, boolean d, boolean e) {",
" boolean r = a || (b && c);",
" return r;",
" }",
"}")
.doTest();
}

@Test
public void extraParenthesis() throws Exception {
helper
.addInputLines(
"Test.java", //
"class Test {",
" void f(boolean a, boolean b, boolean c, boolean d, boolean e) {",
" boolean g = (a || (b && c && d) && e);",
" }",
"}")
.addOutputLines(
"Test.java", //
"class Test {",
" void f(boolean a, boolean b, boolean c, boolean d, boolean e) {",
" boolean g = (a || (b && c && d && e));",
" }",
"}")
.doTest();
}

@Test
public void rightAndParenthesis() throws Exception {
helper
.addInputLines(
"Test.java", //
"class Test {",
" void f(boolean a, boolean b, boolean c, boolean d) {",
" boolean g = a || b && (c && d);",
" }",
"}")
.addOutputLines(
"Test.java", //
"class Test {",
" void f(boolean a, boolean b, boolean c, boolean d) {",
" boolean g = a || (b && c && d);",
" }",
"}")
.doTest();
}

@Test
public void leftAndParenthesis() throws Exception {
helper
.addInputLines(
"Test.java", //
"class Test {",
" void f(boolean a, boolean b, boolean c, boolean d) {",
" boolean g = a || (b && c) && d;",
" }",
"}")
.addOutputLines(
"Test.java", //
"class Test {",
" void f(boolean a, boolean b, boolean c, boolean d) {",
" boolean g = a || (b && c && d);",
" }",
"}")
.doTest();
}

@Test
public void aLotOfParenthesis() throws Exception {
helper
.addInputLines(
"Test.java", //
"class Test {",
" void f(boolean a, boolean b, boolean c, boolean d, boolean e) {",
" boolean g = (a || (b && c && d) && e);",
" }",
"}")
.addOutputLines(
"Test.java", //
"class Test {",
" void f(boolean a, boolean b, boolean c, boolean d, boolean e) {",
" boolean g = (a || (b && c && d && e));",
" }",
"}")
.doTest();
}
}

0 comments on commit c01d43f

Please sign in to comment.