Skip to content

Commit

Permalink
Consolidate checks for reference equality
Browse files Browse the repository at this point in the history
RELNOTES: N/A
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=122224435
  • Loading branch information
cushon committed May 19, 2016
1 parent 2da67ef commit b1d7867
Show file tree
Hide file tree
Showing 6 changed files with 179 additions and 315 deletions.
@@ -0,0 +1,112 @@
/*
* Copyright 2015 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.bugpatterns;

import static com.google.errorprone.dataflow.nullnesspropagation.Nullness.NONNULL;
import static com.google.errorprone.dataflow.nullnesspropagation.Nullness.NULL;

import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher;
import com.google.errorprone.dataflow.nullnesspropagation.Nullness;
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.ExpressionTree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.util.TreePath;

/**
* Abstract implementation of a BugPattern that detects the use of reference equality to compare
* classes with value semantics.
*
* <p>See e.g. {@link NumericEquality}, {@link OptionalEquality},
* {@link ProtoStringFieldReferenceEquality}, and {@link StringEquality}.
*
* @author cushon@google.com (Liam Miller-Cushon)
*/
public abstract class AbstractReferenceEquality extends BugChecker implements BinaryTreeMatcher {

protected abstract boolean matchArgument(ExpressionTree tree, VisitorState state);

@Override
public final Description matchBinary(BinaryTree tree, VisitorState state) {
switch (tree.getKind()) {
case EQUAL_TO:
case NOT_EQUAL_TO:
break;
default:
return Description.NO_MATCH;
}
if (tree.getLeftOperand().getKind() == Kind.NULL_LITERAL
|| !matchArgument(tree.getLeftOperand(), state)) {
return Description.NO_MATCH;
}
if (tree.getRightOperand().getKind() == Kind.NULL_LITERAL
|| !matchArgument(tree.getRightOperand(), state)) {
return Description.NO_MATCH;
}

Description.Builder builder = buildDescription(tree);
addFixes(builder, tree, state);
return builder.build();
}

protected void addFixes(Description.Builder builder, BinaryTree tree, VisitorState state) {
ExpressionTree lhs = tree.getLeftOperand();
ExpressionTree rhs = tree.getRightOperand();

// Swap the order (e.g. rhs.equals(lhs) if the rhs is a non-null constant, and the lhs is not
if (ASTHelpers.constValue(lhs) == null && ASTHelpers.constValue(rhs) != null) {
ExpressionTree tmp = lhs;
lhs = rhs;
rhs = tmp;
}

String prefix = tree.getKind() == Kind.NOT_EQUAL_TO ? "!" : "";
String lhsSource = state.getSourceForNode(lhs);
String rhsSource = state.getSourceForNode(rhs);

Nullness nullness = getNullness(lhs, state);

// If the lhs is possibly-null, provide both options.
if (nullness != NONNULL) {
builder.addFix(
SuggestedFix.builder()
.replace(
tree, String.format("%sObjects.equals(%s, %s)", prefix, lhsSource, rhsSource))
.addImport("java.util.Objects")
.build());
}
if (nullness != NULL) {
builder.addFix(
SuggestedFix.replace(
tree,
String.format(
"%s%s.equals(%s)",
prefix,
lhs instanceof BinaryTree ? String.format("(%s)", lhsSource) : lhsSource,
rhsSource)));
}
}

private Nullness getNullness(ExpressionTree expr, VisitorState state) {
TreePath pathToExpr = new TreePath(state.getPath(), expr);
return state.getNullnessAnalysis().getNullness(pathToExpr, state.context);
}
}
Expand Up @@ -17,77 +17,42 @@
import static com.google.errorprone.BugPattern.Category.JDK;
import static com.google.errorprone.BugPattern.MaturityLevel.EXPERIMENTAL;
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.Matchers.binaryTree;
import static com.google.errorprone.matchers.Matchers.isSubtypeOf;
import static com.google.errorprone.matchers.Matchers.kindIs;
import static com.google.errorprone.matchers.Matchers.not;
import static com.sun.source.tree.Tree.Kind.EQUAL_TO;
import static com.sun.source.tree.Tree.Kind.NOT_EQUAL_TO;

import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;

import com.sun.source.tree.BinaryTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Flags;
import com.sun.tools.javac.code.Symbol;

