Better encapsulation for AnnotatedType

[Jonathan Gibbons]

I have done the necessary.

-- Jon

On 10/19/2013 09:36 PM, Michel Trudeau wrote:
> 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.
>
>