Better encapsulation for AnnotatedType

[Jonathan Gibbons]

On 09/15/2013 12:53 AM, 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.
>
> 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.


Werner,

I'm limiting this email to a review of the changeset you provided. I'll 
answer your other points separately.

Basically very nice. Previously, I'd taken a stab at the easy bits of 
this, but you've managed the type rewriting bits in Attr as well. Thank you.

I have created https://bugs.openjdk.java.net/browse/JDK-8025109 to track 
this work.

I would prefer to work with you to make this change directly to 
tl/langtools.   Right now, the patch from your changeset does not apply 
cleanly to tl/langtools, so the patch will need to be updated.

There are still some explicit references to Type.AnnotatedType which 
need to go away for the encapsulation to be complete.  The first (maybe 
only?) one I've come across is in 
src/share/classes/com/sun/tools/javac/code/TypeAnnotations.java line 
378.  I would expect to see that all casts to Type.AnnotatedTYpe can go 
away, and that declarations can use Type instead of 
Type.AnnotatedType.   It is OK, for now, to see 
visitAnnotatedType(AnnotatedType, ...) methods.  They should go away in 
the next phase to use TypeMetadata.

src/share/classes/com/sun/tools/javac/comp/Attr.java:4060
Remove comment and commented Assert.

src/share/classes/com/sun/tools/javac/comp/Lower.java:2803
Do you really need to go twice to the well, in
   tree.underlyingType.type.unannotatedType()

test/tools/javac/annotations/typeAnnotations/failures/LintCast.out
Do you really need to modify this file?  In general, in a refactoring 
such as this, it is a good indication of success that the tests remain 
unmodified.  Unless you believe this tests was previously wrong, it 
would be better if the test were to remain unchanged.

test/tools/javac/lib/DPrinter.java
The changes here are not wrong, but I think they are unnecessary because 
the context is in a visitAnnotatedType method.  If you revert this file 
and fix the code to the change to LintCast.out is unnecessary, then 
you'll have a clean src/** only refactoring.

> 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.

As I indicated in an earlier email, I think BasicAnnoTests.java is a 
valid test, and that javac code needs to be updated so that it continues 
to pass.  All type annotations that can be accessed via javax.lang.model 
API need to be available at annotation processing time.  We might want 
to have a new test, MoreAnnoTests.java, that tests type annotations 
within a method body, but I agree that should be written to run after FLOW.

-- Jon