From 80936984d2a173d5ac02881ebe2dbc5aeb7aa467 Mon Sep 17 00:00:00 2001 From: Kurt Alfred Kluever Date: Wed, 20 Nov 2024 05:49:44 -0800 Subject: [PATCH] Warn on `futureA.equals(futureB)` (and popular subtypes). Futures do not have well-defined equality semantics. PiperOrigin-RevId: 698360881 --- .../TypesWithUndefinedEquality.java | 7 ++++ .../bugpatterns/UndefinedEquals.java | 41 +++++++------------ .../bugpatterns/UndefinedEqualsTest.java | 38 ++++++++++++++++- 3 files changed, 58 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TypesWithUndefinedEquality.java b/core/src/main/java/com/google/errorprone/bugpatterns/TypesWithUndefinedEquality.java index 1785dbb849e..8ca1ba87995 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TypesWithUndefinedEquality.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TypesWithUndefinedEquality.java @@ -28,6 +28,12 @@ public enum TypesWithUndefinedEquality { CHAR_SEQUENCE("CharSequence", "java.lang.CharSequence"), COLLECTION("Collection", "java.util.Collection"), DATE("Date", "java.util.Date"), + FUTURE( + "Future", + "java.util.concurrent.Future", + "java.util.concurrent.CompletableFuture", + "com.google.common.util.concurrent.ListenableFuture", + "com.google.common.util.concurrent.SettableFuture"), IMMUTABLE_COLLECTION("ImmutableCollection", "com.google.common.collect.ImmutableCollection"), IMMUTABLE_MULTIMAP("ImmutableMultimap", "com.google.common.collect.ImmutableMultimap"), ITERABLE("Iterable", "com.google.common.collect.FluentIterable", "java.lang.Iterable"), @@ -45,6 +51,7 @@ public enum TypesWithUndefinedEquality { private final String shortName; private final ImmutableSet typeNames; + // TODO: kak - should we add a bit which determines if we should check isSameType or isSubtype? TypesWithUndefinedEquality(String shortName, String... typeNames) { this.shortName = shortName; this.typeNames = ImmutableSet.copyOf(typeNames); 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 7e6afc1d022..73a93b82813 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UndefinedEquals.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UndefinedEquals.java @@ -19,6 +19,7 @@ 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; @@ -55,7 +56,7 @@ * @author eleanorh@google.com (Eleanor Harris) */ @BugPattern( - summary = "This type is not guaranteed to implement a useful #equals method.", + summary = "This type is not guaranteed to implement a useful equals() method.", severity = WARNING) public final class UndefinedEquals extends BugChecker implements MethodInvocationTreeMatcher { @@ -102,13 +103,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState .map( b -> buildDescription(tree) - .setMessage( - "Subtypes of " - + b.shortName() - + " are not guaranteed to implement a useful #equals method.") - .addFix( - generateFix(tree, state, receiver, argument) - .orElse(SuggestedFix.emptyFix())) + .setMessage(b.shortName() + " does not have well-defined equality semantics") + .addFix(generateFix(tree, state, receiver, argument).orElse(emptyFix())) .build()) .orElse(Description.NO_MATCH); } @@ -140,18 +136,18 @@ && isSubtype(getType(receiver), type, state)) { // If both are subtypes of Iterable, rewrite Type iterableType = state.getSymtab().iterableType; - Type multimapType = COM_GOOGLE_COMMON_COLLECT_MULTIMAP.get(state); + Type multimapType = MULTIMAP.get(state); Optional fix = - firstPresent( - generateTruthFix.apply(iterableType, "containsExactlyElementsIn"), - generateTruthFix.apply(multimapType, "containsExactlyEntriesIn")); + generateTruthFix + .apply(iterableType, "containsExactlyElementsIn") + .or(() -> generateTruthFix.apply(multimapType, "containsExactlyEntriesIn")); if (fix.isPresent()) { return fix; } } // Generate fix for CharSequence - Type charSequenceType = JAVA_LANG_CHARSEQUENCE.get(state); + Type charSequenceType = CHAR_SEQUENCE.get(state); BiFunction> generateCharSequenceFix = (maybeCharSequence, maybeString) -> { if (charSequenceType != null @@ -161,23 +157,14 @@ && isSameType(getType(maybeString), state.getSymtab().stringType, state)) { } return Optional.empty(); }; - return firstPresent( - generateCharSequenceFix.apply(receiver, argument), - generateCharSequenceFix.apply(argument, receiver)); + return generateCharSequenceFix + .apply(receiver, argument) + .or(() -> generateCharSequenceFix.apply(argument, receiver)); } - private static Optional firstPresent(Optional... optionals) { - for (Optional optional : optionals) { - if (optional.isPresent()) { - return optional; - } - } - return Optional.empty(); - } - - private static final Supplier COM_GOOGLE_COMMON_COLLECT_MULTIMAP = + private static final Supplier MULTIMAP = VisitorState.memoize(state -> state.getTypeFromString("com.google.common.collect.Multimap")); - private static final Supplier JAVA_LANG_CHARSEQUENCE = + private static final Supplier CHAR_SEQUENCE = VisitorState.memoize(state -> state.getTypeFromString("java.lang.CharSequence")); } 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 5745ff18e4f..cffeb83f7ca 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UndefinedEqualsTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UndefinedEqualsTest.java @@ -44,7 +44,7 @@ public void positiveInstanceEquals() { class Test { void f(Queue a, Queue b) { - // BUG: Diagnostic contains: Subtypes of Queue are not guaranteed + // BUG: Diagnostic contains: Queue does not have well-defined equality semantics a.equals(b); } } @@ -514,4 +514,40 @@ void f(CharSequence a, String b) { """) .doTest(); } + + @Test + public void futures() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.common.util.concurrent.ListenableFuture; + import com.google.common.util.concurrent.SettableFuture; + import java.util.concurrent.CompletableFuture; + import java.util.concurrent.Future; + + class Test { + void listenableFuture(ListenableFuture a, ListenableFuture b) { + // BUG: Diagnostic contains: Future does not have well-defined equality semantics + a.equals(b); + } + + void settableFuture(SettableFuture a, SettableFuture b) { + // BUG: Diagnostic contains: Future does not have well-defined equality semantics + a.equals(b); + } + + void completableFuture(CompletableFuture a, CompletableFuture b) { + // BUG: Diagnostic contains: Future does not have well-defined equality semantics + a.equals(b); + } + + void future(Future a, Future b) { + // BUG: Diagnostic contains: Future does not have well-defined equality semantics + a.equals(b); + } + } + """) + .doTest(); + } }