Skip to content

Commit

Permalink
Merge remote-tracking branch 'upstream/master' into jdk12
Browse files Browse the repository at this point in the history
  • Loading branch information
don-vip committed Sep 5, 2018
2 parents c8062e6 + ca3356d commit 8ced61b
Show file tree
Hide file tree
Showing 11 changed files with 400 additions and 30 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/*
* Copyright 2018 The Error Prone Authors.
*
* 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.common.collect.Iterables.getLast;
import static com.google.errorprone.BugPattern.Category.JDK;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.matchers.Matchers.equalsMethodDeclaration;
import static com.google.errorprone.matchers.Matchers.instanceEqualsInvocation;
import static com.google.errorprone.matchers.Matchers.staticEqualsInvocation;
import static com.google.errorprone.util.ASTHelpers.getReceiver;
import static com.google.errorprone.util.ASTHelpers.getType;
import static com.google.errorprone.util.ASTHelpers.isSameType;

import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.ProvidesFix;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Type;
import java.util.List;

/**
* Matches use of {@code BigDecimal#equals}, which compares scale as well (which is not likely to be
* intended).
*
* @author ghm@google.com (Graeme Morgan)
*/
@BugPattern(
name = "BigDecimalEquals",
summary = "BigDecimal#equals has surprising behavior: it also compares scale.",
category = JDK,
severity = WARNING,
providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION)
public final class BigDecimalEquals extends BugChecker implements MethodInvocationTreeMatcher {
private static final String BIG_DECIMAL = "java.math.BigDecimal";

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
Tree receiver;
Tree argument;
List<? extends ExpressionTree> arguments = tree.getArguments();
Type bigDecimal = state.getTypeFromString(BIG_DECIMAL);
boolean handleNulls;

if (staticEqualsInvocation().matches(tree, state)) {
handleNulls = true;
receiver = arguments.get(arguments.size() - 2);
argument = getLast(arguments);
} else if (instanceEqualsInvocation().matches(tree, state)) {
handleNulls = false;
receiver = getReceiver(tree);
argument = arguments.get(0);
} else {
return NO_MATCH;
}
MethodTree enclosingMethod = state.findEnclosing(MethodTree.class);
if (enclosingMethod != null && equalsMethodDeclaration().matches(enclosingMethod, state)) {
return NO_MATCH;
}

boolean isReceiverBigDecimal = isSameType(getType(receiver), bigDecimal, state);
boolean isTargetBigDecimal = isSameType(getType(argument), bigDecimal, state);

if (!isReceiverBigDecimal && !isTargetBigDecimal) {
return NO_MATCH;
}

// One is BigDecimal but the other isn't: report a finding without a fix.
if (isReceiverBigDecimal != isTargetBigDecimal) {
return describeMatch(tree);
}
return describe(tree, state, receiver, argument, handleNulls);
}

