From b8a8390ac9938b9ec59530166525f0ef01f0b78d Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Tue, 13 Oct 2026 16:32:38 -0700 Subject: [PATCH] Merge CollectionUndefinedEquality and ArrayAsKeyOfSetOrMap into UndefinedEquals. `UndefinedEquals`, `CollectionUndefinedEquality`, and `ArrayAsKeyOfSetOrMap` all report code that relies on `equals` or `hashCode` of types where those methods aren't meaningful, but each covered a different subset of use sites: direct equality, collection containment, and hash-based container creation. Add `AbstractEqualsHashCodeUsageChecker`, which finds the places where code relies on `equals` and `hashCode`, either explicitly or implicitly through collections, and lets each check decide which types are a problem for each kind of use. Rebuild `UndefinedEquals` on top of it, absorbing the other two checks and keeping their names as `altNames` so existing suppressions and `-Xep` flags keep working. PiperOrigin-RevId: 998967393 --- .../bugpatterns/AbstractAsKeyOfSetOrMap.java | 118 ------ .../AbstractEqualsHashCodeUsageChecker.java | 280 ++++++++++++++ .../bugpatterns/ArrayAsKeyOfSetOrMap.java | 44 --- .../bugpatterns/UndefinedEquals.java | 86 ++--- .../CollectionUndefinedEquality.java | 90 ----- .../scanner/BuiltInCheckerSuppliers.java | 4 - .../bugpatterns/ArrayAsKeyOfSetOrMapTest.java | 154 -------- .../bugpatterns/UndefinedEqualsTest.java | 359 +++++++++++++++++- .../CollectionUndefinedEqualityTest.java | 119 ------ .../bugpattern/CollectionUndefinedEquality.md | 27 -- docs/bugpattern/UndefinedEquals.md | 24 ++ 11 files changed, 678 insertions(+), 627 deletions(-) delete mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/AbstractAsKeyOfSetOrMap.java create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/AbstractEqualsHashCodeUsageChecker.java delete mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/ArrayAsKeyOfSetOrMap.java delete mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionUndefinedEquality.java delete mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/ArrayAsKeyOfSetOrMapTest.java delete mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionUndefinedEqualityTest.java delete mode 100644 docs/bugpattern/CollectionUndefinedEquality.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractAsKeyOfSetOrMap.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractAsKeyOfSetOrMap.java deleted file mode 100644 index 2357291246e..00000000000 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractAsKeyOfSetOrMap.java +++ /dev/null @@ -1,118 +0,0 @@ -/* - * 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.errorprone.matchers.Matchers.anyOf; - -import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; -import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; -import com.google.errorprone.matchers.Description; -import com.google.errorprone.matchers.Matcher; -import com.google.errorprone.matchers.Matchers; -import com.google.errorprone.matchers.method.MethodMatchers; -import com.google.errorprone.util.ASTHelpers; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.tree.NewClassTree; -import com.sun.tools.javac.code.Type; -import java.util.List; - -/** - * Check for usage of {@code Set} or {@code Map}. - * - * @author seibelsabrina@google.com (Sabrina Seibel) - */ -public abstract class AbstractAsKeyOfSetOrMap extends BugChecker - implements MethodInvocationTreeMatcher, NewClassTreeMatcher { - - private static final Matcher CONSTRUCTS_SET = - anyOf( - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.Sets") - .named("newHashSet"), - Matchers.constructor().forClass("java.util.HashSet")); - - private static final Matcher CONSTRUCTS_MULTISET = - anyOf( - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.HashMultiset") - .named("create") - .withNoParameters(), - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.LinkedHashMultiset") - .named("create")); - - private static final Matcher CONSTRUCTS_MAP = - anyOf( - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.Maps") - .named("newHashMap"), - Matchers.constructor().forClass("java.util.HashMap"), - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.Maps") - .named("newLinkedHashMap"), - Matchers.constructor().forClass("java.util.LinkedHashMap"), - Matchers.constructor().forClass("java.util.concurrent.ConcurrentHashMap"), - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.HashBiMap") - .named("create")); - - private static final Matcher CONSTRUCTS_MULTIMAP = - anyOf( - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.HashMultimap") - .named("create"), - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.LinkedHashMultimap") - .named("create"), - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.ArrayListMultimap") - .named("create"), - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.LinkedListMultimap") - .named("create")); - - private static final Matcher CONSTRUCTS_SET_OR_MAP = - anyOf(CONSTRUCTS_SET, CONSTRUCTS_MAP, CONSTRUCTS_MULTIMAP, CONSTRUCTS_MULTISET); - - protected abstract boolean isBadType(Type type, VisitorState state); - - private Description matchType(ExpressionTree tree, VisitorState state) { - if (CONSTRUCTS_SET_OR_MAP.matches(tree, state)) { - List argumentTypes = ASTHelpers.getResultType(tree).getTypeArguments(); - if (argumentTypes.isEmpty()) { - return Description.NO_MATCH; - } - Type typeArg = argumentTypes.getFirst(); - if (isBadType(typeArg, state)) { - return describeMatch(tree); - } - } - return Description.NO_MATCH; - } - - @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - return matchType(tree, state); - } - - @Override - public Description matchNewClass(NewClassTree tree, VisitorState state) { - return matchType(tree, state); - } -} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractEqualsHashCodeUsageChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractEqualsHashCodeUsageChecker.java new file mode 100644 index 00000000000..58a370da470 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractEqualsHashCodeUsageChecker.java @@ -0,0 +1,280 @@ +/* + * Copyright 2026 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.common.collect.Iterables.getOnlyElement; +import static com.google.errorprone.fixes.SuggestedFix.emptyFix; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.anyMethod; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.assertEqualsInvocation; +import static com.google.errorprone.matchers.Matchers.assertNotEqualsInvocation; +import static com.google.errorprone.matchers.Matchers.instanceEqualsInvocation; +import static com.google.errorprone.matchers.Matchers.receiverOfInvocation; +import static com.google.errorprone.matchers.Matchers.staticEqualsInvocation; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.suppliers.Suppliers.typeFromString; +import static com.google.errorprone.util.ASTHelpers.getReceiver; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.isSubtype; + +import com.google.common.collect.ImmutableList; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MemberReferenceTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; +import com.google.errorprone.bugpatterns.collectionincompatibletype.AbstractCollectionIncompatibleTypeMatcher.MatchResult; +import com.google.errorprone.bugpatterns.collectionincompatibletype.ContainmentMatchers; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.suppliers.Supplier; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberReferenceTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.TypeTag; +import java.util.List; +import java.util.Optional; +import org.jspecify.annotations.Nullable; + +/// A base class for checks that flag code relying on `equals` or `hashCode` of particular types. +/// +/// This class finds the places where `equals` and `hashCode` are used, either explicitly or +/// implicitly by collections, and subclasses decide which types should not be used in which ways. +public abstract class AbstractEqualsHashCodeUsageChecker extends BugChecker + implements MethodInvocationTreeMatcher, MemberReferenceTreeMatcher, NewClassTreeMatcher { + + /// A way that code can rely on the `equals` or `hashCode` of a type. + protected enum Usage { + /// A direct call to `equals`, e.g. `a.equals(b)` or `Objects.equals(a, b)`. + EQUALS, + /// An equality assertion, e.g. `assertEquals(a, b)` or Truth's `isEqualTo`. + ASSERTION, + /// A method that compares elements with `equals`, e.g. `Collection.contains`. + CONTAINMENT, + /// Creating a hash-based container, e.g. a `Set` element or a `Map`, `Multimap`, or `Multiset` + /// key, or a `BiMap` value. + HASH_KEY, + } + + /// Returns a diagnostic message if code should not rely on `type`'s `equals` or `hashCode` in the + /// given way. + protected abstract Optional problem(Type type, Usage usage, VisitorState state); + + /// Returns a fix for an [Usage#EQUALS] or [Usage#ASSERTION] finding. + protected Optional equalityFix( + MethodInvocationTree tree, Tree receiver, Tree argument, VisitorState state) { + return Optional.empty(); + } + + @Override + public final Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + Description description = checkEquality(tree, state); + if (description != NO_MATCH) { + return description; + } + description = checkContainment(tree, state); + if (description != NO_MATCH) { + return description; + } + if (HASH_CONTAINER_FACTORIES.matches(tree, state)) { + return checkHashKeys(tree, state); + } + return NO_MATCH; + } + + @Override + public final Description matchMemberReference(MemberReferenceTree tree, VisitorState state) { + return checkContainment(tree, state); + } + + @Override + public final Description matchNewClass(NewClassTree tree, VisitorState state) { + return checkHashKeys(tree, state); + } + + private Description checkEquality(MethodInvocationTree tree, VisitorState state) { + Tree receiver; + Tree argument; + Usage usage; + List arguments = tree.getArguments(); + if (staticEqualsInvocation().matches(tree, state)) { + receiver = arguments.get(arguments.size() - 2); + argument = getLast(arguments); + usage = Usage.EQUALS; + } else if (instanceEqualsInvocation().matches(tree, state)) { + receiver = getReceiver(tree); + argument = arguments.getFirst(); + usage = Usage.EQUALS; + } else if (assertEqualsInvocation().matches(tree, state) + || assertNotEqualsInvocation().matches(tree, state)) { + receiver = arguments.get(arguments.size() - 2); + argument = getLast(arguments); + usage = Usage.ASSERTION; + } else if (ASSERT_THAT_EQUALS.matches(tree, state)) { + receiver = getOnlyElement(arguments); + argument = getOnlyElement(((MethodInvocationTree) getReceiver(tree)).getArguments()); + usage = Usage.ASSERTION; + } else { + return NO_MATCH; + } + return findProblem(getType(receiver), usage, state) + .or(() -> findProblem(getType(argument), usage, state)) + .map( + message -> + buildDescription(tree) + .setMessage(message) + .addFix(equalityFix(tree, receiver, argument, state).orElse(emptyFix())) + .build()) + .orElse(NO_MATCH); + } + + private Description checkContainment(ExpressionTree tree, VisitorState state) { + MatchResult result = ContainmentMatchers.firstNonNullMatchResult(tree, state); + if (result == null || CONTAINERS_NOT_USING_EQUALS.matches(tree, state)) { + return NO_MATCH; + } + return findProblem(result.sourceType(), Usage.CONTAINMENT, state) + .or(() -> findProblem(result.targetType(), Usage.CONTAINMENT, state)) + .map(message -> buildDescription(tree).setMessage(message).build()) + .orElse(NO_MATCH); + } + + private Description checkHashKeys(ExpressionTree tree, VisitorState state) { + for (Type keyType : hashKeyTypes(getType(tree), state)) { + Optional message = findProblem(keyType, Usage.HASH_KEY, state); + if (message.isPresent()) { + return buildDescription(tree).setMessage(message.get()).build(); + } + } + return NO_MATCH; + } + + private Optional findProblem(@Nullable Type type, Usage usage, VisitorState state) { + if (type == null || type.hasTag(TypeTag.BOT)) { + return Optional.empty(); + } + return problem(type, usage, state); + } + + /// Returns the types whose `equals` and `hashCode` are used by a container of the given type, or + /// an empty list if the type isn't a hash-based container. + private static ImmutableList hashKeyTypes(@Nullable Type type, VisitorState state) { + ImmutableList.Builder result = ImmutableList.builder(); + if (isSubtype(type, MAP.get(state), state)) { + if (!isSubtype(type, SORTED_MAP.get(state), state) + && !isSubtype(type, IDENTITY_HASH_MAP.get(state), state)) { + addTypeArgument(result, type, MAP.get(state), 0, state); + // If `type` does not inherit from `BiMap`, this does nothing. + addTypeArgument(result, type, BIMAP.get(state), 1, state); + } + } else if (isSubtype(type, SET.get(state), state)) { + if (!isSubtype(type, SORTED_SET.get(state), state)) { + addTypeArgument(result, type, SET.get(state), 0, state); + } + } else if (isSubtype(type, MULTISET.get(state), state)) { + if (!isSubtype(type, SORTED_MULTISET.get(state), state)) { + addTypeArgument(result, type, MULTISET.get(state), 0, state); + } + } else if (isSubtype(type, MULTIMAP.get(state), state)) { + addTypeArgument(result, type, MULTIMAP.get(state), 0, state); + } + return result.build(); + } + + private static void addTypeArgument( + ImmutableList.Builder result, + Type type, + @Nullable Type superType, + int index, + VisitorState state) { + if (superType == null) { + return; + } + Type asSuper = state.getTypes().asSuper(type, superType.tsym); + if (asSuper == null || asSuper.getTypeArguments().size() <= index) { + return; + } + result.add(asSuper.getTypeArguments().get(index)); + } + + private static final Matcher ASSERT_THAT_EQUALS = + allOf( + instanceMethod() + .onDescendantOf("com.google.common.truth.Subject") + .namedAnyOf("isEqualTo", "isNotEqualTo"), + receiverOfInvocation( + anyOf( + staticMethod().onClass("com.google.common.truth.Truth").named("assertThat"), + instanceMethod() + .onDescendantOf("com.google.common.truth.StandardSubjectBuilder") + .named("that")))); + + // NOTE: this is a bit crude; methods like `containsValue` are still likely to be subject to + // equality constraints on the value type. + private static final Matcher CONTAINERS_NOT_USING_EQUALS = + anyMethod() + .onDescendantOfAny( + "java.util.IdentityHashMap", "java.util.SortedMap", "java.util.SortedSet"); + + private static final Matcher HASH_CONTAINER_FACTORIES = + anyOf( + staticMethod() + .onClassAny( + "com.google.common.collect.ImmutableSet", + "com.google.common.collect.ImmutableMap", + "com.google.common.collect.HashBiMap"), + instanceMethod() + .onExactClassAny( + "com.google.common.collect.ImmutableSet.Builder", + "com.google.common.collect.ImmutableMap.Builder") + .named("build"), + staticMethod().onClass("com.google.common.collect.Sets").named("newHashSet"), + staticMethod() + .onClass("com.google.common.collect.Maps") + .namedAnyOf("newHashMap", "newLinkedHashMap"), + staticMethod() + .onClassAny( + "com.google.common.collect.HashMultiset", + "com.google.common.collect.LinkedHashMultiset", + "com.google.common.collect.HashMultimap", + "com.google.common.collect.LinkedHashMultimap", + "com.google.common.collect.ArrayListMultimap", + "com.google.common.collect.LinkedListMultimap") + .named("create"), + instanceMethod().onDescendantOf("java.util.stream.Stream").named("collect")); + + private static final Supplier SET = typeFromString("java.util.Set"); + private static final Supplier SORTED_SET = typeFromString("java.util.SortedSet"); + private static final Supplier MAP = typeFromString("java.util.Map"); + private static final Supplier SORTED_MAP = typeFromString("java.util.SortedMap"); + private static final Supplier IDENTITY_HASH_MAP = + typeFromString("java.util.IdentityHashMap"); + private static final Supplier BIMAP = typeFromString("com.google.common.collect.BiMap"); + private static final Supplier MULTIMAP = + typeFromString("com.google.common.collect.Multimap"); + private static final Supplier MULTISET = + typeFromString("com.google.common.collect.Multiset"); + private static final Supplier SORTED_MULTISET = + typeFromString("com.google.common.collect.SortedMultiset"); +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ArrayAsKeyOfSetOrMap.java b/core/src/main/java/com/google/errorprone/bugpatterns/ArrayAsKeyOfSetOrMap.java deleted file mode 100644 index 0cc616884c8..00000000000 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ArrayAsKeyOfSetOrMap.java +++ /dev/null @@ -1,44 +0,0 @@ -/* - * 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.errorprone.BugPattern.SeverityLevel.WARNING; - -import com.google.errorprone.BugPattern; -import com.google.errorprone.VisitorState; -import com.sun.tools.javac.code.Type; - -/** - * Warns that users should not have an array as a key to a Set or Map - * - * @author siyuanl@google.com (Siyuan Liu) - * @author eleanorh@google.com (Eleanor Harris) - */ -@BugPattern( - summary = - "Arrays do not override equals() or hashCode, so comparisons will be done on" - + " reference equality only. If neither deduplication nor lookup are needed, " - + "consider using a List instead. Otherwise, use IdentityHashMap/Set, " - + "a Map from a library that handles object arrays, or an Iterable/List of pairs.", - severity = WARNING) -public class ArrayAsKeyOfSetOrMap extends AbstractAsKeyOfSetOrMap { - - @Override - protected boolean isBadType(Type type, VisitorState state) { - return type instanceof Type.ArrayType; - } -} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UndefinedEquals.java b/core/src/main/java/com/google/errorprone/bugpatterns/UndefinedEquals.java index 353a4d5cb4e..854fd2bafec 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UndefinedEquals.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UndefinedEquals.java @@ -16,38 +16,23 @@ package com.google.errorprone.bugpatterns; -import static com.google.common.collect.Iterables.getLast; -import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; -import static com.google.errorprone.fixes.SuggestedFix.emptyFix; -import static com.google.errorprone.matchers.Matchers.allOf; -import static com.google.errorprone.matchers.Matchers.anyOf; -import static com.google.errorprone.matchers.Matchers.assertEqualsInvocation; -import static com.google.errorprone.matchers.Matchers.assertNotEqualsInvocation; -import static com.google.errorprone.matchers.Matchers.instanceEqualsInvocation; -import static com.google.errorprone.matchers.Matchers.receiverOfInvocation; -import static com.google.errorprone.matchers.Matchers.staticEqualsInvocation; -import static com.google.errorprone.matchers.Matchers.staticMethod; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; import static com.google.errorprone.suppliers.Suppliers.typeFromString; -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 static com.google.errorprone.util.ASTHelpers.isSubtype; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; -import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.suppliers.Supplier; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Type; -import java.util.Arrays; -import java.util.List; +import com.sun.tools.javac.code.TypeTag; import java.util.Optional; import java.util.function.BiFunction; @@ -57,61 +42,40 @@ * @author eleanorh@google.com (Eleanor Harris) */ @BugPattern( + altNames = {"CollectionUndefinedEquality", "ArrayAsKeyOfSetOrMap"}, summary = "This type is not guaranteed to implement a useful equals() method.", severity = WARNING) -public final class UndefinedEquals extends BugChecker implements MethodInvocationTreeMatcher { +public final class UndefinedEquals extends AbstractEqualsHashCodeUsageChecker { + + private static final String ARRAY_MESSAGE = + """ + Arrays do not override equals() or hashCode, so comparisons will be done on reference \ + equality only. If neither deduplication nor lookup are needed, consider using a List \ + instead. Otherwise, use IdentityHashMap/Set, a Map from a library that handles \ + object arrays, or an Iterable/List of pairs.\ + """; private static final Matcher IS_EQUAL_TO = instanceMethod().onDescendantOf("com.google.common.truth.Subject").named("isEqualTo"); - private static final Matcher ASSERT_THAT_EQUALS = - allOf( - instanceMethod() - .onDescendantOf("com.google.common.truth.Subject") - .namedAnyOf("isEqualTo", "isNotEqualTo"), - receiverOfInvocation( - anyOf( - staticMethod().onClass("com.google.common.truth.Truth").named("assertThat"), - instanceMethod() - .onDescendantOf("com.google.common.truth.StandardSubjectBuilder") - .named("that")))); - @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - Tree receiver; - Tree argument; - List arguments = tree.getArguments(); - - if (staticEqualsInvocation().matches(tree, state) - || assertEqualsInvocation().matches(tree, state) - || assertNotEqualsInvocation().matches(tree, state)) { - receiver = arguments.get(arguments.size() - 2); - argument = getLast(arguments); - } else if (instanceEqualsInvocation().matches(tree, state)) { - receiver = getReceiver(tree); - argument = arguments.getFirst(); - } else if (ASSERT_THAT_EQUALS.matches(tree, state)) { - receiver = getOnlyElement(arguments); - argument = getOnlyElement(((MethodInvocationTree) getReceiver(tree)).getArguments()); - } else { - return Description.NO_MATCH; + protected Optional problem(Type type, Usage usage, VisitorState state) { + if (type.hasTag(TypeTag.ARRAY)) { + // Direct comparisons of arrays are handled by ArrayEquals, and Truth and JUnit assertions + // have special handling for arrays. + return usage == Usage.HASH_KEY ? Optional.of(ARRAY_MESSAGE) : Optional.empty(); } - - return Arrays.stream(TypesWithUndefinedEquality.values()) - .filter( - b -> b.matchesType(getType(receiver), state) || b.matchesType(getType(argument), state)) - .findFirst() - .map( - b -> - buildDescription(tree) - .setMessage(b.shortName() + " does not have well-defined equality semantics") - .addFix(generateFix(tree, state, receiver, argument).orElse(emptyFix())) - .build()) - .orElse(Description.NO_MATCH); + for (TypesWithUndefinedEquality badType : TypesWithUndefinedEquality.values()) { + if (badType.matchesType(type, state)) { + return Optional.of(badType.shortName() + " does not have well-defined equality semantics"); + } + } + return Optional.empty(); } - private static Optional generateFix( - MethodInvocationTree tree, VisitorState state, Tree receiver, Tree argument) { + @Override + protected Optional equalityFix( + MethodInvocationTree tree, Tree receiver, Tree argument, VisitorState state) { // Generate fix for certain Truth `isEqualTo` calls if (IS_EQUAL_TO.matches(tree, state)) { String methodText = diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionUndefinedEquality.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionUndefinedEquality.java deleted file mode 100644 index e865b4d5938..00000000000 --- a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionUndefinedEquality.java +++ /dev/null @@ -1,90 +0,0 @@ -/* - * Copyright 2020 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.collectionincompatibletype; - -import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; -import static com.google.errorprone.matchers.Description.NO_MATCH; -import static com.google.errorprone.matchers.Matchers.anyMethod; - -import com.google.errorprone.BugPattern; -import com.google.errorprone.BugPattern.StandardTags; -import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.bugpatterns.BugChecker.MemberReferenceTreeMatcher; -import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; -import com.google.errorprone.bugpatterns.TypesWithUndefinedEquality; -import com.google.errorprone.bugpatterns.collectionincompatibletype.AbstractCollectionIncompatibleTypeMatcher.MatchResult; -import com.google.errorprone.matchers.Description; -import com.google.errorprone.matchers.Matcher; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MemberReferenceTree; -import com.sun.source.tree.MethodInvocationTree; -import java.util.Arrays; - -/** - * Highlights use of {@code Collection#contains} (and others) with types that do not have - * well-defined equals. - */ -@BugPattern( - summary = "This type does not have well-defined equals behavior.", - tags = StandardTags.FRAGILE_CODE, - severity = WARNING) -public final class CollectionUndefinedEquality extends BugChecker - implements MethodInvocationTreeMatcher, MemberReferenceTreeMatcher { - // NOTE: this is a bit crude; methods like `containsValue` are still likely to be subject to - // equality constraints on the value type. - private static final Matcher TYPES_NOT_DEPENDING_ON_OBJECT_EQUALITY = - anyMethod() - .onDescendantOfAny( - "java.util.IdentityHashMap", - "java.util.IdentityHashSet", - "java.util.SortedMap", - "java.util.SortedSet"); - - @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - return match(tree, state); - } - - @Override - public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) { - return match(tree, state); - } - - public Description match(ExpressionTree tree, VisitorState state) { - MatchResult result = ContainmentMatchers.firstNonNullMatchResult(tree, state); - if (result == null) { - return NO_MATCH; - } - if (TYPES_NOT_DEPENDING_ON_OBJECT_EQUALITY.matches(tree, state)) { - return NO_MATCH; - } - - return Arrays.stream(TypesWithUndefinedEquality.values()) - .filter( - b -> - b.matchesType(result.sourceType(), state) - || b.matchesType(result.targetType(), state)) - .findFirst() - .map( - b -> - buildDescription(tree) - .setMessage(b.shortName() + " does not have well-defined equals behavior.") - .build()) - .orElse(NO_MATCH); - } -} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index fa4b2b67813..d65f8e2e02b 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -29,7 +29,6 @@ import com.google.errorprone.bugpatterns.AnnotationMirrorToString; import com.google.errorprone.bugpatterns.AnnotationPosition; import com.google.errorprone.bugpatterns.AnnotationValueToString; -import com.google.errorprone.bugpatterns.ArrayAsKeyOfSetOrMap; import com.google.errorprone.bugpatterns.ArrayEquals; import com.google.errorprone.bugpatterns.ArrayFillIncompatibleType; import com.google.errorprone.bugpatterns.ArrayHashCode; @@ -515,7 +514,6 @@ import com.google.errorprone.bugpatterns.checkreturnvalue.UsingJsr305CheckReturnValue; import com.google.errorprone.bugpatterns.collectionincompatibletype.AssertSameIncompatible; import com.google.errorprone.bugpatterns.collectionincompatibletype.CollectionIncompatibleType; -import com.google.errorprone.bugpatterns.collectionincompatibletype.CollectionUndefinedEquality; import com.google.errorprone.bugpatterns.collectionincompatibletype.CompatibleWithMisuse; import com.google.errorprone.bugpatterns.collectionincompatibletype.IncompatibleArgumentType; import com.google.errorprone.bugpatterns.collectionincompatibletype.JUnitIncompatibleType; @@ -926,7 +924,6 @@ public static ScannerSupplier warningChecks() { AmbiguousMethodReference.class, AnnotateFormatMethod.class, ArgumentSelectionDefectChecker.class, - ArrayAsKeyOfSetOrMap.class, ArrayRecordComponent.class, AssertEqualsArgumentOrderChecker.class, AssertSameIncompatible.class, @@ -965,7 +962,6 @@ public static ScannerSupplier warningChecks() { ClassNewInstance.class, CloseableProvides.class, ClosingStandardOutputStreams.class, - CollectionUndefinedEquality.class, CollectorShouldNotUseState.class, ComparableAndComparator.class, ComplexBooleanConstant.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ArrayAsKeyOfSetOrMapTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ArrayAsKeyOfSetOrMapTest.java deleted file mode 100644 index d9df8827770..00000000000 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ArrayAsKeyOfSetOrMapTest.java +++ /dev/null @@ -1,154 +0,0 @@ -/* - * 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; - -/** - * {@link ArrayAsKeyOfSetOrMap}Test - * - * @author siyuanl@google.com (Siyuan Liu) - * @author eleanorh@google.com (Eleanor Harris) / - *

