Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 418782

Summary: [1.8][dom] ParameterizedType's 'type' property could be AnnotatableType instead of Type
Product: [Eclipse Project] JDT Reporter: Jay Arthanareeswaran <jarthana>
Component: CoreAssignee: Manoj N Palat <manoj.palat>
Status: RESOLVED WORKSFORME QA Contact:
Severity: normal    
Priority: P3 CC: markus.kell.r, srikanth_sankaran
Version: 4.4   
Target Milestone: BETA J8   
Hardware: PC   
OS: Windows 7   
Whiteboard:

Description Jay Arthanareeswaran CLA 2013-10-07 02:56:55 EDT
In an example like this:

@NonNull List<@NonNull List<@Nullable String>>

even though the 'type' property can only be an AnnotatableType, the clients need to cast it into AnnotatableType in order to access the annotations. Ideally, clients shouldn't have to use Type@isAnnotatable() or type cast.

The same can argued for the ParameterizedType#typeArguments. However, in this case, converting 'typeArguments' to AnnotatableType.class node has a little problem:

Currently ParameterizedType is not an AnnotatableType and hence we will have to make the former a subclass of latter (to cover the scenario where the type argument itself will be a ParameterizedType)

Similar code can be found in many places such as WildcardType and other nodes too. To make it consistent, we should make all such cases (where annotations are supported) to use AnnotatableType.

Interestingly, in the current model, the only node to specify AnnotatableType is MethodDeclaration#receiverType.
Comment 1 Srikanth Sankaran CLA 2013-10-07 03:05:08 EDT
Let us also explore whether the annotations on ParameterizedType should/could
be attached to itself instead of being held in the Type property.

i.e Should @NonNull List<@NonNull String> be viewed as:

@NonNull (List<@NonNull String>) or

(@NonNull List)<@NonNull String>

Compiler follows the former model, while DOM/AST encodes the latter.

Looking at the grammar changes for JSR308, I think we should go with the
former model:

Type:
[Annotations] UnannType

Such discrepancy could lead to issues if a client traverses to various parts
of the tree and queries the annotations and their resolved types.
Comment 2 Srikanth Sankaran CLA 2013-10-07 03:08:23 EDT
Per javadoc on Type, the following types are not AnnotatableType's:
ParameterizedType, UnionType, IntersectionType. The latter two should
stay that way, while we need to consider making the former Annotatable.
Comment 3 Srikanth Sankaran CLA 2013-10-07 07:55:05 EDT
(In reply to Jayaprakash Arthanareeswaran from comment #0)

> Currently ParameterizedType is not an AnnotatableType and hence we will have
> to make the former a subclass of latter (to cover the scenario where the
> type argument itself will be a ParameterizedType)

I noticed that ArrayType is not an AnnotatableType either. ArrayTypes can
be type arguments too.

In the saga of modelling arrays, not sure we considered making ArrayType
an AnnotatableType that stores *additional* dimensions in List<Dimension>
and let the base dimension be implicit in the ArrayType itself.
Comment 4 Srikanth Sankaran CLA 2013-10-15 12:01:08 EDT
At the expense of sounding like a broken record: The AnnotatableType abstraction
needs a relook. It can work/be made to work, I don't see a fundamental blocking
problem with that, but as was pointed out in

http://mail.openjdk.java.net/pipermail/type-annotations-spec-observers/2013-February/000092.html

whether or not a type is annotated is a "has-a" relationship and not an "is a"
relationship.

FWIW, This is the approach we have taken in the compiler. 

I'll also mention here that Core/reflection taken the opposite approach
and has come up with an hierarchy of types - AnnotatedType, AnnotatedArrayType,
AnnotatedParameterizedType etc.

AFAICT, AnnotatableType originated in https://bugs.eclipse.org/bugs/show_bug.cgi?id=391890#c7 due to concerns about Type#annotations() being available on
all types. Elsewhere concerns have been raised about making ParameterizedType's
annotatable because it offers multiple places to annotate.

I think these could be solved by documentation and throwing of exceptions.

Summary: If the client side is comfortable with these APIs as they are,
that is fine by us. I agree should voiced this much earlier - blame it on 
too much to do and too little time :(
Comment 5 Srikanth Sankaran CLA 2013-10-15 12:25:13 EDT
(In reply to Srikanth Sankaran from comment #4)

> AFAICT, AnnotatableType originated in
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=391890#c7 due to concerns
> about Type#annotations() being available on
> all types. 

Would it have worked if Type.annotations() simply returned an unmodifiable
empty list and the subtypes that have the annotations property returned
modifiable lists with actual, possibly empty list of annotations ?
Comment 6 Srikanth Sankaran CLA 2013-11-11 01:44:54 EST
Markus and I discussed this at length and he explained the rationale behind
why clients would prefer the current model. These observations are already
recorded though in a scattered form across different bugs. Basically, for
UI team the need to cast is less of an issue as they work with visitors and
being able to block annotatability in multiple places right at compile time
is a better approach for them.

So I will close this as WORKSFORME.