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 46d765bcdd..6f4c34fc4d 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.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 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 AutoValue issue about Eclipse * @author emcmanus@google.com (Éamonn McManus) */ abstract class Overrides { @@ -101,6 +103,14 @@ public boolean 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 @@ private class TypeSubstVisitor extends SimpleTypeVisitor8 { */ private final Map typeBindings = Maps.newLinkedHashMap(); + /** + * Type elements that we are currently visiting. This helps us stay out of trouble when + * looking at something like {@code Enum>}. At the second {@code Enum} we + * will just return raw {@code Enum}. + */ + private final Set visitingTypes = new LinkedHashSet<>(); + @Nullable ImmutableList erasedParameterTypes(ExecutableElement method, TypeElement in) { if (method.getEnclosingElement().equals(in)) { @@ -313,11 +330,18 @@ public TypeMirror visitDeclared(DeclaredType t, Void p) { if (t.getTypeArguments().isEmpty()) { return t; } + TypeElement typeElement = asTypeElement(t); + if (!visitingTypes.add(typeElement)) { + return typeUtils.erasure(t); + } List 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 d2617dc96d..d074d22126 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.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 void getLocalAndInheritedMethods_recursiveTypeVariableBound() { 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 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 @@ private ExecutableElement getMethod(Class c, String methodName, TypeMirror... 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 8d77fc7631..a69f0083d5 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.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 void methodFromSuperinterfaces() { } private void assertTypeListsEqual(@Nullable List actual, List 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.