Creating a clone of a javac.code.Type

[Werner Dietl]

Jon, all,

I just pushed a set of major changes, in particular:

http://hg.openjdk.java.net/type-annotations/type-annotations/langtools/rev/9ab31827865c
http://hg.openjdk.java.net/type-annotations/type-annotations/langtools/rev/dc343861534d
http://hg.openjdk.java.net/type-annotations/type-annotations/langtools/rev/424b7015ffcf

I introduce a new AnnotatedType and copy Types instead of directly
modifying them.

This should solve the aliasing problem observed in javadoc and
elsewhere. Please confirm.

I currently see 5 failing tests that I'm having trouble tracking down.
Support appreciated.

Please let me know what you think of this significant change and
whether this is the direction you wanted things to go.

All the best,
cu, WMD.

On Fri, Dec 28, 2012 at 2:35 AM, Jonathan Gibbons
<jonathan.gibbons at oracle.com> wrote:
> On 12/28/2012 08:35 AM, Werner Dietl wrote:
>>
>> Jon, Joel, all,
>>
>> Steve noticed that CheckExamples was misbehaving on a test case. I
>> investigated this and found that this is related to the problem
>> previously raised by Bhavesh: there is unwanted sharing between Type
>> instances and having a type annotation on one Type results in type
>> annotations on others.
>>
>> The CheckExamples test that failed was CantAnnotateStaticClass, which
>> turns out to be the only test case that was using Types for it's
>> logic.
>> Recent refactorings initialize the type annotations later and this
>> test case failed.
>> I moved the corresponding check and added an extra test case that more
>> directly highlights this issue.
>>
>> However, I don't know what the best solution to the sharing issue is.
>>
>> In previous discussions (sorry, before the mailing list existed;
>> thread "JSR 308 patch discussion: cloning of Type" from September 15,
>> 2011) we came to the conclusion that cloning Type instances was bad
>> and removed the feature:
>>
>>
>> http://code.google.com/p/jsr308-langtools/source/detail?r=48559298cf51c1a0143b25509ae04a4cd0b640ac
>>
>> At that point we didn't have the CantAnnotateStaticClass test case and
>> no JavaDoc tests - therefore, all jtreg tests passed. Also, the
>> Checker Framework doesn't rely on Types and therefore also continued
>> to pass.
>>
>> However, removing cloning was not correct: Type.typeAnnotations is set in
>>
>> com.sun.tools.javac.code.TypeAnnotations.TypeAnnotationPositions.typeWithAnnotations(JCTree,
>> Type, List<TypeCompound>)
>> once the ambiguity between declaration and type annotations has been
>> resolved.
>> At that point, Type instances are being shared and setting field
>> Type.typeAnnotations modifies all instances, showing the effect
>> Bhavesh noticed.
>>
>> The simplest and ugliest solution would be to add cloning again.
>>
>> I tried working on a nicer solution: introduce a class
>> javac.code.Type.AnnotatedType that behaves like its underlying type
>> and also manages a list of type annotations.
>> I also added javax.lang.model.type.AnnotatedType.
>> After fixing a few problems, this mostly works again. However, I still
>> need a way to create a duplicate of an existing Type in
>> typeWithAnnotations, in order to replace a Type with an AnnotatedType.
>> The complication is that the AnnotatedType can be an enclosing type
>> for another type and we therefore need to duplicate the whole Type.
>>
>> Is there a simple way to create a duplicate of an arbitrary
>> javac.code.Type instance?
>> I looked at the existing Type.Mapping and Type.Visitor subclasses, but
>> didn't find something suitable.
>>
>> Which approach would you prefer, re-introducing cloning of Type or
>> spending more time on introducing AnnotatedType and a clean copy
>> mechanism?
>>
>> Joel, did you already start to work on javax.lang.model? Will we
>> expose an AnnotatedType there?
>>
>> All comments welcome,
>> cu, WMD.
>>
> The code previously in javac to do cloning was somewhat suspect
> and was removed because it threw up "red" FindBugs errors. The
> use at that time was sufficiently minimal that it was easy to achieve
> the same effect explicitly.
>
> The precedent for this type of situation would appear to be constant
> values.  A type can have a constant value associated with it, accessed
> through a virtual method.  Subtypes are created in the places where
> a constant value is present and needs to be modeled.  Unfortunately,
> since that solution is used for constant values, we cannot easily reuse
> the idea for annotations, unless we find a way to use subtypes for
> constants and/or annotations.
>
> I don't want to see cloning come back in, especially if it brings back
> FindBugs issues. I think that having a limited copy mechanism sounds
> better. But we should also avoid the need to copy as much as possible
> by keeping Type objects immutable, such that we can share them when
> possible.
>
> -- Jon



-- 
http://www.google.com/profiles/wdietl