/**
* @author scottjohnson@google.com (Scott Johnson)
*/
@BugPattern(name = "NumericEquality",
summary = "Numeric comparison using reference equality instead of value equality",
explanation = "Numbers are compared for reference equality/inequality using == or != "
+ "instead of for value equality using .equals()",
category = JDK, severity = ERROR, maturity = EXPERIMENTAL)
public class NumericEquality extends BugChecker implements BinaryTreeMatcher {

public static final Matcher<ExpressionTree> SUBCLASS_OF_NUMBER =
allOf(isSubtypeOf("java.lang.Number"), not(kindIs(Tree.Kind.NULL_LITERAL)));
public static final Matcher<BinaryTree> MATCHER = allOf(
anyOf(kindIs(EQUAL_TO), kindIs(NOT_EQUAL_TO)),
binaryTree(SUBCLASS_OF_NUMBER, SUBCLASS_OF_NUMBER));
@BugPattern(
name = "NumericEquality",
summary = "Numeric comparison using reference equality instead of value equality",
explanation =
"Numbers are compared for reference equality/inequality using == or != "
+ "instead of for value equality using .equals()",
category = JDK,
severity = ERROR,
maturity = EXPERIMENTAL
)
public class NumericEquality extends AbstractReferenceEquality {

@Override
public Description matchBinary(BinaryTree tree, VisitorState state) {
ExpressionTree leftOperand = tree.getLeftOperand();
ExpressionTree rightOperand = tree.getRightOperand();
Symbol left = ASTHelpers.getSymbol(leftOperand);
Symbol right = ASTHelpers.getSymbol(rightOperand);
if (left == null || right == null) {
return Description.NO_MATCH;
}
// Using a static final object as a sentinel is OK
if ((isFinal(left) && left.isStatic()) || (isFinal(right) && right.isStatic())) {
return Description.NO_MATCH;
}
// Match left and right operand to subclasses of java.lang.Number and not null
if (!MATCHER.matches(tree, state)) {
return Description.NO_MATCH;
protected boolean matchArgument(ExpressionTree tree, VisitorState state) {
if (!ASTHelpers.isSubtype(
ASTHelpers.getType(tree), state.getTypeFromString("java.lang.Number"), state)) {
return false;
}

StringBuilder fixedExpression = new StringBuilder();

if (tree.getKind() == Tree.Kind.NOT_EQUAL_TO) {
fixedExpression.append("!");
Symbol sym = ASTHelpers.getSymbol(tree);
if (isFinal(sym) && sym.isStatic()) {
// Using a static final object as a sentinel is OK
return false;
}
fixedExpression.append(
"Objects.equals(" + leftOperand + ", " + rightOperand + ")");

Fix fix = SuggestedFix.builder()
.replace(tree, fixedExpression.toString())
.addImport("java.util.Objects")
.build();
return describeMatch(tree, fix);
return true;
}

public static boolean isFinal(Symbol s) {
Expand Down
Expand Up @@ -19,21 +19,13 @@
import static com.google.errorprone.BugPattern.Category.GUAVA;
import static com.google.errorprone.BugPattern.MaturityLevel.MATURE;
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static com.google.errorprone.dataflow.nullnesspropagation.Nullness.NONNULL;
import static com.google.errorprone.dataflow.nullnesspropagation.Nullness.NULL;

import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher;
import com.google.errorprone.dataflow.nullnesspropagation.Nullness;
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.ExpressionTree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Type;

/** @author cushon@google.com (Liam Miller-Cushon) */
Expand All @@ -47,63 +39,19 @@
severity = ERROR,
maturity = MATURE
)
public class OptionalEquality extends BugChecker implements BinaryTreeMatcher {
public class OptionalEquality extends AbstractReferenceEquality {

@Override
public final Description matchBinary(BinaryTree tree, VisitorState state) {
switch (tree.getKind()) {
case EQUAL_TO:
case NOT_EQUAL_TO:
break;
default:
return Description.NO_MATCH;
}
if (!isOptionalType(ASTHelpers.getType(tree.getLeftOperand()), state)) {
return Description.NO_MATCH;
}
if (!isOptionalType(ASTHelpers.getType(tree.getRightOperand()), state)) {
return Description.NO_MATCH;
}

Description.Builder builder = buildDescription(tree);
addFixes(builder, tree, state);
return builder.build();
}

void addFixes(Description.Builder builder, BinaryTree tree, VisitorState state) {
String prefix = tree.getKind() == Kind.NOT_EQUAL_TO ? "!" : "";
String lhs = state.getSourceForNode(tree.getLeftOperand());
String rhs = state.getSourceForNode(tree.getRightOperand());
Nullness nullness = getNullness(tree.getLeftOperand(), state);

// If the lhs is possibly-null, provide both options.
private static final ImmutableSet<String> OPTIONAL_CLASSES =
ImmutableSet.of(com.google.common.base.Optional.class.getName(), "java.util.Optional");

// Never swap the order (e.g. rhs.equals(lhs) when lhs is nullable and rhs is non-null), since
// that has observable side-effects.

if (nullness != NONNULL) {
builder.addFix(
SuggestedFix.builder()
.replace(tree, String.format("%sObjects.equals(%s, %s)", prefix, lhs, rhs))
.addImport("java.util.Objects")
.build());
}
if (nullness != NULL) {
builder.addFix(
SuggestedFix.replace(tree, String.format("%s%s.equals(%s)", prefix, lhs, rhs)));
@Override
protected boolean matchArgument(ExpressionTree tree, VisitorState state) {
Type type = ASTHelpers.getType(tree);
for (String className : OPTIONAL_CLASSES) {
if (ASTHelpers.isSameType(type, state.getTypeFromString(className), state)) {
return true;
}
}
}

private Nullness getNullness(ExpressionTree expr, VisitorState state) {
TreePath pathToExpr = new TreePath(state.getPath(), expr);
return state.getNullnessAnalysis().getNullness(pathToExpr, state.context);
}

private static boolean isOptionalType(Type type, VisitorState state) {
Type guavaOptional = state.getTypeFromString(com.google.common.base.Optional.class.getName());
Type utilOptional = state.getTypeFromString("java.util.Optional");
// TODO(cushon): write a test for the util.Optional case once we can depend on 8.
return ASTHelpers.isSameType(type, guavaOptional, state)
|| ASTHelpers.isSameType(type, utilOptional, state);
return false;
}
}
Expand Up @@ -25,49 +25,30 @@

import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;

import com.sun.source.tree.BinaryTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.Tree.Kind;
import com.sun.tools.javac.tree.JCTree;

@BugPattern(category = ONE_OFF, maturity = EXPERIMENTAL,
name = "ProtoStringFieldReferenceEquality", severity = ERROR,
summary = "Comparing protobuf fields of type String using reference equality",
explanation = "Comparing strings with == is almost always an error, but it is an error 100% "
+ "of the time when one of the strings is a protobuf field. Additionally, protobuf "
+ "fields cannot be null, so Object.equals(Object) is always more correct.")
public class ProtoStringFieldReferenceEquality extends BugChecker implements BinaryTreeMatcher {
@BugPattern(
category = ONE_OFF,
maturity = EXPERIMENTAL,
name = "ProtoStringFieldReferenceEquality",
severity = ERROR,
summary = "Comparing protobuf fields of type String using reference equality",
explanation =
"Comparing strings with == is almost always an error, but it is an error 100% "
+ "of the time when one of the strings is a protobuf field. Additionally, protobuf "
+ "fields cannot be null, so Object.equals(Object) is always more correct."
)
public class ProtoStringFieldReferenceEquality extends AbstractReferenceEquality {

private static final String PROTO_SUPER_CLASS = "com.google.protobuf.GeneratedMessage";

private static final Matcher<ExpressionTree> PROTO_STRING_METHOD = allOf(
instanceMethod().onDescendantOf(PROTO_SUPER_CLASS),
isSameType("java.lang.String"));
private static final Matcher<ExpressionTree> PROTO_STRING_METHOD =
allOf(instanceMethod().onDescendantOf(PROTO_SUPER_CLASS), isSameType("java.lang.String"));

@Override
public Description matchBinary(BinaryTree tree, VisitorState state) {
if (tree.getKind() != Kind.EQUAL_TO && tree.getKind() != Kind.NOT_EQUAL_TO) {
return Description.NO_MATCH;
}
String leftOperand = state.getSourceForNode((JCTree) tree.getLeftOperand()).toString();
String rightOperand = state.getSourceForNode((JCTree) tree.getRightOperand()).toString();
if ((PROTO_STRING_METHOD.matches(tree.getLeftOperand(), state)
&& tree.getRightOperand().getKind() != Kind.NULL_LITERAL)
|| (PROTO_STRING_METHOD.matches(tree.getRightOperand(), state)
&& tree.getLeftOperand().getKind() != Kind.NULL_LITERAL)) {
String result = leftOperand + ".equals(" + rightOperand + ")";
if (tree.getKind() == Kind.NOT_EQUAL_TO) {
result = "!" + result;
}
return describeMatch(tree, SuggestedFix.replace(tree, result));
} else {
return Description.NO_MATCH;
}
protected boolean matchArgument(ExpressionTree tree, VisitorState state) {
return PROTO_STRING_METHOD.matches(tree, state);
}

}

0 comments on commit b1d7867

Please sign in to comment.