private Description describe(
MethodInvocationTree tree,
VisitorState state,
Tree receiver,
Tree argument,
boolean handleNulls) {
return describeMatch(tree);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,13 @@
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.ParameterizedTypeTree;
import com.sun.source.tree.ParenthesizedTree;
import com.sun.source.tree.ReturnTree;
import com.sun.source.tree.StatementTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.tree.VariableTree;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -128,22 +131,36 @@ Optional<Fix> maybeFix(NewClassTree tree, VisitorState state, BlockTree block) {
// check the enclosing context: calls to Collections.unmodifiable* are now redundant, and
// if there's an enclosing constant variable declaration we can rewrite its type to Immutable*
Tree unmodifiable = null;
VariableTree enclosingVariable = null;
boolean constant = false;
Tree typeTree = null;
Tree toReplace = null;

for (Tree enclosing : state.getPath().getParentPath()) {
for (TreePath path = state.getPath().getParentPath();
path != null;
path = path.getParentPath()) {
Tree enclosing = path.getLeaf();
if (unmodifiableMatcher.matches(enclosing, state)) {
unmodifiable = enclosing;
continue;
}
if (enclosing instanceof ParenthesizedTree) {
continue;
}
if (enclosing instanceof VariableTree) {
enclosingVariable = (VariableTree) enclosing;
VariableTree enclosingVariable = (VariableTree) enclosing;
toReplace = enclosingVariable.getInitializer();
typeTree = enclosingVariable.getType();
VarSymbol symbol = ASTHelpers.getSymbol(enclosingVariable);
constant =
symbol.isStatic()
&& symbol.getModifiers().contains(Modifier.FINAL)
&& symbol.getKind() == ElementKind.FIELD;
}
if (enclosing instanceof ReturnTree) {
toReplace = ((ReturnTree) enclosing).getExpression();
MethodTree enclosingMethod = ASTHelpers.findEnclosingNode(path, MethodTree.class);
typeTree = enclosingMethod == null ? null : enclosingMethod.getReturnType();
}
break;
}
SuggestedFix.Builder fix = SuggestedFix.builder();
Expand Down Expand Up @@ -172,14 +189,13 @@ Optional<Fix> maybeFix(NewClassTree tree, VisitorState state, BlockTree block) {
if (unmodifiable != null || constant) {
// there's an enclosing unmodifiable* call, or we're in the initializer of a constant,
// so rewrite the variable's type to be immutable and drop the unmodifiable* method
Tree typeType = enclosingVariable.getType();
if (typeType instanceof ParameterizedTypeTree) {
typeType = ((ParameterizedTypeTree) typeType).getType();
if (typeTree instanceof ParameterizedTypeTree) {
typeTree = ((ParameterizedTypeTree) typeTree).getType();
}
if (typeTree != null) {
fix.replace(typeTree, immutableType);
}
fix.replace(typeType, immutableType)
.replace(
unmodifiable != null ? unmodifiable : enclosingVariable.getInitializer(),
replacement);
fix.replace(unmodifiable == null ? toReplace : unmodifiable, replacement);
} else {
// the result may need to be mutable, so rewrite e.g.
// `new ArrayList<>() {{ add(1); }}` -> `new ArrayList<>(ImmutableList.of(1));`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static com.google.errorprone.BugPattern.Category.JDK;
import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.BugPattern.StandardTags.STYLE;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static java.util.stream.Collectors.joining;

Expand Down Expand Up @@ -56,6 +57,7 @@
category = JDK,
severity = SUGGESTION,
linkType = CUSTOM,
tags = STYLE,
link = "https://google.github.io/styleguide/javaguide.html#s3.4.2.1-overloads-never-split"
)
public class UngroupedOverloads extends BugChecker implements ClassTreeMatcher {
Expand Down
32 changes: 25 additions & 7 deletions core/src/main/java/com/google/errorprone/bugpatterns/Unused.java
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ public final class Unused extends BugChecker implements CompilationUnitTreeMatch
"javax.persistence.Id",
"javax.persistence.Version",
"javax.xml.bind.annotation.XmlElement",
"org.junit.Rule",
"org.mockito.Mock",
"org.openqa.selenium.support.FindBy",
"org.openqa.selenium.support.FindBys");
Expand All @@ -152,6 +153,10 @@ public final class Unused extends BugChecker implements CompilationUnitTreeMatch
ImmutableSet.of(
);

/** The set of types exempting a field of type extending them. */
private static final ImmutableSet<String> EXEMPTING_FIELD_SUPER_TYPES =
ImmutableSet.of("org.junit.rules.TestRule");

private static final ImmutableList<String> SPECIAL_FIELDS =
ImmutableList.of(
"serialVersionUID",
Expand Down Expand Up @@ -247,11 +252,15 @@ public Void visitVariable(VariableTree variableTree, Void unused) {
if (isSuppressed(variableTree)) {
return null;
}
super.visitVariable(variableTree, unused);
VarSymbol symbol = getSymbol(variableTree);
if (symbol == null) {
return null;
}
if (symbol.getKind() == ElementKind.FIELD
&& exemptedFieldBySuperType(getType(variableTree), state)) {
return null;
}
super.visitVariable(variableTree, null);
// Return if the element is exempted by an annotation.
if (exemptedByAnnotation(
variableTree.getModifiers().getAnnotations(), EXEMPTING_VARIABLE_ANNOTATIONS, state)) {
Expand Down Expand Up @@ -284,6 +293,11 @@ public Void visitVariable(VariableTree variableTree, Void unused) {
return null;
}

private boolean exemptedFieldBySuperType(Type type, VisitorState state) {
return EXEMPTING_FIELD_SUPER_TYPES.stream()
.anyMatch(t -> isSubtype(type, state.getTypeFromString(t), state));
}

private boolean isFieldEligibleForChecking(VariableTree variableTree, VarSymbol symbol) {
if (variableTree.getModifiers().getFlags().isEmpty()
&& ASTHelpers.hasDirectAnnotationWithSimpleName(variableTree, "Inject")) {
Expand Down Expand Up @@ -659,16 +673,20 @@ private static ImmutableList<SuggestedFix> buildUnusedVarFixes(
VariableTree variableTree = (VariableTree) statement;
if (hasSideEffect(((VariableTree) statement).getInitializer())) {
encounteredSideEffects = true;
String newContent = "";
if (varKind == ElementKind.FIELD) {
newContent =
String newContent =
String.format(
"%s{ %s; }",
varSymbol.isStatic() ? "static " : "", variableTree.getInitializer());
varSymbol.isStatic() ? "static " : "",
state.getSourceForNode(variableTree.getInitializer()));
fix.merge(replaceWithComments(usagePath, newContent, state));
removeSideEffectsFix.replace(statement, "");
} else {
fix.replace(
statement,
String.format("%s;", state.getSourceForNode(variableTree.getInitializer())));
removeSideEffectsFix.replace(statement, "");
}
SuggestedFix replacement = replaceWithComments(usagePath, newContent, state);
fix.merge(replacement);
removeSideEffectsFix.merge(replacement);
} else if (isEnhancedForLoopVar(usagePath)) {
String newContent =
String.format(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import com.google.errorprone.bugpatterns.BadImport;
import com.google.errorprone.bugpatterns.BadInstanceof;
import com.google.errorprone.bugpatterns.BadShiftAmount;
import com.google.errorprone.bugpatterns.BigDecimalEquals;
import com.google.errorprone.bugpatterns.BigDecimalLiteralDouble;
import com.google.errorprone.bugpatterns.BooleanParameter;
import com.google.errorprone.bugpatterns.BoxedPrimitiveConstructor;
Expand Down Expand Up @@ -517,6 +518,7 @@ public static ScannerSupplier errorChecks() {
BadComparable.class,
BadImport.class,
BadInstanceof.class,
BigDecimalEquals.class,
BigDecimalLiteralDouble.class,
BoxedPrimitiveConstructor.class,
ByteBufferBackingArray.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* Copyright 2018 The Error Prone Authors.
*
* 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 com.google.errorprone.CompilationTestHelper;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/**
* Unit tests for {@link BigDecimalEquals}.
*
* @author ghm@google.com (Graeme Morgan)
*/
@RunWith(JUnit4.class)
public final class BigDecimalEqualsTest {
@Test
public void positive() {
CompilationTestHelper.newInstance(BigDecimalEquals.class, getClass())
.addSourceLines(
"Test.java",
"import com.google.common.base.Objects;",
"import java.math.BigDecimal;",
"class Test {",
" void test(BigDecimal a, BigDecimal b) {",
" // BUG: Diagnostic contains:",
" boolean foo = a.equals(b);",
" // BUG: Diagnostic contains:",
" boolean bar = !a.equals(b);",
" // BUG: Diagnostic contains:",
" boolean baz = Objects.equal(a, b);",
" }",
"}")
.doTest();
}


@Test
public void negative() {
CompilationTestHelper.newInstance(BigDecimalEquals.class, getClass())
.addSourceLines(
"Test.java",
"import java.math.BigDecimal;",
"class Test {",
" BigDecimal a;",
" BigDecimal b;",
" boolean f(String a, String b) {",
" return a.equals(b);",
" }",
" @Override public boolean equals(Object o) {",
" return a.equals(b);",
" }",
"}")
.doTest();
}
}
Loading

0 comments on commit 8ced61b

Please sign in to comment.