Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BasicAnnotationProcessor does not recursively validate superclasses / superinterfaces #660

Open
Leland-Takamine opened this issue Aug 22, 2018 · 10 comments · May be fixed by #733
Open

BasicAnnotationProcessor does not recursively validate superclasses / superinterfaces #660

Leland-Takamine opened this issue Aug 22, 2018 · 10 comments · May be fixed by #733
Labels

Comments

@Leland-Takamine
Copy link

Leland-Takamine commented Aug 22, 2018

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
Copy link
Contributor

I'm not positive, but I think here:

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
Copy link
Author

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
Copy link
Contributor

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

@Leland-Takamine
Copy link
Author

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
Copy link
Contributor

ronshapiro commented Nov 1, 2018

@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
Copy link
Contributor

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

@eamonnmcmanus
Copy link
Member

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
Copy link
Author

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

@Leland-Takamine
Copy link
Author

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

@TonyTangAndroid
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants