Better encapsulation for AnnotatedType
wdietl at gmail.com
Sat Oct 19 19:49:27 PDT 2013
Hi Jon, Joel, all,
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
> 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
Do we still want to perform the TypeMetadata refactoring?
> Remove comment and commented Assert.
Done. Comments are bad.
> Do you really need to go twice to the well, in
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.
> 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.
> 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:
>> 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.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.
More information about the type-annotations-dev