Better encapsulation for AnnotatedType

[Michel Trudeau]

Thank you Werner for fixing: https://bugs.openjdk.java.net/browse/JDK-8025109

Next, it needs to be reviewed and if ok'ed, then pushed to TL and closed.

Michel

On Oct 19, 2013, at 7:49 PM, Werner Dietl <wdietl at gmail.com> wrote:

Hi Jon, Joel, all,

I pushed

http://hg.openjdk.java.net/type-annotations/type-annotations/langtools/rev/6d036299f2af

with the comments discussed in this thread.


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

Done. The only uses of Type.AnnotatedType are now in the
visitAnnotatedType methods.
Do we still want to perform the TypeMetadata refactoring?


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

Done. Comments are bad.


> 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()

I think we do. Consider the type "@TA T" where T is a type variable with
upper bound "@TB Object".
The erasure of the underlying type will give "@TB Object".
The correct erasure is to take the unannotated erasure of the underlying
type and add the type annotations from the type variable, giving "@TA
Object" as result.
Would you expect something else? I can imagine "@TA @TB Object" and "@TB
Object" as alternatives. The source contains a @TA, so I found "@TA
Object" the most desirable.


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

We had discussed this earlier and this change is an improvement.


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

The fields that were previously accessed in DPrinter are now private, so
the old code wouldn't compile.


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

This has been fixed by Eric's refactorings.

Unrelated to my changes today, the order of errors in
CantAnnotateStaticClass[23].out changed - I assume because of tl changes
to when annotations are processed.
As it's just the order of errors, I updated the expected output.

In type-annotations, all jtreg tests in tools/javac/annotations/ run
cleanly for me.

All comments welcome.

Cheers,
cu, WMD.