BasicAnnotationProcessor does not recursively validate superclasses / superinterfaces

[Leland-Takamine]

BasicAnnotationProcessor only validates immediate superclass and superinterfaces. This commit demonstrates the behavior. The first test passes, but the second fails. It's possible to update SuperficialValidation to recursively validate superclasses and superinterfaces. Is this something that BasicAnnotationProcessor should support?

[ronshapiro]

I'm not positive, but I think here:

auto/common/src/main/java/com/google/auto/common/SuperficialValidation.java

Line 135 in 9cc04ec

return validateTypes(t.getTypeArguments());

If we also check validateElement(t.asElement()), I think that should do the full type-hierarchy checking you're looking for.

[Leland-Takamine]

That's what I was initially thinking as well. The problem is that this will result in a cycle since isValidBaseElement does the inverse:

private static boolean isValidBaseElement(Element e) {
    return validateType(e.asType())
        && validateAnnotations(e.getAnnotationMirrors())
        && validateElements(e.getEnclosedElements());
  }

[ronshapiro]

It's possible we need to pass some state around for previously visited elements/types. Or we can change isValidBaseElement?

[Leland-Takamine]

So to clarify some options:

1. We can add validateElement(t.asElement()) to TYPE_VALIDATING_VISITOR.visitDeclared to recursively validate superclasses, but this would introduce a cycle. To prevent an infinite loop, we can break the cycle by:
   a) Maintaining a set of visited elements. Sounds like it could work, but it would be a bit of refactoring.
   b) Removing validateType from isValidBaseElement. However, this doesn't actually break the cycle (ie: ClassA self() { return this; } would loop infinitely) so I don't think this would help.
2. Update ELEMENT_VALIDATING_VISITOR.visitType to call validateElement for each superclass and superinterface. This does not introduce a cycle like (1) since a class or interface cannot extend/implement itself.

[ronshapiro]

@eamonnmcmanus would love to get your thoughts on here. We just had another person report a similar issue in Dagger that appears to be a manifestation of this bug.

[ronshapiro]

@Leland-Takamine I tried option 2, but that is also stack-overflowing. Hrm.

[eamonnmcmanus]

Certainly the second approach (just iterate up the hierarchy) is easier to understand and less susceptible to infinite recursion. I would include a test where a class does extend itself, since we're talking about detecting incorrect code here. Probably such a class would never even reach the annotation processing stage, but I'm not sure.

It's possible for a class or interface to inherit from the same interface along more than one path, but it's probably not worth handling that specially since at worst we would just make the same checks more than once.

(Incidentally I'm not particularly expert on this code. AutoValue doesn't use it, because as I recall it doesn't interact well with the round-deferment we do in order to handle references to classes that have not yet been generated but will be.)

[Leland-Takamine]

@ronshapiro That's surprising - would you mind pushing up a branch with those changes?

[Leland-Takamine]

@eamonnmcmanus @ronshapiro Pushed a PR up - let me know what you think.

[TonyTangAndroid]

Is there any plan on how we are to proceed from here? Been keeping on an eye on this issue for a while now.