Better encapsulation for AnnotatedType
jonathan.gibbons at oracle.com
Wed Sep 18 16:58:04 PDT 2013
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
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
> 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.
> 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:
>>> 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:
>>> If this first step is OK, we can next introduce the TypeMetadata
>>> Regarding SymbolMetadata, I find it a bit strange that the methods that
>>> access SymbolMetadata in Symbol are still called "xxxAnnotations", e.g.
>>> 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:
>>> 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
>>> 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.
More information about the type-annotations-dev