Fix a `StackOverflowError` in `getLocalAndInheritedMethods`, involving recursive type bounds.

Also optimize override detection by noting that one method cannot override another if they have a different number of arguments.

RELNOTES=`MoreElements.getLocalAndInheritedMethods` no longer gets a stack overflow in certain circumstances involving recursive type bounds.
PiperOrigin-RevId: 542907665

3 files changed, 45 insertions, 19 deletions

diff --git a/common/src/main/java/com/google/auto/common/Overrides.java b/common/src/main/java/com/google/auto/common/Overrides.java
index 46d765bc..6f4c34fc 100644
--- a/common/src/main/java/com/google/auto/common/Overrides.java
+++ b/common/src/main/java/com/google/auto/common/Overrides.java
@@ -22,8 +22,10 @@ import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import javax.lang.model.element.Element;
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.ExecutableElement;
@@ -44,12 +46,12 @@ import javax.lang.model.util.Types;
 import org.checkerframework.checker.nullness.qual.Nullable;
 
 /**
- * Determines if one method overrides another. This class defines two ways of doing that:
- * {@code NativeOverrides} uses the method
- * {@link Elements#overrides(ExecutableElement, ExecutableElement, TypeElement)} while
- * {@code ExplicitOverrides} reimplements that method in a way that is more consistent between
- * compilers, in particular between javac and ecj (the Eclipse compiler).
+ * Determines if one method overrides another. This class defines two ways of doing that: {@code
+ * NativeOverrides} uses the method {@link Elements#overrides(ExecutableElement, ExecutableElement,
+ * TypeElement)} while {@code ExplicitOverrides} reimplements that method in a way that is more
+ * consistent between compilers, in particular between javac and ecj (the Eclipse compiler).
  *
+ * @see <a href="https://github.com/google/auto/issues/372">AutoValue issue about Eclipse</a>
  * @author emcmanus@google.com (Éamonn McManus)
  */
 abstract class Overrides {
@@ -101,6 +103,14 @@ abstract class Overrides {
         // Static methods can't be overridden (though they can be hidden by other static methods).
         return false;
       }
+      if (overrider.getParameters().size() != overridden.getParameters().size()) {
+        // One method can't override another if they have a different number of parameters.
+        // Varargs `Foo...` appears as `Foo[]` here; there is a separate
+        // ExecutableElement.isVarArgs() method to tell whether a method is varargs, but that has no
+        // effect on override logic.
+        // The check here isn't strictly needed but avoids unnecessary work.
+        return false;
+      }
       Visibility overriddenVisibility = Visibility.ofElement(overridden);
       Visibility overriderVisibility = Visibility.ofElement(overrider);
       if (overriddenVisibility.equals(Visibility.PRIVATE)
@@ -252,6 +262,13 @@ abstract class Overrides {
        */
       private final Map<TypeParameterElement, TypeMirror> typeBindings = Maps.newLinkedHashMap();
 
+      /**
+       * Type elements that we are currently visiting. This helps us stay out of trouble when
+       * looking at something like {@code Enum<E extends Enum<E>>}. At the second {@code Enum} we
+       * will just return raw {@code Enum}.
+       */
+      private final Set<TypeElement> visitingTypes = new LinkedHashSet<>();
+
       @Nullable
       ImmutableList<TypeMirror> erasedParameterTypes(ExecutableElement method, TypeElement in) {
         if (method.getEnclosingElement().equals(in)) {
@@ -313,11 +330,18 @@ abstract class Overrides {
         if (t.getTypeArguments().isEmpty()) {
           return t;
         }
+        TypeElement typeElement = asTypeElement(t);
+        if (!visitingTypes.add(typeElement)) {
+          return typeUtils.erasure(t);
+        }
         List<TypeMirror> newArgs = Lists.newArrayList();
         for (TypeMirror arg : t.getTypeArguments()) {
           newArgs.add(visit(arg));
         }
-        return typeUtils.getDeclaredType(asTypeElement(t), newArgs.toArray(new TypeMirror[0]));
+        TypeMirror result =
+            typeUtils.getDeclaredType(asTypeElement(t), newArgs.toArray(new TypeMirror[0]));
+        visitingTypes.remove(typeElement);
+        return result;
       }
 
       @Override
diff --git a/common/src/test/java/com/google/auto/common/MoreElementsTest.java b/common/src/test/java/com/google/auto/common/MoreElementsTest.java
index d2617dc9..d074d221 100644
--- a/common/src/test/java/com/google/auto/common/MoreElementsTest.java
+++ b/common/src/test/java/com/google/auto/common/MoreElementsTest.java
@@ -20,7 +20,6 @@ import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 import static java.util.Objects.requireNonNull;
 import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -366,12 +365,15 @@ public class MoreElementsTest {
     Types types = compilation.getTypes();
     TypeElement builderElement =
         elements.getTypeElement(FakeProto.Builder.class.getCanonicalName());
-    // TODO: b/287060583 - this should not trigger infinite recursion
-    assertThrows(
-        StackOverflowError.class,
-        () -> {
-          Object unused = MoreElements.getLocalAndInheritedMethods(builderElement, types, elements);
-        });
+    TypeMirror abstractMessageLiteMirror =
+        elements.getTypeElement(AbstractMessageLite.class.getCanonicalName()).asType();
+    ExecutableElement internalMergeFromMethod =
+        getMethod(FakeProto.Builder.class, "internalMergeFrom", abstractMessageLiteMirror);
+
+    ImmutableSet<ExecutableElement> methods =
+        MoreElements.getLocalAndInheritedMethods(builderElement, types, elements);
+
+    assertThat(methods).contains(internalMergeFromMethod);
   }
 
   // The classes that follow mimic the proto classes that triggered the bug that
@@ -486,7 +488,7 @@ public class MoreElementsTest {
         for (int i = 0; i < parameterTypes.length; i++) {
           TypeMirror expectedType = parameterTypes[i];
           TypeMirror actualType = method.getParameters().get(i).asType();
-          match &= types.isSameType(expectedType, actualType);
+          match &= types.isSameType(types.erasure(expectedType), types.erasure(actualType));
         }
         if (match) {
           assertThat(found).isNull();
diff --git a/common/src/test/java/com/google/auto/common/OverridesTest.java b/common/src/test/java/com/google/auto/common/OverridesTest.java
index 8d77fc76..a69f0083 100644
--- a/common/src/test/java/com/google/auto/common/OverridesTest.java
+++ b/common/src/test/java/com/google/auto/common/OverridesTest.java
@@ -27,6 +27,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Range;
 import com.google.common.io.Files;
+import com.google.common.truth.Correspondence;
 import com.google.common.truth.Expect;
 import com.google.testing.compile.CompilationRule;
 import java.io.File;
@@ -574,11 +575,10 @@ public class OverridesTest {
   }
 
   private void assertTypeListsEqual(@Nullable List<TypeMirror> actual, List<TypeMirror> expected) {
-   requireNonNull(actual);
-   assertThat(actual).hasSize(expected.size());
-   for (int i = 0; i < actual.size(); i++) {
-      assertThat(typeUtils.isSameType(actual.get(i), expected.get(i))).isTrue();
-    }
+    assertThat(actual)
+      .comparingElementsUsing(Correspondence.from(typeUtils::isSameType, "is same type as"))
+      .containsExactlyElementsIn(expected)
+      .inOrder();
   }
 
   // TODO(emcmanus): replace this with something from compile-testing when that's available.