Skip to content

Commit

Permalink
Handle fewer trivially constant expressions in ComplexBooleanConstant
Browse files Browse the repository at this point in the history
follow-up to 1b8119b

Restore previous handling of expressions containing constant fields, which
are less likely to be mistakes.

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=206348225
  • Loading branch information
cushon authored and ronshapiro committed Aug 2, 2018
1 parent a6831e7 commit d0ecbc2
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 52 deletions.
Expand Up @@ -15,8 +15,6 @@
*/
package com.google.errorprone.bugpatterns;

import static com.google.errorprone.matchers.Description.NO_MATCH;

import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.Category;
import com.google.errorprone.BugPattern.ProvidesFix;
Expand All @@ -27,10 +25,10 @@
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.BinaryTree;
import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.LiteralTree;
import com.sun.source.tree.UnaryTree;
import com.sun.source.util.SimpleTreeVisitor;
import com.sun.tools.javac.tree.JCTree.JCLiteral;
import java.util.Objects;

/** @author Sumit Bhagwani (bhagwani@google.com) */
Expand All @@ -44,9 +42,9 @@ public class ComplexBooleanConstant extends BugChecker implements BinaryTreeMatc

@Override
public Description matchBinary(BinaryTree tree, VisitorState state) {
Boolean constValue = booleanConstValue(tree);
Boolean constValue = booleanValue(tree);
if (constValue == null) {
return NO_MATCH;
return Description.NO_MATCH;
}
return buildDescription(tree)
.addFix(SuggestedFix.replace(tree, constValue.toString()))
Expand All @@ -57,60 +55,59 @@ public Description matchBinary(BinaryTree tree, VisitorState state) {
.build();
}

Boolean booleanConstValue(Tree tree) {
return tree.accept(
Boolean booleanValue(BinaryTree tree) {
if (tree.getLeftOperand() instanceof JCLiteral && tree.getRightOperand() instanceof JCLiteral) {
return ASTHelpers.constValue(tree, Boolean.class);
}
// Handle `&& false` and `|| true` even if the other operand is a non-trivial constant
// (including constant fields).
SimpleTreeVisitor<Boolean, Void> boolValue =
new SimpleTreeVisitor<Boolean, Void>() {
@Override
protected Boolean defaultAction(Tree node, Void unused) {
return ASTHelpers.constValue(node, Boolean.class);
}

@Override
public Boolean visitIdentifier(IdentifierTree node, Void aVoid) {
// Ignore constant variables.
return null;
}

@Override
public Boolean visitMemberSelect(MemberSelectTree node, Void aVoid) {
// Ignore constant variables.
public Boolean visitLiteral(LiteralTree node, Void aVoid) {
if (node.getValue() instanceof Boolean) {
return (Boolean) node.getValue();
}
return null;
}

@Override
public Boolean visitBinary(BinaryTree node, Void unused) {
Boolean result = defaultAction(node, null);
if (result != null
&& node.getLeftOperand().getKind() != Tree.Kind.IDENTIFIER
&& node.getRightOperand().getKind() != Tree.Kind.IDENTIFIER) {
return result;
public Boolean visitUnary(UnaryTree node, Void aVoid) {
Boolean r = node.getExpression().accept(this, null);
if (r == null) {
return null;
}
Boolean lhs = node.getLeftOperand().accept(this, null);
Boolean rhs = node.getRightOperand().accept(this, null);
switch (node.getKind()) {
case CONDITIONAL_AND:
case AND:
if (lhs != null && rhs != null) {
return lhs && rhs;
}
if (Objects.equals(lhs, Boolean.FALSE) || Objects.equals(rhs, Boolean.FALSE)) {
return false;
}
break;
case CONDITIONAL_OR:
case OR:
if (lhs != null && rhs != null) {
return lhs || rhs;
}
if (Objects.equals(lhs, Boolean.TRUE) || Objects.equals(rhs, Boolean.TRUE)) {
return true;
}
break;
default: // fall out
case LOGICAL_COMPLEMENT:
return !r;
default:
return null;
}
return null;
}
},
null);
};
Boolean lhs = tree.getLeftOperand().accept(boolValue, null);
Boolean rhs = tree.getRightOperand().accept(boolValue, null);
switch (tree.getKind()) {
case CONDITIONAL_AND:
case AND:
if (lhs != null && rhs != null) {
return lhs && rhs;
}
if (Objects.equals(lhs, Boolean.FALSE) || Objects.equals(rhs, Boolean.FALSE)) {
return false;
}
break;
case CONDITIONAL_OR:
case OR:
if (lhs != null && rhs != null) {
return lhs || rhs;
}
if (Objects.equals(lhs, Boolean.TRUE) || Objects.equals(rhs, Boolean.TRUE)) {
return true;
}
break;
default: // fall out
}
return null;
}
}
Expand Up @@ -105,6 +105,8 @@ public void negative() {
" static final int A = 1;",
" static final int B = 2;",
" static final boolean C = A > B;",
" static final boolean D = A + B > 0;",
" static final boolean E = (A + B) > 0;",
"}")
.doTest();
}
Expand Down

0 comments on commit d0ecbc2

Please sign in to comment.