/** {@link ArrayAsKeyOfSetOrMap}Test - */ -@RunWith(JUnit4.class) -public final class ArrayAsKeyOfSetOrMapTest { - - private final CompilationTestHelper compilationHelper = - CompilationTestHelper.newInstance(ArrayAsKeyOfSetOrMap.class, getClass()); - - @Test - public void positive() { - compilationHelper - .addSourceLines( - "Test.java", -""" -import com.google.common.collect.ArrayListMultimap; -import com.google.common.collect.HashBiMap; -import com.google.common.collect.HashMultimap; -import com.google.common.collect.HashMultiset; -import com.google.common.collect.LinkedHashMultimap; -import com.google.common.collect.LinkedHashMultiset; -import com.google.common.collect.LinkedListMultimap; -import com.google.common.collect.Maps; -import com.google.common.collect.Sets; -import java.util.HashMap; -import java.util.HashSet; -import java.util.LinkedHashMap; -import java.util.Map; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; - -class Test { - public static void main(String[] args) { - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - Map testNewMap = Maps.newHashMap(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - Set testNewSet = Sets.newHashSet(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - HashMap testNewHashMap = Maps.newHashMap(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - HashSet testNewHashSet = Sets.newHashSet(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - Map testMap = new HashMap(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - Set testSet = new HashSet(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - HashMap testHashMap = new HashMap(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - HashSet testHashSet = new HashSet(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - HashMultimap testHashMultimap = HashMultimap.create(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - ArrayListMultimap testArrayListMultimap = ArrayListMultimap.create(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - LinkedHashMultimap testLinkedHashMultimap = LinkedHashMultimap.create(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - LinkedListMultimap testLinkedListMultimap = LinkedListMultimap.create(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - HashBiMap testHashBiMap = HashBiMap.create(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - LinkedHashMap testLinkedHashMap = new LinkedHashMap(); - ConcurrentHashMap testConcurrentHashMap = - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - new ConcurrentHashMap(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - HashMultiset testHashMultiSet = HashMultiset.create(); - // BUG: Diagnostic contains: ArrayAsKeyOfSetOrMap - LinkedHashMultiset testLinkedHashMultiSet = LinkedHashMultiset.create(); - } -} -""") - .doTest(); - } - - @Test - public void negative() { - compilationHelper - .addSourceLines( - "Test.java", -""" -import com.google.common.collect.ArrayListMultimap; -import com.google.common.collect.HashBiMap; -import com.google.common.collect.HashMultimap; -import com.google.common.collect.HashMultiset; -import com.google.common.collect.LinkedHashMultimap; -import com.google.common.collect.LinkedHashMultiset; -import com.google.common.collect.LinkedListMultimap; -import com.google.common.collect.Maps; -import com.google.common.collect.Ordering; -import com.google.common.collect.Sets; -import java.util.Arrays; -import java.util.HashMap; -import java.util.HashSet; -import java.util.LinkedHashMap; -import java.util.Map; -import java.util.Set; -import java.util.TreeSet; -import java.util.concurrent.ConcurrentHashMap; - -class Test { - public static void main(String[] args) { - Map testMap = new HashMap(); - Set testSet = new HashSet(); - HashMap testHashMap = new HashMap(); - HashSet testHashSet = new HashSet(); - Set testSet2 = new HashSet(); - Map testMap2 = new HashMap(); - Map mapFromMethod = Maps.newHashMap(); - Set setFromMethod = Sets.newHashSet(); - Set thisShouldWork = - new TreeSet(Ordering.natural().lexicographical().onResultOf(Arrays::asList)); - HashMultimap testHashMultimap = HashMultimap.create(); - ArrayListMultimap testArrayListMultimap = ArrayListMultimap.create(); - LinkedHashMultimap testLinkedHashMultimap = LinkedHashMultimap.create(); - LinkedListMultimap testLinkedListMultimap = LinkedListMultimap.create(); - HashBiMap testHashBiMap = HashBiMap.create(); - LinkedHashMap testLinkedHashMap = new LinkedHashMap(); - ConcurrentHashMap testConcurrentHashMap = - new ConcurrentHashMap(); - HashMultiset testHashMultiSet = HashMultiset.create(); - LinkedHashMultiset testLinkedHashMultiSet = LinkedHashMultiset.create(); - } -} -""") - .doTest(); - } -} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UndefinedEqualsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UndefinedEqualsTest.java index 8146dfc72c5..07434e9fa8d 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UndefinedEqualsTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UndefinedEqualsTest.java @@ -365,8 +365,6 @@ public void truthFixAssertWithMessage() { """ import static com.google.common.truth.Truth.assertWithMessage; - import java.lang.Iterable; - class Test { void f(Iterable a, Iterable b) { assertWithMessage("message").that(a).isEqualTo(b); @@ -378,8 +376,6 @@ void f(Iterable a, Iterable b) { """ import static com.google.common.truth.Truth.assertWithMessage; - import java.lang.Iterable; - class Test { void f(Iterable a, Iterable b) { assertWithMessage("message").that(a).containsExactlyElementsIn(b); @@ -397,8 +393,6 @@ public void truthFixDontRewriteIsNotEqualTo() { """ import static com.google.common.truth.Truth.assertThat; - import java.lang.Iterable; - class Test { void f(Iterable a, Iterable b) { assertThat(a).isNotEqualTo(b); @@ -410,8 +404,6 @@ void f(Iterable a, Iterable b) { """ import static com.google.common.truth.Truth.assertThat; - import java.lang.Iterable; - class Test { void f(Iterable a, Iterable b) { assertThat(a).isNotEqualTo(b); @@ -439,8 +431,6 @@ public void truthFixAcrossMultipleLinesAndPoorlyFormatted() { """ import static com.google.common.truth.Truth.assertThat; - import java.lang.Iterable; - class Test { void f(Iterable a, Iterable b) { assertThat(a).containsExactlyElementsIn(b); @@ -557,4 +547,353 @@ void future(Future a, Future b) { """) .doTest(); } + + @Test + public void collectionOfCollections() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.Collection; + + class Test { + boolean foo(Collection> xs, Collection x) { + // BUG: Diagnostic contains: Collection does not have well-defined equality + return xs.contains(x); + } + } + """) + .doTest(); + } + + @Test + public void collectionOfLists_noFinding() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.Collection; + import java.util.List; + + class Test { + boolean foo(Collection> xs, List x) { + return xs.contains(x); + } + } + """) + .doTest(); + } + + @Test + public void treeMap_noFinding() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.Collection; + import java.util.TreeMap; + + class Test { + boolean foo(TreeMap, Integer> xs, Collection x) { + return xs.containsKey(x); + } + } + """) + .doTest(); + } + + @Test + public void sortedMap_noFinding() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.Collection; + import java.util.SortedMap; + + class Test { + boolean foo(SortedMap, Integer> xs, Collection x) { + return xs.containsKey(x); + } + } + """) + .doTest(); + } + + @Test + public void sortedSet_noFinding() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.Collection; + import java.util.SortedSet; + + class Test { + boolean foo(SortedSet> xs, Collection x) { + return xs.contains(x); + } + } + """) + .doTest(); + } + + @Test + public void arrayAsKeyPositive() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.common.collect.ArrayListMultimap; + import com.google.common.collect.HashBiMap; + import com.google.common.collect.HashMultimap; + import com.google.common.collect.HashMultiset; + import com.google.common.collect.LinkedHashMultimap; + import com.google.common.collect.LinkedHashMultiset; + import com.google.common.collect.LinkedListMultimap; + import com.google.common.collect.Maps; + import com.google.common.collect.Sets; + import java.util.HashMap; + import java.util.HashSet; + import java.util.LinkedHashMap; + import java.util.Map; + import java.util.Set; + import java.util.concurrent.ConcurrentHashMap; + + class Test { + public static void main(String[] args) { + // BUG: Diagnostic contains: Arrays do not override equals() + Map testNewMap = Maps.newHashMap(); + // BUG: Diagnostic contains: Arrays do not override equals() + Set testNewSet = Sets.newHashSet(); + // BUG: Diagnostic contains: Arrays do not override equals() + HashMap testNewHashMap = Maps.newHashMap(); + // BUG: Diagnostic contains: Arrays do not override equals() + HashSet testNewHashSet = Sets.newHashSet(); + // BUG: Diagnostic contains: Arrays do not override equals() + Map testMap = new HashMap(); + // BUG: Diagnostic contains: Arrays do not override equals() + Set testSet = new HashSet(); + // BUG: Diagnostic contains: Arrays do not override equals() + HashMap testHashMap = new HashMap(); + // BUG: Diagnostic contains: Arrays do not override equals() + HashSet testHashSet = new HashSet(); + // BUG: Diagnostic contains: Arrays do not override equals() + HashMultimap testHashMultimap = HashMultimap.create(); + ArrayListMultimap testArrayListMultimap = + // BUG: Diagnostic contains: Arrays do not override equals() + ArrayListMultimap.create(); + LinkedHashMultimap testLinkedHashMultimap = + // BUG: Diagnostic contains: Arrays do not override equals() + LinkedHashMultimap.create(); + LinkedListMultimap testLinkedListMultimap = + // BUG: Diagnostic contains: Arrays do not override equals() + LinkedListMultimap.create(); + // BUG: Diagnostic contains: Arrays do not override equals() + HashBiMap testHashBiMap = HashBiMap.create(); + LinkedHashMap testLinkedHashMap = + // BUG: Diagnostic contains: Arrays do not override equals() + new LinkedHashMap(); + ConcurrentHashMap testConcurrentHashMap = + // BUG: Diagnostic contains: Arrays do not override equals() + new ConcurrentHashMap(); + // BUG: Diagnostic contains: Arrays do not override equals() + HashMultiset testHashMultiSet = HashMultiset.create(); + // BUG: Diagnostic contains: Arrays do not override equals() + LinkedHashMultiset testLinkedHashMultiSet = LinkedHashMultiset.create(); + } + } + """) + .doTest(); + } + + @Test + public void arrayAsKeyNegative() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + + import com.google.common.collect.ArrayListMultimap; + import com.google.common.collect.HashBiMap; + import com.google.common.collect.HashMultimap; + import com.google.common.collect.HashMultiset; + import com.google.common.collect.LinkedHashMultimap; + import com.google.common.collect.LinkedHashMultiset; + import com.google.common.collect.LinkedListMultimap; + import com.google.common.collect.Maps; + import com.google.common.collect.Ordering; + import com.google.common.collect.Sets; + import java.util.Arrays; + import java.util.HashMap; + import java.util.HashSet; + import java.util.LinkedHashMap; + import java.util.Map; + import java.util.Set; + import java.util.TreeSet; + import java.util.concurrent.ConcurrentHashMap; + + class Test { + public static void main(String[] args) { + Map testMap = new HashMap(); + Set testSet = new HashSet(); + HashMap testHashMap = new HashMap(); + HashSet testHashSet = new HashSet(); + Set testSet2 = new HashSet(); + Map testMap2 = new HashMap(); + Map mapFromMethod = Maps.newHashMap(); + Set setFromMethod = Sets.newHashSet(); + Ordering natural = Ordering.natural(); + Set thisShouldWork = + new TreeSet(natural.lexicographical().onResultOf(Arrays::asList)); + HashMultimap testHashMultimap = HashMultimap.create(); + ArrayListMultimap arrayListMultimap = ArrayListMultimap.create(); + LinkedHashMultimap linkedMultimap = LinkedHashMultimap.create(); + LinkedListMultimap listMultimap = LinkedListMultimap.create(); + HashBiMap testHashBiMap = HashBiMap.create(); + LinkedHashMap linkedHashMap = new LinkedHashMap(); + ConcurrentHashMap testConcurrentHashMap = + new ConcurrentHashMap(); + HashMultiset testHashMultiSet = HashMultiset.create(); + LinkedHashMultiset testLinkedHashMultiSet = LinkedHashMultiset.create(); + assertThat(args).isEqualTo(args); + } + } + """) + .doTest(); + } + + @Test + public void collectors() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import static com.google.common.collect.ImmutableMap.toImmutableMap; + import static com.google.common.collect.ImmutableSet.toImmutableSet; + import static java.util.stream.Collectors.groupingBy; + import static java.util.stream.Collectors.toList; + import static java.util.stream.Collectors.toMap; + import static java.util.stream.Collectors.toSet; + + import com.google.common.collect.ImmutableMap; + import com.google.common.collect.ImmutableSet; + import java.util.HashMap; + import java.util.List; + import java.util.Map; + import java.util.Set; + import java.util.TreeMap; + import java.util.stream.Stream; + + class Test { + void f(Stream stream) { + // BUG: Diagnostic contains: Arrays do not override equals() + Set s1 = stream.collect(toSet()); + // BUG: Diagnostic contains: Arrays do not override equals() + ImmutableSet s2 = stream.collect(toImmutableSet()); + // BUG: Diagnostic contains: Arrays do not override equals() + Map m1 = stream.collect(toMap(x -> x, x -> "")); + ImmutableMap m2 = + // BUG: Diagnostic contains: Arrays do not override equals() + stream.collect(toImmutableMap(x -> x, x -> "")); + // BUG: Diagnostic contains: Arrays do not override equals() + Map> m3 = stream.collect(groupingBy(x -> x)); + Map> m4 = + // BUG: Diagnostic contains: Arrays do not override equals() + stream.collect(groupingBy(x -> x, HashMap::new, toList())); + Map> sorted = + stream.collect(groupingBy(x -> x, () -> new TreeMap<>(), toList())); + List list = stream.collect(toList()); + } + } + """) + .doTest(); + } + + @Test + public void hashKeys() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.common.collect.HashMultimap; + import com.google.common.collect.ImmutableSet; + import com.google.common.collect.Multimap; + import java.util.Collection; + import java.util.HashMap; + import java.util.HashSet; + import java.util.IdentityHashMap; + import java.util.List; + import java.util.Map; + import java.util.Set; + import java.util.TreeMap; + + class Test { + void f(CharSequence cs) { + // BUG: Diagnostic contains: CharSequence does not have well-defined equality + Set s = new HashSet<>(); + // BUG: Diagnostic contains: Collection does not have well-defined equality + Map, String> m = new HashMap<>(); + // BUG: Diagnostic contains: CharSequence does not have well-defined equality + ImmutableSet i = ImmutableSet.of(cs); + // BUG: Diagnostic contains: Iterable does not have well-defined equality + Multimap, String> mm = HashMultimap.create(); + Map, String> identity = new IdentityHashMap<>(); + Map, String> sorted = new TreeMap<>(); + Map> values = new HashMap<>(); + Set> lists = new HashSet<>(); + } + } + """) + .doTest(); + } + + @Test + public void hashKeys_arraysSkipIdentityAndSortedContainers() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.IdentityHashMap; + import java.util.Map; + import java.util.TreeMap; + import java.util.WeakHashMap; + + class Test { + void f() { + Map identity = new IdentityHashMap<>(); + Map sorted = new TreeMap<>((a, b) -> 0); + // BUG: Diagnostic contains: Arrays do not override equals() + Map weak = new WeakHashMap<>(); + } + } + """) + .doTest(); + } + + @Test + public void suppressionWithOldNames() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.Collection; + import java.util.HashSet; + import java.util.Set; + + class Test { + @SuppressWarnings("ArrayAsKeyOfSetOrMap") + void f() { + Set s = new HashSet<>(); + } + + @SuppressWarnings("CollectionUndefinedEquality") + boolean g(Collection> xs, Collection x) { + return xs.contains(x); + } + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionUndefinedEqualityTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionUndefinedEqualityTest.java deleted file mode 100644 index 0776aff3619..00000000000 --- a/core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionUndefinedEqualityTest.java +++ /dev/null @@ -1,119 +0,0 @@ -/* - * Copyright 2020 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.collectionincompatibletype; - -import com.google.errorprone.CompilationTestHelper; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests for {@link CollectionUndefinedEquality}. */ -@RunWith(JUnit4.class) -public final class CollectionUndefinedEqualityTest { - private final CompilationTestHelper helper = - CompilationTestHelper.newInstance(CollectionUndefinedEquality.class, getClass()); - - @Test - public void collectionOfCollections() { - helper - .addSourceLines( - "Test.java", - """ - import java.util.Collection; - - class Test { - boolean foo(Collection> xs, Collection x) { - // BUG: Diagnostic contains: - return xs.contains(x); - } - } - """) - .doTest(); - } - - @Test - public void collectionOfLists_noFinding() { - helper - .addSourceLines( - "Test.java", - """ - import java.util.Collection; - import java.util.List; - - class Test { - boolean foo(Collection> xs, List x) { - return xs.contains(x); - } - } - """) - .doTest(); - } - - @Test - public void treeMap_noFinding() { - helper - .addSourceLines( - "Test.java", - """ - import java.util.Collection; - import java.util.TreeMap; - - class Test { - boolean foo(TreeMap, Integer> xs, Collection x) { - return xs.containsKey(x); - } - } - """) - .doTest(); - } - - @Test - public void sortedMap_noFinding() { - helper - .addSourceLines( - "Test.java", - """ - import java.util.Collection; - import java.util.SortedMap; - - class Test { - boolean foo(SortedMap, Integer> xs, Collection x) { - return xs.containsKey(x); - } - } - """) - .doTest(); - } - - @Test - public void sortedSet_noFinding() { - helper - .addSourceLines( - "Test.java", - """ - import java.util.Collection; - import java.util.SortedSet; - - class Test { - boolean foo(SortedSet> xs, Collection x) { - return xs.contains(x); - } - } - """) - .doTest(); - } -} diff --git a/docs/bugpattern/CollectionUndefinedEquality.md b/docs/bugpattern/CollectionUndefinedEquality.md deleted file mode 100644 index b1f3e358bba..00000000000 --- a/docs/bugpattern/CollectionUndefinedEquality.md +++ /dev/null @@ -1,27 +0,0 @@ -Using `Collection`s (and other types which rely on equality) to contain elements -with undefined equality is error-prone. For example, `Collection` itself does -not have well-defined equality: the `List` and `Set` subinterfaces are not -necessarily comparable. - -```java -ImmutableList> collectionsOfIntegers = - ImmutableList.of(ImmutableSet.of(1, 2), ImmutableSet.of(3, 4)); - -collectionsOfIntegers.contains(ImmutableSet.of(1, 2)); // true -collectionsOfIntegers.contains(ImmutableList.of(1, 2)); // false -``` - -```java -boolean containsTest(Collection> charSequences) { - // True if `charSequences` actually contains Strings, but otherwise not necessarily. - return charSequences.contains("test"); -} -``` - -In this case, an appropriate fix may be, - -```java -boolean containsTest(Collection> charSequences) { - return charSequences.stream().anyMatch("test"::contentEquals); -} -``` diff --git a/docs/bugpattern/UndefinedEquals.md b/docs/bugpattern/UndefinedEquals.md index a3aae3a0f2a..958668642c2 100644 --- a/docs/bugpattern/UndefinedEquals.md +++ b/docs/bugpattern/UndefinedEquals.md @@ -68,6 +68,30 @@ Prefer calling `#get` and checking the result, or using `assertThat(...).isSameInstanceAs()` or `assertThat(...).isNotSameInstanceAs()` for cases where the same/different instance is expected. +## In collections + +Methods like `Collection#contains` and `Map#get`, as well as hash-based +containers like `HashSet`, `HashMap`, `ImmutableSet`, and `ImmutableMap`, rely +on `equals` (and `hashCode`), so they are subject to the same problems when the +element or key type does not have well-defined equality. + +```java +ImmutableList> collectionsOfIntegers = + ImmutableList.of(ImmutableSet.of(1, 2), ImmutableSet.of(3, 4)); + +collectionsOfIntegers.contains(ImmutableSet.of(1, 2)); // true +collectionsOfIntegers.contains(ImmutableList.of(1, 2)); // false +``` + +## For arrays + +Arrays do not override `equals` or `hashCode`, so using them as elements of a +hash-based `Set`, or keys of a hash-based `Map`, `Multimap`, or `Multiset`, +compares them by reference rather than by their contents. If neither +deduplication nor lookup are needed, consider using a `List` instead. Otherwise, +use `IdentityHashMap`/`Set`, a `Map` from a library that handles object arrays, +or an `Iterable`/`List` of pairs. + [`Collection`]: https://docs.oracle.com/javase/8/docs/api/java/util/Collection.html [`Future`]: https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/Future.html [`Iterable`]: https://docs.oracle.com/javase/8/docs/api/java/lang/Iterable.html