Refactor ReferenceType

[ftomassetti]

Refactor ReferenceType to do one thing (contains annotations) instead of two (containg annotations and array modifiers).

This is a follow-up of #187

[matozoid]

@ftomassetti - how would you suggest to refactor this? Is this what's in the patch @ptitjes did somewhere else that we cannot see without breaking the law? ;)

[ptitjes]

@matozoid Well, I think having respect for someone's beliefs (and work) should not be felt by anyone sane as adhering to the law...

[matozoid]

It's about licenses - that's the law. Also, smiley :)

[matozoid]

Calling mr. @ftomassetti! Mr. @ftomassetti are you there?

[ftomassetti]

Yes, but it is less than a year that I am thinking about this subject. It is better to not rush this thing too much :)

I think that we should move arrayCount out of ReferenceType to ArrayType

Now, if I am correct, ReferenceType specifies:

• the name of a reference type
• the annotations to that reference type
• the number of dimensions of the array (if any)
• the annotations associated to each specific dimension

About the last point:

In any type context where a type is used, it is possible to annotate the keyword denoting a primitive type or the Identifier denoting the simple name of a reference type. It is also possible to annotate an array type by writing an annotation to the left of the [ at the desired level of nesting in the array type. Annotations in these locations are called type annotations, and are specified in §9.7.4. Here are some examples:
@foo int[] f; annotates the primitive type int
int @foo [] f; annotates the array type int[]
int @foo [][] f; annotates the array type int[][]
int[] @foo [] f; annotates the array type int[] which is the component type of the array type int[][]

(see http://docs.oracle.com/javase/specs/jls/se8/html/jls-4.html#jls-4.3)

Currently we have this field:

private List<List<AnnotationExpr>> arraysAnnotations;

So that for example for:

@a MyClass @b [] @c @d[] f;

• the annotation a will not be in arraysAnnotations
• arraysAnnotations.get(0) should contain just the annotation b
• arraysAnnotations.get(1) should contain just the annotation c and d

To me it seems that this class is doing too much. If we instead move the array decoration to a different class it would means that the annotations associated to that array dimension could stay in that other class, which is a much simpler solution of what we have now.

For the same example we would have a class ArrayType containing just its own annotations (c and d). It would wrap another ArrayType with the annotations b and it would wrap the ReferenceType that at this point would care just about the annotation a.

[matozoid]

Now that's a description :) It looks a lot like the suggestion in #112 - is that correct?

[ftomassetti]

Yes, it is definitely similar. I just do not know if ArrayType should extend ReferenceType and if ArrayType should wrap a Type or just a ReferenceType (probably yes, I just do not have enough coffee in me to confirm this).

[ptitjes]

Also you should take care about the order of the array dimensions w.r.t. to their annotations. It seems to me that the wrapping you describe in your example is incorrect. See JLS8 section 9.7.4.

[matozoid]

I looked up all places where the grammar has the [ character and reverse engineered what's being produced there:

int[][] a = new int[1][2], b[][][] = new int[1][2][3][4][5];
^       ^   ^              ^         ^
|       |   |              |         +-ArrayCreationExpr(arrayCount=0, type=PrimitiveType(int), dimensions=[1,2,3,4,5])
|       |   |              +-VariableDeclaratorId(arrayCount=3)
|       |   +-ArrayCreationExpr(arrayCount=0, type=PrimitiveType(int), dimensions=[1,2])
|       +-VariableDeclaratorId(arrayCount=0)
+-ReferenceType(arrayCount=2, PrimitiveType(int))


a[3][4];
^
+-ArrayAccessExpr(4, ArrayAccessExpr(3, NameExpr("a")))

void x()[]{}
^
+-MethodDeclaration(arrayCount=1)

• Bug: ArrayCreationExpr.arrayCount always stays 0, no matter what. (ArrayCreationExpr.arrayCount always stays 0 #457)
• Bug: support for illegal syntax: array brackets after a method parameter list. (Or am I missing something here?) (Remove support for illegal syntax: array brackets after a method parameter list. #458)
• ArrayAccessExpr seems to support the wanted way of dealing with arrays already: wrap an ArrayX object for every array "depth". Good.
• ArrayCreationExpr could be changed to apply only to one depth: ArrayCreationExpr(2, ArrayCreationExpr(1, PrimitiveType(int)))
• VariableDeclarationExpr.type is probably what we're talking about all the time: instead of ReferenceType(arrayCount=2, PrimitiveType(int)) we want ArrayType(ArrayType(PrimitiveType(int)))

Is all of this correct?

[matozoid]

Or should that be...
ArrayCreationExpr(ArrayDimension(2, ArrayDimension(1, PrimitiveType(int))) ...?

[matozoid]

Okay, that is step one.

[matozoid]

I think this should be solved in a fundamentally different way. The main issue is that in int[] a[], int[] is not a type of interest to anyone, it's just a bit of syntax describing part of a type. The syntax is of use if we want to output this code. But when we want to use the type of a, we need to infer it from its parts: int[] + [].

Therefore I propose to...

• use a simple list of brackets and their annotations where they can be used (int[] a[]; int[] a()[];)
• store the element type in a field
• add get(Complete)Type where we have access to all necessary information, and have it construct the type (with ArrayType(...) ) dynamically.

(array creation is a fundamentally different construct and not part of this.)

Pro: this is the most consistent way to do this that I can think of. Therefore it is the easiest to explain. I can't explain int[] a[] turning into ArrayType(int) a(1 bracket pair)
Con: the type is dynamically constructed which will impact performance. It can be cached, but "thanks" to our love of mutability, we will have to do dirty checking. My suggestion is that users cache it, in the data field for example.

[matozoid]

This would affect...

• List<ArrayBracketPair>: some construct has to be added to store array braces. This would be the simplest one.
• VariableDeclarationExpr: no longer has type, but it gets getElementType(). Also stores a List<ArrayBracketPair>
• VariableDeclaratorId: replace arrayCount with List<ArrayBracketPair>. Add a getType() that take type information from its parents and constructs a complete type.
• MethodDeclaration: replace arrayCount with two List<ArrayBracketPair>'s: one for braces on the type, one for braces after the method parameter list. Add getElementType() and make getType() use getElementType() and the two braces lists to construct the complete type.
• Parameter: gets the same pattern: two fields for the braces, the element type, and the getter to construct the actual type.
• FieldDeclaration: same pattern as VariableDeclarationExpr

[matozoid]

"int arrayCount" is no longer the way to store information about arrays. It has been replaced by two things:

1. in most cases the type will be wrapped in an ArrayType for each level of array brackets, or: int[][] is stored as ArrayType(ArrayType(PrimitiveType()))
2. in other cases we can put these brackets in two positions, as seen below. Here we have chosen to store the brackets in the AST in the node that contains them. Since that means that they actual type is spread over two nodes, we have added getType() methods that collect all information and give you the correct type as in 1.

This gives us the following API:

• getType() always gives you a type with the correct amount of ArrayTypes wrapped around it.
• getElementType() always gives you a type WITHOUT ArrayTypes around it.
• getBracketsOn...() give you a list of ArrayBracketPairs that are found on that node.

Here is an overview:

int[] a, b[], c;

- VariableDeclarationExpr
    - Type elementType
    - List<ArrayBracketPair> bracketsOnElement
    - List<VariableDeclarator> vars
        - Type getType() { makeArrayType(elementType, bracketsOnElement, bracketsOnId) }
        - VariableDeclaratorId id
            - List<ArrayBracketPair> bracketsOnId

class X {
    int[] a, b[], c;
}

- FieldDeclaration
    - Type elementType
    - List<ArrayBracketPair> bracketsOnElement
    - List<VariableDeclarator> variables
        - Type getType() { makeArrayType(elementType, bracketsOnElement, bracketsOnId) }
        - VariableDeclaratorId id
            - List<ArrayBracketPair> bracketsOnId

class X {
    int[] a()[] {
         ...
    }
}

- MethodDeclaration
    - Type elementType
    - List<ArrayBracketPair> bracketsOnElement
    - List<ArrayBracketPair> bracketsAfterParameters
    - Type getType() { makeArrayType(elementType, bracketsOnElement, bracketsAfterParameters) }

void a(int[] b[]) {
    ...
}

- Parameter
    - Type elementType
    - List<ArrayBracketPair> bracketsOnElement
    - VariableDeclaratorId id
        - List<ArrayBracketPair> bracketsOnId
    - Type getType() { makeArrayType(elementType, bracketsOnElement, bracketsOnId) }

Also...

• ReferenceType is no longer a stand-alone class. It has become a base class of types that are reference types, which makes a lot of sense. If you use new ReferenceType(new SomeType()) then you should be able to replace that with just new SomeType()