Add support for @Inherited annotations

This is a fix for https://code.google.com/p/error-prone/issues/detail?id=185

I'm afraid we can't fix annotations(); this is because it runs its submatcher over all AnnotationTrees attached to the given Tree. To take superclasses into account, it would need to access their Trees, and this seems impossible (compiler may not even have access to superclass' source code).

We can, however, access inherited annotations as Attribute.Compound - "compiled" form, to run some less general matchers. I fixed hasAnnotation() this way. If needed, I could also add something a bit more general like:
hasAnnotationWithArgument(String annotationClass, String argumentName, String value);

[radoslaw.burny, 2013-12-01 18:07:17]

Changing reviewers since Eddie's OOO.

[supertri, 2013-12-04 17:56:26]

cushon,

Any comments on this CL?

https://codereview.appspot.com/30940043/diff/20001/core/src/main/java/com/goo...
File core/src/main/java/com/google/errorprone/matchers/Matchers.java (right):

https://codereview.appspot.com/30940043/diff/20001/core/src/main/java/com/goo...
core/src/main/java/com/google/errorprone/matchers/Matchers.java:526: } else if
((sym instanceof ClassSymbol) && (annotationSym.attribute(inheritedSym) !=
null)) {
> 100 chars.

Also, no need to make this an else if since the first block returns.

[cushon, 2013-12-04 18:19:41]

This looks good to me.

The CL description say we can run 'less general' matchers on the annotation
symbols. I'm curious what the limitations are when we don't have access to the
annotation syntax?

https://codereview.appspot.com/30940043/diff/20001/core/src/main/java/com/goo...
File core/src/main/java/com/google/errorprone/matchers/Matchers.java (right):

https://codereview.appspot.com/30940043/diff/20001/core/src/main/java/com/goo...
core/src/main/java/com/google/errorprone/matchers/Matchers.java:522: Symbol
inheritedSym = state.getSymbolFromString("java.lang.annotation.Inherited");
This type is already in the symbol table, so you could just use:

state.getSymtab().inheritedType

[radoslaw.burny, 2013-12-04 23:52:58]

> The CL description say we can run 'less general' matchers on the annotation
> symbols. I'm curious what the limitations are when we don't have access to the
> annotation syntax?

There are no inherent limitations - Attribute.Compound carries all the necessary
information from original AnnotationTree (annotation type & symbol, arguments
and values).
But I hit a design problem - to make annotations() fully general and
inheritance-respecting, we'd need to change its submatcher type to
Matcher<Attribute.Compound> (since AttributeCompound is always available, while
AnnotationTree isn't for inherited annotations). But our matchers only match
Tree subclasses. Hence, matchers of this new type would have to be written from
scratch; they would also operate on different program representation than rest
of error-prone uses. Sounds like a bad idea.

I decided that it makes more sense to provide "less general" matchers for most
used cases (like hasAnnotations()), abstracting aforementioned problems away.
For some really specific cases, one still can write a custom matcher.

[radoslaw.burny, 2013-12-04 23:53:21]

https://codereview.appspot.com/30940043/diff/20001/core/src/main/java/com/goo...
File core/src/main/java/com/google/errorprone/matchers/Matchers.java (right):

https://codereview.appspot.com/30940043/diff/20001/core/src/main/java/com/goo...
core/src/main/java/com/google/errorprone/matchers/Matchers.java:522: Symbol
inheritedSym = state.getSymbolFromString("java.lang.annotation.Inherited");
On 2013/12/04 18:19:42, cushon wrote:
> This type is already in the symbol table, so you could just use:
> 
> state.getSymtab().inheritedType

Done.

https://codereview.appspot.com/30940043/diff/20001/core/src/main/java/com/goo...
core/src/main/java/com/google/errorprone/matchers/Matchers.java:526: } else if
((sym instanceof ClassSymbol) && (annotationSym.attribute(inheritedSym) !=
null)) {
On 2013/12/04 17:56:26, supertri wrote:
> > 100 chars.
> 
> Also, no need to make this an else if since the first block returns.

Done.

[supertri, 2013-12-05 02:22:54]

LGTM

[cushon, 2013-12-05 04:52:26]

On 2013/12/04 23:52:58, radoslaw.burny wrote:
> > The CL description say we can run 'less general' matchers on the annotation
> > symbols. I'm curious what the limitations are when we don't have access to
the
> > annotation syntax?
> 
> There are no inherent limitations - Attribute.Compound carries all the
necessary
> information from original AnnotationTree (annotation type & symbol, arguments
> and values).
> But I hit a design problem - to make annotations() fully general and
> inheritance-respecting, we'd need to change its submatcher type to
> Matcher<Attribute.Compound> (since AttributeCompound is always available,
while
> AnnotationTree isn't for inherited annotations). But our matchers only match
> Tree subclasses. Hence, matchers of this new type would have to be written
from
> scratch; they would also operate on different program representation than rest
> of error-prone uses. Sounds like a bad idea.
> 
> I decided that it makes more sense to provide "less general" matchers for most
> used cases (like hasAnnotations()), abstracting aforementioned problems away.
> For some really specific cases, one still can write a custom matcher.

That makes sense, thanks for the explanation. We can always add a more general
way to match on inherited annotations when/if we need it.

LGTM

[radoslaw.burny, 2013-12-05 10:53:15]

Here's the patch.

On Thu, Dec 5, 2013 at 5:52 AM,  <cushon@google.com> wrote:
> On 2013/12/04 23:52:58, radoslaw.burny wrote:
>>
>> > The CL description say we can run 'less general' matchers on the
>
> annotation
>>
>> > symbols. I'm curious what the limitations are when we don't have
>
> access to the
>>
>> > annotation syntax?
>
>
>> There are no inherent limitations - Attribute.Compound carries all the
>
> necessary
>>
>> information from original AnnotationTree (annotation type & symbol,
>
> arguments
>>
>> and values).
>> But I hit a design problem - to make annotations() fully general and
>> inheritance-respecting, we'd need to change its submatcher type to
>> Matcher<Attribute.Compound> (since AttributeCompound is always
>
> available, while
>>
>> AnnotationTree isn't for inherited annotations). But our matchers only
>
> match
>>
>> Tree subclasses. Hence, matchers of this new type would have to be
>
> written from
>>
>> scratch; they would also operate on different program representation
>
> than rest
>>
>> of error-prone uses. Sounds like a bad idea.
>
>
>> I decided that it makes more sense to provide "less general" matchers
>
> for most
>>
>> used cases (like hasAnnotations()), abstracting aforementioned
>
> problems away.
>>
>> For some really specific cases, one still can write a custom matcher.
>
>
> That makes sense, thanks for the explanation. We can always add a more
> general way to match on inherited annotations when/if we need it.
>
> LGTM
>
> https://codereview.appspot.com/30940043/



-- 
Radoslaw Burny

[cushon, 2013-12-11 18:48:22]

committed as aadcce35324ddb3fc9118f1585cc7fa766a1d31c