Better encapsulation for AnnotatedType

[Jonathan Gibbons]

Werner,

I still want to look at this code in detail, but my primary comment is 
that I believe BasicAnnoTests is valid as-is. Although it contains type 
annotations, they are all in positions on signatures (i.e. not in method 
bodies or variable initializer expressions) and so should be 
evaluated/valid/reflectable at the time that annotation processing is 
called.

-- Jon


On 09/18/2013 04:23 PM, Werner Dietl wrote:
> Hi Joel, all,
>
> thanks for the comments.
>
> I think the only tricky part in this refactoring was that in
> Attr.annotateType(...) instead of modifying the Type and adding the
> type annotations, I'm now setting the type of the tree to the new
> AnnotatedType:
>
> tree.type = tree.type.unannotatedType().annotatedType(compounds);
>
> This means that the tree type appears without type annotations until
> the annotate.typeAnnotation queue is emptied. This shouldn't make a
> difference, as in the old version one would have seen an AnnotatedType
> with empty annotations - which is also unexpected.
>
> Please do let me know whether I should look into TypeMetadata or
> whether one of you is performing that refactoring.
>
> Cheers,
> cu, WMD.
>
>
> On Wed, Sep 18, 2013 at 3:08 AM, Joel Borggren-Franck
> <joel.franck at oracle.com> wrote:
>> Hi Werner,
>>
>> In general I think this looks good.
>>
>> On 2013-09-15, Werner Dietl wrote:
>>> Jon, all,
>>>
>>> as you suggested, I encapsulated the typeAnnotations and underlyingType
>>> fields in com.sun.tools.javac.code.Type.AnnotatedType
>>>
>>> This actually made the code simpler in a few places.
>>> There is one new failure:
>>> langtools/test/tools/javac/processing/model/type/BasicAnnoTests.java
>>>
>>> However, I think the test case is wrong: it uses a "normal" annotation
>>> processor and therefore runs too early. It should be changed to be a
>>> "type annotation processor", i.e. it should run after FLOW.
>>> I suggest we create a subclass/alternative to
>>> JavacTestingAbstractProcessor and use that to test type annotations.
>> Annotation processing will occur before flow. Eventually we need to move
>> to a model were we trigger stuff that annotation processing depends on
>> before attr. Some type anno stuff needs to be done there, and some can
>> be delayed. For example there is this bug:
>> https://bugs.openjdk.java.net/browse/JDK-8014016 that is assigned to Jan
>> in the javac team.
>>
>> I'm not sure the concept of an after flow processor makes sense at all.
>>
>> However it seems a bit strange that your encapsulation introduced an
>> error like this.
>>
>>> I'm not sure whether this had an effect on javadoc - if so, we should
>>> revisit when the typeAnnotations queue is flushed.
>>>
>>> Please review this changeset:
>>>
>>> http://hg.openjdk.java.net/type-annotations/type-annotations/langtools/rev/6f595e036db7
>>>
>>> If this first step is OK, we can next introduce the TypeMetadata
>>> abstraction.
>>>
>>>
>>> Regarding SymbolMetadata, I find it a bit strange that the methods that
>>> access SymbolMetadata in Symbol are still called "xxxAnnotations", e.g.
>>> com.sun.tools.javac.code.Symbol.getAnnotations()
>>> Should these also be renamed?
>>>
>> I'm working towards removing those altogether (but probably in the 9
>> timeframe). There was I reason I didn't want Andreas to rename them, but
>> I can't recall it now :)
>>
>>> I have one more change that I just pushed:
>>>
>>> http://hg.openjdk.java.net/type-annotations/type-annotations/langtools/rev/e1772bcdf768
>>>
>>> I changed com.sun.tools.javac.tree.TreeCopier.visitLabeledStatement
>>> to take the copied body instead of the original body.
>>> Charlie Garrett discovered this problem after painful debugging of a
>>> Checker Framework problem that depended on whether a label is present or
>>> not.
>>> I cannot present a simple test case for the error, but the change
>>> doesn't break any existing test cases.
>>> Also note that this was the only "unused variable" warning in
>>> TreeCopier. Warnings are useful.
>>>
>> Seems reasonable.
>>
>> cheers
>> /Joel
